[PATCH v3] drm/i915/guc: Add synchronization on interrupt enable flag
Dong, Zhanjun
zhanjun.dong at intel.com
Mon Aug 25 23:51:39 UTC 2025
For ct_try_receive_message, issue happens on tasklet path, not the
caller intel_guc_to_host_event_handler, so sounds like need to add it back.
Due to the low reproduce rate, I Will test it for few days before send
next rev.
Regards,
Zhanjun Dong
On 2025-08-25 5:33 p.m., Dong, Zhanjun wrote:
> The skip on disabled case for tasklet add in v2, effected part:
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/
> drm/i915/gt/uc/intel_guc_ct.c
> index 380a11c92d63..f0ee3f1235d4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -1326,6 +1326,9 @@ static void ct_try_receive_message(struct
> intel_guc_ct *ct)
> {
> int ret;
>
> + if (!atomic_read(&ct_to_guc(ct)->interrupts.enabled))
> + return;
> +
> if (GEM_WARN_ON(!ct->enabled))
> return;
>
> This part was dropped, caller intel_guc_to_host_event_handler already
> did that check.
>
> Regards,
> Zhanjun Dong
>
> On 2025-08-25 5:19 p.m., Zhanjun Dong wrote:
>> Boolean flag access from interrupt context might have synchronous issueis
>> on multiple processor platform, flags modified by one core might be read
>> as an old value by another core. This issue on interrupt enable flag
>> might
>> causes interrupt misses or leakage.
>> Change the interrupts.enable type to atomic to ensure memory
>> synchronization.
>>
>> Fixes: a187f13d51fa0 ("drm/i915/guc: handle interrupts from media GuC")
>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14834
>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>> Cc: Andi Shyti <andi.shyti at kernel.org>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>
>> ---
>> Change history:
>> v3: Drop skip on disabled case for tasklet
>> Drop memory barrier
>> v2: Add skip on disabled case for tasklet
>> Add memory barrier after flag changed
>> Add Closes tag and typo fix
>> ---
>> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 2 +-
>> drivers/gpu/drm/i915/gt/uc/intel_guc.c | 11 +++++++----
>> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 4 ++--
>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 5 +++--
>> 4 files changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/
>> i915/gt/intel_gt_irq.c
>> index 75e802e10be2..21804eec8320 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> @@ -20,7 +20,7 @@
>> static void guc_irq_handler(struct intel_guc *guc, u16 iir)
>> {
>> - if (unlikely(!guc->interrupts.enabled))
>> + if (unlikely(!atomic_read(&guc->interrupts.enabled)))
>> return;
>> if (iir & GUC_INTR_GUC2HOST)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/
>> i915/gt/uc/intel_guc.c
>> index f360f020d8f1..1b8d3bbfa16d 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -100,8 +100,8 @@ static void gen9_enable_guc_interrupts(struct
>> intel_guc *guc)
>> gt->pm_guc_events);
>> gen6_gt_pm_enable_irq(gt, gt->pm_guc_events);
>> spin_unlock_irq(gt->irq_lock);
>> + atomic_set(&guc->interrupts.enabled, true);
>> - guc->interrupts.enabled = true;
>> }
>> static void gen9_disable_guc_interrupts(struct intel_guc *guc)
>> @@ -109,7 +109,8 @@ static void gen9_disable_guc_interrupts(struct
>> intel_guc *guc)
>> struct intel_gt *gt = guc_to_gt(guc);
>> assert_rpm_wakelock_held(>->i915->runtime_pm);
>> - guc->interrupts.enabled = false;
>> + atomic_set(&guc->interrupts.enabled, false);
>> +
>> spin_lock_irq(gt->irq_lock);
>> @@ -146,14 +147,16 @@ static void gen11_enable_guc_interrupts(struct
>> intel_guc *guc)
>> __gen11_reset_guc_interrupts(gt);
>> spin_unlock_irq(gt->irq_lock);
>> - guc->interrupts.enabled = true;
>> + atomic_set(&guc->interrupts.enabled, true);
>> +
>> }
>> static void gen11_disable_guc_interrupts(struct intel_guc *guc)
>> {
>> struct intel_gt *gt = guc_to_gt(guc);
>> - guc->interrupts.enabled = false;
>> + atomic_set(&guc->interrupts.enabled, false);
>> +
>> intel_synchronize_irq(gt->i915);
>> gen11_reset_guc_interrupts(guc);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/
>> i915/gt/uc/intel_guc.h
>> index 053780f562c1..40242bbb166e 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>> @@ -93,7 +93,7 @@ struct intel_guc {
>> /** @interrupts: pointers to GuC interrupt-managing functions. */
>> struct {
>> - bool enabled;
>> + atomic_t enabled;
>> void (*reset)(struct intel_guc *guc);
>> void (*enable)(struct intel_guc *guc);
>> void (*disable)(struct intel_guc *guc);
>> @@ -393,7 +393,7 @@ static inline int intel_guc_send_busy_loop(struct
>> intel_guc *guc,
>> /* Only call this from the interrupt handler code */
>> static inline void intel_guc_to_host_event_handler(struct intel_guc
>> *guc)
>> {
>> - if (guc->interrupts.enabled)
>> + if (atomic_read(&guc->interrupts.enabled))
>> intel_guc_ct_event_handler(&guc->ct);
>> }
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/
>> i915/gt/uc/intel_uc.c
>> index 4a3493e8d433..9d01c3c3d504 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -659,7 +659,8 @@ void intel_uc_runtime_suspend(struct intel_uc *uc)
>> struct intel_guc *guc = &uc->guc;
>> if (!intel_guc_is_ready(guc)) {
>> - guc->interrupts.enabled = false;
>> + atomic_set(&guc->interrupts.enabled, false);
>> +
>> return;
>> }
>> @@ -687,7 +688,7 @@ void intel_uc_suspend(struct intel_uc *uc)
>> wake_up_all_tlb_invalidate(guc);
>> if (!intel_guc_is_ready(guc)) {
>> - guc->interrupts.enabled = false;
>> + atomic_set(&guc->interrupts.enabled, false);
>> return;
>> }
>
More information about the Intel-gfx
mailing list