Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor

[Boris Brezillon <boris.brezillon@collabora.com>]

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.