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: <20190624194908.121273-1-john.stultz@linaro.org>
 <20190624194908.121273-5-john.stultz@linaro.org> <20190718100840.GB19666@infradead.org>
In-Reply-To: <20190718100840.GB19666@infradead.org>
From:   John Stultz <john.stultz@linaro.org>
Date:   Mon, 22 Jul 2019 22:04:06 -0700
Message-ID: <CALAqxLWLx_tHVjZqrSNWfQ_M2RGGqh4qth3hi9GGRdSPov-gcw@mail.gmail.com>
Subject: Re: [PATCH v6 4/5] dma-buf: heaps: Add CMA heap to dmabuf heaps
To:     Christoph Hellwig <hch@infradead.org>
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>,
        "Andrew F . Davis" <afd@ti.com>,
        Xu YiPing <xuyiping@hisilicon.com>,
        "Chenfeng (puck)" <puck.chen@hisilicon.com>,
        butao <butao@hisilicon.com>,
        "Xiaqing (A)" <saberlily.xia@hisilicon.com>,
        Yudongbin <yudongbin@hisilicon.com>,
        Chenbo Feng <fengc@google.com>,
        Alistair Strachan <astrachan@google.com>,
        dri-devel <dri-devel@lists.freedesktop.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 18, 2019 at 3:08 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> This and the previous one seem very much duplicated boilerplate
> code.

So yes, there is some duplicate boilerplate between the system and cma
heaps particularly in the allocation function, where we allocate and
set up the helper buffer structure, allocate the pages, then create
and export the dmabuf. I took a pass at trying to minimize some of
this earlier, but those efforts ended up adding helper functions that
take a ton of arguments, and had some trouble properly handling error
paths without leaking memory, so I left it as is.

I'll try to take another pass myself but if you have specific
suggestions for improving here, I'd be happy to try them.

> Why can't just normal branches for allocating and freeing
> normal pages vs cma?  We even have an existing helper for that
> with dma_alloc_contiguous().

Apologies, I'm not sure I'm understanding your suggestion here.
dma_alloc_contiguous() does have some interesting optimizations
(avoiding allocating single page from cma), though its focus on
default area vs specific device area doesn't quite match up the usage
model for dma heaps.  Instead of allocating memory for a single
device, we want to be able to allow userland, for a given usage mode,
to be able to allocate a dmabuf from a specific heap of memory which
will satisfy the usage mode for that buffer pipeline (across multiple
devices).

Now, indeed, the system and cma heaps in this patchset are a bit
simple/trivial (though useful with my devices that require contiguous
buffers for the display driver), but more complex ION heaps have
traditionally stayed out of tree in vendor code, in many cases making
incompatible tweaks to the ION core dmabuf exporter logic. That's why
dmabuf heaps is trying to destage ION in a way that allows heaps to
implement their exporter logic themselves, so we can start pulling
those more complicated heaps out of their vendor hidey-holes and get
some proper upstream review.

But I suspect I just am confused as to what your suggesting. Maybe
could you expand a bit? Apologies for being a bit dense.

Thanks again for the input!
-john