On 19/05/2026 09:39, Boris Brezillon wrote:
On Mon, 18 May 2026 17:36:40 -0700
Chia-I Wu <[email protected]> wrote:
On Mon, May 18, 2026 at 12:16 AM Boris Brezillon
<[email protected]> wrote:
On Wed, 13 May 2026 12:31:32 -0700
Chia-I Wu <[email protected]> wrote:
On Tue, May 12, 2026 at 8:39 AM Liviu Dudau <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
