Re: [PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

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

Hi Tomi and Daniel,

On Wednesday 11 May 2016 09:37:56 Daniel Vetter wrote:
> On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote:
> > On 26/04/16 23:35, Laurent Pinchart wrote:
> >> Instead of conditioning planes update based on the hardware device
> >> state, use the CRTC state stored in the atomic state. This reduces the
> >> dependency from the DRM layer to the DSS layer.
> >> 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++---------
> >>  1 file changed, 14 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6359d7933b93..4c56d6a68390
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> >> *crtc,
> >>  	WARN_ON(omap_crtc->vblank_irq.registered);
> >> 
> >> -	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> >> +	/*
> >> +	 * Only flush the CRTC if it is currently active. CRTCs that require a
> >> +	 * mode set are disabled prior plane updates and enabled afterwards.
> >> +	 * They are thus not active, regardless of what their state report.
> >> +	 */
> >> +	if (!crtc->state->active ||
> >> drm_atomic_crtc_needs_modeset(crtc->state))
> >> +		return;
> > 
> > If the DRM core doesn't track whether a CRTC HW is enabled at the
> > moment, maybe omapdrm should? I guess the above works, but that if()
> > makes me a bit uneasy, as it's not really obvious, and the logic behind
> > it could possibly change later...
> > 
> > A "if (crtc->is_hw_enabled)" would be much more readable.

The whole point of this patch is to remove local state and rely on DRM core 
state, so I'd like to avoid that if possible.

> Look at the active_only paramater of the planes_commit helper. That should
> do exactly what you want.

The drm_atomic_helper_commit_planes() helper has this check

        if (active_only && !crtc->state->active)
                continue;

        funcs->atomic_flush(crtc, old_crtc_state);

I could thus remove the !crtc->state->active check, but the 
drm_atomic_crtc_needs_modeset() check would need to stay, wouldn't it ? When 
CRTCs go through a modeset they are disabled prior to plane updates, but their 
state active status can still be true. Or should that be fixed in the core ?

-- 
Regards,

Laurent Pinchart

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