[PATCH v2 0/4] Retrieve information about DDR from SMEM

[PATCH v2 0/4] Retrieve information about DDR from SMEM

[Konrad Dybcio]

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,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

[PATCH v2 1/4] soc: qcom: Expose DDR data from SMEM

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Most modern Qualcomm platforms (>= SM8150) expose information about the
DDR memory present on the system via SMEM.

Details from this information is used in various scenarios, such as
multimedia drivers configuring the hardware based on the "Highest Bank
address Bit" (hbb), or the list of valid frequencies in validation
scenarios...

Add support for parsing v3-v5 version of the structs. Unforunately,
they are not versioned, so some elbow grease is necessary to determine
which one is present. See for reference:

v3: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/1d11897d2cfcc7b85f28ff74c445018dbbecac7a
v4: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/f6e9aa549260bbc0bdcb156c2b05f48dc5963203
v5: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/blob/uefi.lnx.4.0.r31-rel/QcomModulePkg/Include/Protocol/DDRDetails.h?ref_type=heads

Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 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 +
 5 files changed, 315 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index acbca2ab5cc2a9ab3dce1ff38efd048ba2fab31e..7227f648893d047d7de8819dc159554af6a7b817 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -23,7 +23,8 @@ obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
 qcom_rpmh-y			+= rpmh-rsc.o
 qcom_rpmh-y			+= rpmh.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= rpm-proc.o smd-rpm.o
-obj-$(CONFIG_QCOM_SMEM) +=	smem.o
+qcom_smem-y			+= smem.o smem_dramc.o
+obj-$(CONFIG_QCOM_SMEM) +=	qcom_smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 CFLAGS_smp2p.o := -I$(src)
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 59281970180921b76312fd5020828edced739344..cfd6a9d531d3d2438d7577be0c594d3b960bd003 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/debugfs.h>
 #include <linux/hwspinlock.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -16,6 +17,8 @@
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/socinfo.h>
 
+#include "smem.h"
+
 /*
  * The Qualcomm shared memory system is a allocate only heap structure that
  * consists of one of more memory areas that can be accessed by the processors
@@ -284,6 +287,8 @@ struct qcom_smem {
 	struct smem_partition global_partition;
 	struct smem_partition partitions[SMEM_HOST_COUNT];
 
+	struct dentry *debugfs_dir;
+
 	unsigned num_regions;
 	struct smem_region regions[] __counted_by(num_regions);
 };
@@ -1230,17 +1235,24 @@ static int qcom_smem_probe(struct platform_device *pdev)
 
 	__smem = smem;
 
+	smem->debugfs_dir = smem_dram_parse(smem->dev);
+
 	smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
 						      PLATFORM_DEVID_NONE, NULL,
 						      0);
-	if (IS_ERR(smem->socinfo))
+	if (IS_ERR(smem->socinfo)) {
+		debugfs_remove_recursive(smem->debugfs_dir);
+
 		dev_dbg(&pdev->dev, "failed to register socinfo device\n");
+	}
 
 	return 0;
 }
 
 static void qcom_smem_remove(struct platform_device *pdev)
 {
+	debugfs_remove_recursive(__smem->debugfs_dir);
+
 	platform_device_unregister(__smem->socinfo);
 
 	hwspin_lock_free(__smem->hwlock);
diff --git a/drivers/soc/qcom/smem.h b/drivers/soc/qcom/smem.h
new file mode 100644
index 0000000000000000000000000000000000000000..8bf3f606e1ae80b7aa02b9567870f6a2681f8e5a
--- /dev/null
+++ b/drivers/soc/qcom/smem.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __QCOM_SMEM_INTERNAL__
+#define __QCOM_SMEM_INTERNAL__
+
+#include <linux/device.h>
+
+struct dentry *smem_dram_parse(struct device *dev);
+
+#endif
diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
new file mode 100644
index 0000000000000000000000000000000000000000..6ded45fd55c2ffa0924492f8042b753ec6c925cf
--- /dev/null
+++ b/drivers/soc/qcom/smem_dramc.c
@@ -0,0 +1,287 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/units.h>
+#include <linux/soc/qcom/smem.h>
+
+#include "smem.h"
+
+#define SMEM_DDR_INFO_ID		603
+
+#define MAX_DDR_FREQ_NUM_V3		13
+#define MAX_DDR_FREQ_NUM_V5		14
+
+#define MAX_DDR_REGION_NUM		6
+#define MAX_CHAN_NUM			8
+#define MAX_RANK_NUM			2
+
+static struct smem_dram *__dram;
+
+enum ddr_info_version {
+	INFO_UNKNOWN,
+	INFO_V3,
+	INFO_V3_WITH_14_FREQS,
+	INFO_V4,
+	INFO_V5,
+	INFO_V5_WITH_6_REGIONS,
+};
+
+struct smem_dram {
+	unsigned long frequencies[MAX_DDR_FREQ_NUM_V5];
+	u32 num_frequencies;
+	u8 hbb;
+};
+
+enum ddr_type {
+	DDR_TYPE_NODDR = 0,
+	DDR_TYPE_LPDDR1 = 1,
+	DDR_TYPE_LPDDR2 = 2,
+	DDR_TYPE_PCDDR2 = 3,
+	DDR_TYPE_PCDDR3 = 4,
+	DDR_TYPE_LPDDR3 = 5,
+	DDR_TYPE_LPDDR4 = 6,
+	DDR_TYPE_LPDDR4X = 7,
+	DDR_TYPE_LPDDR5 = 8,
+	DDR_TYPE_LPDDR5X = 9,
+};
+
+/* The data structures below are NOT __packed on purpose! */
+
+/* Structs used across multiple versions */
+struct ddr_part_details {
+	__le16 revision_id1;
+	__le16 revision_id2;
+	__le16 width;
+	__le16 density;
+};
+
+struct ddr_freq_table {
+	u32 freq_khz;
+	u8 enabled;
+};
+
+/* V3 */
+struct ddr_freq_plan_v3 {
+	struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V3]; /* NOTE: some have 14 like v5 */
+	u8 num_ddr_freqs;
+	phys_addr_t clk_period_address;
+};
+
+struct ddr_details_v3 {
+	u8 manufacturer_id;
+	u8 device_type;
+	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
+	struct ddr_freq_plan_v3 ddr_freq_tbl;
+	u8 num_channels;
+};
+
+/* V4 */
+struct ddr_details_v4 {
+	u8 manufacturer_id;
+	u8 device_type;
+	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
+	struct ddr_freq_plan_v3 ddr_freq_tbl;
+	u8 num_channels;
+	u8 num_ranks[MAX_CHAN_NUM];
+	u8 highest_bank_addr_bit[MAX_CHAN_NUM][MAX_RANK_NUM];
+};
+
+/* V5 */
+struct ddr_freq_plan_v5 {
+	struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V5];
+	u8 num_ddr_freqs;
+	phys_addr_t clk_period_address;
+	u32 max_nom_ddr_freq;
+};
+
+struct ddr_region_v5 {
+	u64 start_address;
+	u64 size;
+	u64 mem_controller_address;
+	u32 granule_size; /* MiB */
+	u8  ddr_rank;
+#define DDR_RANK_0	BIT(0)
+#define DDR_RANK_1	BIT(1)
+	u8  segments_start_index;
+	u64 segments_start_offset;
+};
+
+struct ddr_regions_v5 {
+	u32 ddr_region_num; /* We expect this to always be 4 or 6 */
+	u64 ddr_rank0_size;
+	u64 ddr_rank1_size;
+	u64 ddr_cs0_start_addr;
+	u64 ddr_cs1_start_addr;
+	u32 highest_bank_addr_bit;
+	struct ddr_region_v5 ddr_region[] __counted_by(ddr_region_num);
+};
+
+struct ddr_details_v5 {
+	u8 manufacturer_id;
+	u8 device_type;
+	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
+	struct ddr_freq_plan_v5 ddr_freq_tbl;
+	u8 num_channels;
+	struct ddr_regions_v5 ddr_regions;
+};
+
+/**
+ * qcom_smem_dram_get_hbb(): Get the Highest bank address bit
+ *
+ * Context: Check qcom_smem_is_available() before calling this function.
+ * Because __dram * is initialized by smem_dram_parse(), which is in turn
+ * called from * qcom_smem_probe(), __dram will only be NULL if the data
+ * couldn't have been found/interpreted correctly.
+ *
+ * If the function fails, the argument is left unmodified.
+ *
+ * Return: 0 on success, -ENODATA on failure.
+ */
+int qcom_smem_dram_get_hbb(void)
+{
+	return __dram ? __dram->hbb : -ENODATA;
+}
+EXPORT_SYMBOL_GPL(qcom_smem_dram_get_hbb);
+
+static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data, bool additional_freq_entry)
+{
+	/* This may be 13 or 14 */
+	int num_freq_entries = MAX_DDR_FREQ_NUM_V3;
+	struct ddr_details_v3 *details = data;
+
+	if (additional_freq_entry)
+		num_freq_entries++;
+
+	for (int i = 0; i < num_freq_entries; i++) {
+		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
+
+		if (freq_entry->freq_khz && freq_entry->enabled)
+			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
+	}
+}
+
+static void smem_dram_parse_v4_data(struct smem_dram *dram, void *data)
+{
+	struct ddr_details_v4 *details = data;
+
+	/* Rank 0 channel 0 entry holds the correct value */
+	dram->hbb = details->highest_bank_addr_bit[0][0];
+
+	for (int i = 0; i < MAX_DDR_FREQ_NUM_V3; i++) {
+		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
+
+		if (freq_entry->freq_khz && freq_entry->enabled)
+			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
+	}
+}
+
+static void smem_dram_parse_v5_data(struct smem_dram *dram, void *data)
+{
+	struct ddr_details_v5 *details = data;
+	struct ddr_regions_v5 *region = &details->ddr_regions;
+
+	dram->hbb = region[0].highest_bank_addr_bit;
+
+	for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
+		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
+
+		if (freq_entry->freq_khz && freq_entry->enabled)
+			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
+	}
+}
+
+/* The structure contains no version field, so we have to perform some guesswork.. */
+static int smem_dram_infer_struct_version(size_t size)
+{
+	/* Some early versions provided less bytes of less useful data */
+	if (size < sizeof(struct ddr_details_v3))
+		return -EINVAL;
+	if (size == sizeof(struct ddr_details_v3))
+		return INFO_V3;
+	else if (size == sizeof(struct ddr_details_v3) + sizeof(struct ddr_freq_table))
+		return INFO_V3_WITH_14_FREQS;
+	else if (size == sizeof(struct ddr_details_v4))
+		return INFO_V4;
+	else if (size == sizeof(struct ddr_details_v5) + 4 * sizeof(struct ddr_region_v5))
+		return INFO_V5;
+	else if (size == sizeof(struct ddr_details_v5) + 6 * sizeof(struct ddr_region_v5))
+		return INFO_V5_WITH_6_REGIONS;
+
+	return INFO_UNKNOWN;
+}
+
+static int smem_dram_frequencies_show(struct seq_file *s, void *unused)
+{
+	struct smem_dram *dram = s->private;
+
+	for (int i = 0; i < dram->num_frequencies; i++)
+		seq_printf(s, "%lu\n", dram->frequencies[i]);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(smem_dram_frequencies);
+
+struct dentry *smem_dram_parse(struct device *dev)
+{
+	struct dentry *debugfs_dir;
+	enum ddr_info_version ver;
+	struct smem_dram *dram;
+	size_t actual_size;
+	void *data = NULL;
+
+	/* No need to check qcom_smem_is_available(), this func is called by the SMEM driver */
+	data = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_DDR_INFO_ID, &actual_size);
+	if (IS_ERR_OR_NULL(data))
+		return ERR_PTR(-ENODATA);
+
+	ver = smem_dram_infer_struct_version(actual_size);
+	if (ver < 0) {
+		/* Some SoCs don't provide data that's useful for us */
+		return ERR_PTR(-ENODATA);
+	} else if (ver == INFO_UNKNOWN) {
+		/* In other cases, we may not have added support for a newer struct revision */
+		pr_err("Found an unknown type of DRAM info struct (size = %zu)\n", actual_size);
+		return ERR_PTR(-EINVAL);
+	}
+
+	dram = devm_kzalloc(dev, sizeof(*dram), GFP_KERNEL);
+	if (!dram)
+		return ERR_PTR(-ENOMEM);
+
+	switch (ver) {
+	case INFO_V3:
+		smem_dram_parse_v3_data(dram, data, false);
+		break;
+	case INFO_V3_WITH_14_FREQS:
+		smem_dram_parse_v3_data(dram, data, true);
+		break;
+	case INFO_V4:
+		smem_dram_parse_v4_data(dram, data);
+		break;
+	case INFO_V5:
+	case INFO_V5_WITH_6_REGIONS:
+		smem_dram_parse_v5_data(dram, data);
+		break;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Both the entry and its parent dir will be cleaned up by debugfs_remove_recursive */
+	debugfs_dir = debugfs_create_dir("qcom_smem", NULL);
+	debugfs_create_file("dram_frequencies", 0444, debugfs_dir,
+			    dram, &smem_dram_frequencies_fops);
+
+	/* If there was no failure so far, assign the global variable */
+	__dram = dram;
+
+	return debugfs_dir;
+}
diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
index f946e3beca215548ac56dbf779138d05479712f5..223cd5090a2a8d0b29be768c6a9cc76c2997bbce 100644
--- a/include/linux/soc/qcom/smem.h
+++ b/include/linux/soc/qcom/smem.h
@@ -2,6 +2,8 @@
 #ifndef __QCOM_SMEM_H__
 #define __QCOM_SMEM_H__
 
+#include <linux/platform_device.h>
+
 #define QCOM_SMEM_HOST_ANY -1
 
 bool qcom_smem_is_available(void);
@@ -17,4 +19,6 @@ int qcom_smem_get_feature_code(u32 *code);
 
 int qcom_smem_bust_hwspin_lock_by_host(unsigned int host);
 
+int qcom_smem_dram_get_hbb(void);
+
 #endif

-- 
2.49.0

[PATCH v2 2/4] drm/msm/a5xx: Get HBB dynamically, if available

[Konrad Dybcio]

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/a5xx_gpu.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 650e5bac225f372e819130b891f1d020b464f17f..c887d46c3a5798b7aa6813fc6e2575be1e715100 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -9,6 +9,7 @@
 #include <linux/pm_opp.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/slab.h>
+#include <linux/soc/qcom/smem.h>
 #include "msm_gem.h"
 #include "msm_mmu.h"
 #include "a5xx_gpu.h"
@@ -1758,7 +1759,11 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
 	struct adreno_gpu *adreno_gpu;
 	struct msm_gpu *gpu;
 	unsigned int nr_rings;
-	int ret;
+	int hbb, ret;
+
+	/* We need data from SMEM to retrieve HBB below */
+	if (!qcom_smem_is_available())
+		return ERR_PTR(-EPROBE_DEFER);
 
 	a5xx_gpu = kzalloc(sizeof(*a5xx_gpu), GFP_KERNEL);
 	if (!a5xx_gpu)
@@ -1796,6 +1801,11 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
 	else
 		adreno_gpu->ubwc_config.highest_bank_bit = 14;
 
+	/* Attempt to retrieve HBB data from SMEM, keep the above defaults in case of error */
+	hbb = qcom_smem_dram_get_hbb();
+	if (hbb > 0)
+		adreno_gpu->ubwc_config.highest_bank_bit = hbb;
+
 	/* a5xx only supports UBWC 1.0, these are not configurable */
 	adreno_gpu->ubwc_config.macrotile_mode = 0;
 	adreno_gpu->ubwc_config.ubwc_swizzle = 0x7;

-- 
2.49.0

[PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Konrad Dybcio]

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;
 }
 
 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);
