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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx