| Message ID | 20250410-topic-smem_dramc-v2-0-dead15264714@oss.qualcomm.com |
|---|---|
| Headers | show |
| Series | Retrieve information about DDR from SMEM | expand |
On 4/10/2025 11:13 PM, Konrad Dybcio 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 | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 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 > > @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > > static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > { > + int hbb; > + > gpu->ubwc_config.rgb565_predicator = 0; > gpu->ubwc_config.uavflagprd_inv = 0; > gpu->ubwc_config.min_acc_len = 0; > @@ -635,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; > @@ -664,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 the 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; I am worried about blindly relying on SMEM data directly for HBB for legacy chipsets. There is no guarantee it is accurate on every chipset and every version of firmware. Also, until recently, this value was hardcoded in Mesa which matched the value in KMD. So it is better to make this opt in, for newer chipsets or those which somebody can verify. We can invert this logic to something like this: if (!gpu->ubwc_config.highest_bank_bit) gpu->ubwc_config.highest_bank_bit = qcom_smem_dram_get_hbb(); > } > > static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > @@ -2467,6 +2476,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 calc_ubwc_config() */ > + if (!qcom_smem_is_available()) > + return ERR_PTR(-EPROBE_DEFER); > + We should add "depends on QCOM_SMEM" to Kconfig. Is SMEM device present in all Qcom SoC devicetrees? I wonder if there is a scenario where there might be an infinite EPROBE_DEFER here. -Akhil. > a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL); > if (!a6xx_gpu) > return ERR_PTR(-ENOMEM); >
On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > > On 4/10/2025 11:13 PM, Konrad Dybcio 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 | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 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 > > > > @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > > > > static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > > { > > + int hbb; > > + > > gpu->ubwc_config.rgb565_predicator = 0; > > gpu->ubwc_config.uavflagprd_inv = 0; > > gpu->ubwc_config.min_acc_len = 0; > > @@ -635,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; > > @@ -664,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 the 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; > > I am worried about blindly relying on SMEM data directly for HBB for > legacy chipsets. There is no guarantee it is accurate on every chipset > and every version of firmware. Also, until recently, this value was > hardcoded in Mesa which matched the value in KMD. To be clear about this, from the moment we introduced host image copies in Mesa we added support for querying the HBB from the kernel, explicitly so that we could do what this series does without Mesa ever breaking. Mesa will never assume the HBB unless the kernel is too old to support querying it. So don't let Mesa be the thing that stops us here. Connor > So it is better to > make this opt in, for newer chipsets or those which somebody can verify. > We can invert this logic to something like this: > > if (!gpu->ubwc_config.highest_bank_bit) > gpu->ubwc_config.highest_bank_bit = qcom_smem_dram_get_hbb(); > > > } > > > > static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > > @@ -2467,6 +2476,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 calc_ubwc_config() */ > > + if (!qcom_smem_is_available()) > > + return ERR_PTR(-EPROBE_DEFER); > > + > > We should add "depends on QCOM_SMEM" to Kconfig. Is SMEM device present > in all Qcom SoC devicetrees? I wonder if there is a scenario where there > might be an infinite EPROBE_DEFER here. > > -Akhil. > > > a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL); > > if (!a6xx_gpu) > > return ERR_PTR(-ENOMEM); > > >
On 4/17/2025 9:02 PM, Connor Abbott wrote: > On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >> >> On 4/10/2025 11:13 PM, Konrad Dybcio 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 | 15 ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 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 >>> >>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) >>> >>> static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) >>> { >>> + int hbb; >>> + >>> gpu->ubwc_config.rgb565_predicator = 0; >>> gpu->ubwc_config.uavflagprd_inv = 0; >>> gpu->ubwc_config.min_acc_len = 0; >>> @@ -635,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; >>> @@ -664,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 the 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; >> >> I am worried about blindly relying on SMEM data directly for HBB for >> legacy chipsets. There is no guarantee it is accurate on every chipset >> and every version of firmware. Also, until recently, this value was >> hardcoded in Mesa which matched the value in KMD. > > To be clear about this, from the moment we introduced host image > copies in Mesa we added support for querying the HBB from the kernel, > explicitly so that we could do what this series does without Mesa ever > breaking. Mesa will never assume the HBB unless the kernel is too old > to support querying it. So don't let Mesa be the thing that stops us > here. Thanks for clarifying about Mesa. I still don't trust a data source that is unused in production. I have a related question about HBB. Blob driver doesn't support host_image_copy, but it still use HBB configuration. I was under the impression this was required for UMD for compression related configurations. Is that not true for turnip/freedreno? -Akhil. > > Connor > >> So it is better to >> make this opt in, for newer chipsets or those which somebody can verify. >> We can invert this logic to something like this: >> >> if (!gpu->ubwc_config.highest_bank_bit) >> gpu->ubwc_config.highest_bank_bit = qcom_smem_dram_get_hbb(); >> >>> } >>> >>> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) >>> @@ -2467,6 +2476,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 calc_ubwc_config() */ >>> + if (!qcom_smem_is_available()) >>> + return ERR_PTR(-EPROBE_DEFER); >>> + >> >> We should add "depends on QCOM_SMEM" to Kconfig. Is SMEM device present >> in all Qcom SoC devicetrees? I wonder if there is a scenario where there >> might be an infinite EPROBE_DEFER here. >> >> -Akhil. >> >>> a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL); >>> if (!a6xx_gpu) >>> return ERR_PTR(-ENOMEM); >>> >>
On Thu, Apr 17, 2025, 1:50 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > > On 4/17/2025 9:02 PM, Connor Abbott wrote: > > On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > >> > >> On 4/10/2025 11:13 PM, Konrad Dybcio 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 | 15 ++++++++++++++- > >>> 1 file changed, 14 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 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 > >>> > >>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > >>> > >>> static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > >>> { > >>> + int hbb; > >>> + > >>> gpu->ubwc_config.rgb565_predicator = 0; > >>> gpu->ubwc_config.uavflagprd_inv = 0; > >>> gpu->ubwc_config.min_acc_len = 0; > >>> @@ -635,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; > >>> @@ -664,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 the 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; > >> > >> I am worried about blindly relying on SMEM data directly for HBB for > >> legacy chipsets. There is no guarantee it is accurate on every chipset > >> and every version of firmware. Also, until recently, this value was > >> hardcoded in Mesa which matched the value in KMD. > > > > To be clear about this, from the moment we introduced host image > > copies in Mesa we added support for querying the HBB from the kernel, > > explicitly so that we could do what this series does without Mesa ever > > breaking. Mesa will never assume the HBB unless the kernel is too old > > to support querying it. So don't let Mesa be the thing that stops us > > here. > > Thanks for clarifying about Mesa. I still don't trust a data source that > is unused in production. Fair enough, I'm not going to argue with that part. Just wanted to clear up any confusion about Mesa. Although, IIRC kgsl did set different values for a650 depending on memory type... do you know what source that used? > > I have a related question about HBB. Blob driver doesn't support > host_image_copy, but it still use HBB configuration. I was under the > impression this was required for UMD for compression related > configurations. Is that not true for turnip/freedreno? > > -Akhil. AFAIK the HBB (as well as other UBWC config parameters) doesn't have any impact on layout configuration, so the UMD can ignore it except when it's doing CPU texture uploads/downloads. We certainly do in freedreno/turnip. You'd have to ask that team what they use HBB for, but my best guess is that the GLES driver uses it for CPU texture uploads sometimes. That is, the GLES driver might be using functionality similar to host_image_copy "under the hood". It's something we'd probably want for freedreno too. Connor > > > > > Connor > > > >> So it is better to > >> make this opt in, for newer chipsets or those which somebody can verify. > >> We can invert this logic to something like this: > >> > >> if (!gpu->ubwc_config.highest_bank_bit) > >> gpu->ubwc_config.highest_bank_bit = qcom_smem_dram_get_hbb(); > >> > >>> } > >>> > >>> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > >>> @@ -2467,6 +2476,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 calc_ubwc_config() */ > >>> + if (!qcom_smem_is_available()) > >>> + return ERR_PTR(-EPROBE_DEFER); > >>> + > >> > >> We should add "depends on QCOM_SMEM" to Kconfig. Is SMEM device present > >> in all Qcom SoC devicetrees? I wonder if there is a scenario where there > >> might be an infinite EPROBE_DEFER here. > >> > >> -Akhil. > >> > >>> a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL); > >>> if (!a6xx_gpu) > >>> return ERR_PTR(-ENOMEM); > >>> > >> >
On 4/18/2025 6:40 AM, Connor Abbott wrote: > On Thu, Apr 17, 2025, 1:50 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >> >> On 4/17/2025 9:02 PM, Connor Abbott wrote: >>> On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>> >>>> On 4/10/2025 11:13 PM, Konrad Dybcio 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 | 15 ++++++++++++++- >>>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 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 >>>>> >>>>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) >>>>> >>>>> static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) >>>>> { >>>>> + int hbb; >>>>> + >>>>> gpu->ubwc_config.rgb565_predicator = 0; >>>>> gpu->ubwc_config.uavflagprd_inv = 0; >>>>> gpu->ubwc_config.min_acc_len = 0; >>>>> @@ -635,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; >>>>> @@ -664,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 the 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; >>>> >>>> I am worried about blindly relying on SMEM data directly for HBB for >>>> legacy chipsets. There is no guarantee it is accurate on every chipset >>>> and every version of firmware. Also, until recently, this value was >>>> hardcoded in Mesa which matched the value in KMD. >>> >>> To be clear about this, from the moment we introduced host image >>> copies in Mesa we added support for querying the HBB from the kernel, >>> explicitly so that we could do what this series does without Mesa ever >>> breaking. Mesa will never assume the HBB unless the kernel is too old >>> to support querying it. So don't let Mesa be the thing that stops us >>> here. >> >> Thanks for clarifying about Mesa. I still don't trust a data source that >> is unused in production. > > Fair enough, I'm not going to argue with that part. Just wanted to > clear up any confusion about Mesa. > > Although, IIRC kgsl did set different values for a650 depending on > memory type... do you know what source that used? KGSL relies on an undocumented devicetree node populated by bootloader to detect ddrtype and calculates the HBB value based on that. -Akhil. > >> >> I have a related question about HBB. Blob driver doesn't support >> host_image_copy, but it still use HBB configuration. I was under the >> impression this was required for UMD for compression related >> configurations. Is that not true for turnip/freedreno? >> >> -Akhil. > > AFAIK the HBB (as well as other UBWC config parameters) doesn't have > any impact on layout configuration, so the UMD can ignore it except > when it's doing CPU texture uploads/downloads. We certainly do in > freedreno/turnip. You'd have to ask that team what they use HBB for, > but my best guess is that the GLES driver uses it for CPU texture > uploads sometimes. That is, the GLES driver might be using > functionality similar to host_image_copy "under the hood". It's > something we'd probably want for freedreno too. > > Connor > >> >>> >>> Connor >>> >>>> So it is better to >>>> make this opt in, for newer chipsets or those which somebody can verify. >>>> We can invert this logic to something like this: >>>> >>>> if (!gpu->ubwc_config.highest_bank_bit) >>>> gpu->ubwc_config.highest_bank_bit = qcom_smem_dram_get_hbb(); >>>> >>>>> } >>>>> >>>>> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) >>>>> @@ -2467,6 +2476,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 calc_ubwc_config() */ >>>>> + if (!qcom_smem_is_available()) >>>>> + return ERR_PTR(-EPROBE_DEFER); >>>>> + >>>> >>>> We should add "depends on QCOM_SMEM" to Kconfig. Is SMEM device present >>>> in all Qcom SoC devicetrees? I wonder if there is a scenario where there >>>> might be an infinite EPROBE_DEFER here. >>>> >>>> -Akhil. >>>> >>>>> a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL); >>>>> if (!a6xx_gpu) >>>>> return ERR_PTR(-ENOMEM); >>>>> >>>> >>
On Fri, Apr 18, 2025 at 9:00 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > > On 4/18/2025 6:40 AM, Connor Abbott wrote: > > On Thu, Apr 17, 2025, 1:50 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > >> > >> On 4/17/2025 9:02 PM, Connor Abbott wrote: > >>> On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: > >>>> > >>>> On 4/10/2025 11:13 PM, Konrad Dybcio 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 | 15 ++++++++++++++- > >>>>> 1 file changed, 14 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > >>>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 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 > >>>>> > >>>>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > >>>>> > >>>>> static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) > >>>>> { > >>>>> + int hbb; > >>>>> + > >>>>> gpu->ubwc_config.rgb565_predicator = 0; > >>>>> gpu->ubwc_config.uavflagprd_inv = 0; > >>>>> gpu->ubwc_config.min_acc_len = 0; > >>>>> @@ -635,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; > >>>>> @@ -664,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 the 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; > >>>> > >>>> I am worried about blindly relying on SMEM data directly for HBB for > >>>> legacy chipsets. There is no guarantee it is accurate on every chipset > >>>> and every version of firmware. Also, until recently, this value was > >>>> hardcoded in Mesa which matched the value in KMD. > >>> > >>> To be clear about this, from the moment we introduced host image > >>> copies in Mesa we added support for querying the HBB from the kernel, > >>> explicitly so that we could do what this series does without Mesa ever > >>> breaking. Mesa will never assume the HBB unless the kernel is too old > >>> to support querying it. So don't let Mesa be the thing that stops us > >>> here. > >> > >> Thanks for clarifying about Mesa. I still don't trust a data source that > >> is unused in production. > > > > Fair enough, I'm not going to argue with that part. Just wanted to > > clear up any confusion about Mesa. > > > > Although, IIRC kgsl did set different values for a650 depending on > > memory type... do you know what source that used? > > KGSL relies on an undocumented devicetree node populated by bootloader > to detect ddrtype and calculates the HBB value based on that. Would it be reasonable to use the smem value, but if we find the undocumented dt property, WARN_ON() if it's value disagrees with smem? That would at least give some confidence, or justified un-confidence about the smem values BR, -R > > -Akhil. > > > > >> > >> I have a related question about HBB. Blob driver doesn't support > >> host_image_copy, but it still use HBB configuration. I was under the > >> impression this was required for UMD for compression related > >> configurations. Is that not true for turnip/freedreno? > >> > >> -Akhil. > > > > AFAIK the HBB (as well as other UBWC config parameters) doesn't have > > any impact on layout configuration, so the UMD can ignore it except > > when it's doing CPU texture uploads/downloads. We certainly do in > > freedreno/turnip. You'd have to ask that team what they use HBB for, > > but my best guess is that the GLES driver uses it for CPU texture > > uploads sometimes. That is, the GLES driver might be using > > functionality similar to host_image_copy "under the hood". It's > > something we'd probably want for freedreno too. > > > > Connor > > > >> > >>> > >>> Connor > >>> > >>>> So it is better to > >>>> make this opt in, for newer chipsets or those which somebody can verify. > >>>> We can invert this logic to something like this: > >>>> > >>>> if (!gpu->ubwc_config.highest_bank_bit) > >>>> gpu->ubwc_config.highest_bank_bit = qcom_smem_dram_get_hbb(); > >>>> > >>>>> } > >>>>> > >>>>> static void a6xx_set_ubwc_config(struct msm_gpu *gpu) > >>>>> @@ -2467,6 +2476,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 calc_ubwc_config() */ > >>>>> + if (!qcom_smem_is_available()) > >>>>> + return ERR_PTR(-EPROBE_DEFER); > >>>>> + > >>>> > >>>> We should add "depends on QCOM_SMEM" to Kconfig. Is SMEM device present > >>>> in all Qcom SoC devicetrees? I wonder if there is a scenario where there > >>>> might be an infinite EPROBE_DEFER here. > >>>> > >>>> -Akhil. > >>>> > >>>>> a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL); > >>>>> if (!a6xx_gpu) > >>>>> return ERR_PTR(-ENOMEM); > >>>>> > >>>> > >> >
On 4/21/25 10:13 PM, Rob Clark wrote: > On Fri, Apr 18, 2025 at 9:00 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >> >> On 4/18/2025 6:40 AM, Connor Abbott wrote: >>> On Thu, Apr 17, 2025, 1:50 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>> >>>> On 4/17/2025 9:02 PM, Connor Abbott wrote: >>>>> On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>>> >>>>>> On 4/10/2025 11:13 PM, Konrad Dybcio 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 | 15 ++++++++++++++- >>>>>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 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 >>>>>>> >>>>>>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) >>>>>>> >>>>>>> static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) >>>>>>> { >>>>>>> + int hbb; >>>>>>> + >>>>>>> gpu->ubwc_config.rgb565_predicator = 0; >>>>>>> gpu->ubwc_config.uavflagprd_inv = 0; >>>>>>> gpu->ubwc_config.min_acc_len = 0; >>>>>>> @@ -635,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; >>>>>>> @@ -664,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 the 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; >>>>>> >>>>>> I am worried about blindly relying on SMEM data directly for HBB for >>>>>> legacy chipsets. There is no guarantee it is accurate on every chipset >>>>>> and every version of firmware. Also, until recently, this value was >>>>>> hardcoded in Mesa which matched the value in KMD. >>>>> >>>>> To be clear about this, from the moment we introduced host image >>>>> copies in Mesa we added support for querying the HBB from the kernel, >>>>> explicitly so that we could do what this series does without Mesa ever >>>>> breaking. Mesa will never assume the HBB unless the kernel is too old >>>>> to support querying it. So don't let Mesa be the thing that stops us >>>>> here. >>>> >>>> Thanks for clarifying about Mesa. I still don't trust a data source that >>>> is unused in production. >>> >>> Fair enough, I'm not going to argue with that part. Just wanted to >>> clear up any confusion about Mesa. >>> >>> Although, IIRC kgsl did set different values for a650 depending on >>> memory type... do you know what source that used? >> >> KGSL relies on an undocumented devicetree node populated by bootloader >> to detect ddrtype and calculates the HBB value based on that. > > Would it be reasonable to use the smem value, but if we find the > undocumented dt property, WARN_ON() if it's value disagrees with smem? > > That would at least give some confidence, or justified un-confidence > about the smem values The aforementioned value is populated based on the data that this driver reads out, and only on the same range of platforms that this driver happens to cater to Konrad
On 4/23/25 5:23 PM, Dmitry Baryshkov wrote: > On 23/04/2025 17:55, Rob Clark wrote: >> On Tue, Apr 22, 2025 at 4:57 PM Konrad Dybcio >> <konrad.dybcio@oss.qualcomm.com> wrote: >>> >>> On 4/21/25 10:13 PM, Rob Clark wrote: >>>> On Fri, Apr 18, 2025 at 9:00 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>> >>>>> On 4/18/2025 6:40 AM, Connor Abbott wrote: >>>>>> On Thu, Apr 17, 2025, 1:50 PM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>>>> >>>>>>> On 4/17/2025 9:02 PM, Connor Abbott wrote: >>>>>>>> On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@quicinc.com> wrote: >>>>>>>>> >>>>>>>>> On 4/10/2025 11:13 PM, Konrad Dybcio 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 | 15 ++++++++++++++- >>>>>>>>>> 1 file changed, 14 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c >>>>>>>>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 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 >>>>>>>>>> >>>>>>>>>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) >>>>>>>>>> >>>>>>>>>> static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu) >>>>>>>>>> { >>>>>>>>>> + int hbb; >>>>>>>>>> + >>>>>>>>>> gpu->ubwc_config.rgb565_predicator = 0; >>>>>>>>>> gpu->ubwc_config.uavflagprd_inv = 0; >>>>>>>>>> gpu->ubwc_config.min_acc_len = 0; >>>>>>>>>> @@ -635,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; >>>>>>>>>> @@ -664,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 the 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; >>>>>>>>> >>>>>>>>> I am worried about blindly relying on SMEM data directly for HBB for >>>>>>>>> legacy chipsets. There is no guarantee it is accurate on every chipset >>>>>>>>> and every version of firmware. Also, until recently, this value was >>>>>>>>> hardcoded in Mesa which matched the value in KMD. >>>>>>>> >>>>>>>> To be clear about this, from the moment we introduced host image >>>>>>>> copies in Mesa we added support for querying the HBB from the kernel, >>>>>>>> explicitly so that we could do what this series does without Mesa ever >>>>>>>> breaking. Mesa will never assume the HBB unless the kernel is too old >>>>>>>> to support querying it. So don't let Mesa be the thing that stops us >>>>>>>> here. >>>>>>> >>>>>>> Thanks for clarifying about Mesa. I still don't trust a data source that >>>>>>> is unused in production. >>>>>> >>>>>> Fair enough, I'm not going to argue with that part. Just wanted to >>>>>> clear up any confusion about Mesa. >>>>>> >>>>>> Although, IIRC kgsl did set different values for a650 depending on >>>>>> memory type... do you know what source that used? >>>>> >>>>> KGSL relies on an undocumented devicetree node populated by bootloader >>>>> to detect ddrtype and calculates the HBB value based on that. >>>> >>>> Would it be reasonable to use the smem value, but if we find the >>>> undocumented dt property, WARN_ON() if it's value disagrees with smem? >>>> >>>> That would at least give some confidence, or justified un-confidence >>>> about the smem values >>> >>> The aforementioned value is populated based on the data that this >>> driver reads out, and only on the same range of platforms that this >>> driver happens to cater to >> >> Did I understand that correctly to mean that the dt property is based >> on the same smem value that you are using? In that case, there should >> be no argument against using the smem value as the source of truth. > > It is, but is done by the bootloader that knows exact format of the data. Right, so the only point of concern here is the handwavy matching-by-size logic. Konrad
SMEM allows the OS to retrieve information about the DDR memory. Among that information, is a semi-magic value called 'HBB', or Highest Bank address Bit, which multimedia drivers (for hardware like Adreno and MDSS) must retrieve in order to program the IP blocks correctly. This series introduces an API to retrieve that value, uses it in the aforementioned programming sequences and exposes available DDR frequencies in debugfs (to e.g. pass to aoss_qmp debugfs). More information can be exposed in the future, as needed. Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> --- Changes in v2: - Avoid checking for < 0 on unsigned types - Overwrite Adreno UBWC data to keep the data shared with userspace coherent with what's programmed into the hardware - Call get_hbb() in msm_mdss_enable() instead of all UBWC setup branches separately - Pick up Bjorn's rb on patch 1 - Link to v1: https://lore.kernel.org/r/20250409-topic-smem_dramc-v1-0-94d505cd5593@oss.qualcomm.com --- Konrad Dybcio (4): soc: qcom: Expose DDR data from SMEM drm/msm/a5xx: Get HBB dynamically, if available drm/msm/a6xx: Get HBB dynamically, if available drm/msm/mdss: Get HBB dynamically, if available drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 12 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 15 +- drivers/gpu/drm/msm/msm_mdss.c | 30 ++-- drivers/soc/qcom/Makefile | 3 +- drivers/soc/qcom/smem.c | 14 +- drivers/soc/qcom/smem.h | 9 ++ drivers/soc/qcom/smem_dramc.c | 287 ++++++++++++++++++++++++++++++++++ include/linux/soc/qcom/smem.h | 4 + 8 files changed, 360 insertions(+), 14 deletions(-) --- base-commit: 46086739de22d72319e37c37a134d32db52e1c5c change-id: 20250409-topic-smem_dramc-6467187ac865 Best regards,