| Message ID | 1461780195-17434-2-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/04/16 19:03, Dave Gordon wrote: > The knowledge of how to derive the relevant client from the request > should be localised within i915_guc_submission.c; the LRC code shouldn't > have to know about the internal details of the GuC submission process. > And all the information the GuC code needs should be encapsulated in (or > reachable from) the request. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++----- > drivers/gpu/drm/i915/intel_guc.h | 5 ++--- > drivers/gpu/drm/i915/intel_lrc.c | 9 +++------ > 3 files changed, 11 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c > index 42d2efa..66af5ce 100644 > --- a/drivers/gpu/drm/i915/i915_guc_submission.c > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c > @@ -451,10 +451,11 @@ static void guc_fini_ctx_desc(struct intel_guc *guc, > sizeof(desc) * client->ctx_index); > } > > -int i915_guc_wq_check_space(struct i915_guc_client *gc) > +int i915_guc_wq_check_space(struct drm_i915_gem_request *request) > { > + const size_t size = sizeof(struct guc_wq_item); All the other variables passed to CIRC_SPACE are u32 so this is for the worse I think. > + struct i915_guc_client *gc = request->i915->guc.execbuf_client; > struct guc_process_desc *desc; > - u32 size = sizeof(struct guc_wq_item); > int ret = -ETIMEDOUT, timeout_counter = 200; > > if (!gc) > @@ -537,11 +538,11 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc, > * > * Return: 0 if succeed > */ > -int i915_guc_submit(struct i915_guc_client *client, > - struct drm_i915_gem_request *rq) > +int i915_guc_submit(struct drm_i915_gem_request *rq) > { > - struct intel_guc *guc = client->guc; > unsigned int engine_id = rq->engine->guc_id; > + struct intel_guc *guc = &rq->i915->guc; > + struct i915_guc_client *client = guc->execbuf_client; > int q_ret, b_ret; > > q_ret = guc_add_workqueue_item(client, rq); > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index 9d79c4c..b37c731 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -148,10 +148,9 @@ extern int intel_guc_resume(struct drm_device *dev); > /* i915_guc_submission.c */ > int i915_guc_submission_init(struct drm_device *dev); > int i915_guc_submission_enable(struct drm_device *dev); > -int i915_guc_submit(struct i915_guc_client *client, > - struct drm_i915_gem_request *rq); > +int i915_guc_wq_check_space(struct drm_i915_gem_request *rq); > +int i915_guc_submit(struct drm_i915_gem_request *rq); > void i915_guc_submission_disable(struct drm_device *dev); > void i915_guc_submission_fini(struct drm_device *dev); > -int i915_guc_wq_check_space(struct i915_guc_client *client); > > #endif > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 2b7e6bb..b8ec8c6 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -708,9 +708,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request > * going any further, as the i915_add_request() call > * later on mustn't fail ... > */ > - struct intel_guc *guc = &request->i915->guc; > - > - ret = i915_guc_wq_check_space(guc->execbuf_client); > + ret = i915_guc_wq_check_space(request); > if (ret) > return ret; > } > @@ -776,7 +774,6 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, > intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) > { > struct intel_ringbuffer *ringbuf = request->ringbuf; > - struct drm_i915_private *dev_priv = request->i915; > struct intel_engine_cs *engine = request->engine; > > intel_logical_ring_advance(ringbuf); > @@ -806,8 +803,8 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, > } > } > > - if (dev_priv->guc.execbuf_client) > - i915_guc_submit(dev_priv->guc.execbuf_client, request); > + if (i915.enable_guc_submission) > + i915_guc_submit(request); > else > execlists_context_queue(request); > > With the datatype fix (revert) in i915_guc_wq_check_space: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On 29/04/16 16:08, Tvrtko Ursulin wrote: > > On 27/04/16 19:03, Dave Gordon wrote: >> The knowledge of how to derive the relevant client from the request >> should be localised within i915_guc_submission.c; the LRC code shouldn't >> have to know about the internal details of the GuC submission process. >> And all the information the GuC code needs should be encapsulated in (or >> reachable from) the request. >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> --- >> drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++----- >> drivers/gpu/drm/i915/intel_guc.h | 5 ++--- >> drivers/gpu/drm/i915/intel_lrc.c | 9 +++------ >> 3 files changed, 11 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >> b/drivers/gpu/drm/i915/i915_guc_submission.c >> index 42d2efa..66af5ce 100644 >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >> @@ -451,10 +451,11 @@ static void guc_fini_ctx_desc(struct intel_guc >> *guc, >> sizeof(desc) * client->ctx_index); >> } >> >> -int i915_guc_wq_check_space(struct i915_guc_client *gc) >> +int i915_guc_wq_check_space(struct drm_i915_gem_request *request) >> { >> + const size_t size = sizeof(struct guc_wq_item); > > All the other variables passed to CIRC_SPACE are u32 so this is for the > worse I think. This isn't passed to CIRC_SPACE; this is what the result of CIRC_SPACE is compared against. As it's a constant, gcc knows how big it needs to be to correctly represent its actual value, so whether this is the logically correct "size_t" (as it represents a size) or the practical "u32" or even "u8" (as we know it will fit), the generated code is identical (and I'd hope the compiler would warn us if it made a difference, e.g. using u8 for a sizeof() expression greater than 256). Interestingly, the expected value "sizeof(struct work_queue_item)" is 16, but that doesn't appear in the code at all -- instead gcc compares against 15 so it can use "jbe" (Jump if Below or Equal) to get unsigned-comparison semantics. .Dave. >> + struct i915_guc_client *gc = request->i915->guc.execbuf_client; >> struct guc_process_desc *desc; >> - u32 size = sizeof(struct guc_wq_item); >> int ret = -ETIMEDOUT, timeout_counter = 200; >> >> if (!gc) >> @@ -537,11 +538,11 @@ static int guc_add_workqueue_item(struct >> i915_guc_client *gc, >> * >> * Return: 0 if succeed >> */ >> -int i915_guc_submit(struct i915_guc_client *client, >> - struct drm_i915_gem_request *rq) >> +int i915_guc_submit(struct drm_i915_gem_request *rq) >> { >> - struct intel_guc *guc = client->guc; >> unsigned int engine_id = rq->engine->guc_id; >> + struct intel_guc *guc = &rq->i915->guc; >> + struct i915_guc_client *client = guc->execbuf_client; >> int q_ret, b_ret; >> >> q_ret = guc_add_workqueue_item(client, rq); >> diff --git a/drivers/gpu/drm/i915/intel_guc.h >> b/drivers/gpu/drm/i915/intel_guc.h >> index 9d79c4c..b37c731 100644 >> --- a/drivers/gpu/drm/i915/intel_guc.h >> +++ b/drivers/gpu/drm/i915/intel_guc.h >> @@ -148,10 +148,9 @@ extern int intel_guc_resume(struct drm_device *dev); >> /* i915_guc_submission.c */ >> int i915_guc_submission_init(struct drm_device *dev); >> int i915_guc_submission_enable(struct drm_device *dev); >> -int i915_guc_submit(struct i915_guc_client *client, >> - struct drm_i915_gem_request *rq); >> +int i915_guc_wq_check_space(struct drm_i915_gem_request *rq); >> +int i915_guc_submit(struct drm_i915_gem_request *rq); >> void i915_guc_submission_disable(struct drm_device *dev); >> void i915_guc_submission_fini(struct drm_device *dev); >> -int i915_guc_wq_check_space(struct i915_guc_client *client); >> >> #endif >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >> b/drivers/gpu/drm/i915/intel_lrc.c >> index 2b7e6bb..b8ec8c6 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -708,9 +708,7 @@ int intel_logical_ring_alloc_request_extras(struct >> drm_i915_gem_request *request >> * going any further, as the i915_add_request() call >> * later on mustn't fail ... >> */ >> - struct intel_guc *guc = &request->i915->guc; >> - >> - ret = i915_guc_wq_check_space(guc->execbuf_client); >> + ret = i915_guc_wq_check_space(request); >> if (ret) >> return ret; >> } >> @@ -776,7 +774,6 @@ static int logical_ring_wait_for_space(struct >> drm_i915_gem_request *req, >> intel_logical_ring_advance_and_submit(struct drm_i915_gem_request >> *request) >> { >> struct intel_ringbuffer *ringbuf = request->ringbuf; >> - struct drm_i915_private *dev_priv = request->i915; >> struct intel_engine_cs *engine = request->engine; >> >> intel_logical_ring_advance(ringbuf); >> @@ -806,8 +803,8 @@ static int logical_ring_wait_for_space(struct >> drm_i915_gem_request *req, >> } >> } >> >> - if (dev_priv->guc.execbuf_client) >> - i915_guc_submit(dev_priv->guc.execbuf_client, request); >> + if (i915.enable_guc_submission) >> + i915_guc_submit(request); >> else >> execlists_context_queue(request); >> >> > > With the datatype fix (revert) in i915_guc_wq_check_space: > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Regards, > > Tvrtko
On 03/05/16 20:22, Dave Gordon wrote: > On 29/04/16 16:08, Tvrtko Ursulin wrote: >> >> On 27/04/16 19:03, Dave Gordon wrote: >>> The knowledge of how to derive the relevant client from the request >>> should be localised within i915_guc_submission.c; the LRC code shouldn't >>> have to know about the internal details of the GuC submission process. >>> And all the information the GuC code needs should be encapsulated in (or >>> reachable from) the request. >>> >>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++----- >>> drivers/gpu/drm/i915/intel_guc.h | 5 ++--- >>> drivers/gpu/drm/i915/intel_lrc.c | 9 +++------ >>> 3 files changed, 11 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c >>> b/drivers/gpu/drm/i915/i915_guc_submission.c >>> index 42d2efa..66af5ce 100644 >>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c >>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c >>> @@ -451,10 +451,11 @@ static void guc_fini_ctx_desc(struct intel_guc >>> *guc, >>> sizeof(desc) * client->ctx_index); >>> } >>> >>> -int i915_guc_wq_check_space(struct i915_guc_client *gc) >>> +int i915_guc_wq_check_space(struct drm_i915_gem_request *request) >>> { >>> + const size_t size = sizeof(struct guc_wq_item); >> >> All the other variables passed to CIRC_SPACE are u32 so this is for the >> worse I think. > > This isn't passed to CIRC_SPACE; this is what the result of CIRC_SPACE > is compared against. As it's a constant, gcc knows how big it needs to Oh you are right, how did I see it passed in no idea now. Regards, Tvrtko > be to correctly represent its actual value, so whether this is the > logically correct "size_t" (as it represents a size) or the practical > "u32" or even "u8" (as we know it will fit), the generated code is > identical (and I'd hope the compiler would warn us if it made a > difference, e.g. using u8 for a sizeof() expression greater than 256). > > Interestingly, the expected value "sizeof(struct work_queue_item)" is > 16, but that doesn't appear in the code at all -- instead gcc compares > against 15 so it can use "jbe" (Jump if Below or Equal) to get > unsigned-comparison semantics. > > .Dave. > >>> + struct i915_guc_client *gc = request->i915->guc.execbuf_client; >>> struct guc_process_desc *desc; >>> - u32 size = sizeof(struct guc_wq_item); >>> int ret = -ETIMEDOUT, timeout_counter = 200; >>> >>> if (!gc) >>> @@ -537,11 +538,11 @@ static int guc_add_workqueue_item(struct >>> i915_guc_client *gc, >>> * >>> * Return: 0 if succeed >>> */ >>> -int i915_guc_submit(struct i915_guc_client *client, >>> - struct drm_i915_gem_request *rq) >>> +int i915_guc_submit(struct drm_i915_gem_request *rq) >>> { >>> - struct intel_guc *guc = client->guc; >>> unsigned int engine_id = rq->engine->guc_id; >>> + struct intel_guc *guc = &rq->i915->guc; >>> + struct i915_guc_client *client = guc->execbuf_client; >>> int q_ret, b_ret; >>> >>> q_ret = guc_add_workqueue_item(client, rq); >>> diff --git a/drivers/gpu/drm/i915/intel_guc.h >>> b/drivers/gpu/drm/i915/intel_guc.h >>> index 9d79c4c..b37c731 100644 >>> --- a/drivers/gpu/drm/i915/intel_guc.h >>> +++ b/drivers/gpu/drm/i915/intel_guc.h >>> @@ -148,10 +148,9 @@ extern int intel_guc_resume(struct drm_device >>> *dev); >>> /* i915_guc_submission.c */ >>> int i915_guc_submission_init(struct drm_device *dev); >>> int i915_guc_submission_enable(struct drm_device *dev); >>> -int i915_guc_submit(struct i915_guc_client *client, >>> - struct drm_i915_gem_request *rq); >>> +int i915_guc_wq_check_space(struct drm_i915_gem_request *rq); >>> +int i915_guc_submit(struct drm_i915_gem_request *rq); >>> void i915_guc_submission_disable(struct drm_device *dev); >>> void i915_guc_submission_fini(struct drm_device *dev); >>> -int i915_guc_wq_check_space(struct i915_guc_client *client); >>> >>> #endif >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >>> b/drivers/gpu/drm/i915/intel_lrc.c >>> index 2b7e6bb..b8ec8c6 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -708,9 +708,7 @@ int intel_logical_ring_alloc_request_extras(struct >>> drm_i915_gem_request *request >>> * going any further, as the i915_add_request() call >>> * later on mustn't fail ... >>> */ >>> - struct intel_guc *guc = &request->i915->guc; >>> - >>> - ret = i915_guc_wq_check_space(guc->execbuf_client); >>> + ret = i915_guc_wq_check_space(request); >>> if (ret) >>> return ret; >>> } >>> @@ -776,7 +774,6 @@ static int logical_ring_wait_for_space(struct >>> drm_i915_gem_request *req, >>> intel_logical_ring_advance_and_submit(struct drm_i915_gem_request >>> *request) >>> { >>> struct intel_ringbuffer *ringbuf = request->ringbuf; >>> - struct drm_i915_private *dev_priv = request->i915; >>> struct intel_engine_cs *engine = request->engine; >>> >>> intel_logical_ring_advance(ringbuf); >>> @@ -806,8 +803,8 @@ static int logical_ring_wait_for_space(struct >>> drm_i915_gem_request *req, >>> } >>> } >>> >>> - if (dev_priv->guc.execbuf_client) >>> - i915_guc_submit(dev_priv->guc.execbuf_client, request); >>> + if (i915.enable_guc_submission) >>> + i915_guc_submit(request); >>> else >>> execlists_context_queue(request); >>> >>> >> >> With the datatype fix (revert) in i915_guc_wq_check_space: >> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Regards, >> >> Tvrtko > >
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 42d2efa..66af5ce 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -451,10 +451,11 @@ static void guc_fini_ctx_desc(struct intel_guc *guc, sizeof(desc) * client->ctx_index); } -int i915_guc_wq_check_space(struct i915_guc_client *gc) +int i915_guc_wq_check_space(struct drm_i915_gem_request *request) { + const size_t size = sizeof(struct guc_wq_item); + struct i915_guc_client *gc = request->i915->guc.execbuf_client; struct guc_process_desc *desc; - u32 size = sizeof(struct guc_wq_item); int ret = -ETIMEDOUT, timeout_counter = 200; if (!gc) @@ -537,11 +538,11 @@ static int guc_add_workqueue_item(struct i915_guc_client *gc, * * Return: 0 if succeed */ -int i915_guc_submit(struct i915_guc_client *client, - struct drm_i915_gem_request *rq) +int i915_guc_submit(struct drm_i915_gem_request *rq) { - struct intel_guc *guc = client->guc; unsigned int engine_id = rq->engine->guc_id; + struct intel_guc *guc = &rq->i915->guc; + struct i915_guc_client *client = guc->execbuf_client; int q_ret, b_ret; q_ret = guc_add_workqueue_item(client, rq); diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 9d79c4c..b37c731 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -148,10 +148,9 @@ extern int intel_guc_resume(struct drm_device *dev); /* i915_guc_submission.c */ int i915_guc_submission_init(struct drm_device *dev); int i915_guc_submission_enable(struct drm_device *dev); -int i915_guc_submit(struct i915_guc_client *client, - struct drm_i915_gem_request *rq); +int i915_guc_wq_check_space(struct drm_i915_gem_request *rq); +int i915_guc_submit(struct drm_i915_gem_request *rq); void i915_guc_submission_disable(struct drm_device *dev); void i915_guc_submission_fini(struct drm_device *dev); -int i915_guc_wq_check_space(struct i915_guc_client *client); #endif diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 2b7e6bb..b8ec8c6 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -708,9 +708,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request * going any further, as the i915_add_request() call * later on mustn't fail ... */ - struct intel_guc *guc = &request->i915->guc; - - ret = i915_guc_wq_check_space(guc->execbuf_client); + ret = i915_guc_wq_check_space(request); if (ret) return ret; } @@ -776,7 +774,6 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) { struct intel_ringbuffer *ringbuf = request->ringbuf; - struct drm_i915_private *dev_priv = request->i915; struct intel_engine_cs *engine = request->engine; intel_logical_ring_advance(ringbuf); @@ -806,8 +803,8 @@ static int logical_ring_wait_for_space(struct drm_i915_gem_request *req, } } - if (dev_priv->guc.execbuf_client) - i915_guc_submit(dev_priv->guc.execbuf_client, request); + if (i915.enable_guc_submission) + i915_guc_submit(request); else execlists_context_queue(request);
The knowledge of how to derive the relevant client from the request should be localised within i915_guc_submission.c; the LRC code shouldn't have to know about the internal details of the GuC submission process. And all the information the GuC code needs should be encapsulated in (or reachable from) the request. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_guc_submission.c | 11 ++++++----- drivers/gpu/drm/i915/intel_guc.h | 5 ++--- drivers/gpu/drm/i915/intel_lrc.c | 9 +++------ 3 files changed, 11 insertions(+), 14 deletions(-)