Re: [PATCH 1/2] drm/imx: disable planes before DC

[Philipp Zabel <p.zabel@pengutronix.de>]

Am Dienstag, den 08.11.2016, 17:04 +0100 schrieb Lucas Stach:
> If the DC clock is disabled before the attached IDMACs are properly
> stopped the IDMACs may hang the IPU or even the whole system.
> 
> Make sure the IDMACs are in safe state by disabling the planes before
> removal of the DC clock.

This patch not only moves disable_planes into the right place, it also
changes the atomic parameter to false to avoid calling atomic_begin.

Since ipu_crtc_atomic_begin only calls drm_crtc_vblank_on and possibly
drm_crtc_arm_vblank_event, but we disable the IDMAC channels that could
be the source for the vblank interrupts immediately afterwards, I think
this is the right thing to do. I'll amend the commit message as follows:

"Also set the atomic parameter to false to stop calling the atomic_begin
 hook, which does nothing useful as we immediately afterwards turn off
 vblank interrupts and possibly send the pending vblank event."

> Fixes: 33f14235302f (drm/imx: atomic phase 1: Use transitional atomic
>                      CRTC and plane helpers)
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/ipuv3-crtc.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 4e1ae3fc462d..6be515a9fb69 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -68,6 +68,12 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>  	ipu_dc_disable_channel(ipu_crtc->dc);
>  	ipu_di_disable(ipu_crtc->di);
> +	/*
> +	 * Planes must be disabled before DC clock is removed, as otherwise the
> +	 * attached IDMACs will be left in undefined state, possibly hanging
> +	 * the IPU or even system.
> +	 */
> +	drm_atomic_helper_disable_planes_on_crtc(old_crtc_state, false);
>  	ipu_dc_disable(ipu);
>  
>  	spin_lock_irq(&crtc->dev->event_lock);
> @@ -77,9 +83,6 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  	}
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  
> -	/* always disable planes on the CRTC */
> -	drm_atomic_helper_disable_planes_on_crtc(old_crtc_state, true);
> -
>  	drm_crtc_vblank_off(crtc);
>  }
>  

regards
Philipp

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