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

["Summers, Stuart" <stuart.summers@intel.com>]

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

Oh that's a good catch. PS64 has a specific meaning within the PDE. Are
we trying to check if this is > 4K basically?

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?
> 
> Matt
> 
> > > +                               break;
> > >  
> > >                         /* An entry should be added for 64KB but
> > > contigious 4K have XE_PTE_PS64 */
> > >                         if (pte & XE_PTE_PS64)
> >