[PATCH 1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[PATCH 1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[Ville Syrjala]

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

Replace 'crtc->state' with the explicit old crtc state.

Actually it shouldn't matter whether we use the old or the new
crtc state here since any plane that has been removed from the
crtc since the crtc state was duplicated will have been added
to the atomic state already. That is, you can't call
drm_atomic_set_crtc_for_plane() without having the new
plane state already in hand.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3dbfbddae7e6..064c48075917 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -927,6 +927,8 @@ int
 drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 			       struct drm_crtc *crtc)
 {
+	const struct drm_crtc_state *old_crtc_state =
+		drm_atomic_get_old_crtc_state(state, crtc);
 	struct drm_plane *plane;
 
 	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
@@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
 			 crtc->base.id, crtc->name, state);
 
-	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
+	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
 		struct drm_plane_state *plane_state =
 			drm_atomic_get_plane_state(state, plane);
 
-- 
2.18.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjala]

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

Convert drm_atomic_crtc_check() over to using explicit old vs. new
crtc states. Avoids the confusion of "what does crtc->state mean
again?".

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 064c48075917..dde696181efe 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -315,9 +315,11 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_get_crtc_state);
 
-static int drm_atomic_crtc_check(struct drm_crtc *crtc,
-		struct drm_crtc_state *state)
+static int drm_atomic_crtc_check(const struct drm_crtc_state *old_crtc_state,
+				 const struct drm_crtc_state *new_crtc_state)
 {
+	struct drm_crtc *crtc = new_crtc_state->crtc;
+
 	/* NOTE: we explicitly don't enforce constraints such as primary
 	 * layer covering entire screen, since that is something we want
 	 * to allow (on hw that supports it).  For hw that does not, it
@@ -326,7 +328,7 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
 	 * TODO: Add generic modeset state checks once we support those.
 	 */
 
-	if (state->active && !state->enable) {
+	if (new_crtc_state->active && !new_crtc_state->enable) {
 		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active without enabled\n",
 				 crtc->base.id, crtc->name);
 		return -EINVAL;
@@ -336,14 +338,14 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
 	 * as this is a kernel-internal detail that userspace should never
 	 * be able to trigger. */
 	if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
-	    WARN_ON(state->enable && !state->mode_blob)) {
+	    WARN_ON(new_crtc_state->enable && !new_crtc_state->mode_blob)) {
 		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled without mode blob\n",
 				 crtc->base.id, crtc->name);
 		return -EINVAL;
 	}
 
 	if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
-	    WARN_ON(!state->enable && state->mode_blob)) {
+	    WARN_ON(!new_crtc_state->enable && new_crtc_state->mode_blob)) {
 		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] disabled with mode blob\n",
 				 crtc->base.id, crtc->name);
 		return -EINVAL;
@@ -359,7 +361,8 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
 	 * and legacy page_flip IOCTL which also reject service on a disabled
 	 * pipe.
 	 */
-	if (state->event && !state->active && !crtc->state->active) {
+	if (new_crtc_state->event &&
+	    !new_crtc_state->active && !old_crtc_state->active) {
 		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n",
 				 crtc->base.id, crtc->name);
 		return -EINVAL;
@@ -965,7 +968,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	struct drm_plane *plane;
 	struct drm_plane_state *plane_state;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 	struct drm_connector *conn;
 	struct drm_connector_state *conn_state;
 	int i, ret = 0;
@@ -981,8 +985,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		}
 	}
 
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		ret = drm_atomic_crtc_check(crtc, crtc_state);
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		ret = drm_atomic_crtc_check(old_crtc_state, new_crtc_state);
 		if (ret) {
 			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check failed\n",
 					 crtc->base.id, crtc->name);
@@ -1010,8 +1014,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	}
 
 	if (!state->allow_modeset) {
-		for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
 				DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full modeset\n",
 						 crtc->base.id, crtc->name);
 				return -EINVAL;
-- 
2.18.1

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

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

[Daniel Vetter]

On Thu, Nov 01, 2018 at 08:46:45PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Convert drm_atomic_crtc_check() over to using explicit old vs. new
> crtc states. Avoids the confusion of "what does crtc->state mean
> again?".

Yeah much better.

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/drm_atomic.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 064c48075917..dde696181efe 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -315,9 +315,11 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_get_crtc_state);
>  
> -static int drm_atomic_crtc_check(struct drm_crtc *crtc,
> -		struct drm_crtc_state *state)
> +static int drm_atomic_crtc_check(const struct drm_crtc_state *old_crtc_state,
> +				 const struct drm_crtc_state *new_crtc_state)
>  {
> +	struct drm_crtc *crtc = new_crtc_state->crtc;
> +
>  	/* NOTE: we explicitly don't enforce constraints such as primary
>  	 * layer covering entire screen, since that is something we want
>  	 * to allow (on hw that supports it).  For hw that does not, it
> @@ -326,7 +328,7 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>  	 * TODO: Add generic modeset state checks once we support those.
>  	 */
>  
> -	if (state->active && !state->enable) {
> +	if (new_crtc_state->active && !new_crtc_state->enable) {
>  		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active without enabled\n",
>  				 crtc->base.id, crtc->name);
>  		return -EINVAL;
> @@ -336,14 +338,14 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>  	 * as this is a kernel-internal detail that userspace should never
>  	 * be able to trigger. */
>  	if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
> -	    WARN_ON(state->enable && !state->mode_blob)) {
> +	    WARN_ON(new_crtc_state->enable && !new_crtc_state->mode_blob)) {
>  		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled without mode blob\n",
>  				 crtc->base.id, crtc->name);
>  		return -EINVAL;
>  	}
>  
>  	if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
> -	    WARN_ON(!state->enable && state->mode_blob)) {
> +	    WARN_ON(!new_crtc_state->enable && new_crtc_state->mode_blob)) {
>  		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] disabled with mode blob\n",
>  				 crtc->base.id, crtc->name);
>  		return -EINVAL;
> @@ -359,7 +361,8 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>  	 * and legacy page_flip IOCTL which also reject service on a disabled
>  	 * pipe.
>  	 */
> -	if (state->event && !state->active && !crtc->state->active) {
> +	if (new_crtc_state->event &&
> +	    !new_crtc_state->active && !old_crtc_state->active) {
>  		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n",
>  				 crtc->base.id, crtc->name);
>  		return -EINVAL;
> @@ -965,7 +968,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	struct drm_plane *plane;
>  	struct drm_plane_state *plane_state;
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *old_crtc_state;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_connector *conn;
>  	struct drm_connector_state *conn_state;
>  	int i, ret = 0;
> @@ -981,8 +985,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  		}
>  	}
>  
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		ret = drm_atomic_crtc_check(crtc, crtc_state);
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		ret = drm_atomic_crtc_check(old_crtc_state, new_crtc_state);
>  		if (ret) {
>  			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check failed\n",
>  					 crtc->base.id, crtc->name);
> @@ -1010,8 +1014,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	}
>  
>  	if (!state->allow_modeset) {
> -		for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> +		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> +			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>  				DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full modeset\n",
>  						 crtc->base.id, crtc->name);
>  				return -EINVAL;
> -- 
> 2.18.1
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjala]

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

Convert drm_atomic_plane_check() over to using explicit old vs. new
plane states. Avoids the confusion of "what does plane->state mean
again?".

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index dde696181efe..46f8d798efaf 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -492,108 +492,109 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_get_plane_state);
 
 static bool
-plane_switching_crtc(struct drm_atomic_state *state,
-		     struct drm_plane *plane,
-		     struct drm_plane_state *plane_state)
+plane_switching_crtc(const struct drm_plane_state *old_plane_state,
+		     const struct drm_plane_state *new_plane_state)
 {
-	if (!plane->state->crtc || !plane_state->crtc)
-		return false;
-
-	if (plane->state->crtc == plane_state->crtc)
-		return false;
-
 	/* This could be refined, but currently there's no helper or driver code
 	 * to implement direct switching of active planes nor userspace to take
 	 * advantage of more direct plane switching without the intermediate
 	 * full OFF state.
 	 */
-	return true;
+	return old_plane_state->crtc && new_plane_state->crtc &&
+		old_plane_state->crtc != new_plane_state->crtc;
 }
 
 /**
  * drm_atomic_plane_check - check plane state
- * @plane: plane to check
- * @state: plane state to check
+ * @old_plane_state: old plane state to check
+ * @new_plane_state: new plane state to check
  *
  * Provides core sanity checks for plane state.
  *
  * RETURNS:
  * Zero on success, error code on failure
  */
-static int drm_atomic_plane_check(struct drm_plane *plane,
-		struct drm_plane_state *state)
+static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
+				  const struct drm_plane_state *new_plane_state)
 {
+	struct drm_plane *plane = new_plane_state->plane;
+	struct drm_crtc *crtc = new_plane_state->crtc;
+	const struct drm_framebuffer *fb = new_plane_state->fb;
 	unsigned int fb_width, fb_height;
 	int ret;
 
 	/* either *both* CRTC and FB must be set, or neither */
-	if (state->crtc && !state->fb) {
+	if (crtc && !fb) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] CRTC set but no FB\n",
 				 plane->base.id, plane->name);
 		return -EINVAL;
-	} else if (state->fb && !state->crtc) {
+	} else if (fb && !crtc) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] FB set but no CRTC\n",
 				 plane->base.id, plane->name);
 		return -EINVAL;
 	}
 
 	/* if disabled, we don't care about the rest of the state: */
-	if (!state->crtc)
+	if (!crtc)
 		return 0;
 
 	/* Check whether this plane is usable on this CRTC */
-	if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) {
+	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
 		DRM_DEBUG_ATOMIC("Invalid [CRTC:%d:%s] for [PLANE:%d:%s]\n",
-				 state->crtc->base.id, state->crtc->name,
+				 crtc->base.id, crtc->name,
 				 plane->base.id, plane->name);
 		return -EINVAL;
 	}
 
 	/* Check whether this plane supports the fb pixel format. */
