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 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);
> >  }
> >  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. 

> If you remove the check, then I think the driver should make sure that
> omap_crtc_set_enabled() is not called if the output is already
> enabled/disabled. Maybe that can be done in
> omap_crtc_dss_enable/disable, using the new enabled flag.

The situation isn't supposed to happen, would you be more comfortable still 
checking in the enable/disable handlers ? If so I propose using a WARN_ON() to 
catch those impossible situations.

-- 
Regards,

Laurent Pinchart

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