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

[Hellstrom, Thomas]

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.

/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;