Re: [Intel-gfx] [PATCH 1/4] drm: add helper to walk a dma scatter list a page at a time

Re: [Intel-gfx] [PATCH 1/4] drm: add helper to walk a dma scatter list a page at a time

[Daniel Vetter]

Hi Imre!

On Sat, Feb 09, 2013 at 05:27:33PM +0200, Imre Deak wrote:
> Add a helper to walk through a scatter list a page at a time. Needed by
> upcoming patches fixing the scatter list walking logic in the i915 driver.

Nice patch, but I think this would make a rather nice addition to the
common scatterlist api in scatterlist.h, maybe called sg_page_iter.
There's already another helper which does cpu mappings, but it has a
different use-case (gives you the page mapped, which we don't neeed and
can cope with not page-aligned sg tables). With dma-buf using sg tables I
expect more users of such a sg page iterator to pop up. Most possible
users of this will hang around on linaro-mm-sig, so please also cc that
besides the usual suspects.

Cheers, Daniel

> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  include/drm/drmP.h |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index fad21c9..0c0c213 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1578,6 +1578,50 @@ extern int drm_sg_alloc_ioctl(struct drm_device *dev, void *data,
>  extern int drm_sg_alloc(struct drm_device *dev, struct drm_scatter_gather * request);
>  extern int drm_sg_free(struct drm_device *dev, void *data,
>  		       struct drm_file *file_priv);
> +struct drm_sg_iter {
> +	struct scatterlist *sg;
> +	int sg_offset;

Imo using sg_pfn_offset (i.e. sg_offset >> PAGE_SIZE) would make it
clearer that this is all about iterating page-aligned sg tables.

> +	struct page *page;
> +};
> +
> +static inline int
> +__drm_sg_iter_seek(struct drm_sg_iter *iter)
> +{
> +	while (iter->sg && iter->sg_offset >= iter->sg->length) {
> +		iter->sg_offset -= iter->sg->length;
> +		iter->sg = sg_next(iter->sg);

And adding a WARN_ON(sg->legnth & ~PAGE_MASK); here would enforce that.

> +	}
> +
> +	return iter->sg ? 0 : -1;
> +}
> +
> +static inline struct page *
> +drm_sg_iter_next(struct drm_sg_iter *iter)
> +{
> +	struct page *page;
> +
> +	if (__drm_sg_iter_seek(iter))
> +		return NULL;
> +
> +	page = nth_page(sg_page(iter->sg), iter->sg_offset >> PAGE_SHIFT);
> +	iter->sg_offset = (iter->sg_offset + PAGE_SIZE) & PAGE_MASK;
> +
> +	return page;
> +}
> +
> +static inline struct page *
> +drm_sg_iter_start(struct drm_sg_iter *iter, struct scatterlist *sg,
> +		   unsigned long offset)
> +{
> +	iter->sg = sg;
> +	iter->sg_offset = offset;
> +
> +	return drm_sg_iter_next(iter);
> +}
> +
> +#define drm_for_each_sg_page(iter, sg, pgoffset)			\
> +	for ((iter)->page = drm_sg_iter_start((iter), (sg), (pgoffset));\
> +	     (iter)->page; (iter)->page = drm_sg_iter_next(iter))

Again, for the initialization I'd go with page numbers, not an offset in
bytes.

>  
>  			       /* ATI PCIGART support (ati_pcigart.h) */
>  extern int drm_ati_pcigart_init(struct drm_device *dev,
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/4] drm: add helper to walk a dma scatter list a page at a time

[Imre Deak]

On Sat, 2013-02-09 at 19:56 +0100, Daniel Vetter wrote:
> Hi Imre!
> 
> On Sat, Feb 09, 2013 at 05:27:33PM +0200, Imre Deak wrote:
> > Add a helper to walk through a scatter list a page at a time. Needed by
> > upcoming patches fixing the scatter list walking logic in the i915 driver.
> 
> Nice patch, but I think this would make a rather nice addition to the
> common scatterlist api in scatterlist.h, maybe called sg_page_iter.
> There's already another helper which does cpu mappings, but it has a
> different use-case (gives you the page mapped, which we don't neeed and
> can cope with not page-aligned sg tables). With dma-buf using sg tables I
> expect more users of such a sg page iterator to pop up. Most possible
> users of this will hang around on linaro-mm-sig, so please also cc that
> besides the usual suspects.

Ok, I wanted to sneak this in to drm (and win some time) thinking there
isn't any other user of it atm. But I agree the ideal place for it is in
scatterlist.h, so will send it there.

> Cheers, Daniel
> 
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  include/drm/drmP.h |   44 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index fad21c9..0c0c213 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1578,6 +1578,50 @@ extern int drm_sg_alloc_ioctl(struct drm_device *dev, void *data,
> >  extern int drm_sg_alloc(struct drm_device *dev, struct drm_scatter_gather * request);
> >  extern int drm_sg_free(struct drm_device *dev, void *data,
> >  		       struct drm_file *file_priv);
> > +struct drm_sg_iter {
> > +	struct scatterlist *sg;
> > +	int sg_offset;
> 
> Imo using sg_pfn_offset (i.e. sg_offset >> PAGE_SIZE) would make it
> clearer that this is all about iterating page-aligned sg tables.
> 
> > +	struct page *page;
> > +};
> > +
> > +static inline int
> > +__drm_sg_iter_seek(struct drm_sg_iter *iter)
> > +{
> > +	while (iter->sg && iter->sg_offset >= iter->sg->length) {
> > +		iter->sg_offset -= iter->sg->length;
> > +		iter->sg = sg_next(iter->sg);
> 
> And adding a WARN_ON(sg->legnth & ~PAGE_MASK); here would enforce that.
> 
> > +	}
> > +
> > +	return iter->sg ? 0 : -1;
> > +}
> > +
> > +static inline struct page *
> > +drm_sg_iter_next(struct drm_sg_iter *iter)
> > +{
> > +	struct page *page;
> > +
> > +	if (__drm_sg_iter_seek(iter))
> > +		return NULL;
> > +
> > +	page = nth_page(sg_page(iter->sg), iter->sg_offset >> PAGE_SHIFT);
> > +	iter->sg_offset = (iter->sg_offset + PAGE_SIZE) & PAGE_MASK;
> > +
> > +	return page;
> > +}
> > +
> > +static inline struct page *
> > +drm_sg_iter_start(struct drm_sg_iter *iter, struct scatterlist *sg,
> > +		   unsigned long offset)
> > +{
> > +	iter->sg = sg;
> > +	iter->sg_offset = offset;
> > +
> > +	return drm_sg_iter_next(iter);
> > +}
> > +
> > +#define drm_for_each_sg_page(iter, sg, pgoffset)			\
> > +	for ((iter)->page = drm_sg_iter_start((iter), (sg), (pgoffset));\
> > +	     (iter)->page; (iter)->page = drm_sg_iter_next(iter))
> 
> Again, for the initialization I'd go with page numbers, not an offset in
> bytes.

Ok, can change it to use page numbers instead.

--Imre