Hi Boris,

On Tue, May 05, 2026 at 05:20:48PM +0200, Boris Brezillon wrote:

Hi Ketil,

On Tue, 5 May 2026 16:05:07 +0200 Ketil Johnsen ketil.johnsen@arm.com wrote:

...

From: John Stultz jstultz@google.com

Add proper reference counting on the dma_heap structure. While existing heaps are built-in, we may eventually have heaps loaded from modules, and we'll need to be able to properly handle the references to the heaps

It's weird that this "heap as module" thing is mentioned here, but actual robustness to make this safe is not added in the commit or any of the following ones.

...

Signed-off-by: John Stultz jstultz@google.com Signed-off-by: T.J. Mercier tjmercier@google.com Signed-off-by: Yong Wu yong.wu@mediatek.com [Yong: Just add comment for "minor" and "refcount"] Signed-off-by: Yunfei Dong yunfei.dong@mediatek.com [Yunfei: Change reviewer's comments] Signed-off-by: Florent Tomasin florent.tomasin@arm.com [Florent: Rebase] Signed-off-by: Ketil Johnsen ketil.johnsen@arm.com [Ketil: Rebase]

---

drivers/dma-buf/dma-heap.c | 29 +++++++++++++++++++++++++++++ include/linux/dma-heap.h | 2 ++ 2 files changed, 31 insertions(+)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index ac5f8685a6494..9fd365ddbd517 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -12,6 +12,7 @@ #include <linux/dma-heap.h> #include <linux/err.h> #include <linux/export.h> +#include <linux/kref.h> #include <linux/list.h> #include <linux/nospec.h> #include <linux/syscalls.h> @@ -31,6 +32,7 @@

• @heap_devt: heap device node
• @list: list head connecting to list of heaps
• @heap_cdev: heap char device

• • @refcount: reference counter for this heap device
  • Represents a heap of memory from which buffers can be made.
  */

@@ -41,6 +43,7 @@ struct dma_heap { dev_t heap_devt; struct list_head list; struct cdev heap_cdev;

• struct kref refcount;

}; static LIST_HEAD(heap_list); @@ -248,6 +251,7 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info) if (!heap) return ERR_PTR(-ENOMEM);

• kref_init(&heap->refcount); heap->name = exp_info->name; heap->ops = exp_info->ops; heap->priv = exp_info->priv;

@@ -313,6 +317,31 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info) } EXPORT_SYMBOL_NS_GPL(dma_heap_add, "DMA_BUF_HEAP"); +static void dma_heap_release(struct kref *ref) +{

• struct dma_heap *heap = container_of(ref, struct dma_heap, refcount);
• unsigned int minor = MINOR(heap->heap_devt);
• mutex_lock(&heap_list_lock);
• list_del(&heap->list);
• mutex_unlock(&heap_list_lock);
• device_destroy(dma_heap_class, heap->heap_devt);
• cdev_del(&heap->heap_cdev);
• xa_erase(&dma_heap_minors, minor);
• kfree(heap);

That's actually problematic, because cdev_del() doesn't guarantee that all opened FDs have been closed [1], it just guarantees that no new ones can materialize. In order to make that safe, we'd need a

1. kref_get_unless_zero() in dma_heap_open(), with proper locking around the xa_load() to protect against the heap removal that's happening here
2. a dma_heap_put() in a new dma_heap_close() implementation
3. a guarantee that heap implementations won't go away until the last ref is dropped, which means ops and all the data needed for this heap to satisfy ioctl()s (and more generally every passed at dma_heap_add() time) have to stay valid until the last ref is dropped. Alternatively, we could restrict this only to in-flight ioctl()s, and have the ops replaced by some dummy ops using RCU or a rwlock. But I guess live dmabufs allocated on this heap have to retain the heap and its implementation anyway.

For record, #3 is already not satisfied by the current tee_heap implementation (tee_dma_heap objects can vanish before the dma_heap object is gone). The other implementations seem to be fine because they are statically linked, and they either have exp_info.priv set to NULL, or something that's never released.

That statement won't hold for long, see: https://lore.kernel.org/r/20260427-dma-buf-heaps-as-modules-v5-0-b6f5678feef...

However, all upstream heaps can be loaded as module, but not unloaded. So once you get a reference to it, you can assume it will live forever. That's why we didn't merge that patch before, even though it was discussed:

https://lore.kernel.org/all/CANDhNCqk9Uk4aXHhUsL4hR1GHNmWZnH3C9Np-A02wdi+J3D...

Maxime

On Tue, May 05, 2026 at 06:40:21PM +0200, Boris Brezillon wrote:

On Tue, 5 May 2026 17:39:13 +0200 Maxime Ripard mripard@kernel.org wrote:

...

Hi Boris,

On Tue, May 05, 2026 at 05:20:48PM +0200, Boris Brezillon wrote:

...

Hi Ketil,

On Tue, 5 May 2026 16:05:07 +0200 Ketil Johnsen ketil.johnsen@arm.com wrote:

...

From: John Stultz jstultz@google.com

Add proper reference counting on the dma_heap structure. While existing heaps are built-in, we may eventually have heaps loaded from modules, and we'll need to be able to properly handle the references to the heaps

It's weird that this "heap as module" thing is mentioned here, but actual robustness to make this safe is not added in the commit or any of the following ones.

...

Signed-off-by: John Stultz jstultz@google.com Signed-off-by: T.J. Mercier tjmercier@google.com Signed-off-by: Yong Wu yong.wu@mediatek.com [Yong: Just add comment for "minor" and "refcount"] Signed-off-by: Yunfei Dong yunfei.dong@mediatek.com [Yunfei: Change reviewer's comments] Signed-off-by: Florent Tomasin florent.tomasin@arm.com [Florent: Rebase] Signed-off-by: Ketil Johnsen ketil.johnsen@arm.com [Ketil: Rebase]

---

drivers/dma-buf/dma-heap.c | 29 +++++++++++++++++++++++++++++ include/linux/dma-heap.h | 2 ++ 2 files changed, 31 insertions(+)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index ac5f8685a6494..9fd365ddbd517 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -12,6 +12,7 @@ #include <linux/dma-heap.h> #include <linux/err.h> #include <linux/export.h> +#include <linux/kref.h> #include <linux/list.h> #include <linux/nospec.h> #include <linux/syscalls.h> @@ -31,6 +32,7 @@

• @heap_devt: heap device node
• @list: list head connecting to list of heaps
• @heap_cdev: heap char device

• • @refcount: reference counter for this heap device
  • Represents a heap of memory from which buffers can be made.
  */

@@ -41,6 +43,7 @@ struct dma_heap { dev_t heap_devt; struct list_head list; struct cdev heap_cdev;

• struct kref refcount;

}; static LIST_HEAD(heap_list); @@ -248,6 +251,7 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info) if (!heap) return ERR_PTR(-ENOMEM);

• kref_init(&heap->refcount); heap->name = exp_info->name; heap->ops = exp_info->ops; heap->priv = exp_info->priv;

@@ -313,6 +317,31 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info) } EXPORT_SYMBOL_NS_GPL(dma_heap_add, "DMA_BUF_HEAP"); +static void dma_heap_release(struct kref *ref) +{

• struct dma_heap *heap = container_of(ref, struct dma_heap, refcount);
• unsigned int minor = MINOR(heap->heap_devt);
• mutex_lock(&heap_list_lock);
• list_del(&heap->list);
• mutex_unlock(&heap_list_lock);
• device_destroy(dma_heap_class, heap->heap_devt);
• cdev_del(&heap->heap_cdev);
• xa_erase(&dma_heap_minors, minor);
• kfree(heap);

That's actually problematic, because cdev_del() doesn't guarantee that all opened FDs have been closed [1], it just guarantees that no new ones can materialize. In order to make that safe, we'd need a

1. kref_get_unless_zero() in dma_heap_open(), with proper locking around the xa_load() to protect against the heap removal that's happening here
2. a dma_heap_put() in a new dma_heap_close() implementation
3. a guarantee that heap implementations won't go away until the last ref is dropped, which means ops and all the data needed for this heap to satisfy ioctl()s (and more generally every passed at dma_heap_add() time) have to stay valid until the last ref is dropped. Alternatively, we could restrict this only to in-flight ioctl()s, and have the ops replaced by some dummy ops using RCU or a rwlock. But I guess live dmabufs allocated on this heap have to retain the heap and its implementation anyway.

For record, #3 is already not satisfied by the current tee_heap implementation (tee_dma_heap objects can vanish before the dma_heap object is gone). The other implementations seem to be fine because they are statically linked, and they either have exp_info.priv set to NULL, or something that's never released.

That statement won't hold for long, see: https://lore.kernel.org/r/20260427-dma-buf-heaps-as-modules-v5-0-b6f5678feef...

However, all upstream heaps can be loaded as module, but not unloaded. So once you get a reference to it, you can assume it will live forever. That's why we didn't merge that patch before, even though it was discussed:

https://lore.kernel.org/all/CANDhNCqk9Uk4aXHhUsL4hR1GHNmWZnH3C9Np-A02wdi+J3D...

Hm, not too sure that makes the tee_heap implementation sane WRT tee_heap removal though, unless we have a guarantee that tee_device_unregister() will never be called...

I missed that part. You're totally right then :)

Maxime

On Tue, 5 May 2026 16:05:08 +0200 Ketil Johnsen ketil.johnsen@arm.com wrote:

From: John Stultz jstultz@google.com

This allows drivers who don't want to create their own DMA-BUF exporter to be able to allocate DMA-BUFs directly from existing DMA-BUF Heaps.

There is some concern that the premise of DMA-BUF heaps is that userland knows better about what type of heap memory is needed for a pipeline, so it would likely be best for drivers to import and fill DMA-BUFs allocated by userland instead of allocating one themselves, but this is still up for debate.

I think this commit message needs to be updated with more details around what it's actually needed for here (driver needing protected buffer to boot FW and expose a char device, and no clean way to pass dmabufs around before this cdev is exposed).

Signed-off-by: John Stultz jstultz@google.com Signed-off-by: T.J. Mercier tjmercier@google.com Signed-off-by: Yong Wu yong.wu@mediatek.com [Yong: Fix the checkpatch alignment warning] Signed-off-by: Yunfei Dong yunfei.dong@mediatek.com Signed-off-by: Florent Tomasin florent.tomasin@arm.com [Florent: Rebase] Signed-off-by: Ketil Johnsen ketil.johnsen@arm.com [Ketil: Rebase]

---

drivers/dma-buf/dma-heap.c | 80 ++++++++++++++++++++++++++++++-------- include/linux/dma-heap.h | 6 +++ 2 files changed, 70 insertions(+), 16 deletions(-)

diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c index 9fd365ddbd517..854d40d789ff2 100644 --- a/drivers/dma-buf/dma-heap.c +++ b/drivers/dma-buf/dma-heap.c @@ -57,12 +57,24 @@ module_param(mem_accounting, bool, 0444); MODULE_PARM_DESC(mem_accounting, "Enable cgroup-based memory accounting for dma-buf heap allocations (default=false)."); -static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,

• 		 u32 fd_flags,
  

• 		 u64 heap_flags)
  

+/**

• • dma_heap_buffer_alloc - Allocate dma-buf from a dma_heap
• • @heap: DMA-Heap to allocate from
• • @len: size to allocate in bytes
• • @fd_flags: flags to set on returned dma-buf fd
• • @heap_flags: flags to pass to the dma heap
• • This is for internal dma-buf allocations only. Free returned buffers with dma_buf_put().
• */

+struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,

• 		      u32 fd_flags,
  

• 		      u64 heap_flags)
  

{

• struct dma_buf *dmabuf;
• int fd;

• if (fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)

• return ERR_PTR(-EINVAL);
  

• if (heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)

• return ERR_PTR(-EINVAL);
  

I'd probably move the flags checks to dma_heap_buffer_alloc() in a separate patch, to keep the diff easier to read. Same for the dma_heap_buffer_alloc()/dma_heap_bufferfd_alloc() split, though I'm not too sure we need dma_heap_bufferfd_alloc(), we could just move the FD allocation directly in dma_heap_ioctl_allocate().

/* * Allocations from all heaps have to begin @@ -70,9 +82,20 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, */ len = PAGE_ALIGN(len); if (!len)

• return -EINVAL;
  

• return ERR_PTR(-EINVAL);
  

• return heap->ops->allocate(heap, len, fd_flags, heap_flags);

+} +EXPORT_SYMBOL_NS_GPL(dma_heap_buffer_alloc, "DMA_BUF_HEAP");

• dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);

+static int dma_heap_bufferfd_alloc(struct dma_heap *heap, size_t len,

• 		   u32 fd_flags,
  

• 		   u64 heap_flags)
  

+{

• struct dma_buf *dmabuf;
• int fd;
• dmabuf = dma_heap_buffer_alloc(heap, len, fd_flags, heap_flags); if (IS_ERR(dmabuf)) return PTR_ERR(dmabuf);

@@ -110,15 +133,9 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data) if (heap_allocation->fd) return -EINVAL;

• if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)

• return -EINVAL;
  

• if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)

• return -EINVAL;
  

• fd = dma_heap_buffer_alloc(heap, heap_allocation->len,

• 		   heap_allocation->fd_flags,
  

• 		   heap_allocation->heap_flags);
  

• fd = dma_heap_bufferfd_alloc(heap, heap_allocation->len,

• 		     heap_allocation->fd_flags,
  

• 		     heap_allocation->heap_flags);
  

  if (fd < 0) return fd;

@@ -317,6 +334,36 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info) } EXPORT_SYMBOL_NS_GPL(dma_heap_add, "DMA_BUF_HEAP"); +/**

• • dma_heap_find - get the heap registered with the specified name
• • @name: Name of the DMA-Heap to find
• • Returns:
• • The DMA-Heap with the provided name.
• • NOTE: DMA-Heaps returned from this function MUST be released using
• • dma_heap_put() when the user is done to enable the heap to be unloaded.
• */

+struct dma_heap *dma_heap_find(const char *name)

s/dma_heap_find/dma_heap_find_by_name/gc

+{

• struct dma_heap *h;
• mutex_lock(&heap_list_lock);
• list_for_each_entry(h, &heap_list, list) {

• if (!kref_get_unless_zero(&h->refcount))
  

• 	continue;
  

• if (!strcmp(h->name, name)) {
  

I think we should go sysfs_streq(), to make sure we don't return a heap whose name only starts with the searched name.

• 	mutex_unlock(&heap_list_lock);
  

• 	return h;
  

• }
  

• dma_heap_put(h);
  

• }
• mutex_unlock(&heap_list_lock);
• return NULL;

This could be simplified with something like:

struct dma_heap *h, *ret = NULL;

guard(mutex)(&heap_list_lock); list_for_each_entry(h, &heap_list, list) { if (!sysfs_streq(h->name, name)) continue;

if (kref_get_unless_zero(&h->refcount)) ret = h;

break; }

return ret;

+}

+EXPORT_SYMBOL_NS_GPL(dma_heap_find, "DMA_BUF_HEAP");

static void dma_heap_release(struct kref *ref) { struct dma_heap *heap = container_of(ref, struct dma_heap, refcount); @@ -341,6 +388,7 @@ void dma_heap_put(struct dma_heap *heap) { kref_put(&heap->refcount, dma_heap_release); } +EXPORT_SYMBOL_NS_GPL(dma_heap_put, "DMA_BUF_HEAP"); static char *dma_heap_devnode(const struct device *dev, umode_t *mode) { diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h index ff57741700f5f..c3351f8a1f8cf 100644 --- a/include/linux/dma-heap.h +++ b/include/linux/dma-heap.h @@ -46,8 +46,14 @@ const char *dma_heap_get_name(struct dma_heap *heap); struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info); +struct dma_heap *dma_heap_find(const char *name);

void dma_heap_put(struct dma_heap *heap); +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,

• 		      u32 fd_flags,
  

• 		      u64 heap_flags);
  

extern bool mem_accounting; #endif /* _DMA_HEAPS_H */

On Tue, 5 May 2026 16:05:09 +0200 Ketil Johnsen ketil.johnsen@arm.com wrote:

Handle the sync to device of FW memory sections inside panthor_fw_init_section_mem() so that the callers do not have to.

This small improvement is also critical for protected FW sections, so we avoid issuing memory transactions to protected memory from CPU running in normal mode.

Signed-off-by: Ketil Johnsen ketil.johnsen@arm.com

Reviewed-by: Boris Brezillon boris.brezillon@collabora.com

---

drivers/gpu/drm/panthor/panthor_fw.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c index be0da5b1f3abf..0d07a133dc3af 100644 --- a/drivers/gpu/drm/panthor/panthor_fw.c +++ b/drivers/gpu/drm/panthor/panthor_fw.c @@ -446,6 +446,7 @@ static void panthor_fw_init_section_mem(struct panthor_device *ptdev, struct panthor_fw_section *section) { bool was_mapped = !!section->mem->kmap;

• struct sg_table *sgt; int ret;

if (!section->data.size && @@ -464,6 +465,11 @@ static void panthor_fw_init_section_mem(struct panthor_device *ptdev, if (!was_mapped) panthor_kernel_bo_vunmap(section->mem);

• /* An sgt should have been requested when the kernel BO was GPU-mapped. */
• sgt = to_panthor_bo(section->mem->obj)->dmap.sgt;
• if (!drm_WARN_ON_ONCE(&ptdev->base, !sgt))

• dma_sync_sgtable_for_device(ptdev->base.dev, sgt, DMA_TO_DEVICE);
  

} /** @@ -626,7 +632,6 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, section_size = hdr.va.end - hdr.va.start; if (section_size) { u32 cache_mode = hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_CACHE_MODE_MASK;

• struct panthor_gem_object *bo;
  

  u32 vm_map_flags = 0; u64 va = hdr.va.start;

@@ -663,14 +668,6 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, } panthor_fw_init_section_mem(ptdev, section);

• bo = to_panthor_bo(section->mem->obj);
  

• /* An sgt should have been requested when the kernel BO was GPU-mapped. */
  

• if (drm_WARN_ON_ONCE(&ptdev->base, !bo->dmap.sgt))
  

• 	return -EINVAL;
  

• dma_sync_sgtable_for_device(ptdev->base.dev, bo->dmap.sgt, DMA_TO_DEVICE);
  

  }

if (hdr.va.start == CSF_MCU_SHARED_REGION_START) @@ -724,17 +721,10 @@ panthor_reload_fw_sections(struct panthor_device *ptdev, bool full_reload) struct panthor_fw_section *section; list_for_each_entry(section, &ptdev->fw->sections, node) {

• struct sg_table *sgt;
  

• if (!full_reload && !(section->flags & CSF_FW_BINARY_IFACE_ENTRY_WR)) continue;

panthor_fw_init_section_mem(ptdev, section);

• /* An sgt should have been requested when the kernel BO was GPU-mapped. */
  

• sgt = to_panthor_bo(section->mem->obj)->dmap.sgt;
  

• if (!drm_WARN_ON_ONCE(&ptdev->base, !sgt))
  

• 	dma_sync_sgtable_for_device(ptdev->base.dev, sgt, DMA_TO_DEVICE);
  

  }

}

Thanks for sending this series! A few quick notes in-line.

On Tuesday, 5 May 2026 16:05:10 Central European Summer Time Ketil Johnsen wrote:

From: Florent Tomasin florent.tomasin@arm.com

This patch allows Panthor to allocate buffer objects from a protected heap. The Panthor driver should be seen as a consumer of the heap and not an exporter.

