Message-ID: <651f1421-b471-485a-aea0-1b1ef92f8331@oss.qualcomm.com>
Date: Wed, 7 May 2025 00:53:27 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
        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 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")

Konrad

Powered by blists - more mailing lists