[PATCH v3 3/3] drm/imx: Add active plane reconfiguration support

[Daniel Vetter]

On Fri, Aug 19, 2016 at 05:36:59PM +0800, Liu Ying wrote:
> We don't support configuring active plane on-the-fly for imx-drm.
> The relevant CRTC should be disabled before the plane configuration.
> Of course, the plane itself should be disabled as well.
> 
> This patch adds active plane reconfiguration support by forcing CRTC
> mode change in plane's ->atomic_check callback and adding disable
> operation for all appropriate active planes(when necessary) in CRTC's
> ->atomic_disable callback.  The atomic core would reenabled the
> affected CRTC and planes at atomic commit stage.
> 
> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Philipp Zabel <p.zabel at pengutronix.de>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Russell King <linux at armlinux.org.uk>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Peter Senna Tschudin <peter.senna at gmail.com>
> Cc: Lucas Stach <l.stach at pengutronix.de>
> Signed-off-by: Liu Ying <gnuiyl at gmail.com>
> ---
> v2->v3:
> * Disable all appropriate affected planes(when necessary) in CRTC's
>   ->atomic_disable callback, but not in each plane's ->atomic_update callback,
>   as suggested by Daniel Vetter.
> * +Cc Lucas Stach, as he tested the patch v2.
> 
> v1->v2:
> * Do not reject reconfiguring an active overlay plane.
> 
>  drivers/gpu/drm/imx/imx-drm-core.c | 26 +++++++++++++++++++++++++-
>  drivers/gpu/drm/imx/ipuv3-crtc.c   | 26 ++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/ipuv3-plane.c  | 21 ++++++++++++++-------
>  3 files changed, 65 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 438bac8..d61b048 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -146,10 +146,34 @@ static void imx_drm_output_poll_changed(struct drm_device *drm)
>  	drm_fbdev_cma_hotplug_event(imxdrm->fbhelper);
>  }
>  
> +static int imx_drm_atomic_check(struct drm_device *dev,
> +				struct drm_atomic_state *state)
> +{
> +	int ret;
> +
> +	ret = drm_atomic_helper_check_modeset(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_atomic_helper_check_planes(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Check modeset again in case crtc_state->mode_changed is
> +	 * updated in plane's ->atomic_check callback.
> +	 */
> +	ret = drm_atomic_helper_check_modeset(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}
> +
>  static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
>  	.fb_create = drm_fb_cma_create,
>  	.output_poll_changed = imx_drm_output_poll_changed,
> -	.atomic_check = drm_atomic_helper_check,
> +	.atomic_check = imx_drm_atomic_check,
>  	.atomic_commit = drm_atomic_helper_commit,
>  };
>  
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 83c46bd..0779eed 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -76,6 +76,32 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
>  		crtc->state->event = NULL;
>  	}
>  	spin_unlock_irq(&crtc->dev->event_lock);
> +
> +	/*
> +	 * The relevant plane's ->atomic_check callback may set
> +	 * crtc_state->mode_changed to be true when the active
> +	 * plane needs to be reconfigured.  In this case and only
> +	 * this case, active_changed is false - we disable all the
> +	 * appropriate active planes here.
> +	 */
> +	if (!crtc->state->active_changed) {
> +		struct drm_plane *plane;
> +
> +		drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) {
> +			const struct drm_plane_helper_funcs *plane_funcs =
> +				plane->helper_private;
> +
> +			/*
> +			 * Filter out the plane which is explicitly required
> +			 * to be disabled by the user via atomic commit
> +			 * so that it won't be accidentally disabled twice.
> +			 */
> +			if (!plane->state->crtc)
> +				continue;

I think the better place would be to do the filtering in commit_planes(),
not here. And would be great to include your patch to fix up
disable_planes_on_crtc(), too.
-Daniel

> +
> +			plane_funcs->atomic_disable(plane, NULL);
> +		}
> +	}
>  }
>  
>  static void imx_drm_crtc_reset(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 4ad67d0..6063ebe 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -319,13 +319,16 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  		return -EINVAL;
>  
>  	/*
> -	 * since we cannot touch active IDMAC channels, we do not support
> -	 * resizing the enabled plane or changing its format
> +	 * We support resizing active plane or changing its format by
> +	 * forcing CRTC mode change in plane's ->atomic_check callback
> +	 * and disabling all appropriate active planes in CRTC's
> +	 * ->atomic_disable callback.  The planes will be reenabled in
> +	 * plane's ->atomic_update callback.
>  	 */
>  	if (old_fb && (state->src_w != old_state->src_w ||
>  			      state->src_h != old_state->src_h ||
>  			      fb->pixel_format != old_fb->pixel_format))
> -		return -EINVAL;
> +		crtc_state->mode_changed = true;
>  
>  	eba = drm_plane_state_to_eba(state);
>  
> @@ -336,7 +339,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  		return -EINVAL;
>  
>  	if (old_fb && fb->pitches[0] != old_fb->pitches[0])
> -		return -EINVAL;
> +		crtc_state->mode_changed = true;
>  
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_YUV420:
> @@ -372,7 +375,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  			return -EINVAL;
>  
>  		if (old_fb && old_fb->pitches[1] != fb->pitches[1])
> -			return -EINVAL;
> +			crtc_state->mode_changed = true;
>  	}
>  
>  	return 0;
> @@ -392,8 +395,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  	enum ipu_color_space ics;
>  
>  	if (old_state->fb) {
> -		ipu_plane_atomic_set_base(ipu_plane, old_state);
> -		return;
> +		struct drm_crtc_state *crtc_state = state->crtc->state;
> +
> +		if (!crtc_state->mode_changed) {
> +			ipu_plane_atomic_set_base(ipu_plane, old_state);
> +			return;
> +		}
>  	}
>  
>  	switch (ipu_plane->dp_flow) {
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch