Re: [PATCH v2 2/3] drm/xe/sa: Add lockdep annotations for SA manager swap_guard

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi,

On Fri, 2026-02-06 at 10:28 -0800, Matthew Brost wrote:
> On Fri, Feb 06, 2026 at 05:17:46PM +0100, Thomas Hellström wrote:
> > Hi
> > 
> > On Wed, 2026-02-04 at 16:46 +0000, Satyanarayana K V P wrote:
> > > Annotate the SA manager init path to model taking swap_guard
> > > while
> > > under
> > > reclaim context. This helps lockdep catch potential circular
> > > dependencies
> > > between fs_reclaim and swap_guard in debug builds.
> > > 
> > > Signed-off-by: Satyanarayana K V P
> > > <satyanarayana.k.v.p@intel.com>
> > > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > 
> > > ---
> > > V1 -> V2:
> > > - None.
> > > ---
> > >  drivers/gpu/drm/xe/xe_sa.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_sa.c
> > > b/drivers/gpu/drm/xe/xe_sa.c
> > > index b738102575d4..5efbb5a09f77 100644
> > > --- a/drivers/gpu/drm/xe/xe_sa.c
> > > +++ b/drivers/gpu/drm/xe/xe_sa.c
> > > @@ -89,6 +89,12 @@ struct xe_sa_manager
> > > *__xe_sa_bo_manager_init(struct xe_tile *tile, u32 size,
> > >   if (ret)
> > >   return ERR_PTR(ret);
> > >  
> > > + if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
> > > + fs_reclaim_acquire(GFP_KERNEL);
> > > + might_lock(&sa_manager->swap_guard);
> > > + fs_reclaim_release(GFP_KERNEL);
> > > + }
> > > +
> > >   shadow = xe_managed_bo_create_pin_map(xe, tile, size,
> > >         XE_BO_FLAG_VRAM_IF_DGFX(tile) |
> > >         XE_BO_FLAG_GGTT |
> > 
> > Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > 
> > In addition to this, a couple of comments to the code that is
> > already
> > in the driver:
> > 
> > It would be beneficial for understanding if a document section was
> > added for the typical usage flow of the shadow buffer, something
> > like
> > the below (hope I got this correct).
> > 
> > *) Clearly stating the use-case: That the whole buffer is bound to
> > HW
> > and can execute at any time. The shadow buffer is part of a double
> > buffering scheme so that updates are visible to the hardware
> > atomically.
> > 
> > *) The flow:
> > -lock()
> > -Swap buffers: The buffers are identical. The buffer bound to
> > hardware
> > becomes the shadow.
> > -Update the primary buffer.
> > -Flush the cpu buffer to primary on DGFX (BTW IIRC this was missing
> > in
> > the code). -Point the HW to the primary buffer.
> > -sync the shadow to the primary.
> > -unlock()
> > 
> 
> This is is roughly correct. I agree a kernel doc section would be
> good.
> 
> > In addition perhaps more lockdep asserts an also perhaps pin the
> > lock
> 
> More lockdep is also good. What functions do you think are missing
> asserts?
> 
> These are two possible ones I came up with:
> 
> xe_sriov_vf_ccs_rw_update_bb_addr


> xe_bb_ccs_new

These are possible candidates, But I was mostly thinking of the xe_sa
functions involved in the above flow. So that would essentially be
xe_sa_bo_flush_write() missing asserts.


> 
> > in swap buffers and unpin in sync to shadow so that if anybody
> > releases
> 
> Pin/unpin isn’t a bad idea. We could also have xe_sa_bo_swap_shadow /
> xe_sa_bo_sync_shadow acquire and release the swap lock, since that
> clearly
> defines the critical section for this lock - what do think?
> 
> > the lock in between you'd get a warning.
> > 
> > But this can be done as a follow-up, (beware the possibly missing
> > cpu
> 
> There are barriers in xe_sriov_vf_ccs_rw_update_bb_addr, do you think
> anything else is missing?

Yes, on DGFX we maintain a system memory buffer to avoid read
operations over PCIe. So writes to the sa memory doesn't immediately
appear in primary gpu memory. It needs a xe_sa_bo_flush_write(). I
might have missed it, but I don't see it in the code?

Also as an unrelated item, I think we should use scoped_guard() instead
of guard() for xe_bo_swap_guard() to clearly mark the region where the
lock is strictly needed? Would make it easier for a new developer
reading the code or somebody updating the code at the end of the
function to not add unnecessary stuff in the critical section.

Thanks,
Thomas

> 
> Matt
> 
> > buffer flush, though). I think it's worth spending some time on
> > this.
> > 
> > Thanks,
> > Thomas
> >    A.