On 23/04/2025 17:55, Rob Clark wrote:
On Tue, Apr 22, 2025 at 4:57 PM Konrad Dybcio
<konrad.dyb...@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_akhi...@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_akhi...@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_akhi...@quicinc.com> wrote:

On 4/10/2025 11:13 PM, Konrad Dybcio wrote:
From: Konrad Dybcio <konrad.dyb...@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.dyb...@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.


--
With best wishes
Dmitry

Reply via email to