[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

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

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

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

[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