Re: [PATCH v3 06/11] drm: Use state helper instead of plane state pointer in atomic_check

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 9118 bytes --]



Am 19.02.21 um 13:00 schrieb Maxime Ripard:
> Many drivers reference the plane->state pointer in order to get the
> current plane state in their atomic_check hook, which would be the old
> plane state in the global atomic state since _swap_state hasn't happened
> when atomic_check is run.
> 
> Use the drm_atomic_get_old_plane_state helper to get that state to make
> it more obvious.
> 
> This was made using the coccinelle script below:
> 
> @ plane_atomic_func @
> identifier helpers;
> identifier func;
> @@
> 
> static struct drm_plane_helper_funcs helpers = {
> 	...,
> 	.atomic_check = func,
> 	...,
> };
> 
> @ replaces_old_state @
> identifier plane_atomic_func.func;
> identifier plane, state, plane_state;
> @@
> 
>   func(struct drm_plane *plane, struct drm_atomic_state *state) {
>   	...
> -	struct drm_plane_state *plane_state = plane->state;
> +	struct drm_plane_state *plane_state = drm_atomic_get_old_plane_state(state, plane);
>   	...
>   }
> 
> @@
> identifier plane_atomic_func.func;
> identifier plane, state, plane_state;
> @@
> 
>   func(struct drm_plane *plane, struct drm_atomic_state *state) {
>   	struct drm_plane_state *plane_state = drm_atomic_get_old_plane_state(state, plane);
>   	<...
> -	plane->state
> +	plane_state
>   	...>
>   }
> 
> @ adds_old_state @
> identifier plane_atomic_func.func;
> identifier plane, state;
> @@
> 
>   func(struct drm_plane *plane, struct drm_atomic_state *state) {
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>   	<...
> -	plane->state
> +	old_plane_state
>   	...>
>   }
> 
> @ include depends on adds_old_state || replaces_old_state @
> @@
> 
>   #include <drm/drm_atomic.h>
> 
> @ no_include depends on !include && (adds_old_state || replaces_old_state) @
> @@
> 
> + #include <drm/drm_atomic.h>
>    #include <drm/...>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

However, I find 'old plane state' somewhat confusing in this context, 
because it's actually the current plane state. Would it make sense to 
use drm_atomic_get_existing_plane_state() instead?

Best regards
Thomas

> 
> ---
> 
> Changes from v2:
>    - s/.../<.../ in the coccinelle script as suggested by Ville
> ---
>   drivers/gpu/drm/imx/ipuv3-plane.c          |  3 ++-
>   drivers/gpu/drm/ingenic/ingenic-drm-drv.c  | 16 +++++++++-------
>   drivers/gpu/drm/ingenic/ingenic-ipu.c      |  8 +++++---
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c |  4 +++-
>   drivers/gpu/drm/tilcdc/tilcdc_plane.c      |  3 ++-
>   5 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index b5f6123850bb..6484592e3f86 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -341,7 +341,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>   {
>   	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
>   									   plane);
> -	struct drm_plane_state *old_state = plane->state;
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
> +									   plane);
>   	struct drm_crtc_state *crtc_state;
>   	struct device *dev = plane->dev->dev;
>   	struct drm_framebuffer *fb = new_state->fb;
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index e6d7d0a04ddb..c022d9f1e737 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -361,11 +361,13 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>   static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>   					  struct drm_atomic_state *state)
>   {
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state,
> +										 plane);
>   	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>   										 plane);
>   	struct ingenic_drm *priv = drm_device_get_priv(plane->dev);
>   	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc *crtc = new_plane_state->crtc ?: plane->state->crtc;
> +	struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
>   	int ret;
>   
>   	if (!crtc)
> @@ -399,12 +401,12 @@ static int ingenic_drm_plane_atomic_check(struct drm_plane *plane,
>   	 * its position, size or depth.
>   	 */
>   	if (priv->soc_info->has_osd &&
> -	    (!plane->state->fb || !new_plane_state->fb ||
> -	     plane->state->crtc_x != new_plane_state->crtc_x ||
> -	     plane->state->crtc_y != new_plane_state->crtc_y ||
> -	     plane->state->crtc_w != new_plane_state->crtc_w ||
> -	     plane->state->crtc_h != new_plane_state->crtc_h ||
> -	     plane->state->fb->format->format != new_plane_state->fb->format->format))
> +	    (!old_plane_state->fb || !new_plane_state->fb ||
> +	     old_plane_state->crtc_x != new_plane_state->crtc_x ||
> +	     old_plane_state->crtc_y != new_plane_state->crtc_y ||
> +	     old_plane_state->crtc_w != new_plane_state->crtc_w ||
> +	     old_plane_state->crtc_h != new_plane_state->crtc_h ||
> +	     old_plane_state->fb->format->format != new_plane_state->fb->format->format))
>   		crtc_state->mode_changed = true;
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/ingenic/ingenic-ipu.c b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> index 1e1b0fe095bd..3d072b25e1c4 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-ipu.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-ipu.c
> @@ -516,11 +516,13 @@ static void ingenic_ipu_plane_atomic_update(struct drm_plane *plane,
>   static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>   					  struct drm_atomic_state *state)
>   {
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state,
> +										 plane);
>   	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>   										 plane);
>   	unsigned int num_w, denom_w, num_h, denom_h, xres, yres, max_w, max_h;
>   	struct ingenic_ipu *ipu = plane_to_ingenic_ipu(plane);
> -	struct drm_crtc *crtc = new_plane_state->crtc ?: plane->state->crtc;
> +	struct drm_crtc *crtc = new_plane_state->crtc ?: old_plane_state->crtc;
>   	struct drm_crtc_state *crtc_state;
>   
>   	if (!crtc)
> @@ -531,7 +533,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>   		return -EINVAL;
>   
>   	/* Request a full modeset if we are enabling or disabling the IPU. */
> -	if (!plane->state->crtc ^ !new_plane_state->crtc)
> +	if (!old_plane_state->crtc ^ !new_plane_state->crtc)
>   		crtc_state->mode_changed = true;
>   
>   	if (!new_plane_state->crtc ||
> @@ -552,7 +554,7 @@ static int ingenic_ipu_plane_atomic_check(struct drm_plane *plane,
>   	if (((new_plane_state->src_w >> 16) & 1) || (new_plane_state->crtc_w & 1))
>   		return -EINVAL;
>   
> -	if (!osd_changed(new_plane_state, plane->state))
> +	if (!osd_changed(new_plane_state, old_plane_state))
>   		return 0;
>   
>   	crtc_state->mode_changed = true;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index 4aac6217a5ad..6ce6ce09fecc 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -406,12 +406,14 @@ static int mdp5_plane_atomic_check_with_state(struct drm_crtc_state *crtc_state,
>   static int mdp5_plane_atomic_check(struct drm_plane *plane,
>   				   struct drm_atomic_state *state)
>   {
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state,
> +										 plane);
>   	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
>   										 plane);
>   	struct drm_crtc *crtc;
>   	struct drm_crtc_state *crtc_state;
>   
> -	crtc = new_plane_state->crtc ? new_plane_state->crtc : plane->state->crtc;
> +	crtc = new_plane_state->crtc ? new_plane_state->crtc : old_plane_state->crtc;
>   	if (!crtc)
>   		return 0;
>   
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_plane.c b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> index ebdd42dcaf82..c86258132432 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_plane.c
> @@ -26,7 +26,8 @@ static int tilcdc_plane_atomic_check(struct drm_plane *plane,
>   	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
>   									   plane);
>   	struct drm_crtc_state *crtc_state;
> -	struct drm_plane_state *old_state = plane->state;
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
> +									   plane);
>   	unsigned int pitch;
>   
>   	if (!new_state->crtc)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]