Re: [PATCH RFT v2 13/15] soc: qcom: ubwc: Fix SM6125's ubwc_swizzle value

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On 15/05/2025 20:56, Konrad Dybcio wrote:
> On 5/15/25 7:15 PM, Dmitry Baryshkov wrote:
>> On Thu, 15 May 2025 at 19:36, Konrad Dybcio
>> <konrad.dybcio@oss.qualcomm.com> wrote:
>>>
>>> On 5/15/25 6:21 PM, Dmitry Baryshkov wrote:
>>>> On 15/05/2025 19:18, Konrad Dybcio wrote:
>>>>> On 5/14/25 10:33 PM, Dmitry Baryshkov wrote:
>>>>>> On 14/05/2025 23:05, Konrad Dybcio wrote:
>>>>>>> On 5/14/25 9:23 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, May 14, 2025 at 05:10:33PM +0200, Konrad Dybcio wrote:
>>>>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>>>>
>>>>>>>>> The value of 7 (a.k.a. GENMASK(2, 0), a.k.a. disabling levels 1-3 of
>>>>>>>>> swizzling) is what we want on this platform (and others with a UBWC
>>>>>>>>> 1.0 encoder).
>>>>>>>>>
>>>>>>>>> Fix it to make mesa happy (the hardware doesn't care about the 2 higher
>>>>>>>>> bits, as they weren't consumed on this platform).
>>>>>>>>>
>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>>>> ---
>>>>>>>>>     drivers/soc/qcom/ubwc_config.c | 2 +-
>>>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/soc/qcom/ubwc_config.c b/drivers/soc/qcom/ubwc_config.c
>>>>>>>>> index 9caecd071035ccb03f14464e9b7129ba34a7f862..96b94cf01218cce2dacdba22c7573ba6148fcdd1 100644
>>>>>>>>> --- a/drivers/soc/qcom/ubwc_config.c
>>>>>>>>> +++ b/drivers/soc/qcom/ubwc_config.c
>>>>>>>>> @@ -103,7 +103,7 @@ static const struct qcom_ubwc_cfg_data sm6115_data = {
>>>>>>>>>     static const struct qcom_ubwc_cfg_data sm6125_data = {
>>>>>>>>>         .ubwc_enc_version = UBWC_1_0,
>>>>>>>>>         .ubwc_dec_version = UBWC_3_0,
>>>>>>>>> -    .ubwc_swizzle = 1,
>>>>>>>>> +    .ubwc_swizzle = 7,
>>>>>>>>>         .highest_bank_bit = 14,
>>>>>>>>>     };
>>>>>>>>
>>>>>>>> Add a comment and squash into the patch 1.
>>>>>>>
>>>>>>> I don't think that's a good idea, plus this series should be merged
>>>>>>> together anyway
>>>>>>
>>>>>> Well... Granted Rob's comment, I really think the patches should be reordered a bit:
>>>>>>
>>>>>> - MDSS: offset HBB by 13 (patch 2)
>>>>>> - switch drm/msm/mdss and display to common DB (patches 1+3 squashed)
>>>>>> - get a handle (patch 4)
>>>>>> - resolve / simplify (patches 5-10, not squashed)
>>>>>> - fix sm6125 (patch 13)
>>>>>> - WARN_ON (swizzle != swizzle) or (HBB != HBB)
>>>>>> - switch to common R/O config, keeping WARN_ON for the calculated values (with the hope to drop them after testing)
>>>>>
>>>>> Does this bring any functional benefit? This series is unfun to remix
>>>>
>>>> I know the pain.
>>>>
>>>> The functional benefit is to have the WARN_ON and side-by-side comparison of common_ubwc_config vs computed ubwc_config for HBB and swizzle.
>>>
>>> HBB I agree, since we'll be outsourcing it to yet another driver, swizzle
>>> should be good enough (tm) - I scanned through the values in the driver
>>> and couldn't find anything wrong just by eye
>>
>> Well. What is the ubwc_swizzle value used for SDM845? I think it
>> should be 6 according to a6xx_gpu.c and 0 according to msm_mdss.c.
>> Yes, higher bits are most likely ignored. Still, we'd better have one
>> correct value.
> 
> Ehh, so laziness bites after all..
> 
> Unfortunately it seems like I don't have a good answer for you
> - although I can infer a technically valid config for these
> at the very least:
> 
> msm8937
> msm8998
> sc8180x
> sdm670
> sdm845
> sm6150
> sm7150
> sm8150

WARN_ON would be a good thing in the end

> 
> with the ubwc1.0 platforms receiving all 3 levels and ubwc 2.0/
> 3.0 enabling 2/3
> 
> this however I'm not sure matches what downstream does..

Well, let's match previous GPU config, so 6 whenever we don't define 
anything special.

> 
> Konrad


-- 
With best wishes
Dmitry