Date: Wed, 2 Aug 2017 16:01:53 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>, Intel-gfx@lists.freedesktop.org,
        linux-kernel@vger.kernel.org,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Ben Widawsky <benjamin.widawsky@intel.com>,
        "Vetter, Daniel" <daniel.vetter@intel.com>
Subject: Re: [Intel-gfx] [PATCH 3/4] lib/scatterlist: Introduce and export
 __sg_alloc_table_from_pages
Message-Id: <20170802160153.dee32799b03f81b925cf5289@linux-foundation.org>
In-Reply-To: <a9f51319-9a2d-3aba-e7b2-7466493c2ef1@linux.intel.com>
References: <20170727090504.15812-1-tvrtko.ursulin@linux.intel.com>
        <20170727090504.15812-4-tvrtko.ursulin@linux.intel.com>
        <a9f51319-9a2d-3aba-e7b2-7466493c2ef1@linux.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3745
Lines: 96

On Wed, 2 Aug 2017 14:06:39 +0100 Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:

> 
> Hi Andrew,
> 
> We have a couple of small lib/scatterlist.c tidies here, plus exporting 
> the new API which allows drivers to control the maximum coalesced entry 
> as created by __sg_alloc_table_from_pages.
> 
> I am looking for an ack to merge these three patches via the drm-intel tree.
> 
> 
> ...
>
> On 27/07/2017 10:05, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Drivers like i915 benefit from being able to control the maxium
> > size of the sg coallesced segment while building the scatter-

"coalesced"

> > gather list.
> > 
> > Introduce and export the __sg_alloc_table_from_pages function
> > which will allow it that control.
> > 
>
> ...
>
> > --- a/lib/scatterlist.c
> > +++ b/lib/scatterlist.c
> > @@ -370,14 +370,15 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
> >   EXPORT_SYMBOL(sg_alloc_table);
> >   
> >   /**
> > - * sg_alloc_table_from_pages - Allocate and initialize an sg table from
> > - *			       an array of pages
> > - * @sgt:	The sg table header to use
> > - * @pages:	Pointer to an array of page pointers
> > - * @n_pages:	Number of pages in the pages array
> > - * @offset:     Offset from start of the first page to the start of a buffer
> > - * @size:       Number of valid bytes in the buffer (after offset)
> > - * @gfp_mask:	GFP allocation mask
> > + * __sg_alloc_table_from_pages - Allocate and initialize an sg table from
> > + *			         an array of pages
> > + * @sgt:	 The sg table header to use
> > + * @pages:	 Pointer to an array of page pointers
> > + * @n_pages:	 Number of pages in the pages array
> > + * @offset:      Offset from start of the first page to the start of a buffer
> > + * @size:        Number of valid bytes in the buffer (after offset)
> > + * @max_segment: Maximum size of a scatterlist node in bytes (page aligned)
> > + * @gfp_mask:	 GFP allocation mask
> >    *
> >    *  Description:
> >    *    Allocate and initialize an sg table from a list of pages. Contiguous

The Description doesn't describe how this differs from
sg_alloc_table_from_pages(), although it doesn't seem terribly
important.

> > +
> > +/**
> > + * sg_alloc_table_from_pages - Allocate and initialize an sg table from
> > + *			       an array of pages
> > + * @sgt:	 The sg table header to use
> > + * @pages:	 Pointer to an array of page pointers
> > + * @n_pages:	 Number of pages in the pages array
> > + * @offset:      Offset from start of the first page to the start of a buffer
> > + * @size:        Number of valid bytes in the buffer (after offset)
> > + * @gfp_mask:	 GFP allocation mask
> > + *
> > + *  Description:
> > + *    Allocate and initialize an sg table from a list of pages. Contiguous
> > + *    ranges of the pages are squashed into a single scatterlist node. A user
> > + *    may provide an offset at a start and a size of valid data in a buffer
> > + *    specified by the page array. The returned sg table is released by
> > + *    sg_free_table.
> > + *
> > + * Returns:
> > + *   0 on success, negative error on failure
> > + */
> > +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 __sg_alloc_table_from_pages(sgt, pages, n_pages, offset, size,
> > +					   SCATTERLIST_MAX_SEGMENT, gfp_mask);
> > +}

Making this an inline would save a bunch or stack space in the callers?

One could just add the new arg then run around and update the 10ish
callers but I guess the new interface is OK.

Otherwise it's OK by me, please go ahead as proposed.