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

[Goel, Akash]

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 at intel.com wrote:
>>>>>> From: Akash Goel <akash.goel at 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