Re: [PATCH v2 2/5] drm/i915/guc: pass request (not client) to i915_guc_{wq_check_space, submit}()

[Dave Gordon <david.s.gordon@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx