[PATCH v3] drm/xe: add system memory page iterator support to xe_res_cursor
Hajda, Andrzej
andrzej.hajda at intel.com
Mon Oct 14 08:18:13 UTC 2024
Hi Matt,
W dniu 12.10.2024 o 04:23, Matthew Brost pisze:
> 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 at intel.com>
>> Reviewed-by: Jonathan Cavitt <jonathan.cavitt at 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?
IIUC sg tables after mapping operation contains mappings between list of
pages (represented by pair of struct scatterlist fields: page_link and
length) and list of dma addresses (represented by: dma_address and
dma_length), both lists are 'hidden' in sg tables.
Currently xe_res_cursor can iterate only over dma addresses, ie after
every call to xe_res_next xe_res_cursor.sgl points to scatterlist
corresponding to dma_address at requested position.
But for eudebug we need xe_res_cursor.sgl pointing to scatterlist with
page_link corresponding to requested position, ie we need to iterate
'left side' side of the mapping. The only change to achieve this is to
track .length field instead of .dma_length.
I hope my understanding of this is correct, at least tests are positive :)
Userptr implementation in v2 does not work with iommu, and does not use
this patch, and needs to be fixed. My intention was to get early
feedback on this approach and merge it separately, or alternatively post
reviewed together with eudebug v3 patchset and with userptr_vma patch
[1] updated to use introduced xe_res_first_sg_system:
@@ -3662,7 +3662,7 @@ int xe_uvma_access(struct xe_userptr_vma *uvma,
u64 offset,
goto out_unlock_notifier;
}
- for (xe_res_first_sg(up->sg, offset, len, &cur); cur.remaining;
+ for (xe_res_first_sg_system(up->sg, offset, len, &cur);
cur.remaining;
xe_res_next(&cur, cur_len)) {
void *ptr = kmap_local_page(sg_page(cur.sgl)) + cur.start;
OK, also s/xe_uvma_access/xe_vm_userptr_access/, but this is different
story.
Regards
Andrzej
>
> [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 at intel.com>
>>
More information about the Intel-xe
mailing list