Re: [PATCH v2 25/28] drm/i915: Interrupt driven request completion

Daniel, Thomas
2014-11-18T09:40:30Z

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of John.C.Harrison@Intel.com
> Sent: Friday, November 14, 2014 12:19 PM
> To: Intel-GFX@Lists.FreeDesktop.Org
> Subject: [Intel-gfx] [PATCH v2 25/28] drm/i915: Interrupt driven request
> completion
>
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Added a hook to the ring noftification code to process request completion.
> This means that there is no longer a need to explicitly process request
> completions every time a request object is tested. Instead, the test code
> simply becomes 'return req->completed'. Obviously, this only works if ring
> interrupts are enabled, however, this is already the case for the duration of
> __wait_request() which is the point where the driver really needs to know.
>
> To prevent stale requests floating around indefinitely, the retire work
> handler also now performs a completion check periodically.
>
> For: VIZ-4377
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 -----
> drivers/gpu/drm/i915/i915_gem.c | 10 ++++++++++
> drivers/gpu/drm/i915/i915_irq.c | 2 ++
> 3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 8531e0f..66219b5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2072,11 +2072,6 @@ static inline void i915_gem_request_assign(struct
> drm_i915_gem_request **pdst, static inline bool
> i915_gem_request_completed(struct drm_i915_gem_request *req,
> bool lazy_coherency)
> {
> - if (req->complete)
> - return true;
> -
> - i915_gem_complete_requests_ring(req->ring, lazy_coherency);
> -
> return req->complete;
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index edf712b..039dbb8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1264,6 +1264,11 @@ int __i915_wait_request(struct
> drm_i915_gem_request *req,
> if (!irq_test_in_progress && WARN_ON(!ring->irq_get(ring)))
> return -ENODEV;
>
> + /* Completion status should be interrupt driven but it is possible
> + * the request popped out before the interrupt was enabled. So do
> an
> + * explicit check now... */
> + i915_gem_complete_requests_ring(req->ring, false);
> +
> /* Record current time in case interrupted by signal, or wedged */
> trace_i915_gem_request_wait_begin(req);
> before = ktime_get_raw_ns();
> @@ -2487,6 +2492,10 @@ int __i915_add_request(struct intel_engine_cs
> *ring,
> list_add_tail(&request->list, &ring->request_list);
> request->file_priv = NULL;
>
> + /* Avoid race condition where the request completes before it has
> + * been added to the list. */
> + ring->last_read_seqno = 0;
> +
> if (file) {
> struct drm_i915_file_private *file_priv = file->driver_priv;
>
> @@ -2858,6 +2867,7 @@ i915_gem_retire_requests(struct drm_device
> *dev)
> int i;
>
> for_each_ring(ring, dev_priv, i) {
> + i915_gem_complete_requests_ring(ring, false);
> i915_gem_retire_requests_ring(ring);
> idle &= list_empty(&ring->request_list);
> }
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c index 198bbc6..4f63966 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -987,6 +987,8 @@ static void notify_ring(struct drm_device *dev,
>
> trace_i915_gem_request_complete(ring);
>
> + i915_gem_complete_requests_ring(ring, false);
I915_gem_complete_requests_ring takes the request list lock and iterates over the entire list. Are you sure this is safe to do during the interrupt handler?

Thomas.

> +
> wake_up_all(&ring->irq_queue);
> i915_queue_hangcheck(dev);
> }
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx