Message-ID: <bc3d6d8a-cd3b-40b6-8f20-24a28d0e85ec@oss.qualcomm.com>
Date: Thu, 8 May 2025 16:49:03 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
        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 5/7/25 2:27 AM, Dmitry Baryshkov wrote:
> 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.

Fine, let's keep it disabled

Konrad

Powered by blists - more mailing lists