diff mbox series

[RFT,13/14] drm/msm/a6xx: Drop cfg->ubwc_swizzle override

Message ID 20250508-topic-ubwc_central-v1-13-035c4c5cbe50@oss.qualcomm.com
State New
Headers show
Series Add a single source of truth for UBWC configuration data | expand

Commit Message

Konrad Dybcio May 8, 2025, 6:12 p.m. UTC 
From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

On A663 (SA8775P) the value matches exactly.

On A610, the value matches on SM6115, but is different on SM6125. That
turns out not to be a problem, as the bits that differ aren't even
interpreted.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Konrad Dybcio May 9, 2025, 1:17 p.m. UTC | #1 
On 5/8/25 9:26 PM, Connor Abbott wrote:
> On Thu, May 8, 2025 at 2:14 PM Konrad Dybcio <konradybcio@kernel.org> wrote:
>>
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> On A663 (SA8775P) the value matches exactly.
>>
>> On A610, the value matches on SM6115, but is different on SM6125. That
>> turns out not to be a problem, as the bits that differ aren't even
>> interpreted.
> 
> This is definitely going to break userspace, because the kernel
> doesn't expose the UBWC version, instead exposing just the swizzle and
> userspace expects that it sets the right value for older UBWC versions
> before it became configurable (0x7 for UBWC 1.0 and 0x6 for 2.0-3.0).
> It looks like the data for SM6125 is just wrong.

Oh that's sad.. I'll drop this commit

Konrad
Konrad Dybcio May 9, 2025, 1:37 p.m. UTC | #2 
On 5/9/25 3:17 PM, Konrad Dybcio wrote:
> On 5/8/25 9:26 PM, Connor Abbott wrote:
>> On Thu, May 8, 2025 at 2:14 PM Konrad Dybcio <konradybcio@kernel.org> wrote:
>>>
>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>
>>> On A663 (SA8775P) the value matches exactly.
>>>
>>> On A610, the value matches on SM6115, but is different on SM6125. That
>>> turns out not to be a problem, as the bits that differ aren't even
>>> interpreted.
>>
>> This is definitely going to break userspace, because the kernel
>> doesn't expose the UBWC version, instead exposing just the swizzle and
>> userspace expects that it sets the right value for older UBWC versions
>> before it became configurable (0x7 for UBWC 1.0 and 0x6 for 2.0-3.0).
>> It looks like the data for SM6125 is just wrong.
> 
> Oh that's sad.. I'll drop this commit

Wait uh, we have this data in the common config.. why would it break
userspace?

Konrad
Connor Abbott May 9, 2025, 2:48 p.m. UTC | #3 
On Fri, May 9, 2025 at 9:37 AM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 5/9/25 3:17 PM, Konrad Dybcio wrote:
> > On 5/8/25 9:26 PM, Connor Abbott wrote:
> >> On Thu, May 8, 2025 at 2:14 PM Konrad Dybcio <konradybcio@kernel.org> wrote:
> >>>
> >>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>
> >>> On A663 (SA8775P) the value matches exactly.
> >>>
> >>> On A610, the value matches on SM6115, but is different on SM6125. That
> >>> turns out not to be a problem, as the bits that differ aren't even
> >>> interpreted.
> >>
> >> This is definitely going to break userspace, because the kernel
> >> doesn't expose the UBWC version, instead exposing just the swizzle and
> >> userspace expects that it sets the right value for older UBWC versions
> >> before it became configurable (0x7 for UBWC 1.0 and 0x6 for 2.0-3.0).
> >> It looks like the data for SM6125 is just wrong.
> >
> > Oh that's sad.. I'll drop this commit
>
> Wait uh, we have this data in the common config.. why would it break
> userspace?
>
> Konrad

As you said in the commit message SM6125 has ubwc_swizzle = 1 which
seems wrong to me (it should be 7), it just didn't matter before that
it was wrong. You should probably just fix that.

Connor
Konrad Dybcio May 9, 2025, 3:52 p.m. UTC | #4 
On 5/9/25 4:48 PM, Connor Abbott wrote:
> On Fri, May 9, 2025 at 9:37 AM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 5/9/25 3:17 PM, Konrad Dybcio wrote:
>>> On 5/8/25 9:26 PM, Connor Abbott wrote:
>>>> On Thu, May 8, 2025 at 2:14 PM Konrad Dybcio <konradybcio@kernel.org> wrote:
>>>>>
>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>
>>>>> On A663 (SA8775P) the value matches exactly.
>>>>>
>>>>> On A610, the value matches on SM6115, but is different on SM6125. That
>>>>> turns out not to be a problem, as the bits that differ aren't even
>>>>> interpreted.
>>>>
>>>> This is definitely going to break userspace, because the kernel
>>>> doesn't expose the UBWC version, instead exposing just the swizzle and
>>>> userspace expects that it sets the right value for older UBWC versions
>>>> before it became configurable (0x7 for UBWC 1.0 and 0x6 for 2.0-3.0).
>>>> It looks like the data for SM6125 is just wrong.
>>>
>>> Oh that's sad.. I'll drop this commit
>>
>> Wait uh, we have this data in the common config.. why would it break
>> userspace?
>>
>> Konrad
> 
> As you said in the commit message SM6125 has ubwc_swizzle = 1 which
> seems wrong to me (it should be 7), it just didn't matter before that
> it was wrong. You should probably just fix that.

Oh so you meant that the 6125's value would break userspace - gotcha

Konrad
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 28ba0cddd7d222b0a287c7c3a111e123a73b1d39..d96f8cec854a36a77896d39b88c320c29c787edd 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -597,13 +597,10 @@  static int a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
 
 	*cfg = *common_cfg;
 
-	cfg->ubwc_swizzle = 0x6;
 	cfg->highest_bank_bit = 2;
 
-	if (adreno_is_a610(gpu)) {
+	if (adreno_is_a610(gpu))
 		cfg->highest_bank_bit = 0;
-		cfg->ubwc_swizzle = 0x7;
-	}
 
 	if (adreno_is_a618(gpu))
 		cfg->highest_bank_bit = 1;
@@ -630,10 +627,8 @@  static int a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
 		cfg->highest_bank_bit = 3;
 	}
 
-	if (adreno_is_a663(gpu)) {
+	if (adreno_is_a663(gpu))
 		cfg->highest_bank_bit = 0;
-		cfg->ubwc_swizzle = 0x4;
-	}
 
 	if (adreno_is_7c3(gpu))
 		cfg->highest_bank_bit = 1;