Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

21 participants
3590 discussions

Re: [PATCH v3 4/5] drm/panfrost: show device-wide list of DRM GEM objects over DebugFS

by Steven Price

On 20/05/2025 18:44, Adrián Larumbe wrote: > This change is essentially a Panfrost port of commit a3707f53eb3f > ("drm/panthor: show device-wide list of DRM GEM objects over DebugFS"). > > The DebugFS file is almost the same as in Panthor, minus the GEM object > usage flags, since Panfrost has no kernel-only BO's. > > Two additional GEM state flags which are displayed but aren't relevant > to Panthor are 'Purged' and 'Purgeable', since Panfrost implements an > explicit shrinker and a madvise ioctl to flag objects as reclaimable. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> Reviewed-by: Steven Price <steven.price(a)arm.com> > --- > drivers/gpu/drm/panfrost/panfrost_device.c | 5 + > drivers/gpu/drm/panfrost/panfrost_device.h | 15 +++ > drivers/gpu/drm/panfrost/panfrost_drv.c | 35 ++++++ > drivers/gpu/drm/panfrost/panfrost_gem.c | 134 +++++++++++++++++++++ > drivers/gpu/drm/panfrost/panfrost_gem.h | 47 ++++++++ > 5 files changed, 236 insertions(+) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c > index 5d35076b2e6d..04bec27449cb 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -209,6 +209,11 @@ int panfrost_device_init(struct panfrost_device *pfdev) > > spin_lock_init(&pfdev->cycle_counter.lock); > > +#ifdef CONFIG_DEBUG_FS > + mutex_init(&pfdev->debugfs.gems_lock); > + INIT_LIST_HEAD(&pfdev->debugfs.gems_list); > +#endif > + > err = panfrost_pm_domain_init(pfdev); > if (err) > return err; > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h > index dcff70f905cd..077525a3ad68 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.h > +++ b/drivers/gpu/drm/panfrost/panfrost_device.h > @@ -111,6 +111,17 @@ struct panfrost_compatible { > u8 gpu_quirks; > }; > > +/** > + * struct panfrost_device_debugfs - Device-wide DebugFS tracking structures > + */ > +struct panfrost_device_debugfs { > + /** @gems_list: Device-wide list of GEM objects owned by at least one file. */ > + struct list_head gems_list; > + > + /** @gems_lock: Serializes access to the device-wide list of GEM objects. */ > + struct mutex gems_lock; > +}; > + > struct panfrost_device { > struct device *dev; > struct drm_device *ddev; > @@ -164,6 +175,10 @@ struct panfrost_device { > atomic_t use_count; > spinlock_t lock; > } cycle_counter; > + > +#ifdef CONFIG_DEBUG_FS > + struct panfrost_device_debugfs debugfs; > +#endif > }; > > struct panfrost_mmu { > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 179fbaa1cd0c..f576cb215898 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -13,6 +13,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > #include <drm/panfrost_drm.h> > +#include <drm/drm_debugfs.h> > #include <drm/drm_drv.h> > #include <drm/drm_ioctl.h> > #include <drm/drm_syncobj.h> > @@ -659,6 +660,37 @@ static const struct file_operations panfrost_drm_driver_fops = { > .show_fdinfo = drm_show_fdinfo, > }; > > +#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 panfrost_device *pfdev = dev->dev_private; > + > + panfrost_gem_debugfs_print_bos(pfdev, 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 panfrost_debugfs_init(struct drm_minor *minor) > +{ > + panthor_gems_debugfs_init(minor); > +} > +#endif > + > /* > * Panfrost driver version: > * - 1.0 - initial interface > @@ -683,6 +715,9 @@ static const struct drm_driver panfrost_drm_driver = { > > .gem_create_object = panfrost_gem_create_object, > .gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table, > +#ifdef CONFIG_DEBUG_FS > + .debugfs_init = panfrost_debugfs_init, > +#endif > }; > > static int panfrost_probe(struct platform_device *pdev) > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > index 04483d5fb45d..bb73f2a68a12 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > @@ -12,6 +12,36 @@ > #include "panfrost_gem.h" > #include "panfrost_mmu.h" > > +#ifdef CONFIG_DEBUG_FS > +static void panfrost_gem_debugfs_bo_add(struct panfrost_device *pfdev, > + struct panfrost_gem_object *bo) > +{ > + bo->debugfs.creator.tgid = current->group_leader->pid; > + get_task_comm(bo->debugfs.creator.process_name, current->group_leader); > + > + mutex_lock(&pfdev->debugfs.gems_lock); > + list_add_tail(&bo->debugfs.node, &pfdev->debugfs.gems_list); > + mutex_unlock(&pfdev->debugfs.gems_lock); > +} > + > +static void panfrost_gem_debugfs_bo_rm(struct panfrost_gem_object *bo) > +{ > + struct panfrost_device *pfdev = bo->base.base.dev->dev_private; > + > + if (list_empty(&bo->debugfs.node)) > + return; > + > + mutex_lock(&pfdev->debugfs.gems_lock); > + list_del_init(&bo->debugfs.node); > + mutex_unlock(&pfdev->debugfs.gems_lock); > +} > +#else > +static void panfrost_gem_debugfs_bo_add(struct panfrost_device *pfdev, > + struct panfrost_gem_object *bo) > +{} > +static void panfrost_gem_debugfs_bo_rm(struct panfrost_gem_object *bo) {} > +#endif > + > /* Called DRM core on the last userspace/kernel unreference of the > * BO. > */ > @@ -37,6 +67,7 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) > WARN_ON_ONCE(!list_empty(&bo->mappings.list)); > > kfree_const(bo->label.str); > + panfrost_gem_debugfs_bo_rm(bo); > mutex_destroy(&bo->label.lock); > > if (bo->sgts) { > @@ -266,6 +297,8 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t > obj->base.map_wc = !pfdev->coherent; > mutex_init(&obj->label.lock); > > + panfrost_gem_debugfs_bo_add(pfdev, obj); > + > return &obj->base.base; > } > > @@ -354,3 +387,104 @@ panfrost_gem_internal_set_label(struct drm_gem_object *obj, const char *label) > > panfrost_gem_set_label(obj, str); > } > + > +#ifdef CONFIG_DEBUG_FS > +struct gem_size_totals { > + size_t size; > + size_t resident; > + size_t reclaimable; > +}; > + > +struct flag_def { > + u32 flag; > + const char *name; > +}; > + > +static void panfrost_gem_debugfs_print_flag_names(struct seq_file *m) > +{ > + int len; > + int i; > + > + static const struct flag_def gem_state_flags_names[] = { > + {PANFROST_DEBUGFS_GEM_STATE_FLAG_IMPORTED, "imported"}, > + {PANFROST_DEBUGFS_GEM_STATE_FLAG_EXPORTED, "exported"}, > + {PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGED, "purged"}, > + {PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGEABLE, "purgeable"}, > + }; > + > + seq_puts(m, "GEM state flags: "); > + for (i = 0, len = ARRAY_SIZE(gem_state_flags_names); i < len; i++) { > + seq_printf(m, "%s (0x%x)%s", gem_state_flags_names[i].name, > + gem_state_flags_names[i].flag, (i < len - 1) ? ", " : "\n\n"); > + } > +} > + > +static void panfrost_gem_debugfs_bo_print(struct panfrost_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; > + u32 gem_state_flags = 0; > + > + /* Skip BOs being destroyed. */ > + if (!refcount) > + return; > + > + resident_size = bo->base.pages ? 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%-16zd0x%-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) > + gem_state_flags |= PANFROST_DEBUGFS_GEM_STATE_FLAG_IMPORTED; > + if (bo->base.base.dma_buf) > + gem_state_flags |= PANFROST_DEBUGFS_GEM_STATE_FLAG_EXPORTED; > + > + if (bo->base.madv < 0) > + gem_state_flags |= PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGED; > + else if (bo->base.madv > 0) > + gem_state_flags |= PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGEABLE; > + > + seq_printf(m, "0x%-10x", gem_state_flags); > + > + scoped_guard(mutex, &bo->label.lock) { > + seq_printf(m, "%s\n", bo->label.str ? : ""); > + } > + > + totals->size += bo->base.base.size; > + totals->resident += resident_size; > + if (bo->base.madv > 0) > + totals->reclaimable += resident_size; > +} > + > +void panfrost_gem_debugfs_print_bos(struct panfrost_device *pfdev, > + struct seq_file *m) > +{ > + struct gem_size_totals totals = {0}; > + struct panfrost_gem_object *bo; > + > + panfrost_gem_debugfs_print_flag_names(m); > + > + seq_puts(m, "created-by global-name refcount size resident-size file-offset state label\n"); > + seq_puts(m, "-----------------------------------------------------------------------------------------------------------------------------------\n"); > + > + scoped_guard(mutex, &pfdev->debugfs.gems_lock) { > + list_for_each_entry(bo, &pfdev->debugfs.gems_list, debugfs.node) { > + panfrost_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/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h > index 3d87c41ad09d..8de3e76f2717 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.h > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h > @@ -8,9 +8,47 @@ > #include <drm/drm_mm.h> > > struct panfrost_mmu; > +struct panfrost_device; > > #define PANFROST_BO_LABEL_MAXLEN 4096 > > +enum panfrost_debugfs_gem_state_flags { > + /** @PANFROST_DEBUGFS_GEM_STATE_FLAG_IMPORTED: GEM BO is PRIME imported. */ > + PANFROST_DEBUGFS_GEM_STATE_FLAG_IMPORTED = BIT(0), > + > + /** @PANFROST_DEBUGFS_GEM_STATE_FLAG_EXPORTED: GEM BO is PRIME exported. */ > + PANFROST_DEBUGFS_GEM_STATE_FLAG_EXPORTED = BIT(1), > + > + /** @PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGED: GEM BO was reclaimed by the shrinker. */ > + PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGED = BIT(2), > + > + /** > + * @PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGEABLE: GEM BO pages were marked as no longer > + * needed by UM and can be reclaimed by the shrinker. > + */ > + PANFROST_DEBUGFS_GEM_STATE_FLAG_PURGEABLE = BIT(3), > +}; > + > +/** > + * struct panfrost_gem_debugfs - GEM object's DebugFS list information > + */ > +struct panfrost_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; > +}; > + > struct panfrost_gem_object { > struct drm_gem_shmem_object base; > struct sg_table *sgts; > @@ -59,6 +97,10 @@ struct panfrost_gem_object { > > bool noexec :1; > bool is_heap :1; > + > +#ifdef CONFIG_DEBUG_FS > + struct panfrost_gem_debugfs debugfs; > +#endif > }; > > struct panfrost_gem_mapping { > @@ -108,4 +150,9 @@ void panfrost_gem_shrinker_cleanup(struct drm_device *dev); > void panfrost_gem_set_label(struct drm_gem_object *obj, const char *label); > void panfrost_gem_internal_set_label(struct drm_gem_object *obj, const char *label); > > +#ifdef CONFIG_DEBUG_FS > +void panfrost_gem_debugfs_print_bos(struct panfrost_device *pfdev, > + struct seq_file *m); > +#endif > + > #endif /* __PANFROST_GEM_H__ */

1 year, 4 months

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

by Maxime Ripard

Hi Jared, On Thu, Apr 24, 2025 at 09:11:24AM -0700, Jared Kangas wrote: > > > struct cma_heap { > > > struct dma_heap *heap; > > > @@ -394,15 +395,26 @@ static int __init __add_cma_heap(struct cma *cma, const char *name) > > > static int __init add_default_cma_heap(void) > > > { > > > struct cma *default_cma = dev_get_cma_area(NULL); > > > + const char *legacy_cma_name; > > > int ret; > > > > > > if (!default_cma) > > > return 0; > > > > > > - ret = __add_cma_heap(default_cma, cma_get_name(default_cma)); > > > + ret = __add_cma_heap(default_cma, DEFAULT_CMA_NAME); > > > if (ret) > > > return ret; > > > > > > + legacy_cma_name = cma_get_name(default_cma); > > > + > > > + if (IS_ENABLED(CONFIG_DMABUF_HEAPS_CMA_LEGACY) && > > > + strcmp(legacy_cma_name, DEFAULT_CMA_NAME)) { > > > + ret = __add_cma_heap(default_cma, legacy_cma_name); > > > + if (ret) > > > + pr_warn("cma_heap: failed to add legacy heap: %pe\n", > > > + ERR_PTR(-ret)); > > > + } > > > + > > > > It would also simplify this part, since you would always create the legacy heap. > > By "always", do you mean removing the strcmp? I added this to guard > against cases where the devicetree node's name clashed with the default > name, given that the DT name isn't necessarily restricted to one of the > current names in use ("linux,cma" or "default-pool"). It seems like the > strcmp would be relevant regardless of the naming choice, but if this is > overly cautious, I can remove it in v3. That's not overly cautious, that's something I overlooked :) You're totally right that we should check for that. We should probably add a more specific error message in that case though Maxime

1 year, 4 months

Re: [PATCH 2/2] dmabuf/heaps: implement DMA_BUF_IOCTL_RW_FILE for system_heap

by Christian König

On 5/21/25 12:25, wangtao wrote: > [wangtao] I previously explained that read/sendfile/splice/copy_file_range > syscalls can't achieve dmabuf direct IO zero-copy. And why can't you work on improving those syscalls instead of creating a new IOCTL? > My focus is enabling dmabuf direct I/O for [regular file] <--DMA--> [dmabuf] > zero-copy. Yeah and that focus is wrong. You need to work on a general solution to the issue and not specific to your problem. > Any API achieving this would work. Are there other uAPIs you think > could help? Could you recommend experts who might offer suggestions? Well once more: Either work on sendfile or copy_file_range or eventually splice to make it what you want to do. When that is done we can discuss with the VFS people if that approach is feasible. But just bypassing the VFS review by implementing a DMA-buf specific IOCTL is a NO-GO. That is clearly not something you can do in any way. Regards, Christian.

1 year, 4 months

Re: [PATCH 2/2] dmabuf/heaps: implement DMA_BUF_IOCTL_RW_FILE for system_heap

by Christian König

On 5/21/25 06:17, wangtao wrote: >>> Reducing CPU overhead/power consumption is critical for mobile devices. >>> We need simpler and more efficient dmabuf direct I/O support. >>> >>> As Christian evaluated sendfile performance based on your data, could >>> you confirm whether the cache was cleared? If not, please share the >>> post-cache-clearing test data. Thank you for your support. >> >> Yes sorry, I was out yesterday riding motorcycles. I did not clear the cache for >> the buffered reads, I didn't realize you had. The IO plus the copy certainly >> explains the difference. >> >> Your point about the unlikelihood of any of that data being in the cache also >> makes sense. > [wangtao] Thank you for testing and clarifying. > >> >> I'm not sure it changes anything about the ioctl approach though. >> Another way to do this would be to move the (optional) support for direct IO >> into the exporter via dma_buf_fops and dma_buf_ops. Then normal read() >> syscalls would just work for buffers that support them. >> I know that's more complicated, but at least it doesn't require inventing new >> uapi to do it. >> > [wangtao] Thank you for the discussion. I fully support any method that enables > dmabuf direct I/O. > > I understand using sendfile/splice with regular files for dmabuf > adds an extra CPU copy, preventing zero-copy. For example: > sendfile path: [DISK] → DMA → [page cache] → CPU copy → [memory file]. Yeah, but why can't you work on improving that? > The read() syscall can't pass regular file fd parameters, so I added > an ioctl command. > While copy_file_range() supports two fds (fd_in/fd_out), it blocks cross-fs use. > Even without this restriction, file_out->f_op->copy_file_range > only enables dmabuf direct reads from regular files, not writes. > > Since dmabuf's direct I/O limitation comes from its unique > attachment/map/fence model and lacks suitable syscalls, adding > an ioctl seems necessary. I absolutely don't see that. Both splice and sendfile can take two regular file descriptors. That the underlying fops currently can't do that is not a valid argument for adding new uAPI. It just means that you need to work on improving those fops. As long as nobody proves to me that the existing uAPI isn't sufficient for this use case I will systematically reject any approach to adding new one. Regards, Christian. > When system exporters return a duplicated sg_table via map_dma_buf > (used exclusively like a pages array), they should retain control > over it. > > I welcome all solutions to achieve dmabuf direct I/O! Your feedback > is greatly appreciated. > >> 1G from ext4 on 6.12.20 | read/sendfile (ms) w/ 3 > drop_caches >> ------------------------|------------------- >> udmabuf buffer read | 1210 >> udmabuf direct read | 671 >> udmabuf buffer sendfile | 1096 >> udmabuf direct sendfile | 2340 >> >> >> >>> >>>>> >>>>>>> dmabuf buffer read | 51 | 1068 | 1118 >>>>>>> dmabuf direct read | 52 | 297 | 349 >>>>>>> >>>>>>> udmabuf sendfile test steps: >>>>>>> 1. Open data file(1024MB), get back_fd 2. Create memfd(32MB) # >>>>>>> Loop steps 2-6 3. Allocate udmabuf with memfd 4. Call >>>>>>> sendfile(memfd, >>>>>>> back_fd) 5. Close memfd after sendfile 6. Close udmabuf 7. >>>>>>> Close back_fd >>>>>>> >>>>>>>> >>>>>>>> Regards, >>>>>>>> Christian. >>>>>>> >>>>>> >>>

1 year, 4 months

Re: [PATCH v5 01/10] dt-bindings: npu: rockchip,rknn: Add bindings

by Krzysztof Kozlowski

On Tue, May 20, 2025 at 12:26:54PM GMT, Tomeu Vizoso wrote: > Add the bindings for the Neural Processing Unit IP from Rockchip. > > v2: > - Adapt to new node structure (one node per core, each with its own > IOMMU) > - Several misc. fixes from Sebastian Reichel > > v3: > - Split register block in its constituent subblocks, and only require > the ones that the kernel would ever use (Nicolas Frattaroli) > - Group supplies (Rob Herring) > - Explain the way in which the top core is special (Rob Herring) > > v4: > - Change required node name to npu@ (Rob Herring and Krzysztof Kozlowski) > - Remove unneeded items: (Krzysztof Kozlowski) > - Fix use of minItems/maxItems (Krzysztof Kozlowski) > - Add reg-names to list of required properties (Krzysztof Kozlowski) > - Fix example (Krzysztof Kozlowski) > > v5: > - Rename file to rockchip,rk3588-rknn-core.yaml (Krzysztof Kozlowski) > - Streamline compatible property (Krzysztof Kozlowski) > This is a big patchset, so please slow down and do not send it every day but allow people to actually review the version you post. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)linaro.org> Best regards, Krzysztof

1 year, 4 months

Re: [PATCH 2/2] dmabuf/heaps: implement DMA_BUF_IOCTL_RW_FILE for system_heap

by T.J. Mercier

On Mon, May 19, 2025 at 9:06 PM wangtao <tao.wangtao(a)honor.com> wrote: > > > > > -----Original Message----- > > From: wangtao > > Sent: Monday, May 19, 2025 8:04 PM > > To: 'T.J. Mercier' <tjmercier(a)google.com>; Christian König > > <christian.koenig(a)amd.com> > > Cc: sumit.semwal(a)linaro.org; benjamin.gaignard(a)collabora.com; > > Brian.Starkey(a)arm.com; jstultz(a)google.com; linux-media(a)vger.kernel.org; > > dri-devel(a)lists.freedesktop.org; linaro-mm-sig(a)lists.linaro.org; linux- > > kernel(a)vger.kernel.org; wangbintian(BintianWang) > > <bintian.wang(a)honor.com>; yipengxiang <yipengxiang(a)honor.com>; liulu > > 00013167 <liulu.liu(a)honor.com>; hanfeng 00012985 <feng.han(a)honor.com> > > Subject: RE: [PATCH 2/2] dmabuf/heaps: implement > > DMA_BUF_IOCTL_RW_FILE for system_heap > > > > > > > > > -----Original Message----- > > > From: T.J. Mercier <tjmercier(a)google.com> > > > Sent: Saturday, May 17, 2025 2:37 AM > > > To: Christian König <christian.koenig(a)amd.com> > > > Cc: wangtao <tao.wangtao(a)honor.com>; sumit.semwal(a)linaro.org; > > > benjamin.gaignard(a)collabora.com; Brian.Starkey(a)arm.com; > > > jstultz(a)google.com; linux-media(a)vger.kernel.org; dri- > > > devel(a)lists.freedesktop.org; linaro-mm-sig(a)lists.linaro.org; linux- > > > kernel(a)vger.kernel.org; wangbintian(BintianWang) > > > <bintian.wang(a)honor.com>; yipengxiang <yipengxiang(a)honor.com>; liulu > > > 00013167 <liulu.liu(a)honor.com>; hanfeng 00012985 > > <feng.han(a)honor.com> > > > Subject: Re: [PATCH 2/2] dmabuf/heaps: implement > > DMA_BUF_IOCTL_RW_FILE > > > for system_heap > > > > > > On Fri, May 16, 2025 at 1:36 AM Christian König > > > <christian.koenig(a)amd.com> > > > wrote: > > > > > > > > On 5/16/25 09:40, wangtao wrote: > > > > > > > > > > > > > > >> -----Original Message----- > > > > >> From: Christian König <christian.koenig(a)amd.com> > > > > >> Sent: Thursday, May 15, 2025 10:26 PM > > > > >> To: wangtao <tao.wangtao(a)honor.com>; sumit.semwal(a)linaro.org; > > > > >> benjamin.gaignard(a)collabora.com; Brian.Starkey(a)arm.com; > > > > >> jstultz(a)google.com; tjmercier(a)google.com > > > > >> Cc: linux-media(a)vger.kernel.org; dri-devel(a)lists.freedesktop.org; > > > > >> linaro- mm-sig(a)lists.linaro.org; linux-kernel(a)vger.kernel.org; > > > > >> wangbintian(BintianWang) <bintian.wang(a)honor.com>; yipengxiang > > > > >> <yipengxiang(a)honor.com>; liulu 00013167 <liulu.liu(a)honor.com>; > > > > >> hanfeng > > > > >> 00012985 <feng.han(a)honor.com> > > > > >> Subject: Re: [PATCH 2/2] dmabuf/heaps: implement > > > > >> DMA_BUF_IOCTL_RW_FILE for system_heap > > > > >> > > > > >> On 5/15/25 16:03, wangtao wrote: > > > > >>> [wangtao] My Test Configuration (CPU 1GHz, 5-test average): > > > > >>> Allocation: 32x32MB buffer creation > > > > >>> - dmabuf 53ms vs. udmabuf 694ms (10X slower) > > > > >>> - Note: shmem shows excessive allocation time > > > > >> > > > > >> Yeah, that is something already noted by others as well. But that > > > > >> is orthogonal. > > > > >> > > > > >>> > > > > >>> Read 1024MB File: > > > > >>> - dmabuf direct 326ms vs. udmabuf direct 461ms (40% slower) > > > > >>> - Note: pin_user_pages_fast consumes majority CPU cycles > > > > >>> > > > > >>> Key function call timing: See details below. > > > > >> > > > > >> Those aren't valid, you are comparing different functionalities here. > > > > >> > > > > >> Please try using udmabuf with sendfile() as confirmed to be > > > > >> working by > > > T.J. > > > > > [wangtao] Using buffer IO with dmabuf file read/write requires one > > > memory copy. > > > > > Direct IO removes this copy to enable zero-copy. The sendfile > > > > > system call reduces memory copies from two (read/write) to one. > > > > > However, with udmabuf, sendfile still keeps at least one copy, failing > > zero-copy. > > > > > > > > > > > > Then please work on fixing this. > > > > > > > > Regards, > > > > Christian. > > > > > > > > > > > > > > > > > > If udmabuf sendfile uses buffer IO (file page cache), read latency > > > > > matches dmabuf buffer read, but allocation time is much longer. > > > > > With Direct IO, the default 16-page pipe size makes it slower than > > > > > buffer > > > IO. > > > > > > > > > > Test data shows: > > > > > udmabuf direct read is much faster than udmabuf sendfile. > > > > > dmabuf direct read outperforms udmabuf direct read by a large margin. > > > > > > > > > > Issue: After udmabuf is mapped via map_dma_buf, apps using memfd > > > > > or udmabuf for Direct IO might cause errors, but there are no > > > > > safeguards to prevent this. > > > > > > > > > > Allocate 32x32MB buffer and read 1024 MB file Test: > > > > > Metric | alloc (ms) | read (ms) | total (ms) > > > > > -----------------------|------------|-----------|----------- > > > > > udmabuf buffer read | 539 | 2017 | 2555 > > > > > udmabuf direct read | 522 | 658 | 1179 > > > > > > I can't reproduce the part where udmabuf direct reads are faster than > > > buffered reads. That's the opposite of what I'd expect. Something > > > seems wrong with those buffered reads. > > > > > > > > udmabuf buffer sendfile| 505 | 1040 | 1546 > > > > > udmabuf direct sendfile| 510 | 2269 | 2780 > > > > > > I can reproduce the 3.5x slower udambuf direct sendfile compared to > > > udmabuf direct read. It's a pretty disappointing result, so it seems > > > like something could be improved there. > > > > > > 1G from ext4 on 6.12.17 | read/sendfile (ms) > > > ------------------------|------------------- > > > udmabuf buffer read | 351 > > > udmabuf direct read | 540 > > > udmabuf buffer sendfile | 255 > > > udmabuf direct sendfile | 1990 > > > > > [wangtao] By the way, did you clear the file cache during testing? > > Looking at your data again, read and sendfile buffers are faster than Direct > > I/O, which suggests the file cache wasn’t cleared. If you didn’t clear the file > > cache, the test results are unfair and unreliable for reference. On embedded > > devices, it’s nearly impossible to maintain stable caching for multi-GB files. If > > such files could be cached, we might as well cache dmabufs directly to save > > time on creating dmabufs and reading file data. > > You can call posix_fadvise(file_fd, 0, len, POSIX_FADV_DONTNEED) after > > opening the file or before closing it to clear the file cache, ensuring actual file > > I/O operations are tested. > > > [wangtao] Please confirm if cache clearing was performed during testing. > I reduced the test scope from 3GB to 1GB. While results without > cache clearing show general alignment, udmabuf buffer read remains > slower than direct read. Comparative data: > > Your test reading 1GB(ext4 on 6.12.17: > Method | read/sendfile (ms) | read vs. (%) > ---------------------------------------------------------- > udmabuf buffer read | 351 | 138% > udmabuf direct read | 540 | 212% > udmabuf buffer sendfile | 255 | 100% > udmabuf direct sendfile | 1990 | 780% > > My 3.5GHz tests (f2fs): > Without cache clearing: > Method | alloc | read | vs. (%) > ----------------------------------------------- > udmabuf buffer read | 140 | 386 | 310% > udmabuf direct read | 151 | 326 | 262% > udmabuf buffer sendfile | 136 | 124 | 100% > udmabuf direct sendfile | 132 | 892 | 717% > dmabuf buffer read | 23 | 154 | 124% > patch direct read | 29 | 271 | 218% > > With cache clearing: > Method | alloc | read | vs. (%) > ----------------------------------------------- > udmabuf buffer read | 135 | 546 | 180% > udmabuf direct read | 159 | 300 | 99% > udmabuf buffer sendfile | 134 | 303 | 100% > udmabuf direct sendfile | 141 | 912 | 301% > dmabuf buffer read | 22 | 362 | 119% > patch direct read | 29 | 265 | 87% > > Results without cache clearing aren't representative for embedded > mobile devices. Notably, on low-power CPUs @1GHz, sendfile latency > without cache clearing exceeds dmabuf direct I/O read time. > > Without cache clearing: > Method | alloc | read | vs. (%) > ----------------------------------------------- > udmabuf buffer read | 546 | 1745 | 442% > udmabuf direct read | 511 | 704 | 178% > udmabuf buffer sendfile | 496 | 395 | 100% > udmabuf direct sendfile | 498 | 2332 | 591% > dmabuf buffer read | 43 | 453 | 115% > my patch direct read | 49 | 310 | 79% > > With cache clearing: > Method | alloc | read | vs. (%) > ----------------------------------------------- > udmabuf buffer read | 552 | 2067 | 198% > udmabuf direct read | 540 | 627 | 60% > udmabuf buffer sendfile | 497 | 1045 | 100% > udmabuf direct sendfile | 527 | 2330 | 223% > dmabuf buffer read | 40 | 1111 | 106% > my patch direct read | 44 | 310 | 30% > > Reducing CPU overhead/power consumption is critical for mobile devices. > We need simpler and more efficient dmabuf direct I/O support. > > As Christian evaluated sendfile performance based on your data, could > you confirm whether the cache was cleared? If not, please share the > post-cache-clearing test data. Thank you for your support. Yes sorry, I was out yesterday riding motorcycles. I did not clear the cache for the buffered reads, I didn't realize you had. The IO plus the copy certainly explains the difference. Your point about the unlikelihood of any of that data being in the cache also makes sense. I'm not sure it changes anything about the ioctl approach though. Another way to do this would be to move the (optional) support for direct IO into the exporter via dma_buf_fops and dma_buf_ops. Then normal read() syscalls would just work for buffers that support them. I know that's more complicated, but at least it doesn't require inventing new uapi to do it. 1G from ext4 on 6.12.20 | read/sendfile (ms) w/ 3 > drop_caches ------------------------|------------------- udmabuf buffer read | 1210 udmabuf direct read | 671 udmabuf buffer sendfile | 1096 udmabuf direct sendfile | 2340 > > > > > > > > > dmabuf buffer read | 51 | 1068 | 1118 > > > > > dmabuf direct read | 52 | 297 | 349 > > > > > > > > > > udmabuf sendfile test steps: > > > > > 1. Open data file(1024MB), get back_fd 2. Create memfd(32MB) # > > > > > Loop steps 2-6 3. Allocate udmabuf with memfd 4. Call > > > > > sendfile(memfd, > > > > > back_fd) 5. Close memfd after sendfile 6. Close udmabuf 7. Close > > > > > back_fd > > > > > > > > > >> > > > > >> Regards, > > > > >> Christian. > > > > > > > > > >

1 year, 4 months

[PATCH v4] dma-buf: heaps: Introduce a new heap for reserved memory

by Maxime Ripard

Some reserved memory regions might have particular memory setup or attributes that make them good candidates for heaps. Let's provide a heap type that will create a new heap for each reserved memory region flagged as such. Signed-off-by: Maxime Ripard <mripard(a)kernel.org> --- 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 --- Changes in v4: - Rebased on 6.15-rc7 - Map buffers only when map is actually called, not at allocation time - Deal with restricted-dma-pool and shared-dma-pool - Reword Kconfig options - Properly report dma_map_sgtable failures - Link to v3: https://lore.kernel.org/r/20250407-dma-buf-ecc-heap-v3-0-97cdd36a5f29@kerne… 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… --- drivers/dma-buf/heaps/Kconfig | 8 + drivers/dma-buf/heaps/Makefile | 1 + drivers/dma-buf/heaps/carveout_heap.c | 388 ++++++++++++++++++++++++++++++++++ 3 files changed, 397 insertions(+) diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig index a5eef06c422644e8aadaf5aff2bd9a33c49c1ba3..1ce4f6828d8c06bfdd7bc2e5127707f1778586e6 100644 --- a/drivers/dma-buf/heaps/Kconfig +++ b/drivers/dma-buf/heaps/Kconfig @@ -1,5 +1,13 @@ +config DMABUF_HEAPS_CARVEOUT + bool "DMA-BUF Carveout Heaps" + depends on DMABUF_HEAPS + help + Choose this option to enable the carveout dmabuf heap. The carveout + heap is backed by pages from reserved memory regions flagged as + exportable. If in doubt, say Y. + config DMABUF_HEAPS_SYSTEM bool "DMA-BUF System Heap" depends on DMABUF_HEAPS help Choose this option to enable the system dmabuf heap. The system heap diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile index 974467791032ffb8a7aba17b1407d9a19b3f3b44..b734647ad5c84f449106748160258e372f153df2 100644 --- a/drivers/dma-buf/heaps/Makefile +++ b/drivers/dma-buf/heaps/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_DMABUF_HEAPS_CARVEOUT) += carveout_heap.o obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o diff --git a/drivers/dma-buf/heaps/carveout_heap.c b/drivers/dma-buf/heaps/carveout_heap.c new file mode 100644 index 0000000000000000000000000000000000000000..3fac190545bc6f853c18a614f6d89176ed2d7df6 --- /dev/null +++ b/drivers/dma-buf/heaps/carveout_heap.c @@ -0,0 +1,388 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/dma-buf.h> +#include <linux/dma-heap.h> +#include <linux/genalloc.h> +#include <linux/highmem.h> +#include <linux/of_reserved_mem.h> + +struct carveout_heap_priv { + struct dma_heap *heap; + struct gen_pool *pool; +}; + +struct carveout_heap_buffer_priv { + struct mutex lock; + struct list_head attachments; + + unsigned long num_pages; + struct carveout_heap_priv *heap; + phys_addr_t paddr; + void *vaddr; + unsigned int vmap_cnt; +}; + +struct carveout_heap_attachment { + struct list_head head; + struct sg_table table; + + struct device *dev; + bool mapped; +}; + +static int carveout_heap_attach(struct dma_buf *buf, + struct dma_buf_attachment *attachment) +{ + struct carveout_heap_buffer_priv *priv = buf->priv; + struct carveout_heap_attachment *a; + struct sg_table *sgt; + unsigned long len = priv->num_pages * PAGE_SIZE; + int ret; + + a = kzalloc(sizeof(*a), GFP_KERNEL); + if (!a) + return -ENOMEM; + INIT_LIST_HEAD(&a->head); + a->dev = attachment->dev; + attachment->priv = a; + + sgt = &a->table; + ret = sg_alloc_table(sgt, 1, GFP_KERNEL); + if (ret) + goto err_cleanup_attach; + + sg_set_buf(sgt->sgl, priv->vaddr, len); + + mutex_lock(&priv->lock); + list_add(&a->head, &priv->attachments); + mutex_unlock(&priv->lock); + + return 0; + +err_cleanup_attach: + kfree(a); + return ret; +} + +static void carveout_heap_detach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attachment) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + struct carveout_heap_attachment *a = attachment->priv; + + mutex_lock(&priv->lock); + list_del(&a->head); + mutex_unlock(&priv->lock); + + sg_free_table(&a->table); + kfree(a); +} + +static struct sg_table * +carveout_heap_map_dma_buf(struct dma_buf_attachment *attachment, + enum dma_data_direction direction) +{ + struct carveout_heap_attachment *a = attachment->priv; + struct sg_table *table = &a->table; + int ret; + + ret = dma_map_sgtable(a->dev, table, direction, 0); + if (ret) + return ERR_PTR(ret); + + a->mapped = true; + + return table; +} + +static void carveout_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, + struct sg_table *table, + enum dma_data_direction direction) +{ + struct carveout_heap_attachment *a = attachment->priv; + + a->mapped = false; + dma_unmap_sgtable(a->dev, table, direction, 0); +} + +static int +carveout_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, + enum dma_data_direction direction) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + struct carveout_heap_attachment *a; + unsigned long len = priv->num_pages * PAGE_SIZE; + + mutex_lock(&priv->lock); + + if (priv->vmap_cnt) + invalidate_kernel_vmap_range(priv->vaddr, len); + + list_for_each_entry(a, &priv->attachments, head) { + if (!a->mapped) + continue; + + dma_sync_sgtable_for_cpu(a->dev, &a->table, direction); + } + + mutex_unlock(&priv->lock); + + return 0; +} + +static int +carveout_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, + enum dma_data_direction direction) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + struct carveout_heap_attachment *a; + unsigned long len = priv->num_pages * PAGE_SIZE; + + mutex_lock(&priv->lock); + + if (priv->vmap_cnt) + flush_kernel_vmap_range(priv->vaddr, len); + + list_for_each_entry(a, &priv->attachments, head) { + if (!a->mapped) + continue; + + dma_sync_sgtable_for_device(a->dev, &a->table, direction); + } + + mutex_unlock(&priv->lock); + + return 0; +} + +static int carveout_heap_mmap(struct dma_buf *dmabuf, + struct vm_area_struct *vma) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + unsigned long len = priv->num_pages * PAGE_SIZE; + + return vm_iomap_memory(vma, priv->paddr, len); +} + +static int carveout_heap_vmap(struct dma_buf *dmabuf, struct iosys_map *map) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + unsigned long len = priv->num_pages * PAGE_SIZE; + + mutex_lock(&priv->lock); + + if (!priv->vmap_cnt) { + void *vaddr = memremap(priv->paddr, len, MEMREMAP_WB); + + if (!vaddr) { + mutex_unlock(&priv->lock); + return -ENOMEM; + } + + priv->vaddr = vaddr; + } + + WARN_ON(!priv->vaddr); + iosys_map_set_vaddr(map, priv->vaddr); + priv->vmap_cnt++; + + mutex_unlock(&priv->lock); + + return 0; +} + +static void carveout_heap_vunmap(struct dma_buf *dmabuf, struct iosys_map *map) +{ + struct carveout_heap_buffer_priv *priv = dmabuf->priv; + + mutex_lock(&priv->lock); + + priv->vmap_cnt--; + if (!priv->vmap_cnt) { + memunmap(priv->vaddr); + priv->vaddr = NULL; + } + + mutex_unlock(&priv->lock); + + iosys_map_clear(map); +} + +static void carveout_heap_dma_buf_release(struct dma_buf *buf) +{ + struct carveout_heap_buffer_priv *buffer_priv = buf->priv; + struct carveout_heap_priv *heap_priv = buffer_priv->heap; + unsigned long len = buffer_priv->num_pages * PAGE_SIZE; + + gen_pool_free(heap_priv->pool, buffer_priv->paddr, len); + kfree(buffer_priv); +} + +static const struct dma_buf_ops carveout_heap_buf_ops = { + .attach = carveout_heap_attach, + .detach = carveout_heap_detach, + .map_dma_buf = carveout_heap_map_dma_buf, + .unmap_dma_buf = carveout_heap_unmap_dma_buf, + .begin_cpu_access = carveout_heap_dma_buf_begin_cpu_access, + .end_cpu_access = carveout_heap_dma_buf_end_cpu_access, + .mmap = carveout_heap_mmap, + .vmap = carveout_heap_vmap, + .vunmap = carveout_heap_vunmap, + .release = carveout_heap_dma_buf_release, +}; + +static struct dma_buf *carveout_heap_allocate(struct dma_heap *heap, + unsigned long len, + u32 fd_flags, + u64 heap_flags) +{ + struct carveout_heap_priv *heap_priv = dma_heap_get_drvdata(heap); + struct carveout_heap_buffer_priv *buffer_priv; + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct dma_buf *buf; + phys_addr_t paddr; + /* len is guaranteed to be page-aligned by the framework, so we can use it as is. */ + size_t size = len; + int ret; + + buffer_priv = kzalloc(sizeof(*buffer_priv), GFP_KERNEL); + if (!buffer_priv) + return ERR_PTR(-ENOMEM); + + INIT_LIST_HEAD(&buffer_priv->attachments); + mutex_init(&buffer_priv->lock); + + paddr = gen_pool_alloc(heap_priv->pool, size); + if (!paddr) { + ret = -ENOMEM; + goto err_free_buffer_priv; + } + + buffer_priv->paddr = paddr; + buffer_priv->heap = heap_priv; + buffer_priv->num_pages = size >> PAGE_SHIFT; + + /* create the dmabuf */ + exp_info.exp_name = dma_heap_get_name(heap); + exp_info.ops = &carveout_heap_buf_ops; + exp_info.size = size; + exp_info.flags = fd_flags; + exp_info.priv = buffer_priv; + + buf = dma_buf_export(&exp_info); + if (IS_ERR(buf)) { + ret = PTR_ERR(buf); + goto err_free_buffer; + } + + return buf; + +err_free_buffer: + gen_pool_free(heap_priv->pool, paddr, len); +err_free_buffer_priv: + kfree(buffer_priv); + + return ERR_PTR(ret); +} + +static const struct dma_heap_ops carveout_heap_ops = { + .allocate = carveout_heap_allocate, +}; + +static int __init carveout_heap_setup(struct device_node *node) +{ + struct dma_heap_export_info exp_info = {}; + const struct reserved_mem *rmem; + struct carveout_heap_priv *priv; + struct dma_heap *heap; + struct gen_pool *pool; + int ret; + + rmem = of_reserved_mem_lookup(node); + if (!rmem) + return -EINVAL; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + pool = gen_pool_create(PAGE_SHIFT, NUMA_NO_NODE); + if (!pool) { + ret = -ENOMEM; + goto err_cleanup_heap; + } + priv->pool = pool; + + ret = gen_pool_add(pool, rmem->base, rmem->size, NUMA_NO_NODE); + if (ret) + goto err_release_mem_region; + + exp_info.name = node->full_name; + exp_info.ops = &carveout_heap_ops; + exp_info.priv = priv; + + heap = dma_heap_add(&exp_info); + if (IS_ERR(heap)) { + ret = PTR_ERR(heap); + goto err_release_mem_region; + } + priv->heap = heap; + + return 0; + +err_release_mem_region: + gen_pool_destroy(pool); +err_cleanup_heap: + kfree(priv); + return ret; +} + +static int __init carveout_heap_init(void) +{ + struct device_node *rmem_node; + struct device_node *node; + int ret; + + rmem_node = of_find_node_by_path("/reserved-memory"); + if (!rmem_node) + return 0; + + for_each_child_of_node(rmem_node, node) { + /* + * TODO: shared-dma-pools register either a CMA or + * coherent memory region depending on whether the + * reusable property is set. + * + * In order to avoid issues, we would need to use the + * respective allocators if the shared-dma-pool property + * was set. + */ + if (of_device_is_compatible(node, "shared-dma-pool")) { + pr_warn("%pOFn: carveout heap driver doesn't support shared-dma-pools.", + node); + continue; + } + + /* + * TODO: restricted-dma-pools register in swiotlb. In + * order to avoid issues, we would need to use the + * swiotlb if that compatible was set. + */ + if (of_device_is_compatible(node, "restricted-dma-pool")) { + pr_warn("%pOFn: carveout heap driver doesn't support restricted-dma-pools.", + node); + continue; + } + + if (!of_property_read_bool(node, "export")) + continue; + + ret = carveout_heap_setup(node); + if (ret) + return ret; + } + + return 0; +} + +module_init(carveout_heap_init); --- base-commit: a17f109942554a751be2c3b05f898f1b3ae98c78 change-id: 20240515-dma-buf-ecc-heap-28a311d2c94e Best regards, -- Maxime Ripard <mripard(a)kernel.org>

