On Mon, Jan 23, 2023 at 4:38 AM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> 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@xxxxxxx>
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
