[PATCH 1/2] drm/i915: Stop retiring requests from busy-ioctl

[PATCH 1/2] drm/i915: Stop retiring requests from busy-ioctl

[Chris Wilson]

In order to reduce the workload of the caller, we do not want to
actually have to retire requests of others when checking the status of
this object.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8439867..474c027 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3069,14 +3069,8 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
 		if (req == NULL)
 			continue;
 
-		if (list_empty(&req->list))
-			goto retire;
-
-		if (i915_gem_request_completed(req, true)) {
-			__i915_gem_request_retire__upto(req);
-retire:
+		if (i915_gem_request_completed(req, true))
 			i915_gem_object_retire__read(obj, i);
-		}
 	}
 
 	return 0;
-- 
2.8.1

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

[PATCH 2/2] drm/i915: Stop automatically retiring requests after a GPU hang

[Chris Wilson]

Following a GPU hang, we break out of the request loop in order to
unlock the struct_mutex for use by the GPU reset. However, if we retire
all the requests at that moment, we cannot identify the guilty request
after performing the reset.

Fixes: f4457ae71fd6 ("drm/i915: Prevent leaking of -EIO from i915_wait_request()")
Testcase: igt/gem_reset_stats
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 474c027..b1a33fe9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1462,7 +1462,10 @@ i915_wait_request(struct drm_i915_gem_request *req)
 	if (ret)
 		return ret;
 
-	__i915_gem_request_retire__upto(req);
+	/* If the GPU hung, we want to keep the requests to find the guilty. */
+	if (req->reset_counter == i915_reset_counter(&dev_priv->gpu_error))
+		__i915_gem_request_retire__upto(req);
+
 	return 0;
 }
 
@@ -1519,7 +1522,8 @@ i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
 	else if (obj->last_write_req == req)
 		i915_gem_object_retire__write(obj);
 
-	__i915_gem_request_retire__upto(req);
+	if (req->reset_counter == i915_reset_counter(&req->i915->gpu_error))
+		__i915_gem_request_retire__upto(req);
 }
 
 /* A nonblocking variant of the above wait. This is a highly dangerous routine
-- 
2.8.1

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

[PATCH v2] drm/i915: Stop automatically retiring requests after a GPU hang

[Chris Wilson]

Following a GPU hang, we break out of the request loop in order to
unlock the struct_mutex for use by the GPU reset. However, if we retire
all the requests at that moment, we cannot identify the guilty request
after performing the reset.

v2: Not automatically retiring requests forces us to recheck for
available ringspace.

Fixes: f4457ae71fd6 ("drm/i915: Prevent leaking of -EIO from i915_wait_request()")
Testcase: igt/gem_reset_stats
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c         | 8 ++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6e61738fab31..a3d826bb216b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1462,7 +1462,10 @@ i915_wait_request(struct drm_i915_gem_request *req)
 	if (ret)
 		return ret;
 
-	__i915_gem_request_retire__upto(req);
+	/* If the GPU hung, we want to keep the requests to find the guilty. */
+	if (req->reset_counter == i915_reset_counter(&dev_priv->gpu_error))
+		__i915_gem_request_retire__upto(req);
+
 	return 0;
 }
 
@@ -1519,7 +1522,8 @@ i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
 	else if (obj->last_write_req == req)
 		i915_gem_object_retire__write(obj);
 
-	__i915_gem_request_retire__upto(req);
+	if (req->reset_counter == i915_reset_counter(&req->i915->gpu_error))
+		__i915_gem_request_retire__upto(req);
 }
 
 /* A nonblocking variant of the above wait. This is a highly dangerous routine
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0618dd34c3ec..8d35a3978f9b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2450,6 +2450,8 @@ int intel_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
 			return ret;
 
 		intel_ring_update_space(ringbuf);
+		if (unlikely(ringbuf->space < wait_bytes))
+			return -EAGAIN;
 	}
 
 	if (unlikely(need_wrap)) {
-- 
2.8.1

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

✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Stop retiring requests from busy-ioctl (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Stop retiring requests from busy-ioctl (rev2)
URL   : https://patchwork.freedesktop.org/series/7059/
State : failure

== Summary ==

Series 7059v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/7059/revisions/2/mbox

Test drv_hangman:
        Subgroup error-state-basic:
                fail       -> PASS       (ro-ilk1-i5-650)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> FAIL       (fi-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-hsw-i7-4770r)
                pass       -> FAIL       (fi-hsw-i7-4770k)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-c:
                skip       -> PASS       (fi-skl-i5-6260u)

fi-bdw-i7-5557u  total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
fi-bsw-n3050     total:218  pass:174  dwarn:0   dfail:0   fail:2   skip:42 
fi-byt-n2820     total:218  pass:174  dwarn:0   dfail:0   fail:3   skip:41 
fi-hsw-i7-4770k  total:219  pass:197  dwarn:0   dfail:0   fail:1   skip:21 
fi-hsw-i7-4770r  total:219  pass:193  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-i5-6260u  total:219  pass:207  dwarn:0   dfail:0   fail:0   skip:12 
fi-skl-i7-6700k  total:219  pass:191  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-i7-2600   total:37   pass:27   dwarn:0   dfail:0   fail:0   skip:9  
ro-bdw-i5-5250u  total:219  pass:181  dwarn:0   dfail:0   fail:0   skip:38 
ro-bdw-i7-5557U  total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
ro-bdw-i7-5600u  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
ro-bsw-n3050     total:219  pass:175  dwarn:0   dfail:0   fail:2   skip:42 
ro-byt-n2820     total:218  pass:175  dwarn:0   dfail:0   fail:2   skip:41 
ro-hsw-i3-4010u  total:218  pass:193  dwarn:0   dfail:0   fail:0   skip:25 
ro-hsw-i7-4770r  total:219  pass:194  dwarn:0   dfail:0   fail:0   skip:25 
ro-ilk-i7-620lm  total:219  pass:151  dwarn:0   dfail:0   fail:1   skip:67 
ro-ilk1-i5-650   total:214  pass:152  dwarn:0   dfail:0   fail:1   skip:61 
ro-ivb-i7-3770   total:219  pass:183  dwarn:0   dfail:0   fail:0   skip:36 
ro-ivb2-i7-3770  total:219  pass:186  dwarn:0   dfail:0   fail:1   skip:32 
ro-skl-i7-6700hq total:214  pass:190  dwarn:0   dfail:0   fail:0   skip:24 
ro-snb-i7-2620M  total:219  pass:177  dwarn:0   dfail:0   fail:1   skip:41 

Results at /archive/results/CI_IGT_test/RO_Patchwork_879/

a6ac4ab drm-intel-nightly: 2016y-05m-12d-15h-37m-38s UTC integration manifest
f6b2891 drm/i915: Stop automatically retiring requests after a GPU hang
41fa6f9 drm/i915: Stop retiring requests from busy-ioctl

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

Re: [PATCH 1/2] drm/i915: Stop retiring requests from busy-ioctl

[Tvrtko Ursulin]

On 12/05/16 11:25, Chris Wilson wrote:
> In order to reduce the workload of the caller, we do not want to
> actually have to retire requests of others when checking the status of
> this object.

Wrt the subject, and from wait ioctl as well.

Also i915_gem_object_sync / i915_gem_execbuffer_move_to_gpu path 
(execbuf) looks like it could still do the upto retiring so that isn't 
only moving the thing around?

And in general, commit does not say who was impacted by this and how much?

Regards,

Tvrtko

>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8439867..474c027 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3069,14 +3069,8 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
>   		if (req == NULL)
>   			continue;
>
> -		if (list_empty(&req->list))
> -			goto retire;
> -
> -		if (i915_gem_request_completed(req, true)) {
> -			__i915_gem_request_retire__upto(req);
> -retire:
> +		if (i915_gem_request_completed(req, true))
>   			i915_gem_object_retire__read(obj, i);
> -		}
>   	}
>
>   	return 0;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Stop automatically retiring requests after a GPU hang

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> [ text/plain ]
> Following a GPU hang, we break out of the request loop in order to
> unlock the struct_mutex for use by the GPU reset. However, if we retire
> all the requests at that moment, we cannot identify the guilty request
> after performing the reset.
>
> v2: Not automatically retiring requests forces us to recheck for
> available ringspace.
>
> Fixes: f4457ae71fd6 ("drm/i915: Prevent leaking of -EIO from
> i915_wait_request()")

Partially. We don't clear the request list before reset
processing. But we still do release clients before reset
processing. This leads to clients see completed
requests before the reset statistics have been updated.

If we return -EAGAIN in wait_request, then we unblock
the clients only until the reset handling has updated
the seqno past the reset boundary. This would fix
both issues I am seeing.

-Mika

> Testcase: igt/gem_reset_stats
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 8 ++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6e61738fab31..a3d826bb216b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1462,7 +1462,10 @@ i915_wait_request(struct drm_i915_gem_request *req)
>  	if (ret)
>  		return ret;
>  
> -	__i915_gem_request_retire__upto(req);
> +	/* If the GPU hung, we want to keep the requests to find the guilty. */
> +	if (req->reset_counter == i915_reset_counter(&dev_priv->gpu_error))
> +		__i915_gem_request_retire__upto(req);
> +
>  	return 0;
>  }
>  
> @@ -1519,7 +1522,8 @@ i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
>  	else if (obj->last_write_req == req)
>  		i915_gem_object_retire__write(obj);
>  
> -	__i915_gem_request_retire__upto(req);
> +	if (req->reset_counter == i915_reset_counter(&req->i915->gpu_error))
> +		__i915_gem_request_retire__upto(req);
>  }
>  
>  /* A nonblocking variant of the above wait. This is a highly dangerous routine
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 0618dd34c3ec..8d35a3978f9b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2450,6 +2450,8 @@ int intel_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
>  			return ret;
>  
>  		intel_ring_update_space(ringbuf);
> +		if (unlikely(ringbuf->space < wait_bytes))
> +			return -EAGAIN;
>  	}
>  
>  	if (unlikely(need_wrap)) {
> -- 
> 2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Stop retiring requests from busy-ioctl

[Chris Wilson]

On Fri, May 13, 2016 at 09:38:54AM +0100, Tvrtko Ursulin wrote:
> 
> On 12/05/16 11:25, Chris Wilson wrote:
> >In order to reduce the workload of the caller, we do not want to
> >actually have to retire requests of others when checking the status of
> >this object.
> 
> Wrt the subject, and from wait ioctl as well.
> 
> Also i915_gem_object_sync / i915_gem_execbuffer_move_to_gpu path
> (execbuf) looks like it could still do the upto retiring so that
> isn't only moving the thing around?
> 
> And in general, commit does not say who was impacted by this and how much?

Nothing much until we have them all removed. Please see the other
patches! The end result in having a completely lockless busy-ioctl is a
major improvement to a minor path... The impact is really only measured
by reduced contention between clients, for which the busy-ioctl is
mostly a victim.

This was just an opportunistic change to reduce the numer of
retire__upto when thinking about the bugfix in patch 2.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Stop retiring requests from busy-ioctl

[Tvrtko Ursulin]

On 13/05/16 09:50, Chris Wilson wrote:
> On Fri, May 13, 2016 at 09:38:54AM +0100, Tvrtko Ursulin wrote:
>>
>> On 12/05/16 11:25, Chris Wilson wrote:
>>> In order to reduce the workload of the caller, we do not want to
>>> actually have to retire requests of others when checking the status of
>>> this object.
>>
>> Wrt the subject, and from wait ioctl as well.
>>
>> Also i915_gem_object_sync / i915_gem_execbuffer_move_to_gpu path
>> (execbuf) looks like it could still do the upto retiring so that
>> isn't only moving the thing around?
>>
>> And in general, commit does not say who was impacted by this and how much?
>
> Nothing much until we have them all removed. Please see the other
> patches! The end result in having a completely lockless busy-ioctl is a
> major improvement to a minor path... The impact is really only measured
> by reduced contention between clients, for which the busy-ioctl is
> mostly a victim.
>
> This was just an opportunistic change to reduce the numer of
> retire__upto when thinking about the bugfix in patch 2.

I don't mind the idea, just though it needs more explaining in the
commit and updated subject.

I don't know the reset path nearly well enough to comment on 2/2.

Regards,

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

Re: [PATCH v2] drm/i915: Stop automatically retiring requests after a GPU hang

[Chris Wilson]

On Fri, May 13, 2016 at 11:43:25AM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > [ text/plain ]
> > Following a GPU hang, we break out of the request loop in order to
> > unlock the struct_mutex for use by the GPU reset. However, if we retire
> > all the requests at that moment, we cannot identify the guilty request
> > after performing the reset.
> >
> > v2: Not automatically retiring requests forces us to recheck for
> > available ringspace.
> >
> > Fixes: f4457ae71fd6 ("drm/i915: Prevent leaking of -EIO from
> > i915_wait_request()")
> 
> Partially. We don't clear the request list before reset
> processing. But we still do release clients before reset
> processing. This leads to clients see completed
> requests before the reset statistics have been updated.
> 
> If we return -EAGAIN in wait_request, then we unblock
> the clients only until the reset handling has updated
> the seqno past the reset boundary. This would fix
> both issues I am seeing.

I am very wary about reintroducing -EAGAIN here, since the majority of
callers do not care - or worse do not handle such notifications and
would be confused by it (the request they waited upon is no more and
they are ready to continue after all).

Anyway, the bug looks to be on the other side here as the
get-reset-stats ioctl doesn't wait for a pending reset before reporting
victim/guilty counts.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: Move get-reset-stats ioctl from intel_uncore.c to i915_gem_context.c

[Chris Wilson]

The get-reset-stats ioctl reports upon the statistics (number of hangs,
be it as a victim or the guilty party) of a particular context. It is
semantically better as being part of i915_gem_context.c user interface,
as opposed to the hardware level access of intel_uncore.c

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c         |  2 +-
 drivers/gpu/drm/i915/i915_drv.h         |  4 ++--
 drivers/gpu/drm/i915/i915_gem_context.c | 39 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c     | 39 ---------------------------------
 4 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e42fa1769c10..05b2b1901d60 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1631,7 +1631,7 @@ const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_RENDER_ALLOW),
-	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_gem_context_reset_stats_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a0b51301337..d9d07b70f05c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3362,6 +3362,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv);
 int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *file_priv);
+int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data,
+				       struct drm_file *file);
 
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct drm_device *dev,
@@ -3578,8 +3580,6 @@ extern void intel_detect_pch(struct drm_device *dev);
 extern bool i915_semaphore_is_enabled(struct drm_i915_private *dev_priv);
 int i915_reg_read_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file);
-int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
-			       struct drm_file *file);
 
 /* overlay */
 extern struct intel_overlay_error_state *
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 0fffebcc0ace..f3c76ca6b411 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -1040,3 +1040,42 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 
 	return ret;
 }
+
+int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
+				       void *data, struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_reset_stats *args = data;
+	struct i915_ctx_hang_stats *hs;
+	struct intel_context *ctx;
+	int ret;
+
+	if (args->flags || args->pad)
+		return -EINVAL;
+
+	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
+	if (IS_ERR(ctx)) {
+		mutex_unlock(&dev->struct_mutex);
+		return PTR_ERR(ctx);
+	}
+	hs = &ctx->hang_stats;
+
+	if (capable(CAP_SYS_ADMIN))
+		args->reset_count = i915_reset_count(&dev_priv->gpu_error);
+	else
+		args->reset_count = 0;
+
+	args->batch_active = hs->batch_active;
+	args->batch_pending = hs->batch_pending;
+
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 0b1bd07dd04a..385114bca924 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1460,45 +1460,6 @@ out:
 	return ret;
 }
 
-int i915_get_reset_stats_ioctl(struct drm_device *dev,
-			       void *data, struct drm_file *file)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_reset_stats *args = data;
-	struct i915_ctx_hang_stats *hs;
-	struct intel_context *ctx;
-	int ret;
-
-	if (args->flags || args->pad)
-		return -EINVAL;
-
-	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
-
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
-	if (ret)
-		return ret;
-
-	ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
-	if (IS_ERR(ctx)) {
-		mutex_unlock(&dev->struct_mutex);
-		return PTR_ERR(ctx);
-	}
-	hs = &ctx->hang_stats;
-
-	if (capable(CAP_SYS_ADMIN))
-		args->reset_count = i915_reset_count(&dev_priv->gpu_error);
-	else
-		args->reset_count = 0;
-
-	args->batch_active = hs->batch_active;
-	args->batch_pending = hs->batch_pending;
-
-	mutex_unlock(&dev->struct_mutex);
-
-	return 0;
-}
-
 static int i915_reset_complete(struct pci_dev *pdev)
 {
 	u8 gdrst;
-- 
2.8.1

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

[PATCH 2/2] drm/i915: Complete pending resets before get-reset-stats ioctl

[Chris Wilson]

The get-reset-stats ioctls wasn't waiting for a pending reset before
reporting its statistics, and so was ignoring a hang generated by the
context that should have been reported against said context.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f3c76ca6b411..2aedd188473d 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -1056,7 +1056,7 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
 	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
 		return ret;
 
-- 
2.8.1

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

✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Stop retiring requests from busy-ioctl (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Stop retiring requests from busy-ioctl (rev4)
URL   : https://patchwork.freedesktop.org/series/7059/
State : failure

== Summary ==

Applying: drm/i915: Stop retiring requests from busy-ioctl
Applying: drm/i915: Complete pending resets before get-reset-stats ioctl
Patch failed at 0002 drm/i915: Complete pending resets before get-reset-stats ioctl
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 1/2] drm/i915: Move get-reset-stats ioctl from intel_uncore.c to i915_gem_context.c

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> [ text/plain ]
> The get-reset-stats ioctl reports upon the statistics (number of hangs,
> be it as a victim or the guilty party) of a particular context. It is
> semantically better as being part of i915_gem_context.c user interface,
> as opposed to the hardware level access of intel_uncore.c
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_dma.c         |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h         |  4 ++--
>  drivers/gpu/drm/i915/i915_gem_context.c | 39 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uncore.c     | 39 ---------------------------------
>  4 files changed, 42 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e42fa1769c10..05b2b1901d60 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1631,7 +1631,7 @@ const struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE, i915_gem_context_create_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY, i915_gem_context_destroy_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_REG_READ, i915_reg_read_ioctl, DRM_RENDER_ALLOW),
> -	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_get_reset_stats_ioctl, DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_GET_RESET_STATS, i915_gem_context_reset_stats_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a0b51301337..d9d07b70f05c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3362,6 +3362,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>  				    struct drm_file *file_priv);
>  int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>  				    struct drm_file *file_priv);
> +int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data,
> +				       struct drm_file *file);
>  
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct drm_device *dev,
> @@ -3578,8 +3580,6 @@ extern void intel_detect_pch(struct drm_device *dev);
>  extern bool i915_semaphore_is_enabled(struct drm_i915_private *dev_priv);
>  int i915_reg_read_ioctl(struct drm_device *dev, void *data,
>  			struct drm_file *file);
> -int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
> -			       struct drm_file *file);
>  
>  /* overlay */
>  extern struct intel_overlay_error_state *
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 0fffebcc0ace..f3c76ca6b411 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -1040,3 +1040,42 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>  
>  	return ret;
>  }
> +
> +int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
> +				       void *data, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_reset_stats *args = data;
> +	struct i915_ctx_hang_stats *hs;
> +	struct intel_context *ctx;
> +	int ret;
> +
> +	if (args->flags || args->pad)
> +		return -EINVAL;
> +
> +	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	if (ret)
> +		return ret;
> +
> +	ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
> +	if (IS_ERR(ctx)) {
> +		mutex_unlock(&dev->struct_mutex);
> +		return PTR_ERR(ctx);
> +	}
> +	hs = &ctx->hang_stats;
> +
> +	if (capable(CAP_SYS_ADMIN))
> +		args->reset_count = i915_reset_count(&dev_priv->gpu_error);
> +	else
> +		args->reset_count = 0;
> +
> +	args->batch_active = hs->batch_active;
> +	args->batch_pending = hs->batch_pending;
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 0b1bd07dd04a..385114bca924 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1460,45 +1460,6 @@ out:
>  	return ret;
>  }
>  
> -int i915_get_reset_stats_ioctl(struct drm_device *dev,
> -			       void *data, struct drm_file *file)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_i915_reset_stats *args = data;
> -	struct i915_ctx_hang_stats *hs;
> -	struct intel_context *ctx;
> -	int ret;
> -
> -	if (args->flags || args->pad)
> -		return -EINVAL;
> -
> -	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
> -		return -EPERM;
> -
> -	ret = mutex_lock_interruptible(&dev->struct_mutex);
> -	if (ret)
> -		return ret;
> -
> -	ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
> -	if (IS_ERR(ctx)) {
> -		mutex_unlock(&dev->struct_mutex);
> -		return PTR_ERR(ctx);
> -	}
> -	hs = &ctx->hang_stats;
> -
> -	if (capable(CAP_SYS_ADMIN))
> -		args->reset_count = i915_reset_count(&dev_priv->gpu_error);
> -	else
> -		args->reset_count = 0;
> -
> -	args->batch_active = hs->batch_active;
> -	args->batch_pending = hs->batch_pending;
> -
> -	mutex_unlock(&dev->struct_mutex);
> -
> -	return 0;
> -}
> -
>  static int i915_reset_complete(struct pci_dev *pdev)
>  {
>  	u8 gdrst;
> -- 
> 2.8.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Complete pending resets before get-reset-stats ioctl

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> [ text/plain ]
> The get-reset-stats ioctls wasn't waiting for a pending reset before
> reporting its statistics, and so was ignoring a hang generated by the
> context that should have been reported against said context.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

This is the fix we are looking for. And so blatantly obvious that
I blame myself for walking past this yesterday.

Testcase: igt/gem_reset_stats/reset_stats-*
Tested-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index f3c76ca6b411..2aedd188473d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -1056,7 +1056,7 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
>  	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
> -	ret = mutex_lock_interruptible(&dev->struct_mutex);
> +	ret = i915_mutex_lock_interruptible(dev);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.8.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Stop retiring requests from busy/wait ioctls

[Chris Wilson]

In order to reduce the workload of the caller, we do not want to
actually have to retire requests of others when checking the busy status
of this object. This applies to both busy/wait ioctls as the wait ioctl
has a semantically equivalent mode to the busy ioctl.

At the present time, this is only a minor improvement to reduce the
workload of the busy ioctl under the struct_mutex which helps to reduce
its impact upon contention of struct_mutex. However, since it is mostly
a victim in highly contended scenarios, the impact is very minor until
we can eliminate the struct_mutex requirement for busy-ioctl in the near
future.

v2: Mention the patches intended limited impact. It is just paving the
way for greater changes whilst reducing the impact of a bugfix in the
next patch.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index be75689455b2..6e61738fab31 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3066,14 +3066,8 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
 		if (req == NULL)
 			continue;
 
-		if (list_empty(&req->list))
-			goto retire;
-
-		if (i915_gem_request_completed(req, true)) {
-			__i915_gem_request_retire__upto(req);
-retire:
+		if (i915_gem_request_completed(req, true))
 			i915_gem_object_retire__read(obj, i);
-		}
 	}
 
 	return 0;
-- 
2.8.1

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

Re: [PATCH 1/2] drm/i915: Stop retiring requests from busy-ioctl

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> [ text/plain ]
> In order to reduce the workload of the caller, we do not want to
> actually have to retire requests of others when checking the status of
> this object.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8439867..474c027 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3069,14 +3069,8 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
>  		if (req == NULL)
>  			continue;
>  
> -		if (list_empty(&req->list))
> -			goto retire;
> -
> -		if (i915_gem_request_completed(req, true)) {
> -			__i915_gem_request_retire__upto(req);
> -retire:
> +		if (i915_gem_request_completed(req, true))
>  			i915_gem_object_retire__read(obj, i);
> -		}
>  	}
>  
>  	return 0;
> -- 
> 2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Stop automatically retiring requests after a GPU hang

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> [ text/plain ]
> Following a GPU hang, we break out of the request loop in order to
> unlock the struct_mutex for use by the GPU reset. However, if we retire
> all the requests at that moment, we cannot identify the guilty request
> after performing the reset.
>
> v2: Not automatically retiring requests forces us to recheck for
> available ringspace.
>
> Fixes: f4457ae71fd6 ("drm/i915: Prevent leaking of -EIO from i915_wait_request()")
> Testcase: igt/gem_reset_stats

Testcase: igt/gem_reset_stats/ban-*
Tested-by: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 8 ++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6e61738fab31..a3d826bb216b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1462,7 +1462,10 @@ i915_wait_request(struct drm_i915_gem_request *req)
>  	if (ret)
>  		return ret;
>  
> -	__i915_gem_request_retire__upto(req);
> +	/* If the GPU hung, we want to keep the requests to find the guilty. */
> +	if (req->reset_counter == i915_reset_counter(&dev_priv->gpu_error))
> +		__i915_gem_request_retire__upto(req);
> +
>  	return 0;
>  }
>  
> @@ -1519,7 +1522,8 @@ i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
>  	else if (obj->last_write_req == req)
>  		i915_gem_object_retire__write(obj);
>  
> -	__i915_gem_request_retire__upto(req);
> +	if (req->reset_counter == i915_reset_counter(&req->i915->gpu_error))
> +		__i915_gem_request_retire__upto(req);
>  }
>  
>  /* A nonblocking variant of the above wait. This is a highly dangerous routine
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 0618dd34c3ec..8d35a3978f9b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2450,6 +2450,8 @@ int intel_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
>  			return ret;
>  
>  		intel_ring_update_space(ringbuf);
> +		if (unlikely(ringbuf->space < wait_bytes))
> +			return -EAGAIN;
>  	}
>  
>  	if (unlikely(need_wrap)) {
> -- 
> 2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for series starting with [v2] drm/i915: Stop retiring requests from busy/wait ioctls (rev5)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Stop retiring requests from busy/wait ioctls (rev5)
URL   : https://patchwork.freedesktop.org/series/7059/
State : failure

== Summary ==

Applying: drm/i915: Stop retiring requests from busy/wait ioctls
Applying: drm/i915: Complete pending resets before get-reset-stats ioctl
Patch failed at 0002 drm/i915: Complete pending resets before get-reset-stats ioctl
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