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

[Goel, Akash]

On 8/12/2016 7:25 PM, Tvrtko Ursulin wrote:
>
> On 12/08/16 07:25, 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.
>>
>> v2: Use mutex in place of spinlock to serialize, as sleep can happen
>>      while waiting for the action's response from GuC. (Tvrtko)
>>
>> 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 1a2d648..cb9672b 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);
>> +    mutex_lock(&guc->action_lock);
>
> I would probably take the mutex before grabbing forcewake as a general
> rule. Not that I think it matters in this case since we don't expect any
> contention on this one.
>
Yes did not expected a contention for this mutex, hence thought it use 
just around the code where it is actually needed.
Will move it before the forcewake, as you suggested, to conform to the 
rules.

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;
>>
>> +    mutex_unlock(&guc->action_lock);
>>       intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>
>>       return ret;
>> @@ -1312,6 +1314,7 @@ int i915_guc_submission_init(struct
>> drm_i915_private *dev_priv)
>>           return -ENOMEM;
>>
>>       ida_init(&guc->ctx_ids);
>> +    mutex_init(&guc->action_lock);
>>       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 96ef7dc..e4ec8d8 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -156,6 +156,9 @@ struct intel_guc {
>>
>>       uint64_t submissions[I915_NUM_ENGINES];
>>       uint32_t last_seqno[I915_NUM_ENGINES];
>> +
>> +    /* To serialize the Host2GuC actions */
>> +    struct mutex action_lock;
>>   };
>>
>>   /* intel_guc_loader.c */
>>
>
> With or without the mutex vs forcewake ordering change:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> Regards,
>
> Tvrtko