Re: [PATCH v2] drm: Don't force all planes to be added to the state due to zpos

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 10, 2016 at 05:50:56PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> We don't want all planes to be added to the state whenever a
> plane with fixed zpos gets enabled/disabled. This is true
> especially for eg. cursor planes on i915, as we want cursor
> updates to go through w/o throttling. Same holds for drivers
> that don't support zpos at all (i915 actually falls into this
> category right now since we've not yet added zpos support).
> 
> Allow drivers more freedom by letting them deal with zpos
> themselves instead of doing it in drm_atomic_helper_check_planes()
> unconditionally. Let's just inline the required calls into all
> the driver that currently depend on this.
> 
> v2: Inline the stuff into the drivers instead of adding another
>     helper, document things better (Daniel)
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 44d1240d006c ("drm: add generic zpos property")
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c     |  4 ----
>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_drv.h |  1 +
>  drivers/gpu/drm/exynos/exynos_drm_fb.c  |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 12 ++++++++++--
>  drivers/gpu/drm/sti/sti_drv.c           | 22 +++++++++++++++++++++-
>  include/drm/drm_plane.h                 |  8 +++++++-
>  7 files changed, 60 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c3f83476f996..21f992605541 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -594,10 +594,6 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  	struct drm_plane_state *plane_state;
>  	int i, ret = 0;
>  
> -	ret = drm_atomic_normalize_zpos(dev, state);
> -	if (ret)
> -		return ret;
> -
>  	for_each_plane_in_state(state, plane, plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index def78c8c1780..f86e7c846678 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -262,6 +262,26 @@ int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state,
>  	return 0;
>  }
>  
> +int exynos_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_normalize_zpos(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_atomic_helper_check_planes(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}
> +
>  static int exynos_drm_open(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct drm_exynos_file_private *file_priv;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index d215149e737b..80c4d5b81689 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -301,6 +301,7 @@ static inline int exynos_dpi_bind(struct drm_device *dev,
>  
>  int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state,
>  			 bool nonblock);
> +int exynos_atomic_check(struct drm_device *dev, struct drm_atomic_state *state);
>  
>  
>  extern struct platform_driver fimd_driver;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 40ce841eb952..23cce0a3f5fc 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -190,7 +190,7 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
>  static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
>  	.fb_create = exynos_user_fb_create,
>  	.output_poll_changed = exynos_drm_output_poll_changed,
> -	.atomic_check = drm_atomic_helper_check,
> +	.atomic_check = exynos_atomic_check,
>  	.atomic_commit = exynos_atomic_commit,
>  };
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index bd9c3bb9252c..392c7e6de042 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -231,8 +231,16 @@ static int rcar_du_atomic_check(struct drm_device *dev,
>  	struct rcar_du_device *rcdu = dev->dev_private;
>  	int ret;
>  
> -	ret = drm_atomic_helper_check(dev, state);
> -	if (ret < 0)
> +	ret = drm_atomic_helper_check_modeset(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_atomic_normalize_zpos(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_atomic_helper_check_planes(dev, state);
> +	if (ret)
>  		return ret;
>  
>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
> index 2784919a7366..9df308565f6c 100644
> --- a/drivers/gpu/drm/sti/sti_drv.c
> +++ b/drivers/gpu/drm/sti/sti_drv.c
> @@ -195,6 +195,26 @@ static void sti_atomic_work(struct work_struct *work)
>  	sti_atomic_complete(private, private->commit.state);
>  }
>  
> +static int sti_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_normalize_zpos(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_atomic_helper_check_planes(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}
> +
>  static int sti_atomic_commit(struct drm_device *drm,
>  			     struct drm_atomic_state *state, bool nonblock)
>  {
> @@ -248,7 +268,7 @@ static void sti_output_poll_changed(struct drm_device *ddev)
>  static const struct drm_mode_config_funcs sti_mode_config_funcs = {
>  	.fb_create = drm_fb_cma_create,
>  	.output_poll_changed = sti_output_poll_changed,
> -	.atomic_check = drm_atomic_helper_check,
> +	.atomic_check = sti_atomic_check,
>  	.atomic_commit = sti_atomic_commit,
>  };
>  
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 43cf193e54d6..8b4dc62470ff 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -47,8 +47,14 @@ struct drm_crtc;
>   * @src_h: height of visible portion of plane (in 16.16)
>   * @rotation: rotation of the plane
>   * @zpos: priority of the given plane on crtc (optional)
> + *	Note that multiple active planes on the same crtc can have an identical
> + *	zpos value. The rule to solving the conflict is to compare the plane
> + *	object IDs; the plane with a higher ID must be stacked on top of a
> + *	plane with a lower ID.
>   * @normalized_zpos: normalized value of zpos: unique, range from 0 to N-1
> - *	where N is the number of active planes for given crtc
> + *	where N is the number of active planes for given crtc. Note that
> + *	the driver must call drm_atomic_normalize_zpos() to update this before
> + *	it can be trusted.

Hm, for longer kerneldoc for struct members I prefer the in-line layout.
More readable overall, and better grouped. Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'll pick it up once driver maintainers had a bit of time to ack it.
-Daniel

>   * @src: clipped source coordinates of the plane (in 16.16)
>   * @dst: clipped destination coordinates of the plane
>   * @visible: visibility of the plane
> -- 
> 2.7.4
> 

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