[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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm: Extract __setplane_check()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Extract __setplane_check()
URL   : https://patchwork.freedesktop.org/series/45589/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
d301e6d532be drm: Extract __setplane_check()
-:53: CHECK:SPACING: No space is necessary after a cast
#53: FILE: drivers/gpu/drm/drm_plane.c:617:
+	    crtc_x > INT_MAX - (int32_t) crtc_w ||

-:55: CHECK:SPACING: No space is necessary after a cast
#55: FILE: drivers/gpu/drm/drm_plane.c:619:
+	    crtc_y > INT_MAX - (int32_t) crtc_h) {

total: 0 errors, 0 warnings, 2 checks, 92 lines checked
1394e3cb028d drm: Introduce __setplane_atomic()
aa9c2a3eaf0e drm: Skip __drm_mode_set_config_internal() on atomic drivers

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

✗ Fi.CI.BAT: failure for series starting with [1/3] drm: Extract __setplane_check()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Extract __setplane_check()
URL   : https://patchwork.freedesktop.org/series/45589/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4397 -> Patchwork_9473 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9473 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9473, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45589/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9473:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-bxt-dsi:         PASS -> FAIL +2

    
== Known issues ==

  Here are the changes found in Patchwork_9473 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> FAIL (fdo#103375) +3

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008


== Participating hosts (44 -> 39) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4397 -> Patchwork_9473

  CI_DRM_4397: 7306233935b0e426454e8adcf09a8022faa03cbc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9473: aa9c2a3eaf0eb8ec2661d968f6cc23ea4b97855b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

aa9c2a3eaf0e drm: Skip __drm_mode_set_config_internal() on atomic drivers
1394e3cb028d drm: Introduce __setplane_atomic()
d301e6d532be drm: Extract __setplane_check()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9473/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for series starting with [1/3] drm: Extract __setplane_check()

[Ville Syrjälä]

On Thu, Jun 28, 2018 at 03:09:30PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/3] drm: Extract __setplane_check()
> URL   : https://patchwork.freedesktop.org/series/45589/
> State : failure
> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_4397 -> Patchwork_9473 =
> 
> == Summary - FAILURE ==
> 
>   Serious unknown changes coming with Patchwork_9473 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_9473, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   External URL: https://patchwork.freedesktop.org/api/1.0/series/45589/revisions/1/mbox/
> 
> == Possible new issues ==
> 
>   === IGT changes ===
> 
>     ==== Possible regressions ====
> 
>     igt@gem_exec_suspend@basic-s4-devices:
>       fi-bxt-dsi:         PASS -> FAIL +2

hda fail:
<6>[  282.796748] systemd-udevd   D12664   283    243 0x80000104
<4>[  282.796795] Call Trace:
<4>[  282.796832]  ? __schedule+0x364/0xbf0
<4>[  282.796874]  schedule+0x2d/0x90
<4>[  282.796902]  __pm_runtime_barrier+0x9c/0x160
<4>[  282.796936]  ? wait_woken+0xa0/0xa0
<4>[  282.796969]  __pm_runtime_disable+0x84/0xe0
<4>[  282.797019]  snd_hda_codec_cleanup_for_unbind+0x203/0x210 [snd_hda_codec]
<4>[  282.797114]  hda_codec_driver_probe+0x47/0x100 [snd_hda_codec]>

>
> 
>     
> == Known issues ==
> 
>   Here are the changes found in Patchwork_9473 that come from known issues:
> 
>   === IGT changes ===
> 
>     ==== Issues hit ====
> 
>     igt@drv_module_reload@basic-reload:
>       fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
>       fi-bxt-dsi:         PASS -> FAIL (fdo#103375) +3
> 
>     igt@prime_vgem@basic-fence-flip:
>       fi-ilk-650:         PASS -> FAIL (fdo#104008)
> 
>     
>   fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
>   fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
>   fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
> 
> 
> == Participating hosts (44 -> 39) ==
> 
>   Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 
> 
> 
> == Build changes ==
> 
>     * Linux: CI_DRM_4397 -> Patchwork_9473
> 
>   CI_DRM_4397: 7306233935b0e426454e8adcf09a8022faa03cbc @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_9473: aa9c2a3eaf0eb8ec2661d968f6cc23ea4b97855b @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> aa9c2a3eaf0e drm: Skip __drm_mode_set_config_internal() on atomic drivers
> 1394e3cb028d drm: Introduce __setplane_atomic()
> d301e6d532be drm: Extract __setplane_check()
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9473/issues.html

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm: Extract __setplane_check() (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Extract __setplane_check() (rev3)
URL   : https://patchwork.freedesktop.org/series/45589/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
fc689c6dc4d2 drm: Extract __setplane_check()
-:54: CHECK:SPACING: No space is necessary after a cast
#54: FILE: drivers/gpu/drm/drm_plane.c:617:
+	    crtc_x > INT_MAX - (int32_t) crtc_w ||

-:56: CHECK:SPACING: No space is necessary after a cast
#56: FILE: drivers/gpu/drm/drm_plane.c:619:
+	    crtc_y > INT_MAX - (int32_t) crtc_h) {

total: 0 errors, 0 warnings, 2 checks, 92 lines checked
f3486ccbb828 drm: Introduce __setplane_atomic()
215528772d4d drm: Skip __drm_mode_set_config_internal() on atomic drivers

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm: Extract __setplane_check() (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Extract __setplane_check() (rev3)
URL   : https://patchwork.freedesktop.org/series/45589/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4438 -> Patchwork_9557 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45589/revisions/3/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9557 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_chamelium@dp-crc-fast:
      fi-kbl-7500u:       PASS -> DMESG-FAIL (fdo#103841)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-FAIL (fdo#106103, fdo#102614) -> PASS

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103


== Participating hosts (47 -> 42) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4438 -> Patchwork_9557

  CI_DRM_4438: b689733af687b4b8072fb62a6bfe267c4e888f5f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4539: 8b3cc74c6911e9b2835fe6e160f84bae463a70ef @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9557: 215528772d4d5228da89cf104c87690a631b5c6d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

215528772d4d drm: Skip __drm_mode_set_config_internal() on atomic drivers
f3486ccbb828 drm: Introduce __setplane_atomic()
fc689c6dc4d2 drm: Extract __setplane_check()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9557/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/3] drm: Extract __setplane_check() (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm: Extract __setplane_check() (rev3)
URL   : https://patchwork.freedesktop.org/series/45589/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4438_full -> Patchwork_9557_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9557_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9557_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9557_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-blt:
      shard-kbl:          PASS -> SKIP +1

    igt@gem_mocs_settings@mocs-rc6-bsd1:
      shard-kbl:          SKIP -> PASS +1

    
== Known issues ==

  Here are the changes found in Patchwork_9557_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-snb:          PASS -> FAIL (fdo#106886)

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL (fdo#106509, fdo#105454)

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368) +1

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          PASS -> FAIL (fdo#103822)

    igt@perf@polling:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_flip_tiling@flip-to-x-tiled:
      shard-glk:          FAIL (fdo#103822) -> PASS

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          FAIL -> PASS

    
    ==== Warnings ====

    igt@drv_selftest@live_gtt:
      shard-glk:          FAIL (fdo#107127, fdo#105347) -> INCOMPLETE (fdo#107127, k.org#198133, fdo#103359)

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107127 https://bugs.freedesktop.org/show_bug.cgi?id=107127
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4438 -> Patchwork_9557

  CI_DRM_4438: b689733af687b4b8072fb62a6bfe267c4e888f5f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4539: 8b3cc74c6911e9b2835fe6e160f84bae463a70ef @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9557: 215528772d4d5228da89cf104c87690a631b5c6d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9557/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx