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

[Lucas De Marchi]

On Tue, Apr 01, 2025 at 01:55:39PM +0200, Thomas Hellström wrote:
>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.

no, I don't. If we are documenting this, I wouldn't trust me to write it
:).

thanks
Lucas De Marchi