[PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes.

[PATCH v2 0/6] drm/atomic: Fix use-after-free with unbound connectors/planes.

[Maarten Lankhorst]

New iteration with all feedback hopefully addressed.

Maarten Lankhorst (6):
  drm/i915: Always wait for flip_done, v2.
  drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.
  drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.
  drm/atomic: Return commit in drm_crtc_commit_get for better annotation
  drm/atomic: Fix freeing connector/plane state too early by tracking
    commits, v3.
  drm/atomic: Make async plane update checks work as intended, v2.

 drivers/gpu/drm/drm_atomic.c         |  11 +-
 drivers/gpu/drm/drm_atomic_helper.c  | 293 +++++++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_drv.h      |   3 +-
 drivers/gpu/drm/i915/intel_display.c | 109 +++----------
 include/drm/drm_atomic.h             |  19 ++-
 include/drm/drm_connector.h          |   7 +
 include/drm/drm_crtc.h               |  23 ++-
 include/drm/drm_plane.h              |   8 +
 8 files changed, 275 insertions(+), 198 deletions(-)

-- 
2.11.0

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

[PATCH v2 1/6] drm/i915: Always wait for flip_done, v2.

[Maarten Lankhorst]

The next commit removes the wait for flip_done in in
drm_atomic_helper_commit_cleanup_done, but we need it for the tests
to pass. Instead of using complicated vblank tracking which ends
up being ignored anyway, call the correct atomic helper. :)

Changes since v1:
- Always call drm_atomic_helper_wait_for_flip_done,
  even for legacy cursor updates. (danvet)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 +-
 drivers/gpu/drm/i915/intel_display.c | 84 +++---------------------------------
 2 files changed, 8 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 38bd08f2539b..79533ba6f387 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -725,8 +725,7 @@ struct drm_i915_display_funcs {
 			    struct drm_atomic_state *old_state);
 	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
 			     struct drm_atomic_state *old_state);
-	void (*update_crtcs)(struct drm_atomic_state *state,
-			     unsigned int *crtc_vblank_mask);
+	void (*update_crtcs)(struct drm_atomic_state *state);
 	void (*audio_codec_enable)(struct drm_connector *connector,
 				   struct intel_encoder *encoder,
 				   const struct drm_display_mode *adjusted_mode);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 216cd9e0e08f..a6cf1c20c712 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12136,73 +12136,10 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
 	return dev->driver->get_vblank_counter(dev, crtc->pipe);
 }
 
-static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
-					  struct drm_i915_private *dev_priv,
-					  unsigned crtc_mask)
-{
-	unsigned last_vblank_count[I915_MAX_PIPES];
-	enum pipe pipe;
-	int ret;
-
-	if (!crtc_mask)
-		return;
-
-	for_each_pipe(dev_priv, pipe) {
-		struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
-								  pipe);
-
-		if (!((1 << pipe) & crtc_mask))
-			continue;
-
-		ret = drm_crtc_vblank_get(&crtc->base);
-		if (WARN_ON(ret != 0)) {
-			crtc_mask &= ~(1 << pipe);
-			continue;
-		}
-
-		last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
-	}
-
-	for_each_pipe(dev_priv, pipe) {
-		struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
-								  pipe);
-		long lret;
-
-		if (!((1 << pipe) & crtc_mask))
-			continue;
-
-		lret = wait_event_timeout(dev->vblank[pipe].queue,
-				last_vblank_count[pipe] !=
-					drm_crtc_vblank_count(&crtc->base),
-				msecs_to_jiffies(50));
-
-		WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
-
-		drm_crtc_vblank_put(&crtc->base);
-	}
-}
-
-static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
-{
-	/* fb updated, need to unpin old fb */
-	if (crtc_state->fb_changed)
-		return true;
-
-	/* wm changes, need vblank before final wm's */
-	if (crtc_state->update_wm_post)
-		return true;
-
-	if (crtc_state->wm.need_postvbl_update)
-		return true;
-
-	return false;
-}
-
 static void intel_update_crtc(struct drm_crtc *crtc,
 			      struct drm_atomic_state *state,
 			      struct drm_crtc_state *old_crtc_state,
-			      struct drm_crtc_state *new_crtc_state,
-			      unsigned int *crtc_vblank_mask)
+			      struct drm_crtc_state *new_crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -12225,13 +12162,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
 	}
 
 	drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
-
-	if (needs_vblank_wait(pipe_config))
-		*crtc_vblank_mask |= drm_crtc_mask(crtc);
 }
 
-static void intel_update_crtcs(struct drm_atomic_state *state,
-			       unsigned int *crtc_vblank_mask)
+static void intel_update_crtcs(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
@@ -12242,12 +12175,11 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
 			continue;
 
 		intel_update_crtc(crtc, state, old_crtc_state,
-				  new_crtc_state, crtc_vblank_mask);
+				  new_crtc_state);
 	}
 }
 
-static void skl_update_crtcs(struct drm_atomic_state *state,
-			     unsigned int *crtc_vblank_mask)
+static void skl_update_crtcs(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
@@ -12306,7 +12238,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 				vbl_wait = true;
 
 			intel_update_crtc(crtc, state, old_crtc_state,
-					  new_crtc_state, crtc_vblank_mask);
+					  new_crtc_state);
 
 			if (vbl_wait)
 				intel_wait_for_vblank(dev_priv, pipe);
@@ -12368,7 +12300,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	struct intel_crtc_state *intel_cstate;
 	bool hw_check = intel_state->modeset;
 	u64 put_domains[I915_MAX_PIPES] = {};
-	unsigned crtc_vblank_mask = 0;
 	int i;
 
 	intel_atomic_commit_fence_wait(intel_state);
@@ -12458,7 +12389,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
-	dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
+	dev_priv->display.update_crtcs(state);
 
 	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
 	 * already, but still need the state for the delayed optimization. To
@@ -12469,8 +12400,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	 * - switch over to the vblank wait helper in the core after that since
 	 *   we don't need out special handling any more.
 	 */
-	if (!state->legacy_cursor_update)
-		intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
+	drm_atomic_helper_wait_for_flip_done(dev, state);
 
 	/*
 	 * Now that the vblank has passed, we can go ahead and program the
-- 
2.11.0

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

Re: [PATCH v2 1/6] drm/i915: Always wait for flip_done, v2.

[Daniel Vetter]

On Mon, Sep 04, 2017 at 12:48:33PM +0200, Maarten Lankhorst wrote:
> The next commit removes the wait for flip_done in in
> drm_atomic_helper_commit_cleanup_done, but we need it for the tests
> to pass. Instead of using complicated vblank tracking which ends
> up being ignored anyway, call the correct atomic helper. :)
> 
> Changes since v1:
> - Always call drm_atomic_helper_wait_for_flip_done,
>   even for legacy cursor updates. (danvet)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
>  drivers/gpu/drm/i915/intel_display.c | 84 +++---------------------------------
>  2 files changed, 8 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 38bd08f2539b..79533ba6f387 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -725,8 +725,7 @@ struct drm_i915_display_funcs {
>  			    struct drm_atomic_state *old_state);
>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>  			     struct drm_atomic_state *old_state);
> -	void (*update_crtcs)(struct drm_atomic_state *state,
> -			     unsigned int *crtc_vblank_mask);
> +	void (*update_crtcs)(struct drm_atomic_state *state);
>  	void (*audio_codec_enable)(struct drm_connector *connector,
>  				   struct intel_encoder *encoder,
>  				   const struct drm_display_mode *adjusted_mode);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 216cd9e0e08f..a6cf1c20c712 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12136,73 +12136,10 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
>  	return dev->driver->get_vblank_counter(dev, crtc->pipe);
>  }
>  
> -static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
> -					  struct drm_i915_private *dev_priv,
> -					  unsigned crtc_mask)
> -{
> -	unsigned last_vblank_count[I915_MAX_PIPES];
> -	enum pipe pipe;
> -	int ret;
> -
> -	if (!crtc_mask)
> -		return;
> -
> -	for_each_pipe(dev_priv, pipe) {
> -		struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> -								  pipe);
> -
> -		if (!((1 << pipe) & crtc_mask))
> -			continue;
> -
> -		ret = drm_crtc_vblank_get(&crtc->base);
> -		if (WARN_ON(ret != 0)) {
> -			crtc_mask &= ~(1 << pipe);
> -			continue;
> -		}
> -
> -		last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
> -	}
> -
> -	for_each_pipe(dev_priv, pipe) {
> -		struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
> -								  pipe);
> -		long lret;
> -
> -		if (!((1 << pipe) & crtc_mask))
> -			continue;
> -
> -		lret = wait_event_timeout(dev->vblank[pipe].queue,
> -				last_vblank_count[pipe] !=
> -					drm_crtc_vblank_count(&crtc->base),
> -				msecs_to_jiffies(50));
> -
> -		WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
> -
> -		drm_crtc_vblank_put(&crtc->base);
> -	}
> -}
> -
> -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
> -{
> -	/* fb updated, need to unpin old fb */
> -	if (crtc_state->fb_changed)
> -		return true;
> -
> -	/* wm changes, need vblank before final wm's */
> -	if (crtc_state->update_wm_post)
> -		return true;
> -
> -	if (crtc_state->wm.need_postvbl_update)
> -		return true;
> -
> -	return false;
> -}
> -
>  static void intel_update_crtc(struct drm_crtc *crtc,
>  			      struct drm_atomic_state *state,
>  			      struct drm_crtc_state *old_crtc_state,
> -			      struct drm_crtc_state *new_crtc_state,
> -			      unsigned int *crtc_vblank_mask)
> +			      struct drm_crtc_state *new_crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -12225,13 +12162,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>  	}
>  
>  	drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
> -
> -	if (needs_vblank_wait(pipe_config))
> -		*crtc_vblank_mask |= drm_crtc_mask(crtc);
>  }
>  
> -static void intel_update_crtcs(struct drm_atomic_state *state,
> -			       unsigned int *crtc_vblank_mask)
> +static void intel_update_crtcs(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> @@ -12242,12 +12175,11 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
>  			continue;
>  
>  		intel_update_crtc(crtc, state, old_crtc_state,
> -				  new_crtc_state, crtc_vblank_mask);
> +				  new_crtc_state);
>  	}
>  }
>  
> -static void skl_update_crtcs(struct drm_atomic_state *state,
> -			     unsigned int *crtc_vblank_mask)
> +static void skl_update_crtcs(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> @@ -12306,7 +12238,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  				vbl_wait = true;
>  
>  			intel_update_crtc(crtc, state, old_crtc_state,
> -					  new_crtc_state, crtc_vblank_mask);
> +					  new_crtc_state);
>  
>  			if (vbl_wait)
>  				intel_wait_for_vblank(dev_priv, pipe);
> @@ -12368,7 +12300,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	struct intel_crtc_state *intel_cstate;
>  	bool hw_check = intel_state->modeset;
>  	u64 put_domains[I915_MAX_PIPES] = {};
> -	unsigned crtc_vblank_mask = 0;
>  	int i;
>  
>  	intel_atomic_commit_fence_wait(intel_state);
> @@ -12458,7 +12389,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> -	dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
> +	dev_priv->display.update_crtcs(state);
>  
>  	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
>  	 * already, but still need the state for the delayed optimization. To
> @@ -12469,8 +12400,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  	 * - switch over to the vblank wait helper in the core after that since
>  	 *   we don't need out special handling any more.
>  	 */
> -	if (!state->legacy_cursor_update)
> -		intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
> +	drm_atomic_helper_wait_for_flip_done(dev, state);

Superflous if dropped, I'm happy.

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

>  
>  	/*
>  	 * Now that the vblank has passed, we can go ahead and program the
> -- 
> 2.11.0
> 
> _______________________________________________
> 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 v2 1/6] drm/i915: Always wait for flip_done, v2.

[Maarten Lankhorst]

Op 07-09-17 om 11:49 schreef Daniel Vetter:
> On Mon, Sep 04, 2017 at 12:48:33PM +0200, Maarten Lankhorst wrote:
>> The next commit removes the wait for flip_done in in
>> drm_atomic_helper_commit_cleanup_done, but we need it for the tests
>> to pass. Instead of using complicated vblank tracking which ends
>> up being ignored anyway, call the correct atomic helper. :)
>>
>> Changes since v1:
>> - Always call drm_atomic_helper_wait_for_flip_done,
>>   even for legacy cursor updates. (danvet)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
>>  drivers/gpu/drm/i915/intel_display.c | 84 +++---------------------------------
>>  2 files changed, 8 insertions(+), 79 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 38bd08f2539b..79533ba6f387 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -725,8 +725,7 @@ struct drm_i915_display_funcs {
>>  			    struct drm_atomic_state *old_state);
>>  	void (*crtc_disable)(struct intel_crtc_state *old_crtc_state,
>>  			     struct drm_atomic_state *old_state);
>> -	void (*update_crtcs)(struct drm_atomic_state *state,
>> -			     unsigned int *crtc_vblank_mask);
>> +	void (*update_crtcs)(struct drm_atomic_state *state);
>>  	void (*audio_codec_enable)(struct drm_connector *connector,
>>  				   struct intel_encoder *encoder,
>>  				   const struct drm_display_mode *adjusted_mode);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 216cd9e0e08f..a6cf1c20c712 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12136,73 +12136,10 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
>>  	return dev->driver->get_vblank_counter(dev, crtc->pipe);
>>  }
>>  
>> -static void intel_atomic_wait_for_vblanks(struct drm_device *dev,
>> -					  struct drm_i915_private *dev_priv,
>> -					  unsigned crtc_mask)
>> -{
>> -	unsigned last_vblank_count[I915_MAX_PIPES];
>> -	enum pipe pipe;
>> -	int ret;
>> -
>> -	if (!crtc_mask)
>> -		return;
>> -
>> -	for_each_pipe(dev_priv, pipe) {
>> -		struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
>> -								  pipe);
>> -
>> -		if (!((1 << pipe) & crtc_mask))
>> -			continue;
>> -
>> -		ret = drm_crtc_vblank_get(&crtc->base);
>> -		if (WARN_ON(ret != 0)) {
>> -			crtc_mask &= ~(1 << pipe);
>> -			continue;
>> -		}
>> -
>> -		last_vblank_count[pipe] = drm_crtc_vblank_count(&crtc->base);
>> -	}
>> -
>> -	for_each_pipe(dev_priv, pipe) {
>> -		struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv,
>> -								  pipe);
>> -		long lret;
>> -
>> -		if (!((1 << pipe) & crtc_mask))
>> -			continue;
>> -
>> -		lret = wait_event_timeout(dev->vblank[pipe].queue,
>> -				last_vblank_count[pipe] !=
>> -					drm_crtc_vblank_count(&crtc->base),
>> -				msecs_to_jiffies(50));
>> -
>> -		WARN(!lret, "pipe %c vblank wait timed out\n", pipe_name(pipe));
>> -
>> -		drm_crtc_vblank_put(&crtc->base);
>> -	}
>> -}
>> -
>> -static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
>> -{
>> -	/* fb updated, need to unpin old fb */
>> -	if (crtc_state->fb_changed)
>> -		return true;
>> -
>> -	/* wm changes, need vblank before final wm's */
>> -	if (crtc_state->update_wm_post)
>> -		return true;
>> -
>> -	if (crtc_state->wm.need_postvbl_update)
>> -		return true;
>> -
>> -	return false;
>> -}
>> -
>>  static void intel_update_crtc(struct drm_crtc *crtc,
>>  			      struct drm_atomic_state *state,
>>  			      struct drm_crtc_state *old_crtc_state,
>> -			      struct drm_crtc_state *new_crtc_state,
>> -			      unsigned int *crtc_vblank_mask)
>> +			      struct drm_crtc_state *new_crtc_state)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>> @@ -12225,13 +12162,9 @@ static void intel_update_crtc(struct drm_crtc *crtc,
>>  	}
>>  
>>  	drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
>> -
>> -	if (needs_vblank_wait(pipe_config))
>> -		*crtc_vblank_mask |= drm_crtc_mask(crtc);
>>  }
>>  
>> -static void intel_update_crtcs(struct drm_atomic_state *state,
>> -			       unsigned int *crtc_vblank_mask)
>> +static void intel_update_crtcs(struct drm_atomic_state *state)
>>  {
>>  	struct drm_crtc *crtc;
>>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> @@ -12242,12 +12175,11 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
>>  			continue;
>>  
>>  		intel_update_crtc(crtc, state, old_crtc_state,
>> -				  new_crtc_state, crtc_vblank_mask);
>> +				  new_crtc_state);
>>  	}
>>  }
>>  
>> -static void skl_update_crtcs(struct drm_atomic_state *state,
>> -			     unsigned int *crtc_vblank_mask)
>> +static void skl_update_crtcs(struct drm_atomic_state *state)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> @@ -12306,7 +12238,7 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>>  				vbl_wait = true;
>>  
>>  			intel_update_crtc(crtc, state, old_crtc_state,
>> -					  new_crtc_state, crtc_vblank_mask);
>> +					  new_crtc_state);
>>  
>>  			if (vbl_wait)
>>  				intel_wait_for_vblank(dev_priv, pipe);
>> @@ -12368,7 +12300,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  	struct intel_crtc_state *intel_cstate;
>>  	bool hw_check = intel_state->modeset;
>>  	u64 put_domains[I915_MAX_PIPES] = {};
>> -	unsigned crtc_vblank_mask = 0;
>>  	int i;
>>  
>>  	intel_atomic_commit_fence_wait(intel_state);
>> @@ -12458,7 +12389,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  	}
>>  
>>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>> -	dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
>> +	dev_priv->display.update_crtcs(state);
>>  
>>  	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
>>  	 * already, but still need the state for the delayed optimization. To
>> @@ -12469,8 +12400,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>>  	 * - switch over to the vblank wait helper in the core after that since
>>  	 *   we don't need out special handling any more.
>>  	 */
>> -	if (!state->legacy_cursor_update)
>> -		intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask);
>> +	drm_atomic_helper_wait_for_flip_done(dev, state);
> Superflous if dropped, I'm happy.
Pushed the series, thanks for review. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/6] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.

[Maarten Lankhorst]

Most code only cares about the current commit or previous commit.
Fortuantely we already have a place to track those. Move it to
drm_crtc_state where it belongs. :)

The per-crtc commit_list is kept for places where we have to look
deeper than the current or previous commit for checking whether to stall
on unpin. This is used in drm_atomic_helper_setup_commit and
intel_has_pending_fb_unpin.

Changes since v1:
- Update kerneldoc for drm_crtc.commit_list. (danvet)
Changes since v2:
-Remove drm_atomic_helper_async_check hunk. (pinchartl)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic.c        |  7 ----
 drivers/gpu/drm/drm_atomic_helper.c | 64 ++++++++++++-------------------------
 include/drm/drm_atomic.h            |  1 -
 include/drm/drm_crtc.h              | 23 ++++++++++---
 4 files changed, 40 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2fd383d7253a..2cce48f203e0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		crtc->funcs->atomic_destroy_state(crtc,
 						  state->crtcs[i].state);
 
-		if (state->crtcs[i].commit) {
-			kfree(state->crtcs[i].commit->event);
-			state->crtcs[i].commit->event = NULL;
-			drm_crtc_commit_put(state->crtcs[i].commit);
-		}
-
-		state->crtcs[i].commit = NULL;
 		state->crtcs[i].ptr = NULL;
 		state->crtcs[i].state = NULL;
 	}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e53aae9a1fb..9001fc1023b1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
 					  struct drm_atomic_state *old_state)
 {
-	struct drm_crtc_state *unused;
+	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i;
 
-	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
-		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+		struct drm_crtc_commit *commit = new_crtc_state->commit;
 		int ret;
 
 		if (!commit)
@@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		kref_init(&commit->ref);
 		commit->crtc = crtc;
 
-		state->crtcs[i].commit = commit;
+		new_crtc_state->commit = commit;
 
 		ret = stall_checks(crtc, nonblock);
 		if (ret)
@@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
 
-
-static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
-{
-	struct drm_crtc_commit *commit;
-	int i = 0;
-
-	list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
-		/* skip the first entry, that's the current commit */
-		if (i == 1)
-			return commit;
-		i++;
-	}
-
-	return NULL;
-}
-
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
  * @old_state: atomic state object with old state structures
@@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
 void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *new_crtc_state;
+	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 	long ret;
 
-	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		spin_lock(&crtc->commit_lock);
-		commit = preceeding_commit(crtc);
-		if (commit)
-			drm_crtc_commit_get(commit);
-		spin_unlock(&crtc->commit_lock);
+	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		commit = old_crtc_state->commit;
 
 		if (!commit)
 			continue;
@@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 		if (ret == 0)
 			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
 				  crtc->base.id, crtc->name);
-
-		drm_crtc_commit_put(commit);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1857,7 +1835,7 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 	int i;
 
 	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		commit = old_state->crtcs[i].commit;
+		commit = new_crtc_state->commit;
 		if (!commit)
 			continue;
 
@@ -1888,7 +1866,7 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 	long ret;
 
 	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		commit = old_state->crtcs[i].commit;
+		commit = new_crtc_state->commit;
 		if (WARN_ON(!commit))
 			continue;
 
@@ -2294,20 +2272,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_private_state *old_obj_state, *new_obj_state;
 
 	if (stall) {
-		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
-			spin_lock(&crtc->commit_lock);
-			commit = list_first_entry_or_null(&crtc->commit_list,
-					struct drm_crtc_commit, commit_entry);
-			if (commit)
-				drm_crtc_commit_get(commit);
-			spin_unlock(&crtc->commit_lock);
+		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+			commit = old_crtc_state->commit;
 
 			if (!commit)
 				continue;
 
 			ret = wait_for_completion_interruptible(&commit->hw_done);
-			drm_crtc_commit_put(commit);
-
 			if (ret)
 				return ret;
 		}
@@ -2332,13 +2303,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		state->crtcs[i].state = old_crtc_state;
 		crtc->state = new_crtc_state;
 
-		if (state->crtcs[i].commit) {
+		if (new_crtc_state->commit) {
 			spin_lock(&crtc->commit_lock);
-			list_add(&state->crtcs[i].commit->commit_entry,
+			list_add(&new_crtc_state->commit->commit_entry,
 				 &crtc->commit_list);
 			spin_unlock(&crtc->commit_lock);
 
-			state->crtcs[i].commit->event = NULL;
+			new_crtc_state->commit->event = NULL;
 		}
 	}
 
@@ -3186,6 +3157,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	state->connectors_changed = false;
 	state->color_mgmt_changed = false;
 	state->zpos_changed = false;
+	state->commit = NULL;
 	state->event = NULL;
 	state->pageflip_flags = 0;
 }
@@ -3224,6 +3196,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
  */
 void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
 {
+	if (state->commit) {
+		kfree(state->commit->event);
+		state->commit->event = NULL;
+		drm_crtc_commit_put(state->commit);
+	}
+
 	drm_property_blob_put(state->mode_blob);
 	drm_property_blob_put(state->degamma_lut);
 	drm_property_blob_put(state->ctm);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f73b663c1f76..285fbc4ec360 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -144,7 +144,6 @@ struct __drm_planes_state {
 struct __drm_crtcs_state {
 	struct drm_crtc *ptr;
 	struct drm_crtc_state *state, *old_state, *new_state;
-	struct drm_crtc_commit *commit;
 	s32 __user *out_fence_ptr;
 	unsigned last_vblank_count;
 };
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1a642020e306..1a01ff4ea023 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -253,6 +253,15 @@ struct drm_crtc_state {
 	 */
 	struct drm_pending_vblank_event *event;
 
+	/**
+	 * @commit:
+	 *
+	 * This tracks how the commit for this update proceeds through the
+	 * various phases. This is never cleared, except when we destroy the
+	 * state, so that subsequent commits can synchronize with previous ones.
+	 */
+	struct drm_crtc_commit *commit;
+
 	struct drm_atomic_state *state;
 };
 
@@ -808,10 +817,16 @@ struct drm_crtc {
 	 * @commit_list:
 	 *
 	 * List of &drm_crtc_commit structures tracking pending commits.
-	 * Protected by @commit_lock. This list doesn't hold its own full
-	 * reference, but burrows it from the ongoing commit. Commit entries
-	 * must be removed from this list once the commit is fully completed,
-	 * but before it's correspoding &drm_atomic_state gets destroyed.
+	 * Protected by @commit_lock. This list holds its own full reference,
+	 * as does the ongoing commit.
+	 *
+	 * "Note that the commit for a state change is also tracked in
+	 * &drm_crtc_state.commit. For accessing the immediately preceeding
+	 * commit in an atomic update it is recommended to just use that
+	 * pointer in the old CRTC state, since accessing that doesn't need
+	 * any locking or list-walking. @commit_list should only be used to
+	 * stall for framebuffer cleanup that's signalled through
+	 * &drm_crtc_commit.cleanup_done."
 	 */
 	struct list_head commit_list;
 
-- 
2.11.0

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

[PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.

[Maarten Lankhorst]

Most code only cares about the current commit or previous commit.
Fortuantely we already have a place to track those. Move it to
drm_crtc_state where it belongs. :)

The per-crtc commit_list is kept for places where we have to look
deeper than the current or previous commit for checking whether to stall
on unpin. This is used in drm_atomic_helper_setup_commit and
intel_has_pending_fb_unpin.

Changes since v1:
- Update kerneldoc for drm_crtc.commit_list. (danvet)
Changes since v2:
- Remove drm_atomic_helper_async_check hunk. (pinchartl)
Changes since v3:
- Fix use-after-free in drm_atomic_helper_commit_cleanup_done().

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic.c        |  7 ----
 drivers/gpu/drm/drm_atomic_helper.c | 82 ++++++++++++++++---------------------
 include/drm/drm_atomic.h            |  1 -
 include/drm/drm_crtc.h              | 23 +++++++++--
 4 files changed, 54 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2fd383d7253a..2cce48f203e0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 		crtc->funcs->atomic_destroy_state(crtc,
 						  state->crtcs[i].state);
 
-		if (state->crtcs[i].commit) {
-			kfree(state->crtcs[i].commit->event);
-			state->crtcs[i].commit->event = NULL;
-			drm_crtc_commit_put(state->crtcs[i].commit);
-		}
-
-		state->crtcs[i].commit = NULL;
 		state->crtcs[i].ptr = NULL;
 		state->crtcs[i].state = NULL;
 	}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4e53aae9a1fb..80c138cbde9a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
 					  struct drm_atomic_state *old_state)
 {
-	struct drm_crtc_state *unused;
+	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc *crtc;
 	int i;
 
-	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
-		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
+	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
+		struct drm_crtc_commit *commit = new_crtc_state->commit;
 		int ret;
 
 		if (!commit)
@@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		kref_init(&commit->ref);
 		commit->crtc = crtc;
 
-		state->crtcs[i].commit = commit;
+		new_crtc_state->commit = commit;
 
 		ret = stall_checks(crtc, nonblock);
 		if (ret)
@@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
 
-
-static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
-{
-	struct drm_crtc_commit *commit;
-	int i = 0;
-
-	list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
-		/* skip the first entry, that's the current commit */
-		if (i == 1)
-			return commit;
-		i++;
-	}
-
-	return NULL;
-}
-
 /**
  * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
  * @old_state: atomic state object with old state structures
@@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
 void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *new_crtc_state;
+	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 	long ret;
 
-	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		spin_lock(&crtc->commit_lock);
-		commit = preceeding_commit(crtc);
-		if (commit)
-			drm_crtc_commit_get(commit);
-		spin_unlock(&crtc->commit_lock);
+	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		commit = old_crtc_state->commit;
 
 		if (!commit)
 			continue;
@@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 		if (ret == 0)
 			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
 				  crtc->base.id, crtc->name);
-
-		drm_crtc_commit_put(commit);
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
@@ -1852,15 +1830,25 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
 void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *new_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 
-	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		commit = old_state->crtcs[i].commit;
+	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
+		commit = new_crtc_state->commit;
 		if (!commit)
 			continue;
 
+		/*
+		 * copy new_crtc_state->commit to old_crtc_state->commit,
+		 * it's unsafe to touch new_crtc_state after hw_done,
+		 * but we still need to do so in cleanup_done().
+		 */
+		if (old_crtc_state->commit)
+			drm_crtc_commit_put(old_crtc_state->commit);
+
+		old_crtc_state->commit = drm_crtc_commit_get(commit);
+
 		/* backend must have consumed any event by now */
 		WARN_ON(new_crtc_state->event);
 		complete_all(&commit->hw_done);
@@ -1882,13 +1870,13 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
 void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *new_crtc_state;
+	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
 	long ret;
 
-	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		commit = old_state->crtcs[i].commit;
+	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		commit = old_crtc_state->commit;
 		if (WARN_ON(!commit))
 			continue;
 
@@ -2294,20 +2282,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_private_state *old_obj_state, *new_obj_state;
 
 	if (stall) {
-		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
-			spin_lock(&crtc->commit_lock);
-			commit = list_first_entry_or_null(&crtc->commit_list,
-					struct drm_crtc_commit, commit_entry);
-			if (commit)
-				drm_crtc_commit_get(commit);
-			spin_unlock(&crtc->commit_lock);
+		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+			commit = old_crtc_state->commit;
 
 			if (!commit)
 				continue;
 
 			ret = wait_for_completion_interruptible(&commit->hw_done);
-			drm_crtc_commit_put(commit);
-
 			if (ret)
 				return ret;
 		}
@@ -2332,13 +2313,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 		state->crtcs[i].state = old_crtc_state;
 		crtc->state = new_crtc_state;
 
-		if (state->crtcs[i].commit) {
+		if (new_crtc_state->commit) {
 			spin_lock(&crtc->commit_lock);
-			list_add(&state->crtcs[i].commit->commit_entry,
+			list_add(&new_crtc_state->commit->commit_entry,
 				 &crtc->commit_list);
 			spin_unlock(&crtc->commit_lock);
 
-			state->crtcs[i].commit->event = NULL;
+			new_crtc_state->commit->event = NULL;
 		}
 	}
 
@@ -3186,6 +3167,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	state->connectors_changed = false;
 	state->color_mgmt_changed = false;
 	state->zpos_changed = false;
+	state->commit = NULL;
 	state->event = NULL;
 	state->pageflip_flags = 0;
 }
@@ -3224,6 +3206,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
  */
 void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
 {
+	if (state->commit) {
+		kfree(state->commit->event);
+		state->commit->event = NULL;
+		drm_crtc_commit_put(state->commit);
+	}
+
 	drm_property_blob_put(state->mode_blob);
 	drm_property_blob_put(state->degamma_lut);
 	drm_property_blob_put(state->ctm);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index f73b663c1f76..285fbc4ec360 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -144,7 +144,6 @@ struct __drm_planes_state {
 struct __drm_crtcs_state {
 	struct drm_crtc *ptr;
 	struct drm_crtc_state *state, *old_state, *new_state;
-	struct drm_crtc_commit *commit;
 	s32 __user *out_fence_ptr;
 	unsigned last_vblank_count;
 };
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1a642020e306..1a01ff4ea023 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -253,6 +253,15 @@ struct drm_crtc_state {
 	 */
 	struct drm_pending_vblank_event *event;
 
+	/**
+	 * @commit:
+	 *
+	 * This tracks how the commit for this update proceeds through the
+	 * various phases. This is never cleared, except when we destroy the
+	 * state, so that subsequent commits can synchronize with previous ones.
+	 */
+	struct drm_crtc_commit *commit;
+
 	struct drm_atomic_state *state;
 };
 
@@ -808,10 +817,16 @@ struct drm_crtc {
 	 * @commit_list:
 	 *
 	 * List of &drm_crtc_commit structures tracking pending commits.
-	 * Protected by @commit_lock. This list doesn't hold its own full
-	 * reference, but burrows it from the ongoing commit. Commit entries
-	 * must be removed from this list once the commit is fully completed,
-	 * but before it's correspoding &drm_atomic_state gets destroyed.
+	 * Protected by @commit_lock. This list holds its own full reference,
+	 * as does the ongoing commit.
+	 *
+	 * "Note that the commit for a state change is also tracked in
+	 * &drm_crtc_state.commit. For accessing the immediately preceeding
+	 * commit in an atomic update it is recommended to just use that
+	 * pointer in the old CRTC state, since accessing that doesn't need
+	 * any locking or list-walking. @commit_list should only be used to
+	 * stall for framebuffer cleanup that's signalled through
+	 * &drm_crtc_commit.cleanup_done."
 	 */
 	struct list_head commit_list;
 
-- 
2.11.0

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

Re: [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]

Hi Maarten,

[auto build test ERROR on drm/drm-next]
[also build test ERROR on next-20170904]
[cannot apply to v4.13]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Maarten-Lankhorst/drm-atomic-Move-drm_crtc_commit-to-drm_crtc_state-v4/20170905-011023
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x016-201736 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu//drm/drm_atomic_helper.c: In function 'drm_atomic_helper_commit_hw_done':
>> drivers/gpu//drm/drm_atomic_helper.c:1850:26: error: void value not ignored as it ought to be
      old_crtc_state->commit = drm_crtc_commit_get(commit);
                             ^

vim +1850 drivers/gpu//drm/drm_atomic_helper.c

  1814	
  1815	/**
  1816	 * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
  1817	 * @old_state: atomic state object with old state structures
  1818	 *
  1819	 * This function is used to signal completion of the hardware commit step. After
  1820	 * this step the driver is not allowed to read or change any permanent software
  1821	 * or hardware modeset state. The only exception is state protected by other
  1822	 * means than &drm_modeset_lock locks.
  1823	 *
  1824	 * Drivers should try to postpone any expensive or delayed cleanup work after
  1825	 * this function is called.
  1826	 *
  1827	 * This is part of the atomic helper support for nonblocking commits, see
  1828	 * drm_atomic_helper_setup_commit() for an overview.
  1829	 */
  1830	void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
  1831	{
  1832		struct drm_crtc *crtc;
  1833		struct drm_crtc_state *old_crtc_state, *new_crtc_state;
  1834		struct drm_crtc_commit *commit;
  1835		int i;
  1836	
  1837		for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
  1838			commit = new_crtc_state->commit;
  1839			if (!commit)
  1840				continue;
  1841	
  1842			/*
  1843			 * copy new_crtc_state->commit to old_crtc_state->commit,
  1844			 * it's unsafe to touch new_crtc_state after hw_done,
  1845			 * but we still need to do so in cleanup_done().
  1846			 */
  1847			if (old_crtc_state->commit)
  1848				drm_crtc_commit_put(old_crtc_state->commit);
  1849	
> 1850			old_crtc_state->commit = drm_crtc_commit_get(commit);
  1851	
  1852			/* backend must have consumed any event by now */
  1853			WARN_ON(new_crtc_state->event);
  1854			complete_all(&commit->hw_done);
  1855		}
  1856	}
  1857	EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
  1858	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30752 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.

[Daniel Vetter]

On Mon, Sep 04, 2017 at 05:04:56PM +0200, Maarten Lankhorst wrote:
> Most code only cares about the current commit or previous commit.
> Fortuantely we already have a place to track those. Move it to
> drm_crtc_state where it belongs. :)
> 
> The per-crtc commit_list is kept for places where we have to look
> deeper than the current or previous commit for checking whether to stall
> on unpin. This is used in drm_atomic_helper_setup_commit and
> intel_has_pending_fb_unpin.
> 
> Changes since v1:
> - Update kerneldoc for drm_crtc.commit_list. (danvet)
> Changes since v2:
> - Remove drm_atomic_helper_async_check hunk. (pinchartl)
> Changes since v3:
> - Fix use-after-free in drm_atomic_helper_commit_cleanup_done().
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Might be good to drop this, or at least annoate that the r-b is for v3,
just for next time around. But I looked at the patch again, r-b: me still
holds I think. But then I missed the bug in v4 ...
-Daniel


> ---
>  drivers/gpu/drm/drm_atomic.c        |  7 ----
>  drivers/gpu/drm/drm_atomic_helper.c | 82 ++++++++++++++++---------------------
>  include/drm/drm_atomic.h            |  1 -
>  include/drm/drm_crtc.h              | 23 +++++++++--
>  4 files changed, 54 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2fd383d7253a..2cce48f203e0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -163,13 +163,6 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  		crtc->funcs->atomic_destroy_state(crtc,
>  						  state->crtcs[i].state);
>  
> -		if (state->crtcs[i].commit) {
> -			kfree(state->crtcs[i].commit->event);
> -			state->crtcs[i].commit->event = NULL;
> -			drm_crtc_commit_put(state->crtcs[i].commit);
> -		}
> -
> -		state->crtcs[i].commit = NULL;
>  		state->crtcs[i].ptr = NULL;
>  		state->crtcs[i].state = NULL;
>  	}
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 4e53aae9a1fb..80c138cbde9a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1262,12 +1262,12 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>  void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>  					  struct drm_atomic_state *old_state)
>  {
> -	struct drm_crtc_state *unused;
> +	struct drm_crtc_state *new_crtc_state;
>  	struct drm_crtc *crtc;
>  	int i;
>  
> -	for_each_new_crtc_in_state(old_state, crtc, unused, i) {
> -		struct drm_crtc_commit *commit = old_state->crtcs[i].commit;
> +	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> +		struct drm_crtc_commit *commit = new_crtc_state->commit;
>  		int ret;
>  
>  		if (!commit)
> @@ -1731,7 +1731,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  		kref_init(&commit->ref);
>  		commit->crtc = crtc;
>  
> -		state->crtcs[i].commit = commit;
> +		new_crtc_state->commit = commit;
>  
>  		ret = stall_checks(crtc, nonblock);
>  		if (ret)
> @@ -1769,22 +1769,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
>  
> -
> -static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> -{
> -	struct drm_crtc_commit *commit;
> -	int i = 0;
> -
> -	list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
> -		/* skip the first entry, that's the current commit */
> -		if (i == 1)
> -			return commit;
> -		i++;
> -	}
> -
> -	return NULL;
> -}
> -
>  /**
>   * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
>   * @old_state: atomic state object with old state structures
> @@ -1800,17 +1784,13 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>  void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state;
> +	struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  	long ret;
>  
> -	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		spin_lock(&crtc->commit_lock);
> -		commit = preceeding_commit(crtc);
> -		if (commit)
> -			drm_crtc_commit_get(commit);
> -		spin_unlock(&crtc->commit_lock);
> +	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +		commit = old_crtc_state->commit;
>  
>  		if (!commit)
>  			continue;
> @@ -1828,8 +1808,6 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  		if (ret == 0)
>  			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>  				  crtc->base.id, crtc->name);
> -
> -		drm_crtc_commit_put(commit);
>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> @@ -1852,15 +1830,25 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  
> -	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		commit = old_state->crtcs[i].commit;
> +	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> +		commit = new_crtc_state->commit;
>  		if (!commit)
>  			continue;
>  
> +		/*
> +		 * copy new_crtc_state->commit to old_crtc_state->commit,
> +		 * it's unsafe to touch new_crtc_state after hw_done,
> +		 * but we still need to do so in cleanup_done().
> +		 */
> +		if (old_crtc_state->commit)
> +			drm_crtc_commit_put(old_crtc_state->commit);
> +
> +		old_crtc_state->commit = drm_crtc_commit_get(commit);
> +
>  		/* backend must have consumed any event by now */
>  		WARN_ON(new_crtc_state->event);
>  		complete_all(&commit->hw_done);
> @@ -1882,13 +1870,13 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>  void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_crtc_state;
> +	struct drm_crtc_state *old_crtc_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  	long ret;
>  
> -	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> -		commit = old_state->crtcs[i].commit;
> +	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +		commit = old_crtc_state->commit;
>  		if (WARN_ON(!commit))
>  			continue;
>  
> @@ -2294,20 +2282,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_private_state *old_obj_state, *new_obj_state;
>  
>  	if (stall) {
> -		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> -			spin_lock(&crtc->commit_lock);
> -			commit = list_first_entry_or_null(&crtc->commit_list,
> -					struct drm_crtc_commit, commit_entry);
> -			if (commit)
> -				drm_crtc_commit_get(commit);
> -			spin_unlock(&crtc->commit_lock);
> +		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +			commit = old_crtc_state->commit;
>  
>  			if (!commit)
>  				continue;
>  
>  			ret = wait_for_completion_interruptible(&commit->hw_done);
> -			drm_crtc_commit_put(commit);
> -
>  			if (ret)
>  				return ret;
>  		}
> @@ -2332,13 +2313,13 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  		state->crtcs[i].state = old_crtc_state;
>  		crtc->state = new_crtc_state;
>  
> -		if (state->crtcs[i].commit) {
> +		if (new_crtc_state->commit) {
>  			spin_lock(&crtc->commit_lock);
> -			list_add(&state->crtcs[i].commit->commit_entry,
> +			list_add(&new_crtc_state->commit->commit_entry,
>  				 &crtc->commit_list);
>  			spin_unlock(&crtc->commit_lock);
>  
> -			state->crtcs[i].commit->event = NULL;
> +			new_crtc_state->commit->event = NULL;
>  		}
>  	}
>  
> @@ -3186,6 +3167,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  	state->connectors_changed = false;
>  	state->color_mgmt_changed = false;
>  	state->zpos_changed = false;
> +	state->commit = NULL;
>  	state->event = NULL;
>  	state->pageflip_flags = 0;
>  }
> @@ -3224,6 +3206,12 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>   */
>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>  {
> +	if (state->commit) {
> +		kfree(state->commit->event);
> +		state->commit->event = NULL;
> +		drm_crtc_commit_put(state->commit);
> +	}
> +
>  	drm_property_blob_put(state->mode_blob);
>  	drm_property_blob_put(state->degamma_lut);
>  	drm_property_blob_put(state->ctm);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index f73b663c1f76..285fbc4ec360 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -144,7 +144,6 @@ struct __drm_planes_state {
>  struct __drm_crtcs_state {
>  	struct drm_crtc *ptr;
>  	struct drm_crtc_state *state, *old_state, *new_state;
> -	struct drm_crtc_commit *commit;
>  	s32 __user *out_fence_ptr;
>  	unsigned last_vblank_count;
>  };
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1a642020e306..1a01ff4ea023 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -253,6 +253,15 @@ struct drm_crtc_state {
>  	 */
>  	struct drm_pending_vblank_event *event;
>  
> +	/**
> +	 * @commit:
> +	 *
> +	 * This tracks how the commit for this update proceeds through the
> +	 * various phases. This is never cleared, except when we destroy the
> +	 * state, so that subsequent commits can synchronize with previous ones.
> +	 */
> +	struct drm_crtc_commit *commit;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> @@ -808,10 +817,16 @@ struct drm_crtc {
>  	 * @commit_list:
>  	 *
>  	 * List of &drm_crtc_commit structures tracking pending commits.
> -	 * Protected by @commit_lock. This list doesn't hold its own full
> -	 * reference, but burrows it from the ongoing commit. Commit entries
> -	 * must be removed from this list once the commit is fully completed,
> -	 * but before it's correspoding &drm_atomic_state gets destroyed.
> +	 * Protected by @commit_lock. This list holds its own full reference,
> +	 * as does the ongoing commit.
> +	 *
> +	 * "Note that the commit for a state change is also tracked in
> +	 * &drm_crtc_state.commit. For accessing the immediately preceeding
> +	 * commit in an atomic update it is recommended to just use that
> +	 * pointer in the old CRTC state, since accessing that doesn't need
> +	 * any locking or list-walking. @commit_list should only be used to
> +	 * stall for framebuffer cleanup that's signalled through
> +	 * &drm_crtc_commit.cleanup_done."
>  	 */
>  	struct list_head commit_list;
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> 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

[PATCH v2 3/6] drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.

[Maarten Lankhorst]

When commit synchronization through drm_crtc_commit was first
introduced, we tried to solve the problem of the flip_done
needing a reference count by blocking in cleanup_done.

This has been changed by commit 24835e442f28 ("drm: reference count
event->completion") which made the waits here no longer needed.

However, even after this commit we still needed the wait because
otherwise we cannot wait for the flip_done because this item might
have been removed from the list.

Changed since v1:
- Make mention of cleanup_done completing before flip_done.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic_helper.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 9001fc1023b1..1f5cdafb050e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1863,7 +1863,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 	struct drm_crtc_state *new_crtc_state;
 	struct drm_crtc_commit *commit;
 	int i;
-	long ret;
 
 	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
 		commit = new_crtc_state->commit;
@@ -1873,22 +1872,6 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 		complete_all(&commit->cleanup_done);
 		WARN_ON(!try_wait_for_completion(&commit->hw_done));
 
-		/* commit_list borrows our reference, need to remove before we
-		 * clean up our drm_atomic_state. But only after it actually
-		 * completed, otherwise subsequent commits won't stall properly. */
-		if (try_wait_for_completion(&commit->flip_done))
-			goto del_commit;
-
-		/* We must wait for the vblank event to signal our completion
-		 * before releasing our reference, since the vblank work does
-		 * not hold a reference of its own. */
-		ret = wait_for_completion_timeout(&commit->flip_done,
-						  10*HZ);
-		if (ret == 0)
-			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
-				  crtc->base.id, crtc->name);
-
-del_commit:
 		spin_lock(&crtc->commit_lock);
 		list_del(&commit->commit_entry);
 		spin_unlock(&crtc->commit_lock);
-- 
2.11.0

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

[PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation

[Maarten Lankhorst]

This will allow code to do x->commit = drm_crtc_commit_get(commit),
making it clearer where references are used.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 3 +--
 include/drm/drm_atomic.h            | 6 +++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1f5cdafb050e..04629d883114 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1633,8 +1633,7 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
 				return -EBUSY;
 			}
 		} else if (i == 1) {
-			stall_commit = commit;
-			drm_crtc_commit_get(stall_commit);
+			stall_commit = drm_crtc_commit_get(commit);
 			break;
 		}
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 285fbc4ec360..a80a8dadef00 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -251,10 +251,14 @@ void __drm_crtc_commit_free(struct kref *kref);
  * @commit: CRTC commit
  *
  * Increases the reference of @commit.
+ *
+ * Returns:
+ * The pointer to @commit, with reference increased.
  */
-static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
+static inline struct drm_crtc_commit *drm_crtc_commit_get(struct drm_crtc_commit *commit)
 {
 	kref_get(&commit->ref);
+	return commit;
 }
 
 /**
-- 
2.11.0

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

Re: [PATCH v2 4/6] drm/atomic: Return commit in drm_crtc_commit_get for better annotation

[Daniel Vetter]

On Mon, Sep 04, 2017 at 12:48:36PM +0200, Maarten Lankhorst wrote:
> This will allow code to do x->commit = drm_crtc_commit_get(commit),
> making it clearer where references are used.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 3 +--
>  include/drm/drm_atomic.h            | 6 +++++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1f5cdafb050e..04629d883114 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1633,8 +1633,7 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
>  				return -EBUSY;
>  			}
>  		} else if (i == 1) {
> -			stall_commit = commit;
> -			drm_crtc_commit_get(stall_commit);
> +			stall_commit = drm_crtc_commit_get(commit);
>  			break;
>  		}
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 285fbc4ec360..a80a8dadef00 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -251,10 +251,14 @@ void __drm_crtc_commit_free(struct kref *kref);
>   * @commit: CRTC commit
>   *
>   * Increases the reference of @commit.
> + *
> + * Returns:
> + * The pointer to @commit, with reference increased.
>   */
> -static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
> +static inline struct drm_crtc_commit *drm_crtc_commit_get(struct drm_crtc_commit *commit)
>  {
>  	kref_get(&commit->ref);
> +	return commit;
>  }
>  
>  /**
> -- 
> 2.11.0
> 
> _______________________________________________
> 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

[PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.

[Maarten Lankhorst]

Currently we neatly track the crtc state, but forget to look at
plane/connector state.

When doing a nonblocking modeset, immediately followed by a setprop
before the modeset completes, the setprop will see the modesets new
state as the old state and free it.

This has to be solved by waiting for hw_done on the connector, even
if it's not assigned to a crtc. When a connector is unbound we take
the last crtc commit, and when it stays unbound we create a new
fake crtc commit for that gets signaled on hw_done for all the
planes/connectors.

We wait for it the same way as we do for crtc's, which will make
sure we never run into a use-after-free situation.

Changes since v1:
- Only create a single disable commit. (danvet)
- Fix leak in intel_legacy_cursor_update.
Changes since v2:
- Make reference counting in drm_atomic_helper_setup_commit
  more obvious. (pinchartl)
- Call cleanup_done for fake commit. (danvet)
- Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
- Add comment to drm_atomic_helper_swap_state. (pinchartl)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_atomic.c         |   4 +
 drivers/gpu/drm/drm_atomic_helper.c  | 172 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 include/drm/drm_atomic.h             |  12 +++
 include/drm/drm_connector.h          |   7 ++
 include/drm/drm_plane.h              |   7 ++
 6 files changed, 198 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2cce48f203e0..75f5f74de9bf 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 	}
 	state->num_private_objs = 0;
 
+	if (state->fake_commit) {
+		drm_crtc_commit_put(state->fake_commit);
+		state->fake_commit = NULL;
+	}
 }
 EXPORT_SYMBOL(drm_atomic_state_default_clear);
 
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 04629d883114..c81d46927a74 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion)
 	drm_crtc_commit_put(commit);
 }
 
+static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
+{
+	init_completion(&commit->flip_done);
+	init_completion(&commit->hw_done);
+	init_completion(&commit->cleanup_done);
+	INIT_LIST_HEAD(&commit->commit_entry);
+	kref_init(&commit->ref);
+	commit->crtc = crtc;
+}
+
+static struct drm_crtc_commit *
+crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
+{
+	if (crtc) {
+		struct drm_crtc_state *new_crtc_state;
+
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+		return new_crtc_state->commit;
+	}
+
+	if (!state->fake_commit) {
+		state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL);
+		if (!state->fake_commit)
+			return NULL;
+
+		init_commit(state->fake_commit, NULL);
+	}
+
+	return state->fake_commit;
+}
+
 /**
  * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
  * @state: new modeset state to be committed
@@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct drm_connector *conn;
+	struct drm_connector_state *old_conn_state, *new_conn_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct drm_crtc_commit *commit;
 	int i, ret;
 
@@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		if (!commit)
 			return -ENOMEM;
 
-		init_completion(&commit->flip_done);
-		init_completion(&commit->hw_done);
-		init_completion(&commit->cleanup_done);
-		INIT_LIST_HEAD(&commit->commit_entry);
-		kref_init(&commit->ref);
-		commit->crtc = crtc;
+		init_commit(commit, crtc);
 
 		new_crtc_state->commit = commit;
 
@@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		drm_crtc_commit_get(commit);
 	}
 
+	for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
+		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
+		if (new_conn_state->crtc)
+			continue;
+
+		/* Userspace is not allowed to get ahead of the previous
+		 * commit with nonblocking ones. */
+		if (nonblock && old_conn_state->commit &&
+		    !try_wait_for_completion(&old_conn_state->commit->flip_done))
+			return -EBUSY;
+
+		commit = crtc_or_fake_commit(state, old_conn_state->crtc);
+		if (!commit)
+			return -ENOMEM;
+
+		new_conn_state->commit = drm_crtc_commit_get(commit);
+	}
+
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
+		if (new_plane_state->crtc)
+			continue;
+
+		/* Userspace is not allowed to get ahead of the previous
+		 * commit with nonblocking ones. */
+		if (nonblock && old_plane_state->commit &&
+		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
+			return -EBUSY;
+
+		commit = crtc_or_fake_commit(state, old_plane_state->crtc);
+		if (!commit)
+			return -ENOMEM;
+
+		new_plane_state->commit = drm_crtc_commit_get(commit);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
@@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state;
+	struct drm_connector *conn;
+	struct drm_connector_state *old_conn_state;
 	struct drm_crtc_commit *commit;
 	int i;
 	long ret;
@@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
 				  crtc->base.id, crtc->name);
 	}
+
+	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
+		commit = old_conn_state->commit;
+
+		if (!commit)
+			continue;
+
+		ret = wait_for_completion_timeout(&commit->hw_done,
+						  10*HZ);
+		if (ret == 0)
+			DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
+				  conn->base.id, conn->name);
+
+		/* Currently no support for overwriting flips, hence
+		 * stall for previous one to execute completely. */
+		ret = wait_for_completion_timeout(&commit->flip_done,
+						  10*HZ);
+		if (ret == 0)
+			DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
+				  conn->base.id, conn->name);
+	}
+
+	for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
+		commit = old_plane_state->commit;
+
+		if (!commit)
+			continue;
+
+		ret = wait_for_completion_timeout(&commit->hw_done,
+						  10*HZ);
+		if (ret == 0)
+			DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
+				  plane->base.id, plane->name);
+
+		/* Currently no support for overwriting flips, hence
+		 * stall for previous one to execute completely. */
+		ret = wait_for_completion_timeout(&commit->flip_done,
+						  10*HZ);
+		if (ret == 0)
+			DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
+				  plane->base.id, plane->name);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
 
@@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
 		WARN_ON(new_crtc_state->event);
 		complete_all(&commit->hw_done);
 	}
+
+	if (old_state->fake_commit) {
+		complete_all(&old_state->fake_commit->hw_done);
+		complete_all(&old_state->fake_commit->flip_done);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
 
@@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 		list_del(&commit->commit_entry);
 		spin_unlock(&crtc->commit_lock);
 	}
+
+	if (old_state->fake_commit)
+		complete_all(&old_state->fake_commit->cleanup_done);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
 
@@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 	struct drm_private_state *old_obj_state, *new_obj_state;
 
 	if (stall) {
+		/*
+		 * We have to stall for hw_done here before
+		 * drm_atomic_helper_wait_for_dependencies() because flip
+		 * depth > 1 is not yet supported by all drivers. As long as
+		 * obj->state is directly dereferenced anywhere in the drivers
+		 * atomic_commit_tail function, then it's unsafe to swap state
+		 * before drm_atomic_helper_commit_hw_done() is called.
+		 */
+
 		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 			commit = old_crtc_state->commit;
 
@@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
 			if (ret)
 				return ret;
 		}
+
+		for_each_old_connector_in_state(state, connector, old_conn_state, i) {
+			commit = old_conn_state->commit;
+
+			if (!commit)
+				continue;
+
+			ret = wait_for_completion_interruptible(&commit->hw_done);
+			if (ret)
+				return ret;
+		}
+
+		for_each_old_plane_in_state(state, plane, old_plane_state, i) {
+			commit = old_plane_state->commit;
+
+			if (!commit)
+				continue;
+
+			ret = wait_for_completion_interruptible(&commit->hw_done);
+			if (ret)
+				return ret;
+		}
 	}
 
 	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
@@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 		drm_framebuffer_get(state->fb);
 
 	state->fence = NULL;
+	state->commit = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
 
@@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 
 	if (state->fence)
 		dma_fence_put(state->fence);
+
+	if (state->commit)
+		drm_crtc_commit_put(state->commit);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
@@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
 	memcpy(state, connector->state, sizeof(*state));
 	if (state->crtc)
 		drm_connector_get(connector);
+	state->commit = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
 {
 	if (state->crtc)
 		drm_connector_put(state->connector);
+
+	if (state->commit)
+		drm_crtc_commit_put(state->commit);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a6cf1c20c712..7abbc761a635 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 
 	/* Swap plane state */
 	new_plane_state->fence = old_plane_state->fence;
+	new_plane_state->commit = old_plane_state->commit;
 	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
 	new_plane_state->fence = NULL;
+	new_plane_state->commit = NULL;
 	new_plane_state->fb = old_fb;
 	to_intel_plane_state(new_plane_state)->vma = NULL;
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index a80a8dadef00..07a71daa3582 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -236,6 +236,18 @@ struct drm_atomic_state {
 	struct drm_modeset_acquire_ctx *acquire_ctx;
 
 	/**
+	 * @fake_commit:
+	 *
+	 * Used for signaling unbound planes/connectors.
+	 * When a connector or plane is not bound to any CRTC, it's still important
+	 * to preserve linearity to prevent the atomic states from being freed to early.
+	 *
+	 * This commit (if set) is not bound to any crtc, but will be completed when
+	 * drm_atomic_helper_commit_hw_done() is called.
+	 */
+	struct drm_crtc_commit *fake_commit;
+
+	/**
 	 * @commit_work:
 	 *
 	 * Work item which can be used by the driver or helpers to execute the
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ea8da401c93c..8837649d16e8 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -347,6 +347,13 @@ struct drm_connector_state {
 
 	struct drm_atomic_state *state;
 
+	/**
+	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
+	 *
+	 * Is only set when @crtc is NULL.
+	 */
+	struct drm_crtc_commit *commit;
+
 	struct drm_tv_connector_state tv;
 
 	/**
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 73f90f9d057f..7d96116fd4c4 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -123,6 +123,13 @@ struct drm_plane_state {
 	 */
 	bool visible;
 
+	/**
+	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
+	 *
+	 * Is only set when @crtc is NULL.
+	 */
+	struct drm_crtc_commit *commit;
+
 	struct drm_atomic_state *state;
 };
 
-- 
2.11.0

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

Re: [PATCH v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.

[Daniel Vetter]

On Mon, Sep 04, 2017 at 12:48:37PM +0200, Maarten Lankhorst wrote:
> Currently we neatly track the crtc state, but forget to look at
> plane/connector state.
> 
> When doing a nonblocking modeset, immediately followed by a setprop
> before the modeset completes, the setprop will see the modesets new
> state as the old state and free it.
> 
> This has to be solved by waiting for hw_done on the connector, even
> if it's not assigned to a crtc. When a connector is unbound we take
> the last crtc commit, and when it stays unbound we create a new
> fake crtc commit for that gets signaled on hw_done for all the
> planes/connectors.
> 
> We wait for it the same way as we do for crtc's, which will make
> sure we never run into a use-after-free situation.
> 
> Changes since v1:
> - Only create a single disable commit. (danvet)
> - Fix leak in intel_legacy_cursor_update.
> Changes since v2:
> - Make reference counting in drm_atomic_helper_setup_commit
>   more obvious. (pinchartl)
> - Call cleanup_done for fake commit. (danvet)
> - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
> - Add comment to drm_atomic_helper_swap_state. (pinchartl)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_atomic.c         |   4 +
>  drivers/gpu/drm/drm_atomic_helper.c  | 172 +++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |   2 +
>  include/drm/drm_atomic.h             |  12 +++
>  include/drm/drm_connector.h          |   7 ++
>  include/drm/drm_plane.h              |   7 ++
>  6 files changed, 198 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2cce48f203e0..75f5f74de9bf 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  	}
>  	state->num_private_objs = 0;
>  
> +	if (state->fake_commit) {
> +		drm_crtc_commit_put(state->fake_commit);
> +		state->fake_commit = NULL;
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>  
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 04629d883114..c81d46927a74 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion)
>  	drm_crtc_commit_put(commit);
>  }
>  
> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
> +{
> +	init_completion(&commit->flip_done);
> +	init_completion(&commit->hw_done);
> +	init_completion(&commit->cleanup_done);
> +	INIT_LIST_HEAD(&commit->commit_entry);
> +	kref_init(&commit->ref);
> +	commit->crtc = crtc;
> +}
> +
> +static struct drm_crtc_commit *
> +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)

Bikeshed: Would be nice if this function directly increases the refcount,
instead of imposing this on all callers. Would need a rename too like
crtc_or_fake_commit_get().

But since this bug is randomly killing our hsw CI and causing lots of
noise better to push as-is and polish later on.

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

> +{
> +	if (crtc) {
> +		struct drm_crtc_state *new_crtc_state;
> +
> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> +		return new_crtc_state->commit;
> +	}
> +
> +	if (!state->fake_commit) {
> +		state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL);
> +		if (!state->fake_commit)
> +			return NULL;
> +
> +		init_commit(state->fake_commit, NULL);
> +	}
> +
> +	return state->fake_commit;
> +}
> +
>  /**
>   * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
>   * @state: new modeset state to be committed
> @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	struct drm_connector *conn;
> +	struct drm_connector_state *old_conn_state, *new_conn_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct drm_crtc_commit *commit;
>  	int i, ret;
>  
> @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  		if (!commit)
>  			return -ENOMEM;
>  
> -		init_completion(&commit->flip_done);
> -		init_completion(&commit->hw_done);
> -		init_completion(&commit->cleanup_done);
> -		INIT_LIST_HEAD(&commit->commit_entry);
> -		kref_init(&commit->ref);
> -		commit->crtc = crtc;
> +		init_commit(commit, crtc);
>  
>  		new_crtc_state->commit = commit;
>  
> @@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  		drm_crtc_commit_get(commit);
>  	}
>  
> +	for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
> +		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> +		if (new_conn_state->crtc)
> +			continue;
> +
> +		/* Userspace is not allowed to get ahead of the previous
> +		 * commit with nonblocking ones. */
> +		if (nonblock && old_conn_state->commit &&
> +		    !try_wait_for_completion(&old_conn_state->commit->flip_done))
> +			return -EBUSY;
> +
> +		commit = crtc_or_fake_commit(state, old_conn_state->crtc);
> +		if (!commit)
> +			return -ENOMEM;
> +
> +		new_conn_state->commit = drm_crtc_commit_get(commit);
> +	}
> +
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> +		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> +		if (new_plane_state->crtc)
> +			continue;
> +
> +		/* Userspace is not allowed to get ahead of the previous
> +		 * commit with nonblocking ones. */
> +		if (nonblock && old_plane_state->commit &&
> +		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
> +			return -EBUSY;
> +
> +		commit = crtc_or_fake_commit(state, old_plane_state->crtc);
> +		if (!commit)
> +			return -ENOMEM;
> +
> +		new_plane_state->commit = drm_crtc_commit_get(commit);
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> @@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state;
> +	struct drm_connector *conn;
> +	struct drm_connector_state *old_conn_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
>  	long ret;
> @@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>  				  crtc->base.id, crtc->name);
>  	}
> +
> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
> +		commit = old_conn_state->commit;
> +
> +		if (!commit)
> +			continue;
> +
> +		ret = wait_for_completion_timeout(&commit->hw_done,
> +						  10*HZ);
> +		if (ret == 0)
> +			DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
> +				  conn->base.id, conn->name);
> +
> +		/* Currently no support for overwriting flips, hence
> +		 * stall for previous one to execute completely. */
> +		ret = wait_for_completion_timeout(&commit->flip_done,
> +						  10*HZ);
> +		if (ret == 0)
> +			DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
> +				  conn->base.id, conn->name);
> +	}
> +
> +	for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
> +		commit = old_plane_state->commit;
> +
> +		if (!commit)
> +			continue;
> +
> +		ret = wait_for_completion_timeout(&commit->hw_done,
> +						  10*HZ);
> +		if (ret == 0)
> +			DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
> +				  plane->base.id, plane->name);
> +
> +		/* Currently no support for overwriting flips, hence
> +		 * stall for previous one to execute completely. */
> +		ret = wait_for_completion_timeout(&commit->flip_done,
> +						  10*HZ);
> +		if (ret == 0)
> +			DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
> +				  plane->base.id, plane->name);
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  
> @@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  		WARN_ON(new_crtc_state->event);
>  		complete_all(&commit->hw_done);
>  	}
> +
> +	if (old_state->fake_commit) {
> +		complete_all(&old_state->fake_commit->hw_done);
> +		complete_all(&old_state->fake_commit->flip_done);
> +	}
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>  
> @@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
>  		list_del(&commit->commit_entry);
>  		spin_unlock(&crtc->commit_lock);
>  	}
> +
> +	if (old_state->fake_commit)
> +		complete_all(&old_state->fake_commit->cleanup_done);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
>  
> @@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  	struct drm_private_state *old_obj_state, *new_obj_state;
>  
>  	if (stall) {
> +		/*
> +		 * We have to stall for hw_done here before
> +		 * drm_atomic_helper_wait_for_dependencies() because flip
> +		 * depth > 1 is not yet supported by all drivers. As long as
> +		 * obj->state is directly dereferenced anywhere in the drivers
> +		 * atomic_commit_tail function, then it's unsafe to swap state
> +		 * before drm_atomic_helper_commit_hw_done() is called.
> +		 */
> +
>  		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>  			commit = old_crtc_state->commit;
>  
> @@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  			if (ret)
>  				return ret;
>  		}
> +
> +		for_each_old_connector_in_state(state, connector, old_conn_state, i) {
> +			commit = old_conn_state->commit;
> +
> +			if (!commit)
> +				continue;
> +
> +			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		for_each_old_plane_in_state(state, plane, old_plane_state, i) {
> +			commit = old_plane_state->commit;
> +
> +			if (!commit)
> +				continue;
> +
> +			ret = wait_for_completion_interruptible(&commit->hw_done);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
> @@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  		drm_framebuffer_get(state->fb);
>  
>  	state->fence = NULL;
> +	state->commit = NULL;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>  
> @@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>  
>  	if (state->fence)
>  		dma_fence_put(state->fence);
> +
> +	if (state->commit)
> +		drm_crtc_commit_put(state->commit);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> @@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>  	memcpy(state, connector->state, sizeof(*state));
>  	if (state->crtc)
>  		drm_connector_get(connector);
> +	state->commit = NULL;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>  
> @@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
>  {
>  	if (state->crtc)
>  		drm_connector_put(state->connector);
> +
> +	if (state->commit)
> +		drm_crtc_commit_put(state->commit);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a6cf1c20c712..7abbc761a635 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  
>  	/* Swap plane state */
>  	new_plane_state->fence = old_plane_state->fence;
> +	new_plane_state->commit = old_plane_state->commit;
>  	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
>  	new_plane_state->fence = NULL;
> +	new_plane_state->commit = NULL;
>  	new_plane_state->fb = old_fb;
>  	to_intel_plane_state(new_plane_state)->vma = NULL;
>  
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index a80a8dadef00..07a71daa3582 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -236,6 +236,18 @@ struct drm_atomic_state {
>  	struct drm_modeset_acquire_ctx *acquire_ctx;
>  
>  	/**
> +	 * @fake_commit:
> +	 *
> +	 * Used for signaling unbound planes/connectors.
> +	 * When a connector or plane is not bound to any CRTC, it's still important
> +	 * to preserve linearity to prevent the atomic states from being freed to early.
> +	 *
> +	 * This commit (if set) is not bound to any crtc, but will be completed when
> +	 * drm_atomic_helper_commit_hw_done() is called.
> +	 */
> +	struct drm_crtc_commit *fake_commit;
> +
> +	/**
>  	 * @commit_work:
>  	 *
>  	 * Work item which can be used by the driver or helpers to execute the
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ea8da401c93c..8837649d16e8 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -347,6 +347,13 @@ struct drm_connector_state {
>  
>  	struct drm_atomic_state *state;
>  
> +	/**
> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
> +	 *
> +	 * Is only set when @crtc is NULL.
> +	 */
> +	struct drm_crtc_commit *commit;
> +
>  	struct drm_tv_connector_state tv;
>  
>  	/**
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 73f90f9d057f..7d96116fd4c4 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -123,6 +123,13 @@ struct drm_plane_state {
>  	 */
>  	bool visible;
>  
> +	/**
> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
> +	 *
> +	 * Is only set when @crtc is NULL.
> +	 */
> +	struct drm_crtc_commit *commit;
> +
>  	struct drm_atomic_state *state;
>  };
>  
> -- 
> 2.11.0
> 
> _______________________________________________
> 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 v2 5/6] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.

[Maarten Lankhorst]

Op 07-09-17 om 12:05 schreef Daniel Vetter:
> On Mon, Sep 04, 2017 at 12:48:37PM +0200, Maarten Lankhorst wrote:
>> Currently we neatly track the crtc state, but forget to look at
>> plane/connector state.
>>
>> When doing a nonblocking modeset, immediately followed by a setprop
>> before the modeset completes, the setprop will see the modesets new
>> state as the old state and free it.
>>
>> This has to be solved by waiting for hw_done on the connector, even
>> if it's not assigned to a crtc. When a connector is unbound we take
>> the last crtc commit, and when it stays unbound we create a new
>> fake crtc commit for that gets signaled on hw_done for all the
>> planes/connectors.
>>
>> We wait for it the same way as we do for crtc's, which will make
>> sure we never run into a use-after-free situation.
>>
>> Changes since v1:
>> - Only create a single disable commit. (danvet)
>> - Fix leak in intel_legacy_cursor_update.
>> Changes since v2:
>> - Make reference counting in drm_atomic_helper_setup_commit
>>   more obvious. (pinchartl)
>> - Call cleanup_done for fake commit. (danvet)
>> - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
>> - Add comment to drm_atomic_helper_swap_state. (pinchartl)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c         |   4 +
>>  drivers/gpu/drm/drm_atomic_helper.c  | 172 +++++++++++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_display.c |   2 +
>>  include/drm/drm_atomic.h             |  12 +++
>>  include/drm/drm_connector.h          |   7 ++
>>  include/drm/drm_plane.h              |   7 ++
>>  6 files changed, 198 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 2cce48f203e0..75f5f74de9bf 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>>  	}
>>  	state->num_private_objs = 0;
>>  
>> +	if (state->fake_commit) {
>> +		drm_crtc_commit_put(state->fake_commit);
>> +		state->fake_commit = NULL;
>> +	}
>>  }
>>  EXPORT_SYMBOL(drm_atomic_state_default_clear);
>>  
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 04629d883114..c81d46927a74 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion)
>>  	drm_crtc_commit_put(commit);
>>  }
>>  
>> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
>> +{
>> +	init_completion(&commit->flip_done);
>> +	init_completion(&commit->hw_done);
>> +	init_completion(&commit->cleanup_done);
>> +	INIT_LIST_HEAD(&commit->commit_entry);
>> +	kref_init(&commit->ref);
>> +	commit->crtc = crtc;
>> +}
>> +
>> +static struct drm_crtc_commit *
>> +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
> Bikeshed: Would be nice if this function directly increases the refcount,
> instead of imposing this on all callers. Would need a rename too like
> crtc_or_fake_commit_get().
>
> But since this bug is randomly killing our hsw CI and causing lots of
> noise better to push as-is and polish later on.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I chose not to, to make it explicit that a extra refcount is used on the state object.
But sending one final version to trybot to make sure that things don't blow up with the merge conflicts in patch 6. :)
>> +{
>> +	if (crtc) {
>> +		struct drm_crtc_state *new_crtc_state;
>> +
>> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>> +
>> +		return new_crtc_state->commit;
>> +	}
>> +
>> +	if (!state->fake_commit) {
>> +		state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL);
>> +		if (!state->fake_commit)
>> +			return NULL;
>> +
>> +		init_commit(state->fake_commit, NULL);
>> +	}
>> +
>> +	return state->fake_commit;
>> +}
>> +
>>  /**
>>   * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
>>   * @state: new modeset state to be committed
>> @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>>  {
>>  	struct drm_crtc *crtc;
>>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> +	struct drm_connector *conn;
>> +	struct drm_connector_state *old_conn_state, *new_conn_state;
>> +	struct drm_plane *plane;
>> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>>  	struct drm_crtc_commit *commit;
>>  	int i, ret;
>>  
>> @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>>  		if (!commit)
>>  			return -ENOMEM;
>>  
>> -		init_completion(&commit->flip_done);
>> -		init_completion(&commit->hw_done);
>> -		init_completion(&commit->cleanup_done);
>> -		INIT_LIST_HEAD(&commit->commit_entry);
>> -		kref_init(&commit->ref);
>> -		commit->crtc = crtc;
>> +		init_commit(commit, crtc);
>>  
>>  		new_crtc_state->commit = commit;
>>  
>> @@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>>  		drm_crtc_commit_get(commit);
>>  	}
>>  
>> +	for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
>> +		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
>> +		if (new_conn_state->crtc)
>> +			continue;
>> +
>> +		/* Userspace is not allowed to get ahead of the previous
>> +		 * commit with nonblocking ones. */
>> +		if (nonblock && old_conn_state->commit &&
>> +		    !try_wait_for_completion(&old_conn_state->commit->flip_done))
>> +			return -EBUSY;
>> +
>> +		commit = crtc_or_fake_commit(state, old_conn_state->crtc);
>> +		if (!commit)
>> +			return -ENOMEM;
>> +
>> +		new_conn_state->commit = drm_crtc_commit_get(commit);
>> +	}
>> +
>> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>> +		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
>> +		if (new_plane_state->crtc)
>> +			continue;
>> +
>> +		/* Userspace is not allowed to get ahead of the previous
>> +		 * commit with nonblocking ones. */
>> +		if (nonblock && old_plane_state->commit &&
>> +		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
>> +			return -EBUSY;
>> +
>> +		commit = crtc_or_fake_commit(state, old_plane_state->crtc);
>> +		if (!commit)
>> +			return -ENOMEM;
>> +
>> +		new_plane_state->commit = drm_crtc_commit_get(commit);
>> +	}
>> +
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
>> @@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>>  {
>>  	struct drm_crtc *crtc;
>>  	struct drm_crtc_state *old_crtc_state;
>> +	struct drm_plane *plane;
>> +	struct drm_plane_state *old_plane_state;
>> +	struct drm_connector *conn;
>> +	struct drm_connector_state *old_conn_state;
>>  	struct drm_crtc_commit *commit;
>>  	int i;
>>  	long ret;
>> @@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>>  			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>>  				  crtc->base.id, crtc->name);
>>  	}
>> +
>> +	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>> +		commit = old_conn_state->commit;
>> +
>> +		if (!commit)
>> +			continue;
>> +
>> +		ret = wait_for_completion_timeout(&commit->hw_done,
>> +						  10*HZ);
>> +		if (ret == 0)
>> +			DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
>> +				  conn->base.id, conn->name);
>> +
>> +		/* Currently no support for overwriting flips, hence
>> +		 * stall for previous one to execute completely. */
>> +		ret = wait_for_completion_timeout(&commit->flip_done,
>> +						  10*HZ);
>> +		if (ret == 0)
>> +			DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
>> +				  conn->base.id, conn->name);
>> +	}
>> +
>> +	for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
>> +		commit = old_plane_state->commit;
>> +
>> +		if (!commit)
>> +			continue;
>> +
>> +		ret = wait_for_completion_timeout(&commit->hw_done,
>> +						  10*HZ);
>> +		if (ret == 0)
>> +			DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
>> +				  plane->base.id, plane->name);
>> +
>> +		/* Currently no support for overwriting flips, hence
>> +		 * stall for previous one to execute completely. */
>> +		ret = wait_for_completion_timeout(&commit->flip_done,
>> +						  10*HZ);
>> +		if (ret == 0)
>> +			DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
>> +				  plane->base.id, plane->name);
>> +	}
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>>  
>> @@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>>  		WARN_ON(new_crtc_state->event);
>>  		complete_all(&commit->hw_done);
>>  	}
>> +
>> +	if (old_state->fake_commit) {
>> +		complete_all(&old_state->fake_commit->hw_done);
>> +		complete_all(&old_state->fake_commit->flip_done);
>> +	}
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>>  
>> @@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
>>  		list_del(&commit->commit_entry);
>>  		spin_unlock(&crtc->commit_lock);
>>  	}
>> +
>> +	if (old_state->fake_commit)
>> +		complete_all(&old_state->fake_commit->cleanup_done);
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
>>  
>> @@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>  	struct drm_private_state *old_obj_state, *new_obj_state;
>>  
>>  	if (stall) {
>> +		/*
>> +		 * We have to stall for hw_done here before
>> +		 * drm_atomic_helper_wait_for_dependencies() because flip
>> +		 * depth > 1 is not yet supported by all drivers. As long as
>> +		 * obj->state is directly dereferenced anywhere in the drivers
>> +		 * atomic_commit_tail function, then it's unsafe to swap state
>> +		 * before drm_atomic_helper_commit_hw_done() is called.
>> +		 */
>> +
>>  		for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>  			commit = old_crtc_state->commit;
>>  
>> @@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>>  			if (ret)
>>  				return ret;
>>  		}
>> +
>> +		for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>> +			commit = old_conn_state->commit;
>> +
>> +			if (!commit)
>> +				continue;
>> +
>> +			ret = wait_for_completion_interruptible(&commit->hw_done);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>> +		for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>> +			commit = old_plane_state->commit;
>> +
>> +			if (!commit)
>> +				continue;
>> +
>> +			ret = wait_for_completion_interruptible(&commit->hw_done);
>> +			if (ret)
>> +				return ret;
>> +		}
>>  	}
>>  
>>  	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
>> @@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>>  		drm_framebuffer_get(state->fb);
>>  
>>  	state->fence = NULL;
>> +	state->commit = NULL;
>>  }
>>  EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>>  
>> @@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
>>  
>>  	if (state->fence)
>>  		dma_fence_put(state->fence);
>> +
>> +	if (state->commit)
>> +		drm_crtc_commit_put(state->commit);
>>  }
>>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>>  
>> @@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>>  	memcpy(state, connector->state, sizeof(*state));
>>  	if (state->crtc)
>>  		drm_connector_get(connector);
>> +	state->commit = NULL;
>>  }
>>  EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>>  
>> @@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
>>  {
>>  	if (state->crtc)
>>  		drm_connector_put(state->connector);
>> +
>> +	if (state->commit)
>> +		drm_crtc_commit_put(state->commit);
>>  }
>>  EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a6cf1c20c712..7abbc761a635 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>>  
>>  	/* Swap plane state */
>>  	new_plane_state->fence = old_plane_state->fence;
>> +	new_plane_state->commit = old_plane_state->commit;
>>  	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
>>  	new_plane_state->fence = NULL;
>> +	new_plane_state->commit = NULL;
>>  	new_plane_state->fb = old_fb;
>>  	to_intel_plane_state(new_plane_state)->vma = NULL;
>>  
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index a80a8dadef00..07a71daa3582 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -236,6 +236,18 @@ struct drm_atomic_state {
>>  	struct drm_modeset_acquire_ctx *acquire_ctx;
>>  
>>  	/**
>> +	 * @fake_commit:
>> +	 *
>> +	 * Used for signaling unbound planes/connectors.
>> +	 * When a connector or plane is not bound to any CRTC, it's still important
>> +	 * to preserve linearity to prevent the atomic states from being freed to early.
>> +	 *
>> +	 * This commit (if set) is not bound to any crtc, but will be completed when
>> +	 * drm_atomic_helper_commit_hw_done() is called.
>> +	 */
>> +	struct drm_crtc_commit *fake_commit;
>> +
>> +	/**
>>  	 * @commit_work:
>>  	 *
>>  	 * Work item which can be used by the driver or helpers to execute the
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index ea8da401c93c..8837649d16e8 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -347,6 +347,13 @@ struct drm_connector_state {
>>  
>>  	struct drm_atomic_state *state;
>>  
>> +	/**
>> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
>> +	 *
>> +	 * Is only set when @crtc is NULL.
>> +	 */
>> +	struct drm_crtc_commit *commit;
>> +
>>  	struct drm_tv_connector_state tv;
>>  
>>  	/**
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 73f90f9d057f..7d96116fd4c4 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
>> @@ -123,6 +123,13 @@ struct drm_plane_state {
>>  	 */
>>  	bool visible;
>>  
>> +	/**
>> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
>> +	 *
>> +	 * Is only set when @crtc is NULL.
>> +	 */
>> +	struct drm_crtc_commit *commit;
>> +
>>  	struct drm_atomic_state *state;
>>  };
>>  
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

[PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2.

[Maarten Lankhorst]

By always keeping track of the last commit in plane_state, we know
whether there is an active update on the plane or not. With that
information we can reject the fast update, and force the slowpath
to be used as was originally intended.

We cannot use plane_state->crtc->state here, because this only mentions
the most recent commit for the crtc, but not the planes that were part
of it. We specifically care about what the last commit involving this
plane is, which can only be tracked with a pointer in the plane state.

Changes since v1:
- Clean up the whole function here, instead of partially earlier.
- Add mention in the commit message why we need commit in plane_state.
- Swap plane->state in intel_legacy_cursor_update, instead of
  reassigning all variables. With this commit We know that the cursor
  is not part of any active commits so this hack can be removed.

Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> #v1
---
 drivers/gpu/drm/drm_atomic_helper.c  | 53 ++++++++++--------------------------
 drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
 include/drm/drm_plane.h              |  5 ++--
 3 files changed, 35 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c81d46927a74..a2cd432d8b2d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	struct drm_crtc_commit *commit;
-	struct drm_plane *__plane, *plane = NULL;
-	struct drm_plane_state *__plane_state, *plane_state = NULL;
+	struct drm_plane *plane;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	const struct drm_plane_helper_funcs *funcs;
-	int i, j, n_planes = 0;
+	int i, n_planes = 0;
 
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		if (drm_atomic_crtc_needs_modeset(crtc_state))
 			return -EINVAL;
 	}
 
-	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
 		n_planes++;
-		plane = __plane;
-		plane_state = __plane_state;
-	}
 
 	/* FIXME: we support only single plane updates for now */
-	if (!plane || n_planes != 1)
+	if (n_planes != 1)
 		return -EINVAL;
 
-	if (!plane_state->crtc)
+	if (!new_plane_state->crtc)
 		return -EINVAL;
 
 	funcs = plane->helper_private;
 	if (!funcs->atomic_async_update)
 		return -EINVAL;
 
-	if (plane_state->fence)
+	if (new_plane_state->fence)
 		return -EINVAL;
 
 	/*
@@ -1424,31 +1420,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 	 * the plane.  This prevents our async update's changes from getting
 	 * overridden by a previous synchronous update's state.
 	 */
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		if (plane->crtc != crtc)
-			continue;
-
-		spin_lock(&crtc->commit_lock);
-		commit = list_first_entry_or_null(&crtc->commit_list,
-						  struct drm_crtc_commit,
-						  commit_entry);
-		if (!commit) {
-			spin_unlock(&crtc->commit_lock);
-			continue;
-		}
-		spin_unlock(&crtc->commit_lock);
-
-		if (!crtc->state->state)
-			continue;
+	if (old_plane_state->commit &&
+	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
+		return -EBUSY;
 
-		for_each_plane_in_state(crtc->state->state, __plane,
-					__plane_state, j) {
-			if (__plane == plane)
-				return -EINVAL;
-		}
-	}
-
-	return funcs->atomic_async_check(plane, plane_state);
+	return funcs->atomic_async_check(plane, new_plane_state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_async_check);
 
@@ -1814,9 +1790,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 	}
 
 	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
-		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
-		if (new_plane_state->crtc)
-			continue;
+		/*
+		 * Unlike connectors, always track planes explicitly for
+		 * async pageflip support.
+		 */
 
 		/* Userspace is not allowed to get ahead of the previous
 		 * commit with nonblocking ones. */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7abbc761a635..0add029d95f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13064,6 +13064,14 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 		goto slow;
 
 	old_plane_state = plane->state;
+	/*
+	 * Don't do an async update if there is an outstanding commit modifying
+	 * the plane.  This prevents our async update's changes from getting
+	 * overridden by a previous synchronous update's state.
+	 */
+	if (old_plane_state->commit &&
+	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
+		goto slow;
 
 	/*
 	 * If any parameters change that may affect watermarks,
@@ -13125,19 +13133,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	}
 
 	old_fb = old_plane_state->fb;
-	old_vma = to_intel_plane_state(old_plane_state)->vma;
 
 	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
 			  intel_plane->frontbuffer_bit);
 
 	/* Swap plane state */
-	new_plane_state->fence = old_plane_state->fence;
-	new_plane_state->commit = old_plane_state->commit;
-	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
-	new_plane_state->fence = NULL;
-	new_plane_state->commit = NULL;
-	new_plane_state->fb = old_fb;
-	to_intel_plane_state(new_plane_state)->vma = NULL;
+	plane->state = new_plane_state;
 
 	if (plane->state->visible) {
 		trace_intel_update_plane(plane, to_intel_crtc(crtc));
@@ -13149,13 +13150,19 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 		intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
 	}
 
-	if (old_vma)
+	old_vma = to_intel_plane_state(old_plane_state)->vma;
+	if (old_vma) {
+		to_intel_plane_state(old_plane_state)->vma = NULL;
 		intel_unpin_fb_vma(old_vma);
+	}
 
 out_unlock:
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 out_free:
-	intel_plane_destroy_state(plane, new_plane_state);
+	if (ret)
+		intel_plane_destroy_state(plane, new_plane_state);
+	else
+		intel_plane_destroy_state(plane, old_plane_state);
 	return ret;
 
 slow:
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 7d96116fd4c4..feb9941d0cdb 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -124,9 +124,10 @@ struct drm_plane_state {
 	bool visible;
 
 	/**
-	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
+	 * @commit: Tracks the pending commit to prevent use-after-free conditions,
+	 * and for async plane updates.
 	 *
-	 * Is only set when @crtc is NULL.
+	 * May be NULL.
 	 */
 	struct drm_crtc_commit *commit;
 
-- 
2.11.0

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

Re: [PATCH v2 6/6] drm/atomic: Make async plane update checks work as intended, v2.

[Dmitry Osipenko]

On 04.09.2017 13:48, Maarten Lankhorst wrote:
> By always keeping track of the last commit in plane_state, we know
> whether there is an active update on the plane or not. With that
> information we can reject the fast update, and force the slowpath
> to be used as was originally intended.
> 
> We cannot use plane_state->crtc->state here, because this only mentions
> the most recent commit for the crtc, but not the planes that were part
> of it. We specifically care about what the last commit involving this
> plane is, which can only be tracked with a pointer in the plane state.
> 
> Changes since v1:
> - Clean up the whole function here, instead of partially earlier.
> - Add mention in the commit message why we need commit in plane_state.
> - Swap plane->state in intel_legacy_cursor_update, instead of
>   reassigning all variables. With this commit We know that the cursor
>   is not part of any active commits so this hack can be removed.
> 
> Cc: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Reviewed-by: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org> #v1
> ---
>  drivers/gpu/drm/drm_atomic_helper.c  | 53 ++++++++++--------------------------
>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
>  include/drm/drm_plane.h              |  5 ++--
>  3 files changed, 35 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c81d46927a74..a2cd432d8b2d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc_commit *commit;
> -	struct drm_plane *__plane, *plane = NULL;
> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	const struct drm_plane_helper_funcs *funcs;
> -	int i, j, n_planes = 0;
> +	int i, n_planes = 0;
>  
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
>  			return -EINVAL;
>  	}
>  
> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
>  		n_planes++;
> -		plane = __plane;
> -		plane_state = __plane_state;
> -	}
>  
>  	/* FIXME: we support only single plane updates for now */
> -	if (!plane || n_planes != 1)
> +	if (n_planes != 1)
>  		return -EINVAL;
>  
> -	if (!plane_state->crtc)
> +	if (!new_plane_state->crtc)
>  		return -EINVAL;
>  

Hello,

It looks like a NULL-checking of new_plane_state is missed here.

[   70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
(drm_atomic_helper_check+0x4c/0x64)
[   70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
(drm_atomic_check_only+0x2f4/0x56c)
[   70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
(drm_atomic_commit+0x10/0x50)
[   70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
(drm_atomic_helper_update_plane+0xf0/0x100)
[   70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
(__setplane_internal+0x178/0x214)
[   70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
(drm_mode_cursor_universal+0x118/0x1a8)
[   70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
(drm_mode_cursor_common+0x174/0x1ec)
[   70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
(drm_mode_cursor_ioctl+0x58/0x60)
[   70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
(drm_ioctl+0x198/0x368)
[   70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
[   70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
[   70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
(ret_fast_syscall+0x0/0x48)

It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
Tegra cursor patches sometime soon.

This variant works for me:

	if (!new_plane_state || !new_plane_state->crtc)
		return -EINVAL;

>  	funcs = plane->helper_private;
>  	if (!funcs->atomic_async_update)
>  		return -EINVAL;
>  
> -	if (plane_state->fence)
> +	if (new_plane_state->fence)
>  		return -EINVAL;
>  
>  	/*
> @@ -1424,31 +1420,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  	 * the plane.  This prevents our async update's changes from getting
>  	 * overridden by a previous synchronous update's state.
>  	 */
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (plane->crtc != crtc)
> -			continue;
> -
> -		spin_lock(&crtc->commit_lock);
> -		commit = list_first_entry_or_null(&crtc->commit_list,
> -						  struct drm_crtc_commit,
> -						  commit_entry);
> -		if (!commit) {
> -			spin_unlock(&crtc->commit_lock);
> -			continue;
> -		}
> -		spin_unlock(&crtc->commit_lock);
> -
> -		if (!crtc->state->state)
> -			continue;
> +	if (old_plane_state->commit &&
> +	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
> +		return -EBUSY;
>  
> -		for_each_plane_in_state(crtc->state->state, __plane,
> -					__plane_state, j) {
> -			if (__plane == plane)
> -				return -EINVAL;
> -		}
> -	}
> -
> -	return funcs->atomic_async_check(plane, plane_state);
> +	return funcs->atomic_async_check(plane, new_plane_state);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_async_check);
>  
> @@ -1814,9 +1790,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  	}
>  
>  	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> -		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> -		if (new_plane_state->crtc)
> -			continue;
> +		/*
> +		 * Unlike connectors, always track planes explicitly for
> +		 * async pageflip support.
> +		 */
>  
>  		/* Userspace is not allowed to get ahead of the previous
>  		 * commit with nonblocking ones. */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7abbc761a635..0add029d95f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13064,6 +13064,14 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  		goto slow;
>  
>  	old_plane_state = plane->state;
> +	/*
> +	 * Don't do an async update if there is an outstanding commit modifying
> +	 * the plane.  This prevents our async update's changes from getting
> +	 * overridden by a previous synchronous update's state.
> +	 */
> +	if (old_plane_state->commit &&
> +	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
> +		goto slow;
>  
>  	/*
>  	 * If any parameters change that may affect watermarks,
> @@ -13125,19 +13133,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	}
>  
>  	old_fb = old_plane_state->fb;
> -	old_vma = to_intel_plane_state(old_plane_state)->vma;
>  
>  	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
>  			  intel_plane->frontbuffer_bit);
>  
>  	/* Swap plane state */
> -	new_plane_state->fence = old_plane_state->fence;
> -	new_plane_state->commit = old_plane_state->commit;
> -	*to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
> -	new_plane_state->fence = NULL;
> -	new_plane_state->commit = NULL;
> -	new_plane_state->fb = old_fb;
> -	to_intel_plane_state(new_plane_state)->vma = NULL;
> +	plane->state = new_plane_state;
>  
>  	if (plane->state->visible) {
>  		trace_intel_update_plane(plane, to_intel_crtc(crtc));
> @@ -13149,13 +13150,19 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  		intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
>  	}
>  
> -	if (old_vma)
> +	old_vma = to_intel_plane_state(old_plane_state)->vma;
> +	if (old_vma) {
> +		to_intel_plane_state(old_plane_state)->vma = NULL;
>  		intel_unpin_fb_vma(old_vma);
> +	}
>  
>  out_unlock:
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  out_free:
> -	intel_plane_destroy_state(plane, new_plane_state);
> +	if (ret)
> +		intel_plane_destroy_state(plane, new_plane_state);
> +	else
> +		intel_plane_destroy_state(plane, old_plane_state);
>  	return ret;
>  
>  slow:
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 7d96116fd4c4..feb9941d0cdb 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -124,9 +124,10 @@ struct drm_plane_state {
>  	bool visible;
>  
>  	/**
> -	 * @commit: Tracks the pending commit to prevent use-after-free conditions.
> +	 * @commit: Tracks the pending commit to prevent use-after-free conditions,
> +	 * and for async plane updates.
>  	 *
> -	 * Is only set when @crtc is NULL.
> +	 * May be NULL.
>  	 */
>  	struct drm_crtc_commit *commit;
>  
> 


-- 
Dmitry

[PATCH] drm/atomic: Make async plane update checks actually work as intended.

[Maarten Lankhorst]

Op 24-09-17 om 16:33 schreef Dmitry Osipenko:
> On 04.09.2017 13:48, Maarten Lankhorst wrote:
>> By always keeping track of the last commit in plane_state, we know
>> whether there is an active update on the plane or not. With that
>> information we can reject the fast update, and force the slowpath
>> to be used as was originally intended.
>>
>> We cannot use plane_state->crtc->state here, because this only mentions
>> the most recent commit for the crtc, but not the planes that were part
>> of it. We specifically care about what the last commit involving this
>> plane is, which can only be tracked with a pointer in the plane state.
>>
>> Changes since v1:
>> - Clean up the whole function here, instead of partially earlier.
>> - Add mention in the commit message why we need commit in plane_state.
>> - Swap plane->state in intel_legacy_cursor_update, instead of
>>   reassigning all variables. With this commit We know that the cursor
>>   is not part of any active commits so this hack can be removed.
>>
>> Cc: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> Reviewed-by: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org> #v1
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c  | 53 ++++++++++--------------------------
>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
>>  include/drm/drm_plane.h              |  5 ++--
>>  3 files changed, 35 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index c81d46927a74..a2cd432d8b2d 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>>  {
>>  	struct drm_crtc *crtc;
>>  	struct drm_crtc_state *crtc_state;
>> -	struct drm_crtc_commit *commit;
>> -	struct drm_plane *__plane, *plane = NULL;
>> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
>> +	struct drm_plane *plane;
>> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>>  	const struct drm_plane_helper_funcs *funcs;
>> -	int i, j, n_planes = 0;
>> +	int i, n_planes = 0;
>>  
>>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
>>  			return -EINVAL;
>>  	}
>>  
>> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
>> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
>>  		n_planes++;
>> -		plane = __plane;
>> -		plane_state = __plane_state;
>> -	}
>>  
>>  	/* FIXME: we support only single plane updates for now */
>> -	if (!plane || n_planes != 1)
>> +	if (n_planes != 1)
>>  		return -EINVAL;
>>  
>> -	if (!plane_state->crtc)
>> +	if (!new_plane_state->crtc)
>>  		return -EINVAL;
>>  
> Hello,
>
> It looks like a NULL-checking of new_plane_state is missed here.
>
> [   70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
> (drm_atomic_helper_check+0x4c/0x64)
> [   70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
> (drm_atomic_check_only+0x2f4/0x56c)
> [   70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
> (drm_atomic_commit+0x10/0x50)
> [   70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
> (drm_atomic_helper_update_plane+0xf0/0x100)
> [   70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
> (__setplane_internal+0x178/0x214)
> [   70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
> (drm_mode_cursor_universal+0x118/0x1a8)
> [   70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
> (drm_mode_cursor_common+0x174/0x1ec)
> [   70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
> (drm_mode_cursor_ioctl+0x58/0x60)
> [   70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
> (drm_ioctl+0x198/0x368)
> [   70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
> [   70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
> [   70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
> (ret_fast_syscall+0x0/0x48)
>
> It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
> currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
> Tegra cursor patches sometime soon.

Oops.. I messed up there.. for_each_new_plane_in_state overwrites the state internally..
----->8-----
for_each_oldnew_plane_in_state overwrites the iterator values internally, so we cannot rely
on it being set to the last valid plane. This was causing a NULL pointer deref when someone
tries to use the code. Save the plane and use the accessor functions to pull out the relevant
plane state.

Cc: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 32fb61169b4f..8573feaea8c0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1388,23 +1388,30 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	struct drm_plane *plane;
+	struct drm_plane *plane = NULL, *__plane;
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	const struct drm_plane_helper_funcs *funcs;
-	int i, n_planes = 0;
+	int i;
 
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		if (drm_atomic_crtc_needs_modeset(crtc_state))
 			return -EINVAL;
 	}
 
-	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
-		n_planes++;
+	for_each_new_plane_in_state(state, __plane, new_plane_state, i) {
+		/* FIXME: we support only single plane updates for now */
+		if (plane)
+			return -EINVAL;
+
+		plane = __plane;
+	}
 
-	/* FIXME: we support only single plane updates for now */
-	if (n_planes != 1)
+	if (!plane)
 		return -EINVAL;
 
+	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+
 	if (!new_plane_state->crtc)
 		return -EINVAL;
 

Re: [PATCH] drm/atomic: Make async plane update checks actually work as intended.

[Dmitry Osipenko]

On 25.09.2017 09:43, Maarten Lankhorst wrote:
> Op 24-09-17 om 16:33 schreef Dmitry Osipenko:
>> On 04.09.2017 13:48, Maarten Lankhorst wrote:
>>> By always keeping track of the last commit in plane_state, we know
>>> whether there is an active update on the plane or not. With that
>>> information we can reject the fast update, and force the slowpath
>>> to be used as was originally intended.
>>>
>>> We cannot use plane_state->crtc->state here, because this only mentions
>>> the most recent commit for the crtc, but not the planes that were part
>>> of it. We specifically care about what the last commit involving this
>>> plane is, which can only be tracked with a pointer in the plane state.
>>>
>>> Changes since v1:
>>> - Clean up the whole function here, instead of partially earlier.
>>> - Add mention in the commit message why we need commit in plane_state.
>>> - Swap plane->state in intel_legacy_cursor_update, instead of
>>>   reassigning all variables. With this commit We know that the cursor
>>>   is not part of any active commits so this hack can be removed.
>>>
>>> Cc: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>>> Reviewed-by: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org> #v1
>>> ---
>>>  drivers/gpu/drm/drm_atomic_helper.c  | 53 ++++++++++--------------------------
>>>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
>>>  include/drm/drm_plane.h              |  5 ++--
>>>  3 files changed, 35 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index c81d46927a74..a2cd432d8b2d 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>>>  {
>>>  	struct drm_crtc *crtc;
>>>  	struct drm_crtc_state *crtc_state;
>>> -	struct drm_crtc_commit *commit;
>>> -	struct drm_plane *__plane, *plane = NULL;
>>> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
>>> +	struct drm_plane *plane;
>>> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>>>  	const struct drm_plane_helper_funcs *funcs;
>>> -	int i, j, n_planes = 0;
>>> +	int i, n_planes = 0;
>>>  
>>>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
>>>  			return -EINVAL;
>>>  	}
>>>  
>>> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
>>> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
>>>  		n_planes++;
>>> -		plane = __plane;
>>> -		plane_state = __plane_state;
>>> -	}
>>>  
>>>  	/* FIXME: we support only single plane updates for now */
>>> -	if (!plane || n_planes != 1)
>>> +	if (n_planes != 1)
>>>  		return -EINVAL;
>>>  
>>> -	if (!plane_state->crtc)
>>> +	if (!new_plane_state->crtc)
>>>  		return -EINVAL;
>>>  
>> Hello,
>>
>> It looks like a NULL-checking of new_plane_state is missed here.
>>
>> [   70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
>> (drm_atomic_helper_check+0x4c/0x64)
>> [   70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
>> (drm_atomic_check_only+0x2f4/0x56c)
>> [   70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
>> (drm_atomic_commit+0x10/0x50)
>> [   70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
>> (drm_atomic_helper_update_plane+0xf0/0x100)
>> [   70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
>> (__setplane_internal+0x178/0x214)
>> [   70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
>> (drm_mode_cursor_universal+0x118/0x1a8)
>> [   70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
>> (drm_mode_cursor_common+0x174/0x1ec)
>> [   70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
>> (drm_mode_cursor_ioctl+0x58/0x60)
>> [   70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
>> (drm_ioctl+0x198/0x368)
>> [   70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
>> [   70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
>> [   70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
>> (ret_fast_syscall+0x0/0x48)
>>
>> It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
>> currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
>> Tegra cursor patches sometime soon.
> 
> Oops.. I messed up there.. for_each_new_plane_in_state overwrites the state internally..
> ----->8-----
> for_each_oldnew_plane_in_state overwrites the iterator values internally, so we cannot rely
> on it being set to the last valid plane. This was causing a NULL pointer deref when someone
> tries to use the code. Save the plane and use the accessor functions to pull out the relevant
> plane state.
> 
> Cc: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---

It works! Thank you.

Tested-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 32fb61169b4f..8573feaea8c0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1388,23 +1388,30 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_plane *plane;
> +	struct drm_plane *plane = NULL, *__plane;
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	const struct drm_plane_helper_funcs *funcs;
> -	int i, n_planes = 0;
> +	int i;
>  
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
>  			return -EINVAL;
>  	}
>  
> -	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> -		n_planes++;
> +	for_each_new_plane_in_state(state, __plane, new_plane_state, i) {
> +		/* FIXME: we support only single plane updates for now */
> +		if (plane)
> +			return -EINVAL;
> +
> +		plane = __plane;
> +	}
>  
> -	/* FIXME: we support only single plane updates for now */
> -	if (n_planes != 1)
> +	if (!plane)
>  		return -EINVAL;
>  
> +	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +
>  	if (!new_plane_state->crtc)
>  		return -EINVAL;
>  
> 


-- 
Dmitry

Re: [PATCH] drm/atomic: Make async plane update checks actually work as intended.

[Daniel Vetter]

On Mon, Sep 25, 2017 at 08:43:44AM +0200, Maarten Lankhorst wrote:
> Op 24-09-17 om 16:33 schreef Dmitry Osipenko:
> > On 04.09.2017 13:48, Maarten Lankhorst wrote:
> >> By always keeping track of the last commit in plane_state, we know
> >> whether there is an active update on the plane or not. With that
> >> information we can reject the fast update, and force the slowpath
> >> to be used as was originally intended.
> >>
> >> We cannot use plane_state->crtc->state here, because this only mentions
> >> the most recent commit for the crtc, but not the planes that were part
> >> of it. We specifically care about what the last commit involving this
> >> plane is, which can only be tracked with a pointer in the plane state.
> >>
> >> Changes since v1:
> >> - Clean up the whole function here, instead of partially earlier.
> >> - Add mention in the commit message why we need commit in plane_state.
> >> - Swap plane->state in intel_legacy_cursor_update, instead of
> >>   reassigning all variables. With this commit We know that the cursor
> >>   is not part of any active commits so this hack can be removed.
> >>
> >> Cc: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >> Reviewed-by: Gustavo Padovan <gustavo.padovan-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> >> Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org> #v1
> >> ---
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 53 ++++++++++--------------------------
> >>  drivers/gpu/drm/i915/intel_display.c | 27 +++++++++++-------
> >>  include/drm/drm_plane.h              |  5 ++--
> >>  3 files changed, 35 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index c81d46927a74..a2cd432d8b2d 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -1388,35 +1388,31 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> >>  {
> >>  	struct drm_crtc *crtc;
> >>  	struct drm_crtc_state *crtc_state;
> >> -	struct drm_crtc_commit *commit;
> >> -	struct drm_plane *__plane, *plane = NULL;
> >> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
> >> +	struct drm_plane *plane;
> >> +	struct drm_plane_state *old_plane_state, *new_plane_state;
> >>  	const struct drm_plane_helper_funcs *funcs;
> >> -	int i, j, n_planes = 0;
> >> +	int i, n_planes = 0;
> >>  
> >>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> >>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
> >>  			return -EINVAL;
> >>  	}
> >>  
> >> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> >> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> >>  		n_planes++;
> >> -		plane = __plane;
> >> -		plane_state = __plane_state;
> >> -	}
> >>  
> >>  	/* FIXME: we support only single plane updates for now */
> >> -	if (!plane || n_planes != 1)
> >> +	if (n_planes != 1)
> >>  		return -EINVAL;
> >>  
> >> -	if (!plane_state->crtc)
> >> +	if (!new_plane_state->crtc)
> >>  		return -EINVAL;
> >>  
> > Hello,
> >
> > It looks like a NULL-checking of new_plane_state is missed here.
> >
> > [   70.138947] [<c03dd670>] (drm_atomic_helper_async_check) from [<c03e0424>]
> > (drm_atomic_helper_check+0x4c/0x64)
> > [   70.138958] [<c03e0424>] (drm_atomic_helper_check) from [<c03fb6d4>]
> > (drm_atomic_check_only+0x2f4/0x56c)
> > [   70.138969] [<c03fb6d4>] (drm_atomic_check_only) from [<c03fb95c>]
> > (drm_atomic_commit+0x10/0x50)
> > [   70.138979] [<c03fb95c>] (drm_atomic_commit) from [<c03dde50>]
> > (drm_atomic_helper_update_plane+0xf0/0x100)
> > [   70.138991] [<c03dde50>] (drm_atomic_helper_update_plane) from [<c0403548>]
> > (__setplane_internal+0x178/0x214)
> > [   70.139003] [<c0403548>] (__setplane_internal) from [<c04036fc>]
> > (drm_mode_cursor_universal+0x118/0x1a8)
> > [   70.139014] [<c04036fc>] (drm_mode_cursor_universal) from [<c0403900>]
> > (drm_mode_cursor_common+0x174/0x1ec)
> > [   70.139026] [<c0403900>] (drm_mode_cursor_common) from [<c0403fa4>]
> > (drm_mode_cursor_ioctl+0x58/0x60)
> > [   70.139036] [<c0403fa4>] (drm_mode_cursor_ioctl) from [<c03eb0b4>]
> > (drm_ioctl+0x198/0x368)
> > [   70.139047] [<c03eb0b4>] (drm_ioctl) from [<c015fffc>] (do_vfs_ioctl+0x9c/0x8f0)
> > [   70.139058] [<c015fffc>] (do_vfs_ioctl) from [<c0160884>] (SyS_ioctl+0x34/0x5c)
> > [   70.139071] [<c0160884>] (SyS_ioctl) from [<c000f900>]
> > (ret_fast_syscall+0x0/0x48)
> >
> > It's triggered by Tegra DRM driver on Xorg's startup. I also should notice that
> > currently Tegra DRM doesn't have a working HW cursor, I'm going to send out
> > Tegra cursor patches sometime soon.

Please don't in-line reply fixup patches, CI doesn't pick them up but
instead wants to add it to the patch series. Which won't apply because
most of it landed already. Please resend standalone.

> Oops.. I messed up there.. for_each_new_plane_in_state overwrites the state internally..
> ----->8-----
> for_each_oldnew_plane_in_state overwrites the iterator values internally, so we cannot rely
> on it being set to the last valid plane. This was causing a NULL pointer deref when someone
> tries to use the code. Save the plane and use the accessor functions to pull out the relevant
> plane state.

Hm, that's rather tricky :-/ I guess the for_each_if() is the culprit for
this? Could we perhaps fix this somehow by pushing the assignments into
the for_each_if (again using the && trick)? That way these macros are more
robust.

> Cc: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Assuming CI also approves:

Reviewed-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>

> ---
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 32fb61169b4f..8573feaea8c0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1388,23 +1388,30 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  {
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
> -	struct drm_plane *plane;
> +	struct drm_plane *plane = NULL, *__plane;
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	const struct drm_plane_helper_funcs *funcs;
> -	int i, n_planes = 0;
> +	int i;
>  
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (drm_atomic_crtc_needs_modeset(crtc_state))
>  			return -EINVAL;
>  	}
>  
> -	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> -		n_planes++;
> +	for_each_new_plane_in_state(state, __plane, new_plane_state, i) {
> +		/* FIXME: we support only single plane updates for now */
> +		if (plane)
> +			return -EINVAL;
> +
> +		plane = __plane;
> +	}
>  
> -	/* FIXME: we support only single plane updates for now */
> -	if (n_planes != 1)
> +	if (!plane)
>  		return -EINVAL;
>  
> +	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +
>  	if (!new_plane_state->crtc)
>  		return -EINVAL;
>  
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)
URL   : https://patchwork.freedesktop.org/series/29538/
State : success

== Summary ==

Series 29538v2 drm/atomic: Fix use-after-free with unbound connectors/planes.
https://patchwork.freedesktop.org/api/1.0/series/29538/revisions/2/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-x1585l) fdo#101781

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

fi-bdw-5557u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-bdw-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:439s
fi-blb-e6850     total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  time:361s
fi-bsw-n3050     total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  time:554s
fi-bwr-2160      total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 time:255s
fi-bxt-j4205     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:519s
fi-byt-j1900     total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  time:523s
fi-byt-n2820     total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  time:518s
fi-cfl-s         total:285  pass:250  dwarn:1   dfail:0   fail:0   skip:33 
fi-elk-e7500     total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  time:438s
fi-glk-2a        total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:612s
fi-hsw-4770      total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:444s
fi-hsw-4770r     total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:423s
fi-ilk-650       total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:427s
fi-ivb-3520m     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:507s
fi-ivb-3770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:472s
fi-kbl-7500u     total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  time:520s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:596s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:598s
fi-pnv-d510      total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:528s
fi-skl-6260u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:472s
fi-skl-6700k     total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:534s
fi-skl-6770hq    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:518s
fi-skl-gvtdvm    total:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  time:444s
fi-skl-x1585l    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:483s
fi-snb-2520m     total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  time:548s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  time:403s

a53eac614143e349be9ca1ab6c4ac799f8e1f0f9 drm-tip: 2017y-09m-04d-09h-11m-21s UTC integration manifest
a5bb7aa90443 drm/atomic: Make async plane update checks work as intended, v2.
01321b66e3ad drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.
0e520aabc9b9 drm/atomic: Return commit in drm_crtc_commit_get for better annotation
eea1321f34ec drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.
0c1f91d8a2ba drm/atomic: Move drm_crtc_commit to drm_crtc_state, v3.
f65cba7fa70a drm/i915: Always wait for flip_done, v2.

== Logs ==

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

✗ Fi.CI.IGT: failure for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev2)
URL   : https://patchwork.freedesktop.org/series/29538/
State : failure

== Summary ==

Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912
Test kms_cursor_legacy:
        Subgroup long-nonblocking-modeset-vs-cursor-atomic:
                pass       -> INCOMPLETE (shard-hsw)
Test perf:
        Subgroup blocking:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-offscren-pri-shrfb-draw-render:
                skip       -> PASS       (shard-hsw)
        Subgroup fbc-1p-offscren-pri-indfb-draw-mmap-wc:
                skip       -> PASS       (shard-hsw)
        Subgroup fbc-rgb565-draw-mmap-gtt:
                pass       -> FAIL       (shard-hsw)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-C-frame-sequence:
                pass       -> FAIL       (shard-hsw)
Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-C:
                pass       -> INCOMPLETE (shard-hsw)
Test kms_atomic_transition:
        Subgroup plane-use-after-nonblocking-unbind-fencing:
                fail       -> PASS       (shard-hsw) fdo#101847 +1

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#101847 https://bugs.freedesktop.org/show_bug.cgi?id=101847

shard-hsw        total:2118 pass:1153 dwarn:0   dfail:0   fail:17  skip:946 time:8895s

== Logs ==

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

✓ Fi.CI.BAT: success for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)
URL   : https://patchwork.freedesktop.org/series/29538/
State : success

== Summary ==

Series 29538v3 drm/atomic: Fix use-after-free with unbound connectors/planes.
https://patchwork.freedesktop.org/api/1.0/series/29538/revisions/3/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                skip       -> PASS       (fi-skl-x1585l) fdo#101781
Test drv_module_reload:
        Subgroup basic-reload:
                dmesg-warn -> INCOMPLETE (fi-cfl-s) k.org#196765

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781
k.org#196765 https://bugzilla.kernel.org/show_bug.cgi?id=196765

fi-bdw-5557u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:458s
fi-bdw-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:444s
fi-blb-e6850     total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  time:358s
fi-bsw-n3050     total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  time:563s
fi-bwr-2160      total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 time:253s
fi-bxt-j4205     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:517s
fi-byt-j1900     total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  time:526s
fi-byt-n2820     total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  time:514s
fi-cfl-s         total:285  pass:250  dwarn:1   dfail:0   fail:0   skip:33 
fi-elk-e7500     total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  time:440s
fi-glk-2a        total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:612s
fi-hsw-4770      total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:444s
fi-hsw-4770r     total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:426s
fi-ilk-650       total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:424s
fi-ivb-3520m     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-ivb-3770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:482s
fi-kbl-7500u     total:288  pass:264  dwarn:1   dfail:0   fail:0   skip:23  time:510s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:595s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:596s
fi-pnv-d510      total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:537s
fi-skl-6260u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:473s
fi-skl-6700k     total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:538s
fi-skl-6770hq    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:513s
fi-skl-gvtdvm    total:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  time:446s
fi-skl-x1585l    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-snb-2520m     total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  time:551s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  time:406s

59fbfaa48e1837b7a692df34ce6696eb4035e7a8 drm-tip: 2017y-09m-04d-11h-39m-37s UTC integration manifest
b307bc7d0cf6 drm/atomic: Make async plane update checks work as intended, v2.
e23ef2074a02 drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.
08156193021b drm/atomic: Return commit in drm_crtc_commit_get for better annotation
5d5cf3601e90 drm/atomic: Remove waits in drm_atomic_helper_commit_cleanup_done, v2.
e283a7f56851 drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.
cdfa52e3441f drm/i915: Always wait for flip_done, v2.

== Logs ==

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

✗ Fi.CI.IGT: failure for drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix use-after-free with unbound connectors/planes. (rev3)
URL   : https://patchwork.freedesktop.org/series/29538/
State : failure

== Summary ==

Test kms_atomic_transition:
        Subgroup plane-use-after-nonblocking-unbind-fencing:
                fail       -> PASS       (shard-hsw) fdo#101847 +1
Test gem_sync:
        Subgroup basic-many-each:
                pass       -> FAIL       (shard-hsw)
Test perf:
        Subgroup blocking:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (shard-hsw)

fdo#101847 https://bugs.freedesktop.org/show_bug.cgi?id=101847
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2265 pass:1232 dwarn:2   dfail:0   fail:15  skip:1016 time:9556s

== Logs ==

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