Message-ID: <32ktmnjhrfrvpi5glipex7qfrcxa5hyl26em3f6xos5l3n2zl4@b7rmzx6wpsj2>
Date: Wed, 7 May 2025 03:27:53 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: Bjorn Andersson <andersson@...nel.org>, neil.armstrong@...aro.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 Wed, May 07, 2025 at 12:53:27AM +0200, Konrad Dybcio wrote:
> On 5/6/25 10:23 PM, Bjorn Andersson wrote:
> > 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
> >>
> > 
> > Not "available during boot", but rather "available for a particular
> > board".
> 
> I'd argue that if a device is in the hands of users, there already exists
> some acceptable set of fw binaries.. but most developers aren't in the
> position to upload them into l-f.. And quite frankly I'm not the biggest
> fan of having a gigabyte of 99%-the-same files with a dozen lines changed
> and a different signature prepended to them either..
> 
> > We generally avoid enabling device_nodes that depend on vendor-signed
> > firmware until someone has tested the device on such board and specified
> > the proper path to the vendor-specific firmware.
> > 
> > Are you suggesting that we should leave this enabled on all boards for
> > some reason (perhaps to ensure that resources are adequately managed)?
> 
> Yes, for that reason indeed.
> 
> We don't generally need to load firmware to turn something *off*. And
> most IP blocks don't **actually** need to be presented with firmware at
> probe time (I can only think of external ICs like no-storage touch
> controllers that need the fw uploaded on each powerup). 
> 
> Telling the user "hey, this is supported but the firmware file can't
> be loaded on your device" may also be better sounding than "won't work
> on your machine" (with the quiet part being: "because someone hasn't
> copied 5 lines of DTS")

Then we need to make sure _not_ to make a default path useable, so that
the users know that there is no proper firmware rather than facing the
cryptic error of "firmware something -error".

But... I'd rather prefer to keep firmware-backed nodes disabled exactly
for the reason of "making someone copy 5 lines of DTS", which usually
means that somebody has thought about how to get and where to put the
binary.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists