Re: [PATCH v3 4/4] drm/imx: add deferred plane disabling

[Lucas Stach <l.stach@pengutronix.de>]

Am Dienstag, den 28.02.2017, 15:18 +0100 schrieb Philipp Zabel:
> The DP (display processor) channel disable code tried to busy wait for
> the DP sync flow end interrupt status bit when disabling the partial
> plane without a full modeset. That never worked reliably, and it was
> disabled completely by the recent "gpu: ipu-v3: remove IRQ dance on DC
> channel disable" patch, causing ipu_wait_interrupt to always time out
> after 50 ms, which in turn would trigger a timeout in
> drm_atomic_helper_wait_for_vblanks.
> 
> This patch changes ipu_plane_atomic_disable to only queue a DP channel
> register update at the next frame boundary and set a flag, which can be
> done without any waiting whatsoever. The imx_drm_atomic_commit_tail then
> calls a new ipu_plane_disable_deferred function that does the actual
> IDMAC teardown of the planes that are flagged for deferred disabling,
> after waiting for the vblank.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> Changes since v2:
>  - Add missing export for ipu_plane_disable_deferred
>  - Check if there are any planes to be disabled and only then wait for
>    vblanks and call the deferred disable function
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/imx/ipuv3-crtc.c   | 22 +++++++++++++++++++++-
>  drivers/gpu/drm/imx/ipuv3-plane.c  | 25 ++++++++++++++++++-------
>  drivers/gpu/drm/imx/ipuv3-plane.h  |  5 +++++
>  drivers/gpu/ipu-v3/ipu-dp.c        |  3 ---
>  5 files changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 0a5e4fbb906bf..94f9c25e1c67b 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -30,6 +30,7 @@
>  #include <video/imx-ipu-v3.h>
>  
>  #include "imx-drm.h"
> +#include "ipuv3-plane.h"
>  
>  #define MAX_CRTC	4
>  
> @@ -160,6 +161,10 @@ static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
>  static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	bool plane_disabling = false;
> +	int i;
>  
>  	drm_atomic_helper_commit_modeset_disables(dev, state);
>  
> @@ -169,6 +174,19 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_commit_modeset_enables(dev, state);
>  
> +	for_each_plane_in_state(state, plane, plane_state, i) {
> +		if (drm_atomic_plane_disabling(plane, plane_state))
> +			plane_disabling = true;
> +	}
> +
> +	if (plane_disabling) {
> +		drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> +		for_each_plane_in_state(state, plane, plane_state, i)
> +			ipu_plane_disable_deferred(plane);
> +
> +	}
> +
>  	drm_atomic_helper_commit_hw_done(state);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 6be515a9fb694..0f15f11f26e0c 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -60,6 +60,26 @@ static void ipu_crtc_enable(struct drm_crtc *crtc)
>  	ipu_di_enable(ipu_crtc->di);
>  }
>  
> +static void ipu_crtc_disable_planes(struct ipu_crtc *ipu_crtc,
> +				    struct drm_crtc_state *old_crtc_state)
> +{
> +	bool disable_partial = false;
> +	bool disable_full = false;
> +	struct drm_plane *plane;
> +
> +	drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) {
> +		if (plane == &ipu_crtc->plane[0]->base)
> +			disable_full = true;
> +		if (&ipu_crtc->plane[1] && plane == &ipu_crtc->plane[1]->base)
> +			disable_partial = true;
> +	}
> +
> +	if (disable_partial)
> +		ipu_plane_disable(ipu_crtc->plane[1], true);
> +	if (disable_full)
> +		ipu_plane_disable(ipu_crtc->plane[0], false);
> +}
> +
>  static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  				    struct drm_crtc_state *old_crtc_state)
>  {
> @@ -73,7 +93,7 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  	 * 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_crtc_disable_planes(ipu_crtc, old_crtc_state);
>  	ipu_dc_disable(ipu);
>  
>  	spin_lock_irq(&crtc->dev->event_lock);
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 55991d46ced50..a37735298615e 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -172,23 +172,30 @@ static void ipu_plane_enable(struct ipu_plane *ipu_plane)
>  		ipu_dp_enable_channel(ipu_plane->dp);
>  }
>  
> -static int ipu_disable_plane(struct drm_plane *plane)
> +void ipu_plane_disable(struct ipu_plane *ipu_plane, bool disable_dp_channel)
>  {
> -	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
> -
>  	DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__);
>  
>  	ipu_idmac_wait_busy(ipu_plane->ipu_ch, 50);
>  
> -	if (ipu_plane->dp)
> -		ipu_dp_disable_channel(ipu_plane->dp, true);
> +	if (ipu_plane->dp && disable_dp_channel)
> +		ipu_dp_disable_channel(ipu_plane->dp, false);
>  	ipu_idmac_disable_channel(ipu_plane->ipu_ch);
>  	ipu_dmfc_disable_channel(ipu_plane->dmfc);
>  	if (ipu_plane->dp)
>  		ipu_dp_disable(ipu_plane->ipu);
> +}
>  
> -	return 0;
> +void ipu_plane_disable_deferred(struct drm_plane *plane)
> +{
> +	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
> +
> +	if (ipu_plane->disabling) {
> +		ipu_plane->disabling = false;
> +		ipu_plane_disable(ipu_plane, false);
> +	}
>  }
> +EXPORT_SYMBOL_GPL(ipu_plane_disable_deferred);
>  
>  static void ipu_plane_destroy(struct drm_plane *plane)
>  {
> @@ -356,7 +363,11 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  static void ipu_plane_atomic_disable(struct drm_plane *plane,
>  				     struct drm_plane_state *old_state)
>  {
> -	ipu_disable_plane(plane);
> +	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
> +
> +	if (ipu_plane->dp)
> +		ipu_dp_disable_channel(ipu_plane->dp, true);
> +	ipu_plane->disabling = true;
>  }
>  
>  static void ipu_plane_atomic_update(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h
> index 338b88a74eb6e..0e2a723ff9816 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.h
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.h
> @@ -23,6 +23,8 @@ struct ipu_plane {
>  
>  	int			dma;
>  	int			dp_flow;
> +
> +	bool			disabling;
>  };
>  
>  struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
> @@ -42,4 +44,7 @@ void ipu_plane_put_resources(struct ipu_plane *plane);
>  
>  int ipu_plane_irq(struct ipu_plane *plane);
>  
> +void ipu_plane_disable(struct ipu_plane *ipu_plane, bool disable_dp_channel);
> +void ipu_plane_disable_deferred(struct drm_plane *plane);
> +
>  #endif
> diff --git a/drivers/gpu/ipu-v3/ipu-dp.c b/drivers/gpu/ipu-v3/ipu-dp.c
> index 0e09c98248a0d..9b2b3fa479c46 100644
> --- a/drivers/gpu/ipu-v3/ipu-dp.c
> +++ b/drivers/gpu/ipu-v3/ipu-dp.c
> @@ -277,9 +277,6 @@ void ipu_dp_disable_channel(struct ipu_dp *dp, bool sync)
>  	writel(0, flow->base + DP_FG_POS);
>  	ipu_srm_dp_update(priv->ipu, sync);
>  
> -	if (ipu_idmac_channel_busy(priv->ipu, IPUV3_CHANNEL_MEM_BG_SYNC))
> -		ipu_wait_interrupt(priv->ipu, IPU_IRQ_DP_SF_END, 50);
> -
>  	mutex_unlock(&priv->mutex);
>  }
>  EXPORT_SYMBOL_GPL(ipu_dp_disable_channel);


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