* DMA-heap driver hints
@ 2023-01-23 12:37 Christian König
2023-01-23 12:37 ` [PATCH 1/2] dma-heap: add device link and unlink functions Christian König
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Christian König @ 2023-01-23 12:37 UTC (permalink / raw)
To: l.stach, nicolas, ppaalanen, sumit.semwal, daniel, robdclark,
tfiga, sebastian.wick, hverkuil, dri-devel, linaro-mm-sig,
linux-media, benjamin.gaignard, lmark, labbott, Brian.Starkey,
jstultz, laurent.pinchart, mchehab
Hi guys,
this is just an RFC! The last time we discussed the DMA-buf coherency
problem [1] we concluded that DMA-heap first needs a better way to
communicate to userspace which heap to use for a certain device.
As far as I know userspace currently just hard codes that information
which is certainly not desirable considering that we should have this
inside the kernel as well.
So what those two patches here do is to first add some
dma_heap_create_device_link() and dma_heap_remove_device_link()
function and then demonstrating the functionality with uvcvideo
driver.
The preferred DMA-heap is represented with a symlink in sysfs between
the device and the virtual DMA-heap device node.
What's still missing is certainly matching userspace for this since I
wanted to discuss the initial kernel approach first.
Please take a look and comment.
Thanks,
Christian.
[1] https://lore.kernel.org/all/11a6f97c-e45f-f24b-8a73-48d5a388a2cc@gmail.com/T/
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] dma-heap: add device link and unlink functions
2023-01-23 12:37 DMA-heap driver hints Christian König
@ 2023-01-23 12:37 ` Christian König
2023-01-24 4:45 ` John Stultz
2023-01-23 12:37 ` [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace Christian König
2023-01-23 13:55 ` DMA-heap driver hints Laurent Pinchart
2 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2023-01-23 12:37 UTC (permalink / raw)
To: l.stach, nicolas, ppaalanen, sumit.semwal, daniel, robdclark,
tfiga, sebastian.wick, hverkuil, dri-devel, linaro-mm-sig,
linux-media, benjamin.gaignard, lmark, labbott, Brian.Starkey,
jstultz, laurent.pinchart, mchehab
This allows device drivers to specify a DMA-heap where they want their
buffers to be allocated from. This information is then exposed as
sysfs link between the device and the DMA-heap.
Useful for userspace when in need to decide from which provider to
allocate a buffer.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/dma-buf/dma-heap.c | 41 ++++++++++++++++++++++++++++++--------
include/linux/dma-heap.h | 35 ++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 8 deletions(-)
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index c9e41e8a9e27..0f7cf713c22f 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -31,6 +31,7 @@
* @heap_devt heap device node
* @list list head connecting to list of heaps
* @heap_cdev heap char device
+ * @dev: heap device in sysfs
*
* Represents a heap of memory from which buffers can be made.
*/
@@ -41,6 +42,7 @@ struct dma_heap {
dev_t heap_devt;
struct list_head list;
struct cdev heap_cdev;
+ struct device *dev;
};
static LIST_HEAD(heap_list);
@@ -49,6 +51,33 @@ static dev_t dma_heap_devt;
static struct class *dma_heap_class;
static DEFINE_XARRAY_ALLOC(dma_heap_minors);
+int dma_heap_create_device_link(struct device *dev, const char *heap)
+{
+ struct dma_heap *h;
+
+ /* check the name is valid */
+ mutex_lock(&heap_list_lock);
+ list_for_each_entry(h, &heap_list, list) {
+ if (!strcmp(h->name, heap))
+ break;
+ }
+ mutex_unlock(&heap_list_lock);
+
+ if (list_entry_is_head(h, &heap_list, list)) {
+ pr_err("dma_heap: Link to invalid heap requested %s\n", heap);
+ return -EINVAL;
+ }
+
+ return sysfs_create_link(&dev->kobj, &h->dev->kobj, DEVNAME);
+}
+EXPORT_SYMBOL(dma_heap_create_device_link);
+
+void dma_heap_remove_device_link(struct device *dev)
+{
+ sysfs_remove_link(&dev->kobj, DEVNAME);
+}
+EXPORT_SYMBOL(dma_heap_remove_device_link);
+
static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
unsigned int fd_flags,
unsigned int heap_flags)
@@ -219,7 +248,6 @@ const char *dma_heap_get_name(struct dma_heap *heap)
struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
{
struct dma_heap *heap, *h, *err_ret;
- struct device *dev_ret;
unsigned int minor;
int ret;
@@ -261,14 +289,11 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
goto err1;
}
- dev_ret = device_create(dma_heap_class,
- NULL,
- heap->heap_devt,
- NULL,
- heap->name);
- if (IS_ERR(dev_ret)) {
+ heap->dev = device_create(dma_heap_class, NULL, heap->heap_devt, NULL,
+ heap->name);
+ if (IS_ERR(heap->dev)) {
pr_err("dma_heap: Unable to create device\n");
- err_ret = ERR_CAST(dev_ret);
+ err_ret = ERR_CAST(heap->dev);
goto err2;
}
diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
index 0c05561cad6e..097076766496 100644
--- a/include/linux/dma-heap.h
+++ b/include/linux/dma-heap.h
@@ -65,4 +65,39 @@ const char *dma_heap_get_name(struct dma_heap *heap);
*/
struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
+#ifdef CONFIG_DMABUF_HEAPS
+
+/**
+ * dma_heap_create_device_link() - add link between device and heap
+ * @dev: the device which should be linked to a heap
+ * @heap: name of the heap to link to
+ *
+ * Add a sysfs link between a device and a DMA-heap. This link can then be used
+ * by userspace to figure out from which DMA-heap a device wants it's buffers
+ * to be allocated.
+ */
+int dma_heap_create_device_link(struct device *dev, const char *heap);
+
+/**
+ * dma_heap_remove_device_link() - remove link between device and heap
+ * @dev: the device which should be unlinked
+ *
+ * Removes the sysfs link between the device and it's DMA-heap again when a
+ * driver is unloaded.
+ */
+void dma_heap_remove_device_link(struct device *dev);
+
+#else
+
+int dma_heap_create_device_link(struct device *dev, const char *heap)
+{
+ return 0;
+}
+
+void dma_heap_remove_device_link(struct device *dev)
+{
+}
+
+#endif
+
#endif /* _DMA_HEAPS_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace
2023-01-23 12:37 DMA-heap driver hints Christian König
2023-01-23 12:37 ` [PATCH 1/2] dma-heap: add device link and unlink functions Christian König
@ 2023-01-23 12:37 ` Christian König
2023-01-23 14:00 ` Laurent Pinchart
` (2 more replies)
2023-01-23 13:55 ` DMA-heap driver hints Laurent Pinchart
2 siblings, 3 replies; 18+ messages in thread
From: Christian König @ 2023-01-23 12:37 UTC (permalink / raw)
To: l.stach, nicolas, ppaalanen, sumit.semwal, daniel, robdclark,
tfiga, sebastian.wick, hverkuil, dri-devel, linaro-mm-sig,
linux-media, benjamin.gaignard, lmark, labbott, Brian.Starkey,
jstultz, laurent.pinchart, mchehab
Expose an indicator to let userspace know from which dma_heap
to allocate for buffers of this device.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index e4bcb5011360..b247026b68c5 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -7,6 +7,7 @@
*/
#include <linux/atomic.h>
+#include <linux/dma-heap.h>
#include <linux/gpio/consumer.h>
#include <linux/kernel.h>
#include <linux/list.h>
@@ -1909,6 +1910,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
if (dev->vdev.dev)
v4l2_device_unregister(&dev->vdev);
+ dma_heap_remove_device_link(&dev->udev->dev);
+
#ifdef CONFIG_MEDIA_CONTROLLER
if (media_devnode_is_registered(dev->mdev.devnode))
media_device_unregister(&dev->mdev);
@@ -2181,6 +2184,14 @@ static int uvc_probe(struct usb_interface *intf,
dev->uvc_version >> 8, dev->uvc_version & 0xff);
}
+ /*
+ * UVC exports DMA-buf buffers with dirty CPU caches. For compatibility
+ * with device which can't snoop the CPU cache it's best practice to
+ * allocate DMA-bufs from the system DMA-heap.
+ */
+ if (dma_heap_create_device_link(&dev->udev->dev, "system"))
+ goto error;
+
/* Register the V4L2 device. */
if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
goto error;
--
2.34.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-23 12:37 DMA-heap driver hints Christian König
2023-01-23 12:37 ` [PATCH 1/2] dma-heap: add device link and unlink functions Christian König
2023-01-23 12:37 ` [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace Christian König
@ 2023-01-23 13:55 ` Laurent Pinchart
2023-01-23 16:29 ` Christian König
2023-01-24 5:07 ` John Stultz
2 siblings, 2 replies; 18+ messages in thread
From: Laurent Pinchart @ 2023-01-23 13:55 UTC (permalink / raw)
To: Christian König
Cc: James Jones, linaro-mm-sig, sebastian.wick, labbott,
benjamin.gaignard, linux-media, mchehab, ppaalanen, dri-devel,
nicolas, hverkuil, jstultz, lmark, tfiga, sumit.semwal
Hi Christian,
CC'ing James as I think this is related to his work on the unix device
memory allocator ([1]).
[1] https://lore.kernel.org/dri-devel/8b555674-1c5b-c791-4547-2ea7c16aee6c@nvidia.com/
On Mon, Jan 23, 2023 at 01:37:54PM +0100, Christian König wrote:
> Hi guys,
>
> this is just an RFC! The last time we discussed the DMA-buf coherency
> problem [1] we concluded that DMA-heap first needs a better way to
> communicate to userspace which heap to use for a certain device.
>
> As far as I know userspace currently just hard codes that information
> which is certainly not desirable considering that we should have this
> inside the kernel as well.
>
> So what those two patches here do is to first add some
> dma_heap_create_device_link() and dma_heap_remove_device_link()
> function and then demonstrating the functionality with uvcvideo
> driver.
>
> The preferred DMA-heap is represented with a symlink in sysfs between
> the device and the virtual DMA-heap device node.
I'll start with a few high-level comments/questions:
- Instead of tying drivers to heaps, have you considered a system where
a driver would expose constraints, and a heap would then be selected
based on those constraints ? A tight coupling between heaps and
drivers means downstream patches to drivers in order to use
vendor-specific heaps, that sounds painful.
A constraint-based system would also, I think, be easier to extend
with additional constraints in the future.
- I assume some drivers will be able to support multiple heaps. How do
you envision this being implemented ?
- Devices could have different constraints based on particular
configurations. For instance, a device may require specific memory
layout for multi-planar YUV formats only (as in allocating the Y and C
planes of NV12 from different memory banks). A dynamic API may thus be
needed (but may also be very painful to use from userspace).
> What's still missing is certainly matching userspace for this since I
> wanted to discuss the initial kernel approach first.
https://git.libcamera.org/libcamera/libcamera.git/ would be a good place
to prototype userspace support :-)
> Please take a look and comment.
>
> Thanks,
> Christian.
>
> [1] https://lore.kernel.org/all/11a6f97c-e45f-f24b-8a73-48d5a388a2cc@gmail.com/T/
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace
2023-01-23 12:37 ` [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace Christian König
@ 2023-01-23 14:00 ` Laurent Pinchart
2023-01-23 23:58 ` kernel test robot
2023-01-24 3:44 ` kernel test robot
2 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2023-01-23 14:00 UTC (permalink / raw)
To: Christian König
Cc: linaro-mm-sig, sebastian.wick, labbott, benjamin.gaignard,
linux-media, mchehab, ppaalanen, dri-devel, nicolas, hverkuil,
jstultz, lmark, tfiga, sumit.semwal
Hi Christian,
Thank you for the patch.
On Mon, Jan 23, 2023 at 01:37:56PM +0100, Christian König wrote:
> Expose an indicator to let userspace know from which dma_heap
> to allocate for buffers of this device.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index e4bcb5011360..b247026b68c5 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -7,6 +7,7 @@
> */
>
> #include <linux/atomic.h>
> +#include <linux/dma-heap.h>
> #include <linux/gpio/consumer.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> @@ -1909,6 +1910,8 @@ static void uvc_unregister_video(struct uvc_device *dev)
>
> if (dev->vdev.dev)
> v4l2_device_unregister(&dev->vdev);
> + dma_heap_remove_device_link(&dev->udev->dev);
> +
Could we avoid having to call this explicitly in drivers, possibly using
devres in dma_heap_create_device_link() ?
> #ifdef CONFIG_MEDIA_CONTROLLER
> if (media_devnode_is_registered(dev->mdev.devnode))
> media_device_unregister(&dev->mdev);
> @@ -2181,6 +2184,14 @@ static int uvc_probe(struct usb_interface *intf,
> dev->uvc_version >> 8, dev->uvc_version & 0xff);
> }
>
> + /*
> + * UVC exports DMA-buf buffers with dirty CPU caches. For compatibility
> + * with device which can't snoop the CPU cache it's best practice to
> + * allocate DMA-bufs from the system DMA-heap.
> + */
> + if (dma_heap_create_device_link(&dev->udev->dev, "system"))
I don't think this is the right device. A UVC device is usually a
composite USB device with an audio (UAC) function in addition to UVC,
and that may require a different heap (at least conceptually). Wouldn't
the video_device be a better candidate to expose the link ? This would
create a race condition though, as the link will be created after
userspace gets notified of the device being available.
> + goto error;
> +
> /* Register the V4L2 device. */
> if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
> goto error;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-23 13:55 ` DMA-heap driver hints Laurent Pinchart
@ 2023-01-23 16:29 ` Christian König
2023-01-23 16:58 ` Laurent Pinchart
2023-01-24 5:19 ` John Stultz
2023-01-24 5:07 ` John Stultz
1 sibling, 2 replies; 18+ messages in thread
From: Christian König @ 2023-01-23 16:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: James Jones, linaro-mm-sig, sebastian.wick, labbott,
benjamin.gaignard, linux-media, mchehab, ppaalanen, dri-devel,
nicolas, hverkuil, jstultz, lmark, tfiga, sumit.semwal
Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
> Hi Christian,
>
> CC'ing James as I think this is related to his work on the unix device
> memory allocator ([1]).
>
> [1] https://lore.kernel.org/dri-devel/8b555674-1c5b-c791-4547-2ea7c16aee6c@nvidia.com/
>
> On Mon, Jan 23, 2023 at 01:37:54PM +0100, Christian König wrote:
>> Hi guys,
>>
>> this is just an RFC! The last time we discussed the DMA-buf coherency
>> problem [1] we concluded that DMA-heap first needs a better way to
>> communicate to userspace which heap to use for a certain device.
>>
>> As far as I know userspace currently just hard codes that information
>> which is certainly not desirable considering that we should have this
>> inside the kernel as well.
>>
>> So what those two patches here do is to first add some
>> dma_heap_create_device_link() and dma_heap_remove_device_link()
>> function and then demonstrating the functionality with uvcvideo
>> driver.
>>
>> The preferred DMA-heap is represented with a symlink in sysfs between
>> the device and the virtual DMA-heap device node.
> I'll start with a few high-level comments/questions:
>
> - Instead of tying drivers to heaps, have you considered a system where
> a driver would expose constraints, and a heap would then be selected
> based on those constraints ? A tight coupling between heaps and
> drivers means downstream patches to drivers in order to use
> vendor-specific heaps, that sounds painful.
I was wondering the same thing as well, but came to the conclusion that
just the other way around is the less painful approach.
The problem is that there are so many driver specific constrains that I
don't even know where to start from.
> A constraint-based system would also, I think, be easier to extend
> with additional constraints in the future.
>
> - I assume some drivers will be able to support multiple heaps. How do
> you envision this being implemented ?
I don't really see an use case for this.
We do have some drivers which say: for this use case you can use
whatever you want, but for that use case you need to use specific memory
(scan out on GPUs for example works like this).
But those specific use cases are exactly that, very specific. And
exposing all the constrains for them inside a kernel UAPI is a futile
effort (at least for the GPU scan out case). In those situations it's
just better to have the allocator in userspace deal with device specific
stuff.
What I want to do is to separate the problem. The kernel only provides
the information where to allocate from, figuring out the details like
how many bytes, which format, plane layout etc.. is still the job of
userspace.
What we do have is compatibility between heaps. E.g. a CMA heap is
usually compatible with the system heap or might even be a subset of
another CMA heap. But I wanted to add that as next step to the heaps
framework itself.
> - Devices could have different constraints based on particular
> configurations. For instance, a device may require specific memory
> layout for multi-planar YUV formats only (as in allocating the Y and C
> planes of NV12 from different memory banks). A dynamic API may thus be
> needed (but may also be very painful to use from userspace).
Uff, good to know. But I'm not sure how to expose stuff like that.
>> What's still missing is certainly matching userspace for this since I
>> wanted to discuss the initial kernel approach first.
> https://git.libcamera.org/libcamera/libcamera.git/ would be a good place
> to prototype userspace support :-)
Thanks for the pointer and the review,
Christian.
>
>> Please take a look and comment.
>>
>> Thanks,
>> Christian.
>>
>> [1] https://lore.kernel.org/all/11a6f97c-e45f-f24b-8a73-48d5a388a2cc@gmail.com/T/
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-23 16:29 ` Christian König
@ 2023-01-23 16:58 ` Laurent Pinchart
2023-01-24 3:56 ` James Jones
2023-01-24 5:19 ` John Stultz
1 sibling, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2023-01-23 16:58 UTC (permalink / raw)
To: Christian König
Cc: James Jones, linaro-mm-sig, sebastian.wick, labbott,
benjamin.gaignard, linux-media, mchehab, ppaalanen, dri-devel,
nicolas, hverkuil, jstultz, lmark, tfiga, sumit.semwal
Hi Christian,
On Mon, Jan 23, 2023 at 05:29:18PM +0100, Christian König wrote:
> Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
> > Hi Christian,
> >
> > CC'ing James as I think this is related to his work on the unix device
> > memory allocator ([1]).
> >
> > [1] https://lore.kernel.org/dri-devel/8b555674-1c5b-c791-4547-2ea7c16aee6c@nvidia.com/
> >
> > On Mon, Jan 23, 2023 at 01:37:54PM +0100, Christian König wrote:
> >> Hi guys,
> >>
> >> this is just an RFC! The last time we discussed the DMA-buf coherency
> >> problem [1] we concluded that DMA-heap first needs a better way to
> >> communicate to userspace which heap to use for a certain device.
> >>
> >> As far as I know userspace currently just hard codes that information
> >> which is certainly not desirable considering that we should have this
> >> inside the kernel as well.
> >>
> >> So what those two patches here do is to first add some
> >> dma_heap_create_device_link() and dma_heap_remove_device_link()
> >> function and then demonstrating the functionality with uvcvideo
> >> driver.
> >>
> >> The preferred DMA-heap is represented with a symlink in sysfs between
> >> the device and the virtual DMA-heap device node.
> >
> > I'll start with a few high-level comments/questions:
> >
> > - Instead of tying drivers to heaps, have you considered a system where
> > a driver would expose constraints, and a heap would then be selected
> > based on those constraints ? A tight coupling between heaps and
> > drivers means downstream patches to drivers in order to use
> > vendor-specific heaps, that sounds painful.
>
> I was wondering the same thing as well, but came to the conclusion that
> just the other way around is the less painful approach.
From a kernel point of view, sure, it's simpler and thus less painful.
From the point of view of solving the whole issue, I'm not sure :-)
> The problem is that there are so many driver specific constrains that I
> don't even know where to start from.
That's where I was hoping James would have some feedback for us, based
on the work he did on the Unix device memory allocator. If that's not
the case, we can brainstorm this from scratch.
> > A constraint-based system would also, I think, be easier to extend
> > with additional constraints in the future.
> >
> > - I assume some drivers will be able to support multiple heaps. How do
> > you envision this being implemented ?
>
> I don't really see an use case for this.
>
> We do have some drivers which say: for this use case you can use
> whatever you want, but for that use case you need to use specific memory
> (scan out on GPUs for example works like this).
>
> But those specific use cases are exactly that, very specific. And
> exposing all the constrains for them inside a kernel UAPI is a futile
> effort (at least for the GPU scan out case). In those situations it's
> just better to have the allocator in userspace deal with device specific
> stuff.
While the exact constraints will certainly be device-specific, is that
also true of the type of constraints, or the existence of constraints in
the first place ? To give an example, with a video decoder producing
frames that are then rendered by a GPU, the tiling format that would
produce the best result is device-specific, but the fact that the
decoder can produce a tiled format that would work better for the GPU,
or a non-tiled format for other consumers, is a very common constraint.
I don't think we'll be able to do away completely with the
device-specific code in userspace, but I think we should be able to
expose constraints in a generic-enough way that many simple use cases
will be covered by generic code.
> What I want to do is to separate the problem. The kernel only provides
> the information where to allocate from, figuring out the details like
> how many bytes, which format, plane layout etc.. is still the job of
> userspace.
Even with UVC, where to allocate memory from will depend on the use
case. If the consumer is a device that doesn't support non-contiguous
DMA, the system heap won't work.
Actually, could you explain why UVC works better with the system heap ?
I'm looking at videobuf2 as an importer, and it doesn't call the dmabuf
as far as I can tell, so cache management provided by the exporter seems
to be bypassed in any case.
> What we do have is compatibility between heaps. E.g. a CMA heap is
> usually compatible with the system heap or might even be a subset of
> another CMA heap. But I wanted to add that as next step to the heaps
> framework itself.
>
> > - Devices could have different constraints based on particular
> > configurations. For instance, a device may require specific memory
> > layout for multi-planar YUV formats only (as in allocating the Y and C
> > planes of NV12 from different memory banks). A dynamic API may thus be
> > needed (but may also be very painful to use from userspace).
>
> Uff, good to know. But I'm not sure how to expose stuff like that.
Let's see if James has anything to share with us :-) With a bit of luck
we won't have to start from scratch.
> >> What's still missing is certainly matching userspace for this since I
> >> wanted to discuss the initial kernel approach first.
> >
> > https://git.libcamera.org/libcamera/libcamera.git/ would be a good place
> > to prototype userspace support :-)
>
> Thanks for the pointer and the review,
By the way, side question, does anyone know what the status of dma heaps
support is in major distributions ? On my Gentoo box,
/dev/dma_heap/system is 0600 root:root. That's easy to change for a
developer, but not friendly to end-users. If we want to move forward
with dma heaps as standard multimedia allocators (and I would really
like to see that happening), we have to make sure they can be used.
> >> Please take a look and comment.
> >>
> >> Thanks,
> >> Christian.
> >>
> >> [1] https://lore.kernel.org/all/11a6f97c-e45f-f24b-8a73-48d5a388a2cc@gmail.com/T/
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace
2023-01-23 12:37 ` [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace Christian König
2023-01-23 14:00 ` Laurent Pinchart
@ 2023-01-23 23:58 ` kernel test robot
2023-01-24 3:44 ` kernel test robot
2 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2023-01-23 23:58 UTC (permalink / raw)
To: Christian König, l.stach, nicolas, ppaalanen, sumit.semwal,
daniel, robdclark, tfiga, sebastian.wick, hverkuil, dri-devel,
linaro-mm-sig, linux-media, benjamin.gaignard, lmark, labbott,
Brian.Starkey, jstultz, laurent.pinchart, mchehab
Cc: llvm, oe-kbuild-all
Hi Christian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on media-tree/master drm-tip/drm-tip linus/master v6.2-rc5]
[cannot apply to next-20230123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/media-uvcvideo-expose-dma-heap-hint-to-userspace/20230123-213836
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20230123123756.401692-3-christian.koenig%40amd.com
patch subject: [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace
config: hexagon-randconfig-r032-20230123 (https://download.01.org/0day-ci/archive/20230124/202301240717.tim1ggHo-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/adc04dccd892eec7f84c6ec112b48df376172e48
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-K-nig/media-uvcvideo-expose-dma-heap-hint-to-userspace/20230123-213836
git checkout adc04dccd892eec7f84c6ec112b48df376172e48
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/media/usb/uvc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/media/usb/uvc/uvc_driver.c:10:
>> include/linux/dma-heap.h:92:5: warning: no previous prototype for function 'dma_heap_create_device_link' [-Wmissing-prototypes]
int dma_heap_create_device_link(struct device *dev, const char *heap)
^
include/linux/dma-heap.h:92:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int dma_heap_create_device_link(struct device *dev, const char *heap)
^
static
>> include/linux/dma-heap.h:97:6: warning: no previous prototype for function 'dma_heap_remove_device_link' [-Wmissing-prototypes]
void dma_heap_remove_device_link(struct device *dev)
^
include/linux/dma-heap.h:97:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void dma_heap_remove_device_link(struct device *dev)
^
static
In file included from drivers/media/usb/uvc/uvc_driver.c:16:
In file included from include/linux/usb.h:16:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/media/usb/uvc/uvc_driver.c:16:
In file included from include/linux/usb.h:16:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/media/usb/uvc/uvc_driver.c:16:
In file included from include/linux/usb.h:16:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
8 warnings generated.
vim +/dma_heap_create_device_link +92 include/linux/dma-heap.h
4ce5c5c0cf31f4 Christian König 2023-01-23 91
4ce5c5c0cf31f4 Christian König 2023-01-23 @92 int dma_heap_create_device_link(struct device *dev, const char *heap)
4ce5c5c0cf31f4 Christian König 2023-01-23 93 {
4ce5c5c0cf31f4 Christian König 2023-01-23 94 return 0;
4ce5c5c0cf31f4 Christian König 2023-01-23 95 }
4ce5c5c0cf31f4 Christian König 2023-01-23 96
4ce5c5c0cf31f4 Christian König 2023-01-23 @97 void dma_heap_remove_device_link(struct device *dev)
4ce5c5c0cf31f4 Christian König 2023-01-23 98 {
4ce5c5c0cf31f4 Christian König 2023-01-23 99 }
4ce5c5c0cf31f4 Christian König 2023-01-23 100
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace
2023-01-23 12:37 ` [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace Christian König
2023-01-23 14:00 ` Laurent Pinchart
2023-01-23 23:58 ` kernel test robot
@ 2023-01-24 3:44 ` kernel test robot
2 siblings, 0 replies; 18+ messages in thread
From: kernel test robot @ 2023-01-24 3:44 UTC (permalink / raw)
To: Christian König, l.stach, nicolas, ppaalanen, sumit.semwal,
daniel, robdclark, tfiga, sebastian.wick, hverkuil, dri-devel,
linaro-mm-sig, linux-media, benjamin.gaignard, lmark, labbott,
Brian.Starkey, jstultz, laurent.pinchart, mchehab
Cc: oe-kbuild-all
Hi Christian,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on media-tree/master drm-tip/drm-tip linus/master v6.2-rc5]
[cannot apply to next-20230123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/media-uvcvideo-expose-dma-heap-hint-to-userspace/20230123-213836
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20230123123756.401692-3-christian.koenig%40amd.com
patch subject: [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace
config: i386-randconfig-a002-20230123 (https://download.01.org/0day-ci/archive/20230124/202301241137.qT2rnQ5T-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/adc04dccd892eec7f84c6ec112b48df376172e48
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-K-nig/media-uvcvideo-expose-dma-heap-hint-to-userspace/20230123-213836
git checkout adc04dccd892eec7f84c6ec112b48df376172e48
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/media/usb/uvc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/media/usb/uvc/uvc_driver.c:10:
>> include/linux/dma-heap.h:92:5: warning: no previous prototype for 'dma_heap_create_device_link' [-Wmissing-prototypes]
92 | int dma_heap_create_device_link(struct device *dev, const char *heap)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/linux/dma-heap.h:97:6: warning: no previous prototype for 'dma_heap_remove_device_link' [-Wmissing-prototypes]
97 | void dma_heap_remove_device_link(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/dma_heap_create_device_link +92 include/linux/dma-heap.h
4ce5c5c0cf31f4 Christian König 2023-01-23 91
4ce5c5c0cf31f4 Christian König 2023-01-23 @92 int dma_heap_create_device_link(struct device *dev, const char *heap)
4ce5c5c0cf31f4 Christian König 2023-01-23 93 {
4ce5c5c0cf31f4 Christian König 2023-01-23 94 return 0;
4ce5c5c0cf31f4 Christian König 2023-01-23 95 }
4ce5c5c0cf31f4 Christian König 2023-01-23 96
4ce5c5c0cf31f4 Christian König 2023-01-23 @97 void dma_heap_remove_device_link(struct device *dev)
4ce5c5c0cf31f4 Christian König 2023-01-23 98 {
4ce5c5c0cf31f4 Christian König 2023-01-23 99 }
4ce5c5c0cf31f4 Christian König 2023-01-23 100
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-23 16:58 ` Laurent Pinchart
@ 2023-01-24 3:56 ` James Jones
2023-01-24 7:48 ` Christian König
0 siblings, 1 reply; 18+ messages in thread
From: James Jones @ 2023-01-24 3:56 UTC (permalink / raw)
To: Laurent Pinchart, Christian König
Cc: linaro-mm-sig, sebastian.wick, labbott, benjamin.gaignard,
linux-media, mchehab, ppaalanen, dri-devel, nicolas, hverkuil,
jstultz, lmark, tfiga, sumit.semwal
On 1/23/23 08:58, Laurent Pinchart wrote:
> Hi Christian,
>
> On Mon, Jan 23, 2023 at 05:29:18PM +0100, Christian König wrote:
>> Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
>>> Hi Christian,
>>>
>>> CC'ing James as I think this is related to his work on the unix device
>>> memory allocator ([1]).
Thank you for including me.
>>> [1] https://lore.kernel.org/dri-devel/8b555674-1c5b-c791-4547-2ea7c16aee6c@nvidia.com/
>>>
>>> On Mon, Jan 23, 2023 at 01:37:54PM +0100, Christian König wrote:
>>>> Hi guys,
>>>>
>>>> this is just an RFC! The last time we discussed the DMA-buf coherency
>>>> problem [1] we concluded that DMA-heap first needs a better way to
>>>> communicate to userspace which heap to use for a certain device.
>>>>
>>>> As far as I know userspace currently just hard codes that information
>>>> which is certainly not desirable considering that we should have this
>>>> inside the kernel as well.
>>>>
>>>> So what those two patches here do is to first add some
>>>> dma_heap_create_device_link() and dma_heap_remove_device_link()
>>>> function and then demonstrating the functionality with uvcvideo
>>>> driver.
>>>>
>>>> The preferred DMA-heap is represented with a symlink in sysfs between
>>>> the device and the virtual DMA-heap device node.
>>>
>>> I'll start with a few high-level comments/questions:
>>>
>>> - Instead of tying drivers to heaps, have you considered a system where
>>> a driver would expose constraints, and a heap would then be selected
>>> based on those constraints ? A tight coupling between heaps and
>>> drivers means downstream patches to drivers in order to use
>>> vendor-specific heaps, that sounds painful.
>>
>> I was wondering the same thing as well, but came to the conclusion that
>> just the other way around is the less painful approach.
>
> From a kernel point of view, sure, it's simpler and thus less painful.
> From the point of view of solving the whole issue, I'm not sure :-)
>
>> The problem is that there are so many driver specific constrains that I
>> don't even know where to start from.
>
> That's where I was hoping James would have some feedback for us, based
> on the work he did on the Unix device memory allocator. If that's not
> the case, we can brainstorm this from scratch.
Simon Ser's and my presentation from XDC 2020 focused entirely on this.
The idea was not to try to enumerate every constraint up front, but
rather to develop an extensible mechanism that would be flexible enough
to encapsulate many disparate types of constraints and perform set
operations on them (merging sets was the only operation we tried to
solve). Simon implemented a prototype header-only library to implement
the mechanism:
https://gitlab.freedesktop.org/emersion/drm-constraints
The links to the presentation and talk are below, along with notes from
the follow-up workshop.
https://lpc.events/event/9/contributions/615/attachments/704/1301/XDC_2020__Allocation_Constraints.pdf
https://www.youtube.com/watch?v=HZEClOP5TIk
https://paste.sr.ht/~emersion/c43b30be08bab1882f1b107402074462bba3b64a
Note one of the hard parts of this was figuring out how to express a
device or heap within the constraint structs. One of the better ideas
proposed back then was something like heap IDs, where dma heaps would
each have one, and devices could register their own heaps (or even just
themselves?) with the heap subsystem and be assigned a locally-unique ID
that userspace could pass around. This sounds similar to what you're
proposing. Perhaps a reasonable identifier is a device (major, minor)
pair. Such a constraint could be expressed as a symlink for easy
visualization/discoverability from userspace, but might be easier to
serialize over the wire as the (major, minor) pair. I'm not clear which
direction is better to express this either: As a link from heap->device,
or device->heap.
>>> A constraint-based system would also, I think, be easier to extend
>>> with additional constraints in the future.
>>>
>>> - I assume some drivers will be able to support multiple heaps. How do
>>> you envision this being implemented ?
>>
>> I don't really see an use case for this.
One use case I know of here is same-vendor GPU local memory on different
GPUs. NVIDIA GPUs have certain things they can only do on local memory,
certain things they can do on all memory, and certain things they can
only do on memory local to another NVIDIA GPU, especially when there
exists an NVLink interface between the two. So they'd ideally express
different constraints for heap representing each of those.
The same thing is often true of memory on remote devices that are at
various points in a PCIe topology. We've had situations where we could
only get enough bandwidth between two PCIe devices when they were less
than some number of hops away on the PCI tree. We hard-coded logic to
detect that in our userspace drivers, but we could instead expose it as
a constraint on heaps that would express which devices can accomplish
certain operations as pairs.
Similarly to the last one, I would assume (But haven't yet run into in
my personal experience) similar limitations arise when you have a NUMA
memory configuration, if you had a heap representing each NUMA node or
something, some might have more coherency than others, or might have
different bandwidth limitations that you could express through something
like device tree, etc. This is more speculative, but seems like a
generalization of the above two cases.
>> We do have some drivers which say: for this use case you can use
>> whatever you want, but for that use case you need to use specific memory
>> (scan out on GPUs for example works like this).
>>
>> But those specific use cases are exactly that, very specific. And
>> exposing all the constrains for them inside a kernel UAPI is a futile
>> effort (at least for the GPU scan out case). In those situations it's
>> just better to have the allocator in userspace deal with device specific
>> stuff.
>
> While the exact constraints will certainly be device-specific, is that
> also true of the type of constraints, or the existence of constraints in
> the first place ? To give an example, with a video decoder producing
> frames that are then rendered by a GPU, the tiling format that would
> produce the best result is device-specific, but the fact that the
> decoder can produce a tiled format that would work better for the GPU,
> or a non-tiled format for other consumers, is a very common constraint.
> I don't think we'll be able to do away completely with the
> device-specific code in userspace, but I think we should be able to
> expose constraints in a generic-enough way that many simple use cases
> will be covered by generic code.
Yes, agreed, the design we proposed took pains to allow vendor-specific
constraints via a general mechanism. We supported both vendor-specific
types of constraints, and vendor-specific values for general
constraints. Some code repository would act as the central registry of
constraint types, similar to the Linux kernel's drm_fourcc.h for
modifiers, or the Khronos github repository for Vulkan vendor IDs. If
the definition needs to be used by the kernel, the kernel is the logical
repository for that role IMHO.
In our 2020 discussion, there was some debate over whether the kernel
should expose and/or consume constraints directly, or whether it's
sufficient to expose lower-level mechanisms from the kernel and keep the
translation of constraints to the correct mechanism in userspace. There
are pros/cons to both. I don't know that we bottomed out on that part of
the discussion, and it could be the right answer is some combination of
the two, as suggested below.
>> What I want to do is to separate the problem. The kernel only provides
>> the information where to allocate from, figuring out the details like
>> how many bytes, which format, plane layout etc.. is still the job of
>> userspace.
>
> Even with UVC, where to allocate memory from will depend on the use
> case. If the consumer is a device that doesn't support non-contiguous
> DMA, the system heap won't work.
>
> Actually, could you explain why UVC works better with the system heap ?
> I'm looking at videobuf2 as an importer, and it doesn't call the dmabuf
> as far as I can tell, so cache management provided by the exporter seems
> to be bypassed in any case.
>
>> What we do have is compatibility between heaps. E.g. a CMA heap is
>> usually compatible with the system heap or might even be a subset of
>> another CMA heap. But I wanted to add that as next step to the heaps
>> framework itself.
>>
>>> - Devices could have different constraints based on particular
>>> configurations. For instance, a device may require specific memory
>>> layout for multi-planar YUV formats only (as in allocating the Y and C
>>> planes of NV12 from different memory banks). A dynamic API may thus be
>>> needed (but may also be very painful to use from userspace).
>>
>> Uff, good to know. But I'm not sure how to expose stuff like that.
>
> Let's see if James has anything to share with us :-) With a bit of luck
> we won't have to start from scratch.
Well, this is the hard example we keep using as a measure of success for
whatever we come up with. I don't know that someone ever sat down and
tried to express this in the mechanism Simon and I proposed in 2020, but
allowing the expression of something that complex was certainly our
goal. How to resolve it down to an allocation mechanism, I believe, was
further than we got, but we weren't that well versed in DMA heaps back
then, or at least I wasn't.
>>>> What's still missing is certainly matching userspace for this since I
>>>> wanted to discuss the initial kernel approach first.
>>>
>>> https://git.libcamera.org/libcamera/libcamera.git/ would be a good place
>>> to prototype userspace support :-)
>>
>> Thanks for the pointer and the review,
>
> By the way, side question, does anyone know what the status of dma heaps
> support is in major distributions ? On my Gentoo box,
> /dev/dma_heap/system is 0600 root:root. That's easy to change for a
> developer, but not friendly to end-users. If we want to move forward
> with dma heaps as standard multimedia allocators (and I would really
> like to see that happening), we have to make sure they can be used.
We seem to have reached a world where display (primary nodes) are
carefully guarded, and some mildly trusted group of folks (video) can
access render nodes, but then there's some separate group generally for
camera/video/v4l and whatnot from what I've seen (I don't survey this
stuff that often. I live in my developer bubble). I'm curious whether
the right direction is a broader group that encompasses all of render
nodes, multimedia, and heaps, or if a more segmented design is right.
The latter is probably the better design from first principles, but
might lead to headaches if the permissions diverge.
Thanks,
-James
>>>> Please take a look and comment.
>>>>
>>>> Thanks,
>>>> Christian.
>>>>
>>>> [1] https://lore.kernel.org/all/11a6f97c-e45f-f24b-8a73-48d5a388a2cc@gmail.com/T/
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] dma-heap: add device link and unlink functions
2023-01-23 12:37 ` [PATCH 1/2] dma-heap: add device link and unlink functions Christian König
@ 2023-01-24 4:45 ` John Stultz
0 siblings, 0 replies; 18+ messages in thread
From: John Stultz @ 2023-01-24 4:45 UTC (permalink / raw)
To: Christian König
Cc: linaro-mm-sig, sebastian.wick, benjamin.gaignard, linux-media,
mchehab, ppaalanen, dri-devel, nicolas, hverkuil,
laurent.pinchart, tfiga, sumit.semwal, T.J. Mercier
On Mon, Jan 23, 2023 at 4:38 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> This allows device drivers to specify a DMA-heap where they want their
> buffers to be allocated from. This information is then exposed as
> sysfs link between the device and the DMA-heap.
>
> Useful for userspace when in need to decide from which provider to
> allocate a buffer.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Hey Christian!
Thanks so much for sending this out and also thanks for including me
(Adding TJ as well)!
This looks like a really interesting approach, but I have a few thoughts below.
> ---
> drivers/dma-buf/dma-heap.c | 41 ++++++++++++++++++++++++++++++--------
> include/linux/dma-heap.h | 35 ++++++++++++++++++++++++++++++++
> 2 files changed, 68 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index c9e41e8a9e27..0f7cf713c22f 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -31,6 +31,7 @@
> * @heap_devt heap device node
> * @list list head connecting to list of heaps
> * @heap_cdev heap char device
> + * @dev: heap device in sysfs
> *
> * Represents a heap of memory from which buffers can be made.
> */
> @@ -41,6 +42,7 @@ struct dma_heap {
> dev_t heap_devt;
> struct list_head list;
> struct cdev heap_cdev;
> + struct device *dev;
> };
>
> static LIST_HEAD(heap_list);
> @@ -49,6 +51,33 @@ static dev_t dma_heap_devt;
> static struct class *dma_heap_class;
> static DEFINE_XARRAY_ALLOC(dma_heap_minors);
>
> +int dma_heap_create_device_link(struct device *dev, const char *heap)
> +{
> + struct dma_heap *h;
> +
> + /* check the name is valid */
> + mutex_lock(&heap_list_lock);
> + list_for_each_entry(h, &heap_list, list) {
> + if (!strcmp(h->name, heap))
> + break;
> + }
> + mutex_unlock(&heap_list_lock);
> +
> + if (list_entry_is_head(h, &heap_list, list)) {
> + pr_err("dma_heap: Link to invalid heap requested %s\n", heap);
> + return -EINVAL;
> + }
> +
> + return sysfs_create_link(&dev->kobj, &h->dev->kobj, DEVNAME);
> +}
So, one concern with this (if I'm reading this right) is it seems like
a single heap link may be insufficient.
There may be multiple heaps that a driver could work with (especially
if the device isn't very constrained), so when sharing a buffer with a
second device that is more constrained we'd have the same problem we
have now where userspace just needs to know which device is the more
constrained one to allocate for.
So it might be useful to have a sysfs "dma_heaps" directory with the
supported heaps linked from there? That way userland could find across
the various devices the heap-link that was common.
This still has the downside that every driver needs to be aware of
every heap, and when new heaps are added, it may take awhile before
folks might be able to assess if a device is compatible or not to
update it, so I suspect we'll have eventually some loose
constraint-based helpers to register links. But I think this at least
moves us in a workable direction, so again, I'm really glad to see you
send this out!
thanks
-john
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-23 13:55 ` DMA-heap driver hints Laurent Pinchart
2023-01-23 16:29 ` Christian König
@ 2023-01-24 5:07 ` John Stultz
1 sibling, 0 replies; 18+ messages in thread
From: John Stultz @ 2023-01-24 5:07 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linaro-mm-sig, sebastian.wick, benjamin.gaignard, linux-media,
Christian König, mchehab, dri-devel, nicolas, hverkuil,
ppaalanen, James Jones, tfiga, sumit.semwal, T.J. Mercier
On Mon, Jan 23, 2023 at 5:55 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Christian,
>
> CC'ing James as I think this is related to his work on the unix device
> memory allocator ([1]).
>
> [1] https://lore.kernel.org/dri-devel/8b555674-1c5b-c791-4547-2ea7c16aee6c@nvidia.com/
>
> On Mon, Jan 23, 2023 at 01:37:54PM +0100, Christian König wrote:
> > Hi guys,
> >
> > this is just an RFC! The last time we discussed the DMA-buf coherency
> > problem [1] we concluded that DMA-heap first needs a better way to
> > communicate to userspace which heap to use for a certain device.
> >
> > As far as I know userspace currently just hard codes that information
> > which is certainly not desirable considering that we should have this
> > inside the kernel as well.
> >
> > So what those two patches here do is to first add some
> > dma_heap_create_device_link() and dma_heap_remove_device_link()
> > function and then demonstrating the functionality with uvcvideo
> > driver.
> >
> > The preferred DMA-heap is represented with a symlink in sysfs between
> > the device and the virtual DMA-heap device node.
>
> I'll start with a few high-level comments/questions:
>
> - Instead of tying drivers to heaps, have you considered a system where
> a driver would expose constraints, and a heap would then be selected
> based on those constraints ? A tight coupling between heaps and
> drivers means downstream patches to drivers in order to use
> vendor-specific heaps, that sounds painful.
Though, maybe it should be in that case. More motivation to get your
heap (and its users) upstream. :)
> A constraint-based system would also, I think, be easier to extend
> with additional constraints in the future.
I think the issue of enumerating and exposing constraints to userland
is just really tough. While on any one system there is a fixed number
of constraints, it's not clear we could come up with a bounded set for
all systems.
To avoid this back in the ION days I had proposed an idea of userland
having devices share an opaque constraint cookie, which userland could
mask together between devices and then find a heap that matches the
combined cookie, which would avoid exposing specific constraints to
userland, but the processes of using it seemed like such a mess to
explain.
So I think this driver driven links approach is pretty reasonable. I
do worry we might get situations where the drivers ability to use a
heap depends on some other factor (dts iommu setup maybe?), which the
driver might not know on its own, but I think having the driver
special-case that to resolve it would be doable.
> - I assume some drivers will be able to support multiple heaps. How do
> you envision this being implemented ?
Yeah. I also agree we need to have multiple heap links.
> - Devices could have different constraints based on particular
> configurations. For instance, a device may require specific memory
> layout for multi-planar YUV formats only (as in allocating the Y and C
> planes of NV12 from different memory banks). A dynamic API may thus be
> needed (but may also be very painful to use from userspace).
Yeah. While I know folks really don't like the static userspace config
model that Android uses, I do fret that once we get past what a
workable heap is, it still won't address what the ideal heap is.
For instance, we might find that the system heap works for a given
pipeline, but because the cpu doesn't use the buffer in one case, the
system-uncached heap is really the best choice for performance. But in
another pipeline with the same devices, if the cpu is reading and
writing the buffer quite a bit, one would want the standard system
heap.
Because userland is the only one who can know the path a buffer will
take, userland is really the best place to choose the ideal allocation
type.
So while I don't object to this link based approach just to allow a
generic userland to find a working buffer type for a given set of
devices, I don't think it will be able to replace having device
specific userland policy (like gralloc), though it's my personal hope
the policy can be formalized to a config file rather then having
device specific binaries.
thanks
-john
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-23 16:29 ` Christian König
2023-01-23 16:58 ` Laurent Pinchart
@ 2023-01-24 5:19 ` John Stultz
2023-01-24 7:15 ` Christian König
1 sibling, 1 reply; 18+ messages in thread
From: John Stultz @ 2023-01-24 5:19 UTC (permalink / raw)
To: Christian König
Cc: James Jones, linaro-mm-sig, sebastian.wick, labbott,
benjamin.gaignard, linux-media, mchehab, dri-devel, nicolas,
hverkuil, ppaalanen, Laurent Pinchart, lmark, tfiga,
sumit.semwal
On Mon, Jan 23, 2023 at 8:29 AM Christian König
<christian.koenig@amd.com> wrote:
> Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
> > - I assume some drivers will be able to support multiple heaps. How do
> > you envision this being implemented ?
>
> I don't really see an use case for this.
>
> We do have some drivers which say: for this use case you can use
> whatever you want, but for that use case you need to use specific memory
> (scan out on GPUs for example works like this).
>
[snipping the constraints argument, which I agree with]
>
> What we do have is compatibility between heaps. E.g. a CMA heap is
> usually compatible with the system heap or might even be a subset of
> another CMA heap. But I wanted to add that as next step to the heaps
> framework itself.
So the difficult question is how is userland supposed to know which
heap is compatible with which?
If you have two devices, one that points to heap "foo" and the other
points to heap "bar", how does userland know that "foo" satisfies the
constraints of "bar" but "bar" doesn't satisfy the constraints of
"foo".
(foo ="cma", bar="system")
I think it would be much better for device 1 to list "foo" and device
2 to list "foo" and "bar", so you can find that "foo" is the common
heap which will solve both devices' needs.
> > - Devices could have different constraints based on particular
> > configurations. For instance, a device may require specific memory
> > layout for multi-planar YUV formats only (as in allocating the Y and C
> > planes of NV12 from different memory banks). A dynamic API may thus be
> > needed (but may also be very painful to use from userspace).
>
> Uff, good to know. But I'm not sure how to expose stuff like that.
Yeah. These edge cases are really hard to solve generically. And
single devices that have separate constraints for different uses are
also not going to be solvable with a simple linking approach.
But I do wonder if a generic solution to all cases is needed
(especially if it really isn't possible)? If we leave the option for
gralloc like omniscient device-specific userland policy, those edge
cases can be handled by those devices that can't run generic logic.
And those devices just won't be able to be supported by generic
distros, hopefully motivating future designs to have less odd
constraints?
thanks
-john
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-24 5:19 ` John Stultz
@ 2023-01-24 7:15 ` Christian König
2023-01-25 18:59 ` John Stultz
0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2023-01-24 7:15 UTC (permalink / raw)
To: John Stultz
Cc: James Jones, linaro-mm-sig, sebastian.wick, labbott,
benjamin.gaignard, linux-media, mchehab, dri-devel, nicolas,
hverkuil, ppaalanen, Laurent Pinchart, lmark, tfiga,
sumit.semwal
Am 24.01.23 um 06:19 schrieb John Stultz:
> On Mon, Jan 23, 2023 at 8:29 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
>>> - I assume some drivers will be able to support multiple heaps. How do
>>> you envision this being implemented ?
>> I don't really see an use case for this.
>>
>> We do have some drivers which say: for this use case you can use
>> whatever you want, but for that use case you need to use specific memory
>> (scan out on GPUs for example works like this).
>>
> [snipping the constraints argument, which I agree with]
>> What we do have is compatibility between heaps. E.g. a CMA heap is
>> usually compatible with the system heap or might even be a subset of
>> another CMA heap. But I wanted to add that as next step to the heaps
>> framework itself.
> So the difficult question is how is userland supposed to know which
> heap is compatible with which?
The heaps should know which other heap they are compatible with.
E.g. the CMA heap should have a link to the system heap because it can
handle all system memory allocations as well.
If we have a specialized CMA heap (for example for 32bit DMA) it should
have a link to the general CMA heap.
> If you have two devices, one that points to heap "foo" and the other
> points to heap "bar", how does userland know that "foo" satisfies the
> constraints of "bar" but "bar" doesn't satisfy the constraints of
> "foo".
> (foo ="cma", bar="system")
>
> I think it would be much better for device 1 to list "foo" and device
> 2 to list "foo" and "bar", so you can find that "foo" is the common
> heap which will solve both devices' needs.
I think that this would be a rather bad idea because then all devices
need to know about all the possible different heaps they are compatible
with.
For example a device which knows that it's compatible with system memory
should only expose that information.
That a CMA heap is also compatible with system memory is irrelevant for
this device and should be handled between the CMA and system heap.
>>> - Devices could have different constraints based on particular
>>> configurations. For instance, a device may require specific memory
>>> layout for multi-planar YUV formats only (as in allocating the Y and C
>>> planes of NV12 from different memory banks). A dynamic API may thus be
>>> needed (but may also be very painful to use from userspace).
>> Uff, good to know. But I'm not sure how to expose stuff like that.
> Yeah. These edge cases are really hard to solve generically. And
> single devices that have separate constraints for different uses are
> also not going to be solvable with a simple linking approach.
>
> But I do wonder if a generic solution to all cases is needed
> (especially if it really isn't possible)? If we leave the option for
> gralloc like omniscient device-specific userland policy, those edge
> cases can be handled by those devices that can't run generic logic.
> And those devices just won't be able to be supported by generic
> distros, hopefully motivating future designs to have less odd
> constraints?
Potentially yes, but I think that anything more complex than "please
allocate from this piece of memory for me" is not something which should
be handled inside the device independent framework.
Especially device specific memory and allocation constrains (e.g. things
like don't put those two things on the same memory channel) is *not*
something we should have in an inter device framework.
In those cases we should just be able to say that an allocation should
be made from a specific device and then let the device specific drivers
deal with the constrain.
Regards,
Christian.
>
> thanks
> -john
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-24 3:56 ` James Jones
@ 2023-01-24 7:48 ` Christian König
2023-01-24 23:14 ` T.J. Mercier
0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2023-01-24 7:48 UTC (permalink / raw)
To: James Jones, Laurent Pinchart
Cc: linaro-mm-sig, sebastian.wick, labbott, benjamin.gaignard,
linux-media, mchehab, ppaalanen, dri-devel, nicolas, hverkuil,
jstultz, lmark, tfiga, sumit.semwal
Am 24.01.23 um 04:56 schrieb James Jones:
> On 1/23/23 08:58, Laurent Pinchart wrote:
>> Hi Christian,
>>
>> On Mon, Jan 23, 2023 at 05:29:18PM +0100, Christian König wrote:
>>> Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
>>>> Hi Christian,
>>>>
>>>> CC'ing James as I think this is related to his work on the unix device
>>>> memory allocator ([1]).
>
> Thank you for including me.
Sorry for not having you in initially. I wasn't aware of your previous
work in this area.
>
>>>> [1]
>>>> https://lore.kernel.org/dri-devel/8b555674-1c5b-c791-4547-2ea7c16aee6c@nvidia.com/
>>>>
>>>> On Mon, Jan 23, 2023 at 01:37:54PM +0100, Christian König wrote:
>>>>> Hi guys,
>>>>>
>>>>> this is just an RFC! The last time we discussed the DMA-buf coherency
>>>>> problem [1] we concluded that DMA-heap first needs a better way to
>>>>> communicate to userspace which heap to use for a certain device.
>>>>>
>>>>> As far as I know userspace currently just hard codes that information
>>>>> which is certainly not desirable considering that we should have this
>>>>> inside the kernel as well.
>>>>>
>>>>> So what those two patches here do is to first add some
>>>>> dma_heap_create_device_link() and dma_heap_remove_device_link()
>>>>> function and then demonstrating the functionality with uvcvideo
>>>>> driver.
>>>>>
>>>>> The preferred DMA-heap is represented with a symlink in sysfs between
>>>>> the device and the virtual DMA-heap device node.
>>>>
>>>> I'll start with a few high-level comments/questions:
>>>>
>>>> - Instead of tying drivers to heaps, have you considered a system
>>>> where
>>>> a driver would expose constraints, and a heap would then be
>>>> selected
>>>> based on those constraints ? A tight coupling between heaps and
>>>> drivers means downstream patches to drivers in order to use
>>>> vendor-specific heaps, that sounds painful.
>>>
>>> I was wondering the same thing as well, but came to the conclusion that
>>> just the other way around is the less painful approach.
>>
>> From a kernel point of view, sure, it's simpler and thus less painful.
>> From the point of view of solving the whole issue, I'm not sure :-)
>>
>>> The problem is that there are so many driver specific constrains that I
>>> don't even know where to start from.
>>
>> That's where I was hoping James would have some feedback for us, based
>> on the work he did on the Unix device memory allocator. If that's not
>> the case, we can brainstorm this from scratch.
>
> Simon Ser's and my presentation from XDC 2020 focused entirely on
> this. The idea was not to try to enumerate every constraint up front,
> but rather to develop an extensible mechanism that would be flexible
> enough to encapsulate many disparate types of constraints and perform
> set operations on them (merging sets was the only operation we tried
> to solve). Simon implemented a prototype header-only library to
> implement the mechanism:
>
> https://gitlab.freedesktop.org/emersion/drm-constraints
>
> The links to the presentation and talk are below, along with notes
> from the follow-up workshop.
>
> https://lpc.events/event/9/contributions/615/attachments/704/1301/XDC_2020__Allocation_Constraints.pdf
>
> https://www.youtube.com/watch?v=HZEClOP5TIk
> https://paste.sr.ht/~emersion/c43b30be08bab1882f1b107402074462bba3b64a
>
> Note one of the hard parts of this was figuring out how to express a
> device or heap within the constraint structs. One of the better ideas
> proposed back then was something like heap IDs, where dma heaps would
> each have one,
We already have that. Each dma_heap has it's own unique name.
> and devices could register their own heaps (or even just themselves?)
> with the heap subsystem and be assigned a locally-unique ID that
> userspace could pass around.
I was more considering that we expose some kind of flag noting that a
certain device needs its buffer allocated from that device to utilize
all use cases.
> This sounds similar to what you're proposing. Perhaps a reasonable
> identifier is a device (major, minor) pair. Such a constraint could be
> expressed as a symlink for easy visualization/discoverability from
> userspace, but might be easier to serialize over the wire as the
> (major, minor) pair. I'm not clear which direction is better to
> express this either: As a link from heap->device, or device->heap.
>
>>>> A constraint-based system would also, I think, be easier to extend
>>>> with additional constraints in the future.
>>>>
>>>> - I assume some drivers will be able to support multiple heaps. How do
>>>> you envision this being implemented ?
>>>
>>> I don't really see an use case for this.
>
> One use case I know of here is same-vendor GPU local memory on
> different GPUs. NVIDIA GPUs have certain things they can only do on
> local memory, certain things they can do on all memory, and certain
> things they can only do on memory local to another NVIDIA GPU,
> especially when there exists an NVLink interface between the two. So
> they'd ideally express different constraints for heap representing
> each of those.
I strongly think that exposing this complexity is overkill. We have
pretty much the same on AMD GPUs with XGMI, but this is so vendor
specific that I'm pretty sure we shouldn't have that in a common framework.
We should concentrate on solving the problem at hand and not trying to
come up with something to complex to be implementable by everybody.
Extensibility is the key here not getting everything handled in the
initial code drop.
>
> The same thing is often true of memory on remote devices that are at
> various points in a PCIe topology. We've had situations where we could
> only get enough bandwidth between two PCIe devices when they were less
> than some number of hops away on the PCI tree. We hard-coded logic to
> detect that in our userspace drivers, but we could instead expose it
> as a constraint on heaps that would express which devices can
> accomplish certain operations as pairs.
>
> Similarly to the last one, I would assume (But haven't yet run into in
> my personal experience) similar limitations arise when you have a NUMA
> memory configuration, if you had a heap representing each NUMA node or
> something, some might have more coherency than others, or might have
> different bandwidth limitations that you could express through
> something like device tree, etc. This is more speculative, but seems
> like a generalization of the above two cases.
>
>>> We do have some drivers which say: for this use case you can use
>>> whatever you want, but for that use case you need to use specific
>>> memory
>>> (scan out on GPUs for example works like this).
>>>
>>> But those specific use cases are exactly that, very specific. And
>>> exposing all the constrains for them inside a kernel UAPI is a futile
>>> effort (at least for the GPU scan out case). In those situations it's
>>> just better to have the allocator in userspace deal with device
>>> specific
>>> stuff.
>>
>> While the exact constraints will certainly be device-specific, is that
>> also true of the type of constraints, or the existence of constraints in
>> the first place ? To give an example, with a video decoder producing
>> frames that are then rendered by a GPU, the tiling format that would
>> produce the best result is device-specific, but the fact that the
>> decoder can produce a tiled format that would work better for the GPU,
>> or a non-tiled format for other consumers, is a very common constraint.
>> I don't think we'll be able to do away completely with the
>> device-specific code in userspace, but I think we should be able to
>> expose constraints in a generic-enough way that many simple use cases
>> will be covered by generic code.
>
> Yes, agreed, the design we proposed took pains to allow
> vendor-specific constraints via a general mechanism. We supported both
> vendor-specific types of constraints, and vendor-specific values for
> general constraints. Some code repository would act as the central
> registry of constraint types, similar to the Linux kernel's
> drm_fourcc.h for modifiers, or the Khronos github repository for
> Vulkan vendor IDs. If the definition needs to be used by the kernel,
> the kernel is the logical repository for that role IMHO.
>
> In our 2020 discussion, there was some debate over whether the kernel
> should expose and/or consume constraints directly, or whether it's
> sufficient to expose lower-level mechanisms from the kernel and keep
> the translation of constraints to the correct mechanism in userspace.
> There are pros/cons to both. I don't know that we bottomed out on that
> part of the discussion, and it could be the right answer is some
> combination of the two, as suggested below.
>
>>> What I want to do is to separate the problem. The kernel only provides
>>> the information where to allocate from, figuring out the details like
>>> how many bytes, which format, plane layout etc.. is still the job of
>>> userspace.
>>
>> Even with UVC, where to allocate memory from will depend on the use
>> case. If the consumer is a device that doesn't support non-contiguous
>> DMA, the system heap won't work.
>>
>> Actually, could you explain why UVC works better with the system heap ?
>> I'm looking at videobuf2 as an importer, and it doesn't call the dmabuf
>> as far as I can tell, so cache management provided by the exporter seems
>> to be bypassed in any case.
>>
>>> What we do have is compatibility between heaps. E.g. a CMA heap is
>>> usually compatible with the system heap or might even be a subset of
>>> another CMA heap. But I wanted to add that as next step to the heaps
>>> framework itself.
>>>
>>>> - Devices could have different constraints based on particular
>>>> configurations. For instance, a device may require specific memory
>>>> layout for multi-planar YUV formats only (as in allocating the
>>>> Y and C
>>>> planes of NV12 from different memory banks). A dynamic API may
>>>> thus be
>>>> needed (but may also be very painful to use from userspace).
>>>
>>> Uff, good to know. But I'm not sure how to expose stuff like that.
>>
>> Let's see if James has anything to share with us :-) With a bit of luck
>> we won't have to start from scratch.
>
> Well, this is the hard example we keep using as a measure of success
> for whatever we come up with. I don't know that someone ever sat down
> and tried to express this in the mechanism Simon and I proposed in
> 2020, but allowing the expression of something that complex was
> certainly our goal. How to resolve it down to an allocation mechanism,
> I believe, was further than we got, but we weren't that well versed in
> DMA heaps back then, or at least I wasn't.
>
>>>>> What's still missing is certainly matching userspace for this since I
>>>>> wanted to discuss the initial kernel approach first.
>>>>
>>>> https://git.libcamera.org/libcamera/libcamera.git/ would be a good
>>>> place
>>>> to prototype userspace support :-)
>>>
>>> Thanks for the pointer and the review,
>>
>> By the way, side question, does anyone know what the status of dma heaps
>> support is in major distributions ? On my Gentoo box,
>> /dev/dma_heap/system is 0600 root:root. That's easy to change for a
>> developer, but not friendly to end-users. If we want to move forward
>> with dma heaps as standard multimedia allocators (and I would really
>> like to see that happening), we have to make sure they can be used.
>
> We seem to have reached a world where display (primary nodes) are
> carefully guarded, and some mildly trusted group of folks (video) can
> access render nodes, but then there's some separate group generally
> for camera/video/v4l and whatnot from what I've seen (I don't survey
> this stuff that often. I live in my developer bubble). I'm curious
> whether the right direction is a broader group that encompasses all of
> render nodes, multimedia, and heaps, or if a more segmented design is
> right. The latter is probably the better design from first principles,
> but might lead to headaches if the permissions diverge.
The main argument is that this memory is not properly accounted, but
this also counts for things like memory file descriptors returned by
memfd_create().
I have proposed multiple times now that we extend the OOM handling to
take memory allocated through file descriptors into account as well, but
I can't find the time to fully upstream this.
T.J. Mercier is working on some memcg based tracking which sounds like
it might resolve this problem as well.
Regards,
Christian.
>
> Thanks,
> -James
>
>>>>> Please take a look and comment.
>>>>>
>>>>> Thanks,
>>>>> Christian.
>>>>>
>>>>> [1]
>>>>> https://lore.kernel.org/all/11a6f97c-e45f-f24b-8a73-48d5a388a2cc@gmail.com/T/
>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-24 7:48 ` Christian König
@ 2023-01-24 23:14 ` T.J. Mercier
2023-01-25 23:20 ` James Jones
0 siblings, 1 reply; 18+ messages in thread
From: T.J. Mercier @ 2023-01-24 23:14 UTC (permalink / raw)
To: Christian König
Cc: hverkuil, sebastian.wick, mchehab, benjamin.gaignard, lmark,
James Jones, dri-devel, nicolas, linaro-mm-sig, ppaalanen,
jstultz, Laurent Pinchart, tfiga, labbott, sumit.semwal,
linux-media
On Mon, Jan 23, 2023 at 11:49 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 24.01.23 um 04:56 schrieb James Jones:
> > On 1/23/23 08:58, Laurent Pinchart wrote:
> >> Hi Christian,
> >>
> >> On Mon, Jan 23, 2023 at 05:29:18PM +0100, Christian König wrote:
> >>> Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
> >>>> Hi Christian,
> >>>>
> >>>> CC'ing James as I think this is related to his work on the unix device
> >>>> memory allocator ([1]).
> >
> > Thank you for including me.
>
> Sorry for not having you in initially. I wasn't aware of your previous
> work in this area.
>
> >
> >>>> [1]
> >>>> https://lore.kernel.org/dri-devel/8b555674-1c5b-c791-4547-2ea7c16aee6c@nvidia.com/
> >>>>
> >>>> On Mon, Jan 23, 2023 at 01:37:54PM +0100, Christian König wrote:
> >>>>> Hi guys,
> >>>>>
> >>>>> this is just an RFC! The last time we discussed the DMA-buf coherency
> >>>>> problem [1] we concluded that DMA-heap first needs a better way to
> >>>>> communicate to userspace which heap to use for a certain device.
> >>>>>
> >>>>> As far as I know userspace currently just hard codes that information
> >>>>> which is certainly not desirable considering that we should have this
> >>>>> inside the kernel as well.
> >>>>>
> >>>>> So what those two patches here do is to first add some
> >>>>> dma_heap_create_device_link() and dma_heap_remove_device_link()
> >>>>> function and then demonstrating the functionality with uvcvideo
> >>>>> driver.
> >>>>>
> >>>>> The preferred DMA-heap is represented with a symlink in sysfs between
> >>>>> the device and the virtual DMA-heap device node.
> >>>>
> >>>> I'll start with a few high-level comments/questions:
> >>>>
> >>>> - Instead of tying drivers to heaps, have you considered a system
> >>>> where
> >>>> a driver would expose constraints, and a heap would then be
> >>>> selected
> >>>> based on those constraints ? A tight coupling between heaps and
> >>>> drivers means downstream patches to drivers in order to use
> >>>> vendor-specific heaps, that sounds painful.
> >>>
> >>> I was wondering the same thing as well, but came to the conclusion that
> >>> just the other way around is the less painful approach.
> >>
> >> From a kernel point of view, sure, it's simpler and thus less painful.
> >> From the point of view of solving the whole issue, I'm not sure :-)
> >>
> >>> The problem is that there are so many driver specific constrains that I
> >>> don't even know where to start from.
> >>
> >> That's where I was hoping James would have some feedback for us, based
> >> on the work he did on the Unix device memory allocator. If that's not
> >> the case, we can brainstorm this from scratch.
> >
> > Simon Ser's and my presentation from XDC 2020 focused entirely on
> > this. The idea was not to try to enumerate every constraint up front,
> > but rather to develop an extensible mechanism that would be flexible
> > enough to encapsulate many disparate types of constraints and perform
> > set operations on them (merging sets was the only operation we tried
> > to solve). Simon implemented a prototype header-only library to
> > implement the mechanism:
> >
> > https://gitlab.freedesktop.org/emersion/drm-constraints
> >
> > The links to the presentation and talk are below, along with notes
> > from the follow-up workshop.
> >
> > https://lpc.events/event/9/contributions/615/attachments/704/1301/XDC_2020__Allocation_Constraints.pdf
> >
> > https://www.youtube.com/watch?v=HZEClOP5TIk
> > https://paste.sr.ht/~emersion/c43b30be08bab1882f1b107402074462bba3b64a
> >
> > Note one of the hard parts of this was figuring out how to express a
> > device or heap within the constraint structs. One of the better ideas
> > proposed back then was something like heap IDs, where dma heaps would
> > each have one,
>
> We already have that. Each dma_heap has it's own unique name.
>
> > and devices could register their own heaps (or even just themselves?)
> > with the heap subsystem and be assigned a locally-unique ID that
> > userspace could pass around.
>
> I was more considering that we expose some kind of flag noting that a
> certain device needs its buffer allocated from that device to utilize
> all use cases.
>
> > This sounds similar to what you're proposing. Perhaps a reasonable
> > identifier is a device (major, minor) pair. Such a constraint could be
> > expressed as a symlink for easy visualization/discoverability from
> > userspace, but might be easier to serialize over the wire as the
> > (major, minor) pair. I'm not clear which direction is better to
> > express this either: As a link from heap->device, or device->heap.
> >
> >>>> A constraint-based system would also, I think, be easier to extend
> >>>> with additional constraints in the future.
> >>>>
> >>>> - I assume some drivers will be able to support multiple heaps. How do
> >>>> you envision this being implemented ?
> >>>
> >>> I don't really see an use case for this.
> >
> > One use case I know of here is same-vendor GPU local memory on
> > different GPUs. NVIDIA GPUs have certain things they can only do on
> > local memory, certain things they can do on all memory, and certain
> > things they can only do on memory local to another NVIDIA GPU,
> > especially when there exists an NVLink interface between the two. So
> > they'd ideally express different constraints for heap representing
> > each of those.
>
> I strongly think that exposing this complexity is overkill. We have
> pretty much the same on AMD GPUs with XGMI, but this is so vendor
> specific that I'm pretty sure we shouldn't have that in a common framework.
>
> We should concentrate on solving the problem at hand and not trying to
> come up with something to complex to be implementable by everybody.
> Extensibility is the key here not getting everything handled in the
> initial code drop.
>
> >
> > The same thing is often true of memory on remote devices that are at
> > various points in a PCIe topology. We've had situations where we could
> > only get enough bandwidth between two PCIe devices when they were less
> > than some number of hops away on the PCI tree. We hard-coded logic to
> > detect that in our userspace drivers, but we could instead expose it
> > as a constraint on heaps that would express which devices can
> > accomplish certain operations as pairs.
> >
> > Similarly to the last one, I would assume (But haven't yet run into in
> > my personal experience) similar limitations arise when you have a NUMA
> > memory configuration, if you had a heap representing each NUMA node or
> > something, some might have more coherency than others, or might have
> > different bandwidth limitations that you could express through
> > something like device tree, etc. This is more speculative, but seems
> > like a generalization of the above two cases.
> >
> >>> We do have some drivers which say: for this use case you can use
> >>> whatever you want, but for that use case you need to use specific
> >>> memory
> >>> (scan out on GPUs for example works like this).
> >>>
> >>> But those specific use cases are exactly that, very specific. And
> >>> exposing all the constrains for them inside a kernel UAPI is a futile
> >>> effort (at least for the GPU scan out case). In those situations it's
> >>> just better to have the allocator in userspace deal with device
> >>> specific
> >>> stuff.
> >>
> >> While the exact constraints will certainly be device-specific, is that
> >> also true of the type of constraints, or the existence of constraints in
> >> the first place ? To give an example, with a video decoder producing
> >> frames that are then rendered by a GPU, the tiling format that would
> >> produce the best result is device-specific, but the fact that the
> >> decoder can produce a tiled format that would work better for the GPU,
> >> or a non-tiled format for other consumers, is a very common constraint.
> >> I don't think we'll be able to do away completely with the
> >> device-specific code in userspace, but I think we should be able to
> >> expose constraints in a generic-enough way that many simple use cases
> >> will be covered by generic code.
> >
> > Yes, agreed, the design we proposed took pains to allow
> > vendor-specific constraints via a general mechanism. We supported both
> > vendor-specific types of constraints, and vendor-specific values for
> > general constraints. Some code repository would act as the central
> > registry of constraint types, similar to the Linux kernel's
> > drm_fourcc.h for modifiers, or the Khronos github repository for
> > Vulkan vendor IDs. If the definition needs to be used by the kernel,
> > the kernel is the logical repository for that role IMHO.
> >
> > In our 2020 discussion, there was some debate over whether the kernel
> > should expose and/or consume constraints directly, or whether it's
> > sufficient to expose lower-level mechanisms from the kernel and keep
> > the translation of constraints to the correct mechanism in userspace.
> > There are pros/cons to both. I don't know that we bottomed out on that
> > part of the discussion, and it could be the right answer is some
> > combination of the two, as suggested below.
> >
> >>> What I want to do is to separate the problem. The kernel only provides
> >>> the information where to allocate from, figuring out the details like
> >>> how many bytes, which format, plane layout etc.. is still the job of
> >>> userspace.
> >>
> >> Even with UVC, where to allocate memory from will depend on the use
> >> case. If the consumer is a device that doesn't support non-contiguous
> >> DMA, the system heap won't work.
> >>
> >> Actually, could you explain why UVC works better with the system heap ?
> >> I'm looking at videobuf2 as an importer, and it doesn't call the dmabuf
> >> as far as I can tell, so cache management provided by the exporter seems
> >> to be bypassed in any case.
> >>
> >>> What we do have is compatibility between heaps. E.g. a CMA heap is
> >>> usually compatible with the system heap or might even be a subset of
> >>> another CMA heap. But I wanted to add that as next step to the heaps
> >>> framework itself.
> >>>
> >>>> - Devices could have different constraints based on particular
> >>>> configurations. For instance, a device may require specific memory
> >>>> layout for multi-planar YUV formats only (as in allocating the
> >>>> Y and C
> >>>> planes of NV12 from different memory banks). A dynamic API may
> >>>> thus be
> >>>> needed (but may also be very painful to use from userspace).
> >>>
> >>> Uff, good to know. But I'm not sure how to expose stuff like that.
> >>
> >> Let's see if James has anything to share with us :-) With a bit of luck
> >> we won't have to start from scratch.
> >
> > Well, this is the hard example we keep using as a measure of success
> > for whatever we come up with. I don't know that someone ever sat down
> > and tried to express this in the mechanism Simon and I proposed in
> > 2020, but allowing the expression of something that complex was
> > certainly our goal. How to resolve it down to an allocation mechanism,
> > I believe, was further than we got, but we weren't that well versed in
> > DMA heaps back then, or at least I wasn't.
> >
> >>>>> What's still missing is certainly matching userspace for this since I
> >>>>> wanted to discuss the initial kernel approach first.
> >>>>
> >>>> https://git.libcamera.org/libcamera/libcamera.git/ would be a good
> >>>> place
> >>>> to prototype userspace support :-)
> >>>
> >>> Thanks for the pointer and the review,
> >>
> >> By the way, side question, does anyone know what the status of dma heaps
> >> support is in major distributions ? On my Gentoo box,
> >> /dev/dma_heap/system is 0600 root:root. That's easy to change for a
> >> developer, but not friendly to end-users. If we want to move forward
> >> with dma heaps as standard multimedia allocators (and I would really
> >> like to see that happening), we have to make sure they can be used.
> >
> > We seem to have reached a world where display (primary nodes) are
> > carefully guarded, and some mildly trusted group of folks (video) can
> > access render nodes, but then there's some separate group generally
> > for camera/video/v4l and whatnot from what I've seen (I don't survey
> > this stuff that often. I live in my developer bubble). I'm curious
> > whether the right direction is a broader group that encompasses all of
> > render nodes, multimedia, and heaps, or if a more segmented design is
> > right. The latter is probably the better design from first principles,
> > but might lead to headaches if the permissions diverge.
>
> The main argument is that this memory is not properly accounted, but
> this also counts for things like memory file descriptors returned by
> memfd_create().
>
> I have proposed multiple times now that we extend the OOM handling to
> take memory allocated through file descriptors into account as well, but
> I can't find the time to fully upstream this.
>
> T.J. Mercier is working on some memcg based tracking which sounds like
> it might resolve this problem as well.
>
Gosh I hope so. How Android currently does this is with heavy use of
sepolicy to control access to the individual heaps, sometimes even at
a per-application/service level:
raven:/dev/dma_heap # ls -lZ
total 0
cr--r--r-- 1 system audio u:object_r:dmabuf_heap_device:s0
248, 15 2023-01-23 16:14 aaudio_capture_heap
cr--r--r-- 1 system audio u:object_r:dmabuf_heap_device:s0
248, 14 2023-01-23 16:14 aaudio_playback_heap
cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
248, 3 2023-01-23 16:14 faceauth_tpu-secure
cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
248, 4 2023-01-23 16:14 faimg-secure
cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
248, 7 2023-01-23 16:14 famodel-secure
cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
248, 6 2023-01-23 16:14 faprev-secure
cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
248, 5 2023-01-23 16:14 farawimg-secure
cr--r--r-- 1 system graphics u:object_r:sensor_direct_heap_device:s0
248, 13 2023-01-23 16:14 sensor_direct_heap
cr--r--r-- 1 system system u:object_r:dmabuf_system_heap_device:s0
248, 9 2023-01-23 16:14 system
cr--r--r-- 1 system system u:object_r:dmabuf_system_heap_device:s0
248, 10 2023-01-23 16:14 system-uncached
cr--r--r-- 1 system graphics u:object_r:dmabuf_heap_device:s0
248, 8 2023-01-23 16:14 tui-secure
cr--r--r-- 1 system drmrpc
u:object_r:dmabuf_system_secure_heap_device:s0 248, 1 2023-01-23
16:14 vframe-secure
cr--r--r-- 1 system drmrpc u:object_r:dmabuf_heap_device:s0
248, 11 2023-01-23 16:14 video_system
cr--r--r-- 1 system drmrpc u:object_r:dmabuf_heap_device:s0
248, 12 2023-01-23 16:14 video_system-uncached
cr--r--r-- 1 system graphics u:object_r:vscaler_heap_device:s0
248, 2 2023-01-23 16:14 vscaler-secure
cr--r--r-- 1 system drmrpc
u:object_r:dmabuf_system_secure_heap_device:s0 248, 0 2023-01-23
16:14 vstream-secure
I hope we can get to a point where we don't actually need to protect
anything but the unicorns. One of my earlier attempts involved a
counter for each heap that would allow you to limit each one
individually, but that's not what's proposed now.
> Regards,
> Christian.
>
>
> >
> > Thanks,
> > -James
> >
> >>>>> Please take a look and comment.
> >>>>>
> >>>>> Thanks,
> >>>>> Christian.
> >>>>>
> >>>>> [1]
> >>>>> https://lore.kernel.org/all/11a6f97c-e45f-f24b-8a73-48d5a388a2cc@gmail.com/T/
> >>
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-24 7:15 ` Christian König
@ 2023-01-25 18:59 ` John Stultz
0 siblings, 0 replies; 18+ messages in thread
From: John Stultz @ 2023-01-25 18:59 UTC (permalink / raw)
To: Christian König
Cc: James Jones, linaro-mm-sig, sebastian.wick, labbott,
benjamin.gaignard, linux-media, mchehab, dri-devel, nicolas,
hverkuil, ppaalanen, Laurent Pinchart, lmark, tfiga,
sumit.semwal
Sorry for the delay, this was almost ready to send, but then got
forgotten in my drafts folder.
On Mon, Jan 23, 2023 at 11:15 PM Christian König
<christian.koenig@amd.com> wrote:
> Am 24.01.23 um 06:19 schrieb John Stultz:
> > On Mon, Jan 23, 2023 at 8:29 AM Christian König
> > <christian.koenig@amd.com> wrote:
> >> Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
> >>> - I assume some drivers will be able to support multiple heaps. How do
> >>> you envision this being implemented ?
> >> I don't really see an use case for this.
> >>
> >> We do have some drivers which say: for this use case you can use
> >> whatever you want, but for that use case you need to use specific memory
> >> (scan out on GPUs for example works like this).
> >>
> > [snipping the constraints argument, which I agree with]
> >> What we do have is compatibility between heaps. E.g. a CMA heap is
> >> usually compatible with the system heap or might even be a subset of
> >> another CMA heap. But I wanted to add that as next step to the heaps
> >> framework itself.
> > So the difficult question is how is userland supposed to know which
> > heap is compatible with which?
>
> The heaps should know which other heap they are compatible with.
>
> E.g. the CMA heap should have a link to the system heap because it can
> handle all system memory allocations as well.
>
> If we have a specialized CMA heap (for example for 32bit DMA) it should
> have a link to the general CMA heap.
This is an interesting idea, but it seems to assume a linear or at
least converging "compatibility" order, which I don't think is always
the case.
(For instance, there may be secure heaps which a small set of devices
have access to, but supporting secure memory doesn't imply system
memory for all devices or vice versa).
So I really think being able to express support for multiple heaps
would be important to resolve the majority of these edge cases.
Also to have a single link ordering, it means the constraints have to
go from the heap that satisfies more constraints to the heap that
satisfies less (which is sort of reverse of how I'd think of
compatibility). Which makes the solving logic for userland doable, but
somewhat complex/non-intuitive (as you're searching for the most
"satisfying" heap from the set which will be one of the starting
points).
Whereas finding the intersection of lists seems a bit more straightforward.
> > If you have two devices, one that points to heap "foo" and the other
> > points to heap "bar", how does userland know that "foo" satisfies the
> > constraints of "bar" but "bar" doesn't satisfy the constraints of
> > "foo".
> > (foo ="cma", bar="system")
> >
> > I think it would be much better for device 1 to list "foo" and device
> > 2 to list "foo" and "bar", so you can find that "foo" is the common
> > heap which will solve both devices' needs.
>
> I think that this would be a rather bad idea because then all devices
> need to know about all the possible different heaps they are compatible
> with.
I agree it is somewhat burdensome, but I suspect we'd eventually want
registration helpers to abstract out some of the relationships you
mention above (ie: system supporting devices will accept CMA buffers,
dma32 buffers, etc). But at least that logic would be in-kernel and
not exposed to userland.
> >>> - Devices could have different constraints based on particular
> >>> configurations. For instance, a device may require specific memory
> >>> layout for multi-planar YUV formats only (as in allocating the Y and C
> >>> planes of NV12 from different memory banks). A dynamic API may thus be
> >>> needed (but may also be very painful to use from userspace).
> >> Uff, good to know. But I'm not sure how to expose stuff like that.
> > Yeah. These edge cases are really hard to solve generically. And
> > single devices that have separate constraints for different uses are
> > also not going to be solvable with a simple linking approach.
> >
> > But I do wonder if a generic solution to all cases is needed
> > (especially if it really isn't possible)? If we leave the option for
> > gralloc like omniscient device-specific userland policy, those edge
> > cases can be handled by those devices that can't run generic logic.
> > And those devices just won't be able to be supported by generic
> > distros, hopefully motivating future designs to have less odd
> > constraints?
>
> Potentially yes, but I think that anything more complex than "please
> allocate from this piece of memory for me" is not something which should
> be handled inside the device independent framework.
>
> Especially device specific memory and allocation constrains (e.g. things
> like don't put those two things on the same memory channel) is *not*
> something we should have in an inter device framework.
>
> In those cases we should just be able to say that an allocation should
> be made from a specific device and then let the device specific drivers
> deal with the constrain.
Yeah. I don't think we can get away from needing omniscient userland,
but hopefully we can solve a large chunk of the issue with something
like your approach.
thanks
-john
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: DMA-heap driver hints
2023-01-24 23:14 ` T.J. Mercier
@ 2023-01-25 23:20 ` James Jones
0 siblings, 0 replies; 18+ messages in thread
From: James Jones @ 2023-01-25 23:20 UTC (permalink / raw)
To: T.J. Mercier, Christian König
Cc: hverkuil, sebastian.wick, mchehab, benjamin.gaignard, lmark,
dri-devel, nicolas, linaro-mm-sig, ppaalanen, jstultz,
Laurent Pinchart, tfiga, labbott, sumit.semwal, linux-media
On 1/24/23 15:14, T.J. Mercier wrote:
> On Mon, Jan 23, 2023 at 11:49 PM Christian König
> <christian.koenig@amd.com> wrote:
>>
>> Am 24.01.23 um 04:56 schrieb James Jones:
>>> On 1/23/23 08:58, Laurent Pinchart wrote:
>>>> Hi Christian,
>>>>
>>>> On Mon, Jan 23, 2023 at 05:29:18PM +0100, Christian König wrote:
>>>>> Am 23.01.23 um 14:55 schrieb Laurent Pinchart:
>>>>>> Hi Christian,
>>>>>>
>>>>>> CC'ing James as I think this is related to his work on the unix device
>>>>>> memory allocator ([1]).
>>>
>>> Thank you for including me.
>>
>> Sorry for not having you in initially. I wasn't aware of your previous
>> work in this area.
No worries. I've embarrassingly made no progress here since the last XDC
talk, so I wouldn't expect everyone to know or remember.
>>>
>>>>>> [1]
>>>>>> https://lore.kernel.org/dri-devel/8b555674-1c5b-c791-4547-2ea7c16aee6c@nvidia.com/
>>>>>>
>>>>>> On Mon, Jan 23, 2023 at 01:37:54PM +0100, Christian König wrote:
>>>>>>> Hi guys,
>>>>>>>
>>>>>>> this is just an RFC! The last time we discussed the DMA-buf coherency
>>>>>>> problem [1] we concluded that DMA-heap first needs a better way to
>>>>>>> communicate to userspace which heap to use for a certain device.
>>>>>>>
>>>>>>> As far as I know userspace currently just hard codes that information
>>>>>>> which is certainly not desirable considering that we should have this
>>>>>>> inside the kernel as well.
>>>>>>>
>>>>>>> So what those two patches here do is to first add some
>>>>>>> dma_heap_create_device_link() and dma_heap_remove_device_link()
>>>>>>> function and then demonstrating the functionality with uvcvideo
>>>>>>> driver.
>>>>>>>
>>>>>>> The preferred DMA-heap is represented with a symlink in sysfs between
>>>>>>> the device and the virtual DMA-heap device node.
>>>>>>
>>>>>> I'll start with a few high-level comments/questions:
>>>>>>
>>>>>> - Instead of tying drivers to heaps, have you considered a system
>>>>>> where
>>>>>> a driver would expose constraints, and a heap would then be
>>>>>> selected
>>>>>> based on those constraints ? A tight coupling between heaps and
>>>>>> drivers means downstream patches to drivers in order to use
>>>>>> vendor-specific heaps, that sounds painful.
>>>>>
>>>>> I was wondering the same thing as well, but came to the conclusion that
>>>>> just the other way around is the less painful approach.
>>>>
>>>> From a kernel point of view, sure, it's simpler and thus less painful.
>>>> From the point of view of solving the whole issue, I'm not sure :-)
>>>>
>>>>> The problem is that there are so many driver specific constrains that I
>>>>> don't even know where to start from.
>>>>
>>>> That's where I was hoping James would have some feedback for us, based
>>>> on the work he did on the Unix device memory allocator. If that's not
>>>> the case, we can brainstorm this from scratch.
>>>
>>> Simon Ser's and my presentation from XDC 2020 focused entirely on
>>> this. The idea was not to try to enumerate every constraint up front,
>>> but rather to develop an extensible mechanism that would be flexible
>>> enough to encapsulate many disparate types of constraints and perform
>>> set operations on them (merging sets was the only operation we tried
>>> to solve). Simon implemented a prototype header-only library to
>>> implement the mechanism:
>>>
>>> https://gitlab.freedesktop.org/emersion/drm-constraints
>>>
>>> The links to the presentation and talk are below, along with notes
>>> from the follow-up workshop.
>>>
>>> https://lpc.events/event/9/contributions/615/attachments/704/1301/XDC_2020__Allocation_Constraints.pdf
>>>
>>> https://www.youtube.com/watch?v=HZEClOP5TIk
>>> https://paste.sr.ht/~emersion/c43b30be08bab1882f1b107402074462bba3b64a
>>>
>>> Note one of the hard parts of this was figuring out how to express a
>>> device or heap within the constraint structs. One of the better ideas
>>> proposed back then was something like heap IDs, where dma heaps would
>>> each have one,
>>
>> We already have that. Each dma_heap has it's own unique name.
Cool.
>>> and devices could register their own heaps (or even just themselves?)
>>> with the heap subsystem and be assigned a locally-unique ID that
>>> userspace could pass around.
>>
>> I was more considering that we expose some kind of flag noting that a
>> certain device needs its buffer allocated from that device to utilize
>> all use cases.
>>
>>> This sounds similar to what you're proposing. Perhaps a reasonable
>>> identifier is a device (major, minor) pair. Such a constraint could be
>>> expressed as a symlink for easy visualization/discoverability from
>>> userspace, but might be easier to serialize over the wire as the
>>> (major, minor) pair. I'm not clear which direction is better to
>>> express this either: As a link from heap->device, or device->heap.
>>>
>>>>>> A constraint-based system would also, I think, be easier to extend
>>>>>> with additional constraints in the future.
>>>>>>
>>>>>> - I assume some drivers will be able to support multiple heaps. How do
>>>>>> you envision this being implemented ?
>>>>>
>>>>> I don't really see an use case for this.
>>>
>>> One use case I know of here is same-vendor GPU local memory on
>>> different GPUs. NVIDIA GPUs have certain things they can only do on
>>> local memory, certain things they can do on all memory, and certain
>>> things they can only do on memory local to another NVIDIA GPU,
>>> especially when there exists an NVLink interface between the two. So
>>> they'd ideally express different constraints for heap representing
>>> each of those.
>>
>> I strongly think that exposing this complexity is overkill. We have
>> pretty much the same on AMD GPUs with XGMI, but this is so vendor
>> specific that I'm pretty sure we shouldn't have that in a common framework.
I disagree with this. That works for cases where all the devices in
question for a given operation are owned by one vendor, but falls apart
as soon as you want to try to allocate memory that works with some USB
camera as well. Then you've no way to express that, or to even know what
component to ask.
>> We should concentrate on solving the problem at hand and not trying to
>> come up with something to complex to be implementable by everybody.
>> Extensibility is the key here not getting everything handled in the
>> initial code drop.
I agree with the part about solving the problems at hand, and don't see
any harm in adding a hint as you propose as an intermediate step.
However, I think it's worth keeping the harder problems in mind, if only
to guide our interim solutions and to help us better understand what
kind of extensibility might be needed to enable future solutions.
Furthermore, while I know many disagree and it has its own risks, I
think it's often worth solving problems no one has brought to you yet.
When solutions are developed in as general a manner as possible, rather
than focusing on a particular use case, it enables others to innovate
and build as-yet unimagined things without being blocked by a subpar
interface in some level of the stack where they have no expertise. I've
been bitten time and again by taking a shortcut because I thought it
would never affect anyone, only to have someone file a bug a few years
later explaining why they need the exact thing I left out.
>>>
>>> The same thing is often true of memory on remote devices that are at
>>> various points in a PCIe topology. We've had situations where we could
>>> only get enough bandwidth between two PCIe devices when they were less
>>> than some number of hops away on the PCI tree. We hard-coded logic to
>>> detect that in our userspace drivers, but we could instead expose it
>>> as a constraint on heaps that would express which devices can
>>> accomplish certain operations as pairs.
>>>
>>> Similarly to the last one, I would assume (But haven't yet run into in
>>> my personal experience) similar limitations arise when you have a NUMA
>>> memory configuration, if you had a heap representing each NUMA node or
>>> something, some might have more coherency than others, or might have
>>> different bandwidth limitations that you could express through
>>> something like device tree, etc. This is more speculative, but seems
>>> like a generalization of the above two cases.
>>>
>>>>> We do have some drivers which say: for this use case you can use
>>>>> whatever you want, but for that use case you need to use specific
>>>>> memory
>>>>> (scan out on GPUs for example works like this).
>>>>>
>>>>> But those specific use cases are exactly that, very specific. And
>>>>> exposing all the constrains for them inside a kernel UAPI is a futile
>>>>> effort (at least for the GPU scan out case). In those situations it's
>>>>> just better to have the allocator in userspace deal with device
>>>>> specific
>>>>> stuff.
>>>>
>>>> While the exact constraints will certainly be device-specific, is that
>>>> also true of the type of constraints, or the existence of constraints in
>>>> the first place ? To give an example, with a video decoder producing
>>>> frames that are then rendered by a GPU, the tiling format that would
>>>> produce the best result is device-specific, but the fact that the
>>>> decoder can produce a tiled format that would work better for the GPU,
>>>> or a non-tiled format for other consumers, is a very common constraint.
>>>> I don't think we'll be able to do away completely with the
>>>> device-specific code in userspace, but I think we should be able to
>>>> expose constraints in a generic-enough way that many simple use cases
>>>> will be covered by generic code.
>>>
>>> Yes, agreed, the design we proposed took pains to allow
>>> vendor-specific constraints via a general mechanism. We supported both
>>> vendor-specific types of constraints, and vendor-specific values for
>>> general constraints. Some code repository would act as the central
>>> registry of constraint types, similar to the Linux kernel's
>>> drm_fourcc.h for modifiers, or the Khronos github repository for
>>> Vulkan vendor IDs. If the definition needs to be used by the kernel,
>>> the kernel is the logical repository for that role IMHO.
>>>
>>> In our 2020 discussion, there was some debate over whether the kernel
>>> should expose and/or consume constraints directly, or whether it's
>>> sufficient to expose lower-level mechanisms from the kernel and keep
>>> the translation of constraints to the correct mechanism in userspace.
>>> There are pros/cons to both. I don't know that we bottomed out on that
>>> part of the discussion, and it could be the right answer is some
>>> combination of the two, as suggested below.
>>>
>>>>> What I want to do is to separate the problem. The kernel only provides
>>>>> the information where to allocate from, figuring out the details like
>>>>> how many bytes, which format, plane layout etc.. is still the job of
>>>>> userspace.
>>>>
>>>> Even with UVC, where to allocate memory from will depend on the use
>>>> case. If the consumer is a device that doesn't support non-contiguous
>>>> DMA, the system heap won't work.
>>>>
>>>> Actually, could you explain why UVC works better with the system heap ?
>>>> I'm looking at videobuf2 as an importer, and it doesn't call the dmabuf
>>>> as far as I can tell, so cache management provided by the exporter seems
>>>> to be bypassed in any case.
>>>>
>>>>> What we do have is compatibility between heaps. E.g. a CMA heap is
>>>>> usually compatible with the system heap or might even be a subset of
>>>>> another CMA heap. But I wanted to add that as next step to the heaps
>>>>> framework itself.
>>>>>
>>>>>> - Devices could have different constraints based on particular
>>>>>> configurations. For instance, a device may require specific memory
>>>>>> layout for multi-planar YUV formats only (as in allocating the
>>>>>> Y and C
>>>>>> planes of NV12 from different memory banks). A dynamic API may
>>>>>> thus be
>>>>>> needed (but may also be very painful to use from userspace).
>>>>>
>>>>> Uff, good to know. But I'm not sure how to expose stuff like that.
>>>>
>>>> Let's see if James has anything to share with us :-) With a bit of luck
>>>> we won't have to start from scratch.
>>>
>>> Well, this is the hard example we keep using as a measure of success
>>> for whatever we come up with. I don't know that someone ever sat down
>>> and tried to express this in the mechanism Simon and I proposed in
>>> 2020, but allowing the expression of something that complex was
>>> certainly our goal. How to resolve it down to an allocation mechanism,
>>> I believe, was further than we got, but we weren't that well versed in
>>> DMA heaps back then, or at least I wasn't.
>>>
>>>>>>> What's still missing is certainly matching userspace for this since I
>>>>>>> wanted to discuss the initial kernel approach first.
>>>>>>
>>>>>> https://git.libcamera.org/libcamera/libcamera.git/ would be a good
>>>>>> place
>>>>>> to prototype userspace support :-)
>>>>>
>>>>> Thanks for the pointer and the review,
>>>>
>>>> By the way, side question, does anyone know what the status of dma heaps
>>>> support is in major distributions ? On my Gentoo box,
>>>> /dev/dma_heap/system is 0600 root:root. That's easy to change for a
>>>> developer, but not friendly to end-users. If we want to move forward
>>>> with dma heaps as standard multimedia allocators (and I would really
>>>> like to see that happening), we have to make sure they can be used.
>>>
>>> We seem to have reached a world where display (primary nodes) are
>>> carefully guarded, and some mildly trusted group of folks (video) can
>>> access render nodes, but then there's some separate group generally
>>> for camera/video/v4l and whatnot from what I've seen (I don't survey
>>> this stuff that often. I live in my developer bubble). I'm curious
>>> whether the right direction is a broader group that encompasses all of
>>> render nodes, multimedia, and heaps, or if a more segmented design is
>>> right. The latter is probably the better design from first principles,
>>> but might lead to headaches if the permissions diverge.
>>
>> The main argument is that this memory is not properly accounted, but
>> this also counts for things like memory file descriptors returned by
>> memfd_create().
>>
>> I have proposed multiple times now that we extend the OOM handling to
>> take memory allocated through file descriptors into account as well, but
>> I can't find the time to fully upstream this.
>>
>> T.J. Mercier is working on some memcg based tracking which sounds like
>> it might resolve this problem as well.
>>
> Gosh I hope so. How Android currently does this is with heavy use of
> sepolicy to control access to the individual heaps, sometimes even at
> a per-application/service level:
>
> raven:/dev/dma_heap # ls -lZ
> total 0
> cr--r--r-- 1 system audio u:object_r:dmabuf_heap_device:s0
> 248, 15 2023-01-23 16:14 aaudio_capture_heap
> cr--r--r-- 1 system audio u:object_r:dmabuf_heap_device:s0
> 248, 14 2023-01-23 16:14 aaudio_playback_heap
> cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
> 248, 3 2023-01-23 16:14 faceauth_tpu-secure
> cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
> 248, 4 2023-01-23 16:14 faimg-secure
> cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
> 248, 7 2023-01-23 16:14 famodel-secure
> cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
> 248, 6 2023-01-23 16:14 faprev-secure
> cr--r--r-- 1 system graphics u:object_r:faceauth_heap_device:s0
> 248, 5 2023-01-23 16:14 farawimg-secure
> cr--r--r-- 1 system graphics u:object_r:sensor_direct_heap_device:s0
> 248, 13 2023-01-23 16:14 sensor_direct_heap
> cr--r--r-- 1 system system u:object_r:dmabuf_system_heap_device:s0
> 248, 9 2023-01-23 16:14 system
> cr--r--r-- 1 system system u:object_r:dmabuf_system_heap_device:s0
> 248, 10 2023-01-23 16:14 system-uncached
> cr--r--r-- 1 system graphics u:object_r:dmabuf_heap_device:s0
> 248, 8 2023-01-23 16:14 tui-secure
> cr--r--r-- 1 system drmrpc
> u:object_r:dmabuf_system_secure_heap_device:s0 248, 1 2023-01-23
> 16:14 vframe-secure
> cr--r--r-- 1 system drmrpc u:object_r:dmabuf_heap_device:s0
> 248, 11 2023-01-23 16:14 video_system
> cr--r--r-- 1 system drmrpc u:object_r:dmabuf_heap_device:s0
> 248, 12 2023-01-23 16:14 video_system-uncached
> cr--r--r-- 1 system graphics u:object_r:vscaler_heap_device:s0
> 248, 2 2023-01-23 16:14 vscaler-secure
> cr--r--r-- 1 system drmrpc
> u:object_r:dmabuf_system_secure_heap_device:s0 248, 0 2023-01-23
> 16:14 vstream-secure
>
> I hope we can get to a point where we don't actually need to protect
> anything but the unicorns. One of my earlier attempts involved a
> counter for each heap that would allow you to limit each one
> individually, but that's not what's proposed now.
Glad someone's minding the details here. This sounds interesting.
Thanks,
-James
>> Regards,
>> Christian.
>>
>>
>>>
>>> Thanks,
>>> -James
>>>
>>>>>>> Please take a look and comment.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Christian.
>>>>>>>
>>>>>>> [1]
>>>>>>> https://lore.kernel.org/all/11a6f97c-e45f-f24b-8a73-48d5a388a2cc@gmail.com/T/
>>>>
>>>
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-01-25 23:20 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-23 12:37 DMA-heap driver hints Christian König
2023-01-23 12:37 ` [PATCH 1/2] dma-heap: add device link and unlink functions Christian König
2023-01-24 4:45 ` John Stultz
2023-01-23 12:37 ` [PATCH 2/2] media: uvcvideo: expose dma-heap hint to userspace Christian König
2023-01-23 14:00 ` Laurent Pinchart
2023-01-23 23:58 ` kernel test robot
2023-01-24 3:44 ` kernel test robot
2023-01-23 13:55 ` DMA-heap driver hints Laurent Pinchart
2023-01-23 16:29 ` Christian König
2023-01-23 16:58 ` Laurent Pinchart
2023-01-24 3:56 ` James Jones
2023-01-24 7:48 ` Christian König
2023-01-24 23:14 ` T.J. Mercier
2023-01-25 23:20 ` James Jones
2023-01-24 5:19 ` John Stultz
2023-01-24 7:15 ` Christian König
2023-01-25 18:59 ` John Stultz
2023-01-24 5:07 ` John Stultz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).