Re: [RESEND][PATCH v8 0/5] DMA-BUF Heaps (destaging ION)

From: Ayan Halder <Ayan.Halder@arm.com>
To: Sumit Semwal <sumit.semwal@linaro.org>
Cc: nd <nd@arm.com>, Alistair Strachan <astrachan@google.com>,
	Vincent Donnefort <Vincent.Donnefort@arm.com>,
	Chenbo Feng <fengc@google.com>,
	lkml <linux-kernel@vger.kernel.org>,
	Liam Mark <lmark@codeaurora.org>, "Andrew F . Davis" <afd@ti.com>,
	Christoph Hellwig <hch@infradead.org>,
	DRI mailing list <dri-devel@lists.freedesktop.org>,
	Hridya Valsaraju <hridya@google.com>,
	Sudipto Paul <Sudipto.Paul@arm.com>,
	Pratik Patel <pratikp@codeaurora.org>
Subject: Re: [RESEND][PATCH v8 0/5] DMA-BUF Heaps (destaging ION)
Date: Wed, 9 Oct 2019 17:37:44 +0000	[thread overview]
Message-ID: <20191009173742.GA2682@arm.com> (raw)
In-Reply-To: <20190924162217.GA12974@arm.com>

On Tue, Sep 24, 2019 at 04:22:18PM +0000, Ayan Halder wrote:
> On Thu, Sep 19, 2019 at 10:21:52PM +0530, Sumit Semwal wrote:
> > Hello Christoph, everyone,
> > 
> > On Sat, 7 Sep 2019 at 00:17, John Stultz <john.stultz@linaro.org> wrote:
> > >
> > > Here is yet another pass at the dma-buf heaps patchset Andrew
> > > and I have been working on which tries to destage a fair chunk
> > > of ION functionality.
> > >
> > > The patchset implements per-heap devices which can be opened
> > > directly and then an ioctl is used to allocate a dmabuf from the
> > > heap.
> > >
> > > The interface is similar, but much simpler then IONs, only
> > > providing an ALLOC ioctl.
> > >
> > > Also, I've provided relatively simple system and cma heaps.
> > >
> > > I've booted and tested these patches with AOSP on the HiKey960
> > > using the kernel tree here:
> > >   https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap
> > >
> > > And the userspace changes here:
> > >   https://android-review.googlesource.com/c/device/linaro/hikey/+/909436
> > >
> > > Compared to ION, this patchset is missing the system-contig,
> > > carveout and chunk heaps, as I don't have a device that uses
> > > those, so I'm unable to do much useful validation there.
> > > Additionally we have no upstream users of chunk or carveout,
> > > and the system-contig has been deprecated in the common/andoid-*
> > > kernels, so this should be ok.
> > >
> > > I've also removed the stats accounting, since any such accounting
> > > should be implemented by dma-buf core or the heaps themselves.
> > >
> > > Most of the changes in this revision are adddressing the more
> > > concrete feedback from Christoph (many thanks!). Though I'm not
> > > sure if some of the less specific feedback was completely resolved
> > > in discussion last time around. Please let me know!
> > 
> > It looks like most of the feedback has been taken care of. If there's
> > no more objection to this series, I'd like to merge it in soon.
> > 
> > If there are any more review comments, may I request you to please provide them?
> 
> I tested these patches using our internal test suite with Arm,komeda
> driver and the following node in dts
> 
>         reserved-memory {
>                 #address-cells = <0x2>;
>                 #size-cells = <0x2>;
>                 ranges;
> 
>                 framebuffer@60000000 {
>                         compatible = "shared-dma-pool";
>                         linux,cma-default;
>                         reg = <0x0 0x60000000 0x0 0x8000000>;
>                 };
>         }
Apologies for the confusion, this dts node is irrelevant as our tests were using
the cma heap (via /dev/dma_heap/reserved).

