public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [RFC][PATCH 1/2] dma-buf: dma-heap: Provide accessor to get heap name
@ 2021-02-06  5:47 John Stultz
  2021-02-06  5:47 ` [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual " John Stultz
  0 siblings, 1 reply; 7+ messages in thread
From: John Stultz @ 2021-02-06  5:47 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Daniel Vetter, Sumit Semwal, Liam Mark,
	Chris Goldsworthy, Laura Abbott, Brian Starkey, Hridya Valsaraju,
	Suren Baghdasaryan, Sandeep Patil, Daniel Mentz, Ørjan Eide,
	Robin Murphy, Ezequiel Garcia, Simon Ser, James Jones,
	linux-media, dri-devel

It can be useful to access the name for the heap,
so provide an accessor to do so.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
Cc: Laura Abbott <labbott@kernel.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Daniel Mentz <danielmentz@google.com>
Cc: Ørjan Eide <orjan.eide@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Ezequiel Garcia <ezequiel@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: James Jones <jajones@nvidia.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/dma-buf/dma-heap.c | 13 +++++++++++++
 include/linux/dma-heap.h   |  9 +++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index afd22c9dbdcf..6c746ea67676 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -190,6 +190,19 @@ void *dma_heap_get_drvdata(struct dma_heap *heap)
 	return heap->priv;
 }
 
+
+/**
+ * dma_heap_get_name() - get heap name
+ * @heap: DMA-Heap to retrieve private data for
+ *
+ * Returns:
+ * The char* for the heap name.
+ */
+char *dma_heap_get_name(struct dma_heap *heap)
+{
+	return heap->name;
+}
+
 struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
 {
 	struct dma_heap *heap, *h, *err_ret;
diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
index 454e354d1ffb..b91778291fb1 100644
--- a/include/linux/dma-heap.h
+++ b/include/linux/dma-heap.h
@@ -50,6 +50,15 @@ struct dma_heap_export_info {
  */
 void *dma_heap_get_drvdata(struct dma_heap *heap);
 
+/**
+ * dma_heap_get_name() - get heap name
+ * @heap: DMA-Heap to retrieve private data for
+ *
+ * Returns:
+ * The char* for the heap name.
+ */
+char *dma_heap_get_name(struct dma_heap *heap);
+
 /**
  * dma_heap_add - adds a heap to dmabuf heaps
  * @exp_info:		information needed to register this heap
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual heap name
  2021-02-06  5:47 [RFC][PATCH 1/2] dma-buf: dma-heap: Provide accessor to get heap name John Stultz
@ 2021-02-06  5:47 ` John Stultz
  2021-02-08 10:08   ` Daniel Vetter
  0 siblings, 1 reply; 7+ messages in thread
From: John Stultz @ 2021-02-06  5:47 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Daniel Vetter, Sumit Semwal, Liam Mark,
	Chris Goldsworthy, Laura Abbott, Brian Starkey, Hridya Valsaraju,
	Suren Baghdasaryan, Sandeep Patil, Daniel Mentz, Ørjan Eide,
	Robin Murphy, Ezequiel Garcia, Simon Ser, James Jones,
	linux-media, dri-devel

By default dma_buf_export() sets the exporter name to be
KBUILD_MODNAME. Unfortunately this may not be identical to the
string used as the heap name (ie: "system" vs "system_heap").

This can cause some minor confusion with tooling, and there is
the future potential where multiple heap types may be exported
by the same module (but would all have the same name).

So to avoid all this, set the exporter exp_name to the heap name.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
Cc: Laura Abbott <labbott@kernel.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Daniel Mentz <danielmentz@google.com>
Cc: Ørjan Eide <orjan.eide@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Ezequiel Garcia <ezequiel@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: James Jones <jajones@nvidia.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/dma-buf/heaps/cma_heap.c    | 1 +
 drivers/dma-buf/heaps/system_heap.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 364fc2f3e499..62465d61ccc7 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -339,6 +339,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
 	buffer->pagecount = pagecount;
 
 	/* create the dmabuf */
+	exp_info.exp_name = dma_heap_get_name(heap);
 	exp_info.ops = &cma_heap_buf_ops;
 	exp_info.size = buffer->len;
 	exp_info.flags = fd_flags;
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 17e0e9a68baf..2d4afc79c700 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -388,6 +388,7 @@ static int system_heap_allocate(struct dma_heap *heap,
 	}
 
 	/* create the dmabuf */
+	exp_info.exp_name = dma_heap_get_name(heap);
 	exp_info.ops = &system_heap_buf_ops;
 	exp_info.size = buffer->len;
 	exp_info.flags = fd_flags;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual heap name
  2021-02-06  5:47 ` [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual " John Stultz
@ 2021-02-08 10:08   ` Daniel Vetter
  2021-02-08 20:50     ` John Stultz
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2021-02-08 10:08 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Daniel Vetter, Sumit Semwal, Liam Mark, Chris Goldsworthy,
	Laura Abbott, Brian Starkey, Hridya Valsaraju, Suren Baghdasaryan,
	Sandeep Patil, Daniel Mentz, Ørjan Eide, Robin Murphy,
	Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel

On Sat, Feb 06, 2021 at 05:47:48AM +0000, John Stultz wrote:
> By default dma_buf_export() sets the exporter name to be
> KBUILD_MODNAME. Unfortunately this may not be identical to the
> string used as the heap name (ie: "system" vs "system_heap").
> 
> This can cause some minor confusion with tooling, and there is
> the future potential where multiple heap types may be exported
> by the same module (but would all have the same name).
> 
> So to avoid all this, set the exporter exp_name to the heap name.
> 
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
> Cc: Laura Abbott <labbott@kernel.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Sandeep Patil <sspatil@google.com>
> Cc: Daniel Mentz <danielmentz@google.com>
> Cc: Ørjan Eide <orjan.eide@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Ezequiel Garcia <ezequiel@collabora.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: James Jones <jajones@nvidia.com>
> Cc: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>

Looks reasonable to me.

I guess the main worry is "does this mean heap names become uapi", in
which case I'm maybe not so sure anymore how this will tie into the
overall gpu memory accounting story.

Since for dma-buf heaps one name per buffer is perfectly fine, since
dma-buf heaps aren't very dynamic. But on discrete gpu drivers buffers
move, so baking in the assumption that "exporter name = resource usage for
this buffer" is broken.

So I'm not sure we shouldn't instead name all the dma-buf heaps as
"dma-buf heaps" to stop this :-)
-Daniel

