diff mbox series

[v2,06/50] drm/msm/dpu: correct sm8550 scaler

Message ID 20230211231259.1308718-7-dmitry.baryshkov@linaro.org
State Accepted
Commit d113d267c3bfa07c0c8e9f68068a18fa18970c9c
Headers show
Series drm/msm/dpu: rework HW catalog | expand

Commit Message

Dmitry Baryshkov Feb. 11, 2023, 11:12 p.m. UTC 
QSEED4 is a newer variant of QSEED3LITE, which should be used on
sm8550. Fix the DPU caps structure and used feature masks.

Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Neil Armstrong Feb. 13, 2023, 5:36 p.m. UTC | #1 
On 13/02/2023 12:16, Dmitry Baryshkov wrote:
> On 13/02/2023 12:41, Neil Armstrong wrote:
>> On 12/02/2023 00:12, Dmitry Baryshkov wrote:
>>> QSEED4 is a newer variant of QSEED3LITE, which should be used on
>>> sm8550. Fix the DPU caps structure and used feature masks.
>>
>> I found nowhere SM8550 uses Qseed4, on downstream DT, it's written:
>>          qcom,sde-qseed-sw-lib-rev = "qseedv3lite";
>>          qcom,sde-qseed-scalar-version = <0x3002>;
> 
> And then the techpack tells us starting from 0x3000 the v3lite is v4:
> 
> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L59
> 
> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L102

OK then:

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

> 
>>
>> Neil
>>>
>>> Fixes: efcd0107727c ("drm/msm/dpu: add support for SM8550")
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>> index 192fff9238f9..c4e45c472685 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>> @@ -458,7 +458,7 @@ static const struct dpu_caps sm8450_dpu_caps = {
>>>   static const struct dpu_caps sm8550_dpu_caps = {
>>>       .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
>>>       .max_mixer_blendstages = 0xb,
>>> -    .qseed_type = DPU_SSPP_SCALER_QSEED3LITE,
>>> +    .qseed_type = DPU_SSPP_SCALER_QSEED4,
>>>       .smart_dma_rev = DPU_SSPP_SMART_DMA_V2, /* TODO: v2.5 */
>>>       .ubwc_version = DPU_HW_UBWC_VER_40,
>>>       .has_src_split = true,
>>> @@ -1301,13 +1301,13 @@ static const struct dpu_sspp_cfg sm8450_sspp[] = {
>>>   };
>>>   static const struct dpu_sspp_sub_blks sm8550_vig_sblk_0 =
>>> -                _VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED3LITE);
>>> +                _VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED4);
>>>   static const struct dpu_sspp_sub_blks sm8550_vig_sblk_1 =
>>> -                _VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED3LITE);
>>> +                _VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED4);
>>>   static const struct dpu_sspp_sub_blks sm8550_vig_sblk_2 =
>>> -                _VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED3LITE);
>>> +                _VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED4);
>>>   static const struct dpu_sspp_sub_blks sm8550_vig_sblk_3 =
>>> -                _VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED3LITE);
>>> +                _VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED4);
>>>   static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK("12", 5);
>>>   static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK("13", 6);
>>
>
Dmitry Baryshkov Feb. 26, 2023, 12:06 a.m. UTC | #2 
On 26/02/2023 01:27, Abhinav Kumar wrote:
> Hi Dmitry
> 
> On 2/25/2023 3:06 PM, Dmitry Baryshkov wrote:
>> On 24/02/2023 22:51, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/13/2023 9:36 AM, neil.armstrong@linaro.org wrote:
>>>> On 13/02/2023 12:16, Dmitry Baryshkov wrote:
>>>>> On 13/02/2023 12:41, Neil Armstrong wrote:
>>>>>> On 12/02/2023 00:12, Dmitry Baryshkov wrote:
>>>>>>> QSEED4 is a newer variant of QSEED3LITE, which should be used on
>>>>>>> sm8550. Fix the DPU caps structure and used feature masks.
>>>>>>
>>>>>> I found nowhere SM8550 uses Qseed4, on downstream DT, it's written:
>>>>>>          qcom,sde-qseed-sw-lib-rev = "qseedv3lite";
>>>>>>          qcom,sde-qseed-scalar-version = <0x3002>;
>>>>>
>>>>> And then the techpack tells us starting from 0x3000 the v3lite is v4:
>>>>>
>>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L59
>>>>>
>>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L102
>>>>
>>>> OK then:
>>>>
>>>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>
>>>>>
>>>
>>> This little bit of confusion is because with downstream, the qseed is 
>>> a separate usermode library having its own revision. So the SW lib 
>>> version in this case is not exactly correlating with the scalar HW 
>>> revision.
>>
>> Can you possibly spend some more words here? I see that sde_hw_utils.c 
>> programs scalers slightly different depending on the version of the 
>> scaler. At some point the SDE driver was reading the register to 
>> determine the revision. Then it switched to the revision specified in 
>> the DTS (which, as far as I understand, corresponds to the HW register 
>> contents).
>>
>> So, where does SW revision come into the play? (and which library are 
>> we talking about?). Is the 'v3lite' an SW revision? Or is the 0x3002 
>> an SW revision?
>>
> 
> qcom,sde-qseed-sw-lib-rev is the SW library revision for libscale.
> 
> This is a proprietary library used to calculate the LUTs for the qseed 
> block. Its not used in the upstream version of the driver.
> 
> For upstream driver, the driver uses default settings for the LUTs which 
> work for most of the common use-cases we see.