Protected memory buffers needed by the Panthor driver:

• On CSF FW load, the Panthor driver must allocate a protected buffer object to hold data to use by the FW when in protected mode. This protected buffer object is owned by the device and does not belong to a process.
• On CSG creation, the Panthor driver must allocate a protected suspend buffer object for the FW to store data when suspending the CSG while in protected mode. The kernel owns this allocation and does not allow user space mapping. The format of the data in this buffer is only known by the FW and does not need to be shared with other entities.

The driver will retrieve the protected heap using the name of the heap provided to the driver as module parameter.

If the heap is not yet available, the panthor driver will defer the probe until created. It is an integration error to provide a heap name that does not exist or is never created.

Panthor is calling the DMA heap allocation function and obtains a DMA buffer from it. This buffer is then registered to GEM and imported.

Signed-off-by: Florent Tomasin florent.tomasin@arm.com Co-developed-by: Ketil Johnsen ketil.johnsen@arm.com Signed-off-by: Ketil Johnsen ketil.johnsen@arm.com

---

Documentation/gpu/panthor.rst | 47 +++++++++++++++ drivers/gpu/drm/panthor/Kconfig | 1 + drivers/gpu/drm/panthor/panthor_device.c | 28 ++++++++- drivers/gpu/drm/panthor/panthor_device.h | 6 ++ drivers/gpu/drm/panthor/panthor_fw.c | 29 ++++++++- drivers/gpu/drm/panthor/panthor_fw.h | 2 + drivers/gpu/drm/panthor/panthor_gem.c | 77 ++++++++++++++++++++++-- drivers/gpu/drm/panthor/panthor_gem.h | 16 ++++- drivers/gpu/drm/panthor/panthor_heap.c | 2 + drivers/gpu/drm/panthor/panthor_sched.c | 11 +++- 10 files changed, 208 insertions(+), 11 deletions(-)

diff --git a/Documentation/gpu/panthor.rst b/Documentation/gpu/panthor.rst index 7a841741278fb..be20eadea6dd5 100644 --- a/Documentation/gpu/panthor.rst +++ b/Documentation/gpu/panthor.rst @@ -54,3 +54,50 @@ sync object arrays and heap chunks. Because they are all allocated and pinned at creation time, only `panthor-resident-memory` is necessary to tell us their size. `panthor-active-memory` shows the size of kernel BO's associated with VM's and groups currently being scheduled for execution by the GPU.

+Panthor Protected Memory Integration +=====================================

+Panthor requires the platform to provide a protected DMA HEAP. +This DMA heap must be identifiable via a string name. +The name is defined by the system integrator, it could be hard coded +in the heap driver, defined by a module parameter of the heap driver +or else.

+.. code-block:: none

• User

•    ┌─────────────────────────────┐
  

•    |           Application       |
  

•    └─────────────▲───────────────┘
  

•        |         |          |
  

•        | DMA-BUF |          | Protected
  

•        |         |          | Job Submission
  

• --------|---------|----------|---------
• Kernel | | |

•        |         |          |
  

•        |         |  DMA-BUF |
  

• ┌───────▼─────────────┐ ┌─▼───────┐
• | DMA PROTECTED HEAP |◄───| Panthor |
• | (Vendor specific) | | |
• └─────────────────────┘ └─────────┘

•        |                    |
  

• --------|--------------------|---------
• HW | |

•        |                    |
  

• ┌───────▼───────────────┐ ┌─▼───┐
• | Trusted FW | | |
• | Protected Memory ◄──► GPU |
• └───────────────────────┘ └─────┘

+To configure Panthor to use the protected memory heap, pass the protected memory +heap string name as module parameter of the Panthor module.

+Example:

• .. code-block:: shell

•    insmod panthor.ko protected_heap_name=“vendor_protected_heap"
  

+If `protected_heap_name` module parameter is not provided, Panthor will not support +protected job execution. diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig index 911e7f4810c39..fb0bad9a0fd2b 100644 --- a/drivers/gpu/drm/panthor/Kconfig +++ b/drivers/gpu/drm/panthor/Kconfig @@ -7,6 +7,7 @@ config DRM_PANTHOR depends on !GENERIC_ATOMIC64 # for IOMMU_IO_PGTABLE_LPAE depends on MMU select DEVFREQ_GOV_SIMPLE_ONDEMAND

• select DMABUF_HEAPS select DRM_EXEC select DRM_GPUVM select DRM_SCHED

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c index bc62a498a8a84..3a5cdfa99e5fe 100644 --- a/drivers/gpu/drm/panthor/panthor_device.c +++ b/drivers/gpu/drm/panthor/panthor_device.c @@ -5,7 +5,9 @@ /* Copyright 2025 ARM Limited. All rights reserved. */ #include <linux/clk.h> +#include <linux/dma-heap.h> #include <linux/mm.h> +#include <linux/of.h>

Can be dropped, none of the added code in this file requires it.

#include <linux/platform_device.h> #include <linux/pm_domain.h> #include <linux/pm_runtime.h> @@ -27,6 +29,10 @@ #include "panthor_regs.h" #include "panthor_sched.h" +MODULE_PARM_DESC(protected_heap_name, "DMA heap name, from which to allocate protected buffers"); +static char *protected_heap_name; +module_param(protected_heap_name, charp, 0444);

static int panthor_gpu_coherency_init(struct panthor_device *ptdev) { BUILD_BUG_ON(GPU_COHERENCY_NONE != DRM_PANTHOR_GPU_COHERENCY_NONE); @@ -127,6 +133,9 @@ void panthor_device_unplug(struct panthor_device *ptdev) panthor_gpu_unplug(ptdev); panthor_pwr_unplug(ptdev);

• if (ptdev->protm.heap)

• dma_heap_put(ptdev->protm.heap);
  

• pm_runtime_dont_use_autosuspend(ptdev->base.dev); pm_runtime_put_sync_suspend(ptdev->base.dev);

@@ -277,9 +286,21 @@ int panthor_device_init(struct panthor_device *ptdev) return ret; }

• /* If a protected heap name is specified but not found, defer the probe until created */
• if (protected_heap_name && strlen(protected_heap_name)) {

• ptdev->protm.heap = dma_heap_find(protected_heap_name);
  

• if (!ptdev->protm.heap) {
  

• 	drm_warn(&ptdev->base,
  

• 		 "Protected heap \'%s\' not (yet) available - deferring probe",
  

• 		 protected_heap_name);
  

The escaping of the single quotes here is redundant, and I think this is better as a debug message rather than a drm_warn: probe deferral is normal.

Though I'm wondering whether we're open-coding dependency handling here, I guess there's no way for any core to order things for us because the dependency is on a name and the name is from a driver-specific module parameter, so there's no generic solution to this. This second paragraph is just me ruminating though and not an actionable request for changes.

• 	ret = -EPROBE_DEFER;
  

• 	goto err_rpm_put;
  

• }
  

• }
• ret = panthor_hw_init(ptdev); if (ret)

• goto err_rpm_put;
  

• goto err_dma_heap_put;
  

ret = panthor_pwr_init(ptdev); if (ret) @@ -343,6 +364,11 @@ int panthor_device_init(struct panthor_device *ptdev) err_rpm_put: pm_runtime_put_sync_suspend(ptdev->base.dev);

+err_dma_heap_put:

• if (ptdev->protm.heap)

• dma_heap_put(ptdev->protm.heap);
  

This is ordered wrong. Getting the dma heap happens after getting rpm, so the unwind should put the dma heap before putting rpm. Right now, a failure of panthor_hw_init would leave rpm enabled.

As Boris already mentioned in his review though, using devres helpers would get rid of this manual put on error or driver remove entirely, which is preferable.

return ret; } diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h index 5cba272f9b4de..d51fec97fc5fa 100644 --- a/drivers/gpu/drm/panthor/panthor_device.h +++ b/drivers/gpu/drm/panthor/panthor_device.h @@ -7,6 +7,7 @@ #define __PANTHOR_DEVICE_H__ #include <linux/atomic.h> +#include <linux/dma-heap.h> #include <linux/io-pgtable.h> #include <linux/regulator/consumer.h> #include <linux/pm_runtime.h> @@ -329,6 +330,11 @@ struct panthor_device { struct list_head node; } gems; #endif

• /** @protm: Protected mode related data. */
• struct {

• /** @heap: Pointer to the protected heap */
  

• struct dma_heap *heap;
  

• } protm;

}; struct panthor_gpu_usage { diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c index 0d07a133dc3af..1aba29b9779b6 100644 --- a/drivers/gpu/drm/panthor/panthor_fw.c +++ b/drivers/gpu/drm/panthor/panthor_fw.c @@ -500,6 +500,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev, mem = panthor_kernel_bo_create(ptdev, ptdev->fw->vm, SZ_8K, DRM_PANTHOR_BO_NO_MMAP,

• 		       0,
  	       DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC |
  	       DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED,
  	       PANTHOR_VM_KERNEL_AUTO_VA,
  

@@ -534,6 +535,26 @@ panthor_fw_alloc_suspend_buf_mem(struct panthor_device *ptdev, size_t size) { return panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), size, DRM_PANTHOR_BO_NO_MMAP,

• 			0,
  

• 			DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC,
  

• 			PANTHOR_VM_KERNEL_AUTO_VA,
  

• 			"suspend_buf");
  

Looks like we're effectively renaming this from "FW suspend buffer" to "suspend_buf", and calling the protm suspend buf "FW suspend buffer".

This seems a little confusing, to the point where the diff algorithm also had a hard time. Naming comes down to a matter of taste, but I want to make sure the rename was intentional here.

+}

+/**

• • panthor_fw_alloc_protm_suspend_buf_mem() - Allocate a protm suspend buffer
• • for a command stream group.
• • @ptdev: Device.
• • @size: Size of the protm suspend buffer.
• • Return: A valid pointer in case of success, an ERR_PTR() otherwise.
• */

+struct panthor_kernel_bo * +panthor_fw_alloc_protm_suspend_buf_mem(struct panthor_device *ptdev, size_t size) +{

• return panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), size,

• 			DRM_PANTHOR_BO_NO_MMAP,
  

• 			DRM_PANTHOR_KBO_PROTECTED_HEAP,
  		DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC,
  		PANTHOR_VM_KERNEL_AUTO_VA,
  		"FW suspend buffer");
  

@@ -547,6 +568,7 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, ssize_t vm_pgsz = panthor_vm_page_size(ptdev->fw->vm); struct panthor_fw_binary_section_entry_hdr hdr; struct panthor_fw_section *section;

• u32 kbo_flags = 0; u32 section_size; u32 name_len; int ret;

@@ -585,10 +607,13 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, return -EINVAL; }

• if (hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) {

• if ((hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) && !ptdev->protm.heap) { drm_warn(&ptdev->base, "Firmware protected mode entry is not supported, ignoring"); return 0;
• } else if ((hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) && ptdev->protm.heap) {

• drm_info(&ptdev->base, "Firmware protected mode entry supported");
  

• kbo_flags = DRM_PANTHOR_KBO_PROTECTED_HEAP;
  

Instead of the duplicated check in both branches of the condition, nesting it may be less clunky:

if ((hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_PROT) { if (!ptdev->protm.heap) { drm_warn(&ptdev->base, "Firmware protected mode entry is not supported, ignoring"); return 0; }

drm_info(&ptdev->base, "Firmware protected mode entry supported"); kbo_flags = DRM_PANTHOR_KBO_PROTECTED_HEAP; }

That being said, we might want to rethink the warning/info entirely. If it's normal behaviour for a platform to load this fw section, even if the platform doesn't support protm, as I suspect is the case due to the `return 0`, then the warning has always been a bit too noisy.

I think info level that also gives some identifier for what section was ignored (e.g. fw offset start/end) would be fine. The other branch, i.e. "Firmware protected mode entry supported", may be best something that gets printed along with other details about what the GPU supports, so that each protected section does not print this over and over. It feels wrong to do it in panthor_hw_info_init since that's printing features of the hardware rather than of panthor's configuration, so maybe just do it in panthor_device_init after ptdev->protm.heap is non-NULL.

} if (hdr.va.start == CSF_MCU_SHARED_REGION_START && @@ -653,7 +678,7 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev, section->mem = panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), section_size,

• 					DRM_PANTHOR_BO_NO_MMAP,
  

• 					DRM_PANTHOR_BO_NO_MMAP, kbo_flags,
  				vm_map_flags, va, "FW section");
  

  if (IS_ERR(section->mem)) return PTR_ERR(section->mem);

diff --git a/drivers/gpu/drm/panthor/panthor_fw.h b/drivers/gpu/drm/panthor/panthor_fw.h index fbdc21469ba32..0cf3761abf789 100644 --- a/drivers/gpu/drm/panthor/panthor_fw.h +++ b/drivers/gpu/drm/panthor/panthor_fw.h @@ -509,6 +509,8 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev, u32 *input_fw_va, u32 *output_fw_va); struct panthor_kernel_bo * panthor_fw_alloc_suspend_buf_mem(struct panthor_device *ptdev, size_t size); +struct panthor_kernel_bo * +panthor_fw_alloc_protm_suspend_buf_mem(struct panthor_device *ptdev, size_t size); struct panthor_vm *panthor_fw_vm(struct panthor_device *ptdev); diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c index 13295d7a593df..08fe4a5e43817 100644 --- a/drivers/gpu/drm/panthor/panthor_gem.c +++ b/drivers/gpu/drm/panthor/panthor_gem.c @@ -20,12 +20,17 @@ #include <drm/drm_print.h> #include <drm/panthor_drm.h> +#include <uapi/linux/dma-heap.h>

#include "panthor_device.h" #include "panthor_drv.h" #include "panthor_fw.h" #include "panthor_gem.h" #include "panthor_mmu.h" +MODULE_IMPORT_NS("DMA_BUF"); +MODULE_IMPORT_NS("DMA_BUF_HEAP");

void panthor_gem_init(struct panthor_device *ptdev) { int err; @@ -466,7 +471,6 @@ static void panthor_gem_free_object(struct drm_gem_object *obj) } drm_gem_object_release(obj);

Unrelated whitespace change (though I do like it), don't know how we handle "too small for its own commit but also function isn't touched by anything else in this commit" type whitespace cleanups.

kfree(bo); drm_gem_object_put(vm_root_gem); } @@ -1026,6 +1030,7 @@ panthor_gem_create(struct drm_device *dev, size_t size, uint32_t flags, } panthor_gem_debugfs_set_usage_flags(bo, usage_flags);

Unrelated whitespace change (though again, I do like it)

return bo; err_put: @@ -1033,6 +1038,54 @@ panthor_gem_create(struct drm_device *dev, size_t size, uint32_t flags, return ERR_PTR(ret); } +static struct panthor_gem_object * +panthor_gem_create_protected(struct panthor_device *ptdev, size_t size,

• 	     uint32_t flags, struct panthor_vm *exclusive_vm,
  

• 	     u32 usage_flags)
  

+{

• struct dma_buf *dma_bo = NULL;
• struct drm_gem_object *gem_obj;
• struct panthor_gem_object *bo;
• int ret;
• if (!ptdev->protm.heap)

• return ERR_PTR(-EINVAL);
  

• if (flags != DRM_PANTHOR_BO_NO_MMAP)

• return ERR_PTR(-EINVAL);
  

• if (!exclusive_vm)

• return ERR_PTR(-EINVAL);
  

• dma_bo = dma_heap_buffer_alloc(ptdev->protm.heap, size, DMA_HEAP_VALID_FD_FLAGS,

• 		       DMA_HEAP_VALID_HEAP_FLAGS);
  

• if (IS_ERR(dma_bo))

• return ERR_PTR(PTR_ERR(dma_bo));
  

• gem_obj = drm_gem_prime_import(&ptdev->base, dma_bo);

I agree with Boris that putting the dma_buf here is the cleanest solution.

Adding a cleanup.h DEFINE_FREE helper for dmabufs would be a longer- term refactor with its own pros and cons, but would allow us to get rid of the explicit put entirely by adorning the local with a __free attribute.

• if (IS_ERR(gem_obj)) {

• ret = PTR_ERR(gem_obj);
  

• goto err_free_dma_bo;
  

• }
• bo = to_panthor_bo(gem_obj);
• bo->flags = flags;
• panthor_gem_debugfs_set_usage_flags(bo, usage_flags);
• bo->exclusive_vm_root_gem = panthor_vm_root_gem(exclusive_vm);
• drm_gem_object_get(bo->exclusive_vm_root_gem);
• bo->base.resv = bo->exclusive_vm_root_gem->resv;
• return bo;

+err_free_dma_bo:

• if (dma_bo)

• dma_buf_put(dma_bo);
  

• return ERR_PTR(ret);

+}

struct drm_gem_object * panthor_gem_prime_import_sg_table(struct drm_device *dev, struct dma_buf_attachment *attach,

Kind regards, Nicolas Frattaroli

On Tue, 5 May 2026 16:05:11 +0200 Ketil Johnsen ketil.johnsen@arm.com wrote:

From: Florent Tomasin florent.tomasin@arm.com

Refactor parts of the group scheduling logic into new helper functions. This will simplify addition of the protected mode feature.

Remove redundant assignments of csg_slot.

Signed-off-by: Florent Tomasin florent.tomasin@arm.com Co-developed-by: Ketil Johnsen ketil.johnsen@arm.com Signed-off-by: Ketil Johnsen ketil.johnsen@arm.com

Glad to see this big tick_ctx_apply() function split into smaller pieces.

Reviewed-by: Boris Brezillon boris.brezillon@collabora.com

---

drivers/gpu/drm/panthor/panthor_sched.c | 135 +++++++++++++++--------- 1 file changed, 86 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c index 5ee386338005c..987072bd867c4 100644 --- a/drivers/gpu/drm/panthor/panthor_sched.c +++ b/drivers/gpu/drm/panthor/panthor_sched.c @@ -1934,6 +1934,12 @@ static void csgs_upd_ctx_init(struct panthor_csg_slots_upd_ctx *ctx) memset(ctx, 0, sizeof(*ctx)); } +static void csgs_upd_ctx_ring_doorbell(struct panthor_csg_slots_upd_ctx *ctx,

• 		       u32 csg_id)
  

+{

• ctx->update_mask |= BIT(csg_id);

+}

static void csgs_upd_ctx_queue_reqs(struct panthor_device *ptdev, struct panthor_csg_slots_upd_ctx *ctx, u32 csg_id, u32 value, u32 mask) @@ -1944,7 +1950,8 @@ static void csgs_upd_ctx_queue_reqs(struct panthor_device *ptdev, ctx->requests[csg_id].value = (ctx->requests[csg_id].value & ~mask) | (value & mask); ctx->requests[csg_id].mask |= mask;

• ctx->update_mask |= BIT(csg_id);

• csgs_upd_ctx_ring_doorbell(ctx, csg_id);

} static int csgs_upd_ctx_apply_locked(struct panthor_device *ptdev, @@ -1961,8 +1968,12 @@ static int csgs_upd_ctx_apply_locked(struct panthor_device *ptdev, while (update_slots) { struct panthor_fw_csg_iface *csg_iface; u32 csg_id = ffs(update_slots) - 1;

• u32 req_mask = ctx->requests[csg_id].mask;
  

update_slots &= ~BIT(csg_id);

• if (!req_mask)
  

• 	continue;
  

• csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id); panthor_fw_update_reqs(csg_iface, req, ctx->requests[csg_id].value,

@@ -1979,6 +1990,9 @@ static int csgs_upd_ctx_apply_locked(struct panthor_device *ptdev, int ret; update_slots &= ~BIT(csg_id);

• if (!req_mask)
  

• 	continue;
  

• csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);

ret = panthor_fw_csg_wait_acks(ptdev, csg_id, req_mask, &acked, 100); @@ -2266,12 +2280,76 @@ tick_ctx_cleanup(struct panthor_scheduler *sched, } } +static void +tick_ctx_evict_group(struct panthor_scheduler *sched,

•      struct panthor_csg_slots_upd_ctx *upd_ctx,
  

•      struct panthor_group *group)
  

+{

• struct panthor_device *ptdev = sched->ptdev;
• if (drm_WARN_ON(&ptdev->base, group->csg_id < 0))

• return;
  

• csgs_upd_ctx_queue_reqs(ptdev, upd_ctx, group->csg_id,

• 		group_can_run(group) ?
  

• 		CSG_STATE_SUSPEND : CSG_STATE_TERMINATE,
  

• 		CSG_STATE_MASK);
  

+}

+static void +tick_ctx_reschedule_group(struct panthor_scheduler *sched,

• 	  struct panthor_csg_slots_upd_ctx *upd_ctx,
  

• 	  struct panthor_group *group,
  

• 	  int new_csg_prio)
  

+{

• struct panthor_device *ptdev = sched->ptdev;
• struct panthor_fw_csg_iface *csg_iface;
• struct panthor_csg_slot *csg_slot;
• if (group->csg_id < 0)

• return;
  

• csg_iface = panthor_fw_get_csg_iface(ptdev, group->csg_id);
• csg_slot = &sched->csg_slots[group->csg_id];
• if (csg_slot->priority != new_csg_prio) {

• panthor_fw_update_reqs(csg_iface, endpoint_req,
  

• 		       CSG_EP_REQ_PRIORITY(new_csg_prio),
  

• 		       CSG_EP_REQ_PRIORITY_MASK);
  

• csgs_upd_ctx_queue_reqs(ptdev, upd_ctx, group->csg_id,
  

• 			csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
  

• 			CSG_ENDPOINT_CONFIG);
  

• }

+}

+static void +tick_ctx_schedule_group(struct panthor_scheduler *sched,

• 	struct panthor_sched_tick_ctx *ctx,
  

• 	struct panthor_csg_slots_upd_ctx *upd_ctx,
  

• 	struct panthor_group *group,
  

• 	int csg_id, int csg_prio)
  

+{

• struct panthor_device *ptdev = sched->ptdev;
• struct panthor_fw_csg_iface *csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
• group_bind_locked(group, csg_id);
• csg_slot_prog_locked(ptdev, csg_id, csg_prio);
• csgs_upd_ctx_queue_reqs(ptdev, upd_ctx, csg_id,

• 		group->state == PANTHOR_CS_GROUP_SUSPENDED ?
  

• 		CSG_STATE_RESUME : CSG_STATE_START,
  

• 		CSG_STATE_MASK);
  

• csgs_upd_ctx_queue_reqs(ptdev, upd_ctx, csg_id,

• 		csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
  

• 		CSG_ENDPOINT_CONFIG);
  

+}

static void tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *ctx) { struct panthor_group *group, *tmp; struct panthor_device *ptdev = sched->ptdev;

• struct panthor_csg_slot *csg_slot; int prio, new_csg_prio = MAX_CSG_PRIO, i; u32 free_csg_slots = 0; struct panthor_csg_slots_upd_ctx upd_ctx;

@@ -2282,42 +2360,12 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) { /* Suspend or terminate evicted groups. */ list_for_each_entry(group, &ctx->old_groups[prio], run_node) {

• 	bool term = !group_can_run(group);
  

• 	int csg_id = group->csg_id;
  

• 	if (drm_WARN_ON(&ptdev->base, csg_id < 0))
  

• 		continue;
  

• 	csg_slot = &sched->csg_slots[csg_id];
  

• 	csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
  

• 				term ? CSG_STATE_TERMINATE : CSG_STATE_SUSPEND,
  

• 				CSG_STATE_MASK);
  

• 	tick_ctx_evict_group(sched, &upd_ctx, group);
  

  }

/* Update priorities on already running groups. */ list_for_each_entry(group, &ctx->groups[prio], run_node) {

• 	struct panthor_fw_csg_iface *csg_iface;
  

• 	int csg_id = group->csg_id;
  

• 	if (csg_id < 0) {
  

• 		new_csg_prio--;
  

• 		continue;
  

• 	}
  

• 	csg_slot = &sched->csg_slots[csg_id];
  

• 	csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
  

• 	if (csg_slot->priority == new_csg_prio) {
  

• 		new_csg_prio--;
  

• 		continue;
  

• 	}
  

• 	panthor_fw_csg_endpoint_req_update(ptdev, csg_iface,
  

• 					   CSG_EP_REQ_PRIORITY(new_csg_prio),
  

• 					   CSG_EP_REQ_PRIORITY_MASK);
  

• 	csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
  

• 				csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
  

• 				CSG_ENDPOINT_CONFIG);
  

• 	new_csg_prio--;
  

• 	tick_ctx_reschedule_group(sched, &upd_ctx, group, new_csg_prio--);
  

  } }

@@ -2354,28 +2402,17 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1; prio >= 0; prio--) { list_for_each_entry(group, &ctx->groups[prio], run_node) { int csg_id = group->csg_id;

• 	struct panthor_fw_csg_iface *csg_iface;
  

• 	int csg_prio = new_csg_prio--;
  

• 	if (csg_id >= 0) {
  

• 		new_csg_prio--;
  

• 	if (csg_id >= 0)
  	continue;
  

• 	}
  

csg_id = ffs(free_csg_slots) - 1; if (drm_WARN_ON(&ptdev->base, csg_id < 0)) break;

• 	csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
  

• 	csg_slot = &sched->csg_slots[csg_id];
  

• 	group_bind_locked(group, csg_id);
  

• 	csg_slot_prog_locked(ptdev, csg_id, new_csg_prio--);
  

• 	csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
  

• 				group->state == PANTHOR_CS_GROUP_SUSPENDED ?
  

• 				CSG_STATE_RESUME : CSG_STATE_START,
  

• 				CSG_STATE_MASK);
  

• 	csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
  

• 				csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
  

• 				CSG_ENDPOINT_CONFIG);
  

• 	tick_ctx_schedule_group(sched, ctx, &upd_ctx, group, csg_id, csg_prio);
  

• free_csg_slots &= ~BIT(csg_id);
  

  } }

On Tue, 5 May 2026 16:05:13 +0200 Ketil Johnsen ketil.johnsen@arm.com wrote:

Here comes the second part of the review :-).

diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c index 2ab444ee8c710..e93042eaf3fc8 100644 --- a/drivers/gpu/drm/panthor/panthor_gpu.c +++ b/drivers/gpu/drm/panthor/panthor_gpu.c @@ -100,8 +100,11 @@ static void panthor_gpu_irq_handler(struct panthor_device *ptdev, u32 status) fault_status, panthor_exception_name(ptdev, fault_status & 0xFF), address); }

• if (status & GPU_IRQ_PROTM_FAULT)

• if (status & GPU_IRQ_PROTM_FAULT) { drm_warn(&ptdev->base, "GPU Fault in protected mode\n");

• panthor_gpu_disable_protm_fault_interrupt(ptdev);
  

It's only used in a single place, so I'd just inline the content of panthor_gpu_disable_protm_fault_interrupt() here. Also, I think panthor_gpu_disable_protm_fault_interrupt() is not taking the right lock (see below).

• panthor_device_schedule_reset(ptdev);
  

• }

spin_lock(&ptdev->gpu->reqs_lock); if (status & ptdev->gpu->pending_reqs) { @@ -367,6 +370,10 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev) unsigned long flags; spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags);

• /** Re-enable the protm_irq_fault when reset is complete */
• ptdev->gpu->irq.mask |= GPU_IRQ_PROTM_FAULT;

panthor_irq::mask should only be modified with the panthor_irq::mask_lock held. Besides, we have a helper for that:

panthor_gpu_irq_enable_events(&ptdev->gpu->irq, GPU_IRQ_PROTM_FAULT);

• if (!drm_WARN_ON(&ptdev->base, ptdev->gpu->pending_reqs & GPU_IRQ_RESET_COMPLETED)) { ptdev->gpu->pending_reqs |= GPU_IRQ_RESET_COMPLETED;

@@ -427,3 +434,8 @@ void panthor_gpu_resume(struct panthor_device *ptdev) panthor_hw_l2_power_on(ptdev); } +void panthor_gpu_disable_protm_fault_interrupt(struct panthor_device *ptdev) +{

• scoped_guard(spinlock_irqsave, &ptdev->gpu->reqs_lock)

• ptdev->gpu->irq.mask &= ~GPU_IRQ_PROTM_FAULT;
  

Same problem with wrong lock being taken to modify the mask, and panthor_gpu_irq_disable_events() probably being a better option?

+} diff --git a/drivers/gpu/drm/panthor/panthor_gpu.h b/drivers/gpu/drm/panthor/panthor_gpu.h index 12c263a399281..ca66c73f543e6 100644 --- a/drivers/gpu/drm/panthor/panthor_gpu.h +++ b/drivers/gpu/drm/panthor/panthor_gpu.h @@ -54,4 +54,10 @@ int panthor_gpu_soft_reset(struct panthor_device *ptdev); void panthor_gpu_power_changed_off(struct panthor_device *ptdev); int panthor_gpu_power_changed_on(struct panthor_device *ptdev); +/**

• • panthor_gpu_disable_protm_fault_interrupt() - Disable GPU_PROTECTED_FAULT interrupt
• • @ptdev: Device.
• */

+void panthor_gpu_disable_protm_fault_interrupt(struct panthor_device *ptdev);

#endif diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c index 07f54176ec1bf..702f537905b56 100644 --- a/drivers/gpu/drm/panthor/panthor_mmu.c +++ b/drivers/gpu/drm/panthor/panthor_mmu.c @@ -31,6 +31,7 @@ #include <linux/sizes.h> #include "panthor_device.h" +#include "panthor_fw.h" #include "panthor_gem.h" #include "panthor_gpu.h" #include "panthor_heap.h" @@ -1704,8 +1705,12 @@ static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size) if (drm_WARN_ON(&ptdev->base, vm->locked_region.size)) return -EINVAL;

• down_read(&ptdev->protm.lock);
• mutex_lock(&ptdev->mmu->as.slots_lock); if (vm->as.id >= 0 && size) {

• panthor_fw_protm_exit_sync(ptdev);
  

• /* Lock the region that needs to be updated */ gpu_write64(ptdev, AS_LOCKADDR(vm->as.id), pack_region_range(ptdev, &start, &size));

@@ -1720,6 +1725,9 @@ static int panthor_vm_lock_region(struct panthor_vm *vm, u64 start, u64 size) } mutex_unlock(&ptdev->mmu->as.slots_lock);

• if (ret)

• up_read(&ptdev->protm.lock);
  

Let's try to keep the locked section local to a function: the protm.lock should, IMHO, be taken/released in panthor_vm_exec_op(). If we go for that, this also makes the panthor_vm_lock_region() vs panthor_vm_expand_locked_region() distinction useless, though it's probably fine to keep it for clarity.

• return ret;

} @@ -1805,6 +1813,8 @@ static void panthor_vm_unlock_region(struct panthor_vm *vm) vm->locked_region.start = 0; vm->locked_region.size = 0; mutex_unlock(&ptdev->mmu->as.slots_lock);

• up_read(&ptdev->protm.lock);

} static void panthor_mmu_irq_handler(struct panthor_device *ptdev, u32 status) diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c index 987072bd867c4..acb04250c7def 100644 --- a/drivers/gpu/drm/panthor/panthor_sched.c +++ b/drivers/gpu/drm/panthor/panthor_sched.c @@ -308,6 +308,15 @@ struct panthor_scheduler { */ struct list_head stopped_groups; } reset;

• /** @protm: Protected mode related fields. */
• struct {

• /** @protected_mode: True if GPU is in protected mode. */
  

• bool protected_mode;
  

nit: s/protected_mode/enabled/s. But do we even need that boolean? Isn't active_group != NULL providing the same info?

• /** @active_group: The active protected group. */
  

• struct panthor_group *active_group;
  

• } protm;

}; /** @@ -570,6 +579,16 @@ struct panthor_group { /** @fatal_queues: Bitmask reflecting the queues that hit a fatal exception. */ u32 fatal_queues;

• /**

• * @protm_pending_queues: Bitmask reflecting the queues that are waiting
  

• *                        on a CS_PROTM_PENDING.
  

• *
  

• * The GPU will set the bit associated to the queue pending protected mode
  

• * when a PROT_REGION command is executing or when trying to resume previously
  

• * suspended protected mode jobs.
  

• */
  

• u32 protm_pending_queues;
• /** @tiler_oom: Mask of queues that have a tiler OOM event to process. */ atomic_t tiler_oom;

@@ -1176,6 +1195,7 @@ queue_resume_timeout(struct panthor_queue *queue)

• @ptdev: Device.
• @csg_id: Group slot ID.
• @cs_id: Queue slot ID.

• • @protm_ack: Acknowledge pending protected mode queues
  • Program a queue slot with the queue information so things can start being
  • executed on this queue.

@@ -1472,6 +1492,34 @@ csg_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 priority) return 0; } +static void +cs_slot_process_protm_pending_event_locked(struct panthor_device *ptdev,

• 			   u32 csg_id, u32 cs_id)
  

+{

• struct panthor_scheduler *sched = ptdev->scheduler;
• struct panthor_csg_slot *csg_slot = &sched->csg_slots[csg_id];
• struct panthor_group *group = csg_slot->group;
• lockdep_assert_held(&sched->lock);
• if (!group)

• return;
  

• /* No protected memory heap, a user space program tried to

• * submit a protected mode jobs resulting in the GPU raising
  

• * a CS_PROTM_PENDING request.
  

• *
  

• * This scenario is invalid and the protected mode jobs must
  

• * not be allowed to progress.
  

• */
  

• if (!ptdev->protm.heap)

• return;
  

Should we flag the group unusable when that happens and schedule it out as soon as possible?

if (!ptdev->protm.heap) group->fatal_queues |= BIT(cs_id); else group->protm_pending_queues |= BIT(cs_id);

sched_queue_delayed_work(sched, tick, 0);

• group->protm_pending_queues |= BIT(cs_id);
• sched_queue_delayed_work(sched, tick, 0);

+}

static void cs_slot_process_fatal_event_locked(struct panthor_device *ptdev, u32 csg_id, u32 cs_id) @@ -1718,6 +1766,9 @@ static bool cs_slot_process_irq_locked(struct panthor_device *ptdev, if (events & CS_TILER_OOM) cs_slot_process_tiler_oom_event_locked(ptdev, csg_id, cs_id);

• if (events & CS_PROTM_PENDING)

• cs_slot_process_protm_pending_event_locked(ptdev, csg_id, cs_id);
  

• /* We don't acknowledge the TILER_OOM event since its handling is
  • deferred to a separate work.
  */

@@ -1848,6 +1899,17 @@ static void sched_process_idle_event_locked(struct panthor_device *ptdev) sched_queue_delayed_work(ptdev->scheduler, tick, 0); } +static void sched_process_protm_exit_event_locked(struct panthor_device *ptdev) +{

• lockdep_assert_held(&ptdev->scheduler->lock);
• /* Acknowledge the protm exit and schedule a tick. */
• panthor_fw_protm_exit(ptdev);

Let's just inline the content of panthor_fw_protm_exit() here.* It doesn't make sense to have all these indirections, especially since PROTM and scheduling are intertwined anyway, so I consider it part of the scheduler responsibility (just like the scheduler deals with other GLB events).

The same goes for the other panthor_fw_protm_ helpers defined in panthor_fw.c, I think panthor_sched.c would be a better fit for those, or even inlining their content if they are only used in a single place.

• sched_queue_delayed_work(ptdev->scheduler, tick, 0);
• ptdev->scheduler->protm.protected_mode = false;
• ptdev->scheduler->protm.active_group = NULL;

+}

/**

• sched_process_global_irq_locked() - Process the scheduling part of a global IRQ
• @ptdev: Device.

@@ -1863,6 +1925,9 @@ static void sched_process_global_irq_locked(struct panthor_device *ptdev) ack = READ_ONCE(glb_iface->output->ack); evts = (req ^ ack) & GLB_EVT_MASK;

• if (evts & GLB_PROTM_EXIT)

• sched_process_protm_exit_event_locked(ptdev);
  

• if (evts & GLB_IDLE) sched_process_idle_event_locked(ptdev);

} @@ -1872,23 +1937,71 @@ static void process_fw_events_work(struct work_struct *work) struct panthor_scheduler *sched = container_of(work, struct panthor_scheduler, fw_events_work); u32 events = atomic_xchg(&sched->fw_events, 0);

• u32 csg_events = events & ~JOB_INT_GLOBAL_IF; struct panthor_device *ptdev = sched->ptdev;

mutex_lock(&sched->lock);

• while (csg_events) {

• u32 csg_id = ffs(csg_events) - 1;
  

• sched_process_csg_irq_locked(ptdev, csg_id);
  

• csg_events &= ~BIT(csg_id);
  

• }

I'm sure you have a good reason to re-order the processing of CSG and GLB events, and it'd be good to have it documented in a comment.

• if (events & JOB_INT_GLOBAL_IF) { sched_process_global_irq_locked(ptdev); events &= ~JOB_INT_GLOBAL_IF; }

• while (events) {

• u32 csg_id = ffs(events) - 1;
  

• mutex_unlock(&sched->lock);

+}

• sched_process_csg_irq_locked(ptdev, csg_id);
  

• events &= ~BIT(csg_id);
  

+static void handle_protm_fault(struct panthor_device *ptdev)

This is a bit of a misnomer, I think. It doesn't seem to be triggered by a FAULT event, it's more a timeout on a PROTM section that would lead to a reset being scheduled, and this "blocked-in-protm" situation being detected as part of the reset.

+{

• struct panthor_scheduler *sched = ptdev->scheduler;
• u32 csg_id;
• struct panthor_group *protm_group;
• guard(mutex)(&sched->lock);
• if (!sched->protm.protected_mode)

• return;
  

• protm_group = sched->protm.active_group;
• if (drm_WARN_ON(&ptdev->base, !protm_group))

• return;
  

See, that's a perfect example of sched->protm.protected_mode being redundant. Now you're left with a potential protected_mode=true,active_group=NULL inconsistency you don't expect.

• /* Group will be terminated by the device reset */
• protm_group->fatal_queues |= GENMASK(protm_group->queue_count - 1, 0);
• if (!panthor_fw_protm_exit_wait_event_timeout(ptdev))

• goto cleanup_protm;
  

• /**

• * GPU failed to exit protected mode. Mark all non-protected mode CSGs
  

/* GPU failed to exit protected mode. Mark all non-protected mode CSGs

• * as suspended so that they are unaffected by the GPU reset.
  

• */
  

• for (csg_id = 0; csg_id < sched->csg_slot_count; csg_id++) {

• struct panthor_group *group = sched->csg_slots[csg_id].group;
  

• if (!group || group == protm_group)
  

• 	continue;
  

• group->state = PANTHOR_CS_GROUP_SUSPENDED;
  

• group_unbind_locked(group);
  

• list_move(&group->run_node, group_is_idle(group) ?
  

• 				&sched->groups.idle[group->priority] :
  

• 				&sched->groups.runnable[group->priority]);
  

nit: Let's use a local struct list_head * variable to select the right list.

}

• mutex_unlock(&sched->lock);

+cleanup_protm:

• sched->protm.protected_mode = false;
• sched->protm.active_group = NULL;

} /** @@ -2029,6 +2142,7 @@ struct panthor_sched_tick_ctx { bool immediate_tick; bool stop_tick; u32 csg_upd_failed_mask;

• struct panthor_group *protm_group;

}; static bool @@ -2299,6 +2413,7 @@ tick_ctx_evict_group(struct panthor_scheduler *sched, static void tick_ctx_reschedule_group(struct panthor_scheduler *sched,

• 	  struct panthor_sched_tick_ctx *ctx,
    struct panthor_csg_slots_upd_ctx *upd_ctx,
    struct panthor_group *group,
    int new_csg_prio)
  

@@ -2321,6 +2436,30 @@ tick_ctx_reschedule_group(struct panthor_scheduler *sched, csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG, CSG_ENDPOINT_CONFIG); }

• if (ctx->protm_group == group) {

• for (u32 q = 0; q < group->queue_count; q++) {
  

• 	struct panthor_fw_cs_iface *cs_iface;
  

• 	if (!(group->protm_pending_queues & BIT(q)))
  

• 		continue;
  

• 	cs_iface = panthor_fw_get_cs_iface(ptdev, group->csg_id, q);
  

• 	panthor_fw_update_reqs(cs_iface, req, cs_iface->output->ack,
  

• 			       CS_PROTM_PENDING);
  

• }
  

• panthor_fw_toggle_reqs(csg_iface, doorbell_req, doorbell_ack,
  

• 		       group->protm_pending_queues);
  

• csgs_upd_ctx_ring_doorbell(upd_ctx, group->csg_id);
  

• group->protm_pending_queues = 0;
  

• /*
  

•  * We only allow one protected group to run at same time,
  

•  * as it makes it easier to handle faults in protected mode.
  

It's more something to document in the panthor_scheduler::protm::active_group section.

•  */
  

• sched->protm.active_group = group;
  

Would it make sense to move this logic to a tick_ctx_handle_protm_group() helper that's called before/after tick_ctx_reschedule_group()? This way there's no extra if (ctx->protm_group == group) conditional branch in here.

static void tick_ctx_handle_protm_group(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *ctx, struct panthor_csg_slots_upd_ctx *upd_ctx) { struct panthor_device *ptdev = sched->ptdev; struct panthor_group *group = ctx->protm_group; struct panthor_fw_csg_iface *csg_iface;

if (!group || drm_WARN_ON(&ptdev->base, group->csg_id < 0)) return;

csg_iface = panthor_fw_get_csg_iface(ptdev, group->csg_id); for (u32 q = 0; q < group->queue_count; q++) { struct panthor_fw_cs_iface *cs_iface;

if (!(group->protm_pending_queues & BIT(q))) continue;

cs_iface = panthor_fw_get_cs_iface(ptdev, group->csg_id, q); panthor_fw_update_reqs(cs_iface, req, cs_iface->output->ack, CS_PROTM_PENDING); }

panthor_fw_toggle_reqs(csg_iface, doorbell_req, doorbell_ack, group->protm_pending_queues); csgs_upd_ctx_ring_doorbell(upd_ctx, group->csg_id); group->protm_pending_queues = 0; sched->protm.active_group = group; }

• }

} static void @@ -2336,6 +2475,17 @@ tick_ctx_schedule_group(struct panthor_scheduler *sched, group_bind_locked(group, csg_id); csg_slot_prog_locked(ptdev, csg_id, csg_prio);

• /* If the group was waiting for protected mode before suspension,

• * and the tick context enters this mode, it should be serviced
  

• * immediately because the slot reset should have set the
  

• * CS_PROTM_PENDING bit to zero, and cs_prog_slot_locked() sets it to
  

• * zero too.
  

• * It's not clear if we will get a new CS_PROTM_PENDING event in that
  

• * case, but it should be safe either way.
  

• */
  

• if (group->protm_pending_queues && ctx->protm_group)

• group->protm_pending_queues = 0;
  

I'd move this to the path where we do the SUSPEND, or group_unbind(), even.

• csgs_upd_ctx_queue_reqs(ptdev, upd_ctx, csg_id, group->state == PANTHOR_CS_GROUP_SUSPENDED ? CSG_STATE_RESUME : CSG_STATE_START,

@@ -2365,7 +2515,7 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c /* Update priorities on already running groups. */ list_for_each_entry(group, &ctx->groups[prio], run_node) {

• 	tick_ctx_reschedule_group(sched, &upd_ctx, group, new_csg_prio--);
  

• 	tick_ctx_reschedule_group(sched, ctx, &upd_ctx, group, new_csg_prio--);
  

  } }

@@ -2457,6 +2607,15 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c sched->used_csg_slot_count = ctx->group_count; sched->might_have_idle_groups = ctx->idle_group_count > 0;

• if (ctx->protm_group) {

• ret = panthor_fw_protm_enter(ptdev);
  

• if (ret) {
  

• 	panthor_device_schedule_reset(ptdev);
  

• 	ctx->csg_upd_failed_mask = U32_MAX;
  

It's weird to flag it as all CSGs update failed. Should we instead have

/* If we failed to enter PROTM, consider the group who * requested it as failed. */ ctx->csg_upd_failed_mask |= BIT(ctx->protm_group->csg_id);

• }
  

• sched->protm.protected_mode = true;
  

I'd move that to a tick_ctx_service_protm_req() helper that has the panthor_fw_protm_enter() inlined, because again, it doesn't make sense to have this defined in panthor_fw.c if the only user lives in panthor_sched.c

• }

} static u64 @@ -2490,7 +2649,7 @@ static void tick_work(struct work_struct *work) u64 resched_target = sched->resched_target; u64 remaining_jiffies = 0, resched_delay; u64 now = get_jiffies_64();

• int prio, ret, cookie;

• int prio, protm_prio, ret, cookie; bool full_tick;

if (!drm_dev_enter(&ptdev->base, &cookie)) @@ -2564,14 +2723,49 @@ static void tick_work(struct work_struct *work) } }

• /* Check if the highest priority group want to switch to protected mode */
• for (protm_prio = PANTHOR_CSG_PRIORITY_COUNT - 1; protm_prio >= 0; protm_prio--) {

• struct panthor_group *group;
  

• group = list_first_entry_or_null(&ctx.groups[protm_prio],
  

• 				 struct panthor_group,
  

• 				 run_node);
  

• if (group) {
  

• 	ctx.protm_group = group;
  

• 	break;
  

• }
  

Should this be

if (group) { if (group->protm_pending_queues) ctx.protm_group = group;

break; }

?

• }
• /* If we have free CSG slots left, pick idle groups */

• for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1;

•     prio >= 0 && !tick_ctx_is_full(sched, &ctx);
  

•     prio--) {
  

How about we keep it a single indentation level and skip higher prios if PROTM is requested:

/* Pick only idle groups with equal or lower priority than the * group triggering protected mode. Do not bother picking * unscheduled idle groups. */ if (ctx.protm_group && prio < protm_prio) continue;

This saves us an indentation level and limits the code duplication.

• /* Check the old_group queue first to avoid reprogramming the slots */
  

• tick_ctx_pick_groups_from_list(sched, &ctx, &ctx.old_groups[prio], false, true);
  

• tick_ctx_pick_groups_from_list(sched, &ctx, &sched->groups.idle[prio],
  

• 			       false, false);
  

• if (ctx.protm_group) {

• /* Pick only idle groups with equal or lower priority than the
  

•  * group triggering protected mode. Do not bother picking
  

•  * unscheduled idle groups.
  

•  */
  

• for (prio = protm_prio;
  

•      prio >= 0 && !tick_ctx_is_full(sched, &ctx);
  

•      prio--)
  

• 	tick_ctx_pick_groups_from_list(sched, &ctx,
  

• 				       &ctx.old_groups[prio],
  

• 				       false, true);
  

• } else {

• /* No switch to protected, just pick any idle group according
  

•  * to priority
  

•  */
  

• for (prio = PANTHOR_CSG_PRIORITY_COUNT - 1;
  

•      prio >= 0 && !tick_ctx_is_full(sched, &ctx);
  

•      prio--) {
  

• 	/* Check the old_group queue first to avoid
  

• 	 * reprogramming the slots
  

• 	 */
  

• 	tick_ctx_pick_groups_from_list(sched, &ctx,
  

• 				       &ctx.old_groups[prio],
  

• 				       false, true);
  

• 	tick_ctx_pick_groups_from_list(sched, &ctx,
  

• 				       &sched->groups.idle[prio],
  

• 				       false, false);
  

• }
  

• }

tick_ctx_apply(sched, &ctx); @@ -2993,6 +3187,8 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev) cancel_work_sync(&sched->sync_upd_work); cancel_delayed_work_sync(&sched->tick_work);

• handle_protm_fault(ptdev);

I actually wonder if this should be part of the panthor_sched_suspend() logic. That is, we would automatically flag all non-protm groups as suspended if the GPU was in PROTM mode at the time the hang happened.

• panthor_sched_suspend(ptdev);