+
 	a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL);
 	if (!a6xx_gpu)
 		return ERR_PTR(-ENOMEM);

-- 
2.49.0

[PATCH v2 4/4] drm/msm/mdss: Get HBB dynamically, if available

[Konrad Dybcio]

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/msm_mdss.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index dcb49fd30402b80edd2cb5971f95a78eaad6081f..3f5c60ce20c0b66160bcc9bf74bf8f86ab57e9a4 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
+#include <linux/soc/qcom/smem.h>
 
 #include "msm_mdss.h"
 #include "msm_kms.h"
@@ -163,11 +164,11 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
 	return 0;
 }
 
-static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
+static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss, int hbb)
 {
 	const struct msm_mdss_data *data = msm_mdss->mdss_data;
 	u32 value = MDSS_UBWC_STATIC_UBWC_SWIZZLE(data->ubwc_swizzle) |
-		    MDSS_UBWC_STATIC_HIGHEST_BANK_BIT(data->highest_bank_bit);
+		    MDSS_UBWC_STATIC_HIGHEST_BANK_BIT(hbb);
 
 	if (data->ubwc_bank_spread)
 		value |= MDSS_UBWC_STATIC_UBWC_BANK_SPREAD;
@@ -178,11 +179,11 @@ static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
 	writel_relaxed(value, msm_mdss->mmio + REG_MDSS_UBWC_STATIC);
 }
 