1 year, 4 months

Re: [PATCH v5 08/10] accel/rocket: Add IOCTLs for synchronizing memory accesses

by Lucas Stach

Hi Tomeu, Am Dienstag, dem 20.05.2025 um 12:27 +0200 schrieb Tomeu Vizoso: > The NPU cores have their own access to the memory bus, and this isn't > cache coherent with the CPUs. > > Add IOCTLs so userspace can mark when the caches need to be flushed, and > also when a writer job needs to be waited for before the buffer can be > accessed from the CPU. > > Initially based on the same IOCTLs from the Etnaviv driver. > > v2: > - Don't break UABI by reordering the IOCTL IDs (Jeff Hugo) > > v3: > - Check that padding fields in IOCTLs are zero (Jeff Hugo) > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > --- > drivers/accel/rocket/rocket_drv.c | 2 + > drivers/accel/rocket/rocket_gem.c | 80 +++++++++++++++++++++++++++++++++++++++ > drivers/accel/rocket/rocket_gem.h | 5 +++ > include/uapi/drm/rocket_accel.h | 37 ++++++++++++++++++ > 4 files changed, 124 insertions(+) > > diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c > index fef9b93372d3f65c41c1ac35a9bfa0c01ee721a5..c06e66939e6c39909fe08bef3c4f301b07bf8fbf 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -59,6 +59,8 @@ static const struct drm_ioctl_desc rocket_drm_driver_ioctls[] = { > > ROCKET_IOCTL(CREATE_BO, create_bo), > ROCKET_IOCTL(SUBMIT, submit), > + ROCKET_IOCTL(PREP_BO, prep_bo), > + ROCKET_IOCTL(FINI_BO, fini_bo), > }; > > DEFINE_DRM_ACCEL_FOPS(rocket_accel_driver_fops); > diff --git a/drivers/accel/rocket/rocket_gem.c b/drivers/accel/rocket/rocket_gem.c > index 8a8a7185daac4740081293aae6945c9b2bbeb2dd..cdc5238a93fa5978129dc1ac8ec8de955160dc18 100644 > --- a/drivers/accel/rocket/rocket_gem.c > +++ b/drivers/accel/rocket/rocket_gem.c > @@ -129,3 +129,83 @@ int rocket_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file * > > return ret; > } > + > +static inline enum dma_data_direction rocket_op_to_dma_dir(u32 op) > +{ > + if (op & ROCKET_PREP_READ) > + return DMA_FROM_DEVICE; > + else if (op & ROCKET_PREP_WRITE) > + return DMA_TO_DEVICE; > + else > + return DMA_BIDIRECTIONAL; > +} This has copied over the bug fixed in etnaviv commit 58979ad6330a ("drm/etnaviv: fix DMA direction handling for cached RW buffers") Regards, Lucas

1 year, 4 months

Re: [PATCH v4 05/10] accel/rocket: Add a new driver for Rockchip's NPU

by Krzysztof Kozlowski

On Mon, May 19, 2025 at 03:43:37PM GMT, Tomeu Vizoso wrote: > +#endif > diff --git a/drivers/accel/rocket/rocket_device.c b/drivers/accel/rocket/rocket_device.c > new file mode 100644 > index 0000000000000000000000000000000000000000..bb469ac87d36249157f4ba9d9f7106ad558309e4 > --- /dev/null > +++ b/drivers/accel/rocket/rocket_device.c > @@ -0,0 +1,39 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright 2024-2025 Tomeu Vizoso <tomeu(a)tomeuvizoso.net> */ > + > +#include <linux/clk.h> > +#include <linux/dev_printk.h> > + > +#include "rocket_device.h" > + > +int rocket_device_init(struct rocket_device *rdev) > +{ > + struct device *dev = rdev->cores[0].dev; > + int err; > + > + rdev->clk_npu = devm_clk_get(dev, "npu"); > + if (IS_ERR(rdev->clk_npu)) { > + err = PTR_ERR(rdev->clk_npu); > + dev_err(dev, "devm_clk_get failed %d for clock npu\n", err); > + return err; > + } That's probe path? so use standard syntax: return dev_err_probe(). One line instead of four. > + > + rdev->pclk = devm_clk_get(dev, "pclk"); > + if (IS_ERR(rdev->pclk)) { > + err = PTR_ERR(rdev->pclk); > + dev_err(dev, "devm_clk_get failed %d for clock pclk\n", err); > + return err; Same here... except that this should be blk API and entire function gets smaller. > + } > + > + /* Initialize core 0 (top) */ > + err = rocket_core_init(&rdev->cores[0]); > + if (err) > + return err; > + > + return 0; > +} ... > +static int rocket_device_runtime_resume(struct device *dev) > +{ > + struct rocket_device *rdev = dev_get_drvdata(dev); > + int core = find_core_for_dev(dev); > + int err = 0; > + > + if (core < 0) > + return -ENODEV; > + > + if (core == 0) { > + err = clk_prepare_enable(rdev->clk_npu); > + if (err) { > + dev_err(dev, "clk_prepare_enable failed %d for clock npu\n", err); > + return err; > + } > + > + err = clk_prepare_enable(rdev->pclk); > + if (err) { > + dev_err(dev, "clk_prepare_enable failed %d for clock pclk\n", err); > + goto error_clk_npu; > + } > + } > + > + err = clk_prepare_enable(rdev->cores[core].a_clk); > + if (err) { > + dev_err(dev, "clk_prepare_enable failed %d for a_clk in core %d\n", err, core); > + goto error_pclk; > + } > + > + err = clk_prepare_enable(rdev->cores[core].h_clk); > + if (err) { > + dev_err(dev, "clk_prepare_enable failed %d for h_clk in core %d\n", err, core); > + goto error_a_clk; > + } All four above calls could be just one call with bulk API. > + > + return 0; > + > +error_a_clk: > + clk_disable_unprepare(rdev->cores[core].a_clk); > + > +error_pclk: > + if (core == 0) > + clk_disable_unprepare(rdev->pclk); > + > +error_clk_npu: > + if (core == 0) > + clk_disable_unprepare(rdev->clk_npu); And all this would be gone... > + > + return err; Best regards, Krzysztof

1 year, 4 months

Re: [PATCH v4 01/10] dt-bindings: npu: rockchip,rknn: Add bindings

by Krzysztof Kozlowski

On Mon, May 19, 2025 at 03:43:33PM GMT, Tomeu Vizoso wrote: > Add the bindings for the Neural Processing Unit IP from Rockchip. > > v2: > - Adapt to new node structure (one node per core, each with its own > IOMMU) > - Several misc. fixes from Sebastian Reichel > > v3: > - Split register block in its constituent subblocks, and only require > the ones that the kernel would ever use (Nicolas Frattaroli) > - Group supplies (Rob Herring) > - Explain the way in which the top core is special (Rob Herring) > > v4: > - Change required node name to npu@ (Rob Herring and Krzysztof Kozlowski) > - Remove unneeded items: (Krzysztof Kozlowski) > - Fix use of minItems/maxItems (Krzysztof Kozlowski) > - Add reg-names to list of required properties (Krzysztof Kozlowski) > - Fix example (Krzysztof Kozlowski) > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > Signed-off-by: Sebastian Reichel <sebastian.reichel(a)collabora.com> This order of SoB is still odd. You as person sending it should be the last signing person. Are you sure you are using b4 for managing trailers? I would expect it to re-order these on every update and this is already v4. > --- > .../bindings/npu/rockchip,rknn-core.yaml | 149 +++++++++++++++++++++ Filename matching compatible, so rockchip,rk3588-rknn-core.yaml > 1 file changed, 149 insertions(+) > > diff --git a/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..fafd0b01da215c7396262012988e364ef07ea137 > --- /dev/null > +++ b/Documentation/devicetree/bindings/npu/rockchip,rknn-core.yaml > @@ -0,0 +1,149 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/npu/rockchip,rknn-core.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Neural Processing Unit IP from Rockchip > + > +maintainers: > + - Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > + > +description: > + Rockchip IP for accelerating inference of neural networks, based on NVIDIA's > + open source NVDLA IP. > + > + There is to be a node per each core in the NPU. In Rockchip's design there > + will be one core that is special and needs to be powered on before any of the > + other cores can be used. This special core is called the top core and should > + have the compatible string that corresponds to top cores. > + > +properties: > + $nodename: > + pattern: '^npu@[a-f0-9]+$' > + > + compatible: > + oneOf: Drop... if you followed my advice you would notice it is not necessary. > + - enum: > + - rockchip,rk3588-rknn-core-top > + - enum: > + - rockchip,rk3588-rknn-core My comments were only partially implemented. This syntax is really not readable and not necessary and I asked to make it part of previous enum. This is just one enum: compatible: enum: - foo - bar Best regards, Krzysztof

1 year, 4 months

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig