| Message ID | 1415967559-17074-26-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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
On 18/11/2014 09:40, Daniel, Thomas wrote: >> -----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. The list is generally fairly small (max tens of items when running a desktop with a bunch of OGL apps/benchmarks running concurrently, usually single digit counts). Also, the processing is pretty minimal - do an integer comparison and set a flag if true. The overhead really is not great. >> + >> 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
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); + wake_up_all(&ring->irq_queue); i915_queue_hangcheck(dev); }