On Tue, Mar 5, 2013 at 9:17 AM, Archit Taneja archit@ti.com wrote:

The omapdrm driver requires omapdss panel drivers to expose ops like detect, set_timings and check_timings. These can be NULL for fixed panel DPI, DBI, DSI and SDI drivers. At some places, there are no checks to see if the panel driver has these ops or not, and that leads to a crash.

The following things are done to make fixed panels work:

• The omap_connector's detect function is modified such that it considers panel types which are generally fixed panels as always connected(provided the panel driver doesn't have a detect op). Hence, the connector corresponding to these panels is always in a 'connected' state.

• If a panel driver doesn't have a check_timings op, assume that it supports the mode passed to omap_connector_mode_valid(the 'mode_valid' drm helper function)

• The function omap_encoder_update shouldn't really do anything for fixed resolution panels, make sure that it calls set_timings only if the panel driver has one.

Signed-off-by: Archit Taneja archit@ti.com

drivers/gpu/drm/omapdrm/omap_connector.c | 10 ++++++++-- drivers/gpu/drm/omapdrm/omap_encoder.c | 6 ++++-- 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c index c451c41..c952324 100644 --- a/drivers/gpu/drm/omapdrm/omap_connector.c +++ b/drivers/gpu/drm/omapdrm/omap_connector.c @@ -110,6 +110,11 @@ static enum drm_connector_status omap_connector_detect( ret = connector_status_connected; else ret = connector_status_disconnected;

•   } else if (dssdev->type == OMAP_DISPLAY_TYPE_DPI ||
  

•                   dssdev->type == OMAP_DISPLAY_TYPE_DBI ||
  

•                   dssdev->type == OMAP_DISPLAY_TYPE_SDI ||
  

•                   dssdev->type == OMAP_DISPLAY_TYPE_DSI) {
  

•           ret = connector_status_connected;
    } else {
            ret = connector_status_unknown;
    }
  

@@ -189,12 +194,13 @@ static int omap_connector_mode_valid(struct drm_connector *connector, struct omap_video_timings timings = {0}; struct drm_device *dev = connector->dev; struct drm_display_mode *new_mode;

•   int ret = MODE_BAD;
  

•   int r, ret = MODE_BAD;
  
    copy_timings_drm_to_omap(&timings, mode);
    mode->vrefresh = drm_mode_vrefresh(mode);
  
  

•   if (!dssdrv->check_timings(dssdev, &timings)) {
  

•   r = dssdrv->check_timings ? dssdrv->check_timings(dssdev, &timings) : 0;
  

•   if (!r) {
            /* check if vrefresh is still valid */
            new_mode = drm_mode_duplicate(dev, mode);
            new_mode->clock = timings.pixel_clock;
  

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c index d48df71..9aed178 100644 --- a/drivers/gpu/drm/omapdrm/omap_encoder.c +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c @@ -135,13 +135,15 @@ int omap_encoder_update(struct drm_encoder *encoder,

    dssdev->output->manager = mgr;

•   ret = dssdrv->check_timings(dssdev, timings);
  

•   ret = dssdrv->check_timings ?
  

•           dssdrv->check_timings(dssdev, timings) : 0;
    if (ret) {
            dev_err(dev->dev, "could not set timings: %d\n", ret);
            return ret;
    }
  
  

•   dssdrv->set_timings(dssdev, timings);
  

•   if (dssdrv->set_timings)
  

•           dssdrv->set_timings(dssdev, timings);
  

maybe either here or _mode_valid(), for panels that don't have set_timings(), we should double check that the new timings match what we get from the panel's get_timings(). Other than that, it looks fine.

BR, -R

    return 0;

}

1.7.10.4

On Monday 11 March 2013 05:58 PM, Tomi Valkeinen wrote:

On 2013-03-05 16:17, Archit Taneja wrote:

...

The support outputs struct for overlay managers is incorrect for OMAP4. Make these changes:

• DPI isn't supported via the LCD1 overlay manager, remove DPI as a supported output.
• the TV manager can suppport DPI, but the omapdss driver doesn't support that yet, we require some muxing at the DSS level, and we also need to configure the hdmi pll in the DPI driver so that the TV manager has a pixel clock. We don't support that yet.

Signed-off-by: Archit Taneja archit@ti.com

drivers/video/omap2/dss/dss_features.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c index d7d66ef..7f791ae 100644 --- a/drivers/video/omap2/dss/dss_features.c +++ b/drivers/video/omap2/dss/dss_features.c @@ -202,12 +202,10 @@ static const enum omap_dss_output_id omap3630_dss_supported_outputs[] = {

static const enum omap_dss_output_id omap4_dss_supported_outputs[] = { /* OMAP_DSS_CHANNEL_LCD */

• OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI |
• OMAP_DSS_OUTPUT_DSI1,

• OMAP_DSS_OUTPUT_DBI | OMAP_DSS_OUTPUT_DSI1,

  /* OMAP_DSS_CHANNEL_DIGIT */

• OMAP_DSS_OUTPUT_VENC | OMAP_DSS_OUTPUT_HDMI |
• OMAP_DSS_OUTPUT_DPI,

• OMAP_DSS_OUTPUT_VENC | OMAP_DSS_OUTPUT_HDMI,

  /* OMAP_DSS_CHANNEL_LCD2 */ OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI |

Thanks, I'll apply this to omapdss fixes branch.

Hi, just one point here, this patch is a prerequisite for the patch 2/4 in this series. So we need to make sure that the 2/4 patch is not without this one in a kernel.

Tomi,

About patch '2/4', could you have a look at it too? It basically tries to do a dynamic assignment of channels to outputs. I worked on this before you posted the misc series with recommended_channel for outputs. This patch tries to figure out managers with supported_outputs. It isn't the most optimal way, as it can't back track and chose a better manager, but it still seems to do a reasonable job.

We could also use the recommended channel way for omapdrm, I can't figure out what's the better approach at the moment.

Archit

On Tuesday 12 March 2013 04:08 PM, Tomi Valkeinen wrote:

On 2013-03-12 08:07, Archit Taneja wrote:

...

On Monday 11 March 2013 05:58 PM, Tomi Valkeinen wrote:

...

On 2013-03-05 16:17, Archit Taneja wrote:

...

The support outputs struct for overlay managers is incorrect for OMAP4. Make these changes:

• DPI isn't supported via the LCD1 overlay manager, remove DPI as a

supported output.

• the TV manager can suppport DPI, but the omapdss driver doesn't

support that yet, we require some muxing at the DSS level, and we also need to configure the hdmi pll in the DPI driver so that the TV manager has a pixel clock. We don't support that yet.

Signed-off-by: Archit Taneja archit@ti.com

drivers/video/omap2/dss/dss_features.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c index d7d66ef..7f791ae 100644 --- a/drivers/video/omap2/dss/dss_features.c +++ b/drivers/video/omap2/dss/dss_features.c @@ -202,12 +202,10 @@ static const enum omap_dss_output_id omap3630_dss_supported_outputs[] = {

static const enum omap_dss_output_id omap4_dss_supported_outputs[] = { /* OMAP_DSS_CHANNEL_LCD */

• OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI |
• OMAP_DSS_OUTPUT_DSI1,

• OMAP_DSS_OUTPUT_DBI | OMAP_DSS_OUTPUT_DSI1,

  /* OMAP_DSS_CHANNEL_DIGIT */

• OMAP_DSS_OUTPUT_VENC | OMAP_DSS_OUTPUT_HDMI |
• OMAP_DSS_OUTPUT_DPI,

• OMAP_DSS_OUTPUT_VENC | OMAP_DSS_OUTPUT_HDMI,

  /* OMAP_DSS_CHANNEL_LCD2 */ OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI |

Thanks, I'll apply this to omapdss fixes branch.

Hi, just one point here, this patch is a prerequisite for the patch 2/4 in this series. So we need to make sure that the 2/4 patch is not without this one in a kernel.

Tomi,

About patch '2/4', could you have a look at it too? It basically tries to do a dynamic assignment of channels to outputs. I worked on this before you posted the misc series with recommended_channel for outputs. This patch tries to figure out managers with supported_outputs. It isn't the most optimal way, as it can't back track and chose a better manager, but it still seems to do a reasonable job.

We could also use the recommended channel way for omapdrm, I can't figure out what's the better approach at the moment.

Hmm, I think it'd be safer to use the recommended channel from omapdss for now. The current omapdss code doesn't really let you use any other channel than the recommended one (which was thus renamed to dispc_channel in my later version).

I think omapdss needs to give the freedom to set a different dispc manager for an output. This is because in omapdrm crtcs are sort of floating, and when a connector(and it's encoder) needs to be enabled, it tries to look search for a crtc it can connect to. If omapdss sort of fixes the output->manager link, which is not how it should work.

I think it is fine for us to use this approach for omapfb when creating the links, but not within dss.

For example, the code here:

- /* - * XXX We shouldn't need dssdev->channel for this. The dsi pll clock - * source for DPI is SoC integration detail, not something that should - * be configured in the dssdev - */ - dsidev = dpi_get_dsidev(dssdev->channel); + dsidev = dpi_get_dsidev(dpi.output.dispc_channel);

We are sort of using the recommended channel to figure out which DSI pll to use. We don't have much of an option here because dpi_init_display() happens much earlier. But if it were to connect to a different manager later on, we would get into trouble.

Or does your patch do a better job at selecting the outputs (I'm mostly thinking about OMAP5 here, which has a bit more conflicts with the mgrs and outputs than earlier omaps).

My approach is very drm oriented, the problem with omapdrm right now is that we want to limit the number of crtcs(which map to managers). The lesser the number of crtcs, the more free planes we have for overlaying.

If through bootargs or CONFIG, we set NUM_CRTCs to 2. We can only set up only 2 overlay managers. My patch just tries to figure out which are the best 2 managers to choose out of the total number of managers, in such a way that most encoders/panels on the platform are supported.

The drm framework(I think) keeps the connections between crtc and encoders flexible through the possible_crtcs flag maintained by encoders. It figures out which crtc is free, and tries to use that one at that moment. Once that encoder is done using it, the crtc can be used by another encoder later. We can also change the crtc of an encoder(after it's off).

So omapdss fixing the dispc channel for an output doesn't seem suitable for drm.

Archit

On Tuesday 12 March 2013 07:36 PM, Tomi Valkeinen wrote:

On 2013-03-12 15:06, Archit Taneja wrote:

...

The omapdrm driver requires omapdss panel drivers to expose ops like detect, set_timings and check_timings. These can be NULL for fixed panel DPI, DBI, DSI and SDI drivers. At some places, there are no checks to see if the panel driver has these ops or not, and that leads to a crash.

The following things are done to make fixed panels work:

• The omap_connector's detect function is modified such that it considers panel types which are generally fixed panels as always connected(provided the panel driver doesn't have a detect op). Hence, the connector corresponding to these panels is always in a 'connected' state.

• If a panel driver doesn't have a check_timings op, assume that it supports the mode passed to omap_connector_mode_valid(the 'mode_valid' drm helper function)

• The function omap_encoder_update shouldn't really do anything for fixed resolution panels, make sure that it calls set_timings only if the panel driver has one.

Signed-off-by: Archit Taneja archit@ti.com

Note: In v2, we make sure that the mode passed on to omapdrm matches the timings of the fixed resolution panel.

drivers/gpu/drm/omapdrm/omap_connector.c | 27 +++++++++++++++++++++++++-- drivers/gpu/drm/omapdrm/omap_encoder.c | 17 +++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c index c451c41..a72fedd 100644 --- a/drivers/gpu/drm/omapdrm/omap_connector.c +++ b/drivers/gpu/drm/omapdrm/omap_connector.c @@ -110,6 +110,11 @@ static enum drm_connector_status omap_connector_detect( ret = connector_status_connected; else ret = connector_status_disconnected;

• } else if (dssdev->type == OMAP_DISPLAY_TYPE_DPI ||

• 	dssdev->type == OMAP_DISPLAY_TYPE_DBI ||
  

• 	dssdev->type == OMAP_DISPLAY_TYPE_SDI ||
  

• 	dssdev->type == OMAP_DISPLAY_TYPE_DSI) {
  

• ret = connector_status_connected;
  

I have to say I don't like this. We shouldn't care about the type here. I think it's better just to default to connected if there's no detect function (or unknown? I'm not sure what is the practical difference).

If it works fine without using dssdev->type, we have one less place to worry when doing dss dev model changes =).

...

} else { ret = connector_status_unknown; } @@ -189,12 +194,30 @@ static int omap_connector_mode_valid(struct drm_connector *connector, struct omap_video_timings timings = {0}; struct drm_device *dev = connector->dev; struct drm_display_mode *new_mode;

• int ret = MODE_BAD;

• int r, ret = MODE_BAD;

  copy_timings_drm_to_omap(&timings, mode); mode->vrefresh = drm_mode_vrefresh(mode);

• if (!dssdrv->check_timings(dssdev, &timings)) {

• /*

• * if the panel driver doesn't have a check_timings, it's most likely
  

• * a fixed resolution panel, check if the timings match with the
  

• * panel's timings
  

• */
  

• if (dssdrv->check_timings) {

• r = dssdrv->check_timings(dssdev, &timings);
  

• } else {

• struct omap_video_timings t;
  

• dssdrv->get_timings(dssdev, &t);
  

• if (memcmp(&timings, &t, sizeof(struct omap_video_timings)))
  

• 	r = -EINVAL;
  

• else
  

• 	r = 0;
  

memcmp on two structs is often not a good idea. There could be padding bytes there, with uninitialized data. I'm not sure if that's the case here, though, but it could well change any time (perhaps even depending on compiler version).

I saw usage of memcmp on structs in the kernel, but then most of them were in drivers and not core, and could be mistakes :)

adding '__attribute__((packed))' to omap_video_timings might be helpful I suppose. But I don't know if it's safe to do a memcmp even with a packed struct.

I'm still pondering whether it'd just be simpler to require all the dssdrv ops to be filled, probably using a helper macro in the panel drivers... Did you consider that approach? I'm not saying to go for it, I'm saying I can't make my mind which would be better =).

I didn't consider it mainly because I thought we were going to get rid of our private omapdss panel drivers with CDF panels, and efforts in fixing it wouldn't be much useful. But if there isn't any other good alternative, then I can try to see what macros look like.

Of course, simpler options for this patch would be to do a manual compare of the fields instead of a memcmp, or adding default ops in omap_dss_register_driver.

One thing about common panel driver functions in general is that they won't use the panel driver's locking. So if a panel driver doesn't create a get_timings() op assuming that omapdss will make a default func for it, we would miss out on the panel lock. I don't know if that's really bad, and doing a memcmp or anything else in omapdrm also doesn't help with this case.

Archit

The omapdrm driver currently takes a config/module arg to figure out the number of crtcs it needs to create. We could create as many crtcs as there are overlay managers in the DSS hardware, but we don't do that because each crtc eats up one DSS overlay, and that reduces the number of planes we can attach to a single crtc.

Since the number of crtcs may be lesser than the number of hardware overlay managers, we need to figure out which overlay managers to use for our crtcs. The current approach is to use pipe2chan(), which returns a higher numbered manager for the crtc.

The problem with this approach is that it assumes that the overlay managers we choose will connect to the encoders the platform's panels are going to use, this isn't true, an overlay manager connects only to a few outputs/encoders, and choosing any overlay manager for our crtc might lead to a situation where the encoder cannot connect to any of the crtcs we have chosen. For example, an omap5-panda board has just one hdmi output. If num_crtc is set to 1, with the current approach, pipe2chan will pick up the LCD2 overlay manager, which cannot connect to the hdmi encoder at all. The only manager that could have connected to hdmi was the TV overlay manager.

Therefore, there is a need to choose our overlay managers keeping in mind the panels we have on that platform. The new approach iterates through all the available panels, creates encoders and connectors for them, and then tries to get a suitable overlay manager to create a crtc which can connect to the encoders.

We use the dispc_channel field in omap_dss_output to retrieve the desired overlay manager's channel number, we then check whether the manager had already been assigned to a crtc or not. If it was already assigned to a crtc, we assume that out of all the encoders which intend use this crtc, only one will run at a time. If the overlay manager wan't assigned to a crtc till then, we create a new crtc and link it with the overlay manager.

This approach just looks for the best dispc_channel for each encoder. On DSS HW, some encoders can connect to multiple overlay managers. Since we don't try looking for alternate overlay managers, there is a greater possibility that 2 or more encoders end up asking for the same crtc, causing only one encoder to run at a time.

Also, this approach isn't the most optimal one, it can do either good or bad depending on the sequence in which the panels/outputs are parsed. The optimal way would be some sort of back tracking approach, where we improve the set of managers we use as we iterate through the list of panels/encoders. That's something left for later.

Signed-off-by: Archit Taneja archit@ti.com --- drivers/gpu/drm/omapdrm/omap_crtc.c | 21 +++-- drivers/gpu/drm/omapdrm/omap_drv.c | 157 ++++++++++++++++++++++++++------ drivers/gpu/drm/omapdrm/omap_drv.h | 38 +------- drivers/gpu/drm/omapdrm/omap_encoder.c | 7 ++ drivers/gpu/drm/omapdrm/omap_irq.c | 17 ++-- 5 files changed, 165 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index bec66a4..79b200a 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -74,6 +74,13 @@ struct omap_crtc { struct work_struct page_flip_work; };

+uint32_t pipe2vbl(struct drm_crtc *crtc) +{ + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + + return dispc_mgr_get_vsync_irq(omap_crtc->channel); +} + /* * Manager-ops, callbacks from output when they need to configure * the upstream part of the video pipe. @@ -613,7 +620,13 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, omap_crtc->apply.pre_apply = omap_crtc_pre_apply; omap_crtc->apply.post_apply = omap_crtc_post_apply;

- omap_crtc->apply_irq.irqmask = pipe2vbl(id); + omap_crtc->channel = channel; + omap_crtc->plane = plane; + omap_crtc->plane->crtc = crtc; + omap_crtc->name = channel_names[channel]; + omap_crtc->pipe = id; + + omap_crtc->apply_irq.irqmask = pipe2vbl(crtc); omap_crtc->apply_irq.irq = omap_crtc_apply_irq;

omap_crtc->error_irq.irqmask = @@ -621,12 +634,6 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, omap_crtc->error_irq.irq = omap_crtc_error_irq; omap_irq_register(dev, &omap_crtc->error_irq);

- omap_crtc->channel = channel; - omap_crtc->plane = plane; - omap_crtc->plane->crtc = crtc; - omap_crtc->name = channel_names[channel]; - omap_crtc->pipe = id; - /* temporary: */ omap_crtc->mgr.id = channel;

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 77b7225..cbaa003 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -74,49 +74,48 @@ static int get_connector_type(struct omap_dss_device *dssdev) } }

+static bool channel_used(struct drm_device *dev, enum omap_channel channel) +{ + struct omap_drm_private *priv = dev->dev_private; + int i; + + for (i = 0; i < priv->num_crtcs; i++) { + struct drm_crtc *crtc = priv->crtcs[i]; + + if (omap_crtc_channel(crtc) == channel) + return true; + } + + return false; +} + static int omap_modeset_init(struct drm_device *dev) { struct omap_drm_private *priv = dev->dev_private; struct omap_dss_device *dssdev = NULL; int num_ovls = dss_feat_get_num_ovls(); - int id; + int num_mgrs = dss_feat_get_num_mgrs(); + int num_crtcs; + int i, id = 0;

drm_mode_config_init(dev);

omap_drm_irq_install(dev);

/* - * Create private planes and CRTCs for the last NUM_CRTCs overlay - * plus manager: - */ - for (id = 0; id < min(num_crtc, num_ovls); id++) { - struct drm_plane *plane; - struct drm_crtc *crtc; - - plane = omap_plane_init(dev, id, true); - crtc = omap_crtc_init(dev, plane, pipe2chan(id), id); - - BUG_ON(priv->num_crtcs >= ARRAY_SIZE(priv->crtcs)); - priv->crtcs[id] = crtc; - priv->num_crtcs++; - - priv->planes[id] = plane; - priv->num_planes++; - } - - /* - * Create normal planes for the remaining overlays: + * We usually don't want to create a CRTC for each manager, at least + * not until we have a way to expose private planes to userspace. + * Otherwise there would not be enough video pipes left for drm planes. + * We use the num_crtc argument to limit the number of crtcs we create. */ - for (; id < num_ovls; id++) { - struct drm_plane *plane = omap_plane_init(dev, id, false); + num_crtcs = min3(num_crtc, num_mgrs, num_ovls);

- BUG_ON(priv->num_planes >= ARRAY_SIZE(priv->planes)); - priv->planes[priv->num_planes++] = plane; - } + dssdev = NULL;

for_each_dss_dev(dssdev) { struct drm_connector *connector; struct drm_encoder *encoder; + enum omap_channel channel;

if (!dssdev->driver) { dev_warn(dev->dev, "%s has no driver.. skipping it\n", @@ -157,16 +156,118 @@ static int omap_modeset_init(struct drm_device *dev)

drm_mode_connector_attach_encoder(connector, encoder);

+ /* + * if we have reached the limit of the crtcs we are allowed to + * create, let's not try to look for a crtc for this + * panel/encoder and onwards, we will, of course, populate the + * the possible_crtcs field for all the encoders with the final + * set of crtcs we create + */ + if (id == num_crtcs) + continue; + + /* + * get the recommended DISPC channel for this encoder. For now, + * we only try to get create a crtc out of the recommended, the + * other possible channels to which the encoder can connect are + * not considered. + */ + channel = dssdev->output->dispc_channel; + + /* + * if this channel hasn't already been taken by a previously + * allocated crtc, we create a new crtc for it + */ + if (!channel_used(dev, channel)) { + struct drm_plane *plane; + struct drm_crtc *crtc; + + plane = omap_plane_init(dev, id, true); + crtc = omap_crtc_init(dev, plane, channel, id); + + BUG_ON(priv->num_crtcs >= ARRAY_SIZE(priv->crtcs)); + priv->crtcs[id] = crtc; + priv->num_crtcs++; + + priv->planes[id] = plane; + priv->num_planes++; + + id++; + } + } + + /* + * we have allocated crtcs according to the need of the panels/encoders, + * adding more crtcs here if needed + */ + for (; id < num_crtcs; id++) { + + /* find a free manager for this crtc */ + for (i = 0; i < num_mgrs; i++) { + if (!channel_used(dev, i)) { + struct drm_plane *plane; + struct drm_crtc *crtc; + + plane = omap_plane_init(dev, id, true); + crtc = omap_crtc_init(dev, plane, i, id); + + BUG_ON(priv->num_crtcs >= + ARRAY_SIZE(priv->crtcs)); + + priv->crtcs[id] = crtc; + priv->num_crtcs++; + + priv->planes[id] = plane; + priv->num_planes++; + + break; + } else { + continue; + } + } + + if (i == num_mgrs) { + /* this shouldn't really happen */ + dev_err(dev->dev, "no managers left for crtc\n"); + return -ENOMEM; + } + } + + /* + * Create normal planes for the remaining overlays: + */ + for (; id < num_ovls; id++) { + struct drm_plane *plane = omap_plane_init(dev, id, false); + + BUG_ON(priv->num_planes >= ARRAY_SIZE(priv->planes)); + priv->planes[priv->num_planes++] = plane; + } + + for (i = 0; i < priv->num_encoders; i++) { + struct drm_encoder *encoder = priv->encoders[i]; + struct omap_dss_device *dssdev = + omap_encoder_get_dssdev(encoder); + /* figure out which crtc's we can connect the encoder to: */ encoder->possible_crtcs = 0; for (id = 0; id < priv->num_crtcs; id++) { - enum omap_dss_output_id supported_outputs = - dss_feat_get_supported_outputs(pipe2chan(id)); + struct drm_crtc *crtc = priv->crtcs[id]; + enum omap_channel crtc_channel; + enum omap_dss_output_id supported_outputs; + + crtc_channel = omap_crtc_channel(crtc); + supported_outputs = + dss_feat_get_supported_outputs(crtc_channel); + if (supported_outputs & dssdev->output->id) encoder->possible_crtcs |= (1 << id); } }

+ DBG("registered %d planes, %d crtcs, %d encoders and %d connectors\n", + priv->num_planes, priv->num_crtcs, priv->num_encoders, + priv->num_connectors); + dev->mode_config.min_width = 32; dev->mode_config.min_height = 32;

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index d4f997b..215a20d 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -139,8 +139,8 @@ void omap_gem_describe_objects(struct list_head *list, struct seq_file *m); int omap_gem_resume(struct device *dev); #endif

-int omap_irq_enable_vblank(struct drm_device *dev, int crtc); -void omap_irq_disable_vblank(struct drm_device *dev, int crtc); +int omap_irq_enable_vblank(struct drm_device *dev, int crtc_id); +void omap_irq_disable_vblank(struct drm_device *dev, int crtc_id); irqreturn_t omap_irq_handler(DRM_IRQ_ARGS); void omap_irq_preinstall(struct drm_device *dev); int omap_irq_postinstall(struct drm_device *dev); @@ -271,39 +271,9 @@ static inline int align_pitch(int pitch, int width, int bpp) return ALIGN(pitch, 8 * bytespp); }

-static inline enum omap_channel pipe2chan(int pipe) -{ - int num_mgrs = dss_feat_get_num_mgrs(); - - /* - * We usually don't want to create a CRTC for each manager, - * at least not until we have a way to expose private planes - * to userspace. Otherwise there would not be enough video - * pipes left for drm planes. The higher #'d managers tend - * to have more features so start in reverse order. - */ - return num_mgrs - pipe - 1; -} - /* map crtc to vblank mask */ -static inline uint32_t pipe2vbl(int crtc) -{ - enum omap_channel channel = pipe2chan(crtc); - return dispc_mgr_get_vsync_irq(channel); -} - -static inline int crtc2pipe(struct drm_device *dev, struct drm_crtc *crtc) -{ - struct omap_drm_private *priv = dev->dev_private; - int i; - - for (i = 0; i < ARRAY_SIZE(priv->crtcs); i++) - if (priv->crtcs[i] == crtc) - return i; - - BUG(); /* bogus CRTC ptr */ - return -1; -} +uint32_t pipe2vbl(struct drm_crtc *crtc); +struct omap_dss_device *omap_encoder_get_dssdev(struct drm_encoder *encoder);

/* should these be made into common util helpers? */ diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c index 21d126d..d48df71 100644 --- a/drivers/gpu/drm/omapdrm/omap_encoder.c +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c @@ -41,6 +41,13 @@ struct omap_encoder { struct omap_dss_device *dssdev; };

+struct omap_dss_device *omap_encoder_get_dssdev(struct drm_encoder *encoder) +{ + struct omap_encoder *omap_encoder = to_omap_encoder(encoder); + + return omap_encoder->dssdev; +} + static void omap_encoder_destroy(struct drm_encoder *encoder) { struct omap_encoder *omap_encoder = to_omap_encoder(encoder); diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c index e01303e..9263db1 100644 --- a/drivers/gpu/drm/omapdrm/omap_irq.c +++ b/drivers/gpu/drm/omapdrm/omap_irq.c @@ -130,12 +130,13 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait, * Zero on success, appropriate errno if the given @crtc's vblank * interrupt cannot be enabled. */ -int omap_irq_enable_vblank(struct drm_device *dev, int crtc) +int omap_irq_enable_vblank(struct drm_device *dev, int crtc_id) { struct omap_drm_private *priv = dev->dev_private; + struct drm_crtc *crtc = priv->crtcs[crtc_id]; unsigned long flags;

- DBG("dev=%p, crtc=%d", dev, crtc); + DBG("dev=%p, crtc=%d", dev, crtc_id);

dispc_runtime_get(); spin_lock_irqsave(&list_lock, flags); @@ -156,12 +157,13 @@ int omap_irq_enable_vblank(struct drm_device *dev, int crtc) * a hardware vblank counter, this routine should be a no-op, since * interrupts will have to stay on to keep the count accurate. */ -void omap_irq_disable_vblank(struct drm_device *dev, int crtc) +void omap_irq_disable_vblank(struct drm_device *dev, int crtc_id) { struct omap_drm_private *priv = dev->dev_private; + struct drm_crtc *crtc = priv->crtcs[crtc_id]; unsigned long flags;

- DBG("dev=%p, crtc=%d", dev, crtc); + DBG("dev=%p, crtc=%d", dev, crtc_id);

dispc_runtime_get(); spin_lock_irqsave(&list_lock, flags); @@ -186,9 +188,12 @@ irqreturn_t omap_irq_handler(DRM_IRQ_ARGS)

VERB("irqs: %08x", irqstatus);

- for (id = 0; id < priv->num_crtcs; id++) - if (irqstatus & pipe2vbl(id)) + for (id = 0; id < priv->num_crtcs; id++) { + struct drm_crtc *crtc = priv->crtcs[id]; + + if (irqstatus & pipe2vbl(crtc)) drm_handle_vblank(dev, id); + }

spin_lock_irqsave(&list_lock, flags); list_for_each_entry_safe(handler, n, &priv->irq_list, node) {

On 2013-03-26 15:45, Archit Taneja wrote:

The omapdrm driver requires omapdss panel drivers to expose ops like detect, set_timings and check_timings. These can be NULL for fixed panel DPI, DBI, DSI and SDI drivers. At some places, there are no checks to see if the panel driver has these ops or not, and that leads to a crash.

The following things are done to make fixed panels work:

• The omap_connector's detect function is modified such that it considers panel types which are generally fixed panels as always connected(provided the panel driver doesn't have a detect op). Hence, the connector corresponding to these panels is always in a 'connected' state.

• If a panel driver doesn't have a check_timings op, assume that it supports the mode passed to omap_connector_mode_valid(the 'mode_valid' drm helper function)

• The function omap_encoder_update shouldn't really do anything for fixed resolution panels, make sure that it calls set_timings only if the panel driver has one.

Signed-off-by: Archit Taneja archit@ti.com

v3: clear the timings local variable first before using memcmp v2: make sure the timings we try to set for a fixed resolution panel match the panel's timings

drivers/gpu/drm/omapdrm/omap_connector.c | 27 +++++++++++++++++++++++++-- drivers/gpu/drm/omapdrm/omap_encoder.c | 17 +++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c index c451c41..912759d 100644 --- a/drivers/gpu/drm/omapdrm/omap_connector.c +++ b/drivers/gpu/drm/omapdrm/omap_connector.c @@ -110,6 +110,11 @@ static enum drm_connector_status omap_connector_detect( ret = connector_status_connected; else ret = connector_status_disconnected;

• } else if (dssdev->type == OMAP_DISPLAY_TYPE_DPI ||

• 	dssdev->type == OMAP_DISPLAY_TYPE_DBI ||
  

• 	dssdev->type == OMAP_DISPLAY_TYPE_SDI ||
  

• 	dssdev->type == OMAP_DISPLAY_TYPE_DSI) {
  

• ret = connector_status_connected;
  

  } else { ret = connector_status_unknown; }

Can we leave this part out? I don't like hardcoding things like that, and if I'm not mistaken, this only affects VENC.

If the code above would use detect where available, and presume the panel is connected in other cases, it'll be right for all other displays but VENC, for which we have no connect information.

Or, there could be a special case for VENC, like above, which sets the connector status to unknown for that. And connected for all others. It'd still be hardcoded, but for much less cases.

Tomi

The support outputs struct for overlay managers is incorrect for OMAP4. Make these changes:

- DPI isn't supported via the LCD1 overlay manager, remove DPI as a supported output. - the TV manager can suppport DPI, but the omapdss driver doesn't support that yet, we require some muxing at the DSS level, and we also need to configure the hdmi pll in the DPI driver so that the TV manager has a pixel clock. We don't support that yet.

Signed-off-by: Archit Taneja archit@ti.com --- drivers/video/omap2/dss/dss_features.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c index d7d66ef..7f791ae 100644 --- a/drivers/video/omap2/dss/dss_features.c +++ b/drivers/video/omap2/dss/dss_features.c @@ -202,12 +202,10 @@ static const enum omap_dss_output_id omap3630_dss_supported_outputs[] = {

static const enum omap_dss_output_id omap4_dss_supported_outputs[] = { /* OMAP_DSS_CHANNEL_LCD */ - OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI | - OMAP_DSS_OUTPUT_DSI1, + OMAP_DSS_OUTPUT_DBI | OMAP_DSS_OUTPUT_DSI1,

/* OMAP_DSS_CHANNEL_DIGIT */ - OMAP_DSS_OUTPUT_VENC | OMAP_DSS_OUTPUT_HDMI | - OMAP_DSS_OUTPUT_DPI, + OMAP_DSS_OUTPUT_VENC | OMAP_DSS_OUTPUT_HDMI,

/* OMAP_DSS_CHANNEL_LCD2 */ OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI |

On 2013-03-26 15:45, Archit Taneja wrote:

Each version of OMAP has a limitation on the maximum pixel clock frequency supported by an overlay manager. This limit isn't checked by omapdss. Add dispc feats for lcd and tv managers and check whether the target timings can be supported or not.

The pixel clock limitations are actually more complex. They depend on which OPP OMAP is in, and they also depend on which encoder is the manager connected to. The OPP dependence is ignored as DSS forces the PM framework to be on OPP100 when DSS is enabled, and the encoder dependencies are ignored by DISPC for now. These limits should come from the encoder driver.

The OMAP2 TRM doesn't mention the maximum pixel clock limit. This value is left as half of DSS_FCLK, as OMAP2 requires the PCD to be atleast 2.

Signed-off-by: Archit Taneja archit@ti.com

drivers/video/omap2/dss/dispc.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c index 8cfa27b..73a730a 100644 --- a/drivers/video/omap2/dss/dispc.c +++ b/drivers/video/omap2/dss/dispc.c @@ -69,6 +69,8 @@ struct dispc_features { u8 mgr_height_start; u16 mgr_width_max; u16 mgr_height_max;

• unsigned long max_lcd_pclk;
• unsigned long max_tv_pclk; int (*calc_scaling) (unsigned long pclk, unsigned long lclk, const struct omap_video_timings *mgr_timings, u16 width, u16 height, u16 out_width, u16 out_height,

@@ -2825,6 +2827,15 @@ static bool _dispc_lcd_timings_ok(int hsw, int hfp, int hbp, return true; }

+static bool _dispc_mgr_pclk_ok(enum omap_channel channel,

• unsigned long pclk)
  

+{

• if (dss_mgr_is_lcd(channel))

• return pclk <= dispc.feat->max_lcd_pclk ? true : false;
  

• else

• return pclk <= dispc.feat->max_tv_pclk ? true : false;
  

+}

bool dispc_mgr_timings_ok(enum omap_channel channel, const struct omap_video_timings *timings) { @@ -2832,11 +2843,13 @@ bool dispc_mgr_timings_ok(enum omap_channel channel,

timings_ok = _dispc_mgr_size_ok(timings->x_res, timings->y_res);

• if (dss_mgr_is_lcd(channel))

• timings_ok =  timings_ok && _dispc_lcd_timings_ok(timings->hsw,
  

• 				timings->hfp, timings->hbp,
  

• 				timings->vsw, timings->vfp,
  

• 				timings->vbp);
  

• timings_ok &= _dispc_mgr_pclk_ok(channel, timings->pixel_clock * 1000);

• if (dss_mgr_is_lcd(channel)) {

• timings_ok &= _dispc_lcd_timings_ok(timings->hsw, timings->hfp,
  

• 		timings->hbp, timings->vsw, timings->vfp,
  

• 		timings->vbp);
  

• }

  return timings_ok;

} @@ -3491,6 +3504,7 @@ static const struct dispc_features omap24xx_dispc_feats __initconst = { .mgr_height_start = 26, .mgr_width_max = 2048, .mgr_height_max = 2048,

• .max_lcd_pclk = 66500000, .calc_scaling = dispc_ovl_calc_scaling_24xx, .calc_core_clk = calc_core_clk_24xx, .num_fifos = 3,

OMAP2 has VENC output, but there's no max_tv_pclk above. This would make VENC pclk check to fail always, wouldn't it?

Tomi

Increase the DSS_FCLK and DSI_FCLK max supported frequencies, these come because some frequencies were increased from OMAP5 ES1 to OMAP5 ES2. We support only OMAP5 ES2 in the kernel, so replace the ES1 values with ES2 values. Increase the DSI PLL Fint range, this was previously just copied from the OMAP4 param range struct.

Fix the maximum DSS_FCLK on OMAP2, it's 133 Mhz according to the TRM.

Signed-off-by: Archit Taneja archit@ti.com --- drivers/video/omap2/dss/dss_features.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c index 7f791ae..77dbe0c 100644 --- a/drivers/video/omap2/dss/dss_features.c +++ b/drivers/video/omap2/dss/dss_features.c @@ -414,7 +414,7 @@ static const char * const omap5_dss_clk_source_names[] = { };

static const struct dss_param_range omap2_dss_param_range[] = { - [FEAT_PARAM_DSS_FCK] = { 0, 173000000 }, + [FEAT_PARAM_DSS_FCK] = { 0, 133000000 }, [FEAT_PARAM_DSS_PCD] = { 2, 255 }, [FEAT_PARAM_DSIPLL_REGN] = { 0, 0 }, [FEAT_PARAM_DSIPLL_REGM] = { 0, 0 }, @@ -459,15 +459,15 @@ static const struct dss_param_range omap4_dss_param_range[] = { };

static const struct dss_param_range omap5_dss_param_range[] = { - [FEAT_PARAM_DSS_FCK] = { 0, 200000000 }, + [FEAT_PARAM_DSS_FCK] = { 0, 209250000 }, [FEAT_PARAM_DSS_PCD] = { 1, 255 }, [FEAT_PARAM_DSIPLL_REGN] = { 0, (1 << 8) - 1 }, [FEAT_PARAM_DSIPLL_REGM] = { 0, (1 << 12) - 1 }, [FEAT_PARAM_DSIPLL_REGM_DISPC] = { 0, (1 << 5) - 1 }, [FEAT_PARAM_DSIPLL_REGM_DSI] = { 0, (1 << 5) - 1 }, - [FEAT_PARAM_DSIPLL_FINT] = { 500000, 2500000 }, + [FEAT_PARAM_DSIPLL_FINT] = { 150000, 52000000 }, [FEAT_PARAM_DSIPLL_LPDIV] = { 0, (1 << 13) - 1 }, - [FEAT_PARAM_DSI_FCK] = { 0, 170000000 }, + [FEAT_PARAM_DSI_FCK] = { 0, 209250000 }, [FEAT_PARAM_DOWNSCALE] = { 1, 4 }, [FEAT_PARAM_LINEWIDTH] = { 1, 2048 }, };

On Wednesday 27 March 2013 01:03 PM, Tomi Valkeinen wrote:

On 2013-03-26 15:45, Archit Taneja wrote:

...

Increase the DSS_FCLK and DSI_FCLK max supported frequencies, these come because some frequencies were increased from OMAP5 ES1 to OMAP5 ES2. We support only OMAP5 ES2 in the kernel, so replace the ES1 values with ES2 values. Increase the DSI PLL Fint range, this was previously just copied from the OMAP4 param range struct.

Fix the maximum DSS_FCLK on OMAP2, it's 133 Mhz according to the TRM.

Signed-off-by: Archit Taneja archit@ti.com

drivers/video/omap2/dss/dss_features.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c index 7f791ae..77dbe0c 100644 --- a/drivers/video/omap2/dss/dss_features.c +++ b/drivers/video/omap2/dss/dss_features.c @@ -414,7 +414,7 @@ static const char * const omap5_dss_clk_source_names[] = { };

static const struct dss_param_range omap2_dss_param_range[] = {

• [FEAT_PARAM_DSS_FCK] = { 0, 173000000 },

• [FEAT_PARAM_DSS_FCK] = { 0, 133000000 }, [FEAT_PARAM_DSS_PCD] = { 2, 255 }, [FEAT_PARAM_DSIPLL_REGN] = { 0, 0 }, [FEAT_PARAM_DSIPLL_REGM] = { 0, 0 },

@@ -459,15 +459,15 @@ static const struct dss_param_range omap4_dss_param_range[] = { };

static const struct dss_param_range omap5_dss_param_range[] = {

• [FEAT_PARAM_DSS_FCK] = { 0, 200000000 },

• [FEAT_PARAM_DSS_FCK] = { 0, 209250000 }, [FEAT_PARAM_DSS_PCD] = { 1, 255 }, [FEAT_PARAM_DSIPLL_REGN] = { 0, (1 << 8) - 1 }, [FEAT_PARAM_DSIPLL_REGM] = { 0, (1 << 12) - 1 }, [FEAT_PARAM_DSIPLL_REGM_DISPC] = { 0, (1 << 5) - 1 }, [FEAT_PARAM_DSIPLL_REGM_DSI] = { 0, (1 << 5) - 1 },

• [FEAT_PARAM_DSIPLL_FINT] = { 500000, 2500000 },

• [FEAT_PARAM_DSIPLL_FINT] = { 150000, 52000000 },

Just a note, I think the PLL FINT range for OMAP3/4 may be wrong also. Some TRMs mention the FINT range being up to 52MHz or so. I don't think it's ever very clearly stated, though...

I'll drop the FINT range modification for now. It's not that critical anyway since we manage to lock the PLL for most frequencies anyway. I'll do an update later after reading more detailed specs of the PLL.

Archit

DISPC on OMAP5 has a more optimised mechanism of asserting Mstandby to achieve more power savings when DISPC is configured in Smart Standby mode. This mechanism leads to underflows when multiple DISPC pipes are enabled.

There is a register field which can let us revert to the older mechanism of asserting Mstandby. Configure this field to prevent underflows.

Signed-off-by: Archit Taneja archit@ti.com --- drivers/video/omap2/dss/dispc.c | 7 +++++++ drivers/video/omap2/dss/dispc.h | 1 + 2 files changed, 8 insertions(+)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c index ddbf031..b33b016 100644 --- a/drivers/video/omap2/dss/dispc.c +++ b/drivers/video/omap2/dss/dispc.c @@ -87,6 +87,9 @@ struct dispc_features {

/* no DISPC_IRQ_FRAMEDONETV on this SoC */ bool no_framedone_tv:1; + + /* revert to the OMAP4 mechanism of DISPC Smart Standby operation */ + bool mstandby_workaround:1; };

#define DISPC_MAX_NR_FIFOS 5 @@ -3502,6 +3505,9 @@ static void _omap_dispc_initial_config(void) dispc_configure_burst_sizes();

dispc_ovl_enable_zorder_planes(); + + if (dispc.feat->mstandby_workaround) + REG_FLD_MOD(DISPC_MSTANDBY_CTRL, 1, 0, 0); }

static const struct dispc_features omap24xx_dispc_feats __initconst = { @@ -3596,6 +3602,7 @@ static const struct dispc_features omap54xx_dispc_feats __initconst = { .calc_core_clk = calc_core_clk_44xx, .num_fifos = 5, .gfx_fifo_workaround = true, + .mstandby_workaround = true, };

static int __init dispc_init_features(struct platform_device *pdev) diff --git a/drivers/video/omap2/dss/dispc.h b/drivers/video/omap2/dss/dispc.h index 222363c..de4863d 100644 --- a/drivers/video/omap2/dss/dispc.h +++ b/drivers/video/omap2/dss/dispc.h @@ -39,6 +39,7 @@ #define DISPC_GLOBAL_BUFFER 0x0800 #define DISPC_CONTROL3 0x0848 #define DISPC_CONFIG3 0x084C +#define DISPC_MSTANDBY_CTRL 0x0858

/* DISPC overlay registers */ #define DISPC_OVL_BA0(n) (DISPC_OVL_BASE(n) + \