Re: [PATCH v4 5/5] drm/xe: Make the PT code handle placement per PTE rather than per vma / range

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Lucas, Thanks for noticing. See inline.


On Mon, 2025-03-31 at 10:55 -0500, Lucas De Marchi wrote:
> On Wed, Mar 26, 2025 at 09:05:51AM +0100, Thomas Hellström wrote:
> > diff --git a/drivers/gpu/drm/xe/xe_pt.c
> > b/drivers/gpu/drm/xe/xe_pt.c
> > index 9e719535a3bb..82ae159feed1 100644
> > --- a/drivers/gpu/drm/xe/xe_pt.c
> > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > @@ -278,13 +278,15 @@ struct xe_pt_stage_bind_walk {
> > 	struct xe_vm *vm;
> > 	/** @tile: The tile we're building for. */
> > 	struct xe_tile *tile;
> > -	/** @default_pte: PTE flag only template. No address is
> > associated */
> > -	u64 default_pte;
> > +	/** @default_pte: PTE flag only template for VRAM. No
> > address is associated */
> 
> 		     ^
> > +	u64 default_vram_pte;
> 
> 		    ^
> 
> doc is wrong here. This would fix that for this patch:
> 
> // diff --git a/drivers/gpu/drm/xe/xe_pt.c
> b/drivers/gpu/drm/xe/xe_pt.c
> // index 82ae159feed12..91ad347c8c7bf 100644
> // --- a/drivers/gpu/drm/xe/xe_pt.c
> // +++ b/drivers/gpu/drm/xe/xe_pt.c
> // @@ -278,9 +278,9 @@ struct xe_pt_stage_bind_walk {
> //         struct xe_vm *vm;
> //         /** @tile: The tile we're building for. */
> //         struct xe_tile *tile;
> // -       /** @default_pte: PTE flag only template for VRAM. No
> address is associated */
> // +       /** @default_vram_pte: PTE flag only template for VRAM. No
> address is associated */
> //         u64 default_vram_pte;
> // -       /** @default_pte: PTE flag only template for VRAM. No
> address is associated */
> // +       /** @default_system_pte: PTE flag only template for
> system. No address is associated */
> //         u64 default_system_pte;
> //         /** @dma_offset: DMA offset to add to the PTE. */
> //         u64 dma_offset;
> 
> 
> However I was surprised that we didn't get any error in our CI.Hooks.
> 
> 
> It looks like this entire struct is not in our docs at all:
> https://docs.kernel.org/gpu/xe/xe_mm.html#pagetable-building
> 
> Should we add a
> 
> /**
>   * struct xe_pt_stage_bind_walk - ...
>   */
> 
> so it's visible? Or if it's internal detail from
> drivers/gpu/drm/xe/xe_pt.c maybe we could drop the kernel-doc,
> leaving
> them as "/*" comments instead?

I think it's worth having it kerneldoc'ed. I'll submit a patch for that
+ the fixes above unless you have it already in the pipe.

Thanks,
Thomas


> 
> Lucas De Marchi