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

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

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