[v3] soc/qcom/ubwc: rework UBWC configuration database

[PATCH v3 17/27] drm/msm/mdss: use new helper to set amsbc

Posted by Dmitry Baryshkov 3 weeks, 5 days ago

Use freshly defined helper instead of checking the UBWC version
directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_mdss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index ca59bcdde7b2..04efc29f38cd 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -188,7 +188,7 @@ static void msm_mdss_5x_setup_ubwc(struct msm_mdss *msm_mdss)
 	if (qcom_ubwc_macrotile_mode(data))
 		value |= MDSS_UBWC_STATIC_MACROTILE_MODE;
 
-	if (data->ubwc_enc_version == UBWC_3_0)
+	if (qcom_ubwc_enable_amsbc(data))
 		value |= MDSS_UBWC_STATIC_UBWC_AMSBC;
 
 	value |= MDSS_UBWC_STATIC_UBWC_MIN_ACC_LEN(qcom_ubwc_min_acc_length_64b(data));

-- 
2.47.3

Re: [PATCH v3 17/27] drm/msm/mdss: use new helper to set amsbc

Posted by Konrad Dybcio 3 weeks, 4 days ago

On 3/12/26 2:29 PM, Dmitry Baryshkov wrote:
> Use freshly defined helper instead of checking the UBWC version
> directly.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/gpu/drm/msm/msm_mdss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index ca59bcdde7b2..04efc29f38cd 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -188,7 +188,7 @@ static void msm_mdss_5x_setup_ubwc(struct msm_mdss *msm_mdss)
>  	if (qcom_ubwc_macrotile_mode(data))
>  		value |= MDSS_UBWC_STATIC_MACROTILE_MODE;
>  
> -	if (data->ubwc_enc_version == UBWC_3_0)
> +	if (qcom_ubwc_enable_amsbc(data))
>  		value |= MDSS_UBWC_STATIC_UBWC_AMSBC;

I know it's already there, but fwiw downstream doesn't seem to be
concerned with toggling AMSBC in MDSS (perhaps incorrectly?)

Konrad

Re: [PATCH v3 17/27] drm/msm/mdss: use new helper to set amsbc

Posted by Dmitry Baryshkov 3 weeks, 4 days ago

On Fri, Mar 13, 2026 at 11:19:59AM +0100, Konrad Dybcio wrote:
> On 3/12/26 2:29 PM, Dmitry Baryshkov wrote:
> > Use freshly defined helper instead of checking the UBWC version
> > directly.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> >  drivers/gpu/drm/msm/msm_mdss.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index ca59bcdde7b2..04efc29f38cd 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -188,7 +188,7 @@ static void msm_mdss_5x_setup_ubwc(struct msm_mdss *msm_mdss)
> >  	if (qcom_ubwc_macrotile_mode(data))
> >  		value |= MDSS_UBWC_STATIC_MACROTILE_MODE;
> >  
> > -	if (data->ubwc_enc_version == UBWC_3_0)
> > +	if (qcom_ubwc_enable_amsbc(data))
> >  		value |= MDSS_UBWC_STATIC_UBWC_AMSBC;
> 
> I know it's already there, but fwiw downstream doesn't seem to be
> concerned with toggling AMSBC in MDSS (perhaps incorrectly?)

Downstream enables it only for the cases where encoder and decoder
use exactly UBWC 3.0. This is not completely correct as the bit doesn't
exist for MDSS 6.x+. For 5.x this is equivalent to the code from the
patch: enable AMSBC if targeting UBWC >= 3.0 (because MDSS 5.x don't
support UBWC 4.0 or higher).

> 
> Konrad

-- 
With best wishes
Dmitry

Re: [PATCH v3 17/27] drm/msm/mdss: use new helper to set amsbc

Posted by Konrad Dybcio 3 weeks, 1 day ago

On 3/13/26 3:26 PM, Dmitry Baryshkov wrote:
> On Fri, Mar 13, 2026 at 11:19:59AM +0100, Konrad Dybcio wrote:
>> On 3/12/26 2:29 PM, Dmitry Baryshkov wrote:
>>> Use freshly defined helper instead of checking the UBWC version
>>> directly.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>> ---
>>>  drivers/gpu/drm/msm/msm_mdss.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>> index ca59bcdde7b2..04efc29f38cd 100644
>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>> @@ -188,7 +188,7 @@ static void msm_mdss_5x_setup_ubwc(struct msm_mdss *msm_mdss)
>>>  	if (qcom_ubwc_macrotile_mode(data))
>>>  		value |= MDSS_UBWC_STATIC_MACROTILE_MODE;
>>>  
>>> -	if (data->ubwc_enc_version == UBWC_3_0)
>>> +	if (qcom_ubwc_enable_amsbc(data))
>>>  		value |= MDSS_UBWC_STATIC_UBWC_AMSBC;
>>
>> I know it's already there, but fwiw downstream doesn't seem to be
>> concerned with toggling AMSBC in MDSS (perhaps incorrectly?)
> 
> Downstream enables it only for the cases where encoder and decoder
> use exactly UBWC 3.0. This is not completely correct as the bit doesn't
> exist for MDSS 6.x+. For 5.x this is equivalent to the code from the
> patch: enable AMSBC if targeting UBWC >= 3.0 (because MDSS 5.x don't
> support UBWC 4.0 or higher).