-static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
+static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss, int hbb)
 {
 	const struct msm_mdss_data *data = msm_mdss->mdss_data;
 	u32 value = MDSS_UBWC_STATIC_UBWC_SWIZZLE(data->ubwc_swizzle & 0x1) |
-		    MDSS_UBWC_STATIC_HIGHEST_BANK_BIT(data->highest_bank_bit);
+		    MDSS_UBWC_STATIC_HIGHEST_BANK_BIT(hbb);
 
 	if (data->macrotile_mode)
 		value |= MDSS_UBWC_STATIC_MACROTILE_MODE;
@@ -196,11 +197,11 @@ static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
 	writel_relaxed(value, msm_mdss->mmio + REG_MDSS_UBWC_STATIC);
 }
 
-static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
+static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss, int hbb)
 {
 	const struct msm_mdss_data *data = msm_mdss->mdss_data;
 	u32 value = MDSS_UBWC_STATIC_UBWC_SWIZZLE(data->ubwc_swizzle) |
-		    MDSS_UBWC_STATIC_HIGHEST_BANK_BIT(data->highest_bank_bit);
+		    MDSS_UBWC_STATIC_HIGHEST_BANK_BIT(hbb);
 
 	if (data->ubwc_bank_spread)
 		value |= MDSS_UBWC_STATIC_UBWC_BANK_SPREAD;
@@ -287,7 +288,7 @@ const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
 
 static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 {
-	int ret, i;
+	int hbb, ret, i;
 
 	/*
 	 * Several components have AXI clocks that can only be turned on if
@@ -317,6 +318,11 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 	if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
 		return 0;
 
+	/* Attempt to retrieve HBB data from SMEM, keep reasonable defaults in case of error */
+	hbb = qcom_smem_dram_get_hbb() - 13;
+	if (hbb < 0)
+		hbb = msm_mdss->mdss_data->highest_bank_bit;
+
 	/*
 	 * ubwc config is part of the "mdss" region which is not accessible
 	 * from the rest of the driver. hardcode known configurations here
@@ -330,14 +336,14 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
 		/* do nothing */
 		break;
 	case UBWC_2_0:
-		msm_mdss_setup_ubwc_dec_20(msm_mdss);
+		msm_mdss_setup_ubwc_dec_20(msm_mdss, hbb);
 		break;
 	case UBWC_3_0:
-		msm_mdss_setup_ubwc_dec_30(msm_mdss);
+		msm_mdss_setup_ubwc_dec_30(msm_mdss, hbb);
 		break;
 	case UBWC_4_0:
 	case UBWC_4_3:
-		msm_mdss_setup_ubwc_dec_40(msm_mdss);
+		msm_mdss_setup_ubwc_dec_40(msm_mdss, hbb);
 		break;
 	default:
 		dev_err(msm_mdss->dev, "Unsupported UBWC decoder version %x\n",
@@ -538,6 +544,10 @@ static int mdss_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
+	/* We need data from SMEM to retrieve HBB msm_mdss_enable() */
+	if (!qcom_smem_is_available())
+		return -EPROBE_DEFER;
+
 	mdss = msm_mdss_init(pdev, is_mdp5);
 	if (IS_ERR(mdss))
 		return PTR_ERR(mdss);

-- 
2.49.0

Re: [PATCH v2 0/4] Retrieve information about DDR from SMEM

[Dmitry Baryshkov]

On Thu, Apr 10, 2025 at 07:43:43PM +0200, Konrad Dybcio wrote:
> 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 ++--

This misses the dpu_hw_sspp.c, which uses ubwc_config from msm_mdss.c
(but the config isn't being updated with the acquired HBB value).

I'd suggest behaving it slightly differntly: can we please have a helper
module (in drivers/soc/qcom) which would return UBWC configuration data.
We can start with HBB values, migrating the rest of UBWC-related flags
one by one.

Also, were you able to solve the issue of the platforms where GPU and
MDSS disagreed upon HBB data?

>  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,
> -- 
> Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Akhil P Oommen]

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);
> 

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Connor Abbott]

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);
> >
>

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Akhil P Oommen]

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);
>>>
>>

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Connor Abbott]

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);
> >>>
> >>
>

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Akhil P Oommen]

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);
>>>>>
>>>>
>>

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Rob Clark]

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);
> >>>>>
> >>>>
> >>
>

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Konrad Dybcio]

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

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Akhil P Oommen]

