[Intel-xe] [PATCH] drm/xe: Apply upper limit to sg element size

[Niranjana Vishwanathapura]

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_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));
>>+
>>+	/*
>>+	 * 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.

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.

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_size(xe->drm.dev),
>>+						GFP_KERNEL);
>>  	if (ret) {
>>  		vma->userptr.sg = NULL;
>>  		goto out;