Re: [PATCH v3 09/20] drm: omapdrm: Replace DSS manager state check with omapdrm CRTC state

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

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.

-- 
Regards,

Laurent Pinchart

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