Re: [Intel-gfx] [PATCH 5/8] drm/i915: Protect request retirement with timeline->mutex
Quoting Matthew Auld (2019-08-15 21:33:07)
> On Wed, 14 Aug 2019 at 10:28, Chris Wilson wrote:
> >
> > Forgo the struct_mutex requirement for request retirement as we have
> > been transitioning over to only using the timeline->mutex for
> > controlling the lifetime of a request on that timeline.
> >
> > Signed-off-by: Chris Wilson
> > ---
> > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 183 ++
> > drivers/gpu/drm/i915/gt/intel_context.h | 18 +-
> > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 1 -
> > drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 -
> > drivers/gpu/drm/i915/gt/intel_gt.c| 2 -
> > drivers/gpu/drm/i915/gt/intel_gt_types.h | 2 -
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 1 +
> > drivers/gpu/drm/i915/gt/intel_ringbuffer.c| 19 +-
> > drivers/gpu/drm/i915/gt/mock_engine.c | 1 -
> > drivers/gpu/drm/i915/gt/selftest_context.c| 9 +-
> > drivers/gpu/drm/i915/i915_request.c | 156 +++
> > drivers/gpu/drm/i915/i915_request.h | 3 -
> > 12 files changed, 209 insertions(+), 189 deletions(-)
> >
>
> > bool i915_retire_requests(struct drm_i915_private *i915)
> > {
> > - struct intel_ring *ring, *tmp;
> > + struct intel_gt_timelines *timelines = &i915->gt.timelines;
> > + struct intel_timeline *tl, *tn;
> > + LIST_HEAD(free);
> > +
> > + spin_lock(&timelines->lock);
> > + list_for_each_entry_safe(tl, tn, &timelines->active_list, link) {
> > + if (!mutex_trylock(&tl->mutex))
> > + continue;
> >
> > - lockdep_assert_held(&i915->drm.struct_mutex);
> > + intel_timeline_get(tl);
> > + GEM_BUG_ON(!tl->active_count);
> > + tl->active_count++; /* pin the list element */
> > + spin_unlock(&timelines->lock);
> >
> > - list_for_each_entry_safe(ring, tmp,
> > -&i915->gt.active_rings, active_link) {
> > - intel_ring_get(ring); /* last rq holds reference! */
> > - ring_retire_requests(ring);
> > - intel_ring_put(ring);
> > + retire_requests(tl);
> > +
> > + spin_lock(&timelines->lock);
> > +
> > + /* Restart iteration after dropping lock */
> > + list_safe_reset_next(tl, tn, link);
>
> That's a new one.
I was quite chuffed with that loop, all the pinning across the lock
dropping to ensure the list stayed intact and we could resume from where
we left off.
And if all goes to plan, we should be rarely using this loop!
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/8] drm/i915: Protect request retirement with timeline->mutex
On Wed, 14 Aug 2019 at 10:28, Chris Wilson wrote:
>
> Forgo the struct_mutex requirement for request retirement as we have
> been transitioning over to only using the timeline->mutex for
> controlling the lifetime of a request on that timeline.
>
> Signed-off-by: Chris Wilson
> ---
> .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 183 ++
> drivers/gpu/drm/i915/gt/intel_context.h | 18 +-
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 1 -
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 -
> drivers/gpu/drm/i915/gt/intel_gt.c| 2 -
> drivers/gpu/drm/i915/gt/intel_gt_types.h | 2 -
> drivers/gpu/drm/i915/gt/intel_lrc.c | 1 +
> drivers/gpu/drm/i915/gt/intel_ringbuffer.c| 19 +-
> drivers/gpu/drm/i915/gt/mock_engine.c | 1 -
> drivers/gpu/drm/i915/gt/selftest_context.c| 9 +-
> drivers/gpu/drm/i915/i915_request.c | 156 +++
> drivers/gpu/drm/i915/i915_request.h | 3 -
> 12 files changed, 209 insertions(+), 189 deletions(-)
>
> bool i915_retire_requests(struct drm_i915_private *i915)
> {
> - struct intel_ring *ring, *tmp;
> + struct intel_gt_timelines *timelines = &i915->gt.timelines;
> + struct intel_timeline *tl, *tn;
> + LIST_HEAD(free);
> +
> + spin_lock(&timelines->lock);
> + list_for_each_entry_safe(tl, tn, &timelines->active_list, link) {
> + if (!mutex_trylock(&tl->mutex))
> + continue;
>
> - lockdep_assert_held(&i915->drm.struct_mutex);
> + intel_timeline_get(tl);
> + GEM_BUG_ON(!tl->active_count);
> + tl->active_count++; /* pin the list element */
> + spin_unlock(&timelines->lock);
>
> - list_for_each_entry_safe(ring, tmp,
> -&i915->gt.active_rings, active_link) {
> - intel_ring_get(ring); /* last rq holds reference! */
> - ring_retire_requests(ring);
> - intel_ring_put(ring);
> + retire_requests(tl);
> +
> + spin_lock(&timelines->lock);
> +
> + /* Restart iteration after dropping lock */
> + list_safe_reset_next(tl, tn, link);
That's a new one.
"Several hours later",
Reviewed-by: Matthew Auld
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/8] drm/i915: Protect request retirement with timeline->mutex
Forgo the struct_mutex requirement for request retirement as we have
been transitioning over to only using the timeline->mutex for
controlling the lifetime of a request on that timeline.
Signed-off-by: Chris Wilson
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 183 ++
drivers/gpu/drm/i915/gt/intel_context.h | 18 +-
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 1 -
drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 -
drivers/gpu/drm/i915/gt/intel_gt.c| 2 -
drivers/gpu/drm/i915/gt/intel_gt_types.h | 2 -
drivers/gpu/drm/i915/gt/intel_lrc.c | 1 +
drivers/gpu/drm/i915/gt/intel_ringbuffer.c| 19 +-
drivers/gpu/drm/i915/gt/mock_engine.c | 1 -
drivers/gpu/drm/i915/gt/selftest_context.c| 9 +-
drivers/gpu/drm/i915/i915_request.c | 156 +++
drivers/gpu/drm/i915/i915_request.h | 3 -
12 files changed, 209 insertions(+), 189 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 91512cc6d7a6..1263b02011f4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -735,63 +735,6 @@ static int eb_select_context(struct i915_execbuffer *eb)
return 0;
}
-static struct i915_request *__eb_wait_for_ring(struct intel_ring *ring)
-{
- struct i915_request *rq;
-
- /*
-* Completely unscientific finger-in-the-air estimates for suitable
-* maximum user request size (to avoid blocking) and then backoff.
-*/
- if (intel_ring_update_space(ring) >= PAGE_SIZE)
- return NULL;
-
- /*
-* Find a request that after waiting upon, there will be at least half
-* the ring available. The hysteresis allows us to compete for the
-* shared ring and should mean that we sleep less often prior to
-* claiming our resources, but not so long that the ring completely
-* drains before we can submit our next request.
-*/
- list_for_each_entry(rq, &ring->request_list, ring_link) {
- if (__intel_ring_space(rq->postfix,
- ring->emit, ring->size) > ring->size / 2)
- break;
- }
- if (&rq->ring_link == &ring->request_list)
- return NULL; /* weird, we will check again later for real */
-
- return i915_request_get(rq);
-}
-
-static int eb_wait_for_ring(const struct i915_execbuffer *eb)
-{
- struct i915_request *rq;
- int ret = 0;
-
- /*
-* Apply a light amount of backpressure to prevent excessive hogs
-* from blocking waiting for space whilst holding struct_mutex and
-* keeping all of their resources pinned.
-*/
-
- rq = __eb_wait_for_ring(eb->context->ring);
- if (rq) {
- mutex_unlock(&eb->i915->drm.struct_mutex);
-
- if (i915_request_wait(rq,
- I915_WAIT_INTERRUPTIBLE,
- MAX_SCHEDULE_TIMEOUT) < 0)
- ret = -EINTR;
-
- i915_request_put(rq);
-
- mutex_lock(&eb->i915->drm.struct_mutex);
- }
-
- return ret;
-}
-
static int eb_lookup_vmas(struct i915_execbuffer *eb)
{
struct radix_tree_root *handles_vma = &eb->gem_context->handles_vma;
@@ -2132,10 +2075,75 @@ static const enum intel_engine_id user_ring_map[] = {
[I915_EXEC_VEBOX] = VECS0
};
-static int eb_pin_context(struct i915_execbuffer *eb, struct intel_context *ce)
+static struct i915_request *eb_throttle(struct intel_context *ce)
+{
+ struct intel_ring *ring = ce->ring;
+ struct intel_timeline *tl = ce->timeline;
+ struct i915_request *rq;
+
+ /*
+* Completely unscientific finger-in-the-air estimates for suitable
+* maximum user request size (to avoid blocking) and then backoff.
+*/
+ if (intel_ring_update_space(ring) >= PAGE_SIZE)
+ return NULL;
+
+ /*
+* Find a request that after waiting upon, there will be at least half
+* the ring available. The hysteresis allows us to compete for the
+* shared ring and should mean that we sleep less often prior to
+* claiming our resources, but not so long that the ring completely
+* drains before we can submit our next request.
+*/
+ list_for_each_entry(rq, &tl->requests, link) {
+ if (rq->ring != ring)
+ continue;
+
+ if (__intel_ring_space(rq->postfix,
+ ring->emit, ring->size) > ring->size / 2)
+ break;
+ }
+ if (&rq->link == &tl->requests)
+ return NULL; /* weird, we will check again later for real */
+
+ return i915_request_get(rq);
+}
+
+static int
+__eb_pin_context(struct
