[PATCH v14 2/5] dma-buf: heaps: Add heap helpers

[John Stultz]

On Sun, Nov 3, 2019 at 8:13 AM <sspatil at google.com> wrote:
> On Fri, Nov 01, 2019 at 09:42:35PM +0000, John Stultz wrote:
> > Add generic helper dmabuf ops for dma heaps, so we can reduce
> > the amount of duplicative code for the exported dmabufs.
> >
> > This code is an evolution of the Android ION implementation, so
> > thanks to its original authors and maintainters:
> >   Rebecca Schultz Zavin, Colin Cross, Laura Abbott, and others!
> >
> > Cc: Laura Abbott <labbott at redhat.com>
> > Cc: Benjamin Gaignard <benjamin.gaignard at linaro.org>
> > Cc: Sumit Semwal <sumit.semwal at linaro.org>
> > Cc: Liam Mark <lmark at codeaurora.org>
> > Cc: Pratik Patel <pratikp at codeaurora.org>
> > Cc: Brian Starkey <Brian.Starkey at arm.com>
> > Cc: Vincent Donnefort <Vincent.Donnefort at arm.com>
> > Cc: Sudipto Paul <Sudipto.Paul at arm.com>
> > Cc: Andrew F. Davis <afd at ti.com>
> > Cc: Christoph Hellwig <hch at infradead.org>
> > Cc: Chenbo Feng <fengc at google.com>
> > Cc: Alistair Strachan <astrachan at google.com>
> > Cc: Hridya Valsaraju <hridya at google.com>
> > Cc: Sandeep Patil <sspatil at google.com>
> > Cc: Hillf Danton <hdanton at sina.com>
> > Cc: Dave Airlie <airlied at gmail.com>
> > Cc: dri-devel at lists.freedesktop.org
> > Reviewed-by: Benjamin Gaignard <benjamin.gaignard at linaro.org>
> > Reviewed-by: Brian Starkey <brian.starkey at arm.com>
> > Acked-by: Laura Abbott <labbott at redhat.com>
> > Tested-by: Ayan Kumar Halder <ayan.halder at arm.com>
> > Signed-off-by: John Stultz <john.stultz at linaro.org>
>
> I have one question and a naming suggestin below (sorry).
>
> Acked-by: Sandeep Patil <sspatil at android.com>

> > +
> > +static void dma_heap_buffer_vmap_put(struct heap_helper_buffer *buffer)
> > +{
> > +     if (!--buffer->vmap_cnt) {
>
> nit: just checking here cause I don't know the order in which vmap_get() and
> vmap_put() is expected to be called from dmabuf, the manual refcounting
> based map/unmap is ok?
>
> I know ion had this for a while and it worked fine over the years, but I
> don't know if anybody saw any issues with it.
> > +             vunmap(buffer->vaddr);
> > +             buffer->vaddr = NULL;
> > +     }
> > +}
> > +




> > +#ifndef _HEAP_HELPERS_H
> > +#define _HEAP_HELPERS_H
> > +
> > +#include <linux/dma-heap.h>
> > +#include <linux/list.h>
> > +
> > +/**
> > + * struct heap_helper_buffer - helper buffer metadata
> > + * @heap:            back pointer to the heap the buffer came from
> > + * @dmabuf:          backing dma-buf for this buffer
> > + * @size:            size of the buffer
> > + * @flags:           buffer specific flags
> nit: Are thee dmabuf flags, or dmabuf_heap specific / allocation related flags?

Good point. They were going to be for the generic flags but as there's
no supported flags yet, there's no reason to track that in the helper
code.

I'll drop it

> > + * @priv_virt                pointer to heap specific private value
> nit: text looks misaligned (or is it my editor?)

Looks ok to me in vim.


> > + * @lock             mutext to protect the data in this structure
> > + * @vmap_cnt         count of vmap references on the buffer
> > + * @vaddr            vmap'ed virtual address
> > + * @pagecount                number of pages in the buffer
> > + * @pages            list of page pointers
> > + * @attachments              list of device attachments
> ditto
> > + *
> > + * @free             heap callback to free the buffer
> > + */
> > +struct heap_helper_buffer {
> /bikeshed/
> s/heap_helper_buffer/dma_heap_buffer ?
>
> The "heap helper buffer" doesn't really convey what it is.

So its the helper structure that is used with all the helper
functions. Since other dmabuf heaps don't have to use the helper
infrastructure, they wouldn't need this structure, so I don't want to
name it too generically to confuse folks.

thanks
-john