| Message ID | 20230215-sspp-scaler-version-v1-1-416b1500b85b@somainline.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: Initialize SSPP scaler version (from register read) | expand |
On Thu, 16 Feb 2023 at 01:02, Marijn Suijten <marijn.suijten@somainline.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. > > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.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. 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. > }; > > struct dpu_csc_blk { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > index 4246ab0b3bee..d4e181e1378c 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > @@ -430,7 +430,7 @@ static void _dpu_hw_sspp_setup_scaler3(struct dpu_hw_pipe *ctx, > return; > > dpu_hw_setup_scaler3(&ctx->hw, scaler3_cfg, idx, > - ctx->cap->sblk->scaler_blk.version, > + ctx->version, > sspp->layout.format); > } > > @@ -807,6 +807,12 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum dpu_sspp idx, > hw_pipe->mdp = &catalog->mdp[0]; > hw_pipe->idx = idx; > hw_pipe->cap = cfg; > + > + if (test_bit(DPU_SSPP_SCALER_QSEED3, &cfg->features) || > + test_bit(DPU_SSPP_SCALER_QSEED3LITE, &cfg->features) || > + test_bit(DPU_SSPP_SCALER_QSEED4, &cfg->features)) > + hw_pipe->version = _dpu_hw_sspp_get_scaler3_ver(hw_pipe); > + > _setup_layer_ops(hw_pipe, hw_pipe->cap->features); > > return hw_pipe; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > index 0c95b7e64f6c..eeaf16c6af15 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > @@ -352,6 +352,7 @@ struct dpu_hw_sspp_ops { > * @hw: block hardware details > * @catalog: back pointer to catalog > * @mdp: pointer to associated mdp portion of the catalog > + * @version: qseed block revision > * @idx: pipe index > * @cap: pointer to layer_cfg > * @ops: pointer to operations possible for this pipe > @@ -362,6 +363,8 @@ struct dpu_hw_pipe { > const struct dpu_mdss_cfg *catalog; > const struct dpu_mdp_cfg *mdp; > > + u32 version; > + > /* Pipe */ > enum dpu_sspp idx; > const struct dpu_sspp_cfg *cap; > > -- > 2.39.2 >
On 2023-02-16 04:22:13, Dmitry Baryshkov wrote: > On Thu, 16 Feb 2023 at 01:02, Marijn Suijten > <marijn.suijten@somainline.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@somainline.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. > 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
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@somainline.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@somainline.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
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@somainline.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@somainline.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. 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
On Thu, 16 Feb 2023 at 23:46, Marijn Suijten <marijn.suijten@somainline.org> 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@somainline.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@somainline.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. > > 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.
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; }; struct dpu_csc_blk { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c index 4246ab0b3bee..d4e181e1378c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c @@ -430,7 +430,7 @@ static void _dpu_hw_sspp_setup_scaler3(struct dpu_hw_pipe *ctx, return; dpu_hw_setup_scaler3(&ctx->hw, scaler3_cfg, idx, - ctx->cap->sblk->scaler_blk.version, + ctx->version, sspp->layout.format); } @@ -807,6 +807,12 @@ struct dpu_hw_pipe *dpu_hw_sspp_init(enum dpu_sspp idx, hw_pipe->mdp = &catalog->mdp[0]; hw_pipe->idx = idx; hw_pipe->cap = cfg; + + if (test_bit(DPU_SSPP_SCALER_QSEED3, &cfg->features) || + test_bit(DPU_SSPP_SCALER_QSEED3LITE, &cfg->features) || + test_bit(DPU_SSPP_SCALER_QSEED4, &cfg->features)) + hw_pipe->version = _dpu_hw_sspp_get_scaler3_ver(hw_pipe); + _setup_layer_ops(hw_pipe, hw_pipe->cap->features); return hw_pipe; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h index 0c95b7e64f6c..eeaf16c6af15 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h @@ -352,6 +352,7 @@ struct dpu_hw_sspp_ops { * @hw: block hardware details * @catalog: back pointer to catalog * @mdp: pointer to associated mdp portion of the catalog + * @version: qseed block revision * @idx: pipe index * @cap: pointer to layer_cfg * @ops: pointer to operations possible for this pipe @@ -362,6 +363,8 @@ struct dpu_hw_pipe { const struct dpu_mdss_cfg *catalog; const struct dpu_mdp_cfg *mdp; + u32 version; + /* Pipe */ enum dpu_sspp idx; const struct dpu_sspp_cfg *cap;
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. Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") Signed-off-by: Marijn Suijten <marijn.suijten@somainline.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(-)