Re: [PATCH 1/3] drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw
From: Dmitry Baryshkov
Date: Thu Feb 16 2023 - 11:34:57 EST
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.
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