[09/20] drm/i915: New lock to serialize the Host2GuC actions

Submitted by Akash Goel on Aug. 12, 2016, 6:25 a.m.

Details

Message ID 1470983123-22127-10-git-send-email-akash.goel@intel.com
State New
Headers show
Series "Support for sustained capturing of GuC firmware logs" ( rev: 6 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Akash Goel Aug. 12, 2016, 6:25 a.m.
From: Akash Goel <akash.goel@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@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++
 drivers/gpu/drm/i915/intel_guc.h           | 3 +++
 2 files changed, 6 insertions(+)

Patch hide | download patch | download mbox

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);
 
 	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 */

Comments

On 12/08/16 07:25, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@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@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.

>
>   	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@intel.com>

Regards,

Tvrtko
On 8/12/2016 7:25 PM, Tvrtko Ursulin wrote:
>
> On 12/08/16 07:25, akash.goel@intel.com wrote:
>> From: Akash Goel <akash.goel@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@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@intel.com>
>
> Regards,
>
> Tvrtko