| Message ID | 1470983123-22127-10-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 */