-	ret = drm_plane_check_pixel_format(plane, state->fb->format->format,
-					   state->fb->modifier);
+	ret = drm_plane_check_pixel_format(plane, fb->format->format,
+					   fb->modifier);
 	if (ret) {
 		struct drm_format_name_buf format_name;
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid pixel format %s, modifier 0x%llx\n",
 				 plane->base.id, plane->name,
-				 drm_get_format_name(state->fb->format->format,
+				 drm_get_format_name(fb->format->format,
 						     &format_name),
-				 state->fb->modifier);
+				 fb->modifier);
 		return ret;
 	}
 
 	/* Give drivers some help against integer overflows */
-	if (state->crtc_w > INT_MAX ||
-	    state->crtc_x > INT_MAX - (int32_t) state->crtc_w ||
-	    state->crtc_h > INT_MAX ||
-	    state->crtc_y > INT_MAX - (int32_t) state->crtc_h) {
+	if (new_plane_state->crtc_w > INT_MAX ||
+	    new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w ||
+	    new_plane_state->crtc_h > INT_MAX ||
+	    new_plane_state->crtc_y > INT_MAX - (int32_t) new_plane_state->crtc_h) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid CRTC coordinates %ux%u+%d+%d\n",
 				 plane->base.id, plane->name,
-				 state->crtc_w, state->crtc_h,
-				 state->crtc_x, state->crtc_y);
+				 new_plane_state->crtc_w, new_plane_state->crtc_h,
+				 new_plane_state->crtc_x, new_plane_state->crtc_y);
 		return -ERANGE;
 	}
 
-	fb_width = state->fb->width << 16;
-	fb_height = state->fb->height << 16;
+	fb_width = fb->width << 16;
+	fb_height = fb->height << 16;
 
 	/* Make sure source coordinates are inside the fb. */
-	if (state->src_w > fb_width ||
-	    state->src_x > fb_width - state->src_w ||
-	    state->src_h > fb_height ||
-	    state->src_y > fb_height - state->src_h) {
+	if (new_plane_state->src_w > fb_width ||
+	    new_plane_state->src_x > fb_width - new_plane_state->src_w ||
+	    new_plane_state->src_h > fb_height ||
+	    new_plane_state->src_y > fb_height - new_plane_state->src_h) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid source coordinates "
 				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
 				 plane->base.id, plane->name,
-				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
-				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
-				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
-				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10,
-				 state->fb->width, state->fb->height);
+				 new_plane_state->src_w >> 16,
+				 ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
+				 new_plane_state->src_h >> 16,
+				 ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
+				 new_plane_state->src_x >> 16,
+				 ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
+				 new_plane_state->src_y >> 16,
+				 ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
+				 fb->width, fb->height);
 		return -ENOSPC;
 	}
 
-	if (plane_switching_crtc(state->state, plane, state)) {
+	if (plane_switching_crtc(old_plane_state, new_plane_state)) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n",
 				 plane->base.id, plane->name);
 		return -EINVAL;
@@ -966,7 +967,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *old_plane_state;
+	struct drm_plane_state *new_plane_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
@@ -976,8 +978,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 
 	DRM_DEBUG_ATOMIC("checking %p\n", state);
 
-	for_each_new_plane_in_state(state, plane, plane_state, i) {
-		ret = drm_atomic_plane_check(plane, plane_state);
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+		ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
 		if (ret) {
 			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n",
 					 plane->base.id, plane->name);
-- 
2.18.1

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

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

[Daniel Vetter]

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

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

With the old layout here this is:

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

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

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

[Ville Syrjälä]

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

I guess it's a matter of taste. I think my new style is much more
succinct and reads more natural:

"if old exists and new exists and old is not new we're swithing crtcs"

vs. 

"if old doesn't exist or new doesn't exist we're not switching
crtcs, otherwise if old is new we're not switching crtcs, otherwise
we're switching crtcs".

But meh. So I'll change it back.

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

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

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

[Ville Syrjala]

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

Convert drm_atomic_plane_check() over to using explicit old vs. new
plane states. Avoids the confusion of "what does plane->state mean
again?".

v2: Stick to the multi-stage logic in plane_switching_crtc() (Daniel)

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3a4b6b251971..9ac26437051b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -497,14 +497,13 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 EXPORT_SYMBOL(drm_atomic_get_plane_state);
 
 static bool
-plane_switching_crtc(struct drm_atomic_state *state,
-		     struct drm_plane *plane,
-		     struct drm_plane_state *plane_state)
+plane_switching_crtc(const struct drm_plane_state *old_plane_state,
+		     const struct drm_plane_state *new_plane_state)
 {
-	if (!plane->state->crtc || !plane_state->crtc)
+	if (!old_plane_state->crtc || !new_plane_state->crtc)
 		return false;
 
-	if (plane->state->crtc == plane_state->crtc)
+	if (old_plane_state->crtc == new_plane_state->crtc)
 		return false;
 
 	/* This could be refined, but currently there's no helper or driver code
@@ -517,88 +516,95 @@ plane_switching_crtc(struct drm_atomic_state *state,
 
 /**
  * drm_atomic_plane_check - check plane state
- * @plane: plane to check
- * @state: plane state to check
+ * @old_plane_state: old plane state to check
+ * @new_plane_state: new plane state to check
  *
  * Provides core sanity checks for plane state.
  *
  * RETURNS:
  * Zero on success, error code on failure
  */
-static int drm_atomic_plane_check(struct drm_plane *plane,
-		struct drm_plane_state *state)
+static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
+				  const struct drm_plane_state *new_plane_state)
 {
+	struct drm_plane *plane = new_plane_state->plane;
+	struct drm_crtc *crtc = new_plane_state->crtc;
+	const struct drm_framebuffer *fb = new_plane_state->fb;
 	unsigned int fb_width, fb_height;
 	int ret;
 
 	/* either *both* CRTC and FB must be set, or neither */
-	if (state->crtc && !state->fb) {
+	if (crtc && !fb) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] CRTC set but no FB\n",
 				 plane->base.id, plane->name);
 		return -EINVAL;
-	} else if (state->fb && !state->crtc) {
+	} else if (fb && !crtc) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] FB set but no CRTC\n",
 				 plane->base.id, plane->name);
 		return -EINVAL;
 	}
 
 	/* if disabled, we don't care about the rest of the state: */
-	if (!state->crtc)
+	if (!crtc)
 		return 0;
 
 	/* Check whether this plane is usable on this CRTC */
-	if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) {
+	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
 		DRM_DEBUG_ATOMIC("Invalid [CRTC:%d:%s] for [PLANE:%d:%s]\n",
-				 state->crtc->base.id, state->crtc->name,
+				 crtc->base.id, crtc->name,
 				 plane->base.id, plane->name);
 		return -EINVAL;
 	}
 
 	/* Check whether this plane supports the fb pixel format. */
-	ret = drm_plane_check_pixel_format(plane, state->fb->format->format,
-					   state->fb->modifier);
+	ret = drm_plane_check_pixel_format(plane, fb->format->format,
+					   fb->modifier);
 	if (ret) {
 		struct drm_format_name_buf format_name;
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid pixel format %s, modifier 0x%llx\n",
 				 plane->base.id, plane->name,
-				 drm_get_format_name(state->fb->format->format,
+				 drm_get_format_name(fb->format->format,
 						     &format_name),
-				 state->fb->modifier);
+				 fb->modifier);
 		return ret;
 	}
 
 	/* Give drivers some help against integer overflows */
-	if (state->crtc_w > INT_MAX ||
-	    state->crtc_x > INT_MAX - (int32_t) state->crtc_w ||
-	    state->crtc_h > INT_MAX ||
-	    state->crtc_y > INT_MAX - (int32_t) state->crtc_h) {
+	if (new_plane_state->crtc_w > INT_MAX ||
+	    new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w ||
+	    new_plane_state->crtc_h > INT_MAX ||
+	    new_plane_state->crtc_y > INT_MAX - (int32_t) new_plane_state->crtc_h) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid CRTC coordinates %ux%u+%d+%d\n",
 				 plane->base.id, plane->name,
-				 state->crtc_w, state->crtc_h,
-				 state->crtc_x, state->crtc_y);
+				 new_plane_state->crtc_w, new_plane_state->crtc_h,
+				 new_plane_state->crtc_x, new_plane_state->crtc_y);
 		return -ERANGE;
 	}
 
-	fb_width = state->fb->width << 16;
-	fb_height = state->fb->height << 16;
+	fb_width = fb->width << 16;
+	fb_height = fb->height << 16;
 
 	/* Make sure source coordinates are inside the fb. */
-	if (state->src_w > fb_width ||
-	    state->src_x > fb_width - state->src_w ||
-	    state->src_h > fb_height ||
-	    state->src_y > fb_height - state->src_h) {
+	if (new_plane_state->src_w > fb_width ||
+	    new_plane_state->src_x > fb_width - new_plane_state->src_w ||
+	    new_plane_state->src_h > fb_height ||
+	    new_plane_state->src_y > fb_height - new_plane_state->src_h) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] invalid source coordinates "
 				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
 				 plane->base.id, plane->name,
-				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
-				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
-				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
-				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10,
-				 state->fb->width, state->fb->height);
+				 new_plane_state->src_w >> 16,
+				 ((new_plane_state->src_w & 0xffff) * 15625) >> 10,
+				 new_plane_state->src_h >> 16,
+				 ((new_plane_state->src_h & 0xffff) * 15625) >> 10,
+				 new_plane_state->src_x >> 16,
+				 ((new_plane_state->src_x & 0xffff) * 15625) >> 10,
+				 new_plane_state->src_y >> 16,
+				 ((new_plane_state->src_y & 0xffff) * 15625) >> 10,
+				 fb->width, fb->height);
 		return -ENOSPC;
 	}
 
-	if (plane_switching_crtc(state->state, plane, state)) {
+	if (plane_switching_crtc(old_plane_state, new_plane_state)) {
 		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n",
 				 plane->base.id, plane->name);
 		return -EINVAL;
@@ -971,7 +977,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_plane *plane;
-	struct drm_plane_state *plane_state;
+	struct drm_plane_state *old_plane_state;
+	struct drm_plane_state *new_plane_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
@@ -981,8 +988,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 
 	DRM_DEBUG_ATOMIC("checking %p\n", state);
 
-	for_each_new_plane_in_state(state, plane, plane_state, i) {
-		ret = drm_atomic_plane_check(plane, plane_state);
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+		ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
 		if (ret) {
 			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n",
 					 plane->base.id, plane->name);
-- 
2.18.1

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()
URL   : https://patchwork.freedesktop.org/series/51894/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
f26f227aa8ea drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()
605b8ed4d33e drm/atomic: Use explicit old/new state in drm_atomic_crtc_check()
eaacef5406e9 drm/atomic: Use explicit old/new state in drm_atomic_plane_check()
-:120: CHECK:SPACING: No space is necessary after a cast
#120: FILE: drivers/gpu/drm/drm_atomic.c:564:
+	    new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w ||

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

total: 0 errors, 0 warnings, 2 checks, 169 lines checked

_______________________________________________
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/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()
URL   : https://patchwork.freedesktop.org/series/51894/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5069 -> Patchwork_10696 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@gem_exec_suspend@basic-s3:
      fi-skl-iommu:       PASS -> INCOMPLETE (fdo#107773, fdo#104108)

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362)

    
    ==== Possible fixes ====

    igt@kms_frontbuffer_tracking@basic:
      fi-icl-u2:          FAIL (fdo#103167) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773


== Participating hosts (45 -> 40) ==

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


== Build changes ==

    * Linux: CI_DRM_5069 -> Patchwork_10696

  CI_DRM_5069: 85d538085ea267429a81a49765a26de8809f86e5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4704: ace031dcb1e8bf2b32b4b0d54a55eb30e8f41d6f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10696: eaacef5406e91f709706483ead4f0fce22a9000d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

eaacef5406e9 drm/atomic: Use explicit old/new state in drm_atomic_plane_check()
605b8ed4d33e drm/atomic: Use explicit old/new state in drm_atomic_crtc_check()
f26f227aa8ea drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10696/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/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()
URL   : https://patchwork.freedesktop.org/series/51894/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5069_full -> Patchwork_10696_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_schedule@pi-ringfull-bsd:
      shard-skl:          NOTRUN -> FAIL (fdo#103158)

    igt@gem_userptr_blits@readonly-unsync:
      shard-skl:          NOTRUN -> INCOMPLETE (fdo#108074)

    igt@kms_busy@extended-modeset-hang-newfb-render-a:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#107956) +1

    igt@kms_cursor_crc@cursor-128x128-dpms:
      shard-apl:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_crc@cursor-128x42-onscreen:
      shard-glk:          PASS -> FAIL (fdo#103232) +2

    igt@kms_cursor_crc@cursor-256x85-random:
      shard-skl:          NOTRUN -> FAIL (fdo#103232)

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          PASS -> DMESG-WARN (fdo#106538, fdo#105763)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-fullscreen:
      shard-glk:          PASS -> FAIL (fdo#103167)

    igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-mmap-cpu:
      shard-skl:          PASS -> FAIL (fdo#103167)

    igt@kms_plane_alpha_blend@pipe-b-alpha-7efc:
      shard-skl:          NOTRUN -> FAIL (fdo#107815, fdo#108145)

    igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
      shard-skl:          NOTRUN -> FAIL (fdo#108145)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-apl:          PASS -> FAIL (fdo#103166) +1

    igt@pm_rpm@modeset-non-lpsp-stress:
      shard-skl:          SKIP -> INCOMPLETE (fdo#107807)

    
    ==== Possible fixes ====

    igt@kms_busy@extended-pageflip-hang-newfb-render-c:
      shard-apl:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_cursor_crc@cursor-128x128-sliding:
      shard-apl:          FAIL (fdo#103232) -> PASS

    igt@kms_cursor_crc@cursor-256x256-sliding:
      shard-glk:          FAIL (fdo#103232) -> PASS +1

    igt@kms_draw_crc@draw-method-xrgb2101010-blt-ytiled:
      shard-skl:          FAIL (fdo#103184) -> PASS

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

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-wc:
      shard-skl:          FAIL (fdo#103167) -> PASS +4

    igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-pwrite:
      shard-skl:          FAIL (fdo#105682) -> PASS +1

    igt@kms_frontbuffer_tracking@psr-2p-rte:
      shard-apl:          INCOMPLETE (fdo#103927) -> SKIP

    igt@kms_plane@plane-position-covered-pipe-c-planes:
      shard-apl:          FAIL (fdo#103166) -> PASS +1

    igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
      shard-skl:          FAIL (fdo#107815, fdo#108145) -> PASS

    igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@perf@short-reads:
      shard-kbl:          FAIL (fdo#103183) -> PASS

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103183 https://bugs.freedesktop.org/show_bug.cgi?id=103183
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108074 https://bugs.freedesktop.org/show_bug.cgi?id=108074
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_5069 -> Patchwork_10696

  CI_DRM_5069: 85d538085ea267429a81a49765a26de8809f86e5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4704: ace031dcb1e8bf2b32b4b0d54a55eb30e8f41d6f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10696: eaacef5406e91f709706483ead4f0fce22a9000d @ 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_10696/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[Daniel Vetter]

On Thu, Nov 01, 2018 at 08:46:44PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace 'crtc->state' with the explicit old crtc state.
> 
> Actually it shouldn't matter whether we use the old or the new
> crtc state here since any plane that has been removed from the
> crtc since the crtc state was duplicated will have been added
> to the atomic state already. That is, you can't call
> drm_atomic_set_crtc_for_plane() without having the new
> plane state already in hand.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I think this will break amdgpu_notify_freesync because that doesn't grab
the crtc state first. Which worked with the old stuff, because adding a
connector or plane will also add it's crtc. But with the new logic we'll
just oops I think.

Otoh, it's dead code, so what exactly are amd people doing again :-)

Adding Harry so he's aware, but patch here looks good.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3dbfbddae7e6..064c48075917 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -927,6 +927,8 @@ int
>  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>  			       struct drm_crtc *crtc)
>  {
> +	const struct drm_crtc_state *old_crtc_state =
> +		drm_atomic_get_old_crtc_state(state, crtc);
>  	struct drm_plane *plane;
>  
>  	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
> @@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>  	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
>  			 crtc->base.id, crtc->name, state);
>  
> -	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
> +	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
>  		struct drm_plane_state *plane_state =
>  			drm_atomic_get_plane_state(state, plane);
>  
> -- 
> 2.18.1
> 
> _______________________________________________
> 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 1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[Ville Syrjälä]

On Mon, Nov 05, 2018 at 10:26:01AM +0100, Daniel Vetter wrote:
> On Thu, Nov 01, 2018 at 08:46:44PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Replace 'crtc->state' with the explicit old crtc state.
> > 
> > Actually it shouldn't matter whether we use the old or the new
> > crtc state here since any plane that has been removed from the
> > crtc since the crtc state was duplicated will have been added
> > to the atomic state already. That is, you can't call
> > drm_atomic_set_crtc_for_plane() without having the new
> > plane state already in hand.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I think this will break amdgpu_notify_freesync because that doesn't grab
> the crtc state first. Which worked with the old stuff, because adding a
> connector or plane will also add it's crtc. But with the new logic we'll
> just oops I think.

drm_atomic_add_affected_connectors() will add the crtc to the
state. So looks like it shouldn't oops.

> 
> Otoh, it's dead code, so what exactly are amd people doing again :-)

Heh. Thanks for the review.

> 
> Adding Harry so he's aware, but patch here looks good.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 3dbfbddae7e6..064c48075917 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -927,6 +927,8 @@ int
> >  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
> >  			       struct drm_crtc *crtc)
> >  {
> > +	const struct drm_crtc_state *old_crtc_state =
> > +		drm_atomic_get_old_crtc_state(state, crtc);
> >  	struct drm_plane *plane;
> >  
> >  	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
> > @@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
> >  	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
> >  			 crtc->base.id, crtc->name, state);
> >  
> > -	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
> > +	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
> >  		struct drm_plane_state *plane_state =
> >  			drm_atomic_get_plane_state(state, plane);
> >  
> > -- 
> > 2.18.1
> > 
> > _______________________________________________
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[Wentland, Harry]

On 2018-11-05 9:04 a.m., Ville Syrjälä wrote:
> On Mon, Nov 05, 2018 at 10:26:01AM +0100, Daniel Vetter wrote:
>> On Thu, Nov 01, 2018 at 08:46:44PM +0200, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Replace 'crtc->state' with the explicit old crtc state.
>>>
>>> Actually it shouldn't matter whether we use the old or the new
>>> crtc state here since any plane that has been removed from the
>>> crtc since the crtc state was duplicated will have been added
>>> to the atomic state already. That is, you can't call
>>> drm_atomic_set_crtc_for_plane() without having the new
>>> plane state already in hand.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> I think this will break amdgpu_notify_freesync because that doesn't grab
>> the crtc state first. Which worked with the old stuff, because adding a
>> connector or plane will also add it's crtc. But with the new logic we'll
>> just oops I think.
> 
> drm_atomic_add_affected_connectors() will add the crtc to the
> state. So looks like it shouldn't oops.
> 

Thanks. I thought I was too tired on a Monday morning to spot the oops.

This code should be on the way out with the variable refresh patches from Nick. It's currently only used by our DKMS driver.

Looks good to me.

Acked-by: Harry Wentland <harry.wentland@amd.com>

Harry

>>
>> Otoh, it's dead code, so what exactly are amd people doing again :-)
> 
> Heh. Thanks for the review.
> 
>>
>> Adding Harry so he's aware, but patch here looks good.
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>  drivers/gpu/drm/drm_atomic.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index 3dbfbddae7e6..064c48075917 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -927,6 +927,8 @@ int
>>>  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>>>  			       struct drm_crtc *crtc)
>>>  {
>>> +	const struct drm_crtc_state *old_crtc_state =
>>> +		drm_atomic_get_old_crtc_state(state, crtc);
>>>  	struct drm_plane *plane;
>>>  
>>>  	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
>>> @@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>>>  	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
>>>  			 crtc->base.id, crtc->name, state);
>>>  
>>> -	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
>>> +	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
>>>  		struct drm_plane_state *plane_state =
>>>  			drm_atomic_get_plane_state(state, plane);
>>>  
>>> -- 
>>> 2.18.1
>>>
>>> _______________________________________________
>>> 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 1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

[Ville Syrjälä]

On Mon, Nov 05, 2018 at 02:30:50PM +0000, Wentland, Harry wrote:
> 
> 
> On 2018-11-05 9:04 a.m., Ville Syrjälä wrote:
> > On Mon, Nov 05, 2018 at 10:26:01AM +0100, Daniel Vetter wrote:
> >> On Thu, Nov 01, 2018 at 08:46:44PM +0200, Ville Syrjala wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Replace 'crtc->state' with the explicit old crtc state.
> >>>
> >>> Actually it shouldn't matter whether we use the old or the new
> >>> crtc state here since any plane that has been removed from the
> >>> crtc since the crtc state was duplicated will have been added
> >>> to the atomic state already. That is, you can't call
> >>> drm_atomic_set_crtc_for_plane() without having the new
> >>> plane state already in hand.
> >>>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> I think this will break amdgpu_notify_freesync because that doesn't grab
> >> the crtc state first. Which worked with the old stuff, because adding a
> >> connector or plane will also add it's crtc. But with the new logic we'll
> >> just oops I think.
> > 
> > drm_atomic_add_affected_connectors() will add the crtc to the
> > state. So looks like it shouldn't oops.
> > 
> 
> Thanks. I thought I was too tired on a Monday morning to spot the oops.
> 
> This code should be on the way out with the variable refresh patches from Nick. It's currently only used by our DKMS driver.
> 
> Looks good to me.
> 
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> 
> Harry
> 
> >>
> >> Otoh, it's dead code, so what exactly are amd people doing again :-)
> > 
> > Heh. Thanks for the review.
> > 
> >>
> >> Adding Harry so he's aware, but patch here looks good.
> >>
> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> ---
> >>>  drivers/gpu/drm/drm_atomic.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index 3dbfbddae7e6..064c48075917 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -927,6 +927,8 @@ int
> >>>  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
> >>>  			       struct drm_crtc *crtc)
> >>>  {
> >>> +	const struct drm_crtc_state *old_crtc_state =
> >>> +		drm_atomic_get_old_crtc_state(state, crtc);
> >>>  	struct drm_plane *plane;
> >>>  
> >>>  	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));

Oh, and if the crtc hadn't been added to the state this WARN would
have already triggered before my change.

> >>> @@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
> >>>  	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
> >>>  			 crtc->base.id, crtc->name, state);
> >>>  
> >>> -	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
> >>> +	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
> >>>  		struct drm_plane_state *plane_state =
> >>>  			drm_atomic_get_plane_state(state, plane);
> >>>  
> >>> -- 
> >>> 2.18.1
> >>>
> >>> _______________________________________________
> >>> 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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() (rev2)
URL   : https://patchwork.freedesktop.org/series/51894/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
e5ec4f59a037 drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()
1bfa9185dee8 drm/atomic: Use explicit old/new state in drm_atomic_crtc_check()
f6d1a322301f drm/atomic: Use explicit old/new state in drm_atomic_plane_check()
-:118: CHECK:SPACING: No space is necessary after a cast
#118: FILE: drivers/gpu/drm/drm_atomic.c:574:
+	    new_plane_state->crtc_x > INT_MAX - (int32_t) new_plane_state->crtc_w ||

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

total: 0 errors, 0 warnings, 2 checks, 163 lines checked

_______________________________________________
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/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() (rev2)
URL   : https://patchwork.freedesktop.org/series/51894/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5096 -> Patchwork_10745 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51894/revisions/2/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_coherency:
      fi-gdg-551:         PASS -> DMESG-FAIL (fdo#107164)

    igt@drv_selftest@live_contexts:
      fi-icl-u2:          NOTRUN -> DMESG-FAIL (fdo#108569)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-cfl-8109u:       PASS -> INCOMPLETE (fdo#106070, fdo#108126)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       DMESG-WARN (fdo#106725, fdo#106248) -> PASS

    igt@drv_module_reload@basic-reload-inject:
      fi-byt-clapper:     WARN (fdo#108688) -> PASS

    igt@drv_selftest@live_contexts:
      fi-bsw-n3050:       DMESG-FAIL (fdo#108626) -> PASS

    igt@gem_exec_suspend@basic-s3:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

    igt@pm_rpm@module-reload:
      fi-byt-clapper:     FAIL (fdo#108675) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#108126 https://bugs.freedesktop.org/show_bug.cgi?id=108126
  fdo#108569 https://bugs.freedesktop.org/show_bug.cgi?id=108569
  fdo#108626 https://bugs.freedesktop.org/show_bug.cgi?id=108626
  fdo#108675 https://bugs.freedesktop.org/show_bug.cgi?id=108675
  fdo#108688 https://bugs.freedesktop.org/show_bug.cgi?id=108688


== Participating hosts (51 -> 45) ==

  Additional (1): fi-icl-u2 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510 


== Build changes ==

    * Linux: CI_DRM_5096 -> Patchwork_10745

  CI_DRM_5096: 9dd45e92a3e9b238d044adde1061a8ee0ce24b73 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4711: cc41f4c921e56c62c85ec5349c47022ae9b5f008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10745: f6d1a322301fb41cf1850279091178a5fef8ebaf @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f6d1a322301f drm/atomic: Use explicit old/new state in drm_atomic_plane_check()
1bfa9185dee8 drm/atomic: Use explicit old/new state in drm_atomic_crtc_check()
e5ec4f59a037 drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10745/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/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() (rev2)
URL   : https://patchwork.freedesktop.org/series/51894/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5096_full -> Patchwork_10745_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

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

  === IGT changes ===

    ==== Possible fixes ====

    igt@prime_busy@after-vebox:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411


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

  Missing    (1): shard-skl 


== Build changes ==

    * Linux: CI_DRM_5096 -> Patchwork_10745

  CI_DRM_5096: 9dd45e92a3e9b238d044adde1061a8ee0ce24b73 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4711: cc41f4c921e56c62c85ec5349c47022ae9b5f008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10745: f6d1a322301fb41cf1850279091178a5fef8ebaf @ 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_10745/shards.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/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() (rev2)
URL   : https://patchwork.freedesktop.org/series/51894/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5096_full -> Patchwork_10745_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10745_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10745_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_10745_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_busy@extended-modeset-hang-newfb-render-b:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_cursor_crc@cursor-128x128-onscreen:
      shard-apl:          PASS -> FAIL (fdo#103232) +3

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-skl:          PASS -> FAIL (fdo#103232)

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
      shard-hsw:          PASS -> FAIL (fdo#105767)

    igt@kms_draw_crc@draw-method-xrgb2101010-blt-xtiled:
      shard-skl:          PASS -> FAIL (fdo#103184) +2

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-cpu:
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
      shard-glk:          PASS -> FAIL (fdo#103167) +2

    igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-indfb-msflip-blt:
      shard-skl:          PASS -> FAIL (fdo#103167) +2

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108, fdo#107773)

    igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
      shard-skl:          PASS -> FAIL (fdo#107815, fdo#108145)

    igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
      shard-skl:          NOTRUN -> FAIL (fdo#108145)

    igt@kms_universal_plane@universal-plane-pipe-a-functional:
      shard-glk:          PASS -> FAIL (fdo#103166)

    igt@pm_rpm@universal-planes:
      shard-skl:          PASS -> INCOMPLETE (fdo#107807)

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-glk:          INCOMPLETE (fdo#106886, fdo#103359, k.org#198133) -> PASS

    igt@gem_ctx_isolation@vcs0-s3:
      shard-skl:          INCOMPLETE (fdo#104108, fdo#107773) -> PASS

    igt@gem_exec_reuse@baggage:
      shard-apl:          DMESG-WARN (fdo#108690) -> PASS

    igt@kms_color@pipe-a-ctm-max:
      shard-apl:          FAIL (fdo#108147) -> PASS

    igt@kms_cursor_crc@cursor-64x21-onscreen:
      shard-apl:          FAIL (fdo#103232) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-hsw:          FAIL (fdo#102887) -> PASS

    igt@kms_flip_tiling@flip-to-yf-tiled:
      shard-skl:          FAIL (fdo#107931) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
      shard-apl:          FAIL (fdo#103167) -> PASS +2

    igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
      shard-skl:          FAIL (fdo#107815) -> PASS

    igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
      shard-apl:          FAIL (fdo#103166) -> PASS +1

    igt@pm_rpm@dpms-lpsp:
      shard-skl:          INCOMPLETE (fdo#107807) -> PASS +2

    igt@prime_busy@after-vebox:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
  fdo#107931 https://bugs.freedesktop.org/show_bug.cgi?id=107931
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108147 https://bugs.freedesktop.org/show_bug.cgi?id=108147
  fdo#108690 https://bugs.freedesktop.org/show_bug.cgi?id=108690
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_5096 -> Patchwork_10745

  CI_DRM_5096: 9dd45e92a3e9b238d044adde1061a8ee0ce24b73 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4711: cc41f4c921e56c62c85ec5349c47022ae9b5f008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10745: f6d1a322301fb41cf1850279091178a5fef8ebaf @ 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_10745/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx