Re: [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions

["Goel, Akash" <akash.goel@intel.com>]

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