Re: [PATCH v2] drm/i915/guc: Add synchronization on interrupt enable flag

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

On 2025-08-25 6:00 a.m., Andi Shyti wrote:
> Hi Zhanjun,
> 
>> 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..48148cb6cba0 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -100,8 +100,9 @@ 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);
>> -
>> -	guc->interrupts.enabled = true;
>> +	atomic_set(&guc->interrupts.enabled, true);
>> +	/* make sure interrupt handler will see changes */
>> +	smp_mb();
> 
> Are we sure we need the barriers here? Can you please explain
> better what you are trying to achieve?
> 
> My idea of barriers was to use in order to avoid converting
> everything into atomic, which doesn't necessarily mean that it's
> the best solution, it was just a thought.
> 
> But maybe I misunderstood your intention.
> 
> Andi

The barriers seems not needed, the synchronization issue is for 
interrupt.enable only, no extra memory barrier is needed, to be removed 
in next rev.


Regards,
Zhanjun Dong