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@xxxxxxxxx>
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@xxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
---
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(>->irq_lock);
intel_guc_ct_event_handler(&guc->ct);
- spin_unlock_irq(&i915->irq_lock);
+ spin_unlock_irq(>->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@xxxxxxxxx>
Daniele
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
