Re: [PATCH v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

On Tuesday 13 Dec 2016 10:15:35 Tomi Valkeinen wrote:
> On 13/12/16 01:07, Laurent Pinchart wrote:
> > On Tuesday 20 Sep 2016 16:57:59 Tomi Valkeinen wrote:
> >> On 19/09/16 15:27, Laurent Pinchart wrote:
> >>> The omapdrm DSS manager enable/disable operations check the DSS manager
> >>> state to avoid double enabling/disabling. Move that code to the DSS
> >>> manager to decrease the dependency of the DRM layer to the DSS layer.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>>  drivers/gpu/drm/omapdrm/dss/dispc.c  | 1 -
> >>>  drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++
> >>>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 3 ---
> >>>  3 files changed, 6 insertions(+), 4 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >>> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 535240fba671..ab150bf21dd8
> >>> 100644
> >>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >>> @@ -2911,7 +2911,6 @@ bool dispc_mgr_is_enabled(enum omap_channel
> >>> channel)
> >>>  {
> >>>  	return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
> >>>  }
> >>> -EXPORT_SYMBOL(dispc_mgr_is_enabled);
> >>> 
> >>>  void dispc_wb_enable(bool enable)
> >>>  {
> >>> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c
> >>> b/drivers/gpu/drm/omapdrm/dss/output.c index 24f859488201..f0be621895fa
> >>> 100644
> >>> --- a/drivers/gpu/drm/omapdrm/dss/output.c
> >>> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
> >>> @@ -217,12 +217,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config);
> >>>  int dss_mgr_enable(enum omap_channel channel)
> >>>  {
> >>> +	if (dispc_mgr_is_enabled(channel))
> >>> +		return 0;
> >>> +
> >>>  	return dss_mgr_ops->enable(channel);
> >>>  }
> 
> Looking at this again, I think this is not correct. The functions in
> output.c are just trampolines basically. They just pass the calls to
> omapdrm. Let's not add any logic there.

I can't wait to clean all this up and get rid of them :-)

> Also, the above could even cause a crash, as the HW is not necessarily
> enabled before the ->enable() is called.
> 
> >>>  EXPORT_SYMBOL(dss_mgr_enable);
> >>>  
> >>>  void dss_mgr_disable(enum omap_channel channel)
> >>>  {
> >>> +	if (!dispc_mgr_is_enabled(channel))
> >>> +		return;
> >>> +
> >>>  	dss_mgr_ops->disable(channel);
> >>>  }
> >>>  EXPORT_SYMBOL(dss_mgr_disable);
> >>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 4b7e16786e1e..a0c26592fc69
> >>> 100644
> >>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> @@ -141,9 +141,6 @@ static void omap_crtc_set_enabled(struct drm_crtc
> >>> *crtc, bool enable)
> >>>  		return;
> >>>  	}
> >>> 
> >>> -	if (dispc_mgr_is_enabled(channel) == enable)
> >>> -		return;
> >>> -
> >>>  	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
> >>>  		/*
> >>>  		 * Digit output produces some sync lost interrupts during the
> >> 
> >> With this change omap_crtc_set_enabled() will do extra work if the
> >> output is already enabled/disabled, and, if I'm not mistaken, will do
> >> omap_irq_wait() there which might lead to issues.
> > 
> > That's correct, but the DRM core nowadays should really guarantee that
> > CRTCs won't be enabled or disabled multiple times.
> 
> Ok. So we could probably add a dev_warn() there, as getting that state
> wrong might cause issues that are difficult to debug. It would be best
> to check the HW state, but if you want to remove the call to dispc, I
> guess just checking omap_crtc->enabled is good enough.

I'll do that. I don't like reading back the hardware state when it's supposed 
to match the software state. If there's a mismatch then there's a bug, and if 
they always match, it's pointless.

> The comment for omap_crtc_set_enabled talks about suspend/resume.
> Perhaps at some point it was called from suspend/resume, even for crtcs
> that were already disabled, which would explain the need for the check.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel