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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 10, 2016 at 03:19:47PM +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. Easiest solution seems to be to move the call
> up to drm_atomic_helper_check(). But as some drivers might want
> to use that function without the zpos handling, let's provide
> two variants: the normal one, and one that deals with zpos.
> 
> 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>

Ping. Can I get some buy-in from the relevant folks?

> ---
>  drivers/gpu/drm/drm_atomic_helper.c    | 46 +++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>  drivers/gpu/drm/sti/sti_drv.c          |  2 +-
>  include/drm/drm_atomic_helper.h        |  2 ++
>  5 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c3f83476f996..cd19281cdefb 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;
>  
> @@ -673,6 +669,48 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check);
>  
> +/**
> + * drm_atomic_helper_check_with_zpos - validate state object, dealing with zpos
> + * @dev: DRM device
> + * @state: the driver state object
> + *
> + * Check the state object to see if the requested state is physically possible.
> + * Only crtcs and planes have check callbacks, so for any additional (global)
> + * checking that a driver needs it can simply wrap that around this function.
> + * Drivers without such needs can directly use this as their ->atomic_check()
> + * callback.
> + *
> + * This just wraps the two parts of the state checking for planes and modeset
> + * state in the default order: First it calls drm_atomic_helper_check_modeset(),
> + * followed by drm_atomic_normalize_zpos(), and finally
> + * drm_atomic_helper_check_planes(). The assumption is that the
> + * ->atomic_check functions depend upon an updated adjusted_mode.clock to
> + * e.g. properly compute watermarks.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_check_with_zpos(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;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_with_zpos);
> +
>  static void
>  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 40ce841eb952..5c0930af01fa 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 = drm_atomic_helper_check_with_zpos,
>  	.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..2cfd35f3f2f6 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -231,7 +231,7 @@ 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);
> +	ret = drm_atomic_helper_check_with_zpos(dev, state);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
> index 2784919a7366..df5f150021d0 100644
> --- a/drivers/gpu/drm/sti/sti_drv.c
> +++ b/drivers/gpu/drm/sti/sti_drv.c
> @@ -248,7 +248,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 = drm_atomic_helper_check_with_zpos,
>  	.atomic_commit = sti_atomic_commit,
>  };
>  
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 7ff92b09fd9c..b1e7193c9d1c 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -40,6 +40,8 @@ int drm_atomic_helper_check_planes(struct drm_device *dev,
>  			       struct drm_atomic_state *state);
>  int drm_atomic_helper_check(struct drm_device *dev,
>  			    struct drm_atomic_state *state);
> +int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
> +				      struct drm_atomic_state *state);
>  void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
>  int drm_atomic_helper_commit(struct drm_device *dev,
>  			     struct drm_atomic_state *state,
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC