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

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Feb 09, 2026 at 10:09:11AM +0100, Thomas Hellström wrote:
> 
> 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?
> 

You are not blind, that call is missing. This is a iGPU feature only, so
it isn't needed but for completeness / future proofing that call should
likely be added in.

> 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.

+1. Let's do a scoped guard for locking clarity.

Matt

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