Re: [PATCH 1/2] Revert "drm: omapdrm: Let the DRM core skip plane commit on inactive CRTCs"

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

Hi Jyri,

Thank you for the patch.

On Friday 27 Jan 2017 12:04:54 Jyri Sarha wrote:
> This reverts commit dadf4659d0608e034b6633f30300c2eff2dafb4c.
> 
> If planes are not disabled when the they are not on any crtc anymore
> they will remain active and may show as "ghosts" when the crtc they
> were last on is active again.

Sorry for the breakage.

The drm_atomic_helper_commit_planes() helper documentation states

 * Unless otherwise needed, drivers are advised to set the ACTIVE_ONLY flag in
 * @flags in order not to receive plane update notifications related to a
 * disabled CRTC. This avoids the need to manually ignore plane updates in
 * driver code when the driver and/or hardware can't or just don't need to
 * deal with updates on disabled CRTCs, for example when supporting runtime
 * PM.

I wonder what this implies when CRTCs are being disabled. I see very few cases 
where the hardware wouldn't need the plane atomic disable operation being 
called when a plane is being disabled due to its CRTC being disabled. Maybe 
this should thus be addressed in the core. Daniel, any comment on this ?

> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 8 +-------
>  drivers/gpu/drm/omapdrm/omap_drv.c  | 3 +--
>  2 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index dd47dc1..b68c70e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -410,13 +410,7 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> *crtc, dispc_mgr_set_gamma(omap_crtc->channel, lut, length);
>  	}
> 
> -	/*
> -	 * Only flush the CRTC if it is currently enabled. CRTCs that require 
a
> -	 * mode set are disabled prior plane updates and enabled afterwards.
> -	 * They are thus not active (regardless of what their CRTC core state
> -	 * reports) and the DRM core could thus call this function even though
> -	 * the CRTC is currently disabled. Do nothing in that case.
> -	 */
> +	/* Only flush the CRTC if it is currently enabled. */
>  	if (!omap_crtc->enabled)
>  		return;
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> b/drivers/gpu/drm/omapdrm/omap_drv.c index 00aa214..5e55f1b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -96,8 +96,7 @@ static void omap_atomic_complete(struct
> omap_atomic_state_commit *commit) dispc_runtime_get();
> 
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> -	drm_atomic_helper_commit_planes(dev, old_state,
> -					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> +	drm_atomic_helper_commit_planes(dev, old_state, 0);
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> 
>  	omap_atomic_wait_for_completion(dev, old_state);

-- 
Regards,

Laurent Pinchart

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