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

[Dmitry Baryshkov]

On Thu, 16 Feb 2023 at 23:46, Marijn Suijten
<marijn.suijten@xxxxxxxxxxxxxx> wrote:
>
> On 2023-02-16 18:34:43, Dmitry Baryshkov wrote:
> > 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > >>> ---
> > >>>   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.
>
> You want to drop the descriptive #define's, and replace them with magic
> 0x1002/0x2004/0x3000 and whatever other values we know?

And nothing stops us from adding defines for 0x2004
(SCALER_VERSION_QSEED3LITE) and 0x3000 (SCALER_VERSION_QSEED4). I'm
not sure regarding 0x1002: whether it is used on msm8998 and/or sdm630
too or not.

What I want to remove is the duplication of the information. It was
too easy to miss that vig_mask has version1, while the dpu_caps has
version 2. We are going to replace dpu_caps with scaler_version, but
the problem of having the duplicate still exists. I might have
suggested settling on the dpu_caps.qseed_type or on the bit in
dpu_sspp_cfg.features, but it seems that 0x1002 is not represented
this way. Unless we define something like
DPU_SSPP_SCALER_QSEED3_SDM660.

> That seems
> impossible to port without reading back the register value, which we've
> only done for a handful of SoCs.  I hope I'm misunderstanding you?

Newer vendor dts files provide this value, see the
"qcom,sde-qseed-scalar-version" property.
For older platforms we'd have to read the register. See below

> After all the vendor approach (in a random 4.14 kernel I have open now)
> is to read the register value at runtime but their catalog is also
> dynamic and built at runtime based on version ranges and register reads,
> which sometimes is more sensible.  Ours is const.

In later techpacks (since 5.4) they have switched to the property in the DTS.

>
> > 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).
>
> That (safety guard) is exactly what Abhinav requested against, since the
> kernel (and our catalog) should be trustworthy.  I'll let you two fight
> this out and come to a consensus before sending v2.

I'm fine without a fight. Whoever adds a platform is responsible for
setting a register.

For the reference, as far as I know:
msm8998 - ??
(sdm660 - 0x1002)
sdm845 - 0x1003
sm8150 - ?
sc8180x - ?
sm8250 - 0x3000
sc7180 - 0x3000
sm6115 - 0x3000
qcm2290 - no scaler
sm8350 - 0x3000
sc7280 - 0x3000
sc8280xp - ?, supposedly 0x3001
sm8450 - 0x3001
sm8550 - ?, supposedly 0x3002

>
> > >> 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?
>
> Could be, downstream uses it to steer its catalog logic for example
> (which happens before later reading the version register).
>
> > 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.
>
> Is there no common DRM property to composite at a lower resolution and
> upscale the final displayed image to match a CRTC/encoder?  I wish I
> understood the commit message better :)

Yes, I don't think there is one.

>
> > 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.
>
> Downstream SDE already has this code moved to sde_hw_util as far as I
> can see (and SSPP and DS call into it).  But I fully agree as a
> mostly-oblivious-outsider: it seems like there are a lot of features,
> hardware blocks and optimizations not implemented, things which I still
> have no knowledge/experience/understanding of/about.  Let's first focus
> on making it work _on all relevant SoCs and boards_ though :)

For sure. I pointed to the DS as a reason to have the scaler version
in the sblk rather than in the sspp instance.

-- 
With best wishes
Dmitry