| Message ID | 1474288063-5315-10-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19/09/16 15:27, Laurent Pinchart wrote: > Instead of conditioning planes update based on the DD manager state, use s/DD/DSS/ Maybe "manager HW state" to highlight that the status is read from a HW register. > the enabled field newly added to the omap_crtc structure. This reduces > the dependency from the DRM layer to the DSS layer. > > The enabled field is a transitory measure, the implementation should use > the CRTC atomic state instead. However, given that CRTCs are currently > not enabled/disabled through their .enable() and .disable() operations > but through a convoluted code paths starting at the associated encoder > operations, there is not clear guarantee that the atomic state always > matches the hardware state. This will be refactored later, at which > point the enabled field will be removed. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v2: > > - Use enabled field in struct omap_crtc instead of CRTC atomic state > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c > index cdcfda31043e..f41a638c8d65 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -40,6 +40,7 @@ struct omap_crtc { > > bool ignore_digit_sync_lost; > > + bool enabled; > bool pending; > wait_queue_head_t pending_wait; > }; > @@ -136,6 +137,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable) > > if (omap_crtc_output[channel]->output_type == OMAP_DISPLAY_TYPE_HDMI) { > dispc_mgr_enable(channel, enable); > + omap_crtc->enabled = enable; > return; > } > > @@ -172,6 +174,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable) > } > > dispc_mgr_enable(channel, enable); > + omap_crtc->enabled = enable; omap_crtc_set_enabled() is only called from omap_crtc_dss_enable() and omap_crtc_dss_disable(). It's probably a bit cleaner to set the omap_crtc->enabled in those functions. Btw, omap_crtc_set_enabled() has a comment: /* Called only from the encoder enable/disable and suspend/resume handlers. */ which doesn't seem to hold true... Tomi
Hi Tomi, On Tuesday 20 Sep 2016 16:44:21 Tomi Valkeinen wrote: > On 19/09/16 15:27, Laurent Pinchart wrote: > > Instead of conditioning planes update based on the DD manager state, use > > s/DD/DSS/ > > Maybe "manager HW state" to highlight that the status is read from a HW > register. I'll change that. > > the enabled field newly added to the omap_crtc structure. This reduces > > the dependency from the DRM layer to the DSS layer. > > > > The enabled field is a transitory measure, the implementation should use > > the CRTC atomic state instead. However, given that CRTCs are currently > > not enabled/disabled through their .enable() and .disable() operations > > but through a convoluted code paths starting at the associated encoder > > operations, there is not clear guarantee that the atomic state always > > matches the hardware state. This will be refactored later, at which > > point the enabled field will be removed. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v2: > > > > - Use enabled field in struct omap_crtc instead of CRTC atomic state > > --- > > > > drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++--------- > > 1 file changed, 13 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > > b/drivers/gpu/drm/omapdrm/omap_crtc.c index cdcfda31043e..f41a638c8d65 > > 100644 > > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > > @@ -40,6 +40,7 @@ struct omap_crtc { > > > > bool ignore_digit_sync_lost; > > > > + bool enabled; > > bool pending; > > wait_queue_head_t pending_wait; > > }; > > @@ -136,6 +137,7 @@ static void omap_crtc_set_enabled(struct drm_crtc > > *crtc, bool enable) > > > > if (omap_crtc_output[channel]->output_type == OMAP_DISPLAY_TYPE_HDMI) > > { > > dispc_mgr_enable(channel, enable); > > + omap_crtc->enabled = enable; > > return; > > } > > > > @@ -172,6 +174,7 @@ static void omap_crtc_set_enabled(struct drm_crtc > > *crtc, bool enable) > > } > > > > dispc_mgr_enable(channel, enable); > > + omap_crtc->enabled = enable; > > omap_crtc_set_enabled() is only called from omap_crtc_dss_enable() and > omap_crtc_dss_disable(). It's probably a bit cleaner to set the > omap_crtc->enabled in those functions. I like keeping the line close to the dispc_mgr_enable() call. If you insist I can move it, it doesn't make a big difference. I hope to refactor all that as part of the panel and encoder rework anyway. > Btw, omap_crtc_set_enabled() has a comment: > > /* Called only from the encoder enable/disable and suspend/resume > handlers. */ > > which doesn't seem to hold true... omap_crtc_dss_(enable|disable) are only called from the encoder drivers, I think that's what the comment tries to capture.
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index cdcfda31043e..f41a638c8d65 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -40,6 +40,7 @@ struct omap_crtc { bool ignore_digit_sync_lost; + bool enabled; bool pending; wait_queue_head_t pending_wait; }; @@ -136,6 +137,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable) if (omap_crtc_output[channel]->output_type == OMAP_DISPLAY_TYPE_HDMI) { dispc_mgr_enable(channel, enable); + omap_crtc->enabled = enable; return; } @@ -172,6 +174,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable) } dispc_mgr_enable(channel, enable); + omap_crtc->enabled = enable; ret = omap_irq_wait(dev, wait, msecs_to_jiffies(100)); if (ret) { @@ -421,18 +424,19 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc, dispc_mgr_set_gamma(omap_crtc->channel, lut, length); } - if (dispc_mgr_is_enabled(omap_crtc->channel)) { + /* Only flush the CRTC if it is currently enabled. */ + if (!omap_crtc->enabled) + return; - DBG("%s: GO", omap_crtc->name); + DBG("%s: GO", omap_crtc->name); - rmb(); - WARN_ON(omap_crtc->pending); - omap_crtc->pending = true; - wmb(); + rmb(); + WARN_ON(omap_crtc->pending); + omap_crtc->pending = true; + wmb(); - dispc_mgr_go(omap_crtc->channel); - omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); - } + dispc_mgr_go(omap_crtc->channel); + omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); } static bool omap_crtc_is_plane_prop(struct drm_device *dev,
Instead of conditioning planes update based on the DD manager state, use the enabled field newly added to the omap_crtc structure. This reduces the dependency from the DRM layer to the DSS layer. The enabled field is a transitory measure, the implementation should use the CRTC atomic state instead. However, given that CRTCs are currently not enabled/disabled through their .enable() and .disable() operations but through a convoluted code paths starting at the associated encoder operations, there is not clear guarantee that the atomic state always matches the hardware state. This will be refactored later, at which point the enabled field will be removed. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v2: - Use enabled field in struct omap_crtc instead of CRTC atomic state --- drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)