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

[Chang, Yu bruce]

Specific to this particular patch, if following the same patch below for upstreaming i915, the change looks good to me.

https://patchwork.kernel.org/project/intel-gfx/patch/b0f6e50a8cdfc484f2dc18d3215e86465e2e1f1c.1627551226.git.leonro@nvidia.com/

So, with that, 

Reviewed-by: Bruce Chang <yu.bruce.chang at intel.com>

Thanks,
Bruce

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
> Niranjana Vishwanathapura
> Sent: Wednesday, May 17, 2023 10:33 PM
> To: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Cc: Hellstrom, Thomas <thomas.hellstrom at intel.com>; intel-
> xe at lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH] drm/xe: Apply upper limit to sg element size
> 
> 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;