| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4qvz7jcslwfd72ulrjrbp5fya6fbqkfrph7j4byf6vkw6dcz23@ialiag6o4wiv>
Date: Tue, 29 Apr 2025 15:22:01 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: neil.armstrong@...aro.org, Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64: dts: qcom: sm8650: add iris DT node
On Mon, Apr 28, 2025 at 11:14:18PM +0200, Konrad Dybcio wrote:
> On 4/28/25 12:48 PM, Dmitry Baryshkov wrote:
> > On Mon, 28 Apr 2025 at 11:18, Neil Armstrong <neil.armstrong@...aro.org> wrote:
> >>
> >> Hi,
> >>
> >> On 25/04/2025 23:49, Konrad Dybcio wrote:
> >>> On 4/24/25 6:32 PM, Neil Armstrong wrote:
> >>>> Add DT entries for the sm8650 iris decoder.
> >>>>
> >>>> Since the firmware is required to be signed, only enable
> >>>> on Qualcomm development boards where the firmware is
> >>>> available.
> >>>>
> >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
> >>>> ---
> >>>> Changes in v2:
> >>>> - removed useless firmware-name
> >>>> - Link to v1: https://lore.kernel.org/r/20250418-topic-sm8x50-upstream-iris-8650-dt-v1-1-80a6ae50bf10@linaro.org
> >>>> ---
> >>>
> >>> [...]
> >>>
> >>>> + iris: video-codec@...0000 {
> >>>> + compatible = "qcom,sm8650-iris";
> >>>> + reg = <0 0x0aa00000 0 0xf0000>;
> >>>> +
> >>>> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH 0>;
> >>>> +
> >>>> + power-domains = <&videocc VIDEO_CC_MVS0C_GDSC>,
> >>>> + <&videocc VIDEO_CC_MVS0_GDSC>,
> >>>> + <&rpmhpd RPMHPD_MXC>,
> >>>> + <&rpmhpd RPMHPD_MMCX>;
> >>>> + power-domain-names = "venus",
> >>>> + "vcodec0",
> >>>> + "mxc",
> >>>> + "mmcx";
> >>>> +
> >>>> + operating-points-v2 = <&iris_opp_table>;
> >>>> +
> >>>> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>,
> >>>> + <&videocc VIDEO_CC_MVS0C_CLK>,
> >>>> + <&videocc VIDEO_CC_MVS0_CLK>;
> >>>> + clock-names = "iface",
> >>>> + "core",
> >>>> + "vcodec0_core";
> >>>> +
> >>>> + interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
> >>>> + &config_noc SLAVE_VENUS_CFG QCOM_ICC_TAG_ACTIVE_ONLY>,
> >>>> + <&mmss_noc MASTER_VIDEO QCOM_ICC_TAG_ALWAYS
> >>>> + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>;
> >>>> + interconnect-names = "cpu-cfg",
> >>>> + "video-mem";
> >>>> +
> >>>> + /* FW load region */
> >>>
> >>> I don't think this comment brings value
> >>
> >> Right
> >>
> >>>
> >>>> + memory-region = <&video_mem>;
> >>>> +
> >>>> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>,
> >>>> + <&videocc VIDEO_CC_XO_CLK_ARES>,
> >>>> + <&videocc VIDEO_CC_MVS0C_CLK_ARES>;
> >>>> + reset-names = "bus",
> >>>> + "xo",
> >>>> + "core";
> >>>> +
> >>>> + iommus = <&apps_smmu 0x1940 0>,
> >>>> + <&apps_smmu 0x1947 0>;
> >>>
> >>> I think you may also need 0x1942 0x0 (please also make the second value / SMR
> >>> mask hex)> +
> >>
> >> I don't see 0x1942 in the downstream DT, and which mask should I set ? 0x1 ?
>
> I saw it in docs only, maybe Vikash or Dikshita can chime in whether it's
> necessary. It would have mask 0x0 if so.
>
> >>
> >>>> + dma-coherent;
> >>>> +
> >>>> + /*
> >>>> + * IRIS firmware is signed by vendors, only
> >>>> + * enable in boards where the proper signed firmware
> >>>> + * is available.
> >>>> + */
> >>>
> >>> Here's to another angry media article :(
> >>>
> >>> Please keep Iris enabled.. Vikash reassured me this is not an
> >>> issue until the user attempts to use the decoder [1], and reading
> >>> the code myself I come to the same conclusion (though I haven't given
> >>> it a smoke test - please do that yourself, as you seem to have a better
> >>> set up with these platforms).
> >>>
> >>> If the userland is sane, it should throw an error and defer to CPU
> >>> decoding.
> >>>
> >>> This is >>unlike venus<< which if lacking firmware at probe (i.e. boot)
> >>> would prevent .sync_state
> >>
> >> Well sync with Bjorn who asked me to only enable on board with available firmware ;-)
> >
> > I'd second him here: if there is no firmware, don't enable the device.
> > It's better than the users having cryptic messages in the dmesg,
> > trying to understand why the driver errors out.
>
> I don't agree.. the firmware may appear later at boot (e.g. user installs a
> small rootfs and manually pulls in linux-firmware). Plus without the firmware,
> we can still power on and off the IP block, particularly achieve sync_state
> regardless of it
Yes. But the firmware should appear at a well-known location (rather
than a default one), so we are back to the question of having the
firmware at all (even potentially). I really would rather not having
users / developers trying to put qvss.mbn firmware into the default
location at qcom/vpu. Likewise I don't think we should have users face
cryptic errors if the firmware from linux-firmware is not suitable for
the device. With all that in mind, let's follow our standard approach
and not enable firmware-backed devices by default.
Also, if we define the device as disabled, we can reach the sync_state
too, can we not?
--
With best wishes
Dmitry
Powered by blists - more mailing lists