Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 11/20/2020 6:43 AM, Tvrtko Ursulin wrote:
>
> On 20/11/2020 14:32, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2020-11-20 09:56:36)
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> CT event handler is called under the gt->irq_lock from the interrupt
>>> handling paths so make it the same from the init path. I don't think 
>>> this
>>> mismatch caused any functional issue but we need to wean the code of 
>>> the
>>> global i915->irq_lock.
>>
>> ct_read definitely wants to be serialised. Is guc->irq_lock the right
>> choice?
>
> Not under my understanding and also confirmed by Daniele off line.
>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> index 220626c3ad81..6a0452815c41 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> @@ -203,7 +203,8 @@ static void guc_disable_interrupts(struct 
>>> intel_guc *guc)
>>>     static int guc_enable_communication(struct intel_guc *guc)
>>>   {
>>> -       struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>>> +       struct intel_gt *gt = guc_to_gt(guc);
>>> +       struct drm_i915_private *i915 = gt->i915;
>>>          int ret;
>>>            GEM_BUG_ON(guc_communication_enabled(guc));
>>> @@ -223,9 +224,9 @@ static int guc_enable_communication(struct 
>>> intel_guc *guc)
>>>          guc_enable_interrupts(guc);
>>>            /* check for CT messages received before we enabled 
>>> interrupts */
>>> -       spin_lock_irq(&i915->irq_lock);
>>> +       spin_lock_irq(&gt->irq_lock);
>>>          intel_guc_ct_event_handler(&guc->ct);
>>> -       spin_unlock_irq(&i915->irq_lock);
>>> +       spin_unlock_irq(&gt->irq_lock);
>>
>> You used guc->irq_lock in the previous patch. I suggest
>> intel_guc_ct_event_handler() should specify what lock it requires.
>
> There are indeed too many locks and too little asserts to help the 
> reader.
>
> But the other end of the state ct_read needs is updated from the GuC 
> firmware itself, which then send the interrupt, which we process in:
>
>  guc_irq_handler
>    -> intel_guc_to_host_event_handler
>         -> intel_guc_ct_event_handler
>
> And this side runs under the gt->irq_lock.
>

guc->irq_lock is not very aptly named, as it is used to protect access 
to the guc interrupt state variables (msg_enabled_mask, mmio_msg) and 
has nothing to do with protecting the interrupt handler. For that, as 
Tvrtko said, the GuC code can use the same lock the rest of the GT uses, 
i.e. gt->irq_lock. Maybe we can rename guc->irq_lock to 
guc->msg_state_lock for clarity?

Anyway, this is:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> Regards,
>
> Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx