Re: [Intel-xe] [PATCH] drm/xe: Fix lockdep warning in xe_force_wake calls

[Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>]

On 10/11/23 18:47, Matthew Brost wrote:
> On Fri, Nov 10, 2023 at 11:59:07AM +0530, Aravind Iddamsetty wrote:
>> Use spin_lock_irqsave, spin_unlock_irqrestore
>>
>> Fix for below:
>> [13994.811263] ========================================================
>> [13994.811295] WARNING: possible irq lock inversion dependency detected
>> [13994.811326] 6.6.0-rc3-xe #2 Tainted: G     U
>> [13994.811358] --------------------------------------------------------
>> [13994.811388] swapper/0/0 just changed the state of lock:
>> [13994.811416] ffff895c7e044db8 (&cpuctx_lock){-...}-{2:2}, at:
>> __perf_event_read+0xb7/0x3a0
>> [13994.811494] but this lock took another, HARDIRQ-unsafe lock in the
>> past:
>> [13994.811528]  (&fw->lock){+.+.}-{2:2}
>> [13994.811544]
>>
>>                and interrupts could create inverse lock ordering between
>> them.
>>
>> [13994.811606]
>>                other info that might help us debug this:
>> [13994.811636]  Possible interrupt unsafe locking scenario:
>>
>> [13994.811667]        CPU0                    CPU1
>> [13994.811691]        ----                    ----
>> [13994.811715]   lock(&fw->lock);
>> [13994.811744]                                local_irq_disable();
>> [13994.811773]                                lock(&cpuctx_lock);
>> [13994.811810]                                lock(&fw->lock);
>> [13994.811846]   <Interrupt>
>> [13994.811865]     lock(&cpuctx_lock);
>> [13994.811895]
>>                 *** DEADLOCK ***
>>
Hi Matt,

Firstly thanks for your comment and sorry for the late response was OOO on sick the entire week.

> I don't personally like this. Where in an irq context do we grab this
> lock? FW probably shouldn't ever be grabbed from an irq context. I see
> this was changed from a mutex to spin lock in which is pretty suspect
> IMO.
with the PMU interface "drm/xe/pmu: Enable PMU interface" we are exposing engine busyness
counters and the registers fall under GT domain and needed forcewake as PMU is atomic context
so had to change the forcewake to use spinlock. But other than this i do not think forcewake
is being called from any irq context, AFAIU lockdep is predicting a scenario and hence the
warning. which subsided by use of spin_lock_irqsave.


>
> 'drm/xe: Use spinlock in forcewake instead of mutex'  
>
> If this really needs to be a spin lock I'd rather have versions of
> xe_force_wake_get/put that called from non-atomic contexts (e.g. use
> spin_lock_irq) and atomic contexts (e.g. use spin_lock) rather than
> using spin_lock_irqsave.

I somehow doubt if lockdep be able to recognize such a use and not warn again.

Thanks,
Aravind.
>
> Matt
>
>> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_force_wake.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>> index 32d6c4dd2807..9bbe8a5040da 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -145,9 +145,10 @@ int xe_force_wake_get(struct xe_force_wake *fw,
>>  	struct xe_gt *gt = fw_to_gt(fw);
>>  	struct xe_force_wake_domain *domain;
>>  	enum xe_force_wake_domains tmp, woken = 0;
>> +	unsigned long flags;
>>  	int ret, ret2 = 0;
>>  
>> -	spin_lock(&fw->lock);
>> +	spin_lock_irqsave(&fw->lock, flags);
>>  	for_each_fw_domain_masked(domain, domains, fw, tmp) {
>>  		if (!domain->ref++) {
>>  			woken |= BIT(domain->id);
>> @@ -162,7 +163,7 @@ int xe_force_wake_get(struct xe_force_wake *fw,
>>  				   domain->id, ret);
>>  	}
>>  	fw->awake_domains |= woken;
>> -	spin_unlock(&fw->lock);
>> +	spin_unlock_irqrestore(&fw->lock, flags);
>>  
>>  	return ret2;
>>  }
>> @@ -174,9 +175,10 @@ int xe_force_wake_put(struct xe_force_wake *fw,
>>  	struct xe_gt *gt = fw_to_gt(fw);
>>  	struct xe_force_wake_domain *domain;
>>  	enum xe_force_wake_domains tmp, sleep = 0;
>> +	unsigned long flags;
>>  	int ret, ret2 = 0;
>>  
>> -	spin_lock(&fw->lock);
>> +	spin_lock_irqsave(&fw->lock, flags);
>>  	for_each_fw_domain_masked(domain, domains, fw, tmp) {
>>  		if (!--domain->ref) {
>>  			sleep |= BIT(domain->id);
>> @@ -191,7 +193,7 @@ int xe_force_wake_put(struct xe_force_wake *fw,
>>  				   domain->id, ret);
>>  	}
>>  	fw->awake_domains &= ~sleep;
>> -	spin_unlock(&fw->lock);
>> +	spin_unlock_irqrestore(&fw->lock, flags);
>>  
>>  	return ret2;
>>  }
>> -- 
>> 2.25.1
>>