linux-kernel - Re: [PATCH 1/3] drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <305a468a-d792-4f51-5a7a-bf38b9c685eb@linaro.org>
Date:   Thu, 16 Feb 2023 18:34:43 +0200
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Marijn Suijten <marijn.suijten@...ainline.org>
Cc:     Rob Clark <robdclark@...il.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Archit Taneja <architt@...eaurora.org>,
        Chandan Uddaraju <chandanu@...eaurora.org>,
        Sravanthi Kollukuduru <skolluku@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Martin Botka <martin.botka@...ainline.org>,
        Jami Kettunen <jami.kettunen@...ainline.org>,
        phone-devel@...r.kernel.org
Subject: Re: [PATCH 1/3] drm/msm/dpu: Read previously-uninitialized SSPP
 scaler version from hw

On 16/02/2023 10:31, Marijn Suijten wrote:
> On 2023-02-16 04:22:13, Dmitry Baryshkov wrote:
>> On Thu, 16 Feb 2023 at 01:02, Marijn Suijten
>> <marijn.suijten@...ainline.org> wrote:
>>>
>>> DPU's catalog never assigned dpu_scaler_blk::version leading to
>>> initialization code in dpu_hw_setup_scaler3 to wander the wrong
>>> codepaths.  Instead of hardcoding the correct QSEED algorithm version,
>>> read it back from a hardware register.
>>>
>>> Note that this register is only available starting with QSEED3, where
>>> 0x1002 corresponds to QSEED3, 0x2004 to QSEED3LITE and 0x3000 to QSEED4.
>>
>> This is not purely accurate. 0x1003 (sdm845) also corresponds to QSEED3.
>> I'd say instead that there are several variations of QSEED3 scalers,
>> where starting from 0x2004 it is called QSEED3LITE and starting from
>> 0x3000 it is called QSEED4.
> 
> Good catch, I'll update that.
> 
>>> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
>>> Signed-off-by: Marijn Suijten <marijn.suijten@...ainline.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c    | 8 +++++++-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h    | 3 +++
>>>   3 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> index ddab9caebb18..96ce1766f4a1 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>> @@ -324,11 +324,9 @@ struct dpu_src_blk {
>>>   /**
>>>    * struct dpu_scaler_blk: Scaler information
>>>    * @info:   HW register and features supported by this sub-blk
>>> - * @version: qseed block revision
>>>    */
>>>   struct dpu_scaler_blk {
>>>          DPU_HW_SUBBLK_INFO;
>>> -       u32 version;
>>
>> No. Please keep the version in the scaler subblk.  It is a version of
>> the QSEED (scaler block), not the SSPP's version.
> 
> You are right that the new variable in the parent (SSPP) block is
> nondescriptive and should have been named scaler_version.
> 
> However.
> 
> dpu_scaler_blk is only used as a const static struct in the catalog,
> meaning we cannot (should not!) store a runtime-read register value
> here.  Instead I followed your IRC suggestion to read the register in
> dpu_hw_sspp_init, but my original implementation called
> dpu_hw_get_scaler3_ver in _dpu_hw_sspp_setup_scaler3 where we already
> have access to the subblk_offset, allowing us to delete
> _dpu_hw_sspp_get_scaler3_ver.  Would you rather have that?  We don't
> need the register value anywhere else.

After giving it another thought, let's follow the vendor's approach and 
store the predefined scaler_version in hw catalog (in dpu_scaler_blk, as 
it currently is). This way we can still drop all QSEED3/3LITE/4 
crazyness, while keeping the data sane.

Then _dpu_hw_sspp_get_scaler3_ver() can also be dropped (or you can use 
it as a safety guard while doing dpu_hw_sspp init).

> 
>> There is a block called DS (destination scaler), which can be used to
>> scale the resulting image after the LM. This block also uses the
>> QSEED3(,LITE,4) scaler block.
> 
> Is this already supported in mainline, and is it the reason for
> previously having qseed_type globally available?  Is my understanding
> correct that this scaler subblk in the SSPP is merely an interface to
> it, allowing the same hardware to be used from the SSPP for intputs and
> after the LM for outputs?

No, I think qseed_type is a leftover from having the same thing 
implemented in three different ways. Maybe because of NIH syndrome?

DS is not supported, it was removed in the commit 
b033def8741aab3fb58e4bf6c1d5cd73b3beb357. I do not have a clear usecase 
for this block and of course we don't have uABI for it.

It would still be nice to keep it in the picture though. It was the main 
reason for moving scaler code from dpu_hw_sspp to dpu_hw_util.

> 
> <snip>
> 
> - Marijn

-- 
With best wishes
Dmitry