[PATCH v3] drm/xe: add system memory page iterator support to xe_res_cursor
Matthew Brost
matthew.brost at intel.com
Tue Oct 15 04:58:37 UTC 2024
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.
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.
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