[PATCH v3] drm/xe: add system memory page iterator support to xe_res_cursor
Hajda, Andrzej
andrzej.hajda at intel.com
Tue Oct 15 10:48:58 UTC 2024
W dniu 15.10.2024 o 06:58, Matthew Brost pisze:
> On Mon, Oct 14, 2024 at 10:18:13AM +0200, Hajda, Andrzej wrote:
>> 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:
> I'm getting a little lost reading this but I think I'd have to see the
> complete picture to know if this correct - it doesn't seem to be, so
> maybe hold off on merging for now.
Sorry if my explanation is not clear, the patches are on Miku eudebug
public branch[1], it should make at least clear how to use improved
iterator.
Ie this patch is located at [2] and the patch implementing userptr_vma
is at [3].
Of course I can post these patches here if you prefer.
[1]: https://gitlab.freedesktop.org/miku/kernel/-/commits/eudebug-dev
[2]:
https://gitlab.freedesktop.org/miku/kernel/-/commit/c0cd313ec50ba224d6a17149fbd13a85a9d3e906
[3]:
https://gitlab.freedesktop.org/miku/kernel/-/commit/834eb9f9b4ab3fd6ff9e8f2eaa6beb507b578ff1
>
> Also if you see my comments here [2], in general I think the userptr
> implementation needs to be tweaked to operate on pages rather than a sg
> list.
I agree, we should operate on pages, but my point is that we do not need
to tweak implementation, we have access to
these pages via scatterlist.page_link.
So this patch answers your concerns in [2]. With this patch we can
operate on pages, this way:
int cur_len; 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; ||cur_len = min(cur.size, cur.remaining); // do whatever with page mapped
via ptr |}
So the key question is if my assumption on scatterlist.page_link is
correct? If yes then the patch looks correct.
I cannot find documentation proving it but for example presence and
usage of core helpers for_each_sg_page vs for_each_sg_dma_page suggests
it could be true.
Regards
Andrzej
>
> Matt
>
> [2] https://patchwork.freedesktop.org/patch/617481/?series=136572&rev=2
>
>> @@ -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