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

[Daniel Vetter]

On Mon, Aug 15, 2016 at 02:09:13PM +0800, Liu Ying wrote:
> We don't support configuring active primary 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.
> For overlay plane, we currently reject active plane reconfiguration.
> This patch adds active plane reconfiguration support by forcing CRTC
> mode change and disabling-enabling plane in plane's ->atomic_update
> callback.

Why do you reject this for overlays? Userspace can always indicate whether
it wants a full modeset or not through the atomic ALLOW_MODESET flag.
Please don't create such special cases in atomic drivers, but instead
export the full hw capabilities to userspace, and let userspace decide
what it wants.

Note that for legacy interfaces there's no problem at all, we should set
allow_modeset correctly for all of the legacy ioctl -> atomic helpers.
-Daniel

> 
> 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>
> Signed-off-by: Liu Ying <gnuiyl at gmail.com>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 26 +++++++++++++++++++++-
>  drivers/gpu/drm/imx/ipuv3-plane.c  | 44 +++++++++++++++++++++++++++++---------
>  2 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 6dc0ef4..c10c3ea 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -147,10 +147,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-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 4ad67d0..d4dde4a 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -319,13 +319,21 @@ 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
> +	 * For primary plane, we support resizing active plane or changing
> +	 * its format by forcing CRTC mode change and disabling-enabling plane
> +	 * in plane's ->atomic_update callback.
> +	 * For overlay plane, we currently reject the active plane resizing
> +	 * or format change.
>  	 */
>  	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;
> +			      fb->pixel_format != old_fb->pixel_format)) {
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> +			crtc_state->mode_changed = true;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
>  
>  	eba = drm_plane_state_to_eba(state);
>  
> @@ -335,8 +343,13 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  	if (fb->pitches[0] < 1 || fb->pitches[0] > 16384)
>  		return -EINVAL;
>  
> -	if (old_fb && fb->pitches[0] != old_fb->pitches[0])
> -		return -EINVAL;
> +	if (old_fb && fb->pitches[0] != old_fb->pitches[0]) {
> +		if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> +			crtc_state->mode_changed = true;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
>  
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_YUV420:
> @@ -371,8 +384,13 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  		if (fb->pitches[1] < 1 || fb->pitches[1] > 16384)
>  			return -EINVAL;
>  
> -		if (old_fb && old_fb->pitches[1] != fb->pitches[1])
> -			return -EINVAL;
> +		if (old_fb && old_fb->pitches[1] != fb->pitches[1]) {
> +			if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> +				crtc_state->mode_changed = true;
> +			} else {
> +				return -EINVAL;
> +			}
> +		}
>  	}
>  
>  	return 0;
> @@ -392,8 +410,14 @@ 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;
> +		}
> +
> +		ipu_disable_plane(plane);

Most likely you want to put a call to disable all planes into you're
crtc->disable function. This here smells like a hack. But I don't know imx
hw enough to be able to tell.
-Daniel

>  	}
>  
>  	switch (ipu_plane->dp_flow) {
> -- 
> 2.7.4
> 

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