On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
@@ -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)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
I'm not sure why it's needed at all, but if it is really needed, then s/strlen(protected_heap_name)/protected_heap_name[0]/ would simplify this.
It's not so much about how you do the test, and more about the case you're trying to protect against. I guess here you assume that panthor.protected_heap_name="" means "I don't have a protected heap for you". If it's deemed acceptable, this should most certainly be described somewhere.
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);ret = -EPROBE_DEFER;goto err_rpm_put;If you move the heap retrieval before the rpm enablement, you can get rid of this 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)
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
@@ -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)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
So unfortunately adding support for protected mode where the heap name is provided means we have to try our best to set it up at boot time, or otherwise disable protected mode support.
Best regards, Liviu
I'm not sure why it's needed at all, but if it is really needed, then s/strlen(protected_heap_name)/protected_heap_name[0]/ would simplify this.
It's not so much about how you do the test, and more about the case you're trying to protect against. I guess here you assume that panthor.protected_heap_name="" means "I don't have a protected heap for you". If it's deemed acceptable, this should most certainly be described somewhere.
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);ret = -EPROBE_DEFER;goto err_rpm_put;If you move the heap retrieval before the rpm enablement, you can get rid of this 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)
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
@@ -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)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
@@ -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)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
Best regards, Liviu
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote:
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote:
> @@ -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)) {
Do we really need this strlen() > 0? Won't dma_heap_find() fail is the name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if the heap is missing, or - panthor gets a dma-buf from userspace and does the full reset - userspace also needs to provide a dma-buf for each protected group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
For the former, expressing the relation in DT seems to be the best, but only if possible :-). Otherwise, a kconfig option (instead of module param) should be easier to work with.
Looking at the userspace implementation, can we also have an panthor ioctl to return the heap to userspace? A dma-heap ioctl to query the heap size is also lacking.
Best regards, Liviu
--
| I would like to | | fix the world, | | but they're not | | giving me the | \ source code! /
¯\_(ツ)_/¯
On Wed, 13 May 2026 12:31:32 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote:
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote: > > @@ -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)) { > > Do we really need this strlen() > 0? Won't dma_heap_find() fail is the > name is "" already?
If dma_heap_find() will fail, then the whole probe with fail too. This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if
the heap is missing, or
- panthor gets a dma-buf from userspace and does the full reset
- userspace also needs to provide a dma-buf for each protected
group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
Indeed, the second option is more ad-hoc, but when you think about it, userspace has to have this knowledge, because it needs to know the dma-heap to use for buffer allocation that cross a device boundary anyway. Think about frames produced by a video decoder, and composited by the GPU into a protected scanout buffer that's passed to the KMS device. Why would the GPU driver be source of truth when it comes to choosing the heap to use to allocate protected buffers for the video decoder or those used for the display?
For the former, expressing the relation in DT seems to be the best, but only if possible :-). Otherwise, a kconfig option (instead of module param) should be easier to work with.
Looking at the userspace implementation, can we also have an panthor ioctl to return the heap to userspace?
Yes, it's something we can add, but again, I'm questioning the usefulness of this: how can we ensure the heap used by panthor to allocate its protected FW buffers is suitable for scanout buffers (buffers that can be used by display drivers). There needs to be a glue leaving in usersland and taking the decision, and I'm not too sure trusting any of the component in the chain (vdec, gpu, display) is the right thing to do.
On Mon, May 18, 2026 at 12:16 AM Boris Brezillon boris.brezillon@collabora.com wrote:
On Wed, 13 May 2026 12:31:32 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote:
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote:
> On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote: > > > @@ -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)) { > > > > Do we really need this strlen() > 0? Won't dma_heap_find() fail is the > > name is "" already? > > If dma_heap_find() will fail, then the whole probe with fail too. > This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if
the heap is missing, or
- panthor gets a dma-buf from userspace and does the full reset
- userspace also needs to provide a dma-buf for each protected
group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
Indeed, the second option is more ad-hoc, but when you think about it, userspace has to have this knowledge, because it needs to know the dma-heap to use for buffer allocation that cross a device boundary anyway. Think about frames produced by a video decoder, and composited by the GPU into a protected scanout buffer that's passed to the KMS device. Why would the GPU driver be source of truth when it comes to choosing the heap to use to allocate protected buffers for the video decoder or those used for the display?
I don't think the GPU driver is ever the source of truth. If the system integrator wants to specify the source of truth (SoT) from kernel space, they should use the device tree (or module params / config options). If they want to specify the SoT in userspace, then we don't really care how it is done other than providing an ioctl. Panthor is always on the receiving end.
If we don't want to delay this functionality, but it takes time to converge on SoT, maybe a solution that is not a long-term promise can work? Of the options on the table (dt, module params, kconfig options, ioctls), a kconfig option, potentially marked as experimental, seems like a good candidate.
For the former, expressing the relation in DT seems to be the best, but only if possible :-). Otherwise, a kconfig option (instead of module param) should be easier to work with.
Looking at the userspace implementation, can we also have an panthor ioctl to return the heap to userspace?
Yes, it's something we can add, but again, I'm questioning the usefulness of this: how can we ensure the heap used by panthor to allocate its protected FW buffers is suitable for scanout buffers (buffers that can be used by display drivers). There needs to be a glue leaving in usersland and taking the decision, and I'm not too sure trusting any of the component in the chain (vdec, gpu, display) is the right thing to do.
The heap returned by panthor is only for panfrost/panvk. It says nothing about compatibility with other components on the system.
On Mon, 18 May 2026 17:36:40 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Mon, May 18, 2026 at 12:16 AM Boris Brezillon boris.brezillon@collabora.com wrote:
On Wed, 13 May 2026 12:31:32 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote:
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote: > On Thu, 7 May 2026 11:02:26 +0200 > Marcin Ślusarz marcin.slusarz@arm.com wrote: > > > On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote: > > > > @@ -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)) { > > > > > > Do we really need this strlen() > 0? Won't dma_heap_find() fail is the > > > name is "" already? > > > > If dma_heap_find() will fail, then the whole probe with fail too. > > This check prevents that. > > Yeah, that's also a questionable design choice. I mean, we can > currently probe and boot the FW even though we never setup the > protected FW sections, so why should we defer the probe here? Can't we > just retry the next time a group with the protected bit is created and > fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if
the heap is missing, or
- panthor gets a dma-buf from userspace and does the full reset
- userspace also needs to provide a dma-buf for each protected
group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
Indeed, the second option is more ad-hoc, but when you think about it, userspace has to have this knowledge, because it needs to know the dma-heap to use for buffer allocation that cross a device boundary anyway. Think about frames produced by a video decoder, and composited by the GPU into a protected scanout buffer that's passed to the KMS device. Why would the GPU driver be source of truth when it comes to choosing the heap to use to allocate protected buffers for the video decoder or those used for the display?
I don't think the GPU driver is ever the source of truth. If the system integrator wants to specify the source of truth (SoT) from kernel space, they should use the device tree (or module params / config options). If they want to specify the SoT in userspace, then we don't really care how it is done other than providing an ioctl. Panthor is always on the receiving end.
Okay, we're on the same page then.
If we don't want to delay this functionality, but it takes time to converge on SoT, maybe a solution that is not a long-term promise can work? Of the options on the table (dt, module params, kconfig options, ioctls), a kconfig option, potentially marked as experimental, seems like a good candidate.
If Panthor is only a consumer, I actually think it'd be easier to just let userspace pass the protected FW section as an imported buffer through an ioctl for now. It means we don't need any of the modifications to the dma_heap API in this series, and userspace is free to choose its SoT (efuse, DT, ...) and pass the info back to mesa/GBM somehow (envvar, driconf, ...). The only thing we need to ensure is if lazy protected FW section allocation is going to work, but given the current code purely and simply ignores those sections, and the FW is still able to boot and act properly (at least on v10-v13), I'm pretty confident this is okay, unless there's some trick the MCU can do to detect that the protected section isn't mapped (which I doubt, because the MCU doesn't know it lives behind an MMU).
Of course, once we have a consensus on how to describe this in the DT, we can switch Panthor over to "protected dma_heap selection through DT", and reflect that through the ioctl that exposes whether protected support is ready or not (would be a DEV_QUERY), such that userspace can skip this "PROTM initialization" step.
We're talking about an extra ioctl to set those buffers, and a DEV_QUERY to query the state (ready or not), the size of the global protected buffer (protected FW section) and the size of the protected suspend buffer. The protected suspend buffer would be allocated and passed at group creation time (extra arg passed to the existing GROUP_CREATE ioctl). So, overall, I don't consider it a huge liability in term of maintenance cost.
For the former, expressing the relation in DT seems to be the best, but only if possible :-). Otherwise, a kconfig option (instead of module param) should be easier to work with.
Looking at the userspace implementation, can we also have an panthor ioctl to return the heap to userspace?
Yes, it's something we can add, but again, I'm questioning the usefulness of this: how can we ensure the heap used by panthor to allocate its protected FW buffers is suitable for scanout buffers (buffers that can be used by display drivers). There needs to be a glue leaving in usersland and taking the decision, and I'm not too sure trusting any of the component in the chain (vdec, gpu, display) is the right thing to do.
The heap returned by panthor is only for panfrost/panvk. It says nothing about compatibility with other components on the system.
Okay, if it's used only for internal buffers, I guess that's fine.
On 19/05/2026 09:39, Boris Brezillon wrote:
On Mon, 18 May 2026 17:36:40 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Mon, May 18, 2026 at 12:16 AM Boris Brezillon boris.brezillon@collabora.com wrote:
On Wed, 13 May 2026 12:31:32 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote:
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote: > On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote: >> On Thu, 7 May 2026 11:02:26 +0200 >> Marcin Ślusarz marcin.slusarz@arm.com wrote: >> >>> On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote: >>>>> @@ -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)) { >>>> >>>> Do we really need this strlen() > 0? Won't dma_heap_find() fail is the >>>> name is "" already? >>> >>> If dma_heap_find() will fail, then the whole probe with fail too. >>> This check prevents that. >> >> Yeah, that's also a questionable design choice. I mean, we can >> currently probe and boot the FW even though we never setup the >> protected FW sections, so why should we defer the probe here? Can't we >> just retry the next time a group with the protected bit is created and >> fail if we can find a protected heap? > > The problem we have with the current firmware is that it does a number of setup steps at "boot" > time only. One of the steps is preparing its internal structures for when it enters protected > mode and it stores them in the buffer passed in at firmware loading. We cannot later run the > process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if
the heap is missing, or
- panthor gets a dma-buf from userspace and does the full reset
- userspace also needs to provide a dma-buf for each protected
group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
Indeed, the second option is more ad-hoc, but when you think about it, userspace has to have this knowledge, because it needs to know the dma-heap to use for buffer allocation that cross a device boundary anyway. Think about frames produced by a video decoder, and composited by the GPU into a protected scanout buffer that's passed to the KMS device. Why would the GPU driver be source of truth when it comes to choosing the heap to use to allocate protected buffers for the video decoder or those used for the display?
I don't think the GPU driver is ever the source of truth. If the system integrator wants to specify the source of truth (SoT) from kernel space, they should use the device tree (or module params / config options). If they want to specify the SoT in userspace, then we don't really care how it is done other than providing an ioctl. Panthor is always on the receiving end.
Okay, we're on the same page then.
If we don't want to delay this functionality, but it takes time to converge on SoT, maybe a solution that is not a long-term promise can work? Of the options on the table (dt, module params, kconfig options, ioctls), a kconfig option, potentially marked as experimental, seems like a good candidate.
If Panthor is only a consumer, I actually think it'd be easier to just let userspace pass the protected FW section as an imported buffer through an ioctl for now. It means we don't need any of the modifications to the dma_heap API in this series, and userspace is free to choose its SoT (efuse, DT, ...) and pass the info back to mesa/GBM somehow (envvar, driconf, ...). The only thing we need to ensure is if lazy protected FW section allocation is going to work, but given the current code purely and simply ignores those sections, and the FW is still able to boot and act properly (at least on v10-v13), I'm pretty confident this is okay, unless there's some trick the MCU can do to detect that the protected section isn't mapped (which I doubt, because the MCU doesn't know it lives behind an MMU).
Of course, once we have a consensus on how to describe this in the DT, we can switch Panthor over to "protected dma_heap selection through DT", and reflect that through the ioctl that exposes whether protected support is ready or not (would be a DEV_QUERY), such that userspace can skip this "PROTM initialization" step.
We're talking about an extra ioctl to set those buffers, and a DEV_QUERY to query the state (ready or not), the size of the global protected buffer (protected FW section) and the size of the protected suspend buffer. The protected suspend buffer would be allocated and passed at group creation time (extra arg passed to the existing GROUP_CREATE ioctl). So, overall, I don't consider it a huge liability in term of maintenance cost.
If we can avoid the dma-heap changes, then that would surely help! I can try to implement this in the next version unless someone finds a reason why it is a bad idea.
For the former, expressing the relation in DT seems to be the best, but only if possible :-). Otherwise, a kconfig option (instead of module param) should be easier to work with.
Looking at the userspace implementation, can we also have an panthor ioctl to return the heap to userspace?
Yes, it's something we can add, but again, I'm questioning the usefulness of this: how can we ensure the heap used by panthor to allocate its protected FW buffers is suitable for scanout buffers (buffers that can be used by display drivers). There needs to be a glue leaving in usersland and taking the decision, and I'm not too sure trusting any of the component in the chain (vdec, gpu, display) is the right thing to do.
The heap returned by panthor is only for panfrost/panvk. It says nothing about compatibility with other components on the system.
Okay, if it's used only for internal buffers, I guess that's fine.
-- Ketil
On Tue, May 19, 2026 at 1:49 AM Ketil Johnsen ketil.johnsen@arm.com wrote:
On 19/05/2026 09:39, Boris Brezillon wrote:
On Mon, 18 May 2026 17:36:40 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Mon, May 18, 2026 at 12:16 AM Boris Brezillon boris.brezillon@collabora.com wrote:
On Wed, 13 May 2026 12:31:32 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote:
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote: > On Tue, 12 May 2026 14:47:27 +0100 > Liviu Dudau liviu.dudau@arm.com wrote: > >> On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote: >>> On Thu, 7 May 2026 11:02:26 +0200 >>> Marcin Ślusarz marcin.slusarz@arm.com wrote: >>> >>>> On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote: >>>>>> @@ -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)) { >>>>> >>>>> Do we really need this strlen() > 0? Won't dma_heap_find() fail is the >>>>> name is "" already? >>>> >>>> If dma_heap_find() will fail, then the whole probe with fail too. >>>> This check prevents that. >>> >>> Yeah, that's also a questionable design choice. I mean, we can >>> currently probe and boot the FW even though we never setup the >>> protected FW sections, so why should we defer the probe here? Can't we >>> just retry the next time a group with the protected bit is created and >>> fail if we can find a protected heap? >> >> The problem we have with the current firmware is that it does a number of setup steps at "boot" >> time only. One of the steps is preparing its internal structures for when it enters protected >> mode and it stores them in the buffer passed in at firmware loading. We cannot later run the >> process when we have a group with protected mode set. > > No, but we can force a full/slow reset and have that thing > re-initialized, can't we? I mean, that's basically what we do when a > fast reset fails: we re-initialize all the sections and reset again, at > which point the FW should start from a fresh state, and be able to > properly initialize the protected-related stuff if protected sections > are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if
the heap is missing, or
- panthor gets a dma-buf from userspace and does the full reset
- userspace also needs to provide a dma-buf for each protected
group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
Indeed, the second option is more ad-hoc, but when you think about it, userspace has to have this knowledge, because it needs to know the dma-heap to use for buffer allocation that cross a device boundary anyway. Think about frames produced by a video decoder, and composited by the GPU into a protected scanout buffer that's passed to the KMS device. Why would the GPU driver be source of truth when it comes to choosing the heap to use to allocate protected buffers for the video decoder or those used for the display?
I don't think the GPU driver is ever the source of truth. If the system integrator wants to specify the source of truth (SoT) from kernel space, they should use the device tree (or module params / config options). If they want to specify the SoT in userspace, then we don't really care how it is done other than providing an ioctl. Panthor is always on the receiving end.
Okay, we're on the same page then.
If we don't want to delay this functionality, but it takes time to converge on SoT, maybe a solution that is not a long-term promise can work? Of the options on the table (dt, module params, kconfig options, ioctls), a kconfig option, potentially marked as experimental, seems like a good candidate.
If Panthor is only a consumer, I actually think it'd be easier to just let userspace pass the protected FW section as an imported buffer through an ioctl for now. It means we don't need any of the modifications to the dma_heap API in this series, and userspace is free to choose its SoT (efuse, DT, ...) and pass the info back to mesa/GBM somehow (envvar, driconf, ...). The only thing we need to ensure is if lazy protected FW section allocation is going to work, but given the current code purely and simply ignores those sections, and the FW is still able to boot and act properly (at least on v10-v13), I'm pretty confident this is okay, unless there's some trick the MCU can do to detect that the protected section isn't mapped (which I doubt, because the MCU doesn't know it lives behind an MMU).
I set up MMU to map non-protected memory to the protected section the other day. The FW still booted fine. I didn't get access violation until the FW executed PROT_REGION and panthor requested GLB_PROTM_ENTER in response.
This was on v13, but I also doubt it will become an issue. Can ARM help clarify?
Of course, once we have a consensus on how to describe this in the DT, we can switch Panthor over to "protected dma_heap selection through DT", and reflect that through the ioctl that exposes whether protected support is ready or not (would be a DEV_QUERY), such that userspace can skip this "PROTM initialization" step.
We're talking about an extra ioctl to set those buffers, and a DEV_QUERY to query the state (ready or not), the size of the global protected buffer (protected FW section) and the size of the protected suspend buffer. The protected suspend buffer would be allocated and passed at group creation time (extra arg passed to the existing GROUP_CREATE ioctl). So, overall, I don't consider it a huge liability in term of maintenance cost.
If we can avoid the dma-heap changes, then that would surely help! I can try to implement this in the next version unless someone finds a reason why it is a bad idea.
Yeah, that sounds good to me too.
Will the extra ioctl require root? On a system with true protected memory, the FW cannot write to non-protected memory. It seems ok to allow any client to make the ioctl call. But on systems without true protected memory, it can be problematic.
For the former, expressing the relation in DT seems to be the best, but only if possible :-). Otherwise, a kconfig option (instead of module param) should be easier to work with.
Looking at the userspace implementation, can we also have an panthor ioctl to return the heap to userspace?
Yes, it's something we can add, but again, I'm questioning the usefulness of this: how can we ensure the heap used by panthor to allocate its protected FW buffers is suitable for scanout buffers (buffers that can be used by display drivers). There needs to be a glue leaving in usersland and taking the decision, and I'm not too sure trusting any of the component in the chain (vdec, gpu, display) is the right thing to do.
The heap returned by panthor is only for panfrost/panvk. It says nothing about compatibility with other components on the system.
Okay, if it's used only for internal buffers, I guess that's fine.
-- Ketil
On Tue, 19 May 2026 10:07:02 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 19, 2026 at 1:49 AM Ketil Johnsen ketil.johnsen@arm.com wrote:
On 19/05/2026 09:39, Boris Brezillon wrote:
On Mon, 18 May 2026 17:36:40 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Mon, May 18, 2026 at 12:16 AM Boris Brezillon boris.brezillon@collabora.com wrote:
On Wed, 13 May 2026 12:31:32 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote: > > On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote: >> On Tue, 12 May 2026 14:47:27 +0100 >> Liviu Dudau liviu.dudau@arm.com wrote: >> >>> On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote: >>>> On Thu, 7 May 2026 11:02:26 +0200 >>>> Marcin Ślusarz marcin.slusarz@arm.com wrote: >>>> >>>>> On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote: >>>>>>> @@ -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)) { >>>>>> >>>>>> Do we really need this strlen() > 0? Won't dma_heap_find() fail is the >>>>>> name is "" already? >>>>> >>>>> If dma_heap_find() will fail, then the whole probe with fail too. >>>>> This check prevents that. >>>> >>>> Yeah, that's also a questionable design choice. I mean, we can >>>> currently probe and boot the FW even though we never setup the >>>> protected FW sections, so why should we defer the probe here? Can't we >>>> just retry the next time a group with the protected bit is created and >>>> fail if we can find a protected heap? >>> >>> The problem we have with the current firmware is that it does a number of setup steps at "boot" >>> time only. One of the steps is preparing its internal structures for when it enters protected >>> mode and it stores them in the buffer passed in at firmware loading. We cannot later run the >>> process when we have a group with protected mode set. >> >> No, but we can force a full/slow reset and have that thing >> re-initialized, can't we? I mean, that's basically what we do when a >> fast reset fails: we re-initialize all the sections and reset again, at >> which point the FW should start from a fresh state, and be able to >> properly initialize the protected-related stuff if protected sections >> are populated. Am I missing something? > > Right, we can do that. For some reason I keep associating the reset with the > error handling and not with "normal" operations. I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if
the heap is missing, or
- panthor gets a dma-buf from userspace and does the full reset
- userspace also needs to provide a dma-buf for each protected
group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
Indeed, the second option is more ad-hoc, but when you think about it, userspace has to have this knowledge, because it needs to know the dma-heap to use for buffer allocation that cross a device boundary anyway. Think about frames produced by a video decoder, and composited by the GPU into a protected scanout buffer that's passed to the KMS device. Why would the GPU driver be source of truth when it comes to choosing the heap to use to allocate protected buffers for the video decoder or those used for the display?
I don't think the GPU driver is ever the source of truth. If the system integrator wants to specify the source of truth (SoT) from kernel space, they should use the device tree (or module params / config options). If they want to specify the SoT in userspace, then we don't really care how it is done other than providing an ioctl. Panthor is always on the receiving end.
Okay, we're on the same page then.
If we don't want to delay this functionality, but it takes time to converge on SoT, maybe a solution that is not a long-term promise can work? Of the options on the table (dt, module params, kconfig options, ioctls), a kconfig option, potentially marked as experimental, seems like a good candidate.
If Panthor is only a consumer, I actually think it'd be easier to just let userspace pass the protected FW section as an imported buffer through an ioctl for now. It means we don't need any of the modifications to the dma_heap API in this series, and userspace is free to choose its SoT (efuse, DT, ...) and pass the info back to mesa/GBM somehow (envvar, driconf, ...). The only thing we need to ensure is if lazy protected FW section allocation is going to work, but given the current code purely and simply ignores those sections, and the FW is still able to boot and act properly (at least on v10-v13), I'm pretty confident this is okay, unless there's some trick the MCU can do to detect that the protected section isn't mapped (which I doubt, because the MCU doesn't know it lives behind an MMU).
I set up MMU to map non-protected memory to the protected section the other day. The FW still booted fine. I didn't get access violation until the FW executed PROT_REGION and panthor requested GLB_PROTM_ENTER in response.
Ah, thanks for testing! We still don't have a setup with proper protected heap, but that was on my list of things to test.
This was on v13, but I also doubt it will become an issue. Can ARM help clarify?
Of course, once we have a consensus on how to describe this in the DT, we can switch Panthor over to "protected dma_heap selection through DT", and reflect that through the ioctl that exposes whether protected support is ready or not (would be a DEV_QUERY), such that userspace can skip this "PROTM initialization" step.
We're talking about an extra ioctl to set those buffers, and a DEV_QUERY to query the state (ready or not), the size of the global protected buffer (protected FW section) and the size of the protected suspend buffer. The protected suspend buffer would be allocated and passed at group creation time (extra arg passed to the existing GROUP_CREATE ioctl). So, overall, I don't consider it a huge liability in term of maintenance cost.
If we can avoid the dma-heap changes, then that would surely help! I can try to implement this in the next version unless someone finds a reason why it is a bad idea.
Yeah, that sounds good to me too.
Will the extra ioctl require root?
The PROTM_INIT ioctl will certainly require high privilege CAP_SYS_<something>, dunno yet what that <something> would be though.
On a system with true protected memory, the FW cannot write to non-protected memory. It seems ok to allow any client to make the ioctl call. But on systems without true protected memory, it can be problematic.
Yep, I agree we shouldn't let random users pretend they initialized protected mode if the system as a whole doesn't have proper the proper bit hooked up to set that up.
Hi Boris,
On Mon, May 18, 2026 at 09:16:50AM +0200, Boris Brezillon wrote:
On Wed, 13 May 2026 12:31:32 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote:
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote:
On Thu, 7 May 2026 11:02:26 +0200 Marcin Ślusarz marcin.slusarz@arm.com wrote: > On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote: > > > @@ -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)) { > > > > Do we really need this strlen() > 0? Won't dma_heap_find() fail is the > > name is "" already? > > If dma_heap_find() will fail, then the whole probe with fail too. > This check prevents that.
Yeah, that's also a questionable design choice. I mean, we can currently probe and boot the FW even though we never setup the protected FW sections, so why should we defer the probe here? Can't we just retry the next time a group with the protected bit is created and fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if
the heap is missing, or
- panthor gets a dma-buf from userspace and does the full reset
- userspace also needs to provide a dma-buf for each protected
group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
Indeed, the second option is more ad-hoc, but when you think about it, userspace has to have this knowledge, because it needs to know the dma-heap to use for buffer allocation that cross a device boundary anyway. Think about frames produced by a video decoder, and composited by the GPU into a protected scanout buffer that's passed to the KMS device. Why would the GPU driver be source of truth when it comes to choosing the heap to use to allocate protected buffers for the video decoder or those used for the display?
Just fyi, the trend is to go to devices listing the heaps userspace should allocate from and/or using the heaps internally to allocate their buffers, so that last part is where we're headed, and feels totally reasonable to me.
Maxime
On Tue, 19 May 2026 11:52:13 +0200 Maxime Ripard mripard@kernel.org wrote:
Hi Boris,
On Mon, May 18, 2026 at 09:16:50AM +0200, Boris Brezillon wrote:
On Wed, 13 May 2026 12:31:32 -0700 Chia-I Wu olvaffe@gmail.com wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau liviu.dudau@arm.com wrote:
On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote:
On Tue, 12 May 2026 14:47:27 +0100 Liviu Dudau liviu.dudau@arm.com wrote:
On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote: > On Thu, 7 May 2026 11:02:26 +0200 > Marcin Ślusarz marcin.slusarz@arm.com wrote: > > > On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wrote: > > > > @@ -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)) { > > > > > > Do we really need this strlen() > 0? Won't dma_heap_find() fail is the > > > name is "" already? > > > > If dma_heap_find() will fail, then the whole probe with fail too. > > This check prevents that. > > Yeah, that's also a questionable design choice. I mean, we can > currently probe and boot the FW even though we never setup the > protected FW sections, so why should we defer the probe here? Can't we > just retry the next time a group with the protected bit is created and > fail if we can find a protected heap?
The problem we have with the current firmware is that it does a number of setup steps at "boot" time only. One of the steps is preparing its internal structures for when it enters protected mode and it stores them in the buffer passed in at firmware loading. We cannot later run the process when we have a group with protected mode set.
No, but we can force a full/slow reset and have that thing re-initialized, can't we? I mean, that's basically what we do when a fast reset fails: we re-initialize all the sections and reset again, at which point the FW should start from a fresh state, and be able to properly initialize the protected-related stuff if protected sections are populated. Am I missing something?
Right, we can do that. For some reason I keep associating the reset with the error handling and not with "normal" operations.
I kind of hope we end up with either
- panthor knows the exact heap to use and fails with EPROBE_DEFER if
the heap is missing, or
- panthor gets a dma-buf from userspace and does the full reset
- userspace also needs to provide a dma-buf for each protected
group for the suspend buffer
than something in-between. The latter is more ad-hoc and basically kicks the issue to the userspace.
Indeed, the second option is more ad-hoc, but when you think about it, userspace has to have this knowledge, because it needs to know the dma-heap to use for buffer allocation that cross a device boundary anyway. Think about frames produced by a video decoder, and composited by the GPU into a protected scanout buffer that's passed to the KMS device. Why would the GPU driver be source of truth when it comes to choosing the heap to use to allocate protected buffers for the video decoder or those used for the display?
Just fyi, the trend is to go to devices listing the heaps userspace should allocate from
Devices listing the heaps they are able to import buffers from (with the list being different based on the buffer properties, I guess) is a good thing. This way the central allocator is in a position where it can intersect the devices lists and decide which heap to allocate from based on its buffer sharing knowledge.
and/or using the heaps internally to allocate their buffers,
Yes, that too. For internal buffers (especially the device-wide ones, like the protected FW sections we were discussing), it makes sense to leave that up to the driver.
so that last part is where we're headed, and feels totally reasonable to me.
Just to be clear, my main concern right now is not the long term plan, but how realistic it is to assume we'll have all the DT/dma_heap pieces in place in a reasonable amount of time. Looking at the current state of affairs (based on this patchset), it feels like we're a long way till we can have a robust way of exposing dma_heaps to in-kernel users (refcounting, lifetime issues, describing allowed heaps, ensuring heaps truly provide buffers with the expected properties, ...). I'm certainly not saying these are not valid concerns, but I'd like to have a temporary solution to support protected rendering in the meantime...
Maxime
linaro-mm-sig@lists.linaro.org
-
Boris Brezillon -
Chia-I Wu -
Ketil Johnsen -
Liviu Dudau -
Maxime Ripard