> ---
>  drivers/dma-buf/heaps/cma_heap.c    | 1 +
>  drivers/dma-buf/heaps/system_heap.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> index 364fc2f3e499..62465d61ccc7 100644
> --- a/drivers/dma-buf/heaps/cma_heap.c
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -339,6 +339,7 @@ static int cma_heap_allocate(struct dma_heap *heap,
>  	buffer->pagecount = pagecount;
>  
>  	/* create the dmabuf */
> +	exp_info.exp_name = dma_heap_get_name(heap);
>  	exp_info.ops = &cma_heap_buf_ops;
>  	exp_info.size = buffer->len;
>  	exp_info.flags = fd_flags;
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 17e0e9a68baf..2d4afc79c700 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -388,6 +388,7 @@ static int system_heap_allocate(struct dma_heap *heap,
>  	}
>  
>  	/* create the dmabuf */
> +	exp_info.exp_name = dma_heap_get_name(heap);
>  	exp_info.ops = &system_heap_buf_ops;
>  	exp_info.size = buffer->len;
>  	exp_info.flags = fd_flags;
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual heap name
  2021-02-08 10:08   ` Daniel Vetter
@ 2021-02-08 20:50     ` John Stultz
  2021-02-08 21:05       ` Daniel Vetter
  0 siblings, 1 reply; 7+ messages in thread
From: John Stultz @ 2021-02-08 20:50 UTC (permalink / raw)
  To: John Stultz, lkml, Sumit Semwal, Liam Mark, Chris Goldsworthy,
	Laura Abbott, Brian Starkey, Hridya Valsaraju, Suren Baghdasaryan,
	Sandeep Patil, Daniel Mentz, Ørjan Eide, Robin Murphy,
	Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel
  Cc: Daniel Vetter

On Mon, Feb 8, 2021 at 2:08 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Feb 06, 2021 at 05:47:48AM +0000, John Stultz wrote:
> > By default dma_buf_export() sets the exporter name to be
> > KBUILD_MODNAME. Unfortunately this may not be identical to the
> > string used as the heap name (ie: "system" vs "system_heap").
> >
> > This can cause some minor confusion with tooling, and there is
> > the future potential where multiple heap types may be exported
> > by the same module (but would all have the same name).
> >
> > So to avoid all this, set the exporter exp_name to the heap name.
> >
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Liam Mark <lmark@codeaurora.org>
> > Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
> > Cc: Laura Abbott <labbott@kernel.org>
> > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > Cc: Hridya Valsaraju <hridya@google.com>
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: Sandeep Patil <sspatil@google.com>
> > Cc: Daniel Mentz <danielmentz@google.com>
> > Cc: Ørjan Eide <orjan.eide@arm.com>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Ezequiel Garcia <ezequiel@collabora.com>
> > Cc: Simon Ser <contact@emersion.fr>
> > Cc: James Jones <jajones@nvidia.com>
> > Cc: linux-media@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
>
> Looks reasonable to me.
>
> I guess the main worry is "does this mean heap names become uapi", in
> which case I'm maybe not so sure anymore how this will tie into the
> overall gpu memory accounting story.
>
> Since for dma-buf heaps one name per buffer is perfectly fine, since
> dma-buf heaps aren't very dynamic. But on discrete gpu drivers buffers
> move, so baking in the assumption that "exporter name = resource usage for
> this buffer" is broken.

I suspect I'm missing a subtlety in what you're describing. My sense
of the exporter name doesn't account for a buffer's usage, it just
describes what code allocated it and implicitly which dmabuf_ops
handles it.  Maybe could you give a more specific example of what
you're hoping to avoid?

To me this patch is mostly just a consistency/least-surprise thing, so
the heaps exporter name matches the string used for the heap's chardev
device (the interface used to allocate it) in output like
debugfs/dma_buf/bufinfo.

thanks
-john

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual heap name
  2021-02-08 20:50     ` John Stultz
@ 2021-02-08 21:05       ` Daniel Vetter
  2021-02-09  6:02         ` Sumit Semwal
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2021-02-08 21:05 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Sumit Semwal, Liam Mark, Chris Goldsworthy, Laura Abbott,
	Brian Starkey, Hridya Valsaraju, Suren Baghdasaryan,
	Sandeep Patil, Daniel Mentz, Ørjan Eide, Robin Murphy,
	Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel

On Mon, Feb 8, 2021 at 9:51 PM John Stultz <john.stultz@linaro.org> wrote:
> On Mon, Feb 8, 2021 at 2:08 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sat, Feb 06, 2021 at 05:47:48AM +0000, John Stultz wrote:
> > > By default dma_buf_export() sets the exporter name to be
> > > KBUILD_MODNAME. Unfortunately this may not be identical to the
> > > string used as the heap name (ie: "system" vs "system_heap").
> > >
> > > This can cause some minor confusion with tooling, and there is
> > > the future potential where multiple heap types may be exported
> > > by the same module (but would all have the same name).
> > >
> > > So to avoid all this, set the exporter exp_name to the heap name.
> > >
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > > Cc: Liam Mark <lmark@codeaurora.org>
> > > Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
> > > Cc: Laura Abbott <labbott@kernel.org>
> > > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > > Cc: Hridya Valsaraju <hridya@google.com>
> > > Cc: Suren Baghdasaryan <surenb@google.com>
> > > Cc: Sandeep Patil <sspatil@google.com>
> > > Cc: Daniel Mentz <danielmentz@google.com>
> > > Cc: Ørjan Eide <orjan.eide@arm.com>
> > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > Cc: Ezequiel Garcia <ezequiel@collabora.com>
> > > Cc: Simon Ser <contact@emersion.fr>
> > > Cc: James Jones <jajones@nvidia.com>
> > > Cc: linux-media@vger.kernel.org
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> >
> > Looks reasonable to me.
> >
> > I guess the main worry is "does this mean heap names become uapi", in
> > which case I'm maybe not so sure anymore how this will tie into the
> > overall gpu memory accounting story.
> >
> > Since for dma-buf heaps one name per buffer is perfectly fine, since
> > dma-buf heaps aren't very dynamic. But on discrete gpu drivers buffers
> > move, so baking in the assumption that "exporter name = resource usage for
> > this buffer" is broken.
>
> I suspect I'm missing a subtlety in what you're describing. My sense
> of the exporter name doesn't account for a buffer's usage, it just
> describes what code allocated it and implicitly which dmabuf_ops
> handles it.  Maybe could you give a more specific example of what
> you're hoping to avoid?

Just paranoia really - on the linux side where we allocate most
buffers (even shared ones) with the driver, that allocator info isn't
that meaningful, it really just tells you which code
allocated/exported that dma-buf.

But on Android, where all shared buffers come from specific heaps, it
is rather meaningful information. So I wondered whether e.g. the
android dmabuf debug tool uses that to collect per-heap stats, but
sounds like no right now. Plus with the chat we've had I think we have
a long-term plan for how to expose that information properly.

> To me this patch is mostly just a consistency/least-surprise thing, so
> the heaps exporter name matches the string used for the heap's chardev
> device (the interface used to allocate it) in output like
> debugfs/dma_buf/bufinfo.

Yeah for debug this makes sense. a-b: me if you want that somewhere on
the patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual heap name
  2021-02-08 21:05       ` Daniel Vetter
@ 2021-02-09  6:02         ` Sumit Semwal
  2021-02-09  6:06           ` John Stultz
  0 siblings, 1 reply; 7+ messages in thread
From: Sumit Semwal @ 2021-02-09  6:02 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: John Stultz, lkml, Liam Mark, Chris Goldsworthy, Laura Abbott,
	Brian Starkey, Hridya Valsaraju, Suren Baghdasaryan,
	Sandeep Patil, Daniel Mentz, Ørjan Eide, Robin Murphy,
	Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel

Hi Daniel,

On Tue, 9 Feb 2021 at 02:36, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Feb 8, 2021 at 9:51 PM John Stultz <john.stultz@linaro.org> wrote:
> > On Mon, Feb 8, 2021 at 2:08 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Sat, Feb 06, 2021 at 05:47:48AM +0000, John Stultz wrote:
> > > > By default dma_buf_export() sets the exporter name to be
> > > > KBUILD_MODNAME. Unfortunately this may not be identical to the
> > > > string used as the heap name (ie: "system" vs "system_heap").
> > > >
> > > > This can cause some minor confusion with tooling, and there is
> > > > the future potential where multiple heap types may be exported
> > > > by the same module (but would all have the same name).
> > > >
> > > > So to avoid all this, set the exporter exp_name to the heap name.
> > > >
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > > > Cc: Liam Mark <lmark@codeaurora.org>
> > > > Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
> > > > Cc: Laura Abbott <labbott@kernel.org>
> > > > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > > > Cc: Hridya Valsaraju <hridya@google.com>
> > > > Cc: Suren Baghdasaryan <surenb@google.com>
> > > > Cc: Sandeep Patil <sspatil@google.com>
> > > > Cc: Daniel Mentz <danielmentz@google.com>
> > > > Cc: Ørjan Eide <orjan.eide@arm.com>
> > > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > > Cc: Ezequiel Garcia <ezequiel@collabora.com>
> > > > Cc: Simon Ser <contact@emersion.fr>
> > > > Cc: James Jones <jajones@nvidia.com>
> > > > Cc: linux-media@vger.kernel.org
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > >
> > > Looks reasonable to me.
> > >
> > > I guess the main worry is "does this mean heap names become uapi", in
> > > which case I'm maybe not so sure anymore how this will tie into the
> > > overall gpu memory accounting story.
> > >
> > > Since for dma-buf heaps one name per buffer is perfectly fine, since
> > > dma-buf heaps aren't very dynamic. But on discrete gpu drivers buffers
> > > move, so baking in the assumption that "exporter name = resource usage for
> > > this buffer" is broken.
> >
> > I suspect I'm missing a subtlety in what you're describing. My sense
> > of the exporter name doesn't account for a buffer's usage, it just
> > describes what code allocated it and implicitly which dmabuf_ops
> > handles it.  Maybe could you give a more specific example of what
> > you're hoping to avoid?
>
> Just paranoia really - on the linux side where we allocate most
> buffers (even shared ones) with the driver, that allocator info isn't
> that meaningful, it really just tells you which code
> allocated/exported that dma-buf.
>
> But on Android, where all shared buffers come from specific heaps, it
> is rather meaningful information. So I wondered whether e.g. the
> android dmabuf debug tool uses that to collect per-heap stats, but
> sounds like no right now. Plus with the chat we've had I think we have
> a long-term plan for how to expose that information properly.
>
> > To me this patch is mostly just a consistency/least-surprise thing, so
> > the heaps exporter name matches the string used for the heap's chardev
> > device (the interface used to allocate it) in output like
> > debugfs/dma_buf/bufinfo.
>
> Yeah for debug this makes sense. a-b: me if you want that somewhere on
> the patches.

Great that this got sorted; I'll apply both the patches of this series
to drm-misc-next, with your a-b.

> -Daniel

Best
Sumit.

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Thanks and regards,

Sumit Semwal
Linaro Consumer Group - Tech Lead
Linaro.org │ Open source software for ARM SoCs

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual heap name
  2021-02-09  6:02         ` Sumit Semwal
@ 2021-02-09  6:06           ` John Stultz
  0 siblings, 0 replies; 7+ messages in thread
From: John Stultz @ 2021-02-09  6:06 UTC (permalink / raw)
  To: Sumit Semwal
  Cc: Daniel Vetter, lkml, Liam Mark, Chris Goldsworthy, Laura Abbott,
	Brian Starkey, Hridya Valsaraju, Suren Baghdasaryan,
	Sandeep Patil, Daniel Mentz, Ørjan Eide, Robin Murphy,
	Ezequiel Garcia, Simon Ser, James Jones, linux-media, dri-devel

On Mon, Feb 8, 2021 at 10:03 PM Sumit Semwal <sumit.semwal@linaro.org> wrote:
>
> Hi Daniel,
>
> On Tue, 9 Feb 2021 at 02:36, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Feb 8, 2021 at 9:51 PM John Stultz <john.stultz@linaro.org> wrote:
> > > On Mon, Feb 8, 2021 at 2:08 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > On Sat, Feb 06, 2021 at 05:47:48AM +0000, John Stultz wrote:
> > > > > By default dma_buf_export() sets the exporter name to be
> > > > > KBUILD_MODNAME. Unfortunately this may not be identical to the
> > > > > string used as the heap name (ie: "system" vs "system_heap").
> > > > >
> > > > > This can cause some minor confusion with tooling, and there is
> > > > > the future potential where multiple heap types may be exported
> > > > > by the same module (but would all have the same name).
> > > > >
> > > > > So to avoid all this, set the exporter exp_name to the heap name.
> > > > >
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > > > > Cc: Liam Mark <lmark@codeaurora.org>
> > > > > Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
> > > > > Cc: Laura Abbott <labbott@kernel.org>
> > > > > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > > > > Cc: Hridya Valsaraju <hridya@google.com>
> > > > > Cc: Suren Baghdasaryan <surenb@google.com>
> > > > > Cc: Sandeep Patil <sspatil@google.com>
> > > > > Cc: Daniel Mentz <danielmentz@google.com>
> > > > > Cc: Ørjan Eide <orjan.eide@arm.com>
> > > > > Cc: Robin Murphy <robin.murphy@arm.com>
> > > > > Cc: Ezequiel Garcia <ezequiel@collabora.com>
> > > > > Cc: Simon Ser <contact@emersion.fr>
> > > > > Cc: James Jones <jajones@nvidia.com>
> > > > > Cc: linux-media@vger.kernel.org
> > > > > Cc: dri-devel@lists.freedesktop.org
> > > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > >
> > > > Looks reasonable to me.
> > > >
> > > > I guess the main worry is "does this mean heap names become uapi", in
> > > > which case I'm maybe not so sure anymore how this will tie into the
> > > > overall gpu memory accounting story.
> > > >
> > > > Since for dma-buf heaps one name per buffer is perfectly fine, since
> > > > dma-buf heaps aren't very dynamic. But on discrete gpu drivers buffers
> > > > move, so baking in the assumption that "exporter name = resource usage for
> > > > this buffer" is broken.
> > >
> > > I suspect I'm missing a subtlety in what you're describing. My sense
> > > of the exporter name doesn't account for a buffer's usage, it just
> > > describes what code allocated it and implicitly which dmabuf_ops
> > > handles it.  Maybe could you give a more specific example of what
> > > you're hoping to avoid?
> >
> > Just paranoia really - on the linux side where we allocate most
> > buffers (even shared ones) with the driver, that allocator info isn't
> > that meaningful, it really just tells you which code
> > allocated/exported that dma-buf.
> >
> > But on Android, where all shared buffers come from specific heaps, it
> > is rather meaningful information. So I wondered whether e.g. the
> > android dmabuf debug tool uses that to collect per-heap stats, but
> > sounds like no right now. Plus with the chat we've had I think we have
> > a long-term plan for how to expose that information properly.
> >
> > > To me this patch is mostly just a consistency/least-surprise thing, so
> > > the heaps exporter name matches the string used for the heap's chardev
> > > device (the interface used to allocate it) in output like
> > > debugfs/dma_buf/bufinfo.
> >
> > Yeah for debug this makes sense. a-b: me if you want that somewhere on
> > the patches.
>
> Great that this got sorted; I'll apply both the patches of this series
> to drm-misc-next, with your a-b.

Before you do, let me spin a v2 as I got some minor tweaks needed
(using const char*) to fix the kbuild bot errors.

thanks
-john

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-02-09  6:07 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-06  5:47 [RFC][PATCH 1/2] dma-buf: dma-heap: Provide accessor to get heap name John Stultz
2021-02-06  5:47 ` [RFC][PATCH 2/2] dma-buf: heaps: Fix the name used when exporting dmabufs to be the actual " John Stultz
2021-02-08 10:08   ` Daniel Vetter
2021-02-08 20:50     ` John Stultz
2021-02-08 21:05       ` Daniel Vetter
2021-02-09  6:02         ` Sumit Semwal
2021-02-09  6:06           ` John Stultz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox