[Intel-xe] [PATCH] drm/xe: Apply upper limit to sg element size
Niranjana Vishwanathapura
niranjana.vishwanathapura at intel.com
Thu May 25 05:55:45 UTC 2023
On Wed, May 24, 2023 at 01:16:45AM -0700, Hellstrom, Thomas wrote:
>On Wed, 2023-05-17 at 22:32 -0700, Niranjana Vishwanathapura wrote:
>> On Wed, May 17, 2023 at 08:01:08AM +0200, Thomas Hellström wrote:
>> > Hi, Niranjana
>> >
>> > On 5/16/23 20:44, Niranjana Vishwanathapura wrote:
>> > > Specify maximum segment size for sg elements by using
>> > > sg_alloc_table_from_pages_segment() to allocate sg_table.
>> > >
>> > > Signed-off-by: Niranjana Vishwanathapura
>> > > <niranjana.vishwanathapura at intel.com>
>> > > ---
>> > > drivers/gpu/drm/xe/xe_bo.c | 8 +++++---
>> > > drivers/gpu/drm/xe/xe_bo.h | 21 +++++++++++++++++++++
>> > > drivers/gpu/drm/xe/xe_vm.c | 8 +++++---
>> > > 3 files changed, 31 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/xe/xe_bo.c
>> > > b/drivers/gpu/drm/xe/xe_bo.c
>> > > index c82e995df779..21c5aca424dd 100644
>> > > --- a/drivers/gpu/drm/xe/xe_bo.c
>> > > +++ b/drivers/gpu/drm/xe/xe_bo.c
>> > > @@ -251,9 +251,11 @@ static int xe_tt_map_sg(struct ttm_tt *tt)
>> > > if (xe_tt->sg)
>> > > return 0;
>> > > - ret = sg_alloc_table_from_pages(&xe_tt->sgt, tt->pages,
>> > > num_pages,
>> > > - 0, (u64)num_pages <<
>> > > PAGE_SHIFT,
>> > > - GFP_KERNEL);
>> > > + ret = sg_alloc_table_from_pages_segment(&xe_tt->sgt, tt-
>> > > >pages,
>> > > + num_pages, 0,
>> > > + (u64)num_pages <<
>> > > PAGE_SHIFT,
>> > > + xe_sg_segment_siz
>> > > e(xe_tt->dev),
>> > > + GFP_KERNEL);
>> > > if (ret)
>> > > return ret;
>> > > diff --git a/drivers/gpu/drm/xe/xe_bo.h
>> > > b/drivers/gpu/drm/xe/xe_bo.h
>> > > index 7e111332c35a..a1c51cc0ac3c 100644
>> > > --- a/drivers/gpu/drm/xe/xe_bo.h
>> > > +++ b/drivers/gpu/drm/xe/xe_bo.h
>> > > @@ -296,6 +296,27 @@ void xe_bo_put_commit(struct llist_head
>> > > *deferred);
>> > > struct sg_table *xe_bo_get_sg(struct xe_bo *bo);
>> > > +/*
>> > > + * xe_sg_segment_size() - Provides upper limit for sg segment
>> > > size.
>> > > + * @dev: device pointer
>> > > + *
>> > > + * Returns the maximum segment size for the 'struct scatterlist'
>> > > + * elements.
>> > > + */
>> > > +static inline unsigned int xe_sg_segment_size(struct device
>> > > *dev)
>> > > +{
>> > > + size_t max = min_t(size_t, UINT_MAX,
>> > > dma_max_mapping_size(dev));
>> > > +
>> > > + /*
>> > > + * The iommu_dma_map_sg() function ensures iova
>> > > allocation doesn't
>> > > + * cross dma segment boundary. It does so by padding some
>> > > sg elements.
>> > > + * This can cause overflow, ending up with sg->length
>> > > being set to 0.
>> > > + * Avoid this by ensuring maximum segment size is half of
>> > > 'max'
>> > > + * rounded down to PAGE_SIZE.
>> > > + */
>> >
>> > Is this a bug in the IOMMU code? In any case, shouldn't the fix on
>> > our
>> > side be using
>> >
>> > dma_set_seg_boundary() and
>> > dma_set_max_seg_size()
>> >
>> > to set reasonable values that avoid that problem?
>> >
>>
>> Thanks Thomas.
>> Hmm...I am not fully familiar with the iommu code here, but letting
>> sg->length overflow may be an oversight?
>>
>> Looks like dma_set_max_seg_size() doesn't enforce upper limit to
>> sg segment size while sg_table is created. We need to use
>> sg_alloc_table_from_pages_segment() to specify that. It seems
>> dma_set_max_seg_size() is used to specify the maximum segment size
>> the created sg_table has while doing the dma mapping. The iommu code
>> doesn't even complain when it runs into an sg element with size more
>> that the one set through dma_set_max_seg_size() during mapping.
>> So, driver must ensure that. I think we would still need this patch
>> and update it to call dma_set_max_seg_size() also.
>
>OK, I must admit I don't fully understand the relationship between
>dma_set_max_seg_size() but I assumed it needed adjustment if we change
>the segment boundary, like below.
>
>>
>> Yah, looks like we can call dma_set_seg_boundary() to set a very
>> large segment boundary say (0xffffffffffff) such that the maximum
>> sized BO fits into it (ie., we never run into any BO crossing the
>> segment boundary). But I am not sure if that is the right approach.
>> I am not seeing many drivers calling dma_set_seg_boundary() (pci
>> probe sets it to default 0xffffffff). Our GPU may not enforce any
>> dma segment boundary, but not sure somewhere else there is one.
>
>Hm. I was thinking to set it to (0xffffffff >> 1), assuming that the
>iommu padding would then never overflow? Or maybe this wasn't what
>caused the overflow in the first place.
>
You mean setting max_segment to (0xffffffff >> 1)? (because setting
segment boundary to (0xffffffff >> 1) which is less than max_segment
doesn't solve the problem).
And this patch is about setting max_segment to (0xffffffff >> 1)
rounded down to page size. Yes, with this, iommu padding will never
overflow.
I tried 8G BOs on i915, but the BO sg table was more fragmented
there (though max_segment was 0xffffffff), hence did not run into
this issue. My guess is that we have this issue in i915 as well,
but somehow page allocation (by ttm?) resulting in more fragmented
sg element instead of physically contiguous pages, hence not seeing
this overflow issue.
Niranjana
>/Thomas
>
>
>>
>> Any thoughts on which approach we should follow?
>>
>> Niranjana
>>
>> > /Thomas
>> >
>> > > + return round_down(max / 2, PAGE_SIZE);
>> > > +}
>> > > +
>> > > #if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST)
>> > > /**
>> > > * xe_bo_is_mem_type - Whether the bo currently resides in the
>> > > given
>> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
>> > > b/drivers/gpu/drm/xe/xe_vm.c
>> > > index 2aa5bf9cfee1..5c2fdfc0e836 100644
>> > > --- a/drivers/gpu/drm/xe/xe_vm.c
>> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
>> > > @@ -117,9 +117,11 @@ int xe_vma_userptr_pin_pages(struct xe_vma
>> > > *vma)
>> > > if (ret)
>> > > goto out;
>> > > - ret = sg_alloc_table_from_pages(&vma->userptr.sgt, pages,
>> > > pinned,
>> > > - 0, (u64)pinned <<
>> > > PAGE_SHIFT,
>> > > - GFP_KERNEL);
>> > > + ret = sg_alloc_table_from_pages_segment(&vma-
>> > > >userptr.sgt, pages,
>> > > + pinned, 0,
>> > > + (u64)pinned <<
>> > > PAGE_SHIFT,
>> > > + xe_sg_segment_siz
>> > > e(xe->drm.dev),
>> > > + GFP_KERNEL);
>> > > if (ret) {
>> > > vma->userptr.sg = NULL;
>> > > goto out;
>
More information about the Intel-xe
mailing list