| Message ID | 20230123123756.401692-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dma-heap: add device link and unlink functions | expand |
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
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 */
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(-)