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

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

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?

Matt

> > +                               break;
> >  
> >                         /* An entry should be added for 64KB but
> > contigious 4K have XE_PTE_PS64 */
> >                         if (pte & XE_PTE_PS64)
>