Ack, thanks for the explanation. If default settings work, that's good. 
When you wrote about the proprietary lib, I started wondering if we 
loose anything (like worse quality of the images, etc).

> 
> You can refer the below property names, there are programmed by the lib 
> for the downstream driver.
> 
> 3733         msm_property_install_range(
> 3734                 &psde->property_info, "scaler_v2",
> 3735                 0x0, 0, ~0, 0, PLANE_PROP_SCALER_V2);
> 3736         msm_property_install_blob(&psde->property_info,
> 3737                 "lut_sep", 0,
> 3738                 PLANE_PROP_SCALER_LUT_SEP);
> 
> No, 0x3002 is the HW revision of the qseed and thats why this change is 
> correct because the SW library name/rev doesnt exactly match the qseed 
> HW revision as its possible that even qseed3lite library can support the 
> QSEED4 HW.
> 
> So we should be going off qcom,sde-qseed-scalar-version and not 
> qcom,sde-qseed-sw-lib-rev.

Thanks!

So, we should further drop the v3lite/v4 from the scaler name/subblock 
and use qseed3 everywhere. Correct?

> 
>>>
>>> Since upstream DPU only cares about the HW revision of the scaler, we 
>>> should be going off the qcom,sde-qseed-scalar-version.
>>>
>>> This change LGTM,
>>>
>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>>
Abhinav Kumar Feb. 26, 2023, 2:10 a.m. UTC | #3 
On 2/25/2023 4:06 PM, Dmitry Baryshkov wrote:
> On 26/02/2023 01:27, Abhinav Kumar wrote:
>> Hi Dmitry
>>
>> On 2/25/2023 3:06 PM, Dmitry Baryshkov wrote:
>>> On 24/02/2023 22:51, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 2/13/2023 9:36 AM, neil.armstrong@linaro.org wrote:
>>>>> On 13/02/2023 12:16, Dmitry Baryshkov wrote:
>>>>>> On 13/02/2023 12:41, Neil Armstrong wrote:
>>>>>>> On 12/02/2023 00:12, Dmitry Baryshkov wrote:
>>>>>>>> QSEED4 is a newer variant of QSEED3LITE, which should be used on
>>>>>>>> sm8550. Fix the DPU caps structure and used feature masks.
>>>>>>>
>>>>>>> I found nowhere SM8550 uses Qseed4, on downstream DT, it's written:
>>>>>>>          qcom,sde-qseed-sw-lib-rev = "qseedv3lite";
>>>>>>>          qcom,sde-qseed-scalar-version = <0x3002>;
>>>>>>
>>>>>> And then the techpack tells us starting from 0x3000 the v3lite is v4:
>>>>>>
>>>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L59 
>>>>>>
>>>>>>
>>>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/display-kernel.lnx.5.10.r8-rel/msm/sde/sde_hw_util.c#L102 
>>>>>>
>>>>>
>>>>> OK then:
>>>>>
>>>>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>
>>>>>>
>>>>
>>>> This little bit of confusion is because with downstream, the qseed 
>>>> is a separate usermode library having its own revision. So the SW 
>>>> lib version in this case is not exactly correlating with the scalar 
>>>> HW revision.
>>>
>>> Can you possibly spend some more words here? I see that 
>>> sde_hw_utils.c programs scalers slightly different depending on the 
>>> version of the scaler. At some point the SDE driver was reading the 
>>> register to determine the revision. Then it switched to the revision 
>>> specified in the DTS (which, as far as I understand, corresponds to 
>>> the HW register contents).
>>>
>>> So, where does SW revision come into the play? (and which library are 
>>> we talking about?). Is the 'v3lite' an SW revision? Or is the 0x3002 
>>> an SW revision?
>>>
>>
>> qcom,sde-qseed-sw-lib-rev is the SW library revision for libscale.
>>
>> This is a proprietary library used to calculate the LUTs for the qseed 
>> block. Its not used in the upstream version of the driver.
>>
>> For upstream driver, the driver uses default settings for the LUTs 
>> which work for most of the common use-cases we see.
> 
> Ack, thanks for the explanation. If default settings work, that's good. 
> When you wrote about the proprietary lib, I started wondering if we 
> loose anything (like worse quality of the images, etc).
> 
>>
>> You can refer the below property names, there are programmed by the 
>> lib for the downstream driver.
>>
>> 3733         msm_property_install_range(
>> 3734                 &psde->property_info, "scaler_v2",
>> 3735                 0x0, 0, ~0, 0, PLANE_PROP_SCALER_V2);
>> 3736         msm_property_install_blob(&psde->property_info,
>> 3737                 "lut_sep", 0,
>> 3738                 PLANE_PROP_SCALER_LUT_SEP);
>>
>> No, 0x3002 is the HW revision of the qseed and thats why this change 
>> is correct because the SW library name/rev doesnt exactly match the 
>> qseed HW revision as its possible that even qseed3lite library can 
>> support the QSEED4 HW.
>>
>> So we should be going off qcom,sde-qseed-scalar-version and not 
>> qcom,sde-qseed-sw-lib-rev.
> 
> Thanks!
> 
> So, we should further drop the v3lite/v4 from the scaler name/subblock 
> and use qseed3 everywhere. Correct?
> 

No, even that wont be correct because as you pointed out anything we 
need to handle < QSEED4 case differently from others over here

537 		if (pdpu->pipe_hw->cap->features &
538 			BIT(DPU_SSPP_SCALER_QSEED4)) {
539 			scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
540 			scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
541 		} else {
542 			scale_cfg->preload_x[i] = DPU_QSEED3_DEFAULT_PRELOAD_H;
543 			scale_cfg->preload_y[i] = DPU_QSEED3_DEFAULT_PRELOAD_V;
544 		}


If we want to clean this up more accurately, we should add qseed_rev in 
the dpu caps or rename qseed_type to that which will hold the 0x3xxx 
value and then write a small util which would set the the bit correctly 
based on the qseed rev (0x3xxxx value).


>>
>>>>
>>>> Since upstream DPU only cares about the HW revision of the scaler, 
>>>> we should be going off the qcom,sde-qseed-scalar-version.
>>>>
>>>> This change LGTM,
>>>>
>>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>
>>>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 192fff9238f9..c4e45c472685 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -458,7 +458,7 @@  static const struct dpu_caps sm8450_dpu_caps = {
 static const struct dpu_caps sm8550_dpu_caps = {
 	.max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
 	.max_mixer_blendstages = 0xb,
-	.qseed_type = DPU_SSPP_SCALER_QSEED3LITE,
+	.qseed_type = DPU_SSPP_SCALER_QSEED4,
 	.smart_dma_rev = DPU_SSPP_SMART_DMA_V2, /* TODO: v2.5 */
 	.ubwc_version = DPU_HW_UBWC_VER_40,
 	.has_src_split = true,
@@ -1301,13 +1301,13 @@  static const struct dpu_sspp_cfg sm8450_sspp[] = {
 };
 
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_0 =
-				_VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED3LITE);
+				_VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_1 =
-				_VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED3LITE);
+				_VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_2 =
-				_VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED3LITE);
+				_VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_3 =
-				_VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED3LITE);
+				_VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK("12", 5);
 static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK("13", 6);