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

[Matthew Brost]

On Fri, Nov 17, 2023 at 02:18:01PM +0530, Aravind Iddamsetty wrote:
> 
> 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.
> 

That is exactly what these calls are for:

spin_lock_irqsave -> don't know if in atomic or non-atomic context
spin_lock -> in a atomic context (irqs are already disabled)
spin_lock_irq -> in a non-atomic context (irqs are enabled)

Generally we should attempt to use spin_lock/spin_lock_irq if possible
as knowing if we are in an atomic / non-atomic code path is a good
practice.

Matt

> Thanks,
> Aravind.
> >
> > Matt
> >
> >> Cc: Anshuman Gupta <anshuman.gupta at intel.com>
> >> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> >> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty at 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
> >>