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 at 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