/* Stop all groups that might still accept jobs, so we don't get passed

Regards,

Boris

On Tue, 5 May 2026 16:05:14 +0200 Ketil Johnsen ketil.johnsen@arm.com wrote:

Add query for protected rendering capability. Add flag to group creation to specify need for protected rendering. Bump panthor version number.

Signed-off-by: Ketil Johnsen ketil.johnsen@arm.com

drivers/gpu/drm/panthor/panthor_drv.c | 21 +++++++++++- drivers/gpu/drm/panthor/panthor_sched.c | 21 +++++++----- include/uapi/drm/panthor_drm.h | 45 +++++++++++++++++++++++-- 3 files changed, 76 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c index 73fc983dc9b44..817df17f31f15 100644 --- a/drivers/gpu/drm/panthor/panthor_drv.c +++ b/drivers/gpu/drm/panthor/panthor_drv.c @@ -177,6 +177,7 @@ panthor_get_uobj_array(const struct drm_panthor_obj_array *in, u32 min_stride, PANTHOR_UOBJ_DECL(struct drm_panthor_csif_info, pad), \ PANTHOR_UOBJ_DECL(struct drm_panthor_timestamp_info, current_timestamp), \ PANTHOR_UOBJ_DECL(struct drm_panthor_group_priorities_info, pad), \

•  PANTHOR_UOBJ_DECL(struct drm_panthor_protected_info, features), \
  

  PANTHOR_UOBJ_DECL(struct drm_panthor_sync_op, timeline_value), \ PANTHOR_UOBJ_DECL(struct drm_panthor_queue_submit, syncs), \ PANTHOR_UOBJ_DECL(struct drm_panthor_queue_create, ringbuf_size), \

@@ -928,12 +929,20 @@ static void panthor_query_group_priorities_info(struct drm_file *file, } } +static void panthor_query_protected_info(struct panthor_device *ptdev,

• 			 struct drm_panthor_protected_info *arg)
  

+{

• arg->features =

• ptdev->protm.heap ? DRM_PANTHOR_PROTECTED_FEATURE_BASIC : 0;
  

+}

static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct drm_file *file) { struct panthor_device *ptdev = container_of(ddev, struct panthor_device, base); struct drm_panthor_dev_query *args = data; struct drm_panthor_timestamp_info timestamp_info; struct drm_panthor_group_priorities_info priorities_info;

• struct drm_panthor_protected_info protected_info; int ret;

if (!args->pointer) { @@ -954,6 +963,10 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d args->size = sizeof(priorities_info); return 0;

• case DRM_PANTHOR_DEV_QUERY_PROTECTED_INFO:
  

• 	args->size = sizeof(protected_info);
  

• 	return 0;
  

• default: return -EINVAL; }

@@ -984,6 +997,11 @@ static int panthor_ioctl_dev_query(struct drm_device *ddev, void *data, struct d panthor_query_group_priorities_info(file, &priorities_info); return PANTHOR_UOBJ_SET(args->pointer, args->size, priorities_info);

• case DRM_PANTHOR_DEV_QUERY_PROTECTED_INFO:

• panthor_query_protected_info(ptdev, &protected_info);
  

• return PANTHOR_UOBJ_SET(args->pointer, args->size,
  

• 			protected_info);
  

• default: return -EINVAL; }

@@ -1779,6 +1797,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)

•   - adds DRM_IOCTL_PANTHOR_BO_QUERY_INFO ioctl
  

•   - adds drm_panthor_gpu_info::selected_coherency
  

• • 1.8 - extends DEV_QUERY_TIMESTAMP_INFO with flags

• • • 1.9 - adds DEV_QUERY_PROTECTED_INFO query

It's adding more than just DEV_QUERY_PROTECTED_INFO (it also adds a new flags field to group_create and a flag that tells that the group intends to use protected mode).

*/ static const struct drm_driver panthor_drm_driver = { .driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ | @@ -1792,7 +1811,7 @@ static const struct drm_driver panthor_drm_driver = { .name = "panthor", .desc = "Panthor DRM driver", .major = 1,

• .minor = 8,

• .minor = 9,

.gem_prime_import_sg_table = panthor_gem_prime_import_sg_table, .gem_prime_import = panthor_gem_prime_import, diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c index acb04250c7def..0e8a1059de589 100644 --- a/drivers/gpu/drm/panthor/panthor_sched.c +++ b/drivers/gpu/drm/panthor/panthor_sched.c @@ -3868,6 +3868,7 @@ static void add_group_kbo_sizes(struct panthor_device *ptdev, } #define MAX_GROUPS_PER_POOL 128 +#define GROUP_CREATE_FLAGS DRM_PANTHOR_GROUP_CREATE_PROTECTED int panthor_group_create(struct panthor_file *pfile, const struct drm_panthor_group_create *group_args, @@ -3882,10 +3883,10 @@ int panthor_group_create(struct panthor_file *pfile, u32 gid, i, suspend_size; int ret;

• if (group_args->pad)

• if (group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT) return -EINVAL;

• if (group_args->priority >= PANTHOR_CSG_PRIORITY_COUNT)

• if (group_args->flags & ~GROUP_CREATE_FLAGS) return -EINVAL;

if ((group_args->compute_core_mask & ~ptdev->gpu_info.shader_present) || @@ -3937,12 +3938,16 @@ int panthor_group_create(struct panthor_file *pfile, goto err_put_group; }

• suspend_size = csg_iface->control->protm_suspend_size;
• group->protm_suspend_buf = panthor_fw_alloc_protm_suspend_buf_mem(ptdev, suspend_size);
• if (IS_ERR(group->protm_suspend_buf)) {

• ret = PTR_ERR(group->protm_suspend_buf);
  

• group->protm_suspend_buf = NULL;
  

• goto err_put_group;
  

• if (group_args->flags & DRM_PANTHOR_GROUP_CREATE_PROTECTED) {

• suspend_size = csg_iface->control->protm_suspend_size;
  

• group->protm_suspend_buf =
  

• 	panthor_fw_alloc_protm_suspend_buf_mem(ptdev,
  

• 					       suspend_size);
  

• if (IS_ERR(group->protm_suspend_buf)) {
  

• 	ret = PTR_ERR(group->protm_suspend_buf);
  

• 	group->protm_suspend_buf = NULL;
  

• 	goto err_put_group;
  

• }
  

  }

group->syncobjs = panthor_kernel_bo_create(ptdev, group->vm, diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h index 0e455d91e77d4..914110003bcd1 100644 --- a/include/uapi/drm/panthor_drm.h +++ b/include/uapi/drm/panthor_drm.h @@ -253,6 +253,11 @@ enum drm_panthor_dev_query_type { * @DRM_PANTHOR_DEV_QUERY_GROUP_PRIORITIES_INFO: Query allowed group priorities information. */ DRM_PANTHOR_DEV_QUERY_GROUP_PRIORITIES_INFO,

• /**

• * @DRM_PANTHOR_DEV_QUERY_PROTECTED_INFO: Query supported protected rendering information.
  

• */
  

• DRM_PANTHOR_DEV_QUERY_PROTECTED_INFO,

}; /** @@ -504,6 +509,28 @@ struct drm_panthor_group_priorities_info { __u8 pad[3]; }; +/**

• • enum drm_panthor_protected_feature_flags - Supported protected rendering features

Protected rendering is a bit vague, especially since it's usually referred as protected memory/content in graphics APIs. Maybe we should have a short paragraph explaining what we mean by that (access of protected memory from the GPU).

• • Place new types at the end, don't re-order, don't remove or replace.
• */

+enum drm_panthor_protected_feature_flags {

• /** @DRM_PANTHOR_PROTECTED_FEATURE_BASIC: Protected rendering available */
• DRM_PANTHOR_PROTECTED_FEATURE_BASIC = 1 << 0,

I'm not a huge fan of this _BASIC specifier, since it doesn't tell much about the actual implementation, and what the UMD has to do to access protected memory from the GPU. Given the feature/CS-instruction is named _PROTM, I'd go for

/** * @DRM_PANTHOR_PROTECTED_FEATURE_PROTM: Protected memory access * based on PROTM CS instructions * * This is currently the only option to access protected * memory from the GPU. Other modes or advanced features might * be added at some point. */ DRM_PANTHOR_PROTECTED_FEATURE_PROTM = 1 << 0,

+};

+/**

• • struct drm_panthor_protected_info - protected support information
• • Structure grouping all queryable information relating to the allowed group priorities.
• */

+struct drm_panthor_protected_info {

• /**

• * @features: Combination of enum drm_panthor_protected_feature_flags flags.
  

• */
  

• __u32 features;

+};

/**

• struct drm_panthor_dev_query - Arguments passed to DRM_PANTHOR_IOCTL_DEV_QUERY

*/ @@ -843,6 +870,18 @@ enum drm_panthor_group_priority { PANTHOR_GROUP_PRIORITY_REALTIME, }; +/**

• • enum drm_panthor_group_create_flags - Group create flags
• */

+enum drm_panthor_group_create_flags {

s/drm_panthor_group_create_flags/drm_panthor_group_feature_flags/

• /**

• * @DRM_PANTHOR_GROUP_CREATE_PROTECTED: Support protected mode
  

• *
  

• * Enable protected rendering work to be executed on this group.
  

• */
  

• DRM_PANTHOR_GROUP_CREATE_PROTECTED = 1 << 0,

I'd go directly DRM_PANTHOR_GROUP_FEATURE_PROTM, since this is the instruction the group will use to enter protected mode. If we ever have multiple ways to do protected rendering, I guess it would materialize as a different flag, allowing the KMD to know exactly the way protected rendering is going to be done.

+};

/**

• struct drm_panthor_group_create - Arguments passed to DRM_IOCTL_PANTHOR_GROUP_CREATE

*/ @@ -877,8 +916,10 @@ struct drm_panthor_group_create { /** @priority: Group priority (see enum drm_panthor_group_priority). */ __u8 priority;

• /** @pad: Padding field, MBZ. */
• __u32 pad;

• /**

• * @flags: Flags. Must be a combination of drm_panthor_group_create_flags flags.
  

• */
  

• __u32 flags;

/** * @compute_core_mask: Mask encoding cores that can be used for compute jobs.