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
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;
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
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
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
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
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
John Stultz -
Sumit Semwal