[Intel-xe] [PATCH v2] drm/xe: Apply upper limit to sg element size
Niranjana Vishwanathapura
niranjana.vishwanathapura at intel.com
Tue May 30 19:57:43 UTC 2023
On Fri, May 26, 2023 at 02:12:23AM -0700, Hellstrom, Thomas wrote:
>Hi, Niranjana.
>
>On Thu, 2023-05-25 at 12:35 -0700, Niranjana Vishwanathapura wrote:
>> 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 halving the maximum segment size (rounded down to
>> PAGE_SIZE).
>>
>> Specify maximum segment size for sg elements by using
>> sg_alloc_table_from_pages_segment() to allocate sg_table.
>>
>> v2: Use correct max segment size in dma_set_max_seg_size() call
>
>OK, I've finally read up a bit on the code, and as you say
>dma_set_max_seg_size() doesn't affect the sg_alloc_table_from_pages()
>code at all, so we must use sg_alloc_table_from_pages_segment() as you
>say.
>
>The problem stems from the iommu code trying to align scatterlist
>elements to dma segment boundaries, and the scatterlist::length can
>overflow pretty much regardless of the value of the segment boundaries,
>as you said previously.
>
>But the size of the padding can never exceed the length of an sg
>element, because then the sg element would have fit and no padding
>needed. So that the only restriction needed would be
>scatterlist::length must never exceed UINT_MAX / 2, which was pretty
>much the version 1 of the patch. So some comments below:
>
>>
>> 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_mmio.c | 7 ++-----
>> drivers/gpu/drm/xe/xe_vm.c | 8 +++++---
>> 4 files changed, 33 insertions(+), 11 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_size(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));
>
>dma_max_mapping_size() should be unrelated? perhaps
>
> struct scatterlist __maybe_unused sg;
> size_t max = 1ULL << (sizeof(sg.length) * 8);
Thanks Thomas,
'(1ULL << (sizeof(sg.length) * 8) - 1)' is UINT_MAX which is used above.
Regarding whether to limit the 'max' to dma_max_mapping_size(dev), I am
not sure, xe_sg_segment_size() does similiar to i915_sg_segment_size.
Though it sounds logical to limit max segment size to that imposed by the
iommu driver, I am not seeing intel iommu driver defining 'max_mapping_size'
function and not many drivers using dma_max_mapping_size() function. What
do you suggest?
>
>> +
>> + /*
>> + * 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.
>> + */
>> + 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_mmio.c
>> b/drivers/gpu/drm/xe/xe_mmio.c
>> index c7fbb1cc1f64..4c270a07136e 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> @@ -11,6 +11,7 @@
>> #include "regs/xe_engine_regs.h"
>> #include "regs/xe_gt_regs.h"
>> #include "regs/xe_regs.h"
>> +#include "xe_bo.h"
>> #include "xe_device.h"
>> #include "xe_gt.h"
>> #include "xe_gt_mcr.h"
>> @@ -26,11 +27,7 @@ static int xe_set_dma_info(struct xe_device *xe)
>> unsigned int mask_size = xe->info.dma_mask_size;
>> int err;
>>
>> - /*
>> - * We don't have a max segment size, so set it to the max so
>> sg's
>> - * debugging layer doesn't complain
>> - */
>> - dma_set_max_seg_size(xe->drm.dev, UINT_MAX);
>> + dma_set_max_seg_size(xe->drm.dev, xe_sg_segment_size(xe-
>> >drm.dev));
>
>Should not be needed then?
>
After dma mapping, the iommu code in __finalise_sg() function
contanenates segments based on dma address while subjecting the
sg element length to maximum segment size specified here. Though
at this stage where dma mapping is already done, we probably need
not adhere to our maximum segment size, I thought it is probably
better to stick to spcified maximum segment size throughout.
Niranjana
>>
>> err = dma_set_mask(xe->drm.dev, DMA_BIT_MASK(mask_size));
>> if (err)
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index a0306526b269..4d9c8de8b348 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_size(xe
>> ->drm.dev),
>> + GFP_KERNEL);
>> if (ret) {
>> vma->userptr.sg = NULL;
>> goto out;
>
>Thanks
>/Thomas
>
More information about the Intel-xe
mailing list