On Thu, Jul 22, 2021 at 02:07:51PM +0100, Christoph Hellwig wrote:
> On Thu, Jul 22, 2021 at 10:00:40AM -0300, Jason Gunthorpe wrote:
> > this is better:
> >
> > struct sg_append_table state;
> >
> > sg_append_init(&state, sgt, gfp_mask);
> >
> > while (..)
> > ret = sg_append_pages(&state, pages, n_pages, ..)
> > if (ret)
> > sg_append_abort(&state); // Frees the sgt and puts it to NULL
> > sg_append_complete(&state)
> >
> > Which allows sg_alloc_table_from_pages() to be written as
> >
> > struct sg_append_table state;
> > sg_append_init(&state, sgt, gfp_mask);
> > ret = sg_append_pages(&state,pages, n_pages, offset, size, UINT_MAX)
> > if (ret) {
> > sg_append_abort(&state);
> > return ret;
> > }
> > sg_append_complete(&state);
> > return 0;
> >
> > And then the API can manage all of this in some sane and
> > understandable way.
>
> That would be a lot easier to use for sure. Not sure how invasive the
> changes would be, though.
Looks pretty good, Maor can you try it?
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 1739d10a2c556e..6c8e761ab389e8 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -806,27 +806,27 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
struct sg_table *drm_prime_pages_to_sg(struct drm_device *dev,
struct page **pages, unsigned int nr_pages)
{
- struct sg_table *sg;
- struct scatterlist *sge;
+ struct sg_table *sgt;
size_t max_segment = 0;
+ int ret;
- sg = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
- if (!sg)
+ sgt = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
+ if (!sgt)
return ERR_PTR(-ENOMEM);
if (dev)
max_segment = dma_max_mapping_size(dev->dev);
if (max_segment == 0)
max_segment = UINT_MAX;
- sge = __sg_alloc_table_from_pages(sg, pages, nr_pages, 0,
- nr_pages << PAGE_SHIFT,
- max_segment,
- NULL, 0, GFP_KERNEL, NULL);
- if (IS_ERR(sge)) {
- kfree(sg);
- sg = ERR_CAST(sge);
+
+ ret = sg_alloc_table_from_pages_segment(sgt, pages, nr_pages, 0,
+ nr_pages << PAGE_SHIFT,
+ max_segment, GFP_KERNEL);
+ if (ret) {
+ kfree(sgt);
+ return ERR_PTR(ret);
}
- return sg;
+ return sgt;
}
EXPORT_SYMBOL(drm_prime_pages_to_sg);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index c341d3e3386ccb..a2c5e1b30f425f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -131,6 +131,7 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
struct drm_i915_private *i915 = to_i915(obj->base.dev);
const unsigned long num_pages = obj->base.size >> PAGE_SHIFT;
unsigned int max_segment = i915_sg_segment_size();
+ struct sg_append_table state;
struct sg_table *st;
unsigned int sg_page_sizes;
struct scatterlist *sg;
@@ -153,13 +154,11 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
spin_unlock(&i915->mm.notifier_lock);
alloc_table:
- sg = __sg_alloc_table_from_pages(st, pvec, num_pages, 0,
- num_pages << PAGE_SHIFT, max_segment,
- NULL, 0, GFP_KERNEL, NULL);
- if (IS_ERR(sg)) {
- ret = PTR_ERR(sg);
+ ret = sg_alloc_table_from_pages_segment(st, pvec, num_pages, 0,
+ num_pages << PAGE_SHIFT,
+ max_segment, GFP_KERNEL);
+ if (ret)
goto err;
- }
ret = i915_gem_gtt_prepare_pages(obj, st);
if (ret) {
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index 2ad889272b0a15..56214bcc6c5280 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
@@ -386,15 +386,12 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
if (unlikely(ret != 0))
return ret;
- sg = __sg_alloc_table_from_pages(&vmw_tt->sgt, vsgt->pages,
- vsgt->num_pages, 0,
- (unsigned long) vsgt->num_pages << PAGE_SHIFT,
- dma_get_max_seg_size(dev_priv->drm.dev),
- NULL, 0, GFP_KERNEL, NULL);
- if (IS_ERR(sg)) {
- ret = PTR_ERR(sg);
+ ret = sg_alloc_table_from_pages_segment(
+ vmw_tt->sgt, vsgt->pages, vsgt->num_pages, 0,
+ (unsigned long)vsgt->num_pages << PAGE_SHIFT,
+ dma_get_max_seg_size(dev_priv->drm.dev), GFP_KERNEL);
+ if (ret)
goto out_sg_alloc_fail;
- }
if (vsgt->num_pages > vmw_tt->sgt.orig_nents) {
uint64_t over_alloc =
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index 5cc41ae962ec1d..53de1ec77326be 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -556,6 +556,25 @@ struct scatterlist *__sg_alloc_table_from_pages(struct sg_table *sgt,
}
EXPORT_SYMBOL(__sg_alloc_table_from_pages);
+int sg_alloc_table_from_pages_segment(struct sg_table *sgt, struct page **pages,
+ unsigned int n_pages, unsigned int offset,
+ unsigned long size,
+ unsigned int max_segment, gfp_t gfp_mask)
+{
+ struct sg_append_table state;
+ int ret;
+
+ sg_append_init(&state, sgt, max_segment, gfp_mask);
+ ret = sg_append_pages(&state, pages, n_pages, offset, size);
+ if (ret) {
+ sg_append_abort(&state);
+ return ret;
+ }
+ sg_append_complete(&state);
+ return 0;
+}
+EXPORT_SYMBOL(sg_alloc_table_from_pages_segment);
+
/**
* sg_alloc_table_from_pages - Allocate and initialize an sg table from
* an array of pages
@@ -580,8 +599,8 @@ int sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
unsigned int n_pages, unsigned int offset,
unsigned long size, gfp_t gfp_mask)
{
- return PTR_ERR_OR_ZERO(__sg_alloc_table_from_pages(sgt, pages, n_pages,
- offset, size, UINT_MAX, NULL, 0, gfp_mask, NULL));
+ return sg_alloc_table_from_pages_segment(sgt, pages, n_pages, offset,
+ size, UINT_MAX, gfp_mask);
}
EXPORT_SYMBOL(sg_alloc_table_from_pages);
