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

[Marijn Suijten]

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.

> 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?

<snip>

- Marijn