[Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions

[Goel, Akash]

On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote:
>
> On 10/07/16 14:41, akash.goel at intel.com wrote:
>> From: Akash Goel <akash.goel at intel.com>
>>
>> With the addition of new Host2GuC actions related to GuC logging, there
>> is a need of a lock to serialize them, as they can execute concurrently
>> with each other and also with other existing actions.
>
> After which patch in this series is this required?
>
 From patch 6 or 7 saw the problem, when enabled flush interrupts from 
boot (guc_log_level >= 0).

Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & 
UK_LOG_ENABLE_LOGGING can execute concurrently with each other.

>>
>> Signed-off-by: Akash Goel <akash.goel at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++
>>   drivers/gpu/drm/i915/intel_guc.h           | 3 +++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 6043166..c1e637f 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -88,6 +88,7 @@ static int host2guc_action(struct intel_guc *guc,
>> u32 *data, u32 len)
>>           return -EINVAL;
>>
>>       intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>> +    spin_lock(&guc->action_lock);
>
> The code below can sleep waiting for a response from GuC so you cannot
> use a spinlock. Mutex I suppose...

Sorry I missed the sleep.
Probably I did not see any problem, in spite of a spinlock, as _wait_for 
macro does not sleep when used in atomic context, does a busy wait instead.

Best Regards
Akash

>
>>
>>       dev_priv->guc.action_count += 1;
>>       dev_priv->guc.action_cmd = data[0];
>> @@ -126,6 +127,7 @@ static int host2guc_action(struct intel_guc *guc,
>> u32 *data, u32 len)
>>       }
>>       dev_priv->guc.action_status = status;
>>
>> +    spin_unlock(&guc->action_lock);
>>       intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>
>>       return ret;
>> @@ -1304,6 +1306,7 @@ int i915_guc_submission_init(struct
>> drm_i915_private *dev_priv)
>>           return -ENOMEM;
>>
>>       ida_init(&guc->ctx_ids);
>> +    spin_lock_init(&guc->action_lock);
>
> I think this should go to guc_client_alloc which is where the guc client
> object is allocated and initialized.
>
>>       guc_create_log(guc);
>>       guc_create_ads(guc);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index d56bde6..611f4a7 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -157,6 +157,9 @@ struct intel_guc {
>>
>>       uint64_t submissions[I915_NUM_ENGINES];
>>       uint32_t last_seqno[I915_NUM_ENGINES];
>> +
>> +    /* To serialize the Host2GuC actions */
>> +    spinlock_t action_lock;
>>   };
>>
>>   /* intel_guc_loader.c */
>>
>
> Regards,
>
> Tvrtko
>