| Message ID | 1490706496-4959-29-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, Thank you for the patch. On Tuesday 28 Mar 2017 16:08:14 Tomi Valkeinen wrote: > At the moment VSYNC/HSYNC/DE high/low flags set by the panel/encoder > drivers get lost when the videotimings are translated to DRM's > videomode, as DRM's mode does not have corresponding flags. > > DRM has bus-flags for this purpose, and while it lacks a few flags at > the moment, it should be used here. However, until we rewrite omapdrm's > device model, using bus-flags is rather difficult. > > As a short term fix, this patch makes sure that every time the videomode > is set in omap_crtc_mode_set_nofb(), the driver asks for the SYNC/DE > flags from the panel/encoder drivers, and thus we get the correct flags > into use. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_connector.c | 2 -- > drivers/gpu/drm/omapdrm/omap_crtc.c | 28 +++++++++++++++++++++++++--- > 2 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c > b/drivers/gpu/drm/omapdrm/omap_connector.c index 50d2b641c28b..d62c035f050f > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_connector.c > +++ b/drivers/gpu/drm/omapdrm/omap_connector.c > @@ -146,8 +146,6 @@ static int omap_connector_mode_valid(struct > drm_connector *connector, int r, ret = MODE_BAD; > > drm_display_mode_to_videomode(mode, &vm); > - vm.flags |= DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_PIXDATA_POSEDGE | > - DISPLAY_FLAGS_SYNC_NEGEDGE; The dssdrv->get_timings() call below will return a video mode with polarity flags set, which will then be compared to vm. As you don't set the vm flags anymore, won't the comparison fail ? > mode->vrefresh = drm_mode_vrefresh(mode); > > /* > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 1db96b077ae8..606ef807741a > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -373,6 +373,11 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc > *crtc) { > struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > struct drm_display_mode *mode = &crtc->state->adjusted_mode; > + struct omap_drm_private *priv = crtc->dev->dev_private; > + const u32 flags_mask = DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_DE_LOW | > + DISPLAY_FLAGS_PIXDATA_POSEDGE | DISPLAY_FLAGS_PIXDATA_NEGEDGE | > + DISPLAY_FLAGS_SYNC_POSEDGE | DISPLAY_FLAGS_SYNC_NEGEDGE; > + int i; i only takes positive values, you can make it an unsigned int. > > DBG("%s: set mode: %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x", > omap_crtc->name, mode->base.id, mode->name, > @@ -382,9 +387,26 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc > *crtc) mode->type, mode->flags); > > drm_display_mode_to_videomode(mode, &omap_crtc->vm); > - omap_crtc->vm.flags |= DISPLAY_FLAGS_DE_HIGH | > - DISPLAY_FLAGS_PIXDATA_POSEDGE | > - DISPLAY_FLAGS_SYNC_NEGEDGE; > + Could you add a HACK/FIXME comment to explain how this should be fixed properly ? I want to make sure we won't forget to remove this hack. > + for (i = 0; i < priv->num_encoders; ++i) { > + struct drm_encoder *encoder = priv->encoders[i]; > + > + if (encoder->crtc == crtc) { > + struct omap_dss_device *dssdev; > + > + dssdev = omap_encoder_get_dssdev(encoder); > + > + if (dssdev) { > + struct videomode vm = {0}; > + > + dssdev->driver->get_timings(dssdev, &vm); > + > + omap_crtc->vm.flags |= vm.flags & flags_mask; > + } > + > + break; > + } > + } > } > > static int omap_crtc_atomic_check(struct drm_crtc *crtc,
On 29/03/17 11:58, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tuesday 28 Mar 2017 16:08:14 Tomi Valkeinen wrote: >> At the moment VSYNC/HSYNC/DE high/low flags set by the panel/encoder >> drivers get lost when the videotimings are translated to DRM's >> videomode, as DRM's mode does not have corresponding flags. >> >> DRM has bus-flags for this purpose, and while it lacks a few flags at >> the moment, it should be used here. However, until we rewrite omapdrm's >> device model, using bus-flags is rather difficult. >> >> As a short term fix, this patch makes sure that every time the videomode >> is set in omap_crtc_mode_set_nofb(), the driver asks for the SYNC/DE >> flags from the panel/encoder drivers, and thus we get the correct flags >> into use. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> --- >> drivers/gpu/drm/omapdrm/omap_connector.c | 2 -- >> drivers/gpu/drm/omapdrm/omap_crtc.c | 28 +++++++++++++++++++++++++--- >> 2 files changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c >> b/drivers/gpu/drm/omapdrm/omap_connector.c index 50d2b641c28b..d62c035f050f >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_connector.c >> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c >> @@ -146,8 +146,6 @@ static int omap_connector_mode_valid(struct >> drm_connector *connector, int r, ret = MODE_BAD; >> >> drm_display_mode_to_videomode(mode, &vm); >> - vm.flags |= DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_PIXDATA_POSEDGE | >> - DISPLAY_FLAGS_SYNC_NEGEDGE; > > The dssdrv->get_timings() call below will return a video mode with polarity > flags set, which will then be compared to vm. As you don't set the vm flags > anymore, won't the comparison fail ? Hmm, true. We never go to that code path, though, as we have check_timings in all the drivers, except panel-dsi-cs.c (but there's a patch adding it). And the panels don't check the flags, only things like resolution or porches. I think we can just require all drivers to have check_timings. It's hiding the problem a bit, but should work fine until this can be solved properly. Or actually, perhaps as a quick fix I should change the check to ignore the flags for now. >> mode->vrefresh = drm_mode_vrefresh(mode); >> >> /* >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c >> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 1db96b077ae8..606ef807741a >> 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c >> @@ -373,6 +373,11 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc >> *crtc) { >> struct omap_crtc *omap_crtc = to_omap_crtc(crtc); >> struct drm_display_mode *mode = &crtc->state->adjusted_mode; >> + struct omap_drm_private *priv = crtc->dev->dev_private; >> + const u32 flags_mask = DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_DE_LOW | >> + DISPLAY_FLAGS_PIXDATA_POSEDGE | DISPLAY_FLAGS_PIXDATA_NEGEDGE > | >> + DISPLAY_FLAGS_SYNC_POSEDGE | DISPLAY_FLAGS_SYNC_NEGEDGE; >> + int i; > > i only takes positive values, you can make it an unsigned int. Yep. >> >> DBG("%s: set mode: %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x", >> omap_crtc->name, mode->base.id, mode->name, >> @@ -382,9 +387,26 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc >> *crtc) mode->type, mode->flags); >> >> drm_display_mode_to_videomode(mode, &omap_crtc->vm); >> - omap_crtc->vm.flags |= DISPLAY_FLAGS_DE_HIGH | >> - DISPLAY_FLAGS_PIXDATA_POSEDGE | >> - DISPLAY_FLAGS_SYNC_NEGEDGE; >> + > > Could you add a HACK/FIXME comment to explain how this should be fixed > properly ? I want to make sure we won't forget to remove this hack. I'll add a comment. I don't know exactly how this should be fixed, but at least I can explain the hack there. Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c index 50d2b641c28b..d62c035f050f 100644 --- a/drivers/gpu/drm/omapdrm/omap_connector.c +++ b/drivers/gpu/drm/omapdrm/omap_connector.c @@ -146,8 +146,6 @@ static int omap_connector_mode_valid(struct drm_connector *connector, int r, ret = MODE_BAD; drm_display_mode_to_videomode(mode, &vm); - vm.flags |= DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_PIXDATA_POSEDGE | - DISPLAY_FLAGS_SYNC_NEGEDGE; mode->vrefresh = drm_mode_vrefresh(mode); /* diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 1db96b077ae8..606ef807741a 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -373,6 +373,11 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc *crtc) { struct omap_crtc *omap_crtc = to_omap_crtc(crtc); struct drm_display_mode *mode = &crtc->state->adjusted_mode; + struct omap_drm_private *priv = crtc->dev->dev_private; + const u32 flags_mask = DISPLAY_FLAGS_DE_HIGH | DISPLAY_FLAGS_DE_LOW | + DISPLAY_FLAGS_PIXDATA_POSEDGE | DISPLAY_FLAGS_PIXDATA_NEGEDGE | + DISPLAY_FLAGS_SYNC_POSEDGE | DISPLAY_FLAGS_SYNC_NEGEDGE; + int i; DBG("%s: set mode: %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x", omap_crtc->name, mode->base.id, mode->name, @@ -382,9 +387,26 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc *crtc) mode->type, mode->flags); drm_display_mode_to_videomode(mode, &omap_crtc->vm); - omap_crtc->vm.flags |= DISPLAY_FLAGS_DE_HIGH | - DISPLAY_FLAGS_PIXDATA_POSEDGE | - DISPLAY_FLAGS_SYNC_NEGEDGE; + + for (i = 0; i < priv->num_encoders; ++i) { + struct drm_encoder *encoder = priv->encoders[i]; + + if (encoder->crtc == crtc) { + struct omap_dss_device *dssdev; + + dssdev = omap_encoder_get_dssdev(encoder); + + if (dssdev) { + struct videomode vm = {0}; + + dssdev->driver->get_timings(dssdev, &vm); + + omap_crtc->vm.flags |= vm.flags & flags_mask; + } + + break; + } + } } static int omap_crtc_atomic_check(struct drm_crtc *crtc,
At the moment VSYNC/HSYNC/DE high/low flags set by the panel/encoder drivers get lost when the videotimings are translated to DRM's videomode, as DRM's mode does not have corresponding flags. DRM has bus-flags for this purpose, and while it lacks a few flags at the moment, it should be used here. However, until we rewrite omapdrm's device model, using bus-flags is rather difficult. As a short term fix, this patch makes sure that every time the videomode is set in omap_crtc_mode_set_nofb(), the driver asks for the SYNC/DE flags from the panel/encoder drivers, and thus we get the correct flags into use. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_connector.c | 2 -- drivers/gpu/drm/omapdrm/omap_crtc.c | 28 +++++++++++++++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-)