Re: [PATCH v3] drm/xe: add system memory page iterator support to xe_res_cursor

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Oct 11, 2024 at 09:26:30AM +0200, Andrzej Hajda wrote:
> Currently xe_res_cursor allows iteration only over DMA side of sg tables.
> Adding possibility to iterate over pages allows the use of cursor in more
> contexts.
> 
> v2: fixed wording in commit message (Jonathan)
> v3: indentation and author fixes (checkpatch)
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> ---
> - Link to v1: https://lore.kernel.org/r/20241009-xe_res_cursor_add_page_iterator-v1-1-c883446e5770@intel.com
> - Link to v2: https://lore.kernel.org/r/20241011-xe_res_cursor_add_page_iterator-v2-1-367b74c1cc29@intel.com
> ---
> Hi all,
> 
> This patch is required to proper implementation of userptr_vma access
> in configurations with iommu turned on[1]. Required by upcoming eudebug
> feature.
> 
> [1]: https://lore.kernel.org/intel-xe/20241001144306.1991001-13-mika.kuoppala@linux.intel.com/
>

I'm failing to see why this patch is required for [1].

I don't see this patch included in [2].

Both justifications here are pretty vague - 'use of cursor in more
contexts' or 'Required by upcoming eudebug feature'. Not understanding
the why. Can you please elborate?

[2] https://patchwork.freedesktop.org/series/136572/

> Regards
> Andrzej
> ---
>  drivers/gpu/drm/xe/xe_res_cursor.h | 51 +++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_res_cursor.h b/drivers/gpu/drm/xe/xe_res_cursor.h
> index dca374b6521c..610407f23dbd 100644
> --- a/drivers/gpu/drm/xe/xe_res_cursor.h
> +++ b/drivers/gpu/drm/xe/xe_res_cursor.h
> @@ -129,18 +129,35 @@ static inline void __xe_res_sg_next(struct xe_res_cursor *cur)
>  {
>  	struct scatterlist *sgl = cur->sgl;
>  	u64 start = cur->start;
> +	unsigned int len;
>  
> -	while (start >= sg_dma_len(sgl)) {
> -		start -= sg_dma_len(sgl);
> +	while (true) {
> +		len = (cur->mem_type == XE_PL_SYSTEM) ? sgl->length : sg_dma_len(sgl);

This doesn't look right. sg_dma_len should always be sufficient unless
I'm missing something. Can you explain why if 'cur->mem_type ==
XE_PL_SYSTEM' you need to directly look at sgl->length rather than using
sg_dma_len(sgl) helper?

> +		if (start < len)
> +			break;
> +		start -= len;
>  		sgl = sg_next(sgl);
>  		XE_WARN_ON(!sgl);
>  	}
> -
>  	cur->start = start;
> -	cur->size = sg_dma_len(sgl) - start;
> +	cur->size = len - start;
>  	cur->sgl = sgl;
>  }
>  
> +static inline void __xe_res_first_sg(const struct sg_table *sg,
> +				     u64 start, u64 size,
> +				     struct xe_res_cursor *cur, u32 mem_type)
> +{
> +	XE_WARN_ON(!sg);
> +	cur->node = NULL;
> +	cur->start = start;
> +	cur->remaining = size;
> +	cur->size = 0;
> +	cur->sgl = sg->sgl;
> +	cur->mem_type = mem_type;
> +	__xe_res_sg_next(cur);
> +}
> +
>  /**
>   * xe_res_first_sg - initialize a xe_res_cursor with a scatter gather table
>   *
> @@ -155,14 +172,24 @@ static inline void xe_res_first_sg(const struct sg_table *sg,
>  				   u64 start, u64 size,
>  				   struct xe_res_cursor *cur)
>  {
> -	XE_WARN_ON(!sg);
> -	cur->node = NULL;
> -	cur->start = start;
> -	cur->remaining = size;
> -	cur->size = 0;
> -	cur->sgl = sg->sgl;
> -	cur->mem_type = XE_PL_TT;
> -	__xe_res_sg_next(cur);
> +	__xe_res_first_sg(sg, start, size, cur, XE_PL_TT);
> +}
> +
> +/**
> + * xe_res_first_sg_system - initialize a xe_res_cursor for iterate system memory pages
> + *
> + * @sg: scatter gather table to walk
> + * @start: Start of the range
> + * @size: Size of the range
> + * @cur: cursor object to initialize
> + *
> + * Start walking over the range of allocations between @start and @size
> + */
> +static inline void xe_res_first_sg_system(const struct sg_table *sg,
> +					  u64 start, u64 size,
> +					  struct xe_res_cursor *cur)
> +{
> +	__xe_res_first_sg(sg, start, size, cur, XE_PL_SYSTEM);

Not seeing where this function is used in [2].

Matt

>  }
>  
>  /**
> 
> ---
> base-commit: f1561e6c62b5b5c3fe0276f2fbe7325e0d7c262d
> change-id: 20241009-xe_res_cursor_add_page_iterator-d29d73c3eb99
> 
> Best regards,
> -- 
> Andrzej Hajda <andrzej.hajda@intel.com>
>