Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191101214238.78015-1-john.stultz@linaro.org>
 <20191101214238.78015-3-john.stultz@linaro.org> <20191103161348.GB13344@google.com>
In-Reply-To: <20191103161348.GB13344@google.com>
From:   John Stultz <john.stultz@linaro.org>
Date:   Mon, 4 Nov 2019 11:34:19 -0800
Message-ID: <CALAqxLUhot_nceaw5zpJ7QXcsfHNL8bOV-3MOeKu9c76Tfzx=g@mail.gmail.com>
Subject: Re: [PATCH v14 2/5] dma-buf: heaps: Add heap helpers
To:     Sandeep Patil <sspatil@google.com>
Cc:     lkml <linux-kernel@vger.kernel.org>,
        Laura Abbott <labbott@redhat.com>,
        Benjamin Gaignard <benjamin.gaignard@linaro.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Liam Mark <lmark@codeaurora.org>,
        Pratik Patel <pratikp@codeaurora.org>,
        Brian Starkey <Brian.Starkey@arm.com>,
        Vincent Donnefort <Vincent.Donnefort@arm.com>,
        Sudipto Paul <Sudipto.Paul@arm.com>,
        Christoph Hellwig <hch@infradead.org>,
        Chenbo Feng <fengc@google.com>,
        Hridya Valsaraju <hridya@google.com>,
        Hillf Danton <hdanton@sina.com>,
        Dave Airlie <airlied@gmail.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        sspatil+mutt@google.com, "Andrew F . Davis" <afd@ti.com>,
        Alistair Strachan <astrachan@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Nov 3, 2019 at 8:13 AM <sspatil@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@redhat.com>
> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Liam Mark <lmark@codeaurora.org>
> > Cc: Pratik Patel <pratikp@codeaurora.org>
> > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > Cc: Vincent Donnefort <Vincent.Donnefort@arm.com>
> > Cc: Sudipto Paul <Sudipto.Paul@arm.com>
> > Cc: Andrew F. Davis <afd@ti.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: Chenbo Feng <fengc@google.com>
> > Cc: Alistair Strachan <astrachan@google.com>
> > Cc: Hridya Valsaraju <hridya@google.com>
> > Cc: Sandeep Patil <sspatil@google.com>
> > Cc: Hillf Danton <hdanton@sina.com>
> > Cc: Dave Airlie <airlied@gmail.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Reviewed-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> > Reviewed-by: Brian Starkey <brian.starkey@arm.com>
> > Acked-by: Laura Abbott <labbott@redhat.com>
> > Tested-by: Ayan Kumar Halder <ayan.halder@arm.com>
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
>
> I have one question and a naming suggestin below (sorry).
>
> Acked-by: Sandeep Patil <sspatil@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