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

[Dmitry Baryshkov]

On 26/02/2023 04:10, Abhinav Kumar wrote:
> 
> 
> 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 at 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 at 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

And 0x2004 are also programmed slightly differently, etc.



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

Please excuse me if I was not fully obvious here. I meant using QSEED3 
and scaler rev.

> 
> 
>>>
>>>>>
>>>>> 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 at quicinc.com>
>>>>
>>>>
>>

-- 
With best wishes
Dmitry