Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

6 participants
3122 discussions

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

6 months, 2 weeks

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

6 months, 2 weeks

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

6 months, 2 weeks

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>

6 months, 2 weeks

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.

6 months, 2 weeks

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

6 months, 2 weeks

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. > > [...]

6 months, 2 weeks

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.

6 months, 3 weeks

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.

6 months, 3 weeks

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

6 months, 3 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig