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

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