Message-ID: <20250825111956.5x4dn3uguo4xmtss@hu-mojha-hyd.qualcomm.com>
Date: Mon, 25 Aug 2025 16:49:56 +0530
From: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
To: Stephan Gerhold <stephan.gerhold@...aro.org>
Cc: Vikash Garodia <quic_vgarodia@...cinc.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Dikshita Agarwal <quic_dikshita@...cinc.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-media@...r.kernel.org, linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v2 11/11] media: iris: Enable Secure PAS support with
 IOMMU managed by Linux

On Sat, Aug 23, 2025 at 10:43:52PM +0200, Stephan Gerhold wrote:
> On Fri, Aug 22, 2025 at 10:10:30PM +0530, Mukesh Ojha wrote:
> > On Fri, Aug 22, 2025 at 06:26:19PM +0200, Stephan Gerhold wrote:
> > > On Fri, Aug 22, 2025 at 08:36:11PM +0530, Mukesh Ojha wrote:
> > > > On Fri, Aug 22, 2025 at 10:46:20AM +0200, Stephan Gerhold wrote:
> > > > > On Fri, Aug 22, 2025 at 09:56:49AM +0530, Vikash Garodia wrote:
> > > > > > On 8/20/2025 7:09 PM, Stephan Gerhold wrote:
> > > > > > >>>> +int iris_fw_init(struct iris_core *core)
> > > > > > >>>> +{
> > > > > > >>>> +	struct platform_device_info info;
> > > > > > >>>> +	struct iommu_domain *iommu_dom;
> > > > > > >>>> +	struct platform_device *pdev;
> > > > > > >>>> +	struct device_node *np;
> > > > > > >>>> +	int ret;
> > > > > > >>>> +
> > > > > > >>>> +	np = of_get_child_by_name(core->dev->of_node, "video-firmware");
> > > > > > >>>> +	if (!np)
> > > > > > >>>> +		return 0;
> > > > > > >>> You need a dt-bindings change for this as well. This is documented only
> > > > > > >>> for Venus.
> > > > > > >> You are right, wanted to send device tree and binding support separately.
> > > > > > >> But if required, will add with the series in the next version.
> > > > > > >>
> > > > > > > You can send device tree changes separately, but dt-binding changes
> > > > > > > always need to come before the driver changes.
> > > > > > 
> > > > > > Do you mean to update the examples section[1] with the firmware subnode,
> > > > > > something similar to venus schema[2] ?
> > > > > > 
> > > > > 
> > > > > Sorry, I missed the fact that the "video-firmware" subnode is already
> > > > > documented for iris as well through qcom,venus-common.yaml (which is
> > > > > included for qcom,sm8550-iris). I don't think it's strictly required to
> > > > > add every possibility to the examples of the schema, since we'll also
> > > > > have the actual DTBs later to test this part of the schema.
> > > > > 
> > > > > I would recommend to extend the description of the "video-firmware" node
> > > > > in qcom,venus-common.yaml a bit. You do use the reset functionality of
> > > > > TrustZone, so the description there doesn't fit for your use case.
> > > > > 
> > > > > I think we will also have to figure out how to handle the old
> > > > > "ChromeOS"/"non_tz" use case (that resets Iris directly with the
> > > > > registers) vs the EL2 PAS use case (that resets Iris in TZ but still
> > > > > handles IOMMU from Linux). Simply checking for the presence of the
> > > > > "video-firmware" node is not enough, because that doesn't tell us if the
> > > > > PAS support is present in TZ.
> > > > > 
> > > > > I have been experimenting with a similar patch that copies the "non_tz"
> > > > > code paths from Venus into Iris. We need this to upstream the Iris DT
> > > > > patch for X1E without regressing the community-contributed x1-el2.dtso,
> > > > > which doesn't have functional PAS when running in EL2.
> > > > > 
> > > > > Perhaps we could check for __qcom_scm_is_call_available() with the new
> > > > > QCOM_SCM_PIL_PAS_GET_RSCTABLE to choose between invoking reset via PAS
> > > > > or directly with the registers. I don't have a device with the new
> > > > > firmware to verify if that works.
> > > > 
> > > > You can check QCOM_SCM_PIL_PAS_GET_RSCTABLE with __qcom_scm_is_call_available() 
> > > > but there is a possibility that QCOM_SCM_PIL_PAS_GET_RSCTABLE SMC call will be
> > > > used even for Gunyah. So, I believe, __qcom_scm_is_call_available() and
> > > > video-firmware's iommu property is also important.
> > > > 
> > > 
> > > Yeah, this sounds good.
> > > 
> > > > > 
> > > > > I'll try to send out my patch soon, so you can better see the context.
> > > > 
> > > > Are you saying that you are going to send patch to support IRIS on
> > > > x1-el2.dtso in non-secure way i.e., non-PAS way.
> > > > 
> > > 
> > > The background is the following: I have a pending patch to add iris to
> > > x1e80100.dtsi, but that currently breaks x1-el2.dtso. My original plan
> > > was to disable &iris in x1-el2.dtso (because the PAS way seems to be
> > > just broken), but then I saw that e.g. sc7180-el2.dtso does have working
> > > Venus with the "video-firmware" node. Copy-pasting the "no_tz"(/non-PAS)
> > > code as-is from venus into iris works just fine for x1-el2.dtso, so
> > > disabling &iris in x1-el2.dtso just because the "no_tz" code is
> > > currently missing in iris doesn't sound right.
> > > 
> > > As far as I understand the approach you use in this series does not work
> > > without the TZ changes for older platforms like X1E(?), so adding that
> > > code in iris seems to be the best way to move forward.
> > 
> > Yes, this series has dependency on firmware and will not work for older
> > platforms.
> > 
> > > 
> > > I started working on a patch for this a while ago, it just needs a bit
> > > more cleanup. I'll try to finish it up and post it so we can discuss it
> > > further. I think the IOMMU management in my patch would even work as-is
> > > for you, you would just need to toggle a boolean to use the PAS instead
> > > of accessing the registers directly.
> > 
> > Sounds like a plan.
> > Thanks, please cc me when you send the patches; So, I could test along
> > with my changes and make dependency on it.
> > 
> 
> Krzysztof raised the concern that we shouldn't model the IOMMU specifier
> for the firmware using a "video-firmware" subnode [1], similar to the
> discussion for the "non-pixel" subnode recently [2].
> 
> I mostly finished up the cleanup of my patch, but I don't see any point
> in posting it without an alternative proposal for the dt-bindings. For
> this case, I think a simple property like
> 
> 	firmware-iommus = <&apps_smmu ...>;
> 
> instead of
> 
> 	video-firmware {
> 		iommus = <&apps_smmu ...>;
> 	};
> 
> could perhaps work. (XYZ-iommus isn't standardized at the moment, but I
> think something like XYZ-gpios would make sense in this case. There are
> many other possible approaches as well though.)
> 
> Unfortunately, I won't have enough time in the next weeks to fully
> implement and propose an alternative. I'm assuming you still have
> ongoing work for supporting the "non-pixel" IOMMU, perhaps your new
> approach can be adapted for video-firmware as well?

I believe, non-pixel case a bit different and thats not depends on whether
it is PAS or non-PAS.

However, I liked the idea about introducing something similar to -gpios
for -iommus as could pottentially solves at least this issue. Here, we need
to create a platform device and its domain based on firmware-iommu
property.

So, its required change in device link to put supplier/consumer dependency
and addition of firmware-iommu binding for IRIS and little of changes
over your existing changes.

But I have doubt, whether @Krzysztof would be fine with it ?

> 
> I've pushed my current patch to a branch in case it helps. It's similar
> to yours, but it has no external dependencies except for a fix in iris
> I sent recently ("media: iris: Fix firmware reference leak and unmap
> memory after load" [3]). You could use the non-PAS use case as a basis
> to add the initial implementation in iris independent of this larger
> patch series.

Thanks.

> 
> https://git.codelinaro.org/stephan.gerhold/linux/-/commit/1e068f5864d958ab9e807e6e3772b778cd0edea8.patch
> 
> For the PAS+IOMMU use case, it should be enough to set core->use_tz to
> true, plus any changes needed for the SHM bridge (and maybe resource
> table). The IOMMU management is independent from core->use_tz.
> 
> I'm also happy to add the non-PAS approach later on top of your changes,
> whatever works best for you. :)
> 
> Thanks,
> Stephan
> 
> [1]: https://lore.kernel.org/r/20250823155349.22344-2-krzysztof.kozlowski@linaro.org/
> [2]: https://lore.kernel.org/r/20250627-video_cb-v3-0-51e18c0ffbce@quicinc.com/T/
> [3]: https://lore.kernel.org/r/20250818-iris-firmware-leak-v1-1-1e3f9b8d31ce@linaro.org/

-- 
-Mukesh Ojha

Powered by blists - more mailing lists