From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 94F9829ACC5; Tue, 19 May 2026 17:29:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779211780; cv=none; b=T5CRD0vnlMOFis4clKDY397TRdhXUiFo17j8CDElzhH49F9A192Gj9rbQju+Z7/Jfe5P/46CI2VHBadYGPCLQNd5BOYufZpZvh5L2DCNLDDUEJWDC2dOT4Z5HUw6Usmb0FZXPjbcqTYXPQ0M52d9ieM5YMN8i8SwuY941yaT22s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779211780; c=relaxed/simple; bh=KD0GOi9c8kuCzvfBSg/EAGaV+3IxmyoTnkrkk0bXqls=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=m2VOu5Z462MmgCKFU9EQi8eBqQ1kbVXx5RAdcL4Smw103e4zImft9Twzz/yah6882DcqxPZhnQ0RhroILPYoS8xhzaTKx4idh52xhGcN5Xx9f9treDq8q5766nK1nFSxeTrxbN8svYTG6IczjFqTRzGnXGVMTQEiR3ZAED/8PwU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=CcXO6GZ+; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="CcXO6GZ+" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1779211776; bh=KD0GOi9c8kuCzvfBSg/EAGaV+3IxmyoTnkrkk0bXqls=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=CcXO6GZ+H0Yjwf2NfeGPjK2m5QdX4F7pB/lCTm0hp16n1jzasLMSmzlO+ssQnG32w tpwSW7KdB2mEhkFn2Wak83FFtLUHUNc466wT01RwJHY6cwswBPeFvsIifMmJUlAcKY Y64WqyXLCsRUsr5lAo3EyKk944rP77AxapGYeIifjJPKxk1+ZvZPcfzyCXiFLoFkH2 XTYDNA4db8cMBjjXOIApizmud9+VBfWGzaOF5zDgprg+mNHh7swRWmtTVEtEsdNCkX NhndFC23jyw/0vQR1Y1/Ol011CnTiq9KfWXFMtFQL7LQrsfpgbgt415c3VYvFI3QQq SkHpyO2sfyIwg== Received: from fedora (unknown [100.64.0.11]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id 8F14B17E125C; Tue, 19 May 2026 19:29:35 +0200 (CEST) Date: Tue, 19 May 2026 19:29:30 +0200 From: Boris Brezillon To: Chia-I Wu Cc: Ketil Johnsen , Liviu Dudau , Marcin =?UTF-8?B?xZpsdXNhcno=?= , David Airlie , Simona Vetter , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Jonathan Corbet , Shuah Khan , Sumit Semwal , Benjamin Gaignard , Brian Starkey , John Stultz , "T.J. Mercier" , Christian =?UTF-8?B?S8O2bmln?= , Steven Price , Daniel Almeida , Alice Ryhl , Matthias Brugger , AngeloGioacchino Del Regno , dri-devel@lists.freedesktop.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, Florent Tomasin , nd@arm.com Subject: Re: [PATCH 4/8] drm/panthor: Add support for protected memory allocation in panthor Message-ID: <20260519192930.3873accb@fedora> In-Reply-To: References: <20260505140516.1372388-1-ketil.johnsen@arm.com> <20260505140516.1372388-5-ketil.johnsen@arm.com> <20260505181523.49a3d85c@fedora> <20260507135356.5428d50d@fedora> <20260512161111.0cb7000e@fedora> <20260518091650.5a7a4f4a@fedora> <20260519093955.448ff899@fedora> <8f0b1750-a853-4895-9672-73a75f6dbd84@arm.com> Organization: Collabora X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-doc@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, 19 May 2026 10:07:02 -0700 Chia-I Wu wrote: > On Tue, May 19, 2026 at 1:49=E2=80=AFAM Ketil Johnsen wrote: > > > > On 19/05/2026 09:39, Boris Brezillon wrote: =20 > > > On Mon, 18 May 2026 17:36:40 -0700 > > > Chia-I Wu wrote: > > > =20 > > >> On Mon, May 18, 2026 at 12:16=E2=80=AFAM Boris Brezillon > > >> wrote: =20 > > >>> > > >>> On Wed, 13 May 2026 12:31:32 -0700 > > >>> Chia-I Wu wrote: > > >>> =20 > > >>>> On Tue, May 12, 2026 at 8:39=E2=80=AFAM Liviu Dudau wrote: =20 > > >>>>> > > >>>>> On Tue, May 12, 2026 at 04:11:11PM +0200, Boris Brezillon wrote: = =20 > > >>>>>> On Tue, 12 May 2026 14:47:27 +0100 > > >>>>>> Liviu Dudau wrote: > > >>>>>> =20 > > >>>>>>> On Thu, May 07, 2026 at 01:53:56PM +0200, Boris Brezillon wrote= : =20 > > >>>>>>>> On Thu, 7 May 2026 11:02:26 +0200 > > >>>>>>>> Marcin =C5=9Alusarz wrote: > > >>>>>>>> =20 > > >>>>>>>>> On Tue, May 05, 2026 at 06:15:23PM +0200, Boris Brezillon wro= te: =20 > > >>>>>>>>>>> @@ -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))= { =20 > > >>>>>>>>>> > > >>>>>>>>>> Do we really need this strlen() > 0? Won't dma_heap_find() f= ail is the > > >>>>>>>>>> name is "" already? =20 > > >>>>>>>>> > > >>>>>>>>> If dma_heap_find() will fail, then the whole probe with fail = too. > > >>>>>>>>> This check prevents that. =20 > > >>>>>>>> > > >>>>>>>> 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 cre= ated and > > >>>>>>>> fail if we can find a protected heap? =20 > > >>>>>>> > > >>>>>>> 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 structure= s for when it enters protected > > >>>>>>> mode and it stores them in the buffer passed in at firmware loa= ding. We cannot later run the > > >>>>>>> process when we have a group with protected mode set. =20 > > >>>>>> > > >>>>>> 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 wh= en a > > >>>>>> fast reset fails: we re-initialize all the sections and reset ag= ain, at > > >>>>>> which point the FW should start from a fresh state, and be able = to > > >>>>>> properly initialize the protected-related stuff if protected sec= tions > > >>>>>> are populated. Am I missing something? =20 > > >>>>> > > >>>>> Right, we can do that. For some reason I keep associating the res= et with the > > >>>>> error handling and not with "normal" operations. =20 > > >>>> I kind of hope we end up with either > > >>>> > > >>>> - panthor knows the exact heap to use and fails with EPROBE_DEFE= R 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. =20 > > >>> > > >>> 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 composi= ted > > >>> 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? =20 > > >> 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. =20 > > > > > > Okay, we're on the same page then. > > > =20 > > >> > > >> 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 option= s, > > >> ioctls), a kconfig option, potentially marked as experimental, seems > > >> like a good candidate. =20 > > > > > > 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 fr= ee > > > 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). =20 > 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. >=20 > This was on v13, but I also doubt it will become an issue. Can ARM help c= larify? >=20 > > > > > > 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 D= T", > > > and reflect that through the ioctl that exposes whether protected > > > support is ready or not (would be a DEV_QUERY), such that userspace c= an > > > 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. =20 > > > > 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. =20 > Yeah, that sounds good to me too. >=20 > Will the extra ioctl require root? The PROTM_INIT ioctl will certainly require high privilege CAP_SYS_, dunno yet what that 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.