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

[Tvrtko Ursulin]

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.

>>>
>>> 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.

Regards,

Tvrtko