| Message ID | 1468158084-22028-14-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/07/16 14:41, 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. After which patch in this series is this required? > > 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 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... > > 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
On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: > > On 10/07/16 14:41, 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. > > 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@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 >
On 15/07/16 16:51, Goel, Akash wrote: > > > On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: >> >> On 10/07/16 14:41, 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. >> >> 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). That means this patch should come before 6 or 7. :) > Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & > UK_LOG_ENABLE_LOGGING can execute concurrently with each other. Right I see, from the worker/thread vs debugfs activity. >>> >>> 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 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. I wonder about that in general, since in_atomic is not a reliable indicator. But that is beside the point. You probably haven't seen it because the action completes in the first shorter, atomic sleep, check. Regards, Tvrtko
On 7/18/2016 3:42 PM, Tvrtko Ursulin wrote: > > On 15/07/16 16:51, Goel, Akash wrote: >> >> >> On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: >>> >>> On 10/07/16 14:41, 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. >>> >>> 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). > > That means this patch should come before 6 or 7. :) > >> Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & >> UK_LOG_ENABLE_LOGGING can execute concurrently with each other. > > Right I see, from the worker/thread vs debugfs activity. > Will use mutex to serialize and place the patch earlier in the series. Please suggest which would be better, mutex_lock() or mutex_lock_interruptible(). >>>> >>>> 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 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. > > I wonder about that in general, since in_atomic is not a reliable > indicator. But that is beside the point. You probably haven't seen it > because the action completes in the first shorter, atomic sleep, check. > Actually I had profiled host2guc_logbuffer_flush_complete() and saw that on some occasions it was taking more than 100 micro seconds, so presumably it would have went past the first wait. But most of the times it was less than 10 micro seconds only. ret = wait_for_us(host2guc_action_response(dev_priv, &status), 10); if (ret) ret = wait_for(host2guc_action_response(dev_priv, &status), 10); Best regards Akash > Regards, > > Tvrtko
On 18/07/16 11:46, Goel, Akash wrote: > On 7/18/2016 3:42 PM, Tvrtko Ursulin wrote: >> >> On 15/07/16 16:51, Goel, Akash wrote: >>> >>> >>> On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: >>>> >>>> On 10/07/16 14:41, 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. >>>> >>>> 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). >> >> That means this patch should come before 6 or 7. :) >> >>> Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & >>> UK_LOG_ENABLE_LOGGING can execute concurrently with each other. >> >> Right I see, from the worker/thread vs debugfs activity. >> > Will use mutex to serialize and place the patch earlier in the series. > Please suggest which would be better, > mutex_lock() > or > mutex_lock_interruptible(). Interruptible from the debugfs paths, otherwise not. >>>>> 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 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. >> >> I wonder about that in general, since in_atomic is not a reliable >> indicator. But that is beside the point. You probably haven't seen it >> because the action completes in the first shorter, atomic sleep, check. >> > Actually I had profiled host2guc_logbuffer_flush_complete() and saw that > on some occasions it was taking more than 100 micro seconds, > so presumably it would have went past the first wait. > But most of the times it was less than 10 micro seconds only. > > ret = wait_for_us(host2guc_action_response(dev_priv, &status), 10); > if (ret) > ret = wait_for(host2guc_action_response(dev_priv, &status), 10); Yes presumably so. In that case keep in mind that in_atomic always returns false in spinlock sections unless the kernel has CONFIG_PREEMPT_COUNT enabled. Regards, Tvrtko
On 7/18/2016 4:48 PM, Tvrtko Ursulin wrote: > > On 18/07/16 11:46, Goel, Akash wrote: >> On 7/18/2016 3:42 PM, Tvrtko Ursulin wrote: >>> >>> On 15/07/16 16:51, Goel, Akash wrote: >>>> >>>> >>>> On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: >>>>> >>>>> On 10/07/16 14:41, 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. >>>>> >>>>> 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). >>> >>> That means this patch should come before 6 or 7. :) >>> >>>> Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & >>>> UK_LOG_ENABLE_LOGGING can execute concurrently with each other. >>> >>> Right I see, from the worker/thread vs debugfs activity. >>> >> Will use mutex to serialize and place the patch earlier in the series. >> Please suggest which would be better, >> mutex_lock() >> or >> mutex_lock_interruptible(). > > Interruptible from the debugfs paths, otherwise not. Yes calls from debugfs path should ideally use interruptible version, but then how to determine that whether the given host2guc_action call came from debugfs path. Should I add a new argument 'interruptible_wait' to host2guc_action() or to keep things simple use mutex_lock() only ? I thought it would be cleaner to abstract the lock usage, for serialization, entirely inside the host2guc_action only. >>>>>> --- 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. >>> >>> I wonder about that in general, since in_atomic is not a reliable >>> indicator. But that is beside the point. You probably haven't seen it >>> because the action completes in the first shorter, atomic sleep, check. >>> >> Actually I had profiled host2guc_logbuffer_flush_complete() and saw that >> on some occasions it was taking more than 100 micro seconds, >> so presumably it would have went past the first wait. >> But most of the times it was less than 10 micro seconds only. >> >> ret = wait_for_us(host2guc_action_response(dev_priv, &status), 10); >> if (ret) >> ret = wait_for(host2guc_action_response(dev_priv, &status), 10); > > Yes presumably so. In that case keep in mind that in_atomic always > returns false in spinlock sections unless the kernel has > CONFIG_PREEMPT_COUNT enabled. > Thanks for this info, will be mindful of this in future. Best regards Akash > Regards, > > Tvrtko
On 18/07/16 12:31, Goel, Akash wrote: > > > On 7/18/2016 4:48 PM, Tvrtko Ursulin wrote: >> >> On 18/07/16 11:46, Goel, Akash wrote: >>> On 7/18/2016 3:42 PM, Tvrtko Ursulin wrote: >>>> >>>> On 15/07/16 16:51, Goel, Akash wrote: >>>>> >>>>> >>>>> On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: >>>>>> >>>>>> On 10/07/16 14:41, 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. >>>>>> >>>>>> 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). >>>> >>>> That means this patch should come before 6 or 7. :) >>>> >>>>> Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & >>>>> UK_LOG_ENABLE_LOGGING can execute concurrently with each other. >>>> >>>> Right I see, from the worker/thread vs debugfs activity. >>>> >>> Will use mutex to serialize and place the patch earlier in the series. >>> Please suggest which would be better, >>> mutex_lock() >>> or >>> mutex_lock_interruptible(). >> >> Interruptible from the debugfs paths, otherwise not. > > Yes calls from debugfs path should ideally use interruptible version, > but then how to determine that whether the given host2guc_action call > came from debugfs path. > Should I add a new argument 'interruptible_wait' to host2guc_action() or > to keep things simple use mutex_lock() only ? > I thought it would be cleaner to abstract the lock usage, for > serialization, entirely inside the host2guc_action only. Hm yes, good point. I think a simple mutex lock should be fine then. You don't expect to be holding it for more than 10ms in the absolutely worst case and I think simplicity of the implementation wins in this case even if it stalls userspace a bit. Regards, Tvrtko
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); 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); 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 */