- Linaro-mm-sig - lists.linaro.org

Re: [PATCH 1/3] dma-buf: add flags to skip map_dma_buf() for some drivers

by Christian König

On 4/30/25 10:56, oushixiong1025(a)163.com wrote: > From: Shixiong Ou <oushixiong(a)kylinos.cn> > > [WHY] Some Importer does not need to call dma_buf_map_attachment() to > get the scatterlist info, especially those drivers of hardware that do > not support DMA, such as the udl, the virtgpu and the ast. > > [HOW] skip map_dma_buf() when dma_buf_dynamic_attach() for some drivers. This patch is based on outdated code. Please see drm-misc-next where the mapping during attach was already dropped. commit b72f66f22c0e39ae6684c43fead774c13db24e73 Author: Christian König <christian.koenig(a)amd.com> Date: Tue Feb 11 17:20:53 2025 +0100 dma-buf: drop caching of sg_tables That was purely for the transition from static to dynamic dma-buf handling and can be removed again now. Regards, Christian. > Signed-off-by: Shixiong Ou <oushixiong(a)kylinos.cn> > --- > drivers/accel/ivpu/ivpu_gem.c | 2 +- > drivers/accel/qaic/qaic_data.c | 2 +- > drivers/dma-buf/dma-buf.c | 29 ++++++++++--------- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +- > drivers/gpu/drm/armada/armada_gem.c | 2 +- > drivers/gpu/drm/drm_prime.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +- > .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 2 +- > drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 2 +- > drivers/gpu/drm/tegra/gem.c | 4 +-- > drivers/gpu/drm/virtio/virtgpu_prime.c | 2 +- > drivers/gpu/drm/xe/xe_dma_buf.c | 2 +- > drivers/iio/industrialio-buffer.c | 2 +- > drivers/infiniband/core/umem_dmabuf.c | 3 +- > .../common/videobuf2/videobuf2-dma-contig.c | 2 +- > .../media/common/videobuf2/videobuf2-dma-sg.c | 2 +- > .../platform/nvidia/tegra-vde/dmabuf-cache.c | 2 +- > drivers/misc/fastrpc.c | 2 +- > drivers/usb/gadget/function/f_fs.c | 2 +- > drivers/xen/gntdev-dmabuf.c | 2 +- > include/linux/dma-buf.h | 5 ++-- > net/core/devmem.c | 2 +- > 22 files changed, 41 insertions(+), 36 deletions(-) > > diff --git a/drivers/accel/ivpu/ivpu_gem.c b/drivers/accel/ivpu/ivpu_gem.c > index 8741c73b92ce..5258a66ed945 100644 > --- a/drivers/accel/ivpu/ivpu_gem.c > +++ b/drivers/accel/ivpu/ivpu_gem.c > @@ -183,7 +183,7 @@ struct drm_gem_object *ivpu_gem_prime_import(struct drm_device *dev, > struct drm_gem_object *obj; > int ret; > > - attach = dma_buf_attach(dma_buf, attach_dev); > + attach = dma_buf_attach(dma_buf, attach_dev, false); > if (IS_ERR(attach)) > return ERR_CAST(attach); > > diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c > index 43aba57b48f0..c13c64d59143 100644 > --- a/drivers/accel/qaic/qaic_data.c > +++ b/drivers/accel/qaic/qaic_data.c > @@ -803,7 +803,7 @@ struct drm_gem_object *qaic_gem_prime_import(struct drm_device *dev, struct dma_ > obj = &bo->base; > get_dma_buf(dma_buf); > > - attach = dma_buf_attach(dma_buf, dev->dev); > + attach = dma_buf_attach(dma_buf, dev->dev, false); > if (IS_ERR(attach)) { > ret = PTR_ERR(attach); > goto attach_fail; > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 5baa83b85515..dd7fe5fbf197 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -904,7 +904,7 @@ static struct sg_table *__map_dma_buf(struct dma_buf_attachment *attach, > struct dma_buf_attachment * > dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > const struct dma_buf_attach_ops *importer_ops, > - void *importer_priv) > + void *importer_priv, bool skip_map) > { > struct dma_buf_attachment *attach; > int ret; > @@ -941,8 +941,6 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > */ > if (dma_buf_attachment_is_dynamic(attach) != > dma_buf_is_dynamic(dmabuf)) { > - struct sg_table *sgt; > - > dma_resv_lock(attach->dmabuf->resv, NULL); > if (dma_buf_is_dynamic(attach->dmabuf)) { > ret = dmabuf->ops->pin(attach); > @@ -950,16 +948,20 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > goto err_unlock; > } > > - sgt = __map_dma_buf(attach, DMA_BIDIRECTIONAL); > - if (!sgt) > - sgt = ERR_PTR(-ENOMEM); > - if (IS_ERR(sgt)) { > - ret = PTR_ERR(sgt); > - goto err_unpin; > + if (!skip_map) { > + struct sg_table *sgt; > + > + sgt = __map_dma_buf(attach, DMA_BIDIRECTIONAL); > + if (!sgt) > + sgt = ERR_PTR(-ENOMEM); > + if (IS_ERR(sgt)) { > + ret = PTR_ERR(sgt); > + goto err_unpin; > + } > + attach->sgt = sgt; > + attach->dir = DMA_BIDIRECTIONAL; > } > dma_resv_unlock(attach->dmabuf->resv); > - attach->sgt = sgt; > - attach->dir = DMA_BIDIRECTIONAL; > } > > return attach; > @@ -989,9 +991,10 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach, "DMA_BUF"); > * mapping. > */ > struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, > - struct device *dev) > + struct device *dev, > + bool skip_map) > { > - return dma_buf_dynamic_attach(dmabuf, dev, NULL, NULL); > + return dma_buf_dynamic_attach(dmabuf, dev, NULL, NULL, skip_map); > } > EXPORT_SYMBOL_NS_GPL(dma_buf_attach, "DMA_BUF"); > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > index e6913fcf2c7b..26c94834e6d2 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > @@ -479,7 +479,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, > return obj; > > attach = dma_buf_dynamic_attach(dma_buf, dev->dev, > - &amdgpu_dma_buf_attach_ops, obj); > + &amdgpu_dma_buf_attach_ops, obj, false); > if (IS_ERR(attach)) { > drm_gem_object_put(obj); > return ERR_CAST(attach); > diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c > index 1a1680d71486..7e1a82828b87 100644 > --- a/drivers/gpu/drm/armada/armada_gem.c > +++ b/drivers/gpu/drm/armada/armada_gem.c > @@ -514,7 +514,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf) > } > } > > - attach = dma_buf_attach(buf, dev->dev); > + attach = dma_buf_attach(buf, dev->dev, false); > if (IS_ERR(attach)) > return ERR_CAST(attach); > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index bdb51c8f262e..8e70abca33b9 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -949,7 +949,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, > if (!dev->driver->gem_prime_import_sg_table) > return ERR_PTR(-EINVAL); > > - attach = dma_buf_attach(dma_buf, attach_dev); > + attach = dma_buf_attach(dma_buf, attach_dev, false); > if (IS_ERR(attach)) > return ERR_CAST(attach); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > index 9473050ac842..6015f6beb8e6 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > @@ -305,7 +305,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > return ERR_PTR(-E2BIG); > > /* need to attach */ > - attach = dma_buf_attach(dma_buf, dev->dev); > + attach = dma_buf_attach(dma_buf, dev->dev, false); > if (IS_ERR(attach)) > return ERR_CAST(attach); > > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c > index 2fda549dd82d..1992241fdf54 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c > @@ -287,7 +287,7 @@ static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915, > goto out_import; > > /* Now try a fake an importer */ > - import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev); > + import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev, false); > if (IS_ERR(import_attach)) { > err = PTR_ERR(import_attach); > goto out_import; > diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c > index 30cf1cdc1aa3..41fb4149409e 100644 > --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c > +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c > @@ -114,7 +114,7 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev, > } > } > > - attach = dma_buf_attach(dma_buf, dev->dev); > + attach = dma_buf_attach(dma_buf, dev->dev, false); > if (IS_ERR(attach)) > return ERR_CAST(attach); > > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c > index ace3e5a805cf..e5527c9d10bb 100644 > --- a/drivers/gpu/drm/tegra/gem.c > +++ b/drivers/gpu/drm/tegra/gem.c > @@ -79,7 +79,7 @@ static struct host1x_bo_mapping *tegra_bo_pin(struct device *dev, struct host1x_ > if (obj->dma_buf) { > struct dma_buf *buf = obj->dma_buf; > > - map->attach = dma_buf_attach(buf, dev); > + map->attach = dma_buf_attach(buf, dev, false); > if (IS_ERR(map->attach)) { > err = PTR_ERR(map->attach); > goto free; > @@ -470,7 +470,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm, > * domain, map it first to the DRM device to get an sgt. > */ > if (tegra->domain) { > - attach = dma_buf_attach(buf, drm->dev); > + attach = dma_buf_attach(buf, drm->dev, false); > if (IS_ERR(attach)) { > err = PTR_ERR(attach); > goto free; > diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c > index 4de2a63ccd18..6d9d1fe342b6 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_prime.c > +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c > @@ -326,7 +326,7 @@ struct drm_gem_object *virtgpu_gem_prime_import(struct drm_device *dev, > drm_gem_private_object_init(dev, obj, buf->size); > > attach = dma_buf_dynamic_attach(buf, dev->dev, > - &virtgpu_dma_buf_attach_ops, obj); > + &virtgpu_dma_buf_attach_ops, obj, true); > if (IS_ERR(attach)) { > kfree(bo); > return ERR_CAST(attach); > diff --git a/drivers/gpu/drm/xe/xe_dma_buf.c b/drivers/gpu/drm/xe/xe_dma_buf.c > index f7a20264ea33..9f524b9ed425 100644 > --- a/drivers/gpu/drm/xe/xe_dma_buf.c > +++ b/drivers/gpu/drm/xe/xe_dma_buf.c > @@ -293,7 +293,7 @@ struct drm_gem_object *xe_gem_prime_import(struct drm_device *dev, > attach_ops = test->attach_ops; > #endif > > - attach = dma_buf_dynamic_attach(dma_buf, dev->dev, attach_ops, &bo->ttm.base); > + attach = dma_buf_dynamic_attach(dma_buf, dev->dev, attach_ops, &bo->ttm.base, false); > if (IS_ERR(attach)) { > obj = ERR_CAST(attach); > goto out_err; > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index a80f7cc25a27..1296af4c2f7a 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -1679,7 +1679,7 @@ static int iio_buffer_attach_dmabuf(struct iio_dev_buffer_pair *ib, > goto err_free_priv; > } > > - attach = dma_buf_attach(dmabuf, indio_dev->dev.parent); > + attach = dma_buf_attach(dmabuf, indio_dev->dev.parent, false); > if (IS_ERR(attach)) { > err = PTR_ERR(attach); > goto err_dmabuf_put; > diff --git a/drivers/infiniband/core/umem_dmabuf.c b/drivers/infiniband/core/umem_dmabuf.c > index 0ec2e4120cc9..ed635c407cbd 100644 > --- a/drivers/infiniband/core/umem_dmabuf.c > +++ b/drivers/infiniband/core/umem_dmabuf.c > @@ -159,7 +159,8 @@ ib_umem_dmabuf_get_with_dma_device(struct ib_device *device, > dmabuf, > dma_device, > ops, > - umem_dmabuf); > + umem_dmabuf, > + false); > if (IS_ERR(umem_dmabuf->attach)) { > ret = ERR_CAST(umem_dmabuf->attach); > goto out_free_umem; > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > index a13ec569c82f..362f5b555ce2 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > @@ -786,7 +786,7 @@ static void *vb2_dc_attach_dmabuf(struct vb2_buffer *vb, struct device *dev, > buf->vb = vb; > > /* create attachment for the dmabuf with the user device */ > - dba = dma_buf_attach(dbuf, buf->dev); > + dba = dma_buf_attach(dbuf, buf->dev, false); > if (IS_ERR(dba)) { > pr_err("failed to attach dmabuf\n"); > kfree(buf); > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > index c6ddf2357c58..4f9a4e9783a1 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > @@ -632,7 +632,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct vb2_buffer *vb, struct device *dev, > > buf->dev = dev; > /* create attachment for the dmabuf with the user device */ > - dba = dma_buf_attach(dbuf, buf->dev); > + dba = dma_buf_attach(dbuf, buf->dev, false); > if (IS_ERR(dba)) { > pr_err("failed to attach dmabuf\n"); > kfree(buf); > diff --git a/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c b/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c > index b34244ea14dd..d04da2d3e4da 100644 > --- a/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c > +++ b/drivers/media/platform/nvidia/tegra-vde/dmabuf-cache.c > @@ -95,7 +95,7 @@ int tegra_vde_dmabuf_cache_map(struct tegra_vde *vde, > goto ref; > } > > - attachment = dma_buf_attach(dmabuf, dev); > + attachment = dma_buf_attach(dmabuf, dev, false); > if (IS_ERR(attachment)) { > dev_err(dev, "Failed to attach dmabuf\n"); > err = PTR_ERR(attachment); > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index 7b7a22c91fe4..aee6f4cbd6c6 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -778,7 +778,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd, > goto get_err; > } > > - map->attach = dma_buf_attach(map->buf, sess->dev); > + map->attach = dma_buf_attach(map->buf, sess->dev, false); > if (IS_ERR(map->attach)) { > dev_err(sess->dev, "Failed to attach dmabuf\n"); > err = PTR_ERR(map->attach); > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 2dea9e42a0f8..51926ffdb843 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -1487,7 +1487,7 @@ static int ffs_dmabuf_attach(struct file *file, int fd) > if (IS_ERR(dmabuf)) > return PTR_ERR(dmabuf); > > - attach = dma_buf_attach(dmabuf, gadget->dev.parent); > + attach = dma_buf_attach(dmabuf, gadget->dev.parent, false); > if (IS_ERR(attach)) { > err = PTR_ERR(attach); > goto err_dmabuf_put; > diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c > index 5453d86324f6..9de191b6d1f7 100644 > --- a/drivers/xen/gntdev-dmabuf.c > +++ b/drivers/xen/gntdev-dmabuf.c > @@ -587,7 +587,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev, > gntdev_dmabuf->priv = priv; > gntdev_dmabuf->fd = fd; > > - attach = dma_buf_attach(dma_buf, dev); > + attach = dma_buf_attach(dma_buf, dev, false); > if (IS_ERR(attach)) { > ret = ERR_CAST(attach); > goto fail_free_obj; > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index 36216d28d8bd..1ea25089b3ba 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -598,11 +598,12 @@ dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach) > } > > struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, > - struct device *dev); > + struct device *dev, > + bool skip_map); > struct dma_buf_attachment * > dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, > const struct dma_buf_attach_ops *importer_ops, > - void *importer_priv); > + void *importer_priv, bool skip_map); > void dma_buf_detach(struct dma_buf *dmabuf, > struct dma_buf_attachment *attach); > int dma_buf_pin(struct dma_buf_attachment *attach); > diff --git a/net/core/devmem.c b/net/core/devmem.c > index 6e27a47d0493..8137ecff9e39 100644 > --- a/net/core/devmem.c > +++ b/net/core/devmem.c > @@ -202,7 +202,7 @@ net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd, > > binding->dmabuf = dmabuf; > > - binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent); > + binding->attachment = dma_buf_attach(binding->dmabuf, dev->dev.parent, false); > if (IS_ERR(binding->attachment)) { > err = PTR_ERR(binding->attachment); > NL_SET_ERR_MSG(extack, "Failed to bind dmabuf to device");

1 month, 1 week

1
0
0 0

[PATCH] dma-buf: system_heap: No separate allocation for attachment sg_tables

by T.J. Mercier

struct dma_heap_attachment is a separate allocation from the struct sg_table it contains, but there is no reason for this. Let's use the slab allocator just once instead of twice for dma_heap_attachment. Signed-off-by: T.J. Mercier <tjmercier(a)google.com> --- drivers/dma-buf/heaps/system_heap.c | 43 ++++++++++++----------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 26d5dc89ea16..bee10c400cf0 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -35,7 +35,7 @@ struct system_heap_buffer { struct dma_heap_attachment { struct device *dev; - struct sg_table *table; + struct sg_table table; struct list_head list; bool mapped; }; @@ -54,29 +54,22 @@ static gfp_t order_flags[] = {HIGH_ORDER_GFP, HIGH_ORDER_GFP, LOW_ORDER_GFP}; static const unsigned int orders[] = {8, 4, 0}; #define NUM_ORDERS ARRAY_SIZE(orders) -static struct sg_table *dup_sg_table(struct sg_table *table) +static int dup_sg_table(struct sg_table *from, struct sg_table *to) { - struct sg_table *new_table; - int ret, i; struct scatterlist *sg, *new_sg; + int ret, i; - new_table = kzalloc(sizeof(*new_table), GFP_KERNEL); - if (!new_table) - return ERR_PTR(-ENOMEM); - - ret = sg_alloc_table(new_table, table->orig_nents, GFP_KERNEL); - if (ret) { - kfree(new_table); - return ERR_PTR(-ENOMEM); - } + ret = sg_alloc_table(to, from->orig_nents, GFP_KERNEL); + if (ret) + return ret; - new_sg = new_table->sgl; - for_each_sgtable_sg(table, sg, i) { + new_sg = to->sgl; + for_each_sgtable_sg(from, sg, i) { sg_set_page(new_sg, sg_page(sg), sg->length, sg->offset); new_sg = sg_next(new_sg); } - return new_table; + return 0; } static int system_heap_attach(struct dma_buf *dmabuf, @@ -84,19 +77,18 @@ static int system_heap_attach(struct dma_buf *dmabuf, { struct system_heap_buffer *buffer = dmabuf->priv; struct dma_heap_attachment *a; - struct sg_table *table; + int ret; a = kzalloc(sizeof(*a), GFP_KERNEL); if (!a) return -ENOMEM; - table = dup_sg_table(&buffer->sg_table); - if (IS_ERR(table)) { + ret = dup_sg_table(&buffer->sg_table, &a->table); + if (ret) { kfree(a); - return -ENOMEM; + return ret; } - a->table = table; a->dev = attachment->dev; INIT_LIST_HEAD(&a->list); a->mapped = false; @@ -120,8 +112,7 @@ static void system_heap_detach(struct dma_buf *dmabuf, list_del(&a->list); mutex_unlock(&buffer->lock); - sg_free_table(a->table); - kfree(a->table); + sg_free_table(&a->table); kfree(a); } @@ -129,7 +120,7 @@ static struct sg_table *system_heap_map_dma_buf(struct dma_buf_attachment *attac enum dma_data_direction direction) { struct dma_heap_attachment *a = attachment->priv; - struct sg_table *table = a->table; + struct sg_table *table = &a->table; int ret; ret = dma_map_sgtable(attachment->dev, table, direction, 0); @@ -164,7 +155,7 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, list_for_each_entry(a, &buffer->attachments, list) { if (!a->mapped) continue; - dma_sync_sgtable_for_cpu(a->dev, a->table, direction); + dma_sync_sgtable_for_cpu(a->dev, &a->table, direction); } mutex_unlock(&buffer->lock); @@ -185,7 +176,7 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, list_for_each_entry(a, &buffer->attachments, list) { if (!a->mapped) continue; - dma_sync_sgtable_for_device(a->dev, a->table, direction); + dma_sync_sgtable_for_device(a->dev, &a->table, direction); } mutex_unlock(&buffer->lock); base-commit: 8ffd015db85fea3e15a77027fda6c02ced4d2444 -- 2.49.0.805.g082f7c87e0-goog

1 month, 1 week

2
3
0 0

Re: [PATCH 2/3] dma-buf: Add DMA_BUF_IOCTL_GET_DMA_ADDR

by Christian König

On 4/29/25 08:39, Simona Vetter wrote: > Catching up after spring break, hence the late reply ... > > On Fri, Apr 11, 2025 at 02:34:37PM -0400, Nicolas Dufresne wrote: >> Le jeudi 10 avril 2025 à 16:53 +0200, Bastien Curutchet a écrit : >>> There is no way to transmit the DMA address of a buffer to userspace. >>> Some UIO users need this to handle DMA from userspace. >> >> To me this API is against all safe practice we've been pushing forward >> and has no place in DMA_BUF API. >> >> If this is fine for the UIO subsystem to pass around physicial >> addresses, then make this part of the UIO device ioctl. > > Yeah, this has no business in dma-buf since the entire point of dma-buf > was to stop all the nasty "just pass raw dma addr in userspace" hacks that > preceeded it. > > And over the years since dma-buf landed, we've removed a lot of these, > like dri1 drivers. Or where that's not possible like with fbdev, hid the > raw dma addr uapi behind a Kconfig. > > I concur with the overall sentiment that this should be done in > vfio/iommufd interfaces, maybe with some support added to map dma-buf. I > think patches for that have been floating around for a while, but I lost a > bit the status of where exactly they are. My take away is that we need to have a documented way for special driver specific interfaces in DMA-buf. In other words DMA-buf has some standardized rules of doing things which every implementation should follow. The implementations might of course still have bugs (e.g. allocate memory for a dma_fence operation), but at least we have documented what should be done and what's forbidden. What is still missing in the documentation is the use case when you have for example vfio which wants to talk to iommufd through a specialized interface. This doesn't necessarily needs to be part of DMA-buf, but we should still document "do it this way" because that has already worked in the last ten use cases and we don't want people to re-invent the wheel in a new funky way which then later turns out to not work. Regards, Christian. > > Cheers, Sima > >> >> regards, >> Nicolas >> >>> >>> Add a new dma_buf_ops operation that returns the DMA address. >>> Add a new ioctl to transmit this DMA address to userspace. >>> >>> Signed-off-by: Bastien Curutchet <bastien.curutchet(a)bootlin.com> >>> --- >>> drivers/dma-buf/dma-buf.c | 21 +++++++++++++++++++++ >>> include/linux/dma-buf.h | 1 + >>> include/uapi/linux/dma-buf.h | 1 + >>> 3 files changed, 23 insertions(+) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index >>> 398418bd9731ad7a3a1f12eaea6a155fa77a22fe..cbbb518981e54e50f479c3d1fcf >>> 6da6971f639c1 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -454,6 +454,24 @@ static long dma_buf_import_sync_file(struct >>> dma_buf *dmabuf, >>> } >>> #endif >>> >>> +static int dma_buf_get_dma_addr(struct dma_buf *dmabuf, u64 __user >>> *arg) >>> +{ >>> + u64 addr; >>> + int ret; >>> + >>> + if (!dmabuf->ops->get_dma_addr) >>> + return -EINVAL; >>> + >>> + ret = dmabuf->ops->get_dma_addr(dmabuf, &addr); >>> + if (ret) >>> + return ret; >>> + >>> + if (copy_to_user(arg, &addr, sizeof(u64))) >>> + return -EFAULT; >>> + >>> + return 0; >>> +} >>> + >>> static long dma_buf_ioctl(struct file *file, >>> unsigned int cmd, unsigned long arg) >>> { >>> @@ -504,6 +522,9 @@ static long dma_buf_ioctl(struct file *file, >>> return dma_buf_import_sync_file(dmabuf, (const void >>> __user *)arg); >>> #endif >>> >>> + case DMA_BUF_IOCTL_GET_DMA_ADDR: >>> + return dma_buf_get_dma_addr(dmabuf, (u64 __user >>> *)arg); >>> + >>> default: >>> return -ENOTTY; >>> } >>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >>> index >>> 36216d28d8bdc01a9c9c47e27c392413f7f6c5fb..ed4bf15d3ce82e7a86323fff459 >>> 699a9bc8baa3b 100644 >>> --- a/include/linux/dma-buf.h >>> +++ b/include/linux/dma-buf.h >>> @@ -285,6 +285,7 @@ struct dma_buf_ops { >>> >>> int (*vmap)(struct dma_buf *dmabuf, struct iosys_map *map); >>> void (*vunmap)(struct dma_buf *dmabuf, struct iosys_map >>> *map); >>> + int (*get_dma_addr)(struct dma_buf *dmabuf, u64 *addr); >>> }; >>> >>> /** >>> diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma- >>> buf.h >>> index >>> 5a6fda66d9adf01438619e7e67fa69f0fec2d88d..f3aba46942042de6a2e3a4cca3e >>> b3f87175e29c9 100644 >>> --- a/include/uapi/linux/dma-buf.h >>> +++ b/include/uapi/linux/dma-buf.h >>> @@ -178,5 +178,6 @@ struct dma_buf_import_sync_file { >>> #define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, __u64) >>> #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE _IOWR(DMA_BUF_BASE, 2, >>> struct dma_buf_export_sync_file) >>> #define DMA_BUF_IOCTL_IMPORT_SYNC_FILE _IOW(DMA_BUF_BASE, 3, struct >>> dma_buf_import_sync_file) >>> +#define DMA_BUF_IOCTL_GET_DMA_ADDR _IOR(DMA_BUF_BASE, 4, __u64 >>> *) >>> >>> #endif >

1 month, 1 week

1
0
0 0

[PATCH v3 00/33] drm/msm: sparse / "VM_BIND" support

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Conversion to DRM GPU VA Manager[1], and adding support for Vulkan Sparse Memory[2] in the form of: 1. A new VM_BIND submitqueue type for executing VM MSM_SUBMIT_BO_OP_MAP/ MAP_NULL/UNMAP commands 2. A new VM_BIND ioctl to allow submitting batches of one or more MAP/MAP_NULL/UNMAP commands to a VM_BIND submitqueue I did not implement support for synchronous VM_BIND commands. Since userspace could just immediately wait for the `SUBMIT` to complete, I don't think we need this extra complexity in the kernel. Synchronous/immediate VM_BIND operations could be implemented with a 2nd VM_BIND submitqueue. The corresponding mesa MR: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/32533 Changes in v3: - Switched to seperate VM_BIND ioctl. This makes the UABI a bit cleaner, but OTOH the userspace code was cleaner when the end result of either type of VkQueue lead to the same ioctl. So I'm a bit on the fence. - Switched to doing the gpuvm bookkeeping synchronously, and only deferring the pgtable updates. This avoids needing to hold any resv locks in the fence signaling path, resolving the last shrinker related lockdep complaints. OTOH it means userspace can trigger invalid pgtable updates with multiple VM_BIND queues. In this case, we ensure that unmaps happen completely (to prevent userspace from using this to access free'd pages), mark the context as unusable, and move on with life. - Link to v2: https://lore.kernel.org/all/20250319145425.51935-1-robdclark@gmail.com/ Changes in v2: - Dropped Bibek Kumar Patro's arm-smmu patches[3], which have since been merged. - Pre-allocate all the things, and drop HACK patch which disabled shrinker. This includes ensuring that vm_bo objects are allocated up front, pre- allocating VMA objects, and pre-allocating pages used for pgtable updates. The latter utilizes io_pgtable_cfg callbacks for pgtable alloc/free, that were initially added for panthor. - Add back support for BO dumping for devcoredump. - Link to v1 (RFC): https://lore.kernel.org/dri-devel/20241207161651.410556-1-robdclark@gmail.c… [1] https://www.kernel.org/doc/html/next/gpu/drm-mm.html#drm-gpuvm [2] https://docs.vulkan.org/spec/latest/chapters/sparsemem.html [3] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=909700 Rob Clark (33): drm/gpuvm: Don't require obj lock in destructor path drm/gpuvm: Allow VAs to hold soft reference to BOs iommu/io-pgtable-arm: Add quirk to quiet WARN_ON() drm/msm: Rename msm_file_private -> msm_context drm/msm: Improve msm_context comments drm/msm: Rename msm_gem_address_space -> msm_gem_vm drm/msm: Remove vram carveout support drm/msm: Collapse vma allocation and initialization drm/msm: Collapse vma close and delete drm/msm: Don't close VMAs on purge drm/msm: drm_gpuvm conversion drm/msm: Convert vm locking drm/msm: Use drm_gpuvm types more drm/msm: Split out helper to get iommu prot flags drm/msm: Add mmu support for non-zero offset drm/msm: Add PRR support drm/msm: Rename msm_gem_vma_purge() -> _unmap() drm/msm: Lazily create context VM drm/msm: Add opt-in for VM_BIND drm/msm: Mark VM as unusable on GPU hangs drm/msm: Add _NO_SHARE flag drm/msm: Crashdump prep for sparse mappings drm/msm: rd dumping prep for sparse mappings drm/msm: Crashdec support for sparse drm/msm: rd dumping support for sparse drm/msm: Extract out syncobj helpers drm/msm: Use DMA_RESV_USAGE_BOOKKEEP/KERNEL drm/msm: Add VM_BIND submitqueue drm/msm: Support IO_PGTABLE_QUIRK_NO_WARN_ON drm/msm: Support pgtable preallocation drm/msm: Split out map/unmap ops drm/msm: Add VM_BIND ioctl drm/msm: Bump UAPI version drivers/gpu/drm/drm_gpuvm.c | 15 +- drivers/gpu/drm/msm/Kconfig | 1 + drivers/gpu/drm/msm/Makefile | 1 + drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 25 +- drivers/gpu/drm/msm/adreno/a2xx_gpummu.c | 5 +- drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 17 +- drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 17 +- drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 4 +- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 22 +- drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 +- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 10 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 32 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 49 +- drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 6 +- drivers/gpu/drm/msm/adreno/a6xx_preempt.c | 10 +- drivers/gpu/drm/msm/adreno/adreno_device.c | 4 - drivers/gpu/drm/msm/adreno/adreno_gpu.c | 88 +- drivers/gpu/drm/msm/adreno/adreno_gpu.h | 23 +- .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 14 +- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 18 +- drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 18 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 14 +- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 4 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 6 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 28 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 12 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 19 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 12 +- drivers/gpu/drm/msm/dsi/dsi_host.c | 14 +- drivers/gpu/drm/msm/msm_drv.c | 183 +-- drivers/gpu/drm/msm/msm_drv.h | 35 +- drivers/gpu/drm/msm/msm_fb.c | 18 +- drivers/gpu/drm/msm/msm_fbdev.c | 2 +- drivers/gpu/drm/msm/msm_gem.c | 489 +++---- drivers/gpu/drm/msm/msm_gem.h | 217 ++- drivers/gpu/drm/msm/msm_gem_prime.c | 15 + drivers/gpu/drm/msm/msm_gem_shrinker.c | 4 +- drivers/gpu/drm/msm/msm_gem_submit.c | 295 ++-- drivers/gpu/drm/msm/msm_gem_vma.c | 1265 +++++++++++++++-- drivers/gpu/drm/msm/msm_gpu.c | 171 ++- drivers/gpu/drm/msm/msm_gpu.h | 132 +- drivers/gpu/drm/msm/msm_iommu.c | 298 +++- drivers/gpu/drm/msm/msm_kms.c | 18 +- drivers/gpu/drm/msm/msm_kms.h | 2 +- drivers/gpu/drm/msm/msm_mmu.h | 38 +- drivers/gpu/drm/msm/msm_rd.c | 62 +- drivers/gpu/drm/msm/msm_ringbuffer.c | 4 +- drivers/gpu/drm/msm/msm_submitqueue.c | 86 +- drivers/gpu/drm/msm/msm_syncobj.c | 172 +++ drivers/gpu/drm/msm/msm_syncobj.h | 37 + drivers/iommu/io-pgtable-arm.c | 18 +- include/drm/drm_gpuvm.h | 12 +- include/linux/io-pgtable.h | 8 + include/uapi/drm/msm_drm.h | 149 +- 57 files changed, 3012 insertions(+), 1216 deletions(-) create mode 100644 drivers/gpu/drm/msm/msm_syncobj.c create mode 100644 drivers/gpu/drm/msm/msm_syncobj.h -- 2.49.0

1 month, 1 week

1
4
0 0

Re: [PATCH 4/4] drm/nouveau: Check dma_fence in canonical way

by Christian König

On 4/24/25 15:02, Philipp Stanner wrote: > In nouveau_fence_done(), a fence is checked for being signaled by > manually evaluating the base fence's bits. This can be done in a > canonical manner through dma_fence_is_signaled(). > > Replace the bit-check with dma_fence_is_signaled(). > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> I think the bit check was used here as fast path optimization because we later call dma_fence_is_signaled() anyway. Feel free to add my acked-by, but honestly what nouveau does here looks rather suspicious to me. Regards, Christian. > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index fb9811938c82..d5654e26d5bc 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -253,7 +253,7 @@ nouveau_fence_done(struct nouveau_fence *fence) > struct nouveau_channel *chan; > unsigned long flags; > > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > + if (dma_fence_is_signaled(&fence->base)) > return true; > > spin_lock_irqsave(&fctx->lock, flags);

1 month, 1 week

1
0
0 0

Re: [PATCH 3/4] drm/nouveau: Simplify nouveau_fence_done()

by Christian König

On 4/24/25 15:02, Philipp Stanner wrote: > nouveau_fence_done() contains an if branch that checks whether a > nouveau_fence has either of the two existing nouveau_fence backend ops, > which will always evaluate to true. > > Remove the surplus check. > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 2b79bcb7da16..fb9811938c82 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -249,21 +249,19 @@ nouveau_fence_emit(struct nouveau_fence *fence) > bool > nouveau_fence_done(struct nouveau_fence *fence) > { > - if (fence->base.ops == &nouveau_fence_ops_legacy || > - fence->base.ops == &nouveau_fence_ops_uevent) { > - struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > - struct nouveau_channel *chan; > - unsigned long flags; > + struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > + struct nouveau_channel *chan; > + unsigned long flags; > > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > - return true; > + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > + return true; > + > + spin_lock_irqsave(&fctx->lock, flags); > + chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > + if (chan) > + nouveau_fence_update(chan, fctx); > + spin_unlock_irqrestore(&fctx->lock, flags); > > - spin_lock_irqsave(&fctx->lock, flags); > - chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > - if (chan) > - nouveau_fence_update(chan, fctx); > - spin_unlock_irqrestore(&fctx->lock, flags); > - } > return dma_fence_is_signaled(&fence->base); > } >

1 month, 1 week

1
0
0 0

Re: [PATCH v7 04/11] optee: sync secure world ABI headers

by Jens Wiklander

Hi Rouven, On Fri, Apr 25, 2025 at 3:36 PM Rouven Czerwinski <rouven.czerwinski(a)linaro.org> wrote: > > Hi, > > On Fri, 4 Apr 2025 at 16:31, Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > > Update the header files describing the secure world ABI, both with and > > without FF-A. The ABI is extended to deal with protected memory, but as > > usual backward compatible. > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > --- > > drivers/tee/optee/optee_ffa.h | 27 +++++++++--- > > drivers/tee/optee/optee_msg.h | 83 ++++++++++++++++++++++++++++++----- > > drivers/tee/optee/optee_smc.h | 71 +++++++++++++++++++++++++++++- > > 3 files changed, 163 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/tee/optee/optee_ffa.h b/drivers/tee/optee/optee_ffa.h > > index 257735ae5b56..cc257e7956a3 100644 > > --- a/drivers/tee/optee/optee_ffa.h > > +++ b/drivers/tee/optee/optee_ffa.h > > @@ -81,7 +81,7 @@ > > * as the second MSG arg struct for > > * OPTEE_FFA_YIELDING_CALL_WITH_ARG. > > * Bit[31:8]: Reserved (MBZ) > > - * w5: Bitfield of secure world capabilities OPTEE_FFA_SEC_CAP_* below, > > + * w5: Bitfield of OP-TEE capabilities OPTEE_FFA_SEC_CAP_* > > * w6: The maximum secure world notification number > > * w7: Not used (MBZ) > > */ > > @@ -94,6 +94,8 @@ > > #define OPTEE_FFA_SEC_CAP_ASYNC_NOTIF BIT(1) > > /* OP-TEE supports probing for RPMB device if needed */ > > #define OPTEE_FFA_SEC_CAP_RPMB_PROBE BIT(2) > > +/* OP-TEE supports Protected Memory for secure data path */ > > +#define OPTEE_FFA_SEC_CAP_PROTMEM BIT(3) > > > > #define OPTEE_FFA_EXCHANGE_CAPABILITIES OPTEE_FFA_BLOCKING_CALL(2) > > > > @@ -108,7 +110,7 @@ > > * > > * Return register usage: > > * w3: Error code, 0 on success > > - * w4-w7: Note used (MBZ) > > + * w4-w7: Not used (MBZ) > > */ > > #define OPTEE_FFA_UNREGISTER_SHM OPTEE_FFA_BLOCKING_CALL(3) > > > > @@ -119,16 +121,31 @@ > > * Call register usage: > > * w3: Service ID, OPTEE_FFA_ENABLE_ASYNC_NOTIF > > * w4: Notification value to request bottom half processing, should be > > - * less than OPTEE_FFA_MAX_ASYNC_NOTIF_VALUE. > > + * less than OPTEE_FFA_MAX_ASYNC_NOTIF_VALUE > > * w5-w7: Not used (MBZ) > > * > > * Return register usage: > > * w3: Error code, 0 on success > > - * w4-w7: Note used (MBZ) > > + * w4-w7: Not used (MBZ) > > */ > > #define OPTEE_FFA_ENABLE_ASYNC_NOTIF OPTEE_FFA_BLOCKING_CALL(5) > > > > -#define OPTEE_FFA_MAX_ASYNC_NOTIF_VALUE 64 > > +#define OPTEE_FFA_MAX_ASYNC_NOTIF_VALUE 64 > > + > > +/* > > + * Release Protected memory > > + * > > + * Call register usage: > > + * w3: Service ID, OPTEE_FFA_RECLAIM_PROTMEM > > + * w4: Shared memory handle, lower bits > > + * w5: Shared memory handle, higher bits > > + * w6-w7: Not used (MBZ) > > + * > > + * Return register usage: > > + * w3: Error code, 0 on success > > + * w4-w7: Note used (MBZ) > > + */ > > +#define OPTEE_FFA_RELEASE_PROTMEM OPTEE_FFA_BLOCKING_CALL(8) > > > > /* > > * Call with struct optee_msg_arg as argument in the supplied shared memory > > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h > > index e8840a82b983..22d71d6f110d 100644 > > --- a/drivers/tee/optee/optee_msg.h > > +++ b/drivers/tee/optee/optee_msg.h > > @@ -133,13 +133,13 @@ struct optee_msg_param_rmem { > > }; > > > > /** > > - * struct optee_msg_param_fmem - ffa memory reference parameter > > + * struct optee_msg_param_fmem - FF-A memory reference parameter > > * @offs_lower: Lower bits of offset into shared memory reference > > * @offs_upper: Upper bits of offset into shared memory reference > > * @internal_offs: Internal offset into the first page of shared memory > > * reference > > * @size: Size of the buffer > > - * @global_id: Global identifier of Shared memory > > + * @global_id: Global identifier of the shared memory > > */ > > struct optee_msg_param_fmem { > > u32 offs_low; > > @@ -165,7 +165,7 @@ struct optee_msg_param_value { > > * @attr: attributes > > * @tmem: parameter by temporary memory reference > > * @rmem: parameter by registered memory reference > > - * @fmem: parameter by ffa registered memory reference > > + * @fmem: parameter by FF-A registered memory reference > > * @value: parameter by opaque value > > * @octets: parameter by octet string > > * > > @@ -296,6 +296,18 @@ struct optee_msg_arg { > > */ > > #define OPTEE_MSG_FUNCID_GET_OS_REVISION 0x0001 > > > > +/* > > + * Values used in OPTEE_MSG_CMD_LEND_PROTMEM below > > + * OPTEE_MSG_PROTMEM_RESERVED Reserved > > + * OPTEE_MSG_PROTMEM_SECURE_VIDEO_PLAY Secure Video Playback > > + * OPTEE_MSG_PROTMEM_TRUSTED_UI Trused UI > > + * OPTEE_MSG_PROTMEM_SECURE_VIDEO_RECORD Secure Video Recording > > + */ > > +#define OPTEE_MSG_PROTMEM_RESERVED 0 > > +#define OPTEE_MSG_PROTMEM_SECURE_VIDEO_PLAY 1 > > +#define OPTEE_MSG_PROTMEM_TRUSTED_UI 2 > > +#define OPTEE_MSG_PROTMEM_SECURE_VIDEO_RECORD 3 > > + > > /* > > * Do a secure call with struct optee_msg_arg as argument > > * The OPTEE_MSG_CMD_* below defines what goes in struct optee_msg_arg::cmd > > @@ -337,15 +349,62 @@ struct optee_msg_arg { > > * OPTEE_MSG_CMD_STOP_ASYNC_NOTIF informs secure world that from now is > > * normal world unable to process asynchronous notifications. Typically > > * used when the driver is shut down. > > + * > > + * OPTEE_MSG_CMD_LEND_PROTMEM lends protected memory. The passed normal > > + * physical memory is protected from normal world access. The memory > > + * should be unmapped prior to this call since it becomes inaccessible > > + * during the request. > > + * Parameters are passed as: > > + * [in] param[0].attr OPTEE_MSG_ATTR_TYPE_VALUE_INPUT > > + * [in] param[0].u.value.a OPTEE_MSG_PROTMEM_* defined above > > + * [in] param[1].attr OPTEE_MSG_ATTR_TYPE_TMEM_INPUT > > + * [in] param[1].u.tmem.buf_ptr physical address > > + * [in] param[1].u.tmem.size size > > + * [in] param[1].u.tmem.shm_ref holds protected memory reference > > + * > > + * OPTEE_MSG_CMD_RECLAIM_PROTMEM reclaims a previously lent protected > > + * memory reference. The physical memory is accessible by the normal world > > + * after this function has return and can be mapped again. The information > > + * is passed as: > > + * [in] param[0].attr OPTEE_MSG_ATTR_TYPE_VALUE_INPUT > > + * [in] param[0].u.value.a holds protected memory cookie > > + * > > + * OPTEE_MSG_CMD_GET_PROTMEM_CONFIG get configuration for a specific > > + * protected memory use case. Parameters are passed as: > > + * [in] param[0].attr OPTEE_MSG_ATTR_TYPE_VALUE_INOUT > > + * [in] param[0].value.a OPTEE_MSG_PROTMEM_* > > + * [in] param[1].attr OPTEE_MSG_ATTR_TYPE_{R,F}MEM_OUTPUT > > + * [in] param[1].u.{r,f}mem Buffer or NULL > > + * [in] param[1].u.{r,f}mem.size Provided size of buffer or 0 for query > > + * output for the protected use case: > > + * [out] param[0].value.a Minimal size of protected memory > > + * [out] param[0].value.b Required alignment of size and start of > > + * protected memory > > + * [out] param[1].{r,f}mem.size Size of output data > > + * [out] param[1].{r,f}mem If non-NULL, contains an array of > > + * uint16_t holding endpoints that > > + * must be included when lending > > + * memory for this use case > > + * > > + * OPTEE_MSG_CMD_ASSIGN_PROTMEM assigns use-case to protected memory > > + * previously lent using the FFA_LEND framework ABI. Parameters are passed > > + * as: > > + * [in] param[0].attr OPTEE_MSG_ATTR_TYPE_VALUE_INPUT > > + * [in] param[0].u.value.a holds protected memory cookie > > + * [in] param[0].u.value.b OPTEE_MSG_PROTMEM_* defined above > > */ > > -#define OPTEE_MSG_CMD_OPEN_SESSION 0 > > -#define OPTEE_MSG_CMD_INVOKE_COMMAND 1 > > -#define OPTEE_MSG_CMD_CLOSE_SESSION 2 > > -#define OPTEE_MSG_CMD_CANCEL 3 > > -#define OPTEE_MSG_CMD_REGISTER_SHM 4 > > -#define OPTEE_MSG_CMD_UNREGISTER_SHM 5 > > -#define OPTEE_MSG_CMD_DO_BOTTOM_HALF 6 > > -#define OPTEE_MSG_CMD_STOP_ASYNC_NOTIF 7 > > -#define OPTEE_MSG_FUNCID_CALL_WITH_ARG 0x0004 > > +#define OPTEE_MSG_CMD_OPEN_SESSION 0 > > +#define OPTEE_MSG_CMD_INVOKE_COMMAND 1 > > +#define OPTEE_MSG_CMD_CLOSE_SESSION 2 > > +#define OPTEE_MSG_CMD_CANCEL 3 > > +#define OPTEE_MSG_CMD_REGISTER_SHM 4 > > +#define OPTEE_MSG_CMD_UNREGISTER_SHM 5 > > +#define OPTEE_MSG_CMD_DO_BOTTOM_HALF 6 > > +#define OPTEE_MSG_CMD_STOP_ASYNC_NOTIF 7 > > +#define OPTEE_MSG_CMD_LEND_PROTMEM 8 > > +#define OPTEE_MSG_CMD_RECLAIM_PROTMEM 9 > > +#define OPTEE_MSG_CMD_GET_PROTMEM_CONFIG 10 > > +#define OPTEE_MSG_CMD_ASSIGN_PROTMEM 11 > > +#define OPTEE_MSG_FUNCID_CALL_WITH_ARG 0x0004 > > > > #endif /* _OPTEE_MSG_H */ > > diff --git a/drivers/tee/optee/optee_smc.h b/drivers/tee/optee/optee_smc.h > > index 879426300821..b17e81f464a3 100644 > > --- a/drivers/tee/optee/optee_smc.h > > +++ b/drivers/tee/optee/optee_smc.h > > @@ -264,7 +264,6 @@ struct optee_smc_get_shm_config_result { > > #define OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM BIT(0) > > /* Secure world can communicate via previously unregistered shared memory */ > > #define OPTEE_SMC_SEC_CAP_UNREGISTERED_SHM BIT(1) > > - > > /* > > * Secure world supports commands "register/unregister shared memory", > > * secure world accepts command buffers located in any parts of non-secure RAM > > @@ -280,6 +279,10 @@ struct optee_smc_get_shm_config_result { > > #define OPTEE_SMC_SEC_CAP_RPC_ARG BIT(6) > > /* Secure world supports probing for RPMB device if needed */ > > #define OPTEE_SMC_SEC_CAP_RPMB_PROBE BIT(7) > > +/* Secure world supports protected memory */ > > +#define OPTEE_SMC_SEC_CAP_PROTMEM BIT(8) > > +/* Secure world supports dynamic protected memory */ > > +#define OPTEE_SMC_SEC_CAP_DYNAMIC_PROTMEM BIT(9) > > > > #define OPTEE_SMC_FUNCID_EXCHANGE_CAPABILITIES 9 > > #define OPTEE_SMC_EXCHANGE_CAPABILITIES \ > > @@ -451,6 +454,72 @@ struct optee_smc_disable_shm_cache_result { > > > > /* See OPTEE_SMC_CALL_WITH_REGD_ARG above */ > > #define OPTEE_SMC_FUNCID_CALL_WITH_REGD_ARG 19 > > +/* > > + * Get protected memory config > > + * > > + * Returns the protected memory config. > > + * > > + * Call register usage: > > + * a0 SMC Function ID, OPTEE_SMC_GET_PROTMEM_CONFIG > > + * a2-6 Not used, must be zero > > + * a7 Hypervisor Client ID register > > + * > > + * Have config return register usage: > > + * a0 OPTEE_SMC_RETURN_OK > > + * a1 Physical address of start of protected memory > > + * a2 Size of protected memory > > + * a3 Not used > > + * a4-7 Preserved > > + * > > + * Not available register usage: > > + * a0 OPTEE_SMC_RETURN_ENOTAVAIL > > + * a1-3 Not used > > + * a4-7 Preserved > > + */ > > +#define OPTEE_SMC_FUNCID_GET_PROTMEM_CONFIG 20 > > +#define OPTEE_SMC_GET_PROTMEM_CONFIG \ > > + OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_PROTMEM_CONFIG) > > + > > +struct optee_smc_get_protmem_config_result { > > + unsigned long status; > > + unsigned long start; > > + unsigned long size; > > + unsigned long flags; > > The ABI comment does not document a flags return argument, either > this can be removed or the ABI comment needs to be fixed. Sure, I'll remove the field. > Same for > > +}; > > + > > +/* > > + * Get dynamic protected memory config > > + * > > + * Returns the dynamic protected memory config. > > + * > > + * Call register usage: > > + * a0 SMC Function ID, OPTEE_SMC_GET_DYN_SHM_CONFIG > > should be OPTEE_SMC_GET_DYN_PROTMEM_CONFIG Thanks, I'll update. > > > + * a2-6 Not used, must be zero > > + * a7 Hypervisor Client ID register > > + * > > + * Have config return register usage: > > + * a0 OPTEE_SMC_RETURN_OK > > + * a1 Minamal size of protected memory > > Nit: Typo, should be "Minimal" Yes, I'll update. Cheers, Jens > > > + * a2 Required alignment of size and start of registered protected memory > > + * a3 Not used > > + * a4-7 Preserved > > + * > > + * Not available register usage: > > + * a0 OPTEE_SMC_RETURN_ENOTAVAIL > > + * a1-3 Not used > > + * a4-7 Preserved > > + */ > > + > > +#define OPTEE_SMC_FUNCID_GET_DYN_PROTMEM_CONFIG 21 > > +#define OPTEE_SMC_GET_DYN_PROTMEM_CONFIG \ > > + OPTEE_SMC_FAST_CALL_VAL(OPTEE_SMC_FUNCID_GET_DYN_PROTMEM_CONFIG) > > + > > +struct optee_smc_get_dyn_protmem_config_result { > > + unsigned long status; > > + unsigned long size; > > + unsigned long align; > > + unsigned long flags; > > +}; > > > > /* > > * Resume from RPC (for example after processing a foreign interrupt) > > -- > > 2.43.0 > > - Rouven

1 month, 1 week

1
0
0 0

[PATCH AUTOSEL 6.13 15/34] drm/amdgpu: allow pinning DMA-bufs into VRAM if all importers can do P2P

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit f5e7fabd1f5c65b2e077efcdb118cfa67eae7311 ] Try pinning into VRAM to allow P2P with RDMA NICs without ODP support if all attachments can do P2P. If any attachment can't do P2P just pin into GTT instead. Acked-by: Simona Vetter <simona.vetter(a)ffwll.ch> Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Felix Kuehling <felix.kuehling(a)amd.com> Reviewed-by: Felix Kuehling <felix.kuehling(a)amd.com> Tested-by: Pak Nin Lui <pak.lui(a)amd.com> Cc: Simona Vetter <simona.vetter(a)ffwll.ch> Signed-off-by: Alex Deucher <alexander.deucher(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 25 +++++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index 8e81a83d37d84..83390143c2e9f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -72,11 +72,25 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf, */ static int amdgpu_dma_buf_pin(struct dma_buf_attachment *attach) { - struct drm_gem_object *obj = attach->dmabuf->priv; - struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); + struct dma_buf *dmabuf = attach->dmabuf; + struct amdgpu_bo *bo = gem_to_amdgpu_bo(dmabuf->priv); + u32 domains = bo->preferred_domains; - /* pin buffer into GTT */ - return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); + dma_resv_assert_held(dmabuf->resv); + + /* + * Try pinning into VRAM to allow P2P with RDMA NICs without ODP + * support if all attachments can do P2P. If any attachment can't do + * P2P just pin into GTT instead. + */ + list_for_each_entry(attach, &dmabuf->attachments, node) + if (!attach->peer2peer) + domains &= ~AMDGPU_GEM_DOMAIN_VRAM; + + if (domains & AMDGPU_GEM_DOMAIN_VRAM) + bo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED; + + return amdgpu_bo_pin(bo, domains); } /** @@ -131,9 +145,6 @@ static struct sg_table *amdgpu_dma_buf_map(struct dma_buf_attachment *attach, r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); if (r) return ERR_PTR(r); - - } else if (bo->tbo.resource->mem_type != TTM_PL_TT) { - return ERR_PTR(-EBUSY); } switch (bo->tbo.resource->mem_type) { -- 2.39.5

1 month, 1 week

2
2
0 0

[PATCH v3 0/2] dma-buf: heaps: Support carved-out heaps

by Maxime Ripard

Hi, This series is the follow-up of the discussion that John and I had some time ago here: https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrr… The initial problem we were discussing was that I'm currently working on a platform which has a memory layout with ECC enabled. However, enabling the ECC has a number of drawbacks on that platform: lower performance, increased memory usage, etc. So for things like framebuffers, the trade-off isn't great and thus there's a memory region with ECC disabled to allocate from for such use cases. After a suggestion from John, I chose to first start using heap allocations flags to allow for userspace to ask for a particular ECC setup. This is then backed by a new heap type that runs from reserved memory chunks flagged as such, and the existing DT properties to specify the ECC properties. After further discussion, it was considered that flags were not the right solution, and relying on the names of the heaps would be enough to let userspace know the kind of buffer it deals with. Thus, even though the uAPI part of it has been dropped in this second version, we still need a driver to create heaps out of carved-out memory regions. In addition to the original usecase, a similar driver can be found in BSPs from most vendors, so I believe it would be a useful addition to the kernel. I submitted a draft PR to the DT schema for the bindings used in this PR: https://github.com/devicetree-org/dt-schema/pull/138 Let me know what you think, Maxime Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- Changes in v3: - Reworked global variable patch - Link to v2: https://lore.kernel.org/r/20250401-dma-buf-ecc-heap-v2-0-043fd006a1af@kerne… Changes in v2: - Add vmap/vunmap operations - Drop ECC flags uapi - Rebase on top of 6.14 - Link to v1: https://lore.kernel.org/r/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kerne… --- Maxime Ripard (2): dma-buf: heaps: system: Remove global variable dma-buf: heaps: Introduce a new heap for reserved memory drivers/dma-buf/heaps/Kconfig | 8 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/carveout_heap.c | 360 ++++++++++++++++++++++++++++++++++ drivers/dma-buf/heaps/system_heap.c | 3 +- 4 files changed, 370 insertions(+), 2 deletions(-) --- base-commit: fcbf30774e82a441890b722bf0c26542fb82150f change-id: 20240515-dma-buf-ecc-heap-28a311d2c94e Best regards, -- Maxime Ripard <mripard(a)kernel.org>

1 month, 1 week

4
7
0 0

Re: [PATCH 2/4] bpf: Add dmabuf iterator

by T.J. Mercier

On Tue, Apr 22, 2025 at 4:01 PM Alexei Starovoitov <alexei.starovoitov(a)gmail.com> wrote: > > On Tue, Apr 22, 2025 at 12:57 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > On Mon, Apr 21, 2025 at 4:39 PM Alexei Starovoitov > > <alexei.starovoitov(a)gmail.com> wrote: > > > > > > On Mon, Apr 21, 2025 at 1:40 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > > > > > > > new file mode 100644 > > > > > > index 000000000000..b4b8be1d6aa4 > > > > > > --- /dev/null > > > > > > +++ b/kernel/bpf/dmabuf_iter.c > > > > > > > > > > Maybe we should add this file to drivers/dma-buf. I would like to > > > > > hear other folks thoughts on this. > > > > > > > > This is fine with me, and would save us the extra > > > > CONFIG_DMA_SHARED_BUFFER check that's currently needed in > > > > kernel/bpf/Makefile but would require checking CONFIG_BPF instead. > > > > Sumit / Christian any objections to moving the dmabuf bpf iterator > > > > implementation into drivers/dma-buf? > > > > > > The driver directory would need to 'depends on BPF_SYSCALL'. > > > Are you sure you want this? > > > imo kernel/bpf/ is fine for this. > > > > I don't have a strong preference so either way is fine with me. The > > main difference I see is maintainership. > > > > > You also probably want > > > .feature = BPF_ITER_RESCHED > > > in bpf_dmabuf_reg_info. > > > > Thank you, this looks like a good idea. > > > > > Also have you considered open coded iterator for dmabufs? > > > Would it help with the interface to user space? > > > > I read through the open coded iterator patches, and it looks like they > > would be slightly more efficient by avoiding seq_file overhead. As far > > as the interface to userspace, for the purpose of replacing what's > > currently exposed by CONFIG_DMABUF_SYSFS_STATS I don't think there is > > a difference. However it looks like if I were to try to replace all of > > our userspace analysis of dmabufs with a single bpf program then an > > open coded iterator would make that much easier. I had not considered > > attempting that. > > > > One problem I see with open coded iterators is that support is much > > more recent (2023 vs 2020). We support longterm stable kernels (back > > to 5.4 currently but probably 5.10 by the time this would be used), so > > it seems like it would be harder to backport the kernel support for an > > open-coded iterator that far since it only goes back as far as 6.6 > > now. Actually it doesn't look like it is possible while also > > maintaining the stable ABI we provide to device vendors. Which means > > we couldn't get rid of the dmabuf sysfs stats userspace dependency > > until 6.1 EOL in Dec. 2027. :\ So I'm in favor of a traditional bpf > > iterator here for now. > > Fair enough, but please implement both and backport only > the old style pinned iterator. Ok, will do. > The code will be mostly shared between them. > bpf_iter_dmabuf_new/_next will be more flexible with more > options to return data to user space. Like android can invent > their own binary format. Pack into it in a bpf prog, send to > bpf ringbuf and unmarshal efficiently in user space. > Instead of being limited to text output that pinned iterators > are supposed to do usually. Also a neat idea! > You can do binary with bpf_seq_write() too, but it's rare. > > Also please provide full bpf prog that you'll use in production > in a selftest instead of trivial: > +SEC("iter/dmabuf") > +int dmabuf_collector(struct bpf_iter__dmabuf *ctx) > > just to make sure it's tested end to end and future changes > won't break it. The final bpf program should be something pretty close to that, but I'll start working on the AOSP side as well so I can put up patches. > > pw-bot: cr

1 month, 2 weeks

1
0
0 0

Re: [PATCH v2 0/2] dma-buf: heaps: Use constant name for CMA heap

by Sumit Semwal

Hello Jared, On Wed, 23 Apr 2025 at 00:49, Jared Kangas <jkangas(a)redhat.com> wrote: > > Hi all, > > This patch series is based on a previous discussion around CMA heap > naming. [1] The heap's name depends on the device name, which is > generally "reserved", "linux,cma", or "default-pool", but could be any > arbitrary name given to the default CMA area in the devicetree. For a > consistent userspace interface, the series introduces a constant name > for the CMA heap, and for backwards compatibility, an additional Kconfig > that controls the creation of a legacy-named heap with the same CMA > backing. > > The ideas to handle backwards compatibility in [1] are to either use a > symlink or add a heap node with a duplicate minor. However, I assume > that we don't want to create symlinks in /dev from module initcalls, and > attempting to duplicate minors would cause device_create() to fail. > Because of these drawbacks, after brainstorming with Maxime Ripard, I > went with creating a new node in devtmpfs with its own minor. This > admittedly makes it a little unclear that the old and new nodes are > backed by the same heap when both are present. The only approach that I > think would provide total clarity on this in userspace is symlinking, > which seemed like a fairly involved solution for devtmpfs, but if I'm > wrong on this, please let me know. Thanks indeed for this patch; just one minor nit: the link referred to as [1] here seems to be missing. Could you please add it? This would make it easier to follow the chain of discussion in posterity. > > Changelog: > v2: Use tabs instead of spaces for large vertical alignment. > > Jared Kangas (2): > dma-buf: heaps: Parameterize heap name in __add_cma_heap() > dma-buf: heaps: Give default CMA heap a fixed name > > Documentation/userspace-api/dma-buf-heaps.rst | 11 ++++--- > drivers/dma-buf/heaps/Kconfig | 10 +++++++ > drivers/dma-buf/heaps/cma_heap.c | 30 ++++++++++++++----- > 3 files changed, 40 insertions(+), 11 deletions(-) > > -- > 2.49.0 > Best, Sumit

1 month, 2 weeks

1
0
0 0

Re: [PATCH v2 2/2] dma-buf: heaps: Give default CMA heap a fixed name

by John Stultz

On Tue, Apr 22, 2025 at 12:19 PM Jared Kangas <jkangas(a)redhat.com> wrote: > > The CMA heap's name in devtmpfs can vary depending on how the heap is > defined. Its name defaults to "reserved", but if a CMA area is defined > in the devicetree, the heap takes on the devicetree node's name, such as > "default-pool" or "linux,cma". To simplify naming, just name it > "default_cma", and keep a legacy node in place backed by the same > underlying structure for backwards compatibility. > > Signed-off-by: Jared Kangas <jkangas(a)redhat.com> Once again, thanks for working out how to improve the standard naming while keeping compatibility. I do still hope we can get to the point where other cma regions can be registered as heaps with unique/purpose-specific names, but I can see having a standard name for the default region is a nice improvement. Acked-by: John Stultz <jstultz(a)google.com> thanks -john

1 month, 2 weeks

1
0
0 0

Re: [PATCH v2 1/2] dma-buf: heaps: Parameterize heap name in __add_cma_heap()

by John Stultz

On Tue, Apr 22, 2025 at 12:19 PM Jared Kangas <jkangas(a)redhat.com> wrote: > > Prepare for the introduction of a fixed-name CMA heap by replacing the > unused void pointer parameter in __add_cma_heap() with the heap name. > > Signed-off-by: Jared Kangas <jkangas(a)redhat.com> Thanks so much for taking this effort on. Looks good to me! Acked-by: John Stultz <jstultz(a)google.com>

1 month, 2 weeks

1
0
0 0

Re: [PATCH 2/4] bpf: Add dmabuf iterator

by T.J. Mercier

On Mon, Apr 21, 2025 at 4:39 PM Alexei Starovoitov <alexei.starovoitov(a)gmail.com> wrote: > > On Mon, Apr 21, 2025 at 1:40 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > > > new file mode 100644 > > > > index 000000000000..b4b8be1d6aa4 > > > > --- /dev/null > > > > +++ b/kernel/bpf/dmabuf_iter.c > > > > > > Maybe we should add this file to drivers/dma-buf. I would like to > > > hear other folks thoughts on this. > > > > This is fine with me, and would save us the extra > > CONFIG_DMA_SHARED_BUFFER check that's currently needed in > > kernel/bpf/Makefile but would require checking CONFIG_BPF instead. > > Sumit / Christian any objections to moving the dmabuf bpf iterator > > implementation into drivers/dma-buf? > > The driver directory would need to 'depends on BPF_SYSCALL'. > Are you sure you want this? > imo kernel/bpf/ is fine for this. I don't have a strong preference so either way is fine with me. The main difference I see is maintainership. > You also probably want > .feature = BPF_ITER_RESCHED > in bpf_dmabuf_reg_info. Thank you, this looks like a good idea. > Also have you considered open coded iterator for dmabufs? > Would it help with the interface to user space? I read through the open coded iterator patches, and it looks like they would be slightly more efficient by avoiding seq_file overhead. As far as the interface to userspace, for the purpose of replacing what's currently exposed by CONFIG_DMABUF_SYSFS_STATS I don't think there is a difference. However it looks like if I were to try to replace all of our userspace analysis of dmabufs with a single bpf program then an open coded iterator would make that much easier. I had not considered attempting that. One problem I see with open coded iterators is that support is much more recent (2023 vs 2020). We support longterm stable kernels (back to 5.4 currently but probably 5.10 by the time this would be used), so it seems like it would be harder to backport the kernel support for an open-coded iterator that far since it only goes back as far as 6.6 now. Actually it doesn't look like it is possible while also maintaining the stable ABI we provide to device vendors. Which means we couldn't get rid of the dmabuf sysfs stats userspace dependency until 6.1 EOL in Dec. 2027. :\ So I'm in favor of a traditional bpf iterator here for now.

1 month, 2 weeks

1
0
0 0

Re: [PATCH 0/3] uio/dma-buf: Give UIO users access to DMA addresses.

by Jason Gunthorpe

On Mon, Apr 14, 2025 at 09:21:25PM +0200, Thomas Petazzoni wrote: > > "UIO is a broken legacy mess, so let's add more broken things > > to it as broken + broken => still broken, so no harm done", am I > > getting that right? > > Who says UIO is a "broken legacy mess"? Only you says so. I don't see > any indication anywhere in the kernel tree suggesting that UIO is > considered a broken legacy mess. Explain what the difference is between UIO and VFIO, especially VFIO no-iommu mode? I've always understood that UIO is for very simple devices that cannot do DMA. So it's very simple operating model and simple security work fine. IMHO, if the can use DMA it should use VFIO. If you have no iommu then you should use the VFIO unsafe no-iommu path. It still provides a solid framework. As to this series, I have seen a number of requests to improve the VFIO no-iommu path to allow working with the existing IOAS scheme to register memory but to allow the kernel the return the no-iommu DMAable address of the IOAS pinned memory. This would replace the hacky use of mlock and /proc/XX/pagemap that people use today. If that were done, could you use VFIO no-iommu? > Keep in mind that when you're running code as root, you can load a > kernel module, which can do anything on the system security-wise. So > letting UIO expose MMIO registers of devices to userspace applications > running as root is not any worse than that. That isn't fully true.. UIO isn't fitting into the security model by allowing DMA capable devices to be exposed without checking for CAP_SYS_RAW_IO first. Jason

1 month, 2 weeks

1
0
0 0

Re: [PATCH 2/4] bpf: Add dmabuf iterator

by T.J. Mercier

On Mon, Apr 21, 2025 at 10:58 AM Song Liu <song(a)kernel.org> wrote: > > On Mon, Apr 14, 2025 at 3:53 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > The dmabuf iterator traverses the list of all DMA buffers. The list is > > maintained only when CONFIG_DEBUG_FS is enabled. > > > > DMA buffers are refcounted through their associated struct file. A > > reference is taken on each buffer as the list is iterated to ensure each > > buffer persists for the duration of the bpf program execution without > > holding the list mutex. > > > > Signed-off-by: T.J. Mercier <tjmercier(a)google.com> > > --- > > include/linux/btf_ids.h | 1 + > > kernel/bpf/Makefile | 3 + > > kernel/bpf/dmabuf_iter.c | 130 +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 134 insertions(+) > > create mode 100644 kernel/bpf/dmabuf_iter.c > > > > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h > > index 139bdececdcf..899ead57d89d 100644 > > --- a/include/linux/btf_ids.h > > +++ b/include/linux/btf_ids.h > > @@ -284,5 +284,6 @@ extern u32 bpf_cgroup_btf_id[]; > > extern u32 bpf_local_storage_map_btf_id[]; > > extern u32 btf_bpf_map_id[]; > > extern u32 bpf_kmem_cache_btf_id[]; > > +extern u32 bpf_dmabuf_btf_id[]; > > This is not necessary. See below. > > > > > #endif > > diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile > > index 70502f038b92..5b30d37ef055 100644 > > --- a/kernel/bpf/Makefile > > +++ b/kernel/bpf/Makefile > > @@ -53,6 +53,9 @@ obj-$(CONFIG_BPF_SYSCALL) += relo_core.o > > obj-$(CONFIG_BPF_SYSCALL) += btf_iter.o > > obj-$(CONFIG_BPF_SYSCALL) += btf_relocate.o > > obj-$(CONFIG_BPF_SYSCALL) += kmem_cache_iter.o > > +ifeq ($(CONFIG_DEBUG_FS),y) > > +obj-$(CONFIG_BPF_SYSCALL) += dmabuf_iter.o > > +endif > > > > CFLAGS_REMOVE_percpu_freelist.o = $(CC_FLAGS_FTRACE) > > CFLAGS_REMOVE_bpf_lru_list.o = $(CC_FLAGS_FTRACE) > > diff --git a/kernel/bpf/dmabuf_iter.c b/kernel/bpf/dmabuf_iter.c > > new file mode 100644 > > index 000000000000..b4b8be1d6aa4 > > --- /dev/null > > +++ b/kernel/bpf/dmabuf_iter.c > > Maybe we should add this file to drivers/dma-buf. I would like to > hear other folks thoughts on this. This is fine with me, and would save us the extra CONFIG_DMA_SHARED_BUFFER check that's currently needed in kernel/bpf/Makefile but would require checking CONFIG_BPF instead. Sumit / Christian any objections to moving the dmabuf bpf iterator implementation into drivers/dma-buf? > > @@ -0,0 +1,130 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* Copyright (c) 2025 Google LLC */ > > +#include <linux/bpf.h> > > +#include <linux/btf_ids.h> > > +#include <linux/dma-buf.h> > > +#include <linux/kernel.h> > > +#include <linux/seq_file.h> > > + > > +BTF_ID_LIST_GLOBAL_SINGLE(bpf_dmabuf_btf_id, struct, dma_buf) > > Use BTF_ID_LIST_SINGLE(), then we don't need this in btf_ids.h > > > +DEFINE_BPF_ITER_FUNC(dmabuf, struct bpf_iter_meta *meta, struct dma_buf *dmabuf) > > + > > +static void *dmabuf_iter_seq_start(struct seq_file *seq, loff_t *pos) > > +{ > > + struct dma_buf *dmabuf, *ret = NULL; > > + > > + if (*pos) { > > + *pos = 0; > > + return NULL; > > + } > > + /* Look for the first buffer we can obtain a reference to. > > + * The list mutex does not protect a dmabuf's refcount, so it can be > > + * zeroed while we are iterating. Therefore we cannot call get_dma_buf() > > + * since the caller of this program may not already own a reference to > > + * the buffer. > > + */ > > We should use kernel comment style for new code. IOW, > /* > * Look for ... > */ > > > Thanks, > Song Thanks, I have incorporated all of your comments and retested. I will give some time for more feedback before sending a v2. > > [...]

1 month, 2 weeks

1
0
0 0

Re: [PATCH 2/4] bpf: Add dmabuf iterator

by T.J. Mercier

On Thu, Apr 17, 2025 at 1:26 PM Song Liu <song(a)kernel.org> wrote: > > On Thu, Apr 17, 2025 at 9:05 AM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > On Wed, Apr 16, 2025 at 9:56 PM Song Liu <song(a)kernel.org> wrote: > > > > > > On Wed, Apr 16, 2025 at 7:09 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > > > > > On Wed, Apr 16, 2025 at 6:26 PM Song Liu <song(a)kernel.org> wrote: > > > [...] > > > > > > > > > > Here is another rookie question, it appears to me there is a file descriptor > > > > > associated with each DMA buffer, can we achieve the same goal with > > > > > a task-file iterator? > > > > > > > > That would find almost all of them, but not the kernel-only > > > > allocations. (kernel_rss in the dmabuf_dump output I attached earlier. > > > > If there's a leak, it's likely to show up in kernel_rss because some > > > > driver forgot to release its reference(s).) Also wouldn't that be a > > > > ton more iterations since we'd have to visit every FD to find the > > > > small portion that are dmabufs? I'm not actually sure if buffers that > > > > have been mapped, and then have had their file descriptors closed > > > > would show up in task_struct->files; if not I think that would mean > > > > scanning both files and vmas for each task. > > > > > > I don't think scanning all FDs to find a small portion of specific FDs > > > is a real issue. We have a tool that scans all FDs in the system and > > > only dump data for perf_event FDs. I think it should be easy to > > > prototype a tool by scanning all files and all vmas. If that turns out > > > to be very slow, which I highly doubt will be, we can try other > > > approaches. > > > > But this will not find *all* the buffers, and that defeats the purpose > > of having the iterator. > > Do you mean this approach cannot get kernel only allocations? If > that's the case, we probably do need a separate iterator. I am > interested in other folks thoughts on this. Correct. > > > OTOH, I am wondering whether we can build a more generic iterator > > > for a list of objects. Adding a iterator for each important kernel lists > > > seems not scalable in the long term. > > > > I think the wide variety of differences in locking for different > > objects would make this difficult to do in a generic way. > > Agreed it is not easy to build a generic solution. But with the > help from BTF, we can probably build something that covers > a large number of use cases. I'm curious what this would look like. I guess a good test would be to see if anything would work for some subset of the already existing iterators.

1 month, 2 weeks

1
0
0 0

Re: [PATCH 2/4] bpf: Add dmabuf iterator

by T.J. Mercier

On Wed, Apr 16, 2025 at 9:56 PM Song Liu <song(a)kernel.org> wrote: > > On Wed, Apr 16, 2025 at 7:09 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > On Wed, Apr 16, 2025 at 6:26 PM Song Liu <song(a)kernel.org> wrote: > [...] > > > > > > Here is another rookie question, it appears to me there is a file descriptor > > > associated with each DMA buffer, can we achieve the same goal with > > > a task-file iterator? > > > > That would find almost all of them, but not the kernel-only > > allocations. (kernel_rss in the dmabuf_dump output I attached earlier. > > If there's a leak, it's likely to show up in kernel_rss because some > > driver forgot to release its reference(s).) Also wouldn't that be a > > ton more iterations since we'd have to visit every FD to find the > > small portion that are dmabufs? I'm not actually sure if buffers that > > have been mapped, and then have had their file descriptors closed > > would show up in task_struct->files; if not I think that would mean > > scanning both files and vmas for each task. > > I don't think scanning all FDs to find a small portion of specific FDs > is a real issue. We have a tool that scans all FDs in the system and > only dump data for perf_event FDs. I think it should be easy to > prototype a tool by scanning all files and all vmas. If that turns out > to be very slow, which I highly doubt will be, we can try other > approaches. But this will not find *all* the buffers, and that defeats the purpose of having the iterator. > OTOH, I am wondering whether we can build a more generic iterator > for a list of objects. Adding a iterator for each important kernel lists > seems not scalable in the long term. I think the wide variety of differences in locking for different objects would make this difficult to do in a generic way.

1 month, 3 weeks

1
0
0 0

Re: [PATCH v8 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

by Steven Price

On 17/04/2025 15:46, Adrián Larumbe wrote: > Hi Steve, > > On 17.04.2025 15:25, Steven Price wrote: >> On 15/04/2025 20:15, Adrián Larumbe wrote: >>> Add a device DebugFS file that displays a complete list of all the DRM >>> GEM objects that are exposed to UM through a DRM handle. >>> >>> Since leaking object identifiers that might belong to a different NS is >>> inadmissible, this functionality is only made available in debug builds >>> with DEBUGFS support enabled. >>> >>> File format is that of a table, with each entry displaying a variety of >>> fields with information about each GEM object. >>> >>> Each GEM object entry in the file displays the following information >>> fields: Client PID, BO's global name, reference count, BO virtual size, >>> BO resize size, VM address in its DRM-managed range, BO label and a GEM >>> state flags. >>> >>> There's also a usage flags field for the type of BO, which tells us >>> whether it's a kernel BO and/or mapped onto the FW's address space. >>> >>> Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> >>> Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> >>> Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> >> >> Reviewed-by: Steven Price <steven.price(a)arm.com> >> >> Although I feel I have to point out the table output is much wider than >> it needs to be. I'd personally like it if some of the columns were made >> narrower. But I guess the idea is this is going to be read by a script >> anyway. > > In a previous revision, table format was a lot tighter, but Boris > pointed out the number of BO flags might increase in the future, so in > order to save ourselves some headache form having to reformat > everything, it was best to give those fields some extra padding even if > that means BO label spans over the usual screen width. Yeah, fair enough. It's always awkward with this sorts of interfaces. Outputting something very machine readable (e.g. JSON) would be good for scripts, but then it's terrible as a quick debugging aid. I'll just cook up a simple script to reformat it to something I like ;) Thanks, Steve >> Thanks, >> Steve >> >>> --- >>> drivers/gpu/drm/panthor/panthor_device.c | 5 + >>> drivers/gpu/drm/panthor/panthor_device.h | 11 ++ >>> drivers/gpu/drm/panthor/panthor_drv.c | 26 ++++ >>> drivers/gpu/drm/panthor/panthor_gem.c | 182 +++++++++++++++++++++++ >>> drivers/gpu/drm/panthor/panthor_gem.h | 59 ++++++++ >>> 5 files changed, 283 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c >>> index a9da1d1eeb70..b776e1a2e4f3 100644 >>> --- a/drivers/gpu/drm/panthor/panthor_device.c >>> +++ b/drivers/gpu/drm/panthor/panthor_device.c >>> @@ -184,6 +184,11 @@ int panthor_device_init(struct panthor_device *ptdev) >>> if (ret) >>> return ret; >>> >>> +#ifdef CONFIG_DEBUG_FS >>> + drmm_mutex_init(&ptdev->base, &ptdev->gems.lock); >>> + INIT_LIST_HEAD(&ptdev->gems.node); >>> +#endif >>> + >>> atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED); >>> p = alloc_page(GFP_KERNEL | __GFP_ZERO); >>> if (!p) >>> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h >>> index da6574021664..86206a961b38 100644 >>> --- a/drivers/gpu/drm/panthor/panthor_device.h >>> +++ b/drivers/gpu/drm/panthor/panthor_device.h >>> @@ -205,6 +205,17 @@ struct panthor_device { >>> >>> /** @fast_rate: Maximum device clock frequency. Set by DVFS */ >>> unsigned long fast_rate; >>> + >>> +#ifdef CONFIG_DEBUG_FS >>> + /** @gems: Device-wide list of GEM objects owned by at least one file. */ >>> + struct { >>> + /** @gems.lock: Protects the device-wide list of GEM objects. */ >>> + struct mutex lock; >>> + >>> + /** @node: Used to keep track of all the device's DRM objects */ >>> + struct list_head node; >>> + } gems; >>> +#endif >>> }; >>> >>> struct panthor_gpu_usage { >>> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c >>> index 7660627cafa1..13f0b045c5fd 100644 >>> --- a/drivers/gpu/drm/panthor/panthor_drv.c >>> +++ b/drivers/gpu/drm/panthor/panthor_drv.c >>> @@ -1557,9 +1557,35 @@ static const struct file_operations panthor_drm_driver_fops = { >>> }; >>> >>> #ifdef CONFIG_DEBUG_FS >>> +static int panthor_gems_show(struct seq_file *m, void *data) >>> +{ >>> + struct drm_info_node *node = m->private; >>> + struct drm_device *dev = node->minor->dev; >>> + struct panthor_device *ptdev = container_of(dev, struct panthor_device, base); >>> + >>> + panthor_gem_debugfs_print_bos(ptdev, m); >>> + >>> + return 0; >>> +} >>> + >>> + >>> +static struct drm_info_list panthor_debugfs_list[] = { >>> + {"gems", panthor_gems_show, 0, NULL}, >>> +}; >>> + >>> +static int panthor_gems_debugfs_init(struct drm_minor *minor) >>> +{ >>> + drm_debugfs_create_files(panthor_debugfs_list, >>> + ARRAY_SIZE(panthor_debugfs_list), >>> + minor->debugfs_root, minor); >>> + >>> + return 0; >>> +} >>> + >>> static void panthor_debugfs_init(struct drm_minor *minor) >>> { >>> panthor_mmu_debugfs_init(minor); >>> + panthor_gems_debugfs_init(minor); >>> } >>> #endif >>> >>> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c >>> index 3f4ab5a2f2ae..1e3409c05891 100644 >>> --- a/drivers/gpu/drm/panthor/panthor_gem.c >>> +++ b/drivers/gpu/drm/panthor/panthor_gem.c >>> @@ -11,14 +11,51 @@ >>> #include <drm/panthor_drm.h> >>> >>> #include "panthor_device.h" >>> +#include "panthor_fw.h" >>> #include "panthor_gem.h" >>> #include "panthor_mmu.h" >>> >>> +#ifdef CONFIG_DEBUG_FS >>> +static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev, >>> + struct panthor_gem_object *bo) >>> +{ >>> + INIT_LIST_HEAD(&bo->debugfs.node); >>> + >>> + bo->debugfs.creator.tgid = current->group_leader->pid; >>> + get_task_comm(bo->debugfs.creator.process_name, current->group_leader); >>> + >>> + mutex_lock(&ptdev->gems.lock); >>> + list_add_tail(&bo->debugfs.node, &ptdev->gems.node); >>> + mutex_unlock(&ptdev->gems.lock); >>> +} >>> + >>> +static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) >>> +{ >>> + struct panthor_device *ptdev = container_of(bo->base.base.dev, >>> + struct panthor_device, base); >>> + >>> + if (list_empty(&bo->debugfs.node)) >>> + return; >>> + >>> + mutex_lock(&ptdev->gems.lock); >>> + list_del_init(&bo->debugfs.node); >>> + mutex_unlock(&ptdev->gems.lock); >>> +} >>> + >>> +#else >>> +static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev, >>> + struct panthor_gem_object *bo) >>> +{} >>> +static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) {} >>> +#endif >>> + >>> static void panthor_gem_free_object(struct drm_gem_object *obj) >>> { >>> struct panthor_gem_object *bo = to_panthor_bo(obj); >>> struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem; >>> >>> + panthor_gem_debugfs_bo_rm(bo); >>> + >>> /* >>> * Label might have been allocated with kstrdup_const(), >>> * we need to take that into account when freeing the memory >>> @@ -88,6 +125,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, >>> struct drm_gem_shmem_object *obj; >>> struct panthor_kernel_bo *kbo; >>> struct panthor_gem_object *bo; >>> + u32 debug_flags = PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL; >>> int ret; >>> >>> if (drm_WARN_ON(&ptdev->base, !vm)) >>> @@ -107,7 +145,11 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, >>> kbo->obj = &obj->base; >>> bo->flags = bo_flags; >>> >>> + if (vm == panthor_fw_vm(ptdev)) >>> + debug_flags |= PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED; >>> + >>> panthor_gem_kernel_bo_set_label(kbo, name); >>> + panthor_gem_debugfs_set_usage_flags(to_panthor_bo(kbo->obj), debug_flags); >>> >>> /* The system and GPU MMU page size might differ, which becomes a >>> * problem for FW sections that need to be mapped at explicit address >>> @@ -210,6 +252,8 @@ struct drm_gem_object *panthor_gem_create_object(struct drm_device *ddev, size_t >>> drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock); >>> mutex_init(&obj->label.lock); >>> >>> + panthor_gem_debugfs_bo_add(ptdev, obj); >>> + >>> return &obj->base.base; >>> } >>> >>> @@ -258,6 +302,12 @@ panthor_gem_create_with_handle(struct drm_file *file, >>> /* drop reference from allocate - handle holds it now. */ >>> drm_gem_object_put(&shmem->base); >>> >>> + /* >>> + * No explicit flags are needed in the call below, since the >>> + * function internally sets the INITIALIZED bit for us. >>> + */ >>> + panthor_gem_debugfs_set_usage_flags(bo, 0); >>> + >>> return ret; >>> } >>> >>> @@ -296,3 +346,135 @@ panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label) >>> >>> panthor_gem_bo_set_label(bo->obj, str); >>> } >>> + >>> +#ifdef CONFIG_DEBUG_FS >>> +static void >>> +panthor_gem_debugfs_format_flags(char flags_str[], int flags_len, >>> + const char * const names[], u32 name_count, >>> + u32 flags) >>> +{ >>> + bool first = true; >>> + int offset = 0; >>> + >>> +#define ACC_FLAGS(...) \ >>> + ({ \ >>> + offset += snprintf(flags_str + offset, flags_len - offset, ##__VA_ARGS__); \ >>> + if (offset == flags_len) \ >>> + return; \ >>> + }) >>> + >>> + ACC_FLAGS("%c", '('); >>> + >>> + if (!flags) >>> + ACC_FLAGS("%s", "none"); >>> + >>> + while (flags) { >>> + u32 bit = fls(flags) - 1; >>> + u32 idx = bit + 1; >>> + >>> + if (!first) >>> + ACC_FLAGS("%s", ","); >>> + >>> + if (idx < name_count && names[idx]) >>> + ACC_FLAGS("%s", names[idx]); >>> + >>> + first = false; >>> + flags &= ~BIT(bit); >>> + } >>> + >>> + ACC_FLAGS("%c", ')'); >>> + >>> +#undef ACC_FLAGS >>> +} >>> + >>> +struct gem_size_totals { >>> + size_t size; >>> + size_t resident; >>> + size_t reclaimable; >>> +}; >>> + >>> +static void panthor_gem_debugfs_bo_print(struct panthor_gem_object *bo, >>> + struct seq_file *m, >>> + struct gem_size_totals *totals) >>> +{ >>> + unsigned int refcount = kref_read(&bo->base.base.refcount); >>> + char creator_info[32] = {}; >>> + size_t resident_size; >>> + char gem_state_str[64] = {}; >>> + char gem_usage_str[64] = {}; >>> + u32 gem_usage_flags = bo->debugfs.flags & (u32)~PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED; >>> + u32 gem_state_flags = 0; >>> + >>> + static const char * const gem_state_flags_names[] = { >>> + [PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED] = "imported", >>> + [PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED] = "exported", >>> + }; >>> + >>> + static const char * const gem_usage_flags_names[] = { >>> + [PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL] = "kernel", >>> + [PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED] = "fw-mapped", >>> + }; >>> + >>> + /* Skip BOs being destroyed. */ >>> + if (!refcount) >>> + return; >>> + >>> + resident_size = bo->base.pages != NULL ? bo->base.base.size : 0; >>> + >>> + snprintf(creator_info, sizeof(creator_info), >>> + "%s/%d", bo->debugfs.creator.process_name, bo->debugfs.creator.tgid); >>> + seq_printf(m, "%-32s%-16d%-16d%-16zd%-16zd%-16lx", >>> + creator_info, >>> + bo->base.base.name, >>> + refcount, >>> + bo->base.base.size, >>> + resident_size, >>> + drm_vma_node_start(&bo->base.base.vma_node)); >>> + >>> + if (bo->base.base.import_attach != NULL) >>> + gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED; >>> + if (bo->base.base.dma_buf != NULL) >>> + gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED; >>> + >>> + panthor_gem_debugfs_format_flags(gem_state_str, sizeof(gem_state_str), >>> + gem_state_flags_names, ARRAY_SIZE(gem_state_flags_names), >>> + gem_state_flags); >>> + panthor_gem_debugfs_format_flags(gem_usage_str, sizeof(gem_usage_str), >>> + gem_usage_flags_names, ARRAY_SIZE(gem_usage_flags_names), >>> + gem_usage_flags); >>> + >>> + seq_printf(m, "%-64s%-64s", gem_state_str, gem_usage_str); >>> + >>> + scoped_guard(mutex, &bo->label.lock) { >>> + seq_printf(m, "%s", bo->label.str ? : ""); >>> + } >>> + >>> + seq_puts(m, "\n"); >>> + >>> + totals->size += bo->base.base.size; >>> + totals->resident += resident_size; >>> + if (bo->base.madv > 0) >>> + totals->reclaimable += resident_size; >>> +} >>> + >>> +void panthor_gem_debugfs_print_bos(struct panthor_device *ptdev, >>> + struct seq_file *m) >>> +{ >>> + struct gem_size_totals totals = {0}; >>> + struct panthor_gem_object *bo; >>> + >>> + seq_puts(m, "created-by global-name refcount size resident-size file-offset state usage label\n"); >>> + seq_puts(m, "-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------\n"); >>> + >>> + scoped_guard(mutex, &ptdev->gems.lock) { >>> + list_for_each_entry(bo, &ptdev->gems.node, debugfs.node) { >>> + if (bo->debugfs.flags & PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED) >>> + panthor_gem_debugfs_bo_print(bo, m, &totals); >>> + } >>> + } >>> + >>> + seq_puts(m, "=====================================================================================================================================================================================================================================================\n"); >>> + seq_printf(m, "Total size: %zd, Total resident: %zd, Total reclaimable: %zd\n", >>> + totals.size, totals.resident, totals.reclaimable); >>> +} >>> +#endif >>> diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h >>> index 3c09af568e47..94b244f0540e 100644 >>> --- a/drivers/gpu/drm/panthor/panthor_gem.h >>> +++ b/drivers/gpu/drm/panthor/panthor_gem.h >>> @@ -15,6 +15,48 @@ struct panthor_vm; >>> >>> #define PANTHOR_BO_LABEL_MAXLEN 4096 >>> >>> +enum panthor_debugfs_gem_state_flags { >>> + /** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED: GEM BO is PRIME imported. */ >>> + PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED = BIT(0), >>> + >>> + /** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED: GEM BO is PRIME exported. */ >>> + PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED = BIT(1), >>> +}; >>> + >>> +enum panthor_debugfs_gem_usage_flags { >>> + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL: BO is for kernel use only. */ >>> + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL = BIT(0), >>> + >>> + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED: BO is mapped on the FW VM. */ >>> + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED = BIT(1), >>> + >>> + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED: BO is ready for DebugFS display. */ >>> + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED = BIT(31), >>> +}; >>> + >>> +/** >>> + * struct panthor_gem_debugfs - GEM object's DebugFS list information >>> + */ >>> +struct panthor_gem_debugfs { >>> + /** >>> + * @node: Node used to insert the object in the device-wide list of >>> + * GEM objects, to display information about it through a DebugFS file. >>> + */ >>> + struct list_head node; >>> + >>> + /** @creator: Information about the UM process which created the GEM. */ >>> + struct { >>> + /** @creator.process_name: Group leader name in owning thread's process */ >>> + char process_name[TASK_COMM_LEN]; >>> + >>> + /** @creator.tgid: PID of the thread's group leader within its process */ >>> + pid_t tgid; >>> + } creator; >>> + >>> + /** @flags: Combination of panthor_debugfs_gem_usage_flags flags */ >>> + u32 flags; >>> +}; >>> + >>> /** >>> * struct panthor_gem_object - Driver specific GEM object. >>> */ >>> @@ -62,6 +104,10 @@ struct panthor_gem_object { >>> /** @lock.str: Protects access to the @label.str field. */ >>> struct mutex lock; >>> } label; >>> + >>> +#ifdef CONFIG_DEBUG_FS >>> + struct panthor_gem_debugfs debugfs; >>> +#endif >>> }; >>> >>> /** >>> @@ -157,4 +203,17 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, >>> >>> void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo); >>> >>> +#ifdef CONFIG_DEBUG_FS >>> +void panthor_gem_debugfs_print_bos(struct panthor_device *pfdev, >>> + struct seq_file *m); >>> +static inline void >>> +panthor_gem_debugfs_set_usage_flags(struct panthor_gem_object *bo, u32 usage_flags) >>> +{ >>> + bo->debugfs.flags = usage_flags | PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED; >>> +} >>> + >>> +#else >>> +void panthor_gem_debugfs_set_usage_flags(struct panthor_gem_object *bo, u32 usage_flags) {}; >>> +#endif >>> + >>> #endif /* __PANTHOR_GEM_H__ */ > > Adrian Larumbe

1 month, 3 weeks

1
0
0 0

Re: [PATCH v8 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

by Steven Price

On 15/04/2025 20:15, Adrián Larumbe wrote: > Add a device DebugFS file that displays a complete list of all the DRM > GEM objects that are exposed to UM through a DRM handle. > > Since leaking object identifiers that might belong to a different NS is > inadmissible, this functionality is only made available in debug builds > with DEBUGFS support enabled. > > File format is that of a table, with each entry displaying a variety of > fields with information about each GEM object. > > Each GEM object entry in the file displays the following information > fields: Client PID, BO's global name, reference count, BO virtual size, > BO resize size, VM address in its DRM-managed range, BO label and a GEM > state flags. > > There's also a usage flags field for the type of BO, which tells us > whether it's a kernel BO and/or mapped onto the FW's address space. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Steven Price <steven.price(a)arm.com> Although I feel I have to point out the table output is much wider than it needs to be. I'd personally like it if some of the columns were made narrower. But I guess the idea is this is going to be read by a script anyway. Thanks, Steve > --- > drivers/gpu/drm/panthor/panthor_device.c | 5 + > drivers/gpu/drm/panthor/panthor_device.h | 11 ++ > drivers/gpu/drm/panthor/panthor_drv.c | 26 ++++ > drivers/gpu/drm/panthor/panthor_gem.c | 182 +++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_gem.h | 59 ++++++++ > 5 files changed, 283 insertions(+) > > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c > index a9da1d1eeb70..b776e1a2e4f3 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.c > +++ b/drivers/gpu/drm/panthor/panthor_device.c > @@ -184,6 +184,11 @@ int panthor_device_init(struct panthor_device *ptdev) > if (ret) > return ret; > > +#ifdef CONFIG_DEBUG_FS > + drmm_mutex_init(&ptdev->base, &ptdev->gems.lock); > + INIT_LIST_HEAD(&ptdev->gems.node); > +#endif > + > atomic_set(&ptdev->pm.state, PANTHOR_DEVICE_PM_STATE_SUSPENDED); > p = alloc_page(GFP_KERNEL | __GFP_ZERO); > if (!p) > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h > index da6574021664..86206a961b38 100644 > --- a/drivers/gpu/drm/panthor/panthor_device.h > +++ b/drivers/gpu/drm/panthor/panthor_device.h > @@ -205,6 +205,17 @@ struct panthor_device { > > /** @fast_rate: Maximum device clock frequency. Set by DVFS */ > unsigned long fast_rate; > + > +#ifdef CONFIG_DEBUG_FS > + /** @gems: Device-wide list of GEM objects owned by at least one file. */ > + struct { > + /** @gems.lock: Protects the device-wide list of GEM objects. */ > + struct mutex lock; > + > + /** @node: Used to keep track of all the device's DRM objects */ > + struct list_head node; > + } gems; > +#endif > }; > > struct panthor_gpu_usage { > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 7660627cafa1..13f0b045c5fd 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1557,9 +1557,35 @@ static const struct file_operations panthor_drm_driver_fops = { > }; > > #ifdef CONFIG_DEBUG_FS > +static int panthor_gems_show(struct seq_file *m, void *data) > +{ > + struct drm_info_node *node = m->private; > + struct drm_device *dev = node->minor->dev; > + struct panthor_device *ptdev = container_of(dev, struct panthor_device, base); > + > + panthor_gem_debugfs_print_bos(ptdev, m); > + > + return 0; > +} > + > + > +static struct drm_info_list panthor_debugfs_list[] = { > + {"gems", panthor_gems_show, 0, NULL}, > +}; > + > +static int panthor_gems_debugfs_init(struct drm_minor *minor) > +{ > + drm_debugfs_create_files(panthor_debugfs_list, > + ARRAY_SIZE(panthor_debugfs_list), > + minor->debugfs_root, minor); > + > + return 0; > +} > + > static void panthor_debugfs_init(struct drm_minor *minor) > { > panthor_mmu_debugfs_init(minor); > + panthor_gems_debugfs_init(minor); > } > #endif > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c > index 3f4ab5a2f2ae..1e3409c05891 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.c > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > @@ -11,14 +11,51 @@ > #include <drm/panthor_drm.h> > > #include "panthor_device.h" > +#include "panthor_fw.h" > #include "panthor_gem.h" > #include "panthor_mmu.h" > > +#ifdef CONFIG_DEBUG_FS > +static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev, > + struct panthor_gem_object *bo) > +{ > + INIT_LIST_HEAD(&bo->debugfs.node); > + > + bo->debugfs.creator.tgid = current->group_leader->pid; > + get_task_comm(bo->debugfs.creator.process_name, current->group_leader); > + > + mutex_lock(&ptdev->gems.lock); > + list_add_tail(&bo->debugfs.node, &ptdev->gems.node); > + mutex_unlock(&ptdev->gems.lock); > +} > + > +static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) > +{ > + struct panthor_device *ptdev = container_of(bo->base.base.dev, > + struct panthor_device, base); > + > + if (list_empty(&bo->debugfs.node)) > + return; > + > + mutex_lock(&ptdev->gems.lock); > + list_del_init(&bo->debugfs.node); > + mutex_unlock(&ptdev->gems.lock); > +} > + > +#else > +static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev, > + struct panthor_gem_object *bo) > +{} > +static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) {} > +#endif > + > static void panthor_gem_free_object(struct drm_gem_object *obj) > { > struct panthor_gem_object *bo = to_panthor_bo(obj); > struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem; > > + panthor_gem_debugfs_bo_rm(bo); > + > /* > * Label might have been allocated with kstrdup_const(), > * we need to take that into account when freeing the memory > @@ -88,6 +125,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > struct drm_gem_shmem_object *obj; > struct panthor_kernel_bo *kbo; > struct panthor_gem_object *bo; > + u32 debug_flags = PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL; > int ret; > > if (drm_WARN_ON(&ptdev->base, !vm)) > @@ -107,7 +145,11 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > kbo->obj = &obj->base; > bo->flags = bo_flags; > > + if (vm == panthor_fw_vm(ptdev)) > + debug_flags |= PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED; > + > panthor_gem_kernel_bo_set_label(kbo, name); > + panthor_gem_debugfs_set_usage_flags(to_panthor_bo(kbo->obj), debug_flags); > > /* The system and GPU MMU page size might differ, which becomes a > * problem for FW sections that need to be mapped at explicit address > @@ -210,6 +252,8 @@ struct drm_gem_object *panthor_gem_create_object(struct drm_device *ddev, size_t > drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock); > mutex_init(&obj->label.lock); > > + panthor_gem_debugfs_bo_add(ptdev, obj); > + > return &obj->base.base; > } > > @@ -258,6 +302,12 @@ panthor_gem_create_with_handle(struct drm_file *file, > /* drop reference from allocate - handle holds it now. */ > drm_gem_object_put(&shmem->base); > > + /* > + * No explicit flags are needed in the call below, since the > + * function internally sets the INITIALIZED bit for us. > + */ > + panthor_gem_debugfs_set_usage_flags(bo, 0); > + > return ret; > } > > @@ -296,3 +346,135 @@ panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label) > > panthor_gem_bo_set_label(bo->obj, str); > } > + > +#ifdef CONFIG_DEBUG_FS > +static void > +panthor_gem_debugfs_format_flags(char flags_str[], int flags_len, > + const char * const names[], u32 name_count, > + u32 flags) > +{ > + bool first = true; > + int offset = 0; > + > +#define ACC_FLAGS(...) \ > + ({ \ > + offset += snprintf(flags_str + offset, flags_len - offset, ##__VA_ARGS__); \ > + if (offset == flags_len) \ > + return; \ > + }) > + > + ACC_FLAGS("%c", '('); > + > + if (!flags) > + ACC_FLAGS("%s", "none"); > + > + while (flags) { > + u32 bit = fls(flags) - 1; > + u32 idx = bit + 1; > + > + if (!first) > + ACC_FLAGS("%s", ","); > + > + if (idx < name_count && names[idx]) > + ACC_FLAGS("%s", names[idx]); > + > + first = false; > + flags &= ~BIT(bit); > + } > + > + ACC_FLAGS("%c", ')'); > + > +#undef ACC_FLAGS > +} > + > +struct gem_size_totals { > + size_t size; > + size_t resident; > + size_t reclaimable; > +}; > + > +static void panthor_gem_debugfs_bo_print(struct panthor_gem_object *bo, > + struct seq_file *m, > + struct gem_size_totals *totals) > +{ > + unsigned int refcount = kref_read(&bo->base.base.refcount); > + char creator_info[32] = {}; > + size_t resident_size; > + char gem_state_str[64] = {}; > + char gem_usage_str[64] = {}; > + u32 gem_usage_flags = bo->debugfs.flags & (u32)~PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED; > + u32 gem_state_flags = 0; > + > + static const char * const gem_state_flags_names[] = { > + [PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED] = "imported", > + [PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED] = "exported", > + }; > + > + static const char * const gem_usage_flags_names[] = { > + [PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL] = "kernel", > + [PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED] = "fw-mapped", > + }; > + > + /* Skip BOs being destroyed. */ > + if (!refcount) > + return; > + > + resident_size = bo->base.pages != NULL ? bo->base.base.size : 0; > + > + snprintf(creator_info, sizeof(creator_info), > + "%s/%d", bo->debugfs.creator.process_name, bo->debugfs.creator.tgid); > + seq_printf(m, "%-32s%-16d%-16d%-16zd%-16zd%-16lx", > + creator_info, > + bo->base.base.name, > + refcount, > + bo->base.base.size, > + resident_size, > + drm_vma_node_start(&bo->base.base.vma_node)); > + > + if (bo->base.base.import_attach != NULL) > + gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED; > + if (bo->base.base.dma_buf != NULL) > + gem_state_flags |= PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED; > + > + panthor_gem_debugfs_format_flags(gem_state_str, sizeof(gem_state_str), > + gem_state_flags_names, ARRAY_SIZE(gem_state_flags_names), > + gem_state_flags); > + panthor_gem_debugfs_format_flags(gem_usage_str, sizeof(gem_usage_str), > + gem_usage_flags_names, ARRAY_SIZE(gem_usage_flags_names), > + gem_usage_flags); > + > + seq_printf(m, "%-64s%-64s", gem_state_str, gem_usage_str); > + > + scoped_guard(mutex, &bo->label.lock) { > + seq_printf(m, "%s", bo->label.str ? : ""); > + } > + > + seq_puts(m, "\n"); > + > + totals->size += bo->base.base.size; > + totals->resident += resident_size; > + if (bo->base.madv > 0) > + totals->reclaimable += resident_size; > +} > + > +void panthor_gem_debugfs_print_bos(struct panthor_device *ptdev, > + struct seq_file *m) > +{ > + struct gem_size_totals totals = {0}; > + struct panthor_gem_object *bo; > + > + seq_puts(m, "created-by global-name refcount size resident-size file-offset state usage label\n"); > + seq_puts(m, "-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------\n"); > + > + scoped_guard(mutex, &ptdev->gems.lock) { > + list_for_each_entry(bo, &ptdev->gems.node, debugfs.node) { > + if (bo->debugfs.flags & PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED) > + panthor_gem_debugfs_bo_print(bo, m, &totals); > + } > + } > + > + seq_puts(m, "=====================================================================================================================================================================================================================================================\n"); > + seq_printf(m, "Total size: %zd, Total resident: %zd, Total reclaimable: %zd\n", > + totals.size, totals.resident, totals.reclaimable); > +} > +#endif > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index 3c09af568e47..94b244f0540e 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -15,6 +15,48 @@ struct panthor_vm; > > #define PANTHOR_BO_LABEL_MAXLEN 4096 > > +enum panthor_debugfs_gem_state_flags { > + /** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED: GEM BO is PRIME imported. */ > + PANTHOR_DEBUGFS_GEM_STATE_FLAG_IMPORTED = BIT(0), > + > + /** @PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED: GEM BO is PRIME exported. */ > + PANTHOR_DEBUGFS_GEM_STATE_FLAG_EXPORTED = BIT(1), > +}; > + > +enum panthor_debugfs_gem_usage_flags { > + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL: BO is for kernel use only. */ > + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_KERNEL = BIT(0), > + > + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED: BO is mapped on the FW VM. */ > + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED = BIT(1), > + > + /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED: BO is ready for DebugFS display. */ > + PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED = BIT(31), > +}; > + > +/** > + * struct panthor_gem_debugfs - GEM object's DebugFS list information > + */ > +struct panthor_gem_debugfs { > + /** > + * @node: Node used to insert the object in the device-wide list of > + * GEM objects, to display information about it through a DebugFS file. > + */ > + struct list_head node; > + > + /** @creator: Information about the UM process which created the GEM. */ > + struct { > + /** @creator.process_name: Group leader name in owning thread's process */ > + char process_name[TASK_COMM_LEN]; > + > + /** @creator.tgid: PID of the thread's group leader within its process */ > + pid_t tgid; > + } creator; > + > + /** @flags: Combination of panthor_debugfs_gem_usage_flags flags */ > + u32 flags; > +}; > + > /** > * struct panthor_gem_object - Driver specific GEM object. > */ > @@ -62,6 +104,10 @@ struct panthor_gem_object { > /** @lock.str: Protects access to the @label.str field. */ > struct mutex lock; > } label; > + > +#ifdef CONFIG_DEBUG_FS > + struct panthor_gem_debugfs debugfs; > +#endif > }; > > /** > @@ -157,4 +203,17 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm, > > void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo); > > +#ifdef CONFIG_DEBUG_FS > +void panthor_gem_debugfs_print_bos(struct panthor_device *pfdev, > + struct seq_file *m); > +static inline void > +panthor_gem_debugfs_set_usage_flags(struct panthor_gem_object *bo, u32 usage_flags) > +{ > + bo->debugfs.flags = usage_flags | PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED; > +} > + > +#else > +void panthor_gem_debugfs_set_usage_flags(struct panthor_gem_object *bo, u32 usage_flags) {}; > +#endif > + > #endif /* __PANTHOR_GEM_H__ */

1 month, 3 weeks

1
0
0 0

Re: [PATCH] dma-buf: heaps: Set allocation orders for larger page sizes

by Christian König

Am 16.04.25 um 23:51 schrieb Juan Yescas: > On Wed, Apr 16, 2025 at 4:34 AM Christian König > <christian.koenig(a)amd.com> wrote: >> >> >> Am 15.04.25 um 19:19 schrieb Juan Yescas: >>> This change sets the allocation orders for the different page sizes >>> (4k, 16k, 64k) based on PAGE_SHIFT. Before this change, the orders >>> for large page sizes were calculated incorrectly, this caused system >>> heap to allocate from 2% to 4% more memory on 16KiB page size kernels. >>> >>> This change was tested on 4k/16k page size kernels. >>> >>> Signed-off-by: Juan Yescas <jyescas(a)google.com> >>> --- >>> drivers/dma-buf/heaps/system_heap.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c >>> index 26d5dc89ea16..54674c02dcb4 100644 >>> --- a/drivers/dma-buf/heaps/system_heap.c >>> +++ b/drivers/dma-buf/heaps/system_heap.c >>> @@ -50,8 +50,15 @@ static gfp_t order_flags[] = {HIGH_ORDER_GFP, HIGH_ORDER_GFP, LOW_ORDER_GFP}; >>> * to match with the sizes often found in IOMMUs. Using order 4 pages instead >>> * of order 0 pages can significantly improve the performance of many IOMMUs >>> * by reducing TLB pressure and time spent updating page tables. >>> + * >>> + * Note: When the order is 0, the minimum allocation is PAGE_SIZE. The possible >>> + * page sizes for ARM devices could be 4K, 16K and 64K. >>> */ >>> -static const unsigned int orders[] = {8, 4, 0}; >>> +#define ORDER_1M (20 - PAGE_SHIFT) >>> +#define ORDER_64K (16 - PAGE_SHIFT) >>> +#define ORDER_FOR_PAGE_SIZE (0) >>> +static const unsigned int orders[] = {ORDER_1M, ORDER_64K, ORDER_FOR_PAGE_SIZE}; >>> +# >> Good catch, but I think the defines are just overkill. >> >> What you should do instead is to subtract page shift when using the array. >> > There are several occurrences of orders in the file and I think it is > better to do the calculations up front in the array. Would you be ok > if we get rid of the defines as per your suggestion and make the > calculations during the definition of the array. Something like this: > > static const unsigned int orders[] = {20 - PAGE_SHIFT, 16 - PAGE_SHIFT, 0}; Yeah that totally works for me as well. Just make sure that a code comment nearby explains why 20, 16 and 0. >> Apart from that using 1M, 64K and then falling back to 4K just sounds random to me. We have especially pushed back on 64K more than once because it is actually not beneficial in almost all cases. >> > In the hardware where the driver is used, the 64K is beneficial for: > > Arm SMMUv3 (4KB granule size): > 64KB (contiguous bit groups 16 4KB pages) > > SysMMU benefits from 64KB (“large” page) on 4k/16k page sizes. Question could this HW also work with pages larger than 64K? (I strongly expect yes). >> I suggest to fix the code in system_heap_allocate to not over allocate instead and just try the available orders like TTM does. This has proven to be working architecture independent. >> > Do you mean to have an implementation similar to __ttm_pool_alloc()? Yeah something like that. > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree… > > If that is the case, we can try the change, run some benchmarks and > submit the patch if we see positive results. Could be that this doesn't matter for your platform, but it's minimal extra overhead asking for larger chunks first and it just avoids fragmenting everything into 64K chunks. That is kind of important since the code in DMA-heaps should be platform neutral if possible. Regards, Christian. > > Thanks > Juan > >> Regards, >> Christian. >> >>> #define NUM_ORDERS ARRAY_SIZE(orders) >>> >>> static struct sg_table *dup_sg_table(struct sg_table *table)

1 month, 3 weeks

1
0
0 0

Re: [PATCH 2/4] bpf: Add dmabuf iterator

by T.J. Mercier

On Wed, Apr 16, 2025 at 6:26 PM Song Liu <song(a)kernel.org> wrote: > > On Wed, Apr 16, 2025 at 4:40 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > > On Wed, Apr 16, 2025 at 4:08 PM Song Liu <song(a)kernel.org> wrote: > > > > > > On Wed, Apr 16, 2025 at 3:51 PM T.J. Mercier <tjmercier(a)google.com> wrote: > > > [...] > > > > > > > > > > IIUC, the iterator simply traverses elements in a linked list. I feel it is > > > > > an overkill to implement a new BPF iterator for it. > > > > > > > > Like other BPF iterators such as kmem_cache_iter or task_iter. > > > > Cgroup_iter iterates trees instead of lists. This is iterating over > > > > kernel objects just like the docs say, "A BPF iterator is a type of > > > > BPF program that allows users to iterate over specific types of kernel > > > > objects". More complicated iteration should not be a requirement here. > > > > > > > > > Maybe we simply > > > > > use debugging tools like crash or drgn for this? The access with > > > > > these tools will not be protected by the mutex. But from my personal > > > > > experience, this is not a big issue for user space debugging tools. > > > > > > > > drgn is *way* too slow, and even if it weren't the dependencies for > > > > running it aren't available. crash needs debug symbols which also > > > > aren't available on user builds. This is not just for manual > > > > debugging, it's for reporting memory use in production. Or anything > > > > else someone might care to extract like attachment info or refcounts. > > > > > > Could you please share more information about the use cases and > > > the time constraint here, and why drgn is too slow. Is most of the delay > > > comes from parsing DWARF? This is mostly for my curiosity, because > > > I have been thinking about using drgn to do some monitoring in > > > production. > > > > > > Thanks, > > > Song > > > > These RunCommands have 10 second timeouts for example. It's rare that > > I see them get exceeded but it happens occasionally.: > > https://cs.android.com/android/platform/superproject/main/+/main:frameworks… > > Thanks for sharing this information. > > > The last time I used drgn (admittedly back in 2023) it took over a > > minute to iterate through less than 200 cgroups. I'm not sure what the > > root cause of the slowness was, but I'd expect the DWARF processing to > > be done up-front once and the slowness I experienced was not just at > > startup. Eventually I switched over to tracefs for that issue, which > > we still use for some telemetry. > > I haven't tried drgn on Android. On server side, iterating should 200 > cgroups should be fairly fast (< 5 seconds, where DWARF parsing is > the most expensive part). > > > Other uses are by statsd for telemetry, memory reporting on app kills > > or death, and for "dumpsys meminfo". > > Here is another rookie question, it appears to me there is a file descriptor > associated with each DMA buffer, can we achieve the same goal with > a task-file iterator? That would find almost all of them, but not the kernel-only allocations. (kernel_rss in the dmabuf_dump output I attached earlier. If there's a leak, it's likely to show up in kernel_rss because some driver forgot to release its reference(s).) Also wouldn't that be a ton more iterations since we'd have to visit every FD to find the small portion that are dmabufs? I'm not actually sure if buffers that have been mapped, and then have had their file descriptors closed would show up in task_struct->files; if not I think that would mean scanning both files and vmas for each task.

1 month, 3 weeks

1
0
0 0

Re: [PATCH 2/4] bpf: Add dmabuf iterator

by T.J. Mercier

On Wed, Apr 16, 2025 at 4:08 PM Song Liu <song(a)kernel.org> wrote: > > On Wed, Apr 16, 2025 at 3:51 PM T.J. Mercier <tjmercier(a)google.com> wrote: > [...] > > > > > > IIUC, the iterator simply traverses elements in a linked list. I feel it is > > > an overkill to implement a new BPF iterator for it. > > > > Like other BPF iterators such as kmem_cache_iter or task_iter. > > Cgroup_iter iterates trees instead of lists. This is iterating over > > kernel objects just like the docs say, "A BPF iterator is a type of > > BPF program that allows users to iterate over specific types of kernel > > objects". More complicated iteration should not be a requirement here. > > > > > Maybe we simply > > > use debugging tools like crash or drgn for this? The access with > > > these tools will not be protected by the mutex. But from my personal > > > experience, this is not a big issue for user space debugging tools. > > > > drgn is *way* too slow, and even if it weren't the dependencies for > > running it aren't available. crash needs debug symbols which also > > aren't available on user builds. This is not just for manual > > debugging, it's for reporting memory use in production. Or anything > > else someone might care to extract like attachment info or refcounts. > > Could you please share more information about the use cases and > the time constraint here, and why drgn is too slow. Is most of the delay > comes from parsing DWARF? This is mostly for my curiosity, because > I have been thinking about using drgn to do some monitoring in > production. > > Thanks, > Song These RunCommands have 10 second timeouts for example. It's rare that I see them get exceeded but it happens occasionally.: https://cs.android.com/android/platform/superproject/main/+/main:frameworks… The last time I used drgn (admittedly back in 2023) it took over a minute to iterate through less than 200 cgroups. I'm not sure what the root cause of the slowness was, but I'd expect the DWARF processing to be done up-front once and the slowness I experienced was not just at startup. Eventually I switched over to tracefs for that issue, which we still use for some telemetry. Other uses are by statsd for telemetry, memory reporting on app kills or death, and for "dumpsys meminfo". Thanks, T.J.

1 month, 3 weeks

1
0
0 0

Re: [PATCH 2/4] bpf: Add dmabuf iterator

by T.J. Mercier

On Wed, Apr 16, 2025 at 3:02 PM Song Liu <song(a)kernel.org> wrote: > > On Mon, Apr 14, 2025 at 3:53 PM T.J. Mercier <tjmercier(a)google.com> wrote: > [...] > > + > > +BTF_ID_LIST_GLOBAL_SINGLE(bpf_dmabuf_btf_id, struct, dma_buf) > > +DEFINE_BPF_ITER_FUNC(dmabuf, struct bpf_iter_meta *meta, struct dma_buf *dmabuf) > > + > > +static void *dmabuf_iter_seq_start(struct seq_file *seq, loff_t *pos) > > +{ > > + struct dma_buf *dmabuf, *ret = NULL; > > + > > + if (*pos) { > > + *pos = 0; > > + return NULL; > > + } > > + /* Look for the first buffer we can obtain a reference to. > > + * The list mutex does not protect a dmabuf's refcount, so it can be > > + * zeroed while we are iterating. Therefore we cannot call get_dma_buf() > > + * since the caller of this program may not already own a reference to > > + * the buffer. > > + */ > > + mutex_lock(&dmabuf_debugfs_list_mutex); > > + list_for_each_entry(dmabuf, &dmabuf_debugfs_list, list_node) { > > + if (file_ref_get(&dmabuf->file->f_ref)) { > > + ret = dmabuf; > > + break; > > + } > > + } > > + mutex_unlock(&dmabuf_debugfs_list_mutex); > > IIUC, the iterator simply traverses elements in a linked list. I feel it is > an overkill to implement a new BPF iterator for it. Like other BPF iterators such as kmem_cache_iter or task_iter. Cgroup_iter iterates trees instead of lists. This is iterating over kernel objects just like the docs say, "A BPF iterator is a type of BPF program that allows users to iterate over specific types of kernel objects". More complicated iteration should not be a requirement here. > Maybe we simply > use debugging tools like crash or drgn for this? The access with > these tools will not be protected by the mutex. But from my personal > experience, this is not a big issue for user space debugging tools. drgn is *way* too slow, and even if it weren't the dependencies for running it aren't available. crash needs debug symbols which also aren't available on user builds. This is not just for manual debugging, it's for reporting memory use in production. Or anything else someone might care to extract like attachment info or refcounts. > Thanks, > Song > > > > + > > + return ret; > > +}

1 month, 3 weeks

1
0
0 0

[PATCH 0/4] Replace CONFIG_DMABUF_SYSFS_STATS with BPF

by T.J. Mercier

Until CONFIG_DMABUF_SYSFS_STATS was added [1] it was only possible to perform per-buffer accounting with debugfs which is not suitable for production environments. Eventually we discovered the overhead with per-buffer sysfs file creation/removal was significantly impacting allocation and free times, and exacerbated kernfs lock contention. [2] dma_buf_stats_setup() is responsible for 39% of single-page buffer creation duration, or 74% of single-page dma_buf_export() duration when stressing dmabuf allocations and frees. I prototyped a change from per-buffer to per-exporter statistics with a RCU protected list of exporter allocations that accommodates most (but not all) of our use-cases and avoids almost all of the sysfs overhead. While that adds less overhead than per-buffer sysfs, and less even than the maintenance of the dmabuf debugfs_list, it's still *additional* overhead on top of the debugfs_list and doesn't give us per-buffer info. This series uses the existing dmabuf debugfs_list to implement a BPF dmabuf iterator, which adds no overhead to buffer allocation/free and provides per-buffer info. While the kernel must have CONFIG_DEBUG_FS for the dmabuf_iter to be available, debugfs does not need to be mounted. The BPF program loaded by userspace that extracts per-buffer information gets to define its own interface which avoids the lack of ABI stability with debugfs (even if it were mounted). As this is a replacement for our use of CONFIG_DMABUF_SYSFS_STATS, the last patch is a RFC for removing it from the kernel. Please see my suggestion there regarding the timeline for that. [1] https://lore.kernel.org/linux-media/20201210044400.1080308-1-hridya@google.… [2] https://lore.kernel.org/all/20220516171315.2400578-1-tjmercier@google.com/ T.J. Mercier (4): dma-buf: Rename and expose debugfs symbols bpf: Add dmabuf iterator selftests/bpf: Add test for dmabuf_iter RFC: dma-buf: Remove DMA-BUF statistics .../ABI/testing/sysfs-kernel-dmabuf-buffers | 24 --- Documentation/driver-api/dma-buf.rst | 5 - drivers/dma-buf/Kconfig | 15 -- drivers/dma-buf/Makefile | 1 - drivers/dma-buf/dma-buf-sysfs-stats.c | 202 ------------------ drivers/dma-buf/dma-buf-sysfs-stats.h | 35 --- drivers/dma-buf/dma-buf.c | 40 +--- include/linux/btf_ids.h | 1 + include/linux/dma-buf.h | 6 + kernel/bpf/Makefile | 3 + kernel/bpf/dmabuf_iter.c | 130 +++++++++++ tools/testing/selftests/bpf/config | 1 + .../selftests/bpf/prog_tests/dmabuf_iter.c | 116 ++++++++++ .../testing/selftests/bpf/progs/dmabuf_iter.c | 31 +++ 14 files changed, 299 insertions(+), 311 deletions(-) delete mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers delete mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c delete mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h create mode 100644 kernel/bpf/dmabuf_iter.c create mode 100644 tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c create mode 100644 tools/testing/selftests/bpf/progs/dmabuf_iter.c -- 2.49.0.604.gff1f9ca942-goog

1 month, 3 weeks

3
8
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig