| Message ID | 20180322131034.6036-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Both request_submit and request_unsubmit deal with transferring the > request from the client's timeline onto the execution timeline and back > again. As both functions deal with a pair of timeline's, using a > shorthand for just one of them is slightly confusing, especially as the The amount confusion is correlated with readers previous knowledge about timelines. Slightly is an understatement for the newcomers but we can accept that as a good amount of confusion, on average. > different functions use the shorthand for the alternate timeline. > Instead, use the full version of each timeline so it should be easier to > keep track of the transfer between the request/client and the engine. > I pondered if the comment can even be removed on this, much easier to read, version. But as the comment gives a proper names to timelines, it has value. Only one thing remains is that should the 'client's timeline' be changed to 'per context timeline' on this commit msg, to reflect the comment in the code. > v2: Refactor the common lock+list_move > v3: Be clear we require the other timeline list to be locked as well. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_request.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index 2325886d1d55..854af07994f3 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -492,10 +492,20 @@ static u32 timeline_get_seqno(struct intel_timeline *tl) > return ++tl->seqno; > } > > +static void move_to_timeline(struct i915_request *request, > + struct intel_timeline *timeline) > +{ > + GEM_BUG_ON(request->timeline == request->engine->timeline); > + lockdep_assert_held(&request->engine->timeline->lock); > + > + spin_lock(&request->timeline->lock); > + list_move_tail(&request->link, &timeline->requests); > + spin_unlock(&request->timeline->lock); > +} > + > void __i915_request_submit(struct i915_request *request) > { > struct intel_engine_cs *engine = request->engine; > - struct intel_timeline *timeline; > u32 seqno; > > GEM_TRACE("%s fence %llx:%d -> global_seqno %d\n", > @@ -506,12 +516,9 @@ void __i915_request_submit(struct i915_request *request) > GEM_BUG_ON(!irqs_disabled()); > lockdep_assert_held(&engine->timeline->lock); > > - /* Transfer from per-context onto the global per-engine timeline */ > - timeline = engine->timeline; > - GEM_BUG_ON(timeline == request->timeline); > GEM_BUG_ON(request->global_seqno); > > - seqno = timeline_get_seqno(timeline); > + seqno = timeline_get_seqno(engine->timeline); > GEM_BUG_ON(!seqno); > GEM_BUG_ON(i915_seqno_passed(intel_engine_get_seqno(engine), seqno)); > > @@ -525,9 +532,8 @@ void __i915_request_submit(struct i915_request *request) > engine->emit_breadcrumb(request, > request->ring->vaddr + request->postfix); > > - spin_lock(&request->timeline->lock); > - list_move_tail(&request->link, &timeline->requests); > - spin_unlock(&request->timeline->lock); > + /* Transfer from per-context onto the global per-engine timeline */ > + move_to_timeline(request, engine->timeline); > > trace_i915_request_execute(request); > > @@ -550,7 +556,6 @@ void i915_request_submit(struct i915_request *request) > void __i915_request_unsubmit(struct i915_request *request) > { > struct intel_engine_cs *engine = request->engine; > - struct intel_timeline *timeline; > > GEM_TRACE("%s fence %llx:%d <- global_seqno %d\n", > request->engine->name, > @@ -578,12 +583,7 @@ void __i915_request_unsubmit(struct i915_request *request) > spin_unlock(&request->lock); > > /* Transfer back from the global per-engine timeline to per-context */ > - timeline = request->timeline; > - GEM_BUG_ON(timeline == engine->timeline); > - > - spin_lock(&timeline->lock); > - list_move(&request->link, &timeline->requests); > - spin_unlock(&timeline->lock); > + move_to_timeline(request, request->timeline); > > /* > * We don't need to wake_up any waiters on request->execute, they > -- > 2.16.2
Quoting Mika Kuoppala (2018-03-22 13:24:13) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Both request_submit and request_unsubmit deal with transferring the > > request from the client's timeline onto the execution timeline and back > > again. As both functions deal with a pair of timeline's, using a > > shorthand for just one of them is slightly confusing, especially as the > > The amount confusion is correlated with readers previous knowledge about > timelines. Slightly is an understatement for the newcomers but > we can accept that as a good amount of confusion, on average. > > > different functions use the shorthand for the alternate timeline. > > Instead, use the full version of each timeline so it should be easier to > > keep track of the transfer between the request/client and the engine. > > > > I pondered if the comment can even be removed on this, much > easier to read, version. But as the comment gives a > proper names to timelines, it has value. > > Only one thing remains is that should the 'client's timeline' > be changed to 'per context timeline' on this commit msg, > to reflect the comment in the code. I use client/context quite interchangeably. Every context is a client, but a client may have more than one context. I should probably avoid "client" altogether since we only have fd and contexts, and a hint of process (which is more or less limited to mmu, more or less meaningless when we look at the GPU structs). -Chris
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 2325886d1d55..854af07994f3 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -492,10 +492,20 @@ static u32 timeline_get_seqno(struct intel_timeline *tl) return ++tl->seqno; } +static void move_to_timeline(struct i915_request *request, + struct intel_timeline *timeline) +{ + GEM_BUG_ON(request->timeline == request->engine->timeline); + lockdep_assert_held(&request->engine->timeline->lock); + + spin_lock(&request->timeline->lock); + list_move_tail(&request->link, &timeline->requests); + spin_unlock(&request->timeline->lock); +} + void __i915_request_submit(struct i915_request *request) { struct intel_engine_cs *engine = request->engine; - struct intel_timeline *timeline; u32 seqno; GEM_TRACE("%s fence %llx:%d -> global_seqno %d\n", @@ -506,12 +516,9 @@ void __i915_request_submit(struct i915_request *request) GEM_BUG_ON(!irqs_disabled()); lockdep_assert_held(&engine->timeline->lock); - /* Transfer from per-context onto the global per-engine timeline */ - timeline = engine->timeline; - GEM_BUG_ON(timeline == request->timeline); GEM_BUG_ON(request->global_seqno); - seqno = timeline_get_seqno(timeline); + seqno = timeline_get_seqno(engine->timeline); GEM_BUG_ON(!seqno); GEM_BUG_ON(i915_seqno_passed(intel_engine_get_seqno(engine), seqno)); @@ -525,9 +532,8 @@ void __i915_request_submit(struct i915_request *request) engine->emit_breadcrumb(request, request->ring->vaddr + request->postfix); - spin_lock(&request->timeline->lock); - list_move_tail(&request->link, &timeline->requests); - spin_unlock(&request->timeline->lock); + /* Transfer from per-context onto the global per-engine timeline */ + move_to_timeline(request, engine->timeline); trace_i915_request_execute(request); @@ -550,7 +556,6 @@ void i915_request_submit(struct i915_request *request) void __i915_request_unsubmit(struct i915_request *request) { struct intel_engine_cs *engine = request->engine; - struct intel_timeline *timeline; GEM_TRACE("%s fence %llx:%d <- global_seqno %d\n", request->engine->name, @@ -578,12 +583,7 @@ void __i915_request_unsubmit(struct i915_request *request) spin_unlock(&request->lock); /* Transfer back from the global per-engine timeline to per-context */ - timeline = request->timeline; - GEM_BUG_ON(timeline == engine->timeline); - - spin_lock(&timeline->lock); - list_move(&request->link, &timeline->requests); - spin_unlock(&timeline->lock); + move_to_timeline(request, request->timeline); /* * We don't need to wake_up any waiters on request->execute, they
Both request_submit and request_unsubmit deal with transferring the request from the client's timeline onto the execution timeline and back again. As both functions deal with a pair of timeline's, using a shorthand for just one of them is slightly confusing, especially as the different functions use the shorthand for the alternate timeline. Instead, use the full version of each timeline so it should be easier to keep track of the transfer between the request/client and the engine. v2: Refactor the common lock+list_move v3: Be clear we require the other timeline list to be locked as well. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/i915_request.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)