Re: [PATCH 1/2] drm/xe: Skip over non leaf pte for PRL generation

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

On Mon, Feb 23, 2026 at 05:45:23PM -0800, Matthew Brost wrote:
> On Mon, Feb 23, 2026 at 04:33:30PM -0700, Nguyen, Brian3 wrote:
> > On Monday, February 23, 2026 3:07 PM, Stuart Summers wrote:
> > > On Mon, 2026-02-23 at 14:59 -0800, Matthew Brost wrote:
> > > > On Mon, Feb 23, 2026 at 10:49:21PM +0000, Summers, Stuart wrote:
> > > > > On Thu, 2026-01-29 at 08:27 +0000, Brian Nguyen wrote:
> > > > > > The check using xe_child->base.children was insufficient in
> > > > > > determining if a pte was a leaf node. So explicitly check for if a
> > > > > > pte is a leaf through the bit checks.
> > > > > >
> > > > > > Fixes: b912138df299 ("drm/xe: Create page reclaim list on
> > > > > > unbind")
> > > >
> > > > Move the Fixes tag by other tags (Signed-off-by, Cc)
> > > >
> > 
> > Got it, will move.
> > 
> > > > > >
> > > > > > v2:
> > > > > >  - Remove old assert. (Matt R)
> > > > > >
> > > > > > Signed-off-by: Brian Nguyen <brian3.nguyen@intel.com>
> > > > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/xe/xe_pt.c | 13 ++++++++-----
> > > > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_pt.c
> > > > > > b/drivers/gpu/drm/xe/xe_pt.c index 6703a7049227..b73a356d0fa1
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_pt.c
> > > > > > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > > > > > @@ -1655,7 +1655,7 @@ static int xe_pt_stage_unbind_entry(struct
> > > > > > xe_ptw *parent, pgoff_t offset,
> > > > > >
> > > > > >         XE_WARN_ON(!*child);
> > > > > >         XE_WARN_ON(!level);
> > > > > > -       /* Check for leaf node */
> > > > > > +       /* Optimistically check for leaf node, may not be
> > > > > > guaranteed
> > > > >
> > > > > I would keep the comments the same - we're still trying to check a
> > > > > leaf node here and we aren't really doing anything special. If we
> > > > > have questions, we can look at the commit history to determine what
> > > > > changed from your prior implementation.
> > > > >
> > > > > Or if you want documentation here, it's more interesting to me
> > > > > *why* we
> > > > > can't see this from the child alone than just the fact that we can't
> > > > > (which we can observe by the if condition).
> > > > >
> > > > > Also applies to the comment below too.
> > > > >
> > 
> > Ahh got it. Let me reword the comments here with more details depending on the changes
> > we make in the comments below. 
> > 
> > > > > Thanks,
> > > > > Stuart
> > > > >
> > > > > > from children alone */
> > > > > >         if (xe_walk->prl && xe_page_reclaim_list_valid(xe_walk-
> > > > > > >prl)
> > > > > > &&
> > > > > >             (!xe_child->base.children || !xe_child-
> > > > > > > base.children[first])) {
> > > > > >                 struct iosys_map *leaf_map = &xe_child->bo->vmap;
> > > > > > @@ -1675,10 +1675,13 @@ static int xe_pt_stage_unbind_entry(struct
> > > > > > xe_ptw *parent, pgoff_t offset,
> > > > > >                                 break;
> > > > > >                         }
> > > > > >
> > > > > > -                       /* Ensure it is a defined page */
> > > > > > -                       xe_tile_assert(xe_walk->tile,
> > > > > > -                                      xe_child->level == 0 ||
> > > > > > -                                      (pte & (XE_PTE_PS64 |
> > > > > > XE_PDE_PS_2M | XE_PDPE_PS_1G)));
> > > > > > +                       /*
> > > > > > +                        * The check for xe_pt's children is
> > > > > > insufficient to determine leaf.
> > > > > > +                        * If not leaf, break out and continue in
> > > > > > next page walk level.
> > > > > > +                        */
> > > > > > +                       if (xe_child->level > 0 &&
> > > > > > +                           !(pte & (XE_PTE_PS64 | XE_PDE_PS_2M |
> > > > > > XE_PDPE_PS_1G)))
> > > >
> > > > I don't think XE_PTE_PS64 needs to be checked here as that should only
> > > > be set at level 0.
> > > 
> > > Oh that's a good catch. PS64 has a specific meaning within the PDE. Are we trying to check if this is > 4K basically?
> > > 
> > 
> > This was checking if one of the bits indicating that this is a leaf entry was raised and if not, skip adding this.
> > So, checking for if it’s a leaf PTE (which would have one of those bits raised), not necessarily if it is just > 4K.
> > 
> > Previously I had thought to remove XE_PTE_PS64 since level == 0 covers the condition but just for clarity of
> > checking all leaf PTE, I kept it with the assert and then kept it since I inverted condition in this patch.
> > I'll remove XE_PTE_PS64 in next patch. Thanks.
> > 
> > > Thanks,
> > > Stuart
> > > 
> > > >
> > > > I agree we xe_child->base.children can be set at level > 0 but now I'm
> > > > thinking the outer if statement is wrong wrt to
> > > > 'xe_child->base.children[first]'. Couldn't
> > > > xe_child->base.children[first] be NULL when subsequent
> > > > xe_child->base.children[first + 1] be non-NULL?
> > > >
> > 
> > I believe this was the issue I was running into with one of the test cases. Having some sort of non-NULL subsequent child declared here
> 
> Yes, but any check at outer level of individual children doesn't seem
> right.
> 
> Let's give an example of a 6M unbind.
> 
> Case 1:
> - children[0] == NULL (leaf)
> - children[1] == Valid (not a leaf)
> - children[2] == NULL (leaf)
> 
> I believe you correct code correctly generate PRL for children[0], break
> on children[1], so children[3] is skipped. I don't think this is
> correct... On hitting children[1], I believe in addition to breaking the
> loop, you the PRL needs to be invalidated, right?
> 
> Case 2:
> - children[0] == Valid (not a leaf)
> - children[1] == NULL (leaf)
> - children[2] == NULL (leaf)
> 
> The entire PRL generation is skipped but the PRL is not invalidated
> which seems to be problem as leafs are skipped here.
> 
> So I believe this entire logic needs a bit more rework than this patch
> alone.
> 
> I believe the logic is roughly - at level 1 / 0 always walk all the
> PTEs in the PRL is valid, at level 1 if any non-2M pages are found
> invalidate the PRL if we are not going to desend further down (i.e., we
> cover the entire 2M range).
> 
> This fairly complicated, so I'd have to type this out + get on test
> machine to provide further advise here.
> 

I can give you some test cases I suppose...

- Test case A - 2M binds:

BO[0] = alloc(8M);
bind(BO[0], aligned VA, 8M);
unbind(aligned VA, 8M);	/* Get four 2M PRL */

- Test base B - 4k binds - Same as above but 4k

- Test case C - 2M mixed:

BO[0] = alloc(2M);
BO[1] = alloc(4K);
BO[2] = alloc(2M);
bind(BO[0], aligned VA, 2M);
bind(BO[1], aligned VA + 2M, 4K);
bind(BO[2], aligned VA + 4M, 2M);
unbind(aligned VA, 6M);	/* PRL should be invalid */

- Test case C - 2M mixed:

BO[0] = alloc(4K);
BO[1] = alloc(2M);
BO[2] = alloc(2M);
bind(BO[0], aligned VA, 4k);
bind(BO[1], aligned VA + 2M, 2M);
bind(BO[2], aligned VA + 4M, 2M);	/* PRL should be invalid */
unbind(aligned VA, 6M);	/* PRL should be invalid */

- test case D - 2M, partial:

BO[0] = alloc(2M);
BO[1] = alloc(4k);
bind(BO[0], aligned VA, 2M);
bind(BO[1], aligned VA + 2M, 4k);
unbind(aligned VA, 2M + 4k);	/* Get one 2M PRL, one 4K PRL */

- test case E - 2M, partial:

BO[0] = alloc(4K);
BO[1] = alloc(2M);
bind(BO[0], aligned VA - 4K, 4K);
bind(BO[1], aligned VA, 2M);
unbind(aligned VA - 4K, 2M + 4K);	/* Get one 2M PRL, one 4K PRL */

- test case F - 1 GB, partial:

BO[0] = alloc(1G);
BO[1] = alloc(2M);
bind(BO[0], aligned VA, 1G);
bind(BO[1], aligned VA + 1G, 2M);
unbind(aligned VA, 1G + 2M); /* PRL should be invalid */

Matt

> Matt
> 
> > with the xe_child->base.children[first] = NULL. But determining if this PTE is a leaf entry would require us to iterate through all the childrens
> > I believe, which is why I had kept that check (xe_child->base.children[first] != NULL) as sort of an optimistic filter and had this new inner if statement
> > to ensure that these entries are leaf entries.
> > 
> > So, what is your suggestion here?
> > 
> > Thanks,
> > Brian
> > 
> > > > Matt
> > > >
> > > > > > +                               break;
> > > > > >
> > > > > >                         /* An entry should be added for 64KB but
> > > > > > contigious 4K have XE_PTE_PS64 */
> > > > > >                         if (pte & XE_PTE_PS64)
> > > > >
> >