That raises a question. How do we represent the reserved-memory nodes
(as shown above) via the dma-buf heaps framework ?
> 
> The tests went fine. Our tests allocates framebuffers of different
> sizes, posts them on screen and the driver writes back to one of the
> framebuffers. I havenot tested for any performance, latency or
> cache management related stuff. So, it that looks appropriate, feel
> free to add:-
> Tested-by:- Ayan Kumar Halder <ayan.halder@arm.com>
> 
> Are you planning to write some igt tests for it ?
> > 
> > >
> > > New in v8:
> > > * Make struct dma_heap_ops consts (Suggested by Christoph)
> > > * Add flush_kernel_vmap_range/invalidate_kernel_vmap_range calls
> > >   (suggested by Christoph)
> > > * Condense dma_heap_buffer and heap_helper_buffer (suggested by
> > >   Christoph)
> > > * Get rid of needless struct system_heap (suggested by Christoph)
> > > * Fix indentation by using shorter argument names (suggested by
> > >   Christoph)
> > > * Remove unused private_flags value
> > > * Add forgotten include file to fix build issue on x86
> > > * Checkpatch whitespace fixups
> > >
> > > Thoughts and feedback would be greatly appreciated!
> > >
> > > thanks
> > > -john
> > Best,
> > Sumit.
> > >
> > > 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: dri-devel@lists.freedesktop.org
> > >
> > >
> > > Andrew F. Davis (1):
> > >   dma-buf: Add dma-buf heaps framework
> > >
> > > John Stultz (4):
> > >   dma-buf: heaps: Add heap helpers
> > >   dma-buf: heaps: Add system heap to dmabuf heaps
> > >   dma-buf: heaps: Add CMA heap to dmabuf heaps
> > >   kselftests: Add dma-heap test
> > >
> > >  MAINTAINERS                                   |  18 ++
> > >  drivers/dma-buf/Kconfig                       |  11 +
> > >  drivers/dma-buf/Makefile                      |   2 +
> > >  drivers/dma-buf/dma-heap.c                    | 250 ++++++++++++++++
> > >  drivers/dma-buf/heaps/Kconfig                 |  14 +
> > >  drivers/dma-buf/heaps/Makefile                |   4 +
> > >  drivers/dma-buf/heaps/cma_heap.c              | 164 +++++++++++
> > >  drivers/dma-buf/heaps/heap-helpers.c          | 269 ++++++++++++++++++
> > >  drivers/dma-buf/heaps/heap-helpers.h          |  55 ++++
> > >  drivers/dma-buf/heaps/system_heap.c           | 122 ++++++++
> > >  include/linux/dma-heap.h                      |  59 ++++
> > >  include/uapi/linux/dma-heap.h                 |  55 ++++
> > >  tools/testing/selftests/dmabuf-heaps/Makefile |   9 +
> > >  .../selftests/dmabuf-heaps/dmabuf-heap.c      | 230 +++++++++++++++
> > >  14 files changed, 1262 insertions(+)
> > >  create mode 100644 drivers/dma-buf/dma-heap.c
> > >  create mode 100644 drivers/dma-buf/heaps/Kconfig
> > >  create mode 100644 drivers/dma-buf/heaps/Makefile
> > >  create mode 100644 drivers/dma-buf/heaps/cma_heap.c
> > >  create mode 100644 drivers/dma-buf/heaps/heap-helpers.c
> > >  create mode 100644 drivers/dma-buf/heaps/heap-helpers.h
> > >  create mode 100644 drivers/dma-buf/heaps/system_heap.c
> > >  create mode 100644 include/linux/dma-heap.h
> > >  create mode 100644 include/uapi/linux/dma-heap.h
> > >  create mode 100644 tools/testing/selftests/dmabuf-heaps/Makefile
> > >  create mode 100644 tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
> > >
> > > --
> > > 2.17.1
> > >
> > 
> > 
> > -- 
> > Thanks and regards,
> > 
> > Sumit Semwal
> > Linaro Consumer Group - Kernel Team Lead
> > Linaro.org │ Open source software for ARM SoCs
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel