Re: [PATCH 2/3] drm: Introduce __setplane_atomic()

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 28, 2018 at 04:54:56PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> All the plane->fb/old_fb/crtc dance of __setplane_internal() is
> pointless on atomic drivers. So let's just introduce a simpler
> version that skips all that.
> 
> Ideally we could also skip the __setplane_check() as
> drm_atomic_plane_check() already checks for everything, but the
> legacy cursor/"async" .update_plane() tricks bypass that so
> we still need to call __setplane_check(). Toss in a FIXME to
> remind someone to clean this up later.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 68 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 57 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 5c97a0131484..58702340808c 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -649,6 +649,8 @@ static int __setplane_internal(struct drm_plane *plane,
>  {
>  	int ret = 0;
>  
> +	WARN_ON(plane->state);
> +
>  	/* No fb means shut it down */
>  	if (!fb) {
>  		plane->old_fb = plane->fb;
> @@ -673,11 +675,9 @@ static int __setplane_internal(struct drm_plane *plane,
>  					 crtc_x, crtc_y, crtc_w, crtc_h,
>  					 src_x, src_y, src_w, src_h, ctx);
>  	if (!ret) {
> -		if (!plane->state) {
> -			plane->crtc = crtc;
> -			plane->fb = fb;
> -			drm_framebuffer_get(plane->fb);
> -		}
> +		plane->crtc = crtc;
> +		plane->fb = fb;
> +		drm_framebuffer_get(plane->fb);
>  	} else {
>  		plane->old_fb = NULL;
>  	}
> @@ -690,6 +690,41 @@ static int __setplane_internal(struct drm_plane *plane,
>  	return ret;
>  }
>  
> +static int __setplane_atomic(struct drm_plane *plane,
> +			     struct drm_crtc *crtc,
> +			     struct drm_framebuffer *fb,
> +			     int32_t crtc_x, int32_t crtc_y,
> +			     uint32_t crtc_w, uint32_t crtc_h,
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h,
> +			     struct drm_modeset_acquire_ctx *ctx)
> +{
> +	int ret;
> +
> +	WARN_ON(!plane->state);
> +
> +	/* No fb means shut it down */
> +	if (!fb)
> +		return plane->funcs->disable_plane(plane, ctx);
> +
> +	/*
> +	 * FIXME: This is redundant with drm_atomic_plane_check(),
> +	 * but the legacy cursor/"async" .update_plane() tricks
> +	 * don't call that so we still need this here. Should remove
> +	 * this when all .update_plane() implementations have been
> +	 * fixed to call drm_atomic_plane_check().
> +	 */
> +	ret = __setplane_check(plane, crtc, fb,
> +			       crtc_x, crtc_y, crtc_w, crtc_h,
> +			       src_x, src_y, src_w, src_h);
> +	if (ret)
> +		return ret;
> +
> +	return plane->funcs->update_plane(plane, crtc, fb,
> +					  crtc_x, crtc_y, crtc_w, crtc_h,
> +					  src_x, src_y, src_w, src_h, ctx);
> +}
> +
>  static int setplane_internal(struct drm_plane *plane,
>  			     struct drm_crtc *crtc,
>  			     struct drm_framebuffer *fb,
> @@ -707,9 +742,15 @@ static int setplane_internal(struct drm_plane *plane,
>  	ret = drm_modeset_lock_all_ctx(plane->dev, &ctx);
>  	if (ret)
>  		goto fail;
> -	ret = __setplane_internal(plane, crtc, fb,
> -				  crtc_x, crtc_y, crtc_w, crtc_h,
> -				  src_x, src_y, src_w, src_h, &ctx);
> +
> +	if (plane->state)

We're not 100% consistent, but I think this here will make life harder for
drivers transitioning to atomic. Not many left, but still some.

Please use drm_drv_uses_atomic_modeset() instead and check instead for
DRIVER_ATOMIC in your WARN_ON in all the places you're checking for
->state in this patch and the next one.

With that fixed, on the series:

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

> +		ret = __setplane_atomic(plane, crtc, fb,
> +					crtc_x, crtc_y, crtc_w, crtc_h,
> +					src_x, src_y, src_w, src_h, &ctx);
> +	else
> +		ret = __setplane_internal(plane, crtc, fb,
> +					  crtc_x, crtc_y, crtc_w, crtc_h,
> +					  src_x, src_y, src_w, src_h, &ctx);
>  
>  fail:
>  	if (ret == -EDEADLK) {
> @@ -841,9 +882,14 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  		src_h = fb->height << 16;
>  	}
>  
> -	ret = __setplane_internal(plane, crtc, fb,
> -				  crtc_x, crtc_y, crtc_w, crtc_h,
> -				  0, 0, src_w, src_h, ctx);
> +	if (plane->state)
> +		ret = __setplane_atomic(plane, crtc, fb,
> +					crtc_x, crtc_y, crtc_w, crtc_h,
> +					0, 0, src_w, src_h, ctx);
> +	else
> +		ret = __setplane_internal(plane, crtc, fb,
> +					  crtc_x, crtc_y, crtc_w, crtc_h,
> +					  0, 0, src_w, src_h, ctx);
>  
>  	if (fb)
>  		drm_framebuffer_put(fb);
> -- 
> 2.16.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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