[Intel-gfx] [PATCH v2 4/7] drm/i915/guc/ct: Group request-related variables in a sub-structure

[Daniele Ceraolo Spurio]

On 12/17/19 1:42 PM, Michal Wajdeczko wrote:
> On Tue, 17 Dec 2019 02:23:13 +0100, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio at intel.com> wrote:
> 
>> For better isolation of the request tracking from the rest of the
>> CT-related data.
>>
>> v2: split to separate patch, move next_fence to substructure (Michal)
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: John Harrison <John.C.Harrison at Intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> ---
> 
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> 
> with some nits below (we may fix them later)
> 
> /snip/
> 
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> index 6e3d789b9f01..29a026dc3a13 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
>> @@ -48,12 +48,15 @@ struct intel_guc_ct {
>>      /* buffers for sending(0) and receiving(1) commands */
>>      struct intel_guc_ct_buffer ctbs[2];
>> -    u32 next_fence; /* fence to be used with next send command */
>> +    struct {
>> +        u32 next_fence; /* fence to be used with next request to send */
> 
> nit: strictly speaking this is "last" fence
>       we just use it to generate next one
> 
>> -    spinlock_t lock; /* protects pending requests list */
>> -    struct list_head pending_requests; /* requests waiting for 
>> response */
>> -    struct list_head incoming_requests; /* incoming requests */
>> -    struct work_struct worker; /* handler for incoming requests */
>> +        spinlock_t lock; /* protects pending requests list */
> 
> nit: do we want to use this lock to protect "next/last" fence ?
>       if yes, then maybe lock shall be first ?

We currently only touch this while holding send_mutex, so we don't need 
the spinlock as well. We can move it later if we ever re-organize the 
locking structure.

Daniele

> 
>> +        struct list_head pending; /* requests waiting for response */
>> +
>> +        struct list_head incoming; /* incoming requests */
>> +        struct work_struct worker; /* handler for incoming requests */
>> +    } requests;
>>  };
>> void intel_guc_ct_init_early(struct intel_guc_ct *ct);