Re: [PATCH 3/3] drm/atomic: Use explicit old/new state in drm_atomic_plane_check()

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Nov 01, 2018 at 08:46:46PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Convert drm_atomic_plane_check() over to using explicit old vs. new
> plane states. Avoids the confusion of "what does plane->state mean
> again?".
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 90 ++++++++++++++++++------------------
>  1 file changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index dde696181efe..46f8d798efaf 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -492,108 +492,109 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  EXPORT_SYMBOL(drm_atomic_get_plane_state);
>  
>  static bool
> -plane_switching_crtc(struct drm_atomic_state *state,
> -		     struct drm_plane *plane,
> -		     struct drm_plane_state *plane_state)
> +plane_switching_crtc(const struct drm_plane_state *old_plane_state,
> +		     const struct drm_plane_state *new_plane_state)
>  {
> -	if (!plane->state->crtc || !plane_state->crtc)
> -		return false;
> -
> -	if (plane->state->crtc == plane_state->crtc)
> -		return false;
> -
>  	/* This could be refined, but currently there's no helper or driver code
>  	 * to implement direct switching of active planes nor userspace to take
>  	 * advantage of more direct plane switching without the intermediate
>  	 * full OFF state.
>  	 */
> -	return true;
> +	return old_plane_state->crtc && new_plane_state->crtc &&
> +		old_plane_state->crtc != new_plane_state->crtc;

I think the old layout was clearer, instead of an obscure monster
condition.

With the old layout here this is:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>  }
>  
>  /**
>   * drm_atomic_plane_check - check plane state
> - * @plane: plane to check
> - * @state: plane state to check
> + * @old_plane_state: old plane state to check
> + * @new_plane_state: new plane state to check
>   *
>   * Provides core sanity checks for plane state.
>   *
>   * RETURNS:
>   * Zero on success, error code on failure
>   */
> -static int drm_atomic_plane_check(struct drm_plane *plane,
> -		struct drm_plane_state *state)
> +static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
> +				  const struct drm_plane_state *new_plane_state)
>  {
> +	struct drm_plane *plane = new_plane_state->plane;
> +	struct drm_crtc *crtc = new_plane_state->crtc;
> +	const struct drm_framebuffer *fb = new_plane_state->fb;
>  	unsigned int fb_width, fb_height;
>  	int ret;
>  
>  	/* either *both* CRTC and FB must be set, or neither */
> -	if (state->crtc && !state->fb) {
> +	if (crtc && !fb) {
>  		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] CRTC set but no FB\n",
>  				 plane->base.id, plane->name);
>  		return -EINVAL;
> -	} else if (state->fb && !state->crtc) {
> +	} else if (fb && !crtc) {
>  		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] FB set but no CRTC\n",
>  				 plane->base.id, plane->name);
>  		return -EINVAL;
>  	}
>  
>  	/* if disabled, we don't care about the rest of the state: */
> -	if (!state->crtc)
> +	if (!crtc)
>  		return 0;
>  
>  	/* Check whether this plane is usable on this CRTC */
> -	if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) {
> +	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
>  		DRM_DEBUG_ATOMIC("Invalid [CRTC:%d:%s] for [PLANE:%d:%s]\n",
> -				 state->crtc->base.id, state->crtc->name,
> +				 crtc->base.id, crtc->name,
>  				 plane->base.id, plane->name);
>  		return -EINVAL;
>  	}
>  
>  	/* Check whether this plane supports the fb pixel format. */
> -	ret = drm_plane_check_pixel_format(plane, state->fb->format->format,
> -					   state->fb->modifier);
> +	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> +					   fb->modifier);
>  	if (ret) {
>  		struct drm_format_name_buf format_name;
>  		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid pixel format %s, modifier 0x%llx\n",
>  				 plane->base.id, plane->name,
> -				 drm_get_format_name(state->fb->format->format,
> +				 drm_get_format_name(fb->format->format,
>  						     &format_name),
> -				 state->fb->modifier);
> +				 fb->modifier);
>  		return ret;
>  	}
>  
>  	/* Give drivers some help against integer overflows */
> -	if (state->crtc_w > INT_MAX ||
> -	    state->crtc_x > INT_MAX - (int32_t) state->crtc_w ||
> -	    state->crtc_h > INT_MAX ||
> -	    state->crtc_y > INT_MAX - (int32_t) state->crtc_h) {
> +	if (new_plane_state->crtc_w > INT_MAX ||
> +	    new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w ||
> +	    new_plane_state->crtc_h > INT_MAX ||
> +	    new_plane_state->crtc_y > INT_MAX - (int32_t) new_plane_state->crtc_h) {
>  		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid CRTC coordinates %ux%u+%d+%d\n",
>  				 plane->base.id, plane->name,
> -				 state->crtc_w, state->crtc_h,
> -				 state->crtc_x, state->crtc_y);
> +				 new_plane_state->crtc_w, new_plane_state->crtc_h,
> +				 new_plane_state->crtc_x, new_plane_state->crtc_y);
>  		return -ERANGE;
>  	}
>  
> -	fb_width = state->fb->width << 16;
> -	fb_height = state->fb->height << 16;
> +	fb_width = fb->width << 16;
> +	fb_height = fb->height << 16;
>  
>  	/* Make sure source coordinates are inside the fb. */
> -	if (state->src_w > fb_width ||
> -	    state->src_x > fb_width - state->src_w ||
> -	    state->src_h > fb_height ||
> -	    state->src_y > fb_height - state->src_h) {
> +	if (new_plane_state->src_w > fb_width ||
> +	    new_plane_state->src_x > fb_width - new_plane_state->src_w ||
> +	    new_plane_state->src_h > fb_height ||
> +	    new_plane_state->src_y > fb_height - new_plane_state->src_h) {
>  		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid source coordinates "
>  				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
>  				 plane->base.id, plane->name,
> -				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> -				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> -				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> -				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10,
> -				 state->fb->width, state->fb->height);
> +				 new_plane_state->src_w >> 16,
> +				 ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
> +				 new_plane_state->src_h >> 16,
> +				 ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
> +				 new_plane_state->src_x >> 16,
> +				 ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
> +				 new_plane_state->src_y >> 16,
> +				 ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
> +				 fb->width, fb->height);
>  		return -ENOSPC;
>  	}
>  
> -	if (plane_switching_crtc(state->state, plane, state)) {
> +	if (plane_switching_crtc(old_plane_state, new_plane_state)) {
>  		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n",
>  				 plane->base.id, plane->name);
>  		return -EINVAL;
> @@ -966,7 +967,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_plane *plane;
> -	struct drm_plane_state *plane_state;
> +	struct drm_plane_state *old_plane_state;
> +	struct drm_plane_state *new_plane_state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_state *new_crtc_state;
> @@ -976,8 +978,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  
>  	DRM_DEBUG_ATOMIC("checking %p\n", state);
>  
> -	for_each_new_plane_in_state(state, plane, plane_state, i) {
> -		ret = drm_atomic_plane_check(plane, plane_state);
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> +		ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
>  		if (ret) {
>  			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n",
>  					 plane->base.id, plane->name);
> -- 
> 2.18.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx