[PATCH 1/3] drm: Extract __setplane_check()

[PATCH 1/3] drm: Extract __setplane_check()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pull all the error checking out from __set_plane_internal() to a helper
function. We'll have another user of this soon.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_plane.c | 80 +++++++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index df0b4ebbedbf..5c97a0131484 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -583,6 +583,52 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 	return 0;
 }
 
+static int __setplane_check(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)
+{
+	int ret;
+
+	/* Check whether this plane is usable on this CRTC */
+	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
+		DRM_DEBUG_KMS("Invalid crtc for plane\n");
+		return -EINVAL;
+	}
+
+	/* Check whether this plane supports the fb pixel format. */
+	ret = drm_plane_check_pixel_format(plane, fb->format->format,
+					   fb->modifier);
+	if (ret) {
+		struct drm_format_name_buf format_name;
+
+		DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
+			      drm_get_format_name(fb->format->format,
+						  &format_name),
+			      fb->modifier);
+		return ret;
+	}
+
+	/* Give drivers some help against integer overflows */
+	if (crtc_w > INT_MAX ||
+	    crtc_x > INT_MAX - (int32_t) crtc_w ||
+	    crtc_h > INT_MAX ||
+	    crtc_y > INT_MAX - (int32_t) crtc_h) {
+		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
+			      crtc_w, crtc_h, crtc_x, crtc_y);
+		return -ERANGE;
+	}
+
+	ret = drm_framebuffer_check_src_coords(src_x, src_y, src_w, src_h, fb);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /*
  * __setplane_internal - setplane handler for internal callers
  *
@@ -616,37 +662,9 @@ static int __setplane_internal(struct drm_plane *plane,
 		goto out;
 	}
 
-	/* Check whether this plane is usable on this CRTC */
-	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
-		DRM_DEBUG_KMS("Invalid crtc for plane\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	/* Check whether this plane supports the fb pixel format. */
-	ret = drm_plane_check_pixel_format(plane, fb->format->format,
-					   fb->modifier);
-	if (ret) {
-		struct drm_format_name_buf format_name;
-		DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
-			      drm_get_format_name(fb->format->format,
-						  &format_name),
-			      fb->modifier);
-		goto out;
-	}
-
-	/* Give drivers some help against integer overflows */
-	if (crtc_w > INT_MAX ||
-	    crtc_x > INT_MAX - (int32_t) crtc_w ||
-	    crtc_h > INT_MAX ||
-	    crtc_y > INT_MAX - (int32_t) crtc_h) {
-		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
-			      crtc_w, crtc_h, crtc_x, crtc_y);
-		ret = -ERANGE;
-		goto out;
-	}
-
-	ret = drm_framebuffer_check_src_coords(src_x, src_y, src_w, src_h, fb);
+	ret = __setplane_check(plane, crtc, fb,
+			       crtc_x, crtc_y, crtc_w, crtc_h,
+			       src_x, src_y, src_w, src_h);
 	if (ret)
 		goto out;
 
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjala]

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

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

[Daniel Vetter]

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

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

[Ville Syrjälä]

On Thu, Jun 28, 2018 at 07:05:10PM +0200, Daniel Vetter wrote:
> 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.

We're already using obj->state as the atomic indicator here,
so this patch itself isn't changing anything.

I guess you're suggesting that we change all the ->state checks
we already have to the uses_atomic() thing instead? And the
aim is to allow drives to add obj->state before actually
implementing atomic fully? To me that feels like asking for
trouble, but who knows, maybe I'm wrong.

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Jun 28, 2018 at 7:36 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Jun 28, 2018 at 07:05:10PM +0200, Daniel Vetter wrote:
>> 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.
>
> We're already using obj->state as the atomic indicator here,
> so this patch itself isn't changing anything.

Huh, must have missed that when reviewing your legacy state removal series.

> I guess you're suggesting that we change all the ->state checks
> we already have to the uses_atomic() thing instead? And the
> aim is to allow drives to add obj->state before actually
> implementing atomic fully? To me that feels like asking for
> trouble, but who knows, maybe I'm wrong.

Yup that's actually required by the transitional helpers. You start
building up the ->state stuff before it's all fully in place, since
otherwise you have a bit a chicken&egg situation. Which would mean a
full driver rewrite to get to atomic instead of more gradual
transition.

The one exception is the various getfoo ioctls, where the ->state
stuff should be accurate enough as soon as it's there.
-Daniel


>>
>> 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
>
> --
> Ville Syrjälä
> Intel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjala]

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.

v2: Use drm_drv_uses_atomic_modeset() (Daniel)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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..6153cbda239f 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(drm_drv_uses_atomic_modeset(plane->dev));
+
 	/* 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(!drm_drv_uses_atomic_modeset(plane->dev));
+
+	/* 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 (drm_drv_uses_atomic_modeset(plane->dev))
+		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 (drm_drv_uses_atomic_modeset(dev))
+		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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm: Skip __drm_mode_set_config_internal() on atomic drivers

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Everything (apart from the actual ->set_config() call)
__drm_mode_set_config_internal() does is now useless on
atomic drivers. So let's just skip all the foreplay.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f45e7a8d4acd..f3c8af6c2068 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -468,6 +468,8 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
 	struct drm_crtc *tmp;
 	int ret;
 
+	WARN_ON(crtc->state);
+
 	/*
 	 * NOTE: ->set_config can also disable other crtcs (if we steal all
 	 * connectors from it), hence we need to refcount the fbs across all
@@ -485,10 +487,8 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
 	if (ret == 0) {
 		struct drm_plane *plane = crtc->primary;
 
-		if (!plane->state) {
-			plane->crtc = fb ? crtc : NULL;
-			plane->fb = fb;
-		}
+		plane->crtc = fb ? crtc : NULL;
+		plane->fb = fb;
 	}
 
 	drm_for_each_crtc(tmp, crtc->dev) {
@@ -503,6 +503,7 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
 
 	return ret;
 }
+
 /**
  * drm_mode_set_config_internal - helper to call &drm_mode_config_funcs.set_config
  * @set: modeset config to set
@@ -747,7 +748,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	set.connectors = connector_set;
 	set.num_connectors = crtc_req->count_connectors;
 	set.fb = fb;
-	ret = __drm_mode_set_config_internal(&set, &ctx);
+
+	if (crtc->state)
+		ret = crtc->funcs->set_config(&set, &ctx);
+	else
+		ret = __drm_mode_set_config_internal(&set, &ctx);
 
 out:
 	if (fb)
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/3] drm: Skip __drm_mode_set_config_internal() on atomic drivers

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Everything (apart from the actual ->set_config() call)
__drm_mode_set_config_internal() does is now useless on
atomic drivers. So let's just skip all the foreplay.

v2: Use drm_drv_uses_atomic_modeset() (Daniel)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a6906c4ab880..bae43938c8f6 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -461,6 +461,8 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
 	struct drm_crtc *tmp;
 	int ret;
 
+	WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev));
+
 	/*
 	 * NOTE: ->set_config can also disable other crtcs (if we steal all
 	 * connectors from it), hence we need to refcount the fbs across all
@@ -478,10 +480,8 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
 	if (ret == 0) {
 		struct drm_plane *plane = crtc->primary;
 
-		if (!plane->state) {
-			plane->crtc = fb ? crtc : NULL;
-			plane->fb = fb;
-		}
+		plane->crtc = fb ? crtc : NULL;
+		plane->fb = fb;
 	}
 
 	drm_for_each_crtc(tmp, crtc->dev) {
@@ -496,6 +496,7 @@ static int __drm_mode_set_config_internal(struct drm_mode_set *set,
 
 	return ret;
 }
+
 /**
  * drm_mode_set_config_internal - helper to call &drm_mode_config_funcs.set_config
  * @set: modeset config to set
@@ -740,7 +741,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 	set.connectors = connector_set;
 	set.num_connectors = crtc_req->count_connectors;
 	set.fb = fb;
-	ret = __drm_mode_set_config_internal(&set, &ctx);
+
+	if (drm_drv_uses_atomic_modeset(dev))
+		ret = crtc->funcs->set_config(&set, &ctx);
+	else
+		ret = __drm_mode_set_config_internal(&set, &ctx);
 
 out:
 	if (fb)
-- 
2.16.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm: Extract __setplane_check()

[Rodrigo Vivi]

On Thu, Jun 28, 2018 at 04:54:55PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Pull all the error checking out from __set_plane_internal() to a helper
> function. We'll have another user of this soon.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/drm_plane.c | 80 +++++++++++++++++++++++++++------------------
>  1 file changed, 49 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index df0b4ebbedbf..5c97a0131484 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -583,6 +583,52 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
>  	return 0;
>  }
>  
> +static int __setplane_check(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)
> +{
> +	int ret;
> +
> +	/* Check whether this plane is usable on this CRTC */
> +	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
> +		DRM_DEBUG_KMS("Invalid crtc for plane\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Check whether this plane supports the fb pixel format. */
> +	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> +					   fb->modifier);
> +	if (ret) {
> +		struct drm_format_name_buf format_name;
> +
> +		DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
> +			      drm_get_format_name(fb->format->format,
> +						  &format_name),
> +			      fb->modifier);
> +		return ret;
> +	}
> +
> +	/* Give drivers some help against integer overflows */
> +	if (crtc_w > INT_MAX ||
> +	    crtc_x > INT_MAX - (int32_t) crtc_w ||
> +	    crtc_h > INT_MAX ||
> +	    crtc_y > INT_MAX - (int32_t) crtc_h) {
> +		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> +			      crtc_w, crtc_h, crtc_x, crtc_y);
> +		return -ERANGE;
> +	}
> +
> +	ret = drm_framebuffer_check_src_coords(src_x, src_y, src_w, src_h, fb);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  /*
>   * __setplane_internal - setplane handler for internal callers
>   *
> @@ -616,37 +662,9 @@ static int __setplane_internal(struct drm_plane *plane,
>  		goto out;
>  	}
>  
> -	/* Check whether this plane is usable on this CRTC */
> -	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
> -		DRM_DEBUG_KMS("Invalid crtc for plane\n");
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	/* Check whether this plane supports the fb pixel format. */
> -	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> -					   fb->modifier);
> -	if (ret) {
> -		struct drm_format_name_buf format_name;
> -		DRM_DEBUG_KMS("Invalid pixel format %s, modifier 0x%llx\n",
> -			      drm_get_format_name(fb->format->format,
> -						  &format_name),
> -			      fb->modifier);
> -		goto out;
> -	}
> -
> -	/* Give drivers some help against integer overflows */
> -	if (crtc_w > INT_MAX ||
> -	    crtc_x > INT_MAX - (int32_t) crtc_w ||
> -	    crtc_h > INT_MAX ||
> -	    crtc_y > INT_MAX - (int32_t) crtc_h) {
> -		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> -			      crtc_w, crtc_h, crtc_x, crtc_y);
> -		ret = -ERANGE;
> -		goto out;
> -	}
> -
> -	ret = drm_framebuffer_check_src_coords(src_x, src_y, src_w, src_h, fb);
> +	ret = __setplane_check(plane, crtc, fb,
> +			       crtc_x, crtc_y, crtc_w, crtc_h,
> +			       src_x, src_y, src_w, src_h);
>  	if (ret)
>  		goto out;
>  
> -- 
> 2.16.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx