[PATCH 0/2] drm/i915: Fix wait_req unreferencing.

[PATCH 0/2] drm/i915: Fix wait_req unreferencing.

[Maarten Lankhorst]

Small cleanup plus a fix for the reference dropping.
This removes a source of heisenbugs. :-)

Maarten Lankhorst (2):
  drm/i915: Remove early return in prepare/cleanup plane
  drm/i915: Drop reference to current state wait req as soon as it goes
    unused

 drivers/gpu/drm/i915/intel_display.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

-- 
2.7.4

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

[PATCH 1/2] drm/i915: Remove early return in prepare/cleanup plane

[Maarten Lankhorst]

This is a noop in prepare_plane, which already carefully handles this
with if guards and early return.

cleanup_plane_fb has it as a noop too, it only unpins old_fb and
unsets wait_req. The latter is a noop because prepare_plane won't
set wait_req when obj == NULL. It's also dangerous to keep because
plane->state may change before cleanup_plane_fb is called.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b12c8302580f..f56707289615 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14117,9 +14117,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	struct reservation_object *resv;
 	int ret = 0;
 
-	if (!obj && !old_obj)
-		return 0;
-
 	if (old_obj) {
 		struct drm_crtc_state *crtc_state =
 			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
@@ -14193,15 +14190,10 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 		       const struct drm_plane_state *old_state)
 {
 	struct drm_device *dev = plane->dev;
-	struct intel_plane_state *old_intel_state;
 	struct intel_plane_state *intel_state = to_intel_plane_state(plane->state);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
-	struct drm_i915_gem_object *obj = intel_fb_obj(plane->state->fb);
-
-	old_intel_state = to_intel_plane_state(old_state);
-
-	if (!obj && !old_obj)
-		return;
+	struct intel_plane_state *old_intel_state =
+		to_intel_plane_state(old_state);
 
 	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
 	    !INTEL_INFO(dev)->cursor_needs_physical))
-- 
2.7.4

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

[PATCH 2/2] drm/i915: Drop reference to current state wait req as soon as it goes unused

[Maarten Lankhorst]

There are two paths into intel_cleanup_plane_fb, the normal completion
path and the failure path.

In the failure case, intel_cleanup_plane_fb is called before
drm_atomic_helper_swap_state, so any wait_req reference made in
intel_prepare_plane_fb will be in old_intel_state->wait_req.

In the normal completion path, wait_req is not freed until
the next commit, which is no longer used after waiting.

Free it as soon as possible, so we don't hold on to it indefinitely.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Keith Packard <keithp@keithp.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to current state wait_req")
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f56707289615..e72ad97998a4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13754,6 +13754,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		/* EIO should be eaten, and we can't get interrupted in the
 		 * worker, and blocking commits have waited already. */
 		WARN_ON(ret);
+
+		i915_gem_request_assign(&intel_plane_state->wait_req, NULL);
 	}
 
 	if (!state->legacy_cursor_update)
@@ -14190,7 +14192,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 		       const struct drm_plane_state *old_state)
 {
 	struct drm_device *dev = plane->dev;
-	struct intel_plane_state *intel_state = to_intel_plane_state(plane->state);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
 	struct intel_plane_state *old_intel_state =
 		to_intel_plane_state(old_state);
@@ -14199,7 +14200,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	    !INTEL_INFO(dev)->cursor_needs_physical))
 		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
 
-	i915_gem_request_assign(&intel_state->wait_req, NULL);
 	i915_gem_request_assign(&old_intel_state->wait_req, NULL);
 }
 
-- 
2.7.4

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

✗ Ro.CI.BAT: failure for drm/i915: Fix wait_req unreferencing.

[Patchwork]

== Series Details ==

Series: drm/i915: Fix wait_req unreferencing.
URL   : https://patchwork.freedesktop.org/series/10781/
State : failure

== Summary ==

Applying: drm/i915: Remove early return in prepare/cleanup plane
Applying: drm/i915: Drop reference to current state wait req as soon as it goes unused
fatal: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_display.c).
error: could not build fake ancestor
Patch failed at 0002 drm/i915: Drop reference to current state wait req as soon as it goes unused
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH 2/2] drm/i915: Drop reference to current state wait req as soon as it goes unused

[Daniel Vetter]

On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> There are two paths into intel_cleanup_plane_fb, the normal completion
> path and the failure path.
>
> In the failure case, intel_cleanup_plane_fb is called before
> drm_atomic_helper_swap_state, so any wait_req reference made in
> intel_prepare_plane_fb will be in old_intel_state->wait_req.
>
> In the normal completion path, wait_req is not freed until
> the next commit, which is no longer used after waiting.
>
> Free it as soon as possible, so we don't hold on to it indefinitely.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to current state wait_req")

We still need to clean up the reference in case of failure, which
means latest in intel_plane_destroy_state(). Also hanging onto a
request isn't that evil really, why can't we just only clean up in the
destroy function?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f56707289615..e72ad97998a4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13754,6 +13754,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>                 /* EIO should be eaten, and we can't get interrupted in the
>                  * worker, and blocking commits have waited already. */
>                 WARN_ON(ret);
> +
> +               i915_gem_request_assign(&intel_plane_state->wait_req, NULL);
>         }
>
>         if (!state->legacy_cursor_update)
> @@ -14190,7 +14192,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>                        const struct drm_plane_state *old_state)
>  {
>         struct drm_device *dev = plane->dev;
> -       struct intel_plane_state *intel_state = to_intel_plane_state(plane->state);
>         struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
>         struct intel_plane_state *old_intel_state =
>                 to_intel_plane_state(old_state);
> @@ -14199,7 +14200,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>             !INTEL_INFO(dev)->cursor_needs_physical))
>                 intel_unpin_fb_obj(old_state->fb, old_state->rotation);
>
> -       i915_gem_request_assign(&intel_state->wait_req, NULL);
>         i915_gem_request_assign(&old_intel_state->wait_req, NULL);
>  }
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 2/2] drm/i915: Drop reference to current state wait req as soon as it goes unused

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 660 bytes --]

Daniel Vetter <daniel@ffwll.ch> writes:

> We still need to clean up the reference in case of failure, which
> means latest in intel_plane_destroy_state(). Also hanging onto a
> request isn't that evil really, why can't we just only clean up in the
> destroy function?

Sticking a cleanup there as well is good insurance, but it seems like a
reasonable policy to drop references when values go 'out of scope' as
they do in cleanup_plane_fb. But, you're right, we only *need* to drop
the reference in the destroy function, it's just that the state hangs
around until the next mode setting operation, which is likely to be days
away.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 810 bytes --]

[-- Attachment #2: 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 2/2] drm/i915: Drop reference to current state wait req as soon as it goes unused

[Maarten Lankhorst]

Op 08-08-16 om 17:34 schreef Daniel Vetter:
> On Mon, Aug 8, 2016 at 1:23 PM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> There are two paths into intel_cleanup_plane_fb, the normal completion
>> path and the failure path.
>>
>> In the failure case, intel_cleanup_plane_fb is called before
>> drm_atomic_helper_swap_state, so any wait_req reference made in
>> intel_prepare_plane_fb will be in old_intel_state->wait_req.
>>
>> In the normal completion path, wait_req is not freed until
>> the next commit, which is no longer used after waiting.
>>
>> Free it as soon as possible, so we don't hold on to it indefinitely.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Keith Packard <keithp@keithp.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: intel-gfx@lists.freedesktop.org
>> Cc: dri-devel@lists.freedesktop.org
>> Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to current state wait_req")
> We still need to clean up the reference in case of failure, which
> means latest in intel_plane_destroy_state(). Also hanging onto a
> request isn't that evil really, why can't we just only clean up in the
> destroy function?
Hm true, we're still guaranteed to call cleanup_plane_fb in case of failure though, else the WARN in unref would trigger.

I think it's harmless to hang onto the request, worst case we keep an extra MAX_PLANES * MAX_CRTCS * sizeof(i915_gem_request) allocated,
which is slightly more than 4k memory. (4 * 3 * 352)

If we unref the request in the destructor, it's also one step closer to constifying plane_state.

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

[PATCH 2/2] drm/i915: Drop reference to plane state wait req in destructor.

[Maarten Lankhorst]

With gem requests reworked to only keep some memory around after
it's completed it's now mostly harmless to keep a reference to the
request until the state is destroyed. This makes it easy to hang
on to the request until the plane state is destroyed since it is
just a bunch of memory now.

On my 64-bits system a i915_gem_request is 352 bytes. With all
3 crtc's, each 4 planes enabled the total is 4224 bytes,
low enough not to optimize this case too much.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Keith Packard <keithp@keithp.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to current state wait_req")
---
 drivers/gpu/drm/i915/intel_atomic_plane.c | 5 ++++-
 drivers/gpu/drm/i915/intel_display.c      | 6 ------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 7de7721f65bc..f98071a1dcc0 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -101,7 +101,10 @@ void
 intel_plane_destroy_state(struct drm_plane *plane,
 			  struct drm_plane_state *state)
 {
-	WARN_ON(state && to_intel_plane_state(state)->wait_req);
+	struct intel_plane_state *intel_state = to_intel_plane_state(state);
+
+	i915_gem_request_assign(&intel_state->wait_req, NULL);
+
 	drm_atomic_helper_plane_destroy_state(plane, state);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af45ee206239..774895288bcc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14063,17 +14063,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 		       const struct drm_plane_state *old_state)
 {
 	struct drm_device *dev = plane->dev;
-	struct intel_plane_state *intel_state = to_intel_plane_state(plane->state);
 	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
-	struct intel_plane_state *old_intel_state =
-		to_intel_plane_state(old_state);
 
 	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
 	    !INTEL_INFO(dev)->cursor_needs_physical))
 		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
-
-	i915_gem_request_assign(&intel_state->wait_req, NULL);
-	i915_gem_request_assign(&old_intel_state->wait_req, NULL);
 }
 
 int
-- 
2.7.4


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

Re: [PATCH 2/2] drm/i915: Drop reference to plane state wait req in destructor.

[Daniel Vetter]

On Tue, Aug 09, 2016 at 12:15:51PM +0200, Maarten Lankhorst wrote:
> With gem requests reworked to only keep some memory around after
> it's completed it's now mostly harmless to keep a reference to the
> request until the state is destroyed. This makes it easy to hang
> on to the request until the plane state is destroyed since it is
> just a bunch of memory now.
> 
> On my 64-bits system a i915_gem_request is 352 bytes. With all
> 3 crtc's, each 4 planes enabled the total is 4224 bytes,
> low enough not to optimize this case too much.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Keith Packard <keithp@keithp.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to current state wait_req")

Yeah, I think hanging onto requests like that is perfectly fine, and it
simplifies the gymnastics for tracking things if we always allocate
drm_*_state resources in atomic_duplicate, update the refs in atomic_check
(with the various set functions the core has, or our own) and then release
references in atomic_state destroy.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_atomic_plane.c | 5 ++++-
>  drivers/gpu/drm/i915/intel_display.c      | 6 ------
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 7de7721f65bc..f98071a1dcc0 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -101,7 +101,10 @@ void
>  intel_plane_destroy_state(struct drm_plane *plane,
>  			  struct drm_plane_state *state)
>  {
> -	WARN_ON(state && to_intel_plane_state(state)->wait_req);
> +	struct intel_plane_state *intel_state = to_intel_plane_state(state);
> +
> +	i915_gem_request_assign(&intel_state->wait_req, NULL);
> +
>  	drm_atomic_helper_plane_destroy_state(plane, state);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af45ee206239..774895288bcc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14063,17 +14063,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  		       const struct drm_plane_state *old_state)
>  {
>  	struct drm_device *dev = plane->dev;
> -	struct intel_plane_state *intel_state = to_intel_plane_state(plane->state);
>  	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb);
> -	struct intel_plane_state *old_intel_state =
> -		to_intel_plane_state(old_state);
>  
>  	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
>  	    !INTEL_INFO(dev)->cursor_needs_physical))
>  		intel_unpin_fb_obj(old_state->fb, old_state->rotation);
> -
> -	i915_gem_request_assign(&intel_state->wait_req, NULL);
> -	i915_gem_request_assign(&old_intel_state->wait_req, NULL);
>  }
>  
>  int
> -- 
> 2.7.4
> 
> 

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