| Message ID | 20250409-topic-smem_dramc-v1-3-94d505cd5593@oss.qualcomm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Retrieve information about DDR from SMEM | expand |
On Wed, Apr 9, 2025 at 10:48 AM Konrad Dybcio <konradybcio@kernel.org> wrote: > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > The Highest Bank address Bit value can change based on memory type used. > > Attempt to retrieve it dynamically, and fall back to a reasonable > default (the one used prior to this change) on error. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > --- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..0cc397378c99db35315209d0265ad9223e8b55c7 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -13,6 +13,7 @@ > #include <linux/firmware/qcom/qcom_scm.h> > #include <linux/pm_domain.h> > #include <linux/soc/qcom/llcc-qcom.h> > +#include <linux/soc/qcom/smem.h> > > #define GPU_PAS_ID 13 > > @@ -669,17 +670,22 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > { > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > + u32 hbb = qcom_smem_dram_get_hbb(); > + u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; > + u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); > + u32 hbb_hi, hbb_lo; > + > /* > * We subtract 13 from the highest bank bit (13 is the minimum value > * allowed by hw) and write the lowest two bits of the remaining value > * as hbb_lo and the one above it as hbb_hi to the hardware. > */ > - BUG_ON(adreno_gpu->ubwc_config.highest_bank_bit < 13); > - u32 hbb = adreno_gpu->ubwc_config.highest_bank_bit - 13; > - u32 hbb_hi = hbb >> 2; > - u32 hbb_lo = hbb & 3; > - u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; > - u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); > + if (hbb < 0) > + hbb = adreno_gpu->ubwc_config.highest_bank_bit; No. The value we expose to userspace must match what we program. You'll break VK_EXT_host_image_copy otherwise. Connor > + hbb -= 13; > + BUG_ON(hbb < 0); > + hbb_hi = hbb >> 2; > + hbb_lo = hbb & 3; > > gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL, > level2_swizzling_dis << 12 | > @@ -2467,6 +2473,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev) > bool is_a7xx; > int ret; > > + /* We need data from SMEM to retrieve HBB in set_ubwc_config() */ > + if (!qcom_smem_is_available()) > + return ERR_PTR(-EPROBE_DEFER); > + > a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL); > if (!a6xx_gpu) > return ERR_PTR(-ENOMEM); > > -- > 2.49.0 >
On 4/9/25 5:12 PM, Connor Abbott wrote: > On Wed, Apr 9, 2025 at 10:48 AM Konrad Dybcio <konradybcio@kernel.org> wrote: >> >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >> >> The Highest Bank address Bit value can change based on memory type used. >> >> Attempt to retrieve it dynamically, and fall back to a reasonable >> default (the one used prior to this change) on error. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >> --- >> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 22 ++++++++++++++++------ >> 1 file changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..0cc397378c99db35315209d0265ad9223e8b55c7 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> @@ -13,6 +13,7 @@ >> #include <linux/firmware/qcom/qcom_scm.h> >> #include <linux/pm_domain.h> >> #include <linux/soc/qcom/llcc-qcom.h> >> +#include <linux/soc/qcom/smem.h> >> >> #define GPU_PAS_ID 13 >> >> @@ -669,17 +670,22 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) >> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) >> { >> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); >> + u32 hbb = qcom_smem_dram_get_hbb(); >> + u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; >> + u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); >> + u32 hbb_hi, hbb_lo; >> + >> /* >> * We subtract 13 from the highest bank bit (13 is the minimum value >> * allowed by hw) and write the lowest two bits of the remaining value >> * as hbb_lo and the one above it as hbb_hi to the hardware. >> */ >> - BUG_ON(adreno_gpu->ubwc_config.highest_bank_bit < 13); >> - u32 hbb = adreno_gpu->ubwc_config.highest_bank_bit - 13; >> - u32 hbb_hi = hbb >> 2; >> - u32 hbb_lo = hbb & 3; >> - u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; >> - u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); >> + if (hbb < 0) >> + hbb = adreno_gpu->ubwc_config.highest_bank_bit; > > No. The value we expose to userspace must match what we program. > You'll break VK_EXT_host_image_copy otherwise. I didn't know that was exposed to userspace. The value must be altered either way - ultimately, the hardware must receive the correct information. ubwc_config doesn't seem to be const, so I can edit it there if you like it better. Konrad
On Wed, Apr 9, 2025 at 11:22 AM Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> wrote: > > On 4/9/25 5:12 PM, Connor Abbott wrote: > > On Wed, Apr 9, 2025 at 10:48 AM Konrad Dybcio <konradybcio@kernel.org> wrote: > >> > >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > >> > >> The Highest Bank address Bit value can change based on memory type used. > >> > >> Attempt to retrieve it dynamically, and fall back to a reasonable > >> default (the one used prior to this change) on error. > >> > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > >> --- > >> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 22 ++++++++++++++++------ > >> 1 file changed, 16 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..0cc397378c99db35315209d0265ad9223e8b55c7 100644 > >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >> @@ -13,6 +13,7 @@ > >> #include <linux/firmware/qcom/qcom_scm.h> > >> #include <linux/pm_domain.h> > >> #include <linux/soc/qcom/llcc-qcom.h> > >> +#include <linux/soc/qcom/smem.h> > >> > >> #define GPU_PAS_ID 13 > >> > >> @@ -669,17 +670,22 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > >> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > >> { > >> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > >> + u32 hbb = qcom_smem_dram_get_hbb(); > >> + u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; > >> + u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); > >> + u32 hbb_hi, hbb_lo; > >> + > >> /* > >> * We subtract 13 from the highest bank bit (13 is the minimum value > >> * allowed by hw) and write the lowest two bits of the remaining value > >> * as hbb_lo and the one above it as hbb_hi to the hardware. > >> */ > >> - BUG_ON(adreno_gpu->ubwc_config.highest_bank_bit < 13); > >> - u32 hbb = adreno_gpu->ubwc_config.highest_bank_bit - 13; > >> - u32 hbb_hi = hbb >> 2; > >> - u32 hbb_lo = hbb & 3; > >> - u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; > >> - u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); > >> + if (hbb < 0) > >> + hbb = adreno_gpu->ubwc_config.highest_bank_bit; > > > > No. The value we expose to userspace must match what we program. > > You'll break VK_EXT_host_image_copy otherwise. > > I didn't know that was exposed to userspace. > > The value must be altered either way - ultimately, the hardware must > receive the correct information. ubwc_config doesn't seem to be const, > so I can edit it there if you like it better. > > Konrad Yes, you should be calling qcom_smem_dram_get_hbb() in a6xx_calc_ubwc_config(). You can already see there's a TODO there to plug it in. Connor
On 4/9/25 5:30 PM, Connor Abbott wrote: > On Wed, Apr 9, 2025 at 11:22 AM Konrad Dybcio > <konrad.dybcio@oss.qualcomm.com> wrote: >> >> On 4/9/25 5:12 PM, Connor Abbott wrote: >>> On Wed, Apr 9, 2025 at 10:48 AM Konrad Dybcio <konradybcio@kernel.org> wrote: >>>> >>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >>>> >>>> The Highest Bank address Bit value can change based on memory type used. >>>> >>>> Attempt to retrieve it dynamically, and fall back to a reasonable >>>> default (the one used prior to this change) on error. >>>> >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >>>> --- >>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 22 ++++++++++++++++------ >>>> 1 file changed, 16 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..0cc397378c99db35315209d0265ad9223e8b55c7 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>> @@ -13,6 +13,7 @@ >>>> #include <linux/firmware/qcom/qcom_scm.h> >>>> #include <linux/pm_domain.h> >>>> #include <linux/soc/qcom/llcc-qcom.h> >>>> +#include <linux/soc/qcom/smem.h> >>>> >>>> #define GPU_PAS_ID 13 >>>> >>>> @@ -669,17 +670,22 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) >>>> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) >>>> { >>>> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); >>>> + u32 hbb = qcom_smem_dram_get_hbb(); >>>> + u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; >>>> + u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); >>>> + u32 hbb_hi, hbb_lo; >>>> + >>>> /* >>>> * We subtract 13 from the highest bank bit (13 is the minimum value >>>> * allowed by hw) and write the lowest two bits of the remaining value >>>> * as hbb_lo and the one above it as hbb_hi to the hardware. >>>> */ >>>> - BUG_ON(adreno_gpu->ubwc_config.highest_bank_bit < 13); >>>> - u32 hbb = adreno_gpu->ubwc_config.highest_bank_bit - 13; >>>> - u32 hbb_hi = hbb >> 2; >>>> - u32 hbb_lo = hbb & 3; >>>> - u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; >>>> - u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); >>>> + if (hbb < 0) >>>> + hbb = adreno_gpu->ubwc_config.highest_bank_bit; >>> >>> No. The value we expose to userspace must match what we program. >>> You'll break VK_EXT_host_image_copy otherwise. >> >> I didn't know that was exposed to userspace. >> >> The value must be altered either way - ultimately, the hardware must >> receive the correct information. ubwc_config doesn't seem to be const, >> so I can edit it there if you like it better. >> >> Konrad > > Yes, you should be calling qcom_smem_dram_get_hbb() in > a6xx_calc_ubwc_config(). You can already see there's a TODO there to > plug it in. Does this look good instead? diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 0cc397378c99..ae8dbc250e6a 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -588,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) { + u8 hbb; + gpu->ubwc_config.rgb565_predicator = 0; gpu->ubwc_config.uavflagprd_inv = 0; gpu->ubwc_config.min_acc_len = 0; @@ -636,7 +638,6 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) adreno_is_a690(gpu) || adreno_is_a730(gpu) || adreno_is_a740_family(gpu)) { - /* TODO: get ddr type from bootloader and use 2 for LPDDR4 */ gpu->ubwc_config.highest_bank_bit = 16; gpu->ubwc_config.amsbc = 1; gpu->ubwc_config.rgb565_predicator = 1; @@ -665,6 +666,13 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) gpu->ubwc_config.highest_bank_bit = 14; gpu->ubwc_config.min_acc_len = 1; } + + /* Attempt to retrieve HBB data from SMEM, keep the above defaults in case of error */ + hbb = qcom_smem_dram_get_hbb(); + if (hbb < 0) + return; + + gpu->ubwc_config.highest_bank_bit = hbb; } static void a6xx_set_ubwc_config(struct msm_gpu *gpu) Konrad
On Wed, Apr 9, 2025 at 11:40 AM Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> wrote: > > On 4/9/25 5:30 PM, Connor Abbott wrote: > > On Wed, Apr 9, 2025 at 11:22 AM Konrad Dybcio > > <konrad.dybcio@oss.qualcomm.com> wrote: > >> > >> On 4/9/25 5:12 PM, Connor Abbott wrote: > >>> On Wed, Apr 9, 2025 at 10:48 AM Konrad Dybcio <konradybcio@kernel.org> wrote: > >>>> > >>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > >>>> > >>>> The Highest Bank address Bit value can change based on memory type used. > >>>> > >>>> Attempt to retrieve it dynamically, and fall back to a reasonable > >>>> default (the one used prior to this change) on error. > >>>> > >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > >>>> --- > >>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 22 ++++++++++++++++------ > >>>> 1 file changed, 16 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..0cc397378c99db35315209d0265ad9223e8b55c7 100644 > >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>>> @@ -13,6 +13,7 @@ > >>>> #include <linux/firmware/qcom/qcom_scm.h> > >>>> #include <linux/pm_domain.h> > >>>> #include <linux/soc/qcom/llcc-qcom.h> > >>>> +#include <linux/soc/qcom/smem.h> > >>>> > >>>> #define GPU_PAS_ID 13 > >>>> > >>>> @@ -669,17 +670,22 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > >>>> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > >>>> { > >>>> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > >>>> + u32 hbb = qcom_smem_dram_get_hbb(); > >>>> + u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; > >>>> + u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); > >>>> + u32 hbb_hi, hbb_lo; > >>>> + > >>>> /* > >>>> * We subtract 13 from the highest bank bit (13 is the minimum value > >>>> * allowed by hw) and write the lowest two bits of the remaining value > >>>> * as hbb_lo and the one above it as hbb_hi to the hardware. > >>>> */ > >>>> - BUG_ON(adreno_gpu->ubwc_config.highest_bank_bit < 13); > >>>> - u32 hbb = adreno_gpu->ubwc_config.highest_bank_bit - 13; > >>>> - u32 hbb_hi = hbb >> 2; > >>>> - u32 hbb_lo = hbb & 3; > >>>> - u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; > >>>> - u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); > >>>> + if (hbb < 0) > >>>> + hbb = adreno_gpu->ubwc_config.highest_bank_bit; > >>> > >>> No. The value we expose to userspace must match what we program. > >>> You'll break VK_EXT_host_image_copy otherwise. > >> > >> I didn't know that was exposed to userspace. > >> > >> The value must be altered either way - ultimately, the hardware must > >> receive the correct information. ubwc_config doesn't seem to be const, > >> so I can edit it there if you like it better. > >> > >> Konrad > > > > Yes, you should be calling qcom_smem_dram_get_hbb() in > > a6xx_calc_ubwc_config(). You can already see there's a TODO there to > > plug it in. > > Does this look good instead? > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 0cc397378c99..ae8dbc250e6a 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -588,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > > static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > { > + u8 hbb; You can't make it u8 and then test for a negative value on error. Other than that, looks good. Connor > + > gpu->ubwc_config.rgb565_predicator = 0; > gpu->ubwc_config.uavflagprd_inv = 0; > gpu->ubwc_config.min_acc_len = 0; > @@ -636,7 +638,6 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > adreno_is_a690(gpu) || > adreno_is_a730(gpu) || > adreno_is_a740_family(gpu)) { > - /* TODO: get ddr type from bootloader and use 2 for LPDDR4 */ > gpu->ubwc_config.highest_bank_bit = 16; > gpu->ubwc_config.amsbc = 1; > gpu->ubwc_config.rgb565_predicator = 1; > @@ -665,6 +666,13 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > gpu->ubwc_config.highest_bank_bit = 14; > gpu->ubwc_config.min_acc_len = 1; > } > + > + /* Attempt to retrieve HBB data from SMEM, keep the above defaults in case of error */ > + hbb = qcom_smem_dram_get_hbb(); > + if (hbb < 0) > + return; > + > + gpu->ubwc_config.highest_bank_bit = hbb; > } > > static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > > > Konrad
On 4/9/25 5:44 PM, Connor Abbott wrote: > On Wed, Apr 9, 2025 at 11:40 AM Konrad Dybcio > <konrad.dybcio@oss.qualcomm.com> wrote: >> >> On 4/9/25 5:30 PM, Connor Abbott wrote: >>> On Wed, Apr 9, 2025 at 11:22 AM Konrad Dybcio >>> <konrad.dybcio@oss.qualcomm.com> wrote: >>>> >>>> On 4/9/25 5:12 PM, Connor Abbott wrote: >>>>> On Wed, Apr 9, 2025 at 10:48 AM Konrad Dybcio <konradybcio@kernel.org> wrote: >>>>>> >>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >>>>>> >>>>>> The Highest Bank address Bit value can change based on memory type used. >>>>>> >>>>>> Attempt to retrieve it dynamically, and fall back to a reasonable >>>>>> default (the one used prior to this change) on error. >>>>>> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >>>>>> --- >>>>>> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 22 ++++++++++++++++------ >>>>>> 1 file changed, 16 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..0cc397378c99db35315209d0265ad9223e8b55c7 100644 >>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>>> @@ -13,6 +13,7 @@ >>>>>> #include <linux/firmware/qcom/qcom_scm.h> >>>>>> #include <linux/pm_domain.h> >>>>>> #include <linux/soc/qcom/llcc-qcom.h> >>>>>> +#include <linux/soc/qcom/smem.h> >>>>>> >>>>>> #define GPU_PAS_ID 13 >>>>>> >>>>>> @@ -669,17 +670,22 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) >>>>>> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) >>>>>> { >>>>>> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); >>>>>> + u32 hbb = qcom_smem_dram_get_hbb(); >>>>>> + u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; >>>>>> + u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); >>>>>> + u32 hbb_hi, hbb_lo; >>>>>> + >>>>>> /* >>>>>> * We subtract 13 from the highest bank bit (13 is the minimum value >>>>>> * allowed by hw) and write the lowest two bits of the remaining value >>>>>> * as hbb_lo and the one above it as hbb_hi to the hardware. >>>>>> */ >>>>>> - BUG_ON(adreno_gpu->ubwc_config.highest_bank_bit < 13); >>>>>> - u32 hbb = adreno_gpu->ubwc_config.highest_bank_bit - 13; >>>>>> - u32 hbb_hi = hbb >> 2; >>>>>> - u32 hbb_lo = hbb & 3; >>>>>> - u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; >>>>>> - u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); >>>>>> + if (hbb < 0) >>>>>> + hbb = adreno_gpu->ubwc_config.highest_bank_bit; >>>>> >>>>> No. The value we expose to userspace must match what we program. >>>>> You'll break VK_EXT_host_image_copy otherwise. >>>> >>>> I didn't know that was exposed to userspace. >>>> >>>> The value must be altered either way - ultimately, the hardware must >>>> receive the correct information. ubwc_config doesn't seem to be const, >>>> so I can edit it there if you like it better. >>>> >>>> Konrad >>> >>> Yes, you should be calling qcom_smem_dram_get_hbb() in >>> a6xx_calc_ubwc_config(). You can already see there's a TODO there to >>> plug it in. >> >> Does this look good instead? >> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> index 0cc397378c99..ae8dbc250e6a 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >> @@ -588,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) >> >> static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) >> { >> + u8 hbb; > > You can't make it u8 and then test for a negative value on error. Fair. I think it was u8 in a pre-release version of the patchset and it stuck in my mind.. though I'dve expected clang to warn me here.. > Other than that, looks good. Thanks, I'll change it in v2. Konrad
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..0cc397378c99db35315209d0265ad9223e8b55c7 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -13,6 +13,7 @@ #include <linux/firmware/qcom/qcom_scm.h> #include <linux/pm_domain.h> #include <linux/soc/qcom/llcc-qcom.h> +#include <linux/soc/qcom/smem.h> #define GPU_PAS_ID 13 @@ -669,17 +670,22 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) static void a6xx_set_ubwc_config(struct msm_gpu *gpu) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); + u32 hbb = qcom_smem_dram_get_hbb(); + u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; + u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); + u32 hbb_hi, hbb_lo; + /* * We subtract 13 from the highest bank bit (13 is the minimum value * allowed by hw) and write the lowest two bits of the remaining value * as hbb_lo and the one above it as hbb_hi to the hardware. */ - BUG_ON(adreno_gpu->ubwc_config.highest_bank_bit < 13); - u32 hbb = adreno_gpu->ubwc_config.highest_bank_bit - 13; - u32 hbb_hi = hbb >> 2; - u32 hbb_lo = hbb & 3; - u32 ubwc_mode = adreno_gpu->ubwc_config.ubwc_swizzle & 1; - u32 level2_swizzling_dis = !(adreno_gpu->ubwc_config.ubwc_swizzle & 2); + if (hbb < 0) + hbb = adreno_gpu->ubwc_config.highest_bank_bit; + hbb -= 13; + BUG_ON(hbb < 0); + hbb_hi = hbb >> 2; + hbb_lo = hbb & 3; gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL, level2_swizzling_dis << 12 | @@ -2467,6 +2473,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev) bool is_a7xx; int ret; + /* We need data from SMEM to retrieve HBB in set_ubwc_config() */ + if (!qcom_smem_is_available()) + return ERR_PTR(-EPROBE_DEFER); + a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL); if (!a6xx_gpu) return ERR_PTR(-ENOMEM);