* [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
2026-01-08 14:21 [PATCH v3 0/3] Retrieve information about DDR from SMEM Konrad Dybcio
@ 2026-01-08 14:21 ` Konrad Dybcio
2026-01-09 13:36 ` Mukesh Ojha
` (2 more replies)
2026-01-08 14:21 ` [PATCH v3 2/3] soc: qcom: ubwc: Get HBB " Konrad Dybcio
` (4 subsequent siblings)
5 siblings, 3 replies; 33+ messages in thread
From: Konrad Dybcio @ 2026-01-08 14:21 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter
Cc: linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, 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-v7 version of the structs. Unforunately,
they are not versioned, so some elbow grease is necessary to determine
which one is present. See for reference:
ver 3: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/1d11897d2cfcc7b85f28ff74c445018dbbecac7a
ver 4: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/f6e9aa549260bbc0bdcb156c2b05f48dc5963203
ver 5: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/617d3297abe8b1b8dd3de3d1dd69c3961e6f343f
ver 5 with 6regions: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/d770e009f9bae58d56d926f7490bbfb45af8341f
ver 6: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/62659b557fdb1551b20fae8073d1d701dfa8a62e
ver 7: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/734d95599c5ebb1ca0d4e1639142e65c590532b7
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 | 408 ++++++++++++++++++++++++++++++++++++++++++
include/linux/soc/qcom/smem.h | 4 +
5 files changed, 436 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index b7f1d2a57367..798643be3590 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 088b2bbee9e6..a53bf9ed8e92 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);
};
@@ -1236,17 +1241,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);
__smem = NULL;
diff --git a/drivers/soc/qcom/smem.h b/drivers/soc/qcom/smem.h
new file mode 100644
index 000000000000..8bf3f606e1ae
--- /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 000000000000..017bb894a91b
--- /dev/null
+++ b/drivers/soc/qcom/smem_dramc.c
@@ -0,0 +1,408 @@
+// 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_CHAN_NUM 8
+#define MAX_RANK_NUM 2
+
+#define DDR_HBB_MIN 13
+#define DDR_HBB_MAX 19
+
+#define MAX_SHUB_ENTRIES 8
+
+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,
+ INFO_V6, /* INFO_V6 seems to only have shipped with 6 DDR regions, unlike V7 */
+ INFO_V7,
+ INFO_V7_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 shub_freq_table {
+ u8 enable;
+ u32 freq_khz;
+};
+
+struct shub_freq_plan_entry {
+ u8 num_shub_freqs;
+ struct shub_freq_table shub_freq[MAX_SHUB_ENTRIES];
+};
+
+struct ddr_xbl2quantum_smem_data {
+ phys_addr_t ssr_cookie_addr;
+ u32 reserved[10];
+};
+
+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;
+ u8 _padding;
+ struct ddr_regions_v5 ddr_regions;
+};
+
+/* V6 */
+struct ddr_misc_info_v6 {
+ u32 dsf_version;
+ u32 reserved[10];
+};
+
+/* V7 */
+struct ddr_details_v7 {
+ 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;
+ u8 sct_config;
+ 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.
+ *
+ * Return: 0 on success, -ENODATA on failure.
+ */
+int qcom_smem_dram_get_hbb(void)
+{
+ int hbb;
+
+ if (!__dram)
+ return -ENODATA;
+
+ hbb = __dram->hbb;
+ if (hbb == 0)
+ return -ENODATA;
+ else if (hbb < DDR_HBB_MIN || hbb > DDR_HBB_MAX)
+ return -EINVAL;
+
+ return hbb;
+}
+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;
+ }
+}
+
+static void smem_dram_parse_v7_data(struct smem_dram *dram, void *data)
+{
+ struct ddr_details_v7 *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;
+
+ if (size == sizeof(struct ddr_details_v3)
+ + sizeof(struct ddr_freq_table))
+ return INFO_V3_WITH_14_FREQS;
+
+ if (size == sizeof(struct ddr_details_v4))
+ return INFO_V4;
+
+ if (size == sizeof(struct ddr_details_v5)
+ + 4 * sizeof(struct ddr_region_v5))
+ return INFO_V5;
+
+ if (size == sizeof(struct ddr_details_v5)
+ + 4 * sizeof(struct ddr_region_v5)
+ + sizeof(struct ddr_xbl2quantum_smem_data)
+ + sizeof(struct shub_freq_plan_entry))
+ return INFO_V5;
+
+ if (size == sizeof(struct ddr_details_v5)
+ + 6 * sizeof(struct ddr_region_v5))
+ return INFO_V5_WITH_6_REGIONS;
+
+ if (size == sizeof(struct ddr_details_v5)
+ + 6 * sizeof(struct ddr_region_v5)
+ + sizeof(struct ddr_xbl2quantum_smem_data)
+ + sizeof(struct shub_freq_plan_entry))
+ return INFO_V5_WITH_6_REGIONS;
+
+ if (size == sizeof(struct ddr_details_v5)
+ + 6 * sizeof(struct ddr_region_v5)
+ + sizeof(struct ddr_misc_info_v6)
+ + sizeof(struct shub_freq_plan_entry))
+ return INFO_V6;
+
+ if (size == sizeof(struct ddr_details_v7)
+ + 4 * sizeof(struct ddr_region_v5)
+ + sizeof(struct ddr_misc_info_v6)
+ + sizeof(struct shub_freq_plan_entry))
+ return INFO_V7;
+
+ if (size == sizeof(struct ddr_details_v7)
+ + 6 * sizeof(struct ddr_region_v5)
+ + sizeof(struct ddr_misc_info_v6)
+ + sizeof(struct shub_freq_plan_entry))
+ return INFO_V7_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);
+
+static int smem_hbb_show(struct seq_file *s, void *unused)
+{
+ struct smem_dram *dram = s->private;
+
+ if (!dram->hbb)
+ return -EINVAL;
+
+ seq_printf(s, "%d\n", dram->hbb);
+
+ return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(smem_hbb);
+
+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:
+ case INFO_V6:
+ smem_dram_parse_v5_data(dram, data);
+ break;
+ case INFO_V7:
+ case INFO_V7_WITH_6_REGIONS:
+ smem_dram_parse_v7_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);
+ debugfs_create_file("hbb", 0444, debugfs_dir, dram, &smem_hbb_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 f946e3beca21..223cd5090a2a 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.52.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
2026-01-08 14:21 ` [PATCH v3 1/3] soc: qcom: smem: Expose DDR data " Konrad Dybcio
@ 2026-01-09 13:36 ` Mukesh Ojha
2026-01-27 14:22 ` Konrad Dybcio
2026-01-09 18:08 ` Bjorn Andersson
2026-01-14 16:36 ` kernel test robot
2 siblings, 1 reply; 33+ messages in thread
From: Mukesh Ojha @ 2026-01-09 13:36 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Thu, Jan 08, 2026 at 03:21:50PM +0100, Konrad Dybcio wrote:
> 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-v7 version of the structs. Unforunately,
> they are not versioned, so some elbow grease is necessary to determine
> which one is present. See for reference:
>
> ver 3: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/1d11897d2cfcc7b85f28ff74c445018dbbecac7a
> ver 4: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/f6e9aa549260bbc0bdcb156c2b05f48dc5963203
> ver 5: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/617d3297abe8b1b8dd3de3d1dd69c3961e6f343f
> ver 5 with 6regions: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/d770e009f9bae58d56d926f7490bbfb45af8341f
> ver 6: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/62659b557fdb1551b20fae8073d1d701dfa8a62e
> ver 7: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/734d95599c5ebb1ca0d4e1639142e65c590532b7
>
> 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 | 408 ++++++++++++++++++++++++++++++++++++++++++
> include/linux/soc/qcom/smem.h | 4 +
> 5 files changed, 436 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index b7f1d2a57367..798643be3590 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 088b2bbee9e6..a53bf9ed8e92 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);
> };
> @@ -1236,17 +1241,24 @@ static int qcom_smem_probe(struct platform_device *pdev)
>
> __smem = smem;
>
> + smem->debugfs_dir = smem_dram_parse(smem->dev);
Is it possible, even after calling qcom_smem_is_available() before calling
qcom_smem_dram_get_hbb() we are getting __dram as NULL.
is it good to move __smem assignment to the end with barrier so all the
changes before the assignment are seen when somebody checking qcom_smem_is_available()
with a pair smp store/release pair.
> +
> 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);
>
> __smem = NULL;
> diff --git a/drivers/soc/qcom/smem.h b/drivers/soc/qcom/smem.h
> new file mode 100644
> index 000000000000..8bf3f606e1ae
> --- /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 000000000000..017bb894a91b
> --- /dev/null
> +++ b/drivers/soc/qcom/smem_dramc.c
> @@ -0,0 +1,408 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
Year less.. ??
> + */
> +
> +#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>
2nd time..
> +
> +#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_CHAN_NUM 8
> +#define MAX_RANK_NUM 2
> +
> +#define DDR_HBB_MIN 13
> +#define DDR_HBB_MAX 19
> +
> +#define MAX_SHUB_ENTRIES 8
> +
> +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,
> + INFO_V6, /* INFO_V6 seems to only have shipped with 6 DDR regions, unlike V7 */
> + INFO_V7,
> + INFO_V7_WITH_6_REGIONS,
> +};
> +
> +struct smem_dram {
> + unsigned long frequencies[MAX_DDR_FREQ_NUM_V5];
> + u32 num_frequencies;
freq and num_freq_entries ? since you have used freq at various places..
> + 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 shub_freq_table {
> + u8 enable;
> + u32 freq_khz;
> +};
> +
> +struct shub_freq_plan_entry {
> + u8 num_shub_freqs;
> + struct shub_freq_table shub_freq[MAX_SHUB_ENTRIES];
> +};
> +
> +struct ddr_xbl2quantum_smem_data {
> + phys_addr_t ssr_cookie_addr;
> + u32 reserved[10];
> +};
> +
> +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;
> + u8 _padding;
> + struct ddr_regions_v5 ddr_regions;
> +};
> +
> +/* V6 */
> +struct ddr_misc_info_v6 {
> + u32 dsf_version;
> + u32 reserved[10];
> +};
> +
> +/* V7 */
> +struct ddr_details_v7 {
> + 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;
> + u8 sct_config;
> + 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.
> + *
> + * Return: 0 on success, -ENODATA on failure.
> + */
> +int qcom_smem_dram_get_hbb(void)
> +{
> + int hbb;
> +
> + if (!__dram)
> + return -ENODATA;
> +
> + hbb = __dram->hbb;
> + if (hbb == 0)
> + return -ENODATA;
Incase, you want to consider to save some lines..
if (!__dram || !__dram->hbb)
return -ENODATA;
> + else if (hbb < DDR_HBB_MIN || hbb > DDR_HBB_MAX)
> + return -EINVAL;
> +
> + return hbb;
> +}
> +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;
> + }
> +}
> +
> +static void smem_dram_parse_v7_data(struct smem_dram *dram, void *data)
> +{
> + struct ddr_details_v7 *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;
> +
> + if (size == sizeof(struct ddr_details_v3)
> + + sizeof(struct ddr_freq_table))
> + return INFO_V3_WITH_14_FREQS;
> +
> + if (size == sizeof(struct ddr_details_v4))
> + return INFO_V4;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 4 * sizeof(struct ddr_region_v5))
> + return INFO_V5;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 4 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_xbl2quantum_smem_data)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V5;
Why this does not have separate name ?
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5))
> + return INFO_V5_WITH_6_REGIONS;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_xbl2quantum_smem_data)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V5_WITH_6_REGIONS;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V6;
> +
> + if (size == sizeof(struct ddr_details_v7)
> + + 4 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V7;
> +
> + if (size == sizeof(struct ddr_details_v7)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V7_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);
> +
> +static int smem_hbb_show(struct seq_file *s, void *unused)
> +{
> + struct smem_dram *dram = s->private;
> +
> + if (!dram->hbb)
> + return -EINVAL;
> +
> + seq_printf(s, "%d\n", dram->hbb);
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(smem_hbb);
> +
> +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 */
This comment seems redundant..
> + 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:
> + case INFO_V6:
> + smem_dram_parse_v5_data(dram, data);
> + break;
> + case INFO_V7:
> + case INFO_V7_WITH_6_REGIONS:
> + smem_dram_parse_v7_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);
> + debugfs_create_file("hbb", 0444, debugfs_dir, dram, &smem_hbb_fops);
> +
> + /* If there was no failure so far, assign the global variable */
nit: I agree that we should write comments, but sometimes it is too obvious.
This is a simple assignment, not something complex like a barrier that requires
detailed explanation.
> + __dram = dram;
> +
> + return debugfs_dir;
> +}
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index f946e3beca21..223cd5090a2a 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>
> +
Why this ?
> #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.52.0
>
--
-Mukesh Ojha
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
2026-01-09 13:36 ` Mukesh Ojha
@ 2026-01-27 14:22 ` Konrad Dybcio
0 siblings, 0 replies; 33+ messages in thread
From: Konrad Dybcio @ 2026-01-27 14:22 UTC (permalink / raw)
To: Mukesh Ojha, Konrad Dybcio
Cc: Bjorn Andersson, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno
On 1/9/26 2:36 PM, Mukesh Ojha wrote:
> On Thu, Jan 08, 2026 at 03:21:50PM +0100, Konrad Dybcio wrote:
>> 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.
[...]
>> @@ -1236,17 +1241,24 @@ static int qcom_smem_probe(struct platform_device *pdev)
>>
>> __smem = smem;
>>
>> + smem->debugfs_dir = smem_dram_parse(smem->dev);
>
> Is it possible, even after calling qcom_smem_is_available() before calling
> qcom_smem_dram_get_hbb() we are getting __dram as NULL.
>
> is it good to move __smem assignment to the end with barrier so all the
> changes before the assignment are seen when somebody checking qcom_smem_is_available()
> with a pair smp store/release pair.
I think just moving the __smem assignment down will be enough, no?
What scenario do you have in mind that would require SMP barriers?
[...]
>> +struct smem_dram {
>> + unsigned long frequencies[MAX_DDR_FREQ_NUM_V5];
>> + u32 num_frequencies;
>
> freq and num_freq_entries ? since you have used freq at various places..
The names in structs come from internal shmem definitions that I didn't
want to stray away from
Making the kernel-side struct fields named the same feels like added
confusion to me
[...]
>> + if (size == sizeof(struct ddr_details_v5)
>> + + 4 * sizeof(struct ddr_region_v5)
>> + + sizeof(struct ddr_xbl2quantum_smem_data)
>> + + sizeof(struct shub_freq_plan_entry))
>> + return INFO_V5;
>
> Why this does not have separate name ?
Because it's the same DDR info structure as "normal v5", with trailing
extras that we don't really care about
[...]
>> +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 */
>
> This comment seems redundant..
With this one specifically, I don't agree it's obvious..
Konrad
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
2026-01-08 14:21 ` [PATCH v3 1/3] soc: qcom: smem: Expose DDR data " Konrad Dybcio
2026-01-09 13:36 ` Mukesh Ojha
@ 2026-01-09 18:08 ` Bjorn Andersson
2026-01-14 16:36 ` kernel test robot
2 siblings, 0 replies; 33+ messages in thread
From: Bjorn Andersson @ 2026-01-09 18:08 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Kees Cook, Gustavo A. R. Silva, Rob Clark, Sean Paul,
Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Marijn Suijten, David Airlie, Simona Vetter, linux-kernel,
linux-arm-msm, linux-hardening, dri-devel, freedreno,
Konrad Dybcio
On Thu, Jan 08, 2026 at 03:21:50PM +0100, Konrad Dybcio wrote:
> diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
[..]
> +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;
Aren't all these structs encoded in little endian? __leXX?
> + struct ddr_region_v5 ddr_region[] __counted_by(ddr_region_num);
Was going to joke about this one, but realized that there's a
__counted_by_le()
> +};
> +
> +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;
> + u8 _padding;
> + struct ddr_regions_v5 ddr_regions;
> +};
> +
> +/* V6 */
> +struct ddr_misc_info_v6 {
> + u32 dsf_version;
> + u32 reserved[10];
> +};
> +
> +/* V7 */
> +struct ddr_details_v7 {
> + 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;
> + u8 sct_config;
> + 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.
> + *
> + * Return: 0 on success, -ENODATA on failure.
Seems more like "highest bank bit on success, -ENODATA on failure.
> + */
> +int qcom_smem_dram_get_hbb(void)
> +{
> + int hbb;
> +
> + if (!__dram)
> + return -ENODATA;
> +
> + hbb = __dram->hbb;
> + if (hbb == 0)
> + return -ENODATA;
> + else if (hbb < DDR_HBB_MIN || hbb > DDR_HBB_MAX)
> + return -EINVAL;
Not really "Invalid argument", -ENODATA is probably better here as well.
> +
> + return hbb;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_dram_get_hbb);
> +
[..]
> +/* 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;
> +
> + if (size == sizeof(struct ddr_details_v3)
> + + sizeof(struct ddr_freq_table))
Don't you find it weird to have the + after the wrap?
> + return INFO_V3_WITH_14_FREQS;
> +
> + if (size == sizeof(struct ddr_details_v4))
> + return INFO_V4;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 4 * sizeof(struct ddr_region_v5))
> + return INFO_V5;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 4 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_xbl2quantum_smem_data)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V5;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5))
> + return INFO_V5_WITH_6_REGIONS;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_xbl2quantum_smem_data)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V5_WITH_6_REGIONS;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V6;
> +
> + if (size == sizeof(struct ddr_details_v7)
> + + 4 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V7;
> +
> + if (size == sizeof(struct ddr_details_v7)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V7_WITH_6_REGIONS;
> +
> + return INFO_UNKNOWN;
> +}
> +
[..]
> +
> +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);
Is there a reason why this isn't dev_err(dev, ...)?
> + 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:
> + case INFO_V6:
> + smem_dram_parse_v5_data(dram, data);
> + break;
> + case INFO_V7:
> + case INFO_V7_WITH_6_REGIONS:
> + smem_dram_parse_v7_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);
> + debugfs_create_file("hbb", 0444, debugfs_dir, dram, &smem_hbb_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 f946e3beca21..223cd5090a2a 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>
I'm not able to see why.
Regards,
Bjorn
> +
> #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.52.0
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
2026-01-08 14:21 ` [PATCH v3 1/3] soc: qcom: smem: Expose DDR data " Konrad Dybcio
2026-01-09 13:36 ` Mukesh Ojha
2026-01-09 18:08 ` Bjorn Andersson
@ 2026-01-14 16:36 ` kernel test robot
2 siblings, 0 replies; 33+ messages in thread
From: kernel test robot @ 2026-01-14 16:36 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter
Cc: oe-kbuild-all, linux-kernel, linux-arm-msm, linux-hardening,
dri-devel, freedreno
Hi Konrad,
kernel test robot noticed the following build warnings:
[auto build test WARNING on fc4e91c639c0af93d63c3d5bc0ee45515dd7504a]
url: https://github.com/intel-lab-lkp/linux/commits/Konrad-Dybcio/soc-qcom-smem-Expose-DDR-data-from-SMEM/20260108-222445
base: fc4e91c639c0af93d63c3d5bc0ee45515dd7504a
patch link: https://lore.kernel.org/r/20260108-topic-smem_dramc-v3-1-6b64df58a017%40oss.qualcomm.com
patch subject: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20260115/202601150105.Pod3agMP-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260115/202601150105.Pod3agMP-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601150105.Pod3agMP-lkp@intel.com/
All warnings (new ones prefixed by >>):
In function 'smem_dram_parse_v3_data',
inlined from 'smem_dram_parse' at drivers/soc/qcom/smem_dramc.c:380:3:
>> drivers/soc/qcom/smem_dramc.c:216:31: warning: iteration 13 invokes undefined behavior [-Waggressive-loop-optimizations]
216 | if (freq_entry->freq_khz && freq_entry->enabled)
| ~~~~~~~~~~^~~~~~~~~~
drivers/soc/qcom/smem_dramc.c:213:27: note: within this loop
213 | for (int i = 0; i < num_freq_entries; i++) {
| ~~^~~~~~~~~~~~~~~~~~
--
>> Warning: drivers/soc/qcom/smem.c:293 struct member 'debugfs_dir' not described in 'qcom_smem'
>> Warning: drivers/soc/qcom/smem.c:293 struct member 'debugfs_dir' not described in 'qcom_smem'
vim +216 drivers/soc/qcom/smem_dramc.c
203
204 static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data, bool additional_freq_entry)
205 {
206 /* This may be 13 or 14 */
207 int num_freq_entries = MAX_DDR_FREQ_NUM_V3;
208 struct ddr_details_v3 *details = data;
209
210 if (additional_freq_entry)
211 num_freq_entries++;
212
213 for (int i = 0; i < num_freq_entries; i++) {
214 struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
215
> 216 if (freq_entry->freq_khz && freq_entry->enabled)
217 dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
218 }
219 }
220
221 static void smem_dram_parse_v4_data(struct smem_dram *dram, void *data)
222 {
223 struct ddr_details_v4 *details = data;
224
225 /* Rank 0 channel 0 entry holds the correct value */
226 dram->hbb = details->highest_bank_addr_bit[0][0];
227
228 for (int i = 0; i < MAX_DDR_FREQ_NUM_V3; i++) {
229 struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
230
231 if (freq_entry->freq_khz && freq_entry->enabled)
232 dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
233 }
234 }
235
236 static void smem_dram_parse_v5_data(struct smem_dram *dram, void *data)
237 {
238 struct ddr_details_v5 *details = data;
239 struct ddr_regions_v5 *region = &details->ddr_regions;
240
241 dram->hbb = region[0].highest_bank_addr_bit;
242
243 for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
244 struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
245
246 if (freq_entry->freq_khz && freq_entry->enabled)
247 dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
248 }
249 }
250
251 static void smem_dram_parse_v7_data(struct smem_dram *dram, void *data)
252 {
253 struct ddr_details_v7 *details = data;
254 struct ddr_regions_v5 *region = &details->ddr_regions;
255
256 dram->hbb = region[0].highest_bank_addr_bit;
257
258 for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
259 struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
260
261 if (freq_entry->freq_khz && freq_entry->enabled)
262 dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
263 }
264 }
265
266 /* The structure contains no version field, so we have to perform some guesswork.. */
267 static int smem_dram_infer_struct_version(size_t size)
268 {
269 /* Some early versions provided less bytes of less useful data */
270 if (size < sizeof(struct ddr_details_v3))
271 return -EINVAL;
272
273 if (size == sizeof(struct ddr_details_v3))
274 return INFO_V3;
275
276 if (size == sizeof(struct ddr_details_v3)
277 + sizeof(struct ddr_freq_table))
278 return INFO_V3_WITH_14_FREQS;
279
280 if (size == sizeof(struct ddr_details_v4))
281 return INFO_V4;
282
283 if (size == sizeof(struct ddr_details_v5)
284 + 4 * sizeof(struct ddr_region_v5))
285 return INFO_V5;
286
287 if (size == sizeof(struct ddr_details_v5)
288 + 4 * sizeof(struct ddr_region_v5)
289 + sizeof(struct ddr_xbl2quantum_smem_data)
290 + sizeof(struct shub_freq_plan_entry))
291 return INFO_V5;
292
293 if (size == sizeof(struct ddr_details_v5)
294 + 6 * sizeof(struct ddr_region_v5))
295 return INFO_V5_WITH_6_REGIONS;
296
297 if (size == sizeof(struct ddr_details_v5)
298 + 6 * sizeof(struct ddr_region_v5)
299 + sizeof(struct ddr_xbl2quantum_smem_data)
300 + sizeof(struct shub_freq_plan_entry))
301 return INFO_V5_WITH_6_REGIONS;
302
303 if (size == sizeof(struct ddr_details_v5)
304 + 6 * sizeof(struct ddr_region_v5)
305 + sizeof(struct ddr_misc_info_v6)
306 + sizeof(struct shub_freq_plan_entry))
307 return INFO_V6;
308
309 if (size == sizeof(struct ddr_details_v7)
310 + 4 * sizeof(struct ddr_region_v5)
311 + sizeof(struct ddr_misc_info_v6)
312 + sizeof(struct shub_freq_plan_entry))
313 return INFO_V7;
314
315 if (size == sizeof(struct ddr_details_v7)
316 + 6 * sizeof(struct ddr_region_v5)
317 + sizeof(struct ddr_misc_info_v6)
318 + sizeof(struct shub_freq_plan_entry))
319 return INFO_V7_WITH_6_REGIONS;
320
321 return INFO_UNKNOWN;
322 }
323
324 static int smem_dram_frequencies_show(struct seq_file *s, void *unused)
325 {
326 struct smem_dram *dram = s->private;
327
328 for (int i = 0; i < dram->num_frequencies; i++)
329 seq_printf(s, "%lu\n", dram->frequencies[i]);
330
331 return 0;
332 }
333 DEFINE_SHOW_ATTRIBUTE(smem_dram_frequencies);
334
335 static int smem_hbb_show(struct seq_file *s, void *unused)
336 {
337 struct smem_dram *dram = s->private;
338
339 if (!dram->hbb)
340 return -EINVAL;
341
342 seq_printf(s, "%d\n", dram->hbb);
343
344 return 0;
345 }
346 DEFINE_SHOW_ATTRIBUTE(smem_hbb);
347
348 struct dentry *smem_dram_parse(struct device *dev)
349 {
350 struct dentry *debugfs_dir;
351 enum ddr_info_version ver;
352 struct smem_dram *dram;
353 size_t actual_size;
354 void *data = NULL;
355
356 /* No need to check qcom_smem_is_available(), this func is called by the SMEM driver */
357 data = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_DDR_INFO_ID, &actual_size);
358 if (IS_ERR_OR_NULL(data))
359 return ERR_PTR(-ENODATA);
360
361 ver = smem_dram_infer_struct_version(actual_size);
362 if (ver < 0) {
363 /* Some SoCs don't provide data that's useful for us */
364 return ERR_PTR(-ENODATA);
365 } else if (ver == INFO_UNKNOWN) {
366 /* In other cases, we may not have added support for a newer struct revision */
367 pr_err("Found an unknown type of DRAM info struct (size = %zu)\n", actual_size);
368 return ERR_PTR(-EINVAL);
369 }
370
371 dram = devm_kzalloc(dev, sizeof(*dram), GFP_KERNEL);
372 if (!dram)
373 return ERR_PTR(-ENOMEM);
374
375 switch (ver) {
376 case INFO_V3:
377 smem_dram_parse_v3_data(dram, data, false);
378 break;
379 case INFO_V3_WITH_14_FREQS:
> 380 smem_dram_parse_v3_data(dram, data, true);
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-08 14:21 [PATCH v3 0/3] Retrieve information about DDR from SMEM Konrad Dybcio
2026-01-08 14:21 ` [PATCH v3 1/3] soc: qcom: smem: Expose DDR data " Konrad Dybcio
@ 2026-01-08 14:21 ` Konrad Dybcio
2026-01-08 14:45 ` Dmitry Baryshkov
2026-01-08 14:21 ` [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config Konrad Dybcio
` (3 subsequent siblings)
5 siblings, 1 reply; 33+ messages in thread
From: Konrad Dybcio @ 2026-01-08 14:21 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter
Cc: linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
To make sure the correct settings for a given DRAM configuration get
applied, attempt to retrieve that data from SMEM (which happens to be
what the BSP kernel does, albeit with through convoluted means of the
bootloader altering the DT with this data).
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
I'm not sure about this approach - perhaps a global variable storing
the selected config, which would then be non-const would be better?
---
drivers/soc/qcom/ubwc_config.c | 69 ++++++++++++++++++++++++++----------------
1 file changed, 43 insertions(+), 26 deletions(-)
diff --git a/drivers/soc/qcom/ubwc_config.c b/drivers/soc/qcom/ubwc_config.c
index 1c25aaf55e52..21bb444dc27c 100644
--- a/drivers/soc/qcom/ubwc_config.c
+++ b/drivers/soc/qcom/ubwc_config.c
@@ -11,12 +11,13 @@
#include <linux/platform_device.h>
#include <linux/soc/qcom/ubwc.h>
+#include <linux/soc/qcom/smem.h>
-static const struct qcom_ubwc_cfg_data no_ubwc_data = {
+static struct qcom_ubwc_cfg_data no_ubwc_data = {
/* no UBWC, no HBB */
};
-static const struct qcom_ubwc_cfg_data kaanapali_data = {
+static struct qcom_ubwc_cfg_data kaanapali_data = {
.ubwc_enc_version = UBWC_6_0,
.ubwc_dec_version = UBWC_6_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -26,7 +27,7 @@ static const struct qcom_ubwc_cfg_data kaanapali_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data msm8937_data = {
+static struct qcom_ubwc_cfg_data msm8937_data = {
.ubwc_enc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_1_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL1 |
@@ -35,7 +36,7 @@ static const struct qcom_ubwc_cfg_data msm8937_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data msm8998_data = {
+static struct qcom_ubwc_cfg_data msm8998_data = {
.ubwc_enc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_1_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL1 |
@@ -44,12 +45,12 @@ static const struct qcom_ubwc_cfg_data msm8998_data = {
.highest_bank_bit = 15,
};
-static const struct qcom_ubwc_cfg_data qcm2290_data = {
+static struct qcom_ubwc_cfg_data qcm2290_data = {
/* no UBWC */
.highest_bank_bit = 15,
};
-static const struct qcom_ubwc_cfg_data sa8775p_data = {
+static struct qcom_ubwc_cfg_data sa8775p_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL3,
@@ -58,7 +59,7 @@ static const struct qcom_ubwc_cfg_data sa8775p_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sar2130p_data = {
+static struct qcom_ubwc_cfg_data sar2130p_data = {
.ubwc_enc_version = UBWC_3_0, /* 4.0.2 in hw */
.ubwc_dec_version = UBWC_4_3,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -68,7 +69,7 @@ static const struct qcom_ubwc_cfg_data sar2130p_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sc7180_data = {
+static struct qcom_ubwc_cfg_data sc7180_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -77,7 +78,7 @@ static const struct qcom_ubwc_cfg_data sc7180_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sc7280_data = {
+static struct qcom_ubwc_cfg_data sc7280_data = {
.ubwc_enc_version = UBWC_3_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -87,7 +88,7 @@ static const struct qcom_ubwc_cfg_data sc7280_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sc8180x_data = {
+static struct qcom_ubwc_cfg_data sc8180x_data = {
.ubwc_enc_version = UBWC_3_0,
.ubwc_dec_version = UBWC_3_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -96,7 +97,7 @@ static const struct qcom_ubwc_cfg_data sc8180x_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sc8280xp_data = {
+static struct qcom_ubwc_cfg_data sc8280xp_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -106,7 +107,7 @@ static const struct qcom_ubwc_cfg_data sc8280xp_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sdm670_data = {
+static struct qcom_ubwc_cfg_data sdm670_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -114,7 +115,7 @@ static const struct qcom_ubwc_cfg_data sdm670_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sdm845_data = {
+static struct qcom_ubwc_cfg_data sdm845_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -122,7 +123,7 @@ static const struct qcom_ubwc_cfg_data sdm845_data = {
.highest_bank_bit = 15,
};
-static const struct qcom_ubwc_cfg_data sm6115_data = {
+static struct qcom_ubwc_cfg_data sm6115_data = {
.ubwc_enc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL1 |
@@ -132,7 +133,7 @@ static const struct qcom_ubwc_cfg_data sm6115_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm6125_data = {
+static struct qcom_ubwc_cfg_data sm6125_data = {
.ubwc_enc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_3_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL1 |
@@ -141,7 +142,7 @@ static const struct qcom_ubwc_cfg_data sm6125_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm6150_data = {
+static struct qcom_ubwc_cfg_data sm6150_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -149,7 +150,7 @@ static const struct qcom_ubwc_cfg_data sm6150_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm6350_data = {
+static struct qcom_ubwc_cfg_data sm6350_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -158,7 +159,7 @@ static const struct qcom_ubwc_cfg_data sm6350_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm7150_data = {
+static struct qcom_ubwc_cfg_data sm7150_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -166,7 +167,7 @@ static const struct qcom_ubwc_cfg_data sm7150_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm8150_data = {
+static struct qcom_ubwc_cfg_data sm8150_data = {
.ubwc_enc_version = UBWC_3_0,
.ubwc_dec_version = UBWC_3_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -174,7 +175,7 @@ static const struct qcom_ubwc_cfg_data sm8150_data = {
.highest_bank_bit = 15,
};
-static const struct qcom_ubwc_cfg_data sm8250_data = {
+static struct qcom_ubwc_cfg_data sm8250_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -185,7 +186,7 @@ static const struct qcom_ubwc_cfg_data sm8250_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sm8350_data = {
+static struct qcom_ubwc_cfg_data sm8350_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -196,7 +197,7 @@ static const struct qcom_ubwc_cfg_data sm8350_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sm8550_data = {
+static struct qcom_ubwc_cfg_data sm8550_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_3,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -207,7 +208,7 @@ static const struct qcom_ubwc_cfg_data sm8550_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sm8750_data = {
+static struct qcom_ubwc_cfg_data sm8750_data = {
.ubwc_enc_version = UBWC_5_0,
.ubwc_dec_version = UBWC_5_0,
.ubwc_swizzle = 6,
@@ -217,7 +218,7 @@ static const struct qcom_ubwc_cfg_data sm8750_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data x1e80100_data = {
+static struct qcom_ubwc_cfg_data x1e80100_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_3,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -301,14 +302,30 @@ static const struct of_device_id qcom_ubwc_configs[] __maybe_unused = {
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
{
- const struct qcom_ubwc_cfg_data *data;
+ struct qcom_ubwc_cfg_data *data;
+ int hbb;
- data = of_machine_get_match_data(qcom_ubwc_configs);
+ if (!qcom_smem_is_available())
+ return ERR_PTR(-EPROBE_DEFER);
+
+ /* Discard the const qualifier, but still return a const pointer to consumers */
+ data = (struct qcom_ubwc_cfg_data *)of_machine_get_match_data(qcom_ubwc_configs);
if (!data) {
pr_err("Couldn't find UBWC config data for this platform!\n");
return ERR_PTR(-EINVAL);
}
+ hbb = qcom_smem_dram_get_hbb();
+ if (hbb == -ENODATA) {
+ /* Lack of HBB data is OK - it was only introduced later */
+ return data;
+ } else if (hbb < 0) {
+ pr_err("Couldn't get HBB data from SMEM: %d\n", hbb);
+ return ERR_PTR(hbb);
+ }
+
+ data->highest_bank_bit = hbb;
+
return data;
}
EXPORT_SYMBOL_GPL(qcom_ubwc_config_get_data);
--
2.52.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-08 14:21 ` [PATCH v3 2/3] soc: qcom: ubwc: Get HBB " Konrad Dybcio
@ 2026-01-08 14:45 ` Dmitry Baryshkov
2026-01-08 17:49 ` Bjorn Andersson
0 siblings, 1 reply; 33+ messages in thread
From: Dmitry Baryshkov @ 2026-01-08 14:45 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> To make sure the correct settings for a given DRAM configuration get
> applied, attempt to retrieve that data from SMEM (which happens to be
> what the BSP kernel does, albeit with through convoluted means of the
> bootloader altering the DT with this data).
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> ---
> I'm not sure about this approach - perhaps a global variable storing
> the selected config, which would then be non-const would be better?
I'd prefer if const data was const, split HBB to a separate API.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-08 14:45 ` Dmitry Baryshkov
@ 2026-01-08 17:49 ` Bjorn Andersson
2026-01-09 3:21 ` Dmitry Baryshkov
2026-01-13 15:36 ` Konrad Dybcio
0 siblings, 2 replies; 33+ messages in thread
From: Bjorn Andersson @ 2026-01-08 17:49 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Konrad Dybcio, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >
> > To make sure the correct settings for a given DRAM configuration get
> > applied, attempt to retrieve that data from SMEM (which happens to be
> > what the BSP kernel does, albeit with through convoluted means of the
> > bootloader altering the DT with this data).
> >
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >
> > ---
> > I'm not sure about this approach - perhaps a global variable storing
> > the selected config, which would then be non-const would be better?
>
> I'd prefer if const data was const, split HBB to a separate API.
>
I agree, but I'd prefer to avoid a separate API for it.
Instead I'd like to either return the struct by value (after updating
the hbb), but we then loose the ability to return errors, or by changing
the signature to:
int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
This costs us an additional 16 bytes in each client (as the pointer is
replaced with the data), but I think it's a cleaner API.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-08 17:49 ` Bjorn Andersson
@ 2026-01-09 3:21 ` Dmitry Baryshkov
2026-01-09 17:50 ` Bjorn Andersson
2026-01-13 15:36 ` Konrad Dybcio
1 sibling, 1 reply; 33+ messages in thread
From: Dmitry Baryshkov @ 2026-01-09 3:21 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Konrad Dybcio, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> > On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > >
> > > To make sure the correct settings for a given DRAM configuration get
> > > applied, attempt to retrieve that data from SMEM (which happens to be
> > > what the BSP kernel does, albeit with through convoluted means of the
> > > bootloader altering the DT with this data).
> > >
> > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > >
> > > ---
> > > I'm not sure about this approach - perhaps a global variable storing
> > > the selected config, which would then be non-const would be better?
> >
> > I'd prefer if const data was const, split HBB to a separate API.
> >
>
> I agree, but I'd prefer to avoid a separate API for it.
>
> Instead I'd like to either return the struct by value (after updating
> the hbb), but we then loose the ability to return errors, or by changing
> the signature to:
>
> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
>
> This costs us an additional 16 bytes in each client (as the pointer is
> replaced with the data), but I think it's a cleaner API.
What about:
const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
I really want to keep the data as const and, as important, use it as a
const pointer.
>
> Regards,
> Bjorn
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-09 3:21 ` Dmitry Baryshkov
@ 2026-01-09 17:50 ` Bjorn Andersson
2026-01-10 10:45 ` Dmitry Baryshkov
0 siblings, 1 reply; 33+ messages in thread
From: Bjorn Andersson @ 2026-01-09 17:50 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Konrad Dybcio, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> > On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> > > On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> > > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > >
> > > > To make sure the correct settings for a given DRAM configuration get
> > > > applied, attempt to retrieve that data from SMEM (which happens to be
> > > > what the BSP kernel does, albeit with through convoluted means of the
> > > > bootloader altering the DT with this data).
> > > >
> > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > >
> > > > ---
> > > > I'm not sure about this approach - perhaps a global variable storing
> > > > the selected config, which would then be non-const would be better?
> > >
> > > I'd prefer if const data was const, split HBB to a separate API.
> > >
> >
> > I agree, but I'd prefer to avoid a separate API for it.
> >
> > Instead I'd like to either return the struct by value (after updating
> > the hbb), but we then loose the ability to return errors, or by changing
> > the signature to:
> >
> > int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> >
> > This costs us an additional 16 bytes in each client (as the pointer is
> > replaced with the data), but I think it's a cleaner API.
>
> What about:
>
> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
>
> I really want to keep the data as const and, as important, use it as a
> const pointer.
>
I guess the question is what are you actually trying to achive; my goal
was to keep the base data constant, but I'm guessing that you also want
to retain the "const" classifier in the client's context struct (e.g.
the "mdss" member in struct dpu_kms)
If we're returning the data by value, there's no way for you to mark
it as "const" in the calling code's context object (as by definition you
shouldn't be able to change the value after initializing the object).
You also can't return the data by value and then track it by reference -
as that value lives on the stack. This has the benefit of making the
lifecycle of that object clear (it lives in each client) - but perhaps
not a goal of ours...
How come the ubwc config is const but the hbb isn't?
If we want both the per-target data to remain const and data in the
client's context to be carrying the const qualifier, the one solution I
can see is:
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
{
const struct qcom_ubwc_cfg_data *data;
static struct qcom_ubwc_cfg_data cfg;
int hbb;
...
data = of_machine_get_match_data(qcom_ubwc_configs);
...
hbb = qcom_smem_dram_get_hbb();
...
cfg = *data;
cfg.highest_bank_bit = hbb;
return &cfg;
}
But we'd need to deal with the race in cfg assignment...
Regards,
Bjorn
> >
> > Regards,
> > Bjorn
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-09 17:50 ` Bjorn Andersson
@ 2026-01-10 10:45 ` Dmitry Baryshkov
2026-01-13 15:31 ` Konrad Dybcio
0 siblings, 1 reply; 33+ messages in thread
From: Dmitry Baryshkov @ 2026-01-10 10:45 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Konrad Dybcio, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
> > On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> > > On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> > > > On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> > > > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > > >
> > > > > To make sure the correct settings for a given DRAM configuration get
> > > > > applied, attempt to retrieve that data from SMEM (which happens to be
> > > > > what the BSP kernel does, albeit with through convoluted means of the
> > > > > bootloader altering the DT with this data).
> > > > >
> > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > > >
> > > > > ---
> > > > > I'm not sure about this approach - perhaps a global variable storing
> > > > > the selected config, which would then be non-const would be better?
> > > >
> > > > I'd prefer if const data was const, split HBB to a separate API.
> > > >
> > >
> > > I agree, but I'd prefer to avoid a separate API for it.
> > >
> > > Instead I'd like to either return the struct by value (after updating
> > > the hbb), but we then loose the ability to return errors, or by changing
> > > the signature to:
> > >
> > > int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> > >
> > > This costs us an additional 16 bytes in each client (as the pointer is
> > > replaced with the data), but I think it's a cleaner API.
> >
> > What about:
> >
> > const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
> >
> > I really want to keep the data as const and, as important, use it as a
> > const pointer.
> >
>
> I guess the question is what are you actually trying to achive; my goal
> was to keep the base data constant, but I'm guessing that you also want
> to retain the "const" classifier in the client's context struct (e.g.
> the "mdss" member in struct dpu_kms)
>
> If we're returning the data by value, there's no way for you to mark
> it as "const" in the calling code's context object (as by definition you
> shouldn't be able to change the value after initializing the object).
And I, of course, misssed one star:
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
This leaks the knowledge that HBB is slightly different kind of property
than the rest of UBWC data.
>
> You also can't return the data by value and then track it by reference -
> as that value lives on the stack. This has the benefit of making the
> lifecycle of that object clear (it lives in each client) - but perhaps
> not a goal of ours...
>
> How come the ubwc config is const but the hbb isn't?
>
>
> If we want both the per-target data to remain const and data in the
> client's context to be carrying the const qualifier, the one solution I
> can see is:
>
> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> {
> const struct qcom_ubwc_cfg_data *data;
> static struct qcom_ubwc_cfg_data cfg;
> int hbb;
>
> ...
>
> data = of_machine_get_match_data(qcom_ubwc_configs);
> ...
>
> hbb = qcom_smem_dram_get_hbb();
> ...
>
> cfg = *data;
> cfg.highest_bank_bit = hbb;
>
> return &cfg;
> }
>
> But we'd need to deal with the race in cfg assignment...
static struct qcom_ubwc_cfg_data *cfg;
static DEFINE_MUTEX(cfg_mutex);
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
{
const struct qcom_ubwc_cfg_data *data;
int hbb;
guard(mutex)(&cfg_mutex);
if (cfg)
return cfg;
data = of_machine_get_match_data(qcom_ubwc_configs);
if (!data)
return ERR_PTR(-ENOMEM);
hbb = qcom_smem_dram_get_hbb();
if (hbb = -ENODATA)
hbb = 15; /* I think it was default */
else if (hbb < 0)
return ERR_PTR(hbb);
cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
if (!cfg)
return ERR_PTR(-ENOMEM);
cfg->highest_bank_bit = hbb;
return cfg;
}
This potentially leaks sizeof(*data) memory if the module gets removed.
Granted that all users also use qcom_ubwc_config_get_data() symbol, it
should be safe to kfree(cfg) on module removal.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-10 10:45 ` Dmitry Baryshkov
@ 2026-01-13 15:31 ` Konrad Dybcio
2026-01-13 16:29 ` Dmitry Baryshkov
0 siblings, 1 reply; 33+ messages in thread
From: Konrad Dybcio @ 2026-01-13 15:31 UTC (permalink / raw)
To: Dmitry Baryshkov, Bjorn Andersson
Cc: Konrad Dybcio, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno
On 1/10/26 11:45 AM, Dmitry Baryshkov wrote:
> On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
>> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
>>> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
>>>> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
>>>>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>
>>>>>> To make sure the correct settings for a given DRAM configuration get
>>>>>> applied, attempt to retrieve that data from SMEM (which happens to be
>>>>>> what the BSP kernel does, albeit with through convoluted means of the
>>>>>> bootloader altering the DT with this data).
>>>>>>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>
>>>>>> ---
>>>>>> I'm not sure about this approach - perhaps a global variable storing
>>>>>> the selected config, which would then be non-const would be better?
>>>>>
>>>>> I'd prefer if const data was const, split HBB to a separate API.
>>>>>
>>>>
>>>> I agree, but I'd prefer to avoid a separate API for it.
>>>>
>>>> Instead I'd like to either return the struct by value (after updating
>>>> the hbb), but we then loose the ability to return errors, or by changing
>>>> the signature to:
>>>>
>>>> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
>>>>
>>>> This costs us an additional 16 bytes in each client (as the pointer is
>>>> replaced with the data), but I think it's a cleaner API.
>>>
>>> What about:
>>>
>>> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
>>>
>>> I really want to keep the data as const and, as important, use it as a
>>> const pointer.
>>>
>>
>> I guess the question is what are you actually trying to achive; my goal
>> was to keep the base data constant, but I'm guessing that you also want
>> to retain the "const" classifier in the client's context struct (e.g.
>> the "mdss" member in struct dpu_kms)
>>
>> If we're returning the data by value, there's no way for you to mark
>> it as "const" in the calling code's context object (as by definition you
>> shouldn't be able to change the value after initializing the object).
>
> And I, of course, misssed one star:
>
> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
>
> This leaks the knowledge that HBB is slightly different kind of property
> than the rest of UBWC data.
>
>>
>> You also can't return the data by value and then track it by reference -
>> as that value lives on the stack. This has the benefit of making the
>> lifecycle of that object clear (it lives in each client) - but perhaps
>> not a goal of ours...
>>
>> How come the ubwc config is const but the hbb isn't?
>>
>>
>> If we want both the per-target data to remain const and data in the
>> client's context to be carrying the const qualifier, the one solution I
>> can see is:
>>
>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
>> {
>> const struct qcom_ubwc_cfg_data *data;
>> static struct qcom_ubwc_cfg_data cfg;
>> int hbb;
>>
>> ...
>>
>> data = of_machine_get_match_data(qcom_ubwc_configs);
>> ...
>>
>> hbb = qcom_smem_dram_get_hbb();
>> ...
>>
>> cfg = *data;
>> cfg.highest_bank_bit = hbb;
>>
>> return &cfg;
>> }
>>
>> But we'd need to deal with the race in cfg assignment...
>
> static struct qcom_ubwc_cfg_data *cfg;
> static DEFINE_MUTEX(cfg_mutex);
> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> {
> const struct qcom_ubwc_cfg_data *data;
> int hbb;
>
> guard(mutex)(&cfg_mutex);
>
> if (cfg)
> return cfg;
>
> data = of_machine_get_match_data(qcom_ubwc_configs);
> if (!data)
> return ERR_PTR(-ENOMEM);
>
> hbb = qcom_smem_dram_get_hbb();
> if (hbb = -ENODATA)
> hbb = 15; /* I think it was default */
> else if (hbb < 0)
> return ERR_PTR(hbb);
>
> cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
> if (!cfg)
> return ERR_PTR(-ENOMEM);
>
> cfg->highest_bank_bit = hbb;
>
> return cfg;
> }
>
> This potentially leaks sizeof(*data) memory if the module gets removed.
> Granted that all users also use qcom_ubwc_config_get_data() symbol, it
> should be safe to kfree(cfg) on module removal.
I really don't understand why you'd want a separate API for hbb, if
hbb is already available from the larger struct *and* if a driver needs
to know about the value of hbb, it really needs to know about all the
other values as well
Konrad
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-13 15:31 ` Konrad Dybcio
@ 2026-01-13 16:29 ` Dmitry Baryshkov
2026-02-17 12:59 ` Konrad Dybcio
0 siblings, 1 reply; 33+ messages in thread
From: Dmitry Baryshkov @ 2026-01-13 16:29 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Konrad Dybcio, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter, linux-kernel, linux-arm-msm, linux-hardening,
dri-devel, freedreno
On Tue, Jan 13, 2026 at 04:31:15PM +0100, Konrad Dybcio wrote:
> On 1/10/26 11:45 AM, Dmitry Baryshkov wrote:
> > On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
> >> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
> >>> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> >>>> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> >>>>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> >>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>
> >>>>>> To make sure the correct settings for a given DRAM configuration get
> >>>>>> applied, attempt to retrieve that data from SMEM (which happens to be
> >>>>>> what the BSP kernel does, albeit with through convoluted means of the
> >>>>>> bootloader altering the DT with this data).
> >>>>>>
> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>
> >>>>>> ---
> >>>>>> I'm not sure about this approach - perhaps a global variable storing
> >>>>>> the selected config, which would then be non-const would be better?
> >>>>>
> >>>>> I'd prefer if const data was const, split HBB to a separate API.
> >>>>>
> >>>>
> >>>> I agree, but I'd prefer to avoid a separate API for it.
> >>>>
> >>>> Instead I'd like to either return the struct by value (after updating
> >>>> the hbb), but we then loose the ability to return errors, or by changing
> >>>> the signature to:
> >>>>
> >>>> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> >>>>
> >>>> This costs us an additional 16 bytes in each client (as the pointer is
> >>>> replaced with the data), but I think it's a cleaner API.
> >>>
> >>> What about:
> >>>
> >>> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
> >>>
> >>> I really want to keep the data as const and, as important, use it as a
> >>> const pointer.
> >>>
> >>
> >> I guess the question is what are you actually trying to achive; my goal
> >> was to keep the base data constant, but I'm guessing that you also want
> >> to retain the "const" classifier in the client's context struct (e.g.
> >> the "mdss" member in struct dpu_kms)
> >>
> >> If we're returning the data by value, there's no way for you to mark
> >> it as "const" in the calling code's context object (as by definition you
> >> shouldn't be able to change the value after initializing the object).
> >
> > And I, of course, misssed one star:
> >
> > const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
> >
> > This leaks the knowledge that HBB is slightly different kind of property
> > than the rest of UBWC data.
> >
> >>
> >> You also can't return the data by value and then track it by reference -
> >> as that value lives on the stack. This has the benefit of making the
> >> lifecycle of that object clear (it lives in each client) - but perhaps
> >> not a goal of ours...
> >>
> >> How come the ubwc config is const but the hbb isn't?
> >>
> >>
> >> If we want both the per-target data to remain const and data in the
> >> client's context to be carrying the const qualifier, the one solution I
> >> can see is:
> >>
> >> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> >> {
> >> const struct qcom_ubwc_cfg_data *data;
> >> static struct qcom_ubwc_cfg_data cfg;
> >> int hbb;
> >>
> >> ...
> >>
> >> data = of_machine_get_match_data(qcom_ubwc_configs);
> >> ...
> >>
> >> hbb = qcom_smem_dram_get_hbb();
> >> ...
> >>
> >> cfg = *data;
> >> cfg.highest_bank_bit = hbb;
> >>
> >> return &cfg;
> >> }
> >>
> >> But we'd need to deal with the race in cfg assignment...
> >
> > static struct qcom_ubwc_cfg_data *cfg;
> > static DEFINE_MUTEX(cfg_mutex);
> > const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> > {
> > const struct qcom_ubwc_cfg_data *data;
> > int hbb;
> >
> > guard(mutex)(&cfg_mutex);
> >
> > if (cfg)
> > return cfg;
> >
> > data = of_machine_get_match_data(qcom_ubwc_configs);
> > if (!data)
> > return ERR_PTR(-ENOMEM);
> >
> > hbb = qcom_smem_dram_get_hbb();
> > if (hbb = -ENODATA)
> > hbb = 15; /* I think it was default */
> > else if (hbb < 0)
> > return ERR_PTR(hbb);
> >
> > cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
> > if (!cfg)
> > return ERR_PTR(-ENOMEM);
> >
> > cfg->highest_bank_bit = hbb;
> >
> > return cfg;
> > }
> >
> > This potentially leaks sizeof(*data) memory if the module gets removed.
> > Granted that all users also use qcom_ubwc_config_get_data() symbol, it
> > should be safe to kfree(cfg) on module removal.
>
> I really don't understand why you'd want a separate API for hbb, if
> hbb is already available from the larger struct *and* if a driver needs
> to know about the value of hbb, it really needs to know about all the
> other values as well
Please take another look, qcom_ubwc_config_get_data() is the only public
API, qcom_smem_dram_get_hbb() is an internal API.
My goal is to keep having UBWC db which keeps const data and which which
also returns a const pointer.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-13 16:29 ` Dmitry Baryshkov
@ 2026-02-17 12:59 ` Konrad Dybcio
2026-02-17 22:53 ` Dmitry Baryshkov
0 siblings, 1 reply; 33+ messages in thread
From: Konrad Dybcio @ 2026-02-17 12:59 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Konrad Dybcio, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter, linux-kernel, linux-arm-msm, linux-hardening,
dri-devel, freedreno
On 1/13/26 5:29 PM, Dmitry Baryshkov wrote:
> On Tue, Jan 13, 2026 at 04:31:15PM +0100, Konrad Dybcio wrote:
>> On 1/10/26 11:45 AM, Dmitry Baryshkov wrote:
>>> On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
>>>> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
>>>>> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
>>>>>> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
>>>>>>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
>>>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>>>
>>>>>>>> To make sure the correct settings for a given DRAM configuration get
>>>>>>>> applied, attempt to retrieve that data from SMEM (which happens to be
>>>>>>>> what the BSP kernel does, albeit with through convoluted means of the
>>>>>>>> bootloader altering the DT with this data).
>>>>>>>>
>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>> I'm not sure about this approach - perhaps a global variable storing
>>>>>>>> the selected config, which would then be non-const would be better?
>>>>>>>
>>>>>>> I'd prefer if const data was const, split HBB to a separate API.
>>>>>>>
>>>>>>
>>>>>> I agree, but I'd prefer to avoid a separate API for it.
>>>>>>
>>>>>> Instead I'd like to either return the struct by value (after updating
>>>>>> the hbb), but we then loose the ability to return errors, or by changing
>>>>>> the signature to:
>>>>>>
>>>>>> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
>>>>>>
>>>>>> This costs us an additional 16 bytes in each client (as the pointer is
>>>>>> replaced with the data), but I think it's a cleaner API.
>>>>>
>>>>> What about:
>>>>>
>>>>> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
>>>>>
>>>>> I really want to keep the data as const and, as important, use it as a
>>>>> const pointer.
>>>>>
>>>>
>>>> I guess the question is what are you actually trying to achive; my goal
>>>> was to keep the base data constant, but I'm guessing that you also want
>>>> to retain the "const" classifier in the client's context struct (e.g.
>>>> the "mdss" member in struct dpu_kms)
>>>>
>>>> If we're returning the data by value, there's no way for you to mark
>>>> it as "const" in the calling code's context object (as by definition you
>>>> shouldn't be able to change the value after initializing the object).
>>>
>>> And I, of course, misssed one star:
>>>
>>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
>>>
>>> This leaks the knowledge that HBB is slightly different kind of property
>>> than the rest of UBWC data.
>>>
>>>>
>>>> You also can't return the data by value and then track it by reference -
>>>> as that value lives on the stack. This has the benefit of making the
>>>> lifecycle of that object clear (it lives in each client) - but perhaps
>>>> not a goal of ours...
>>>>
>>>> How come the ubwc config is const but the hbb isn't?
>>>>
>>>>
>>>> If we want both the per-target data to remain const and data in the
>>>> client's context to be carrying the const qualifier, the one solution I
>>>> can see is:
>>>>
>>>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
>>>> {
>>>> const struct qcom_ubwc_cfg_data *data;
>>>> static struct qcom_ubwc_cfg_data cfg;
>>>> int hbb;
>>>>
>>>> ...
>>>>
>>>> data = of_machine_get_match_data(qcom_ubwc_configs);
>>>> ...
>>>>
>>>> hbb = qcom_smem_dram_get_hbb();
>>>> ...
>>>>
>>>> cfg = *data;
>>>> cfg.highest_bank_bit = hbb;
>>>>
>>>> return &cfg;
>>>> }
>>>>
>>>> But we'd need to deal with the race in cfg assignment...
>>>
>>> static struct qcom_ubwc_cfg_data *cfg;
>>> static DEFINE_MUTEX(cfg_mutex);
>>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
>>> {
>>> const struct qcom_ubwc_cfg_data *data;
>>> int hbb;
>>>
>>> guard(mutex)(&cfg_mutex);
>>>
>>> if (cfg)
>>> return cfg;
>>>
>>> data = of_machine_get_match_data(qcom_ubwc_configs);
>>> if (!data)
>>> return ERR_PTR(-ENOMEM);
>>>
>>> hbb = qcom_smem_dram_get_hbb();
>>> if (hbb = -ENODATA)
>>> hbb = 15; /* I think it was default */
>>> else if (hbb < 0)
>>> return ERR_PTR(hbb);
>>>
>>> cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
>>> if (!cfg)
>>> return ERR_PTR(-ENOMEM);
>>>
>>> cfg->highest_bank_bit = hbb;
>>>
>>> return cfg;
>>> }
>>>
>>> This potentially leaks sizeof(*data) memory if the module gets removed.
>>> Granted that all users also use qcom_ubwc_config_get_data() symbol, it
>>> should be safe to kfree(cfg) on module removal.
>>
>> I really don't understand why you'd want a separate API for hbb, if
>> hbb is already available from the larger struct *and* if a driver needs
>> to know about the value of hbb, it really needs to know about all the
>> other values as well
>
> Please take another look, qcom_ubwc_config_get_data() is the only public
> API, qcom_smem_dram_get_hbb() is an internal API.
>
> My goal is to keep having UBWC db which keeps const data and which which
> also returns a const pointer.
Right
So if I understand correctly, this is almost exactly what I originally had
in mind in the under-"---" message (modulo having a static global ptr vs full
struct instance) but I failed to express that I wanted to keep returning a
const pointer to the consumers
So in the end it's
A) int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
vs
B) const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
I think the latter is better since we won't have to store a separate copy
of the config in each consumer driver (which the SSOT rework was largely
sparked by), essentially removing the ability for any of these drivers to
mess with the config internally and make it out-of-sync with others again
Konrad
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-02-17 12:59 ` Konrad Dybcio
@ 2026-02-17 22:53 ` Dmitry Baryshkov
2026-02-17 23:08 ` Rob Clark
0 siblings, 1 reply; 33+ messages in thread
From: Dmitry Baryshkov @ 2026-02-17 22:53 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Konrad Dybcio, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter, linux-kernel, linux-arm-msm, linux-hardening,
dri-devel, freedreno
On Tue, Feb 17, 2026 at 01:59:48PM +0100, Konrad Dybcio wrote:
> On 1/13/26 5:29 PM, Dmitry Baryshkov wrote:
> > On Tue, Jan 13, 2026 at 04:31:15PM +0100, Konrad Dybcio wrote:
> >> On 1/10/26 11:45 AM, Dmitry Baryshkov wrote:
> >>> On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
> >>>> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
> >>>>> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> >>>>>> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> >>>>>>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> >>>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>>>
> >>>>>>>> To make sure the correct settings for a given DRAM configuration get
> >>>>>>>> applied, attempt to retrieve that data from SMEM (which happens to be
> >>>>>>>> what the BSP kernel does, albeit with through convoluted means of the
> >>>>>>>> bootloader altering the DT with this data).
> >>>>>>>>
> >>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>>>
> >>>>>>>> ---
> >>>>>>>> I'm not sure about this approach - perhaps a global variable storing
> >>>>>>>> the selected config, which would then be non-const would be better?
> >>>>>>>
> >>>>>>> I'd prefer if const data was const, split HBB to a separate API.
> >>>>>>>
> >>>>>>
> >>>>>> I agree, but I'd prefer to avoid a separate API for it.
> >>>>>>
> >>>>>> Instead I'd like to either return the struct by value (after updating
> >>>>>> the hbb), but we then loose the ability to return errors, or by changing
> >>>>>> the signature to:
> >>>>>>
> >>>>>> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> >>>>>>
> >>>>>> This costs us an additional 16 bytes in each client (as the pointer is
> >>>>>> replaced with the data), but I think it's a cleaner API.
> >>>>>
> >>>>> What about:
> >>>>>
> >>>>> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
> >>>>>
> >>>>> I really want to keep the data as const and, as important, use it as a
> >>>>> const pointer.
> >>>>>
> >>>>
> >>>> I guess the question is what are you actually trying to achive; my goal
> >>>> was to keep the base data constant, but I'm guessing that you also want
> >>>> to retain the "const" classifier in the client's context struct (e.g.
> >>>> the "mdss" member in struct dpu_kms)
> >>>>
> >>>> If we're returning the data by value, there's no way for you to mark
> >>>> it as "const" in the calling code's context object (as by definition you
> >>>> shouldn't be able to change the value after initializing the object).
> >>>
> >>> And I, of course, misssed one star:
> >>>
> >>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
> >>>
> >>> This leaks the knowledge that HBB is slightly different kind of property
> >>> than the rest of UBWC data.
> >>>
> >>>>
> >>>> You also can't return the data by value and then track it by reference -
> >>>> as that value lives on the stack. This has the benefit of making the
> >>>> lifecycle of that object clear (it lives in each client) - but perhaps
> >>>> not a goal of ours...
> >>>>
> >>>> How come the ubwc config is const but the hbb isn't?
> >>>>
> >>>>
> >>>> If we want both the per-target data to remain const and data in the
> >>>> client's context to be carrying the const qualifier, the one solution I
> >>>> can see is:
> >>>>
> >>>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> >>>> {
> >>>> const struct qcom_ubwc_cfg_data *data;
> >>>> static struct qcom_ubwc_cfg_data cfg;
> >>>> int hbb;
> >>>>
> >>>> ...
> >>>>
> >>>> data = of_machine_get_match_data(qcom_ubwc_configs);
> >>>> ...
> >>>>
> >>>> hbb = qcom_smem_dram_get_hbb();
> >>>> ...
> >>>>
> >>>> cfg = *data;
> >>>> cfg.highest_bank_bit = hbb;
> >>>>
> >>>> return &cfg;
> >>>> }
> >>>>
> >>>> But we'd need to deal with the race in cfg assignment...
> >>>
> >>> static struct qcom_ubwc_cfg_data *cfg;
> >>> static DEFINE_MUTEX(cfg_mutex);
> >>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> >>> {
> >>> const struct qcom_ubwc_cfg_data *data;
> >>> int hbb;
> >>>
> >>> guard(mutex)(&cfg_mutex);
> >>>
> >>> if (cfg)
> >>> return cfg;
> >>>
> >>> data = of_machine_get_match_data(qcom_ubwc_configs);
> >>> if (!data)
> >>> return ERR_PTR(-ENOMEM);
> >>>
> >>> hbb = qcom_smem_dram_get_hbb();
> >>> if (hbb = -ENODATA)
> >>> hbb = 15; /* I think it was default */
> >>> else if (hbb < 0)
> >>> return ERR_PTR(hbb);
> >>>
> >>> cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
> >>> if (!cfg)
> >>> return ERR_PTR(-ENOMEM);
> >>>
> >>> cfg->highest_bank_bit = hbb;
> >>>
> >>> return cfg;
> >>> }
> >>>
> >>> This potentially leaks sizeof(*data) memory if the module gets removed.
> >>> Granted that all users also use qcom_ubwc_config_get_data() symbol, it
> >>> should be safe to kfree(cfg) on module removal.
> >>
> >> I really don't understand why you'd want a separate API for hbb, if
> >> hbb is already available from the larger struct *and* if a driver needs
> >> to know about the value of hbb, it really needs to know about all the
> >> other values as well
> >
> > Please take another look, qcom_ubwc_config_get_data() is the only public
> > API, qcom_smem_dram_get_hbb() is an internal API.
> >
> > My goal is to keep having UBWC db which keeps const data and which which
> > also returns a const pointer.
>
> Right
>
> So if I understand correctly, this is almost exactly what I originally had
> in mind in the under-"---" message (modulo having a static global ptr vs full
> struct instance) but I failed to express that I wanted to keep returning a
> const pointer to the consumers
>
> So in the end it's
>
> A) int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
>
> vs
>
> B) const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
>
> I think the latter is better since we won't have to store a separate copy
> of the config in each consumer driver (which the SSOT rework was largely
> sparked by), essentially removing the ability for any of these drivers to
> mess with the config internally and make it out-of-sync with others again
You have my vote for the latter option.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-02-17 22:53 ` Dmitry Baryshkov
@ 2026-02-17 23:08 ` Rob Clark
0 siblings, 0 replies; 33+ messages in thread
From: Rob Clark @ 2026-02-17 23:08 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Konrad Dybcio, Bjorn Andersson, Konrad Dybcio, Kees Cook,
Gustavo A. R. Silva, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter, linux-kernel, linux-arm-msm, linux-hardening,
dri-devel, freedreno
On Tue, Feb 17, 2026 at 2:53 PM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Tue, Feb 17, 2026 at 01:59:48PM +0100, Konrad Dybcio wrote:
> > On 1/13/26 5:29 PM, Dmitry Baryshkov wrote:
> > > On Tue, Jan 13, 2026 at 04:31:15PM +0100, Konrad Dybcio wrote:
> > >> On 1/10/26 11:45 AM, Dmitry Baryshkov wrote:
> > >>> On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
> > >>>> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
> > >>>>> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> > >>>>>> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> > >>>>>>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> > >>>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > >>>>>>>>
> > >>>>>>>> To make sure the correct settings for a given DRAM configuration get
> > >>>>>>>> applied, attempt to retrieve that data from SMEM (which happens to be
> > >>>>>>>> what the BSP kernel does, albeit with through convoluted means of the
> > >>>>>>>> bootloader altering the DT with this data).
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > >>>>>>>>
> > >>>>>>>> ---
> > >>>>>>>> I'm not sure about this approach - perhaps a global variable storing
> > >>>>>>>> the selected config, which would then be non-const would be better?
> > >>>>>>>
> > >>>>>>> I'd prefer if const data was const, split HBB to a separate API.
> > >>>>>>>
> > >>>>>>
> > >>>>>> I agree, but I'd prefer to avoid a separate API for it.
> > >>>>>>
> > >>>>>> Instead I'd like to either return the struct by value (after updating
> > >>>>>> the hbb), but we then loose the ability to return errors, or by changing
> > >>>>>> the signature to:
> > >>>>>>
> > >>>>>> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> > >>>>>>
> > >>>>>> This costs us an additional 16 bytes in each client (as the pointer is
> > >>>>>> replaced with the data), but I think it's a cleaner API.
> > >>>>>
> > >>>>> What about:
> > >>>>>
> > >>>>> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
> > >>>>>
> > >>>>> I really want to keep the data as const and, as important, use it as a
> > >>>>> const pointer.
> > >>>>>
> > >>>>
> > >>>> I guess the question is what are you actually trying to achive; my goal
> > >>>> was to keep the base data constant, but I'm guessing that you also want
> > >>>> to retain the "const" classifier in the client's context struct (e.g.
> > >>>> the "mdss" member in struct dpu_kms)
> > >>>>
> > >>>> If we're returning the data by value, there's no way for you to mark
> > >>>> it as "const" in the calling code's context object (as by definition you
> > >>>> shouldn't be able to change the value after initializing the object).
> > >>>
> > >>> And I, of course, misssed one star:
> > >>>
> > >>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
> > >>>
> > >>> This leaks the knowledge that HBB is slightly different kind of property
> > >>> than the rest of UBWC data.
> > >>>
> > >>>>
> > >>>> You also can't return the data by value and then track it by reference -
> > >>>> as that value lives on the stack. This has the benefit of making the
> > >>>> lifecycle of that object clear (it lives in each client) - but perhaps
> > >>>> not a goal of ours...
> > >>>>
> > >>>> How come the ubwc config is const but the hbb isn't?
> > >>>>
> > >>>>
> > >>>> If we want both the per-target data to remain const and data in the
> > >>>> client's context to be carrying the const qualifier, the one solution I
> > >>>> can see is:
> > >>>>
> > >>>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> > >>>> {
> > >>>> const struct qcom_ubwc_cfg_data *data;
> > >>>> static struct qcom_ubwc_cfg_data cfg;
> > >>>> int hbb;
> > >>>>
> > >>>> ...
> > >>>>
> > >>>> data = of_machine_get_match_data(qcom_ubwc_configs);
> > >>>> ...
> > >>>>
> > >>>> hbb = qcom_smem_dram_get_hbb();
> > >>>> ...
> > >>>>
> > >>>> cfg = *data;
> > >>>> cfg.highest_bank_bit = hbb;
> > >>>>
> > >>>> return &cfg;
> > >>>> }
> > >>>>
> > >>>> But we'd need to deal with the race in cfg assignment...
> > >>>
> > >>> static struct qcom_ubwc_cfg_data *cfg;
> > >>> static DEFINE_MUTEX(cfg_mutex);
> > >>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> > >>> {
> > >>> const struct qcom_ubwc_cfg_data *data;
> > >>> int hbb;
> > >>>
> > >>> guard(mutex)(&cfg_mutex);
> > >>>
> > >>> if (cfg)
> > >>> return cfg;
> > >>>
> > >>> data = of_machine_get_match_data(qcom_ubwc_configs);
> > >>> if (!data)
> > >>> return ERR_PTR(-ENOMEM);
> > >>>
> > >>> hbb = qcom_smem_dram_get_hbb();
> > >>> if (hbb = -ENODATA)
> > >>> hbb = 15; /* I think it was default */
> > >>> else if (hbb < 0)
> > >>> return ERR_PTR(hbb);
> > >>>
> > >>> cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
> > >>> if (!cfg)
> > >>> return ERR_PTR(-ENOMEM);
> > >>>
> > >>> cfg->highest_bank_bit = hbb;
> > >>>
> > >>> return cfg;
> > >>> }
> > >>>
> > >>> This potentially leaks sizeof(*data) memory if the module gets removed.
> > >>> Granted that all users also use qcom_ubwc_config_get_data() symbol, it
> > >>> should be safe to kfree(cfg) on module removal.
> > >>
> > >> I really don't understand why you'd want a separate API for hbb, if
> > >> hbb is already available from the larger struct *and* if a driver needs
> > >> to know about the value of hbb, it really needs to know about all the
> > >> other values as well
> > >
> > > Please take another look, qcom_ubwc_config_get_data() is the only public
> > > API, qcom_smem_dram_get_hbb() is an internal API.
> > >
> > > My goal is to keep having UBWC db which keeps const data and which which
> > > also returns a const pointer.
> >
> > Right
> >
> > So if I understand correctly, this is almost exactly what I originally had
> > in mind in the under-"---" message (modulo having a static global ptr vs full
> > struct instance) but I failed to express that I wanted to keep returning a
> > const pointer to the consumers
> >
> > So in the end it's
> >
> > A) int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> >
> > vs
> >
> > B) const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> >
> > I think the latter is better since we won't have to store a separate copy
> > of the config in each consumer driver (which the SSOT rework was largely
> > sparked by), essentially removing the ability for any of these drivers to
> > mess with the config internally and make it out-of-sync with others again
>
> You have my vote for the latter option.
same here, B pls
BR,
-R
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] soc: qcom: ubwc: Get HBB from SMEM
2026-01-08 17:49 ` Bjorn Andersson
2026-01-09 3:21 ` Dmitry Baryshkov
@ 2026-01-13 15:36 ` Konrad Dybcio
1 sibling, 0 replies; 33+ messages in thread
From: Konrad Dybcio @ 2026-01-13 15:36 UTC (permalink / raw)
To: Bjorn Andersson, Dmitry Baryshkov
Cc: Konrad Dybcio, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno
On 1/8/26 6:49 PM, Bjorn Andersson wrote:
> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>
>>> To make sure the correct settings for a given DRAM configuration get
>>> applied, attempt to retrieve that data from SMEM (which happens to be
>>> what the BSP kernel does, albeit with through convoluted means of the
>>> bootloader altering the DT with this data).
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>
>>> ---
>>> I'm not sure about this approach - perhaps a global variable storing
>>> the selected config, which would then be non-const would be better?
>>
>> I'd prefer if const data was const, split HBB to a separate API.
>>
>
> I agree, but I'd prefer to avoid a separate API for it.
>
> Instead I'd like to either return the struct by value (after updating
> the hbb), but we then loose the ability to return errors, or by changing
> the signature to:
>
> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
>
> This costs us an additional 16 bytes in each client (as the pointer is
> replaced with the data), but I think it's a cleaner API.
I don't see how that's much better than
static const qcom_ubwc_cfg_data foobar_ubwc_data = {
...
};
static qcom_ubwc_cfg __data;
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
{
...
if (__data)
return data;
__data = of_machine_get_match_data()
...
hbb = ...;
__data->hbb = hbb;
return data; //still returns a constptr
}
Since we essentially do the same thing, except we save space and rest
assured the various client drivers don't mess with the data they receive
Konrad
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config
2026-01-08 14:21 [PATCH v3 0/3] Retrieve information about DDR from SMEM Konrad Dybcio
2026-01-08 14:21 ` [PATCH v3 1/3] soc: qcom: smem: Expose DDR data " Konrad Dybcio
2026-01-08 14:21 ` [PATCH v3 2/3] soc: qcom: ubwc: Get HBB " Konrad Dybcio
@ 2026-01-08 14:21 ` Konrad Dybcio
2026-01-08 14:46 ` Dmitry Baryshkov
` (2 more replies)
2026-01-09 8:20 ` [PATCH v3 0/3] Retrieve information about DDR from SMEM Neil Armstrong
` (2 subsequent siblings)
5 siblings, 3 replies; 33+ messages in thread
From: Konrad Dybcio @ 2026-01-08 14:21 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter
Cc: linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Now that the highest_bank_bit value is retrieved from the running
system and the global config has been part of the tree for a couple
of releases, there is no reason to keep any hardcoded values inside
the GPU driver.
Get rid of them.
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 ++---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 ++-------------------------------
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 --
3 files changed, 6 insertions(+), 92 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 56eaff2ee4e4..eba6e74d0084 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1728,7 +1728,6 @@ static struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
struct msm_drm_private *priv = dev->dev_private;
struct platform_device *pdev = priv->gpu_pdev;
struct adreno_platform_config *config = pdev->dev.platform_data;
- const struct qcom_ubwc_cfg_data *common_cfg;
struct a5xx_gpu *a5xx_gpu = NULL;
struct adreno_gpu *adreno_gpu;
struct msm_gpu *gpu;
@@ -1766,13 +1765,9 @@ static struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
a5xx_preempt_init(gpu);
/* Inherit the common config and make some necessary fixups */
- common_cfg = qcom_ubwc_config_get_data();
- if (IS_ERR(common_cfg))
- return ERR_CAST(common_cfg);
-
- /* Copy the data into the internal struct to drop the const qualifier (temporarily) */
- adreno_gpu->_ubwc_config = *common_cfg;
- adreno_gpu->ubwc_config = &adreno_gpu->_ubwc_config;
+ adreno_gpu->ubwc_config = qcom_ubwc_config_get_data();
+ if (IS_ERR(adreno_gpu->ubwc_config))
+ return ERR_CAST(adreno_gpu->ubwc_config);
adreno_gpu->uche_trap_base = 0x0001ffffffff0000ull;
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 2129d230a92b..3a2429632225 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -729,82 +729,6 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
gpu_write(gpu, REG_A6XX_CP_PROTECT(protect->count_max - 1), protect->regs[i]);
}
-static int a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
-{
- const struct qcom_ubwc_cfg_data *common_cfg;
- struct qcom_ubwc_cfg_data *cfg = &gpu->_ubwc_config;
-
- /* Inherit the common config and make some necessary fixups */
- common_cfg = qcom_ubwc_config_get_data();
- if (IS_ERR(common_cfg))
- return PTR_ERR(common_cfg);
-
- /* Copy the data into the internal struct to drop the const qualifier (temporarily) */
- *cfg = *common_cfg;
-
- /* Use common config as is for A8x */
- if (!adreno_is_a8xx(gpu)) {
- cfg->ubwc_swizzle = 0x6;
- cfg->highest_bank_bit = 15;
- }
-
- if (adreno_is_a610(gpu)) {
- cfg->highest_bank_bit = 13;
- cfg->ubwc_swizzle = 0x7;
- }
-
- if (adreno_is_a612(gpu))
- cfg->highest_bank_bit = 14;
-
- if (adreno_is_a618(gpu))
- cfg->highest_bank_bit = 14;
-
- if (adreno_is_a619(gpu))
- /* TODO: Should be 14 but causes corruption at e.g. 1920x1200 on DP */
- cfg->highest_bank_bit = 13;
-
- if (adreno_is_a619_holi(gpu))
- cfg->highest_bank_bit = 13;
-
- if (adreno_is_a621(gpu))
- cfg->highest_bank_bit = 13;
-
- if (adreno_is_a623(gpu))
- cfg->highest_bank_bit = 16;
-
- if (adreno_is_a650(gpu) ||
- adreno_is_a660(gpu) ||
- adreno_is_a690(gpu) ||
- adreno_is_a730(gpu) ||
- adreno_is_a740_family(gpu)) {
- /* TODO: get ddr type from bootloader and use 15 for LPDDR4 */
- cfg->highest_bank_bit = 16;
- }
-
- if (adreno_is_a663(gpu)) {
- cfg->highest_bank_bit = 13;
- cfg->ubwc_swizzle = 0x4;
- }
-
- if (adreno_is_7c3(gpu))
- cfg->highest_bank_bit = 14;
-
- if (adreno_is_a702(gpu))
- cfg->highest_bank_bit = 14;
-
- if (cfg->highest_bank_bit != common_cfg->highest_bank_bit)
- DRM_WARN_ONCE("Inconclusive highest_bank_bit value: %u (GPU) vs %u (UBWC_CFG)\n",
- cfg->highest_bank_bit, common_cfg->highest_bank_bit);
-
- if (cfg->ubwc_swizzle != common_cfg->ubwc_swizzle)
- DRM_WARN_ONCE("Inconclusive ubwc_swizzle value: %u (GPU) vs %u (UBWC_CFG)\n",
- cfg->ubwc_swizzle, common_cfg->ubwc_swizzle);
-
- gpu->ubwc_config = &gpu->_ubwc_config;
-
- return 0;
-}
-
static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
@@ -2721,10 +2645,10 @@ static struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
msm_mmu_set_fault_handler(to_msm_vm(gpu->vm)->mmu, gpu,
adreno_gpu->funcs->mmu_fault_handler);
- ret = a6xx_calc_ubwc_config(adreno_gpu);
- if (ret) {
+ adreno_gpu->ubwc_config = qcom_ubwc_config_get_data();
+ if (IS_ERR(adreno_gpu->ubwc_config)) {
a6xx_destroy(&(a6xx_gpu->base.base));
- return ERR_PTR(ret);
+ return ERR_CAST(adreno_gpu->ubwc_config);
}
/* Set up the preemption specific bits and pieces for each ringbuffer */
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 1d0145f8b3ec..da9a6da7c108 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -237,12 +237,7 @@ struct adreno_gpu {
/* firmware: */
const struct firmware *fw[ADRENO_FW_MAX];
- /*
- * The migration to the central UBWC config db is still in flight - keep
- * a copy containing some local fixups until that's done.
- */
const struct qcom_ubwc_cfg_data *ubwc_config;
- struct qcom_ubwc_cfg_data _ubwc_config;
/*
* Register offsets are different between some GPUs.
--
2.52.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config
2026-01-08 14:21 ` [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config Konrad Dybcio
@ 2026-01-08 14:46 ` Dmitry Baryshkov
2026-01-16 18:32 ` Rob Clark
2026-02-28 22:16 ` Val Packett
2 siblings, 0 replies; 33+ messages in thread
From: Dmitry Baryshkov @ 2026-01-08 14:46 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Thu, Jan 08, 2026 at 03:21:52PM +0100, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> Now that the highest_bank_bit value is retrieved from the running
> system and the global config has been part of the tree for a couple
> of releases, there is no reason to keep any hardcoded values inside
> the GPU driver.
>
> Get rid of them.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 ++---
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 ++-------------------------------
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 --
> 3 files changed, 6 insertions(+), 92 deletions(-)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config
2026-01-08 14:21 ` [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config Konrad Dybcio
2026-01-08 14:46 ` Dmitry Baryshkov
@ 2026-01-16 18:32 ` Rob Clark
2026-02-28 22:16 ` Val Packett
2 siblings, 0 replies; 33+ messages in thread
From: Rob Clark @ 2026-01-16 18:32 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Kees Cook, Gustavo A. R. Silva, Sean Paul,
Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Marijn Suijten, David Airlie, Simona Vetter, linux-kernel,
linux-arm-msm, linux-hardening, dri-devel, freedreno,
Konrad Dybcio
On Thu, Jan 8, 2026 at 6:22 AM Konrad Dybcio <konradybcio@kernel.org> wrote:
>
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> Now that the highest_bank_bit value is retrieved from the running
> system and the global config has been part of the tree for a couple
> of releases, there is no reason to keep any hardcoded values inside
> the GPU driver.
>
> Get rid of them.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Reviewed-by: Rob Clark <robin.clark@oss.qualcomm.com>
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 ++---
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 ++-------------------------------
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 --
> 3 files changed, 6 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 56eaff2ee4e4..eba6e74d0084 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1728,7 +1728,6 @@ static struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> struct msm_drm_private *priv = dev->dev_private;
> struct platform_device *pdev = priv->gpu_pdev;
> struct adreno_platform_config *config = pdev->dev.platform_data;
> - const struct qcom_ubwc_cfg_data *common_cfg;
> struct a5xx_gpu *a5xx_gpu = NULL;
> struct adreno_gpu *adreno_gpu;
> struct msm_gpu *gpu;
> @@ -1766,13 +1765,9 @@ static struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> a5xx_preempt_init(gpu);
>
> /* Inherit the common config and make some necessary fixups */
> - common_cfg = qcom_ubwc_config_get_data();
> - if (IS_ERR(common_cfg))
> - return ERR_CAST(common_cfg);
> -
> - /* Copy the data into the internal struct to drop the const qualifier (temporarily) */
> - adreno_gpu->_ubwc_config = *common_cfg;
> - adreno_gpu->ubwc_config = &adreno_gpu->_ubwc_config;
> + adreno_gpu->ubwc_config = qcom_ubwc_config_get_data();
> + if (IS_ERR(adreno_gpu->ubwc_config))
> + return ERR_CAST(adreno_gpu->ubwc_config);
>
> adreno_gpu->uche_trap_base = 0x0001ffffffff0000ull;
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 2129d230a92b..3a2429632225 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -729,82 +729,6 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
> gpu_write(gpu, REG_A6XX_CP_PROTECT(protect->count_max - 1), protect->regs[i]);
> }
>
> -static int a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
> -{
> - const struct qcom_ubwc_cfg_data *common_cfg;
> - struct qcom_ubwc_cfg_data *cfg = &gpu->_ubwc_config;
> -
> - /* Inherit the common config and make some necessary fixups */
> - common_cfg = qcom_ubwc_config_get_data();
> - if (IS_ERR(common_cfg))
> - return PTR_ERR(common_cfg);
> -
> - /* Copy the data into the internal struct to drop the const qualifier (temporarily) */
> - *cfg = *common_cfg;
> -
> - /* Use common config as is for A8x */
> - if (!adreno_is_a8xx(gpu)) {
> - cfg->ubwc_swizzle = 0x6;
> - cfg->highest_bank_bit = 15;
> - }
> -
> - if (adreno_is_a610(gpu)) {
> - cfg->highest_bank_bit = 13;
> - cfg->ubwc_swizzle = 0x7;
> - }
> -
> - if (adreno_is_a612(gpu))
> - cfg->highest_bank_bit = 14;
> -
> - if (adreno_is_a618(gpu))
> - cfg->highest_bank_bit = 14;
> -
> - if (adreno_is_a619(gpu))
> - /* TODO: Should be 14 but causes corruption at e.g. 1920x1200 on DP */
> - cfg->highest_bank_bit = 13;
> -
> - if (adreno_is_a619_holi(gpu))
> - cfg->highest_bank_bit = 13;
> -
> - if (adreno_is_a621(gpu))
> - cfg->highest_bank_bit = 13;
> -
> - if (adreno_is_a623(gpu))
> - cfg->highest_bank_bit = 16;
> -
> - if (adreno_is_a650(gpu) ||
> - adreno_is_a660(gpu) ||
> - adreno_is_a690(gpu) ||
> - adreno_is_a730(gpu) ||
> - adreno_is_a740_family(gpu)) {
> - /* TODO: get ddr type from bootloader and use 15 for LPDDR4 */
> - cfg->highest_bank_bit = 16;
> - }
> -
> - if (adreno_is_a663(gpu)) {
> - cfg->highest_bank_bit = 13;
> - cfg->ubwc_swizzle = 0x4;
> - }
> -
> - if (adreno_is_7c3(gpu))
> - cfg->highest_bank_bit = 14;
> -
> - if (adreno_is_a702(gpu))
> - cfg->highest_bank_bit = 14;
> -
> - if (cfg->highest_bank_bit != common_cfg->highest_bank_bit)
> - DRM_WARN_ONCE("Inconclusive highest_bank_bit value: %u (GPU) vs %u (UBWC_CFG)\n",
> - cfg->highest_bank_bit, common_cfg->highest_bank_bit);
> -
> - if (cfg->ubwc_swizzle != common_cfg->ubwc_swizzle)
> - DRM_WARN_ONCE("Inconclusive ubwc_swizzle value: %u (GPU) vs %u (UBWC_CFG)\n",
> - cfg->ubwc_swizzle, common_cfg->ubwc_swizzle);
> -
> - gpu->ubwc_config = &gpu->_ubwc_config;
> -
> - return 0;
> -}
> -
> static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
> {
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> @@ -2721,10 +2645,10 @@ static struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
> msm_mmu_set_fault_handler(to_msm_vm(gpu->vm)->mmu, gpu,
> adreno_gpu->funcs->mmu_fault_handler);
>
> - ret = a6xx_calc_ubwc_config(adreno_gpu);
> - if (ret) {
> + adreno_gpu->ubwc_config = qcom_ubwc_config_get_data();
> + if (IS_ERR(adreno_gpu->ubwc_config)) {
> a6xx_destroy(&(a6xx_gpu->base.base));
> - return ERR_PTR(ret);
> + return ERR_CAST(adreno_gpu->ubwc_config);
> }
>
> /* Set up the preemption specific bits and pieces for each ringbuffer */
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 1d0145f8b3ec..da9a6da7c108 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -237,12 +237,7 @@ struct adreno_gpu {
> /* firmware: */
> const struct firmware *fw[ADRENO_FW_MAX];
>
> - /*
> - * The migration to the central UBWC config db is still in flight - keep
> - * a copy containing some local fixups until that's done.
> - */
> const struct qcom_ubwc_cfg_data *ubwc_config;
> - struct qcom_ubwc_cfg_data _ubwc_config;
>
> /*
> * Register offsets are different between some GPUs.
>
> --
> 2.52.0
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config
2026-01-08 14:21 ` [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config Konrad Dybcio
2026-01-08 14:46 ` Dmitry Baryshkov
2026-01-16 18:32 ` Rob Clark
@ 2026-02-28 22:16 ` Val Packett
2 siblings, 0 replies; 33+ messages in thread
From: Val Packett @ 2026-02-28 22:16 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter
Cc: linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On 1/8/26 11:21 AM, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> Now that the highest_bank_bit value is retrieved from the running
> system and the global config has been part of the tree for a couple
> of releases, there is no reason to keep any hardcoded values inside
> the GPU driver.
[…]
> - if (adreno_is_a610(gpu)) {
> - cfg->highest_bank_bit = 13;
> - cfg->ubwc_swizzle = 0x7;
> - }
Just noticed that the SoCs with A610 (SM6115/SM6125) have
.highest_bank_bit = 14 in drivers/soc/qcom/ubwc_config.c unlike this 13
value here.
Could this have been the cause of the corruption I saw on SM6115
initially? [1]
What's really strange though is that I wanted to test this now, but I
removed the FD_MESA_DEBUG=noubwc workaround that solved it initially…
and the corruption *did not* come back so I can't even repro it to
confirm that this would fix it o.0
[1]:
https://cache.treehouse.systems/media_attachments/files/116/083/578/070/293/038/original/9b8e73e15bed644f.jpg
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-08 14:21 [PATCH v3 0/3] Retrieve information about DDR from SMEM Konrad Dybcio
` (2 preceding siblings ...)
2026-01-08 14:21 ` [PATCH v3 3/3] drm/msm/adreno: Trust the SSoT UBWC config Konrad Dybcio
@ 2026-01-09 8:20 ` Neil Armstrong
2026-01-09 10:15 ` Konrad Dybcio
2026-01-09 8:31 ` Neil Armstrong
2026-01-09 19:11 ` Connor Abbott
5 siblings, 1 reply; 33+ messages in thread
From: Neil Armstrong @ 2026-01-09 8:20 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter
Cc: linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
Hi,
On 1/8/26 15:21, 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.
>
> Patch 3 should really be merged after 1&2
While trying it, I got the following warning:
In function ‘smem_dram_parse_v3_data’,
inlined from ‘smem_dram_parse’ at drivers/soc/qcom/smem_dramc.c:380:3:
drivers/soc/qcom/smem_dramc.c:216:31: warning: iteration 13 invokes undefined behavior [-Waggressive-loop-optimizations]
216 | if (freq_entry->freq_khz && freq_entry->enabled)
| ~~~~~~~~~~^~~~~~~~~~
drivers/soc/qcom/smem_dramc.c:213:27: note: within this loop
213 | for (int i = 0; i < num_freq_entries; i++) {
| ~~^~~~~~~~~~~~~~~~~~
Neil
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> Changes in v3:
> - Support v6 and v7 DDRInfo (v7 is used on e.g. Hamoa)
> - Handle rare cases of DDRInfo v5 with additional trailing data
> - Rebase/adjust to SSoT UBWC
> - Expose hbb value in debugfs
> - cosmetic changes
> - Link to v2: https://lore.kernel.org/r/20250410-topic-smem_dramc-v2-0-dead15264714@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 (3):
> soc: qcom: smem: Expose DDR data from SMEM
> soc: qcom: ubwc: Get HBB from SMEM
> drm/msm/adreno: Trust the SSoT UBWC config
>
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +-
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 +------
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 -
> drivers/soc/qcom/Makefile | 3 +-
> drivers/soc/qcom/smem.c | 14 +-
> drivers/soc/qcom/smem.h | 9 +
> drivers/soc/qcom/smem_dramc.c | 408 ++++++++++++++++++++++++++++++++
> drivers/soc/qcom/ubwc_config.c | 69 ++++--
> include/linux/soc/qcom/smem.h | 4 +
> 9 files changed, 485 insertions(+), 120 deletions(-)
> ---
> base-commit: fc4e91c639c0af93d63c3d5bc0ee45515dd7504a
> change-id: 20250409-topic-smem_dramc-6467187ac865
>
> Best regards,
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-09 8:20 ` [PATCH v3 0/3] Retrieve information about DDR from SMEM Neil Armstrong
@ 2026-01-09 10:15 ` Konrad Dybcio
0 siblings, 0 replies; 33+ messages in thread
From: Konrad Dybcio @ 2026-01-09 10:15 UTC (permalink / raw)
To: Neil Armstrong, Konrad Dybcio, Bjorn Andersson, Kees Cook,
Gustavo A. R. Silva, Rob Clark, Sean Paul, Akhil P Oommen,
Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang, Marijn Suijten,
David Airlie, Simona Vetter
Cc: linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno
On 1/9/26 9:20 AM, Neil Armstrong wrote:
> Hi,
>
> On 1/8/26 15:21, 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.
>>
>> Patch 3 should really be merged after 1&2
>
>
> While trying it, I got the following warning:
>
> In function ‘smem_dram_parse_v3_data’,
> inlined from ‘smem_dram_parse’ at drivers/soc/qcom/smem_dramc.c:380:3:
> drivers/soc/qcom/smem_dramc.c:216:31: warning: iteration 13 invokes undefined behavior [-Waggressive-loop-optimizations]
> 216 | if (freq_entry->freq_khz && freq_entry->enabled)
> | ~~~~~~~~~~^~~~~~~~~~
> drivers/soc/qcom/smem_dramc.c:213:27: note: within this loop
> 213 | for (int i = 0; i < num_freq_entries; i++) {
> | ~~^~~~~~~~~~~~~~~~~~
clang didn't seem to care..
A (really grumpy) solution would be to add:
diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
index 017bb894a91b..dc2cd7e13b88 100644
--- a/drivers/soc/qcom/smem_dramc.c
+++ b/drivers/soc/qcom/smem_dramc.c
@@ -78,7 +78,7 @@ struct ddr_freq_table {
/* V3 */
struct ddr_freq_plan_v3 {
- struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V3]; /* NOTE: some have 14 like v5 */
+ struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V3];
u8 num_ddr_freqs;
phys_addr_t clk_period_address;
};
@@ -91,6 +91,21 @@ struct ddr_details_v3 {
u8 num_channels;
};
+/* Some V3 structs have an additional frequency level */
+struct ddr_freq_plan_v3_14freqs {
+ struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V3 + 1];
+ u8 num_ddr_freqs;
+ phys_addr_t clk_period_address;
+};
+
+struct ddr_details_v3_14freqs {
+ u8 manufacturer_id;
+ u8 device_type;
+ struct ddr_part_details ddr_params[MAX_CHAN_NUM];
+ struct ddr_freq_plan_v3_14freqs ddr_freq_tbl;
+ u8 num_channels;
+};
+
/* V4 */
struct ddr_details_v4 {
u8 manufacturer_id;
@@ -201,16 +216,11 @@ int qcom_smem_dram_get_hbb(void)
}
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)
+static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data)
{
- /* 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++) {
+ 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)
@@ -218,6 +228,18 @@ static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data, bool add
}
}
+static void smem_dram_parse_v3_14freqs_data(struct smem_dram *dram, void *data)
+{
+ struct ddr_details_v3_14freqs *details = data;
+
+ for (int i = 0; i < MAX_DDR_FREQ_NUM_V3 + 1; 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;
@@ -273,8 +295,7 @@ static int smem_dram_infer_struct_version(size_t size)
if (size == sizeof(struct ddr_details_v3))
return INFO_V3;
- if (size == sizeof(struct ddr_details_v3)
- + sizeof(struct ddr_freq_table))
+ if (size == sizeof(struct ddr_details_v3_14freqs))
return INFO_V3_WITH_14_FREQS;
if (size == sizeof(struct ddr_details_v4))
@@ -374,10 +395,10 @@ struct dentry *smem_dram_parse(struct device *dev)
switch (ver) {
case INFO_V3:
- smem_dram_parse_v3_data(dram, data, false);
+ smem_dram_parse_v3_data(dram, data);
break;
case INFO_V3_WITH_14_FREQS:
- smem_dram_parse_v3_data(dram, data, true);
+ smem_dram_parse_v3_14freqs_data(dram, data);
break;
case INFO_V4:
smem_dram_parse_v4_data(dram, data);
A less grumpy one that I'm not sure would make the compiler happy would be:
diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
index 017bb894a91b..3653402fa70c 100644
--- a/drivers/soc/qcom/smem_dramc.c
+++ b/drivers/soc/qcom/smem_dramc.c
@@ -206,15 +206,16 @@ static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data, bool add
/* This may be 13 or 14 */
int num_freq_entries = MAX_DDR_FREQ_NUM_V3;
struct ddr_details_v3 *details = data;
+ struct ddr_freq_table *freq_entry;
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];
+ freq_entry = details->ddr_freq_tbl.ddr_freq;
+ for (int i = 0; i < num_freq_entries; i++) {
if (freq_entry->freq_khz && freq_entry->enabled)
- dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
+ dram->frequencies[dram->num_frequencies++] = 1000 * (freq_entry++)->freq_khz;
}
}
WDYT?
Konrad
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-08 14:21 [PATCH v3 0/3] Retrieve information about DDR from SMEM Konrad Dybcio
` (3 preceding siblings ...)
2026-01-09 8:20 ` [PATCH v3 0/3] Retrieve information about DDR from SMEM Neil Armstrong
@ 2026-01-09 8:31 ` Neil Armstrong
2026-01-09 19:11 ` Connor Abbott
5 siblings, 0 replies; 33+ messages in thread
From: Neil Armstrong @ 2026-01-09 8:31 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter
Cc: linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On 1/8/26 15:21, 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.
>
> Patch 3 should really be merged after 1&2
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> Changes in v3:
> - Support v6 and v7 DDRInfo (v7 is used on e.g. Hamoa)
> - Handle rare cases of DDRInfo v5 with additional trailing data
> - Rebase/adjust to SSoT UBWC
> - Expose hbb value in debugfs
> - cosmetic changes
> - Link to v2: https://lore.kernel.org/r/20250410-topic-smem_dramc-v2-0-dead15264714@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 (3):
> soc: qcom: smem: Expose DDR data from SMEM
> soc: qcom: ubwc: Get HBB from SMEM
> drm/msm/adreno: Trust the SSoT UBWC config
>
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +-
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 +------
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 -
> drivers/soc/qcom/Makefile | 3 +-
> drivers/soc/qcom/smem.c | 14 +-
> drivers/soc/qcom/smem.h | 9 +
> drivers/soc/qcom/smem_dramc.c | 408 ++++++++++++++++++++++++++++++++
> drivers/soc/qcom/ubwc_config.c | 69 ++++--
> include/linux/soc/qcom/smem.h | 4 +
> 9 files changed, 485 insertions(+), 120 deletions(-)
> ---
> base-commit: fc4e91c639c0af93d63c3d5bc0ee45515dd7504a
> change-id: 20250409-topic-smem_dramc-6467187ac865
>
> Best regards,
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK
root@qemuarm64:~# cat /sys/kernel/debug/qcom_smem/
dram_frequencies hbb
root@qemuarm64:~# cat /sys/kernel/debug/qcom_smem/dram_frequencies
200000000
547200000
768000000
1555200000
1708800000
2092800000
2736000000
3187200000
3686400000
4224000000
root@qemuarm64:~# cat /sys/kernel/debug/qcom_smem/hbb
16
Thanks,
Neil
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-08 14:21 [PATCH v3 0/3] Retrieve information about DDR from SMEM Konrad Dybcio
` (4 preceding siblings ...)
2026-01-09 8:31 ` Neil Armstrong
@ 2026-01-09 19:11 ` Connor Abbott
2026-01-09 20:41 ` Rob Clark
` (2 more replies)
5 siblings, 3 replies; 33+ messages in thread
From: Connor Abbott @ 2026-01-09 19:11 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Thu, Jan 8, 2026 at 9:22 AM Konrad Dybcio <konradybcio@kernel.org> 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.
>
> Patch 3 should really be merged after 1&2
No. The HBB value currently returned by the bootloader is *not* always
the same as what we use currently, because some SoCs (like SM8250)
with the same DT ship with multiple different DRAM configurations and
we've been using a sub-optimal value the whole time. After all, that's
the whole point of using the bootloader value. But patches 1&2 will
only make the DPU use the bootloader value for HBB, not the GPU. So on
one of the affected SoCs, it will introduce a mismatch. You can't
change anything until the GPU side uses the new ubwc config as its
source of truth.
Connor
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> Changes in v3:
> - Support v6 and v7 DDRInfo (v7 is used on e.g. Hamoa)
> - Handle rare cases of DDRInfo v5 with additional trailing data
> - Rebase/adjust to SSoT UBWC
> - Expose hbb value in debugfs
> - cosmetic changes
> - Link to v2: https://lore.kernel.org/r/20250410-topic-smem_dramc-v2-0-dead15264714@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 (3):
> soc: qcom: smem: Expose DDR data from SMEM
> soc: qcom: ubwc: Get HBB from SMEM
> drm/msm/adreno: Trust the SSoT UBWC config
>
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +-
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 +------
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 -
> drivers/soc/qcom/Makefile | 3 +-
> drivers/soc/qcom/smem.c | 14 +-
> drivers/soc/qcom/smem.h | 9 +
> drivers/soc/qcom/smem_dramc.c | 408 ++++++++++++++++++++++++++++++++
> drivers/soc/qcom/ubwc_config.c | 69 ++++--
> include/linux/soc/qcom/smem.h | 4 +
> 9 files changed, 485 insertions(+), 120 deletions(-)
> ---
> base-commit: fc4e91c639c0af93d63c3d5bc0ee45515dd7504a
> change-id: 20250409-topic-smem_dramc-6467187ac865
>
> Best regards,
> --
> Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-09 19:11 ` Connor Abbott
@ 2026-01-09 20:41 ` Rob Clark
2026-01-09 21:03 ` Connor Abbott
2026-01-10 10:49 ` Dmitry Baryshkov
2026-01-13 15:31 ` Konrad Dybcio
2 siblings, 1 reply; 33+ messages in thread
From: Rob Clark @ 2026-01-09 20:41 UTC (permalink / raw)
To: Connor Abbott
Cc: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Fri, Jan 9, 2026 at 11:11 AM Connor Abbott <cwabbott0@gmail.com> wrote:
>
> On Thu, Jan 8, 2026 at 9:22 AM Konrad Dybcio <konradybcio@kernel.org> 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.
> >
> > Patch 3 should really be merged after 1&2
>
> No. The HBB value currently returned by the bootloader is *not* always
> the same as what we use currently, because some SoCs (like SM8250)
> with the same DT ship with multiple different DRAM configurations and
> we've been using a sub-optimal value the whole time. After all, that's
> the whole point of using the bootloader value. But patches 1&2 will
> only make the DPU use the bootloader value for HBB, not the GPU. So on
> one of the affected SoCs, it will introduce a mismatch. You can't
> change anything until the GPU side uses the new ubwc config as its
> source of truth.
Hmm, how is this even working today if DPU is using HBB from the
global table but GPU is not? Are we just getting lucky with
compositors that don't know about modifiers and end up scanning out
linear?
We do log warnings when the global ubwc config does not match the
"fixed up" config.. google search for those msgs doesn't seem to turn
up anything other than the patch which introduced them. Idk if that
is conclusive in any way, but I hope that means we could just delete
the fixup code on the GPU side. I suppose we could add:
*cfg = *common_cfg;
after the warning as a first step. That would maybe get some bug
reports along with enough details in dmesg?
BR,
-R
> Connor
>
> >
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > ---
> > Changes in v3:
> > - Support v6 and v7 DDRInfo (v7 is used on e.g. Hamoa)
> > - Handle rare cases of DDRInfo v5 with additional trailing data
> > - Rebase/adjust to SSoT UBWC
> > - Expose hbb value in debugfs
> > - cosmetic changes
> > - Link to v2: https://lore.kernel.org/r/20250410-topic-smem_dramc-v2-0-dead15264714@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 (3):
> > soc: qcom: smem: Expose DDR data from SMEM
> > soc: qcom: ubwc: Get HBB from SMEM
> > drm/msm/adreno: Trust the SSoT UBWC config
> >
> > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +-
> > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 +------
> > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 -
> > drivers/soc/qcom/Makefile | 3 +-
> > drivers/soc/qcom/smem.c | 14 +-
> > drivers/soc/qcom/smem.h | 9 +
> > drivers/soc/qcom/smem_dramc.c | 408 ++++++++++++++++++++++++++++++++
> > drivers/soc/qcom/ubwc_config.c | 69 ++++--
> > include/linux/soc/qcom/smem.h | 4 +
> > 9 files changed, 485 insertions(+), 120 deletions(-)
> > ---
> > base-commit: fc4e91c639c0af93d63c3d5bc0ee45515dd7504a
> > change-id: 20250409-topic-smem_dramc-6467187ac865
> >
> > Best regards,
> > --
> > Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-09 20:41 ` Rob Clark
@ 2026-01-09 21:03 ` Connor Abbott
2026-01-10 4:17 ` Rob Clark
2026-02-17 11:23 ` Konrad Dybcio
0 siblings, 2 replies; 33+ messages in thread
From: Connor Abbott @ 2026-01-09 21:03 UTC (permalink / raw)
To: rob.clark
Cc: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Fri, Jan 9, 2026 at 3:41 PM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
>
> On Fri, Jan 9, 2026 at 11:11 AM Connor Abbott <cwabbott0@gmail.com> wrote:
> >
> > On Thu, Jan 8, 2026 at 9:22 AM Konrad Dybcio <konradybcio@kernel.org> 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.
> > >
> > > Patch 3 should really be merged after 1&2
> >
> > No. The HBB value currently returned by the bootloader is *not* always
> > the same as what we use currently, because some SoCs (like SM8250)
> > with the same DT ship with multiple different DRAM configurations and
> > we've been using a sub-optimal value the whole time. After all, that's
> > the whole point of using the bootloader value. But patches 1&2 will
> > only make the DPU use the bootloader value for HBB, not the GPU. So on
> > one of the affected SoCs, it will introduce a mismatch. You can't
> > change anything until the GPU side uses the new ubwc config as its
> > source of truth.
>
> Hmm, how is this even working today if DPU is using HBB from the
> global table but GPU is not? Are we just getting lucky with
> compositors that don't know about modifiers and end up scanning out
> linear?
It works out as well as it's always worked out, i.e. we try to make
GPU and DPU config match and pray that we didn't mess it up. At least
now we'll get a warning when they don't match.
>
> We do log warnings when the global ubwc config does not match the
> "fixed up" config.. google search for those msgs doesn't seem to turn
> up anything other than the patch which introduced them. Idk if that
> is conclusive in any way, but I hope that means we could just delete
> the fixup code on the GPU side. I suppose we could add:
>
> *cfg = *common_cfg;
>
> after the warning as a first step. That would maybe get some bug
> reports along with enough details in dmesg?
Yes, the plan was always to delete the fixup code in the GPU config.
And even that first step would be enough to prevent regressions when
switching to the bootloader HBB value.
There is a problem in that ubwc_swizzle isn't as well tested. Older
parts supporting UBWC 1.0-3.0 partially or entirely ignore
ubwc_swizzle, because it wasn't configurable back then, but we rely on
it being set correctly in Mesa for VK_EXT_host_image_copy and sparse
textures. So if ubwc_swizzle is incorrect you probably wouldn't
notice, until you hit the HIC codepath in zink or some game using
sparse textures. I think we fixed up all the cases where it was
incorrectly set to 0x1 instead of 0x7, but it would be worth it to
check again.
Connor
>
> BR,
> -R
>
> > Connor
> >
> > >
> > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > ---
> > > Changes in v3:
> > > - Support v6 and v7 DDRInfo (v7 is used on e.g. Hamoa)
> > > - Handle rare cases of DDRInfo v5 with additional trailing data
> > > - Rebase/adjust to SSoT UBWC
> > > - Expose hbb value in debugfs
> > > - cosmetic changes
> > > - Link to v2: https://lore.kernel.org/r/20250410-topic-smem_dramc-v2-0-dead15264714@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 (3):
> > > soc: qcom: smem: Expose DDR data from SMEM
> > > soc: qcom: ubwc: Get HBB from SMEM
> > > drm/msm/adreno: Trust the SSoT UBWC config
> > >
> > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +-
> > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 +------
> > > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 -
> > > drivers/soc/qcom/Makefile | 3 +-
> > > drivers/soc/qcom/smem.c | 14 +-
> > > drivers/soc/qcom/smem.h | 9 +
> > > drivers/soc/qcom/smem_dramc.c | 408 ++++++++++++++++++++++++++++++++
> > > drivers/soc/qcom/ubwc_config.c | 69 ++++--
> > > include/linux/soc/qcom/smem.h | 4 +
> > > 9 files changed, 485 insertions(+), 120 deletions(-)
> > > ---
> > > base-commit: fc4e91c639c0af93d63c3d5bc0ee45515dd7504a
> > > change-id: 20250409-topic-smem_dramc-6467187ac865
> > >
> > > Best regards,
> > > --
> > > Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-09 21:03 ` Connor Abbott
@ 2026-01-10 4:17 ` Rob Clark
2026-02-17 11:23 ` Konrad Dybcio
1 sibling, 0 replies; 33+ messages in thread
From: Rob Clark @ 2026-01-10 4:17 UTC (permalink / raw)
To: Connor Abbott
Cc: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno, Konrad Dybcio
On Fri, Jan 9, 2026 at 1:03 PM Connor Abbott <cwabbott0@gmail.com> wrote:
>
> On Fri, Jan 9, 2026 at 3:41 PM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> >
> > On Fri, Jan 9, 2026 at 11:11 AM Connor Abbott <cwabbott0@gmail.com> wrote:
> > >
> > > On Thu, Jan 8, 2026 at 9:22 AM Konrad Dybcio <konradybcio@kernel.org> 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.
> > > >
> > > > Patch 3 should really be merged after 1&2
> > >
> > > No. The HBB value currently returned by the bootloader is *not* always
> > > the same as what we use currently, because some SoCs (like SM8250)
> > > with the same DT ship with multiple different DRAM configurations and
> > > we've been using a sub-optimal value the whole time. After all, that's
> > > the whole point of using the bootloader value. But patches 1&2 will
> > > only make the DPU use the bootloader value for HBB, not the GPU. So on
> > > one of the affected SoCs, it will introduce a mismatch. You can't
> > > change anything until the GPU side uses the new ubwc config as its
> > > source of truth.
> >
> > Hmm, how is this even working today if DPU is using HBB from the
> > global table but GPU is not? Are we just getting lucky with
> > compositors that don't know about modifiers and end up scanning out
> > linear?
>
> It works out as well as it's always worked out, i.e. we try to make
> GPU and DPU config match and pray that we didn't mess it up. At least
> now we'll get a warning when they don't match.
I mean, upstream we kinda have a lack of feedback metrics other than
people reporting bugs... so maybe the first step is to break it with
*cfg = *common_cfg;
and see what happens.
> >
> > We do log warnings when the global ubwc config does not match the
> > "fixed up" config.. google search for those msgs doesn't seem to turn
> > up anything other than the patch which introduced them. Idk if that
> > is conclusive in any way, but I hope that means we could just delete
> > the fixup code on the GPU side. I suppose we could add:
> >
> > *cfg = *common_cfg;
> >
> > after the warning as a first step. That would maybe get some bug
> > reports along with enough details in dmesg?
>
> Yes, the plan was always to delete the fixup code in the GPU config.
> And even that first step would be enough to prevent regressions when
> switching to the bootloader HBB value.
>
> There is a problem in that ubwc_swizzle isn't as well tested. Older
> parts supporting UBWC 1.0-3.0 partially or entirely ignore
> ubwc_swizzle, because it wasn't configurable back then, but we rely on
> it being set correctly in Mesa for VK_EXT_host_image_copy and sparse
> textures. So if ubwc_swizzle is incorrect you probably wouldn't
> notice, until you hit the HIC codepath in zink or some game using
> sparse textures. I think we fixed up all the cases where it was
> incorrectly set to 0x1 instead of 0x7, but it would be worth it to
> check again.
There are a lot more things out there than there are things running
games that would hit that. Idk about zink, but there is a gallium
driver. Either way, we don't have metrics feedback so the only option
is to break things with enough info in dmesg to figure out what is
going wrong AFAICT.
BR,
-R
>
> Connor
>
> >
> > BR,
> > -R
> >
> > > Connor
> > >
> > > >
> > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > > ---
> > > > Changes in v3:
> > > > - Support v6 and v7 DDRInfo (v7 is used on e.g. Hamoa)
> > > > - Handle rare cases of DDRInfo v5 with additional trailing data
> > > > - Rebase/adjust to SSoT UBWC
> > > > - Expose hbb value in debugfs
> > > > - cosmetic changes
> > > > - Link to v2: https://lore.kernel.org/r/20250410-topic-smem_dramc-v2-0-dead15264714@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 (3):
> > > > soc: qcom: smem: Expose DDR data from SMEM
> > > > soc: qcom: ubwc: Get HBB from SMEM
> > > > drm/msm/adreno: Trust the SSoT UBWC config
> > > >
> > > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 11 +-
> > > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 +------
> > > > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 -
> > > > drivers/soc/qcom/Makefile | 3 +-
> > > > drivers/soc/qcom/smem.c | 14 +-
> > > > drivers/soc/qcom/smem.h | 9 +
> > > > drivers/soc/qcom/smem_dramc.c | 408 ++++++++++++++++++++++++++++++++
> > > > drivers/soc/qcom/ubwc_config.c | 69 ++++--
> > > > include/linux/soc/qcom/smem.h | 4 +
> > > > 9 files changed, 485 insertions(+), 120 deletions(-)
> > > > ---
> > > > base-commit: fc4e91c639c0af93d63c3d5bc0ee45515dd7504a
> > > > change-id: 20250409-topic-smem_dramc-6467187ac865
> > > >
> > > > Best regards,
> > > > --
> > > > Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-09 21:03 ` Connor Abbott
2026-01-10 4:17 ` Rob Clark
@ 2026-02-17 11:23 ` Konrad Dybcio
2026-02-18 15:58 ` Connor Abbott
1 sibling, 1 reply; 33+ messages in thread
From: Konrad Dybcio @ 2026-02-17 11:23 UTC (permalink / raw)
To: Connor Abbott, rob.clark
Cc: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno
On 1/9/26 10:03 PM, Connor Abbott wrote:
> On Fri, Jan 9, 2026 at 3:41 PM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
>>
>> On Fri, Jan 9, 2026 at 11:11 AM Connor Abbott <cwabbott0@gmail.com> wrote:
>>>
>>> On Thu, Jan 8, 2026 at 9:22 AM Konrad Dybcio <konradybcio@kernel.org> 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.
>>>>
>>>> Patch 3 should really be merged after 1&2
>>>
>>> No. The HBB value currently returned by the bootloader is *not* always
>>> the same as what we use currently, because some SoCs (like SM8250)
>>> with the same DT ship with multiple different DRAM configurations and
>>> we've been using a sub-optimal value the whole time. After all, that's
>>> the whole point of using the bootloader value. But patches 1&2 will
>>> only make the DPU use the bootloader value for HBB, not the GPU. So on
>>> one of the affected SoCs, it will introduce a mismatch. You can't
>>> change anything until the GPU side uses the new ubwc config as its
>>> source of truth.
>>
>> Hmm, how is this even working today if DPU is using HBB from the
>> global table but GPU is not? Are we just getting lucky with
>> compositors that don't know about modifiers and end up scanning out
>> linear?
>
> It works out as well as it's always worked out, i.e. we try to make
> GPU and DPU config match and pray that we didn't mess it up. At least
> now we'll get a warning when they don't match.
>
>>
>> We do log warnings when the global ubwc config does not match the
>> "fixed up" config.. google search for those msgs doesn't seem to turn
>> up anything other than the patch which introduced them. Idk if that
>> is conclusive in any way, but I hope that means we could just delete
>> the fixup code on the GPU side. I suppose we could add:
>>
>> *cfg = *common_cfg;
>>
>> after the warning as a first step. That would maybe get some bug
>> reports along with enough details in dmesg?
>
> Yes, the plan was always to delete the fixup code in the GPU config.
> And even that first step would be enough to prevent regressions when
> switching to the bootloader HBB value.
>
> There is a problem in that ubwc_swizzle isn't as well tested. Older
> parts supporting UBWC 1.0-3.0 partially or entirely ignore
> ubwc_swizzle, because it wasn't configurable back then, but we rely on
> it being set correctly in Mesa for VK_EXT_host_image_copy and sparse
> textures. So if ubwc_swizzle is incorrect you probably wouldn't
> notice, until you hit the HIC codepath in zink or some game using
> sparse textures. I think we fixed up all the cases where it was
> incorrectly set to 0x1 instead of 0x7, but it would be worth it to
> check again.
Just to double-check, is your expectation to just double-check the kernel
settings, or would that require some intervention on the mesa side too?
Konrad
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-02-17 11:23 ` Konrad Dybcio
@ 2026-02-18 15:58 ` Connor Abbott
0 siblings, 0 replies; 33+ messages in thread
From: Connor Abbott @ 2026-02-18 15:58 UTC (permalink / raw)
To: Konrad Dybcio
Cc: rob.clark, Konrad Dybcio, Bjorn Andersson, Kees Cook,
Gustavo A. R. Silva, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter, linux-kernel, linux-arm-msm, linux-hardening,
dri-devel, freedreno
On Tue, Feb 17, 2026 at 6:23 AM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 1/9/26 10:03 PM, Connor Abbott wrote:
> > On Fri, Jan 9, 2026 at 3:41 PM Rob Clark <rob.clark@oss.qualcomm.com> wrote:
> >>
> >> On Fri, Jan 9, 2026 at 11:11 AM Connor Abbott <cwabbott0@gmail.com> wrote:
> >>>
> >>> On Thu, Jan 8, 2026 at 9:22 AM Konrad Dybcio <konradybcio@kernel.org> 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.
> >>>>
> >>>> Patch 3 should really be merged after 1&2
> >>>
> >>> No. The HBB value currently returned by the bootloader is *not* always
> >>> the same as what we use currently, because some SoCs (like SM8250)
> >>> with the same DT ship with multiple different DRAM configurations and
> >>> we've been using a sub-optimal value the whole time. After all, that's
> >>> the whole point of using the bootloader value. But patches 1&2 will
> >>> only make the DPU use the bootloader value for HBB, not the GPU. So on
> >>> one of the affected SoCs, it will introduce a mismatch. You can't
> >>> change anything until the GPU side uses the new ubwc config as its
> >>> source of truth.
> >>
> >> Hmm, how is this even working today if DPU is using HBB from the
> >> global table but GPU is not? Are we just getting lucky with
> >> compositors that don't know about modifiers and end up scanning out
> >> linear?
> >
> > It works out as well as it's always worked out, i.e. we try to make
> > GPU and DPU config match and pray that we didn't mess it up. At least
> > now we'll get a warning when they don't match.
> >
> >>
> >> We do log warnings when the global ubwc config does not match the
> >> "fixed up" config.. google search for those msgs doesn't seem to turn
> >> up anything other than the patch which introduced them. Idk if that
> >> is conclusive in any way, but I hope that means we could just delete
> >> the fixup code on the GPU side. I suppose we could add:
> >>
> >> *cfg = *common_cfg;
> >>
> >> after the warning as a first step. That would maybe get some bug
> >> reports along with enough details in dmesg?
> >
> > Yes, the plan was always to delete the fixup code in the GPU config.
> > And even that first step would be enough to prevent regressions when
> > switching to the bootloader HBB value.
> >
> > There is a problem in that ubwc_swizzle isn't as well tested. Older
> > parts supporting UBWC 1.0-3.0 partially or entirely ignore
> > ubwc_swizzle, because it wasn't configurable back then, but we rely on
> > it being set correctly in Mesa for VK_EXT_host_image_copy and sparse
> > textures. So if ubwc_swizzle is incorrect you probably wouldn't
> > notice, until you hit the HIC codepath in zink or some game using
> > sparse textures. I think we fixed up all the cases where it was
> > incorrectly set to 0x1 instead of 0x7, but it would be worth it to
> > check again.
>
> Just to double-check, is your expectation to just double-check the kernel
> settings, or would that require some intervention on the mesa side too?
>
> Konrad
Just to double-check the kernel, and I double-checked since then and
everything seems to be right.
Connor
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-09 19:11 ` Connor Abbott
2026-01-09 20:41 ` Rob Clark
@ 2026-01-10 10:49 ` Dmitry Baryshkov
2026-01-13 15:31 ` Konrad Dybcio
2 siblings, 0 replies; 33+ messages in thread
From: Dmitry Baryshkov @ 2026-01-10 10:49 UTC (permalink / raw)
To: Connor Abbott
Cc: Konrad Dybcio, Bjorn Andersson, Kees Cook, Gustavo A. R. Silva,
Rob Clark, Sean Paul, Akhil P Oommen, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Marijn Suijten, David Airlie,
Simona Vetter, linux-kernel, linux-arm-msm, linux-hardening,
dri-devel, freedreno, Konrad Dybcio
On Fri, Jan 09, 2026 at 02:11:19PM -0500, Connor Abbott wrote:
> On Thu, Jan 8, 2026 at 9:22 AM Konrad Dybcio <konradybcio@kernel.org> 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.
> >
> > Patch 3 should really be merged after 1&2
>
> No. The HBB value currently returned by the bootloader is *not* always
> the same as what we use currently, because some SoCs (like SM8250)
> with the same DT ship with multiple different DRAM configurations and
> we've been using a sub-optimal value the whole time. After all, that's
> the whole point of using the bootloader value. But patches 1&2 will
> only make the DPU use the bootloader value for HBB, not the GPU. So on
> one of the affected SoCs, it will introduce a mismatch. You can't
> change anything until the GPU side uses the new ubwc config as its
> source of truth.
Which, unfortunately, also means that Iris / Venus must also start using
the UBWC config API beforehand.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 0/3] Retrieve information about DDR from SMEM
2026-01-09 19:11 ` Connor Abbott
2026-01-09 20:41 ` Rob Clark
2026-01-10 10:49 ` Dmitry Baryshkov
@ 2026-01-13 15:31 ` Konrad Dybcio
2 siblings, 0 replies; 33+ messages in thread
From: Konrad Dybcio @ 2026-01-13 15:31 UTC (permalink / raw)
To: Connor Abbott, Konrad Dybcio
Cc: Bjorn Andersson, Kees Cook, Gustavo A. R. Silva, Rob Clark,
Sean Paul, Akhil P Oommen, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Marijn Suijten, David Airlie, Simona Vetter,
linux-kernel, linux-arm-msm, linux-hardening, dri-devel,
freedreno
On 1/9/26 8:11 PM, Connor Abbott wrote:
> On Thu, Jan 8, 2026 at 9:22 AM Konrad Dybcio <konradybcio@kernel.org> 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.
>>
>> Patch 3 should really be merged after 1&2
>
> No. The HBB value currently returned by the bootloader is *not* always
> the same as what we use currently, because some SoCs (like SM8250)
> with the same DT ship with multiple different DRAM configurations and
> we've been using a sub-optimal value the whole time. After all, that's
> the whole point of using the bootloader value. But patches 1&2 will
> only make the DPU use the bootloader value for HBB, not the GPU. So on
> one of the affected SoCs, it will introduce a mismatch. You can't
> change anything until the GPU side uses the new ubwc config as its
> source of truth.
You're right, sorry for that
Konrad
^ permalink raw reply [flat|nested] 33+ messages in thread
