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

[Marijn Suijten]

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

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

> >> 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 :)

> 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 :)

- Marijn