On 4/23/2025 5:27 AM, Konrad Dybcio 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

Like I suggested privately, can we centralize all ubwc configuration so
that it is consistent across all drivers. With that, we will need to
maintain a table of ubwc config for each chipset and HBB can be
calculated based on the DDR configuration identified from SMEM. Once we
migrate the downstream drivers to the new API, we can hopefully move to
the HBB value from SMEM. This will ensure that the SMEM data for HBB is
accurate in all future chipsets.

-Akhil.



> 
> Konrad

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Dmitry Baryshkov]

On Wed, 23 Apr 2025 at 09:55, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>
> On 4/23/2025 5:27 AM, Konrad Dybcio 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
>
> Like I suggested privately, can we centralize all ubwc configuration so
> that it is consistent across all drivers. With that, we will need to
> maintain a table of ubwc config for each chipset and HBB can be
> calculated based on the DDR configuration identified from SMEM. Once we
> migrate the downstream drivers to the new API, we can hopefully move to
> the HBB value from SMEM. This will ensure that the SMEM data for HBB is
> accurate in all future chipsets.

I like this suggestion.

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Rob Clark]

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.

BR,
-R

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Rob Clark]

On Tue, Apr 22, 2025 at 11:55 PM Akhil P Oommen
<quic_akhilpo@quicinc.com> wrote:
>
> On 4/23/2025 5:27 AM, Konrad Dybcio 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
>
> Like I suggested privately, can we centralize all ubwc configuration so
> that it is consistent across all drivers. With that, we will need to
> maintain a table of ubwc config for each chipset and HBB can be
> calculated based on the DDR configuration identified from SMEM. Once we
> migrate the downstream drivers to the new API, we can hopefully move to
> the HBB value from SMEM. This will ensure that the SMEM data for HBB is
> accurate in all future chipsets.
>

yes please

BR,
-R

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Konrad Dybcio]

On 4/23/25 4:55 PM, 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?

Yes, abl reads it out and modifies the FDT based on what's in there

Konrad

>  In that case, there should
> be no argument against using the smem value as the source of truth.
> 
> BR,
> -R

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Konrad Dybcio]

On 4/23/25 4:56 PM, Rob Clark wrote:
> On Tue, Apr 22, 2025 at 11:55 PM Akhil P Oommen
> <quic_akhilpo@quicinc.com> wrote:
>>
>> On 4/23/2025 5:27 AM, Konrad Dybcio 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
>>
>> Like I suggested privately, can we centralize all ubwc configuration so
>> that it is consistent across all drivers. With that, we will need to
>> maintain a table of ubwc config for each chipset and HBB can be
>> calculated based on the DDR configuration identified from SMEM. Once we
>> migrate the downstream drivers to the new API, we can hopefully move to
>> the HBB value from SMEM. This will ensure that the SMEM data for HBB is
>> accurate in all future chipsets.
>>
> 
> yes please

Alright, I'll get this sorted out 

Konrad

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Dmitry Baryshkov]

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.


-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Konrad Dybcio]

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

Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Dmitry Baryshkov]

On Thu, Apr 24, 2025 at 10:28:58PM +0200, Konrad Dybcio wrote:
> 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.

Kind of. The other issue is several cases where GPU and MDSS drivers
disagree about UBWC config. I think the plan might be:
- Introduce UBWC config database, ruling out incoherences between these
  drivers
- Parse SMEM and the non-standard OF property, verify both against each
  other and against the UBWC database
- One-by-one drop entries from UBWC database as they are verified
  against the readout values.

I understand that it is a long-term plan, but granted the issues we've
had before (tiling screen corruptions, which was fixed / worked around
by lowering HBB) I think this is the most viable path forward.

-- 
With best wishes
Dmitry