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

[Tvrtko Ursulin]

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.

>>>>> Signed-off-by: Akash Goel <akash.goel at 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