Right, this seems to indeed be the case

While trying to confirm that, I noticed that some lower-end MDSS 5.x
chips (like Talos) report UBWC3 in the register, but the MDSS docs say
they're UBWC2 - any idea?

Konrad

Re: [PATCH v3 17/27] drm/msm/mdss: use new helper to set amsbc

Posted by Dmitry Baryshkov 3 weeks, 1 day ago

On Mon, Mar 16, 2026 at 11:08:00AM +0100, Konrad Dybcio wrote:
> On 3/13/26 3:26 PM, Dmitry Baryshkov wrote:
> > On Fri, Mar 13, 2026 at 11:19:59AM +0100, Konrad Dybcio wrote:
> >> On 3/12/26 2:29 PM, Dmitry Baryshkov wrote:
> >>> Use freshly defined helper instead of checking the UBWC version
> >>> directly.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> >>> ---
> >>>  drivers/gpu/drm/msm/msm_mdss.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> >>> index ca59bcdde7b2..04efc29f38cd 100644
> >>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> >>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> >>> @@ -188,7 +188,7 @@ static void msm_mdss_5x_setup_ubwc(struct msm_mdss *msm_mdss)
> >>>  	if (qcom_ubwc_macrotile_mode(data))
> >>>  		value |= MDSS_UBWC_STATIC_MACROTILE_MODE;
> >>>  
> >>> -	if (data->ubwc_enc_version == UBWC_3_0)
> >>> +	if (qcom_ubwc_enable_amsbc(data))
> >>>  		value |= MDSS_UBWC_STATIC_UBWC_AMSBC;
> >>
> >> I know it's already there, but fwiw downstream doesn't seem to be
> >> concerned with toggling AMSBC in MDSS (perhaps incorrectly?)
> > 
> > Downstream enables it only for the cases where encoder and decoder
> > use exactly UBWC 3.0. This is not completely correct as the bit doesn't
> > exist for MDSS 6.x+. For 5.x this is equivalent to the code from the
> > patch: enable AMSBC if targeting UBWC >= 3.0 (because MDSS 5.x don't
> > support UBWC 4.0 or higher).
> 
> Right, this seems to indeed be the case
> 
> While trying to confirm that, I noticed that some lower-end MDSS 5.x
> chips (like Talos) report UBWC3 in the register, but the MDSS docs say
> they're UBWC2 - any idea?

I don't see a complete pixel format document for Talos, but my guess is
that one of the blocks (GPU, Venus or Camera) doesn't support UBWC 3.0
pixel formats. MDSS is just one piece of picture and unfortunately we
need to use format which is supported by all the blocks.

-- 
With best wishes
Dmitry

Re: [PATCH v3 17/27] drm/msm/mdss: use new helper to set amsbc

Posted by Dmitry Baryshkov 3 weeks, 1 day ago

On Mon, Mar 16, 2026 at 04:04:46PM +0200, Dmitry Baryshkov wrote:
> On Mon, Mar 16, 2026 at 11:08:00AM +0100, Konrad Dybcio wrote:
> > On 3/13/26 3:26 PM, Dmitry Baryshkov wrote:
> > > On Fri, Mar 13, 2026 at 11:19:59AM +0100, Konrad Dybcio wrote:
> > >> On 3/12/26 2:29 PM, Dmitry Baryshkov wrote:
> > >>> Use freshly defined helper instead of checking the UBWC version
> > >>> directly.
> > >>>
> > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > >>> ---
> > >>>  drivers/gpu/drm/msm/msm_mdss.c | 2 +-
> > >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > >>> index ca59bcdde7b2..04efc29f38cd 100644
> > >>> --- a/drivers/gpu/drm/msm/msm_mdss.c
> > >>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > >>> @@ -188,7 +188,7 @@ static void msm_mdss_5x_setup_ubwc(struct msm_mdss *msm_mdss)
> > >>>  	if (qcom_ubwc_macrotile_mode(data))
> > >>>  		value |= MDSS_UBWC_STATIC_MACROTILE_MODE;
> > >>>  
> > >>> -	if (data->ubwc_enc_version == UBWC_3_0)
> > >>> +	if (qcom_ubwc_enable_amsbc(data))
> > >>>  		value |= MDSS_UBWC_STATIC_UBWC_AMSBC;
> > >>
> > >> I know it's already there, but fwiw downstream doesn't seem to be
> > >> concerned with toggling AMSBC in MDSS (perhaps incorrectly?)
> > > 
> > > Downstream enables it only for the cases where encoder and decoder
> > > use exactly UBWC 3.0. This is not completely correct as the bit doesn't
> > > exist for MDSS 6.x+. For 5.x this is equivalent to the code from the
> > > patch: enable AMSBC if targeting UBWC >= 3.0 (because MDSS 5.x don't
> > > support UBWC 4.0 or higher).
> > 
> > Right, this seems to indeed be the case
> > 
> > While trying to confirm that, I noticed that some lower-end MDSS 5.x
> > chips (like Talos) report UBWC3 in the register, but the MDSS docs say
> > they're UBWC2 - any idea?
> 
> I don't see a complete pixel format document for Talos, but my guess is
> that one of the blocks (GPU, Venus or Camera) doesn't support UBWC 3.0
> pixel formats. MDSS is just one piece of picture and unfortunately we
> need to use format which is supported by all the blocks.

And after cross-checking the docs, it is really interesting. It really
looks like for those chips we have used the older core for UBWC, but the
registeres were not updated. One more reason not to trust those and to
use the database instead.

-- 
With best wishes
Dmitry

Re: [PATCH v3 17/27] drm/msm/mdss: use new helper to set amsbc

Posted by Konrad Dybcio 2 weeks, 6 days ago

On 3/16/26 3:20 PM, Dmitry Baryshkov wrote:
> On Mon, Mar 16, 2026 at 04:04:46PM +0200, Dmitry Baryshkov wrote:
>> On Mon, Mar 16, 2026 at 11:08:00AM +0100, Konrad Dybcio wrote:
>>> On 3/13/26 3:26 PM, Dmitry Baryshkov wrote:
>>>> On Fri, Mar 13, 2026 at 11:19:59AM +0100, Konrad Dybcio wrote:
>>>>> On 3/12/26 2:29 PM, Dmitry Baryshkov wrote:
>>>>>> Use freshly defined helper instead of checking the UBWC version
>>>>>> directly.
>>>>>>
>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/msm/msm_mdss.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>> index ca59bcdde7b2..04efc29f38cd 100644
>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>> @@ -188,7 +188,7 @@ static void msm_mdss_5x_setup_ubwc(struct msm_mdss *msm_mdss)
>>>>>>  	if (qcom_ubwc_macrotile_mode(data))
>>>>>>  		value |= MDSS_UBWC_STATIC_MACROTILE_MODE;
>>>>>>  
>>>>>> -	if (data->ubwc_enc_version == UBWC_3_0)
>>>>>> +	if (qcom_ubwc_enable_amsbc(data))
>>>>>>  		value |= MDSS_UBWC_STATIC_UBWC_AMSBC;
>>>>>
>>>>> I know it's already there, but fwiw downstream doesn't seem to be
>>>>> concerned with toggling AMSBC in MDSS (perhaps incorrectly?)
>>>>
>>>> Downstream enables it only for the cases where encoder and decoder
>>>> use exactly UBWC 3.0. This is not completely correct as the bit doesn't
>>>> exist for MDSS 6.x+. For 5.x this is equivalent to the code from the
>>>> patch: enable AMSBC if targeting UBWC >= 3.0 (because MDSS 5.x don't
>>>> support UBWC 4.0 or higher).
>>>
>>> Right, this seems to indeed be the case
>>>
>>> While trying to confirm that, I noticed that some lower-end MDSS 5.x
>>> chips (like Talos) report UBWC3 in the register, but the MDSS docs say
>>> they're UBWC2 - any idea?
>>
>> I don't see a complete pixel format document for Talos, but my guess is
>> that one of the blocks (GPU, Venus or Camera) doesn't support UBWC 3.0
>> pixel formats. MDSS is just one piece of picture and unfortunately we
>> need to use format which is supported by all the blocks.
> 
> And after cross-checking the docs, it is really interesting. It really
> looks like for those chips we have used the older core for UBWC, but the
> registeres were not updated. One more reason not to trust those and to
> use the database instead.

Yeah, downstream seems to have passed:

qcom,sde-ubwc-version = <0x200>;

as well

Konrad