[RFT PATCH v6 0/5] Add SMEM-based speedbin matching

[RFT PATCH v6 0/5] Add SMEM-based speedbin matching

[Konrad Dybcio]

Newer (SM8550+) SoCs don't seem to have a nice speedbin fuse anymore,
but instead rely on a set of combinations of "feature code" (FC) and
"product code" (PC) identifiers to match the bins. This series adds
support for that.

I suppose a qcom/for-soc immutable branch would be in order if we want
to land this in the upcoming cycle.

FWIW I preferred the fuses myself..

---
Changes in v6:
- Rebase
- Some cosmetic changes in comments
- Better explain the backwards compatibility issues stemming from
  incomplete platform descriptions
- Hopefully fix all the remaining edge cases..
- Link to v5: https://lore.kernel.org/linux-arm-msm/20240709-topic-smem_speedbin-v5-0-e2146be0c96f@linaro.org/

Changes in v5:
- Rebase
- Fix some unhandled cases (Elliot)
- Fix unused variable warning
- Touch up some comments
- Link to v4: https://lore.kernel.org/r/20240625-topic-smem_speedbin-v4-0-f6f8493ab814@linaro.org

Changes in v4:
- Drop applied qcom patches
- Make the fuse/speedbin fields u16 again (as Pcode is unused)
- Add comments explaining why there's only speedbin0 for 8550
- Fix some checkpatch fluff (code style)
- Rebase on next-20240625

Changes in v3:
- Wrap the argument usage in new preprocessor macros in braces (Bjorn)
- Make the SOCINFO_FC_INT_MAX define inclusive, adjust .h and .c (Bjorn)
- Pick up rbs
- Rebase on next-20240605
- Drop the already-applied ("Avoid a nullptr dereference when speedbin
  setting fails")

Changes in v2:
- Separate moving existing and adding new defines
- Fix kerneldoc copypasta
- Remove some wrong comments and defines
- Remove assumed "max" values for PCs and external FCs
- Improve some commit messages
- Return -EOPNOTSUPP instead of -EINVAL when calling p/fcode getters
  on socinfo older than v16
- Drop pcode getters and evaluation (doesn't matter for Adreno on
  non-proto SoCs)
- Rework the speedbin logic to be hopefully saner
- Link to v1: https://lore.kernel.org/r/20240405-topic-smem_speedbin-v1-0-ce2b864251b1@linaro.org

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

---
Konrad Dybcio (5):
      drm/msm/adreno: Implement SMEM-based speed bin
      drm/msm/adreno: Add speedbin data for SM8550 / A740
      drm/msm/adreno: Define A530 speed bins explicitly
      drm/msm/adreno: Redo the speedbin assignment
      arm64: dts: qcom: sm8550: Wire up GPU speed bin & more OPPs

 arch/arm64/boot/dts/qcom/sm8550.dtsi       |  21 +++++-
 drivers/gpu/drm/msm/adreno/a5xx_catalog.c  |   6 ++
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c      |  34 ---------
 drivers/gpu/drm/msm/adreno/a6xx_catalog.c  |   8 +++
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c      |  54 ---------------
 drivers/gpu/drm/msm/adreno/adreno_device.c |   2 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 107 +++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |   6 +-
 8 files changed, 141 insertions(+), 97 deletions(-)
---
base-commit: 07e7f436c1caa294bd689004077c553957915afd
change-id: 20250425-topic-smem_speedbin_respin-b167a957a56b

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

[PATCH RFT v6 1/5] drm/msm/adreno: Implement SMEM-based speed bin

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@linaro.org>

On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
abstracted through SMEM, instead of being directly available in a fuse.

Add support for SMEM-based speed binning, which includes getting
"feature code" and "product code" from said source and parsing them
to form something that lets us match OPPs against.

Due to the product code being ignored in the context of Adreno on
production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 14 +++++-----
 drivers/gpu/drm/msm/adreno/adreno_device.c |  2 ++
 drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 42 +++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  7 ++++-
 4 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 242d02d48c0cd0972bb96a960872b73384fe043b..0db57e4b7596b01c091ed82510cf14cf2a8e0d03 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2328,18 +2328,20 @@ static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse)
 	return UINT_MAX;
 }
 
-static int a6xx_set_supported_hw(struct device *dev, const struct adreno_info *info)
+static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu,
+				 struct device *dev,
+				 const struct adreno_info *info)
 {
 	u32 supp_hw;
 	u32 speedbin;
 	int ret;
 
-	ret = adreno_read_speedbin(dev, &speedbin);
+	ret = adreno_read_speedbin(adreno_gpu, dev, &speedbin);
 	/*
-	 * -ENOENT means that the platform doesn't support speedbin which is
-	 * fine
+	 * -ENOENT/EOPNOTSUPP means that the platform doesn't support speedbin
+	 * which is fine
 	 */
-	if (ret == -ENOENT) {
+	if (ret == -ENOENT || ret == -EOPNOTSUPP) {
 		return 0;
 	} else if (ret) {
 		dev_err_probe(dev, ret,
@@ -2495,7 +2497,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 
 	a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx);
 
-	ret = a6xx_set_supported_hw(&pdev->dev, config->info);
+	ret = a6xx_set_supported_hw(adreno_gpu, &pdev->dev, config->info);
 	if (ret) {
 		a6xx_llc_slices_destroy(a6xx_gpu);
 		kfree(a6xx_gpu);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 236b25c094cd5d462f4b6653de7b7910985cccb6..a8376574381abff90d4a56e86f3f05735027ca9f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -6,6 +6,8 @@
  * Copyright (c) 2014,2017 The Linux Foundation. All rights reserved.
  */
 
+#include <linux/soc/qcom/socinfo.h>
+
 #include "adreno_gpu.h"
 
 bool hang_debug = false;
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 26db1f4b5fb90930bdbd2f17682bf47e35870936..b0ec64e9a35591507f26e16b4ef60ec874dafe12 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -21,6 +21,9 @@
 #include "msm_gem.h"
 #include "msm_mmu.h"
 
+#include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/socinfo.h>
+
 static u64 address_space_size = 0;
 MODULE_PARM_DESC(address_space_size, "Override for size of processes private GPU address space");
 module_param(address_space_size, ullong, 0600);
@@ -1090,9 +1093,40 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
 			   adreno_ocmem->hdl);
 }
 
-int adreno_read_speedbin(struct device *dev, u32 *speedbin)
+int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
+			 struct device *dev, u32 *fuse)
 {
-	return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin);
+	int ret;
+
+	/*
+	 * Try reading the speedbin via a nvmem cell first
+	 * -ENOENT means "no nvmem-cells" and essentially means "old DT" or
+	 * "nvmem fuse is irrelevant", simply assume it's fine.
+	 */
+	ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", fuse);
+	if (!ret)
+		return 0;
+	else if (ret != -ENOENT)
+		return dev_err_probe(dev, ret, "Couldn't read the speed bin fuse value\n");
+
+#ifdef CONFIG_QCOM_SMEM
+	u32 fcode;
+
+	/*
+	 * Only check the feature code - the product code only matters for
+	 * proto SoCs unavailable outside Qualcomm labs, as far as GPU bin
+	 * matching is concerned.
+	 *
+	 * Ignore EOPNOTSUPP, as not all SoCs expose this info through SMEM.
+	 */
+	ret = qcom_smem_get_feature_code(&fcode);
+	if (!ret)
+		*fuse = ADRENO_SKU_ID(fcode);
+	else if (ret != -EOPNOTSUPP)
+		return dev_err_probe(dev, ret, "Couldn't get feature code from SMEM\n");
+#endif
+
+	return ret;
 }
 
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
@@ -1132,9 +1166,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 			devm_pm_opp_set_clkname(dev, "core");
 	}
 
-	if (adreno_read_speedbin(dev, &speedbin) || !speedbin)
+	if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin)
 		speedbin = 0xffff;
-	adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
+	adreno_gpu->speedbin = speedbin;
 
 	gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
 			ADRENO_CHIPID_ARGS(config->chip_id));
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 92caba3584da0400b44a903e465814af165d40a3..3946b9e992b9a8e2fd81f3e03354f9f83717b270 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -80,6 +80,10 @@ struct adreno_reglist {
 
 struct adreno_speedbin {
 	uint16_t fuse;
+/* As of SM8650, PCODE on production SoCs is meaningless wrt the GPU bin */
+#define ADRENO_SKU_ID_FCODE		GENMASK(15, 0)
+#define ADRENO_SKU_ID(fcode)	(fcode)
+
 	uint16_t speedbin;
 };
 
@@ -634,7 +638,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
 			 struct adreno_smmu_fault_info *info, const char *block,
 			 u32 scratch[4]);
 
-int adreno_read_speedbin(struct device *dev, u32 *speedbin);
+int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
+			 struct device *dev, u32 *speedbin);
 
 /*
  * For a5xx and a6xx targets load the zap shader that is used to pull the GPU

-- 
2.49.0

[PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@linaro.org>

Add speebin data for A740, as found on SM8550 and derivative SoCs.

For non-development SoCs it seems that "everything except FC_AC, FC_AF
should be speedbin 1", but what the values are for said "everything" are
not known, so that's an exercise left to the user..

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
@@ -11,6 +11,9 @@
 #include "a6xx.xml.h"
 #include "a6xx_gmu.xml.h"
 
+#include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/socinfo.h>
+
 static const struct adreno_reglist a612_hwcg[] = {
 	{REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
 	{REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
@@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
 		},
 		.address_space_size = SZ_16G,
 		.preempt_record_size = 4192 * SZ_1K,
+		.speedbins = ADRENO_SPEEDBINS(
+			{ ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
+			{ ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
+			/* Other feature codes (on prod SoCs) should match to speedbin 1 */
+		),
 	}, {
 		.chip_ids = ADRENO_CHIP_IDS(0x43050c01), /* "C512v2" */
 		.family = ADRENO_7XX_GEN2,

-- 
2.49.0

[PATCH RFT v6 3/5] drm/msm/adreno: Define A530 speed bins explicitly

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@linaro.org>

In preparation for commonizing the speedbin handling code.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_catalog.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_catalog.c b/drivers/gpu/drm/msm/adreno/a5xx_catalog.c
index 633f3153916277b34f90acc046ed2ee04a761727..105b3d14bd7592c784863346cfbcc28f000c2e8f 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_catalog.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_catalog.c
@@ -129,6 +129,12 @@ static const struct adreno_info a5xx_gpus[] = {
 			ADRENO_QUIRK_FAULT_DETECT_MASK,
 		.init = a5xx_gpu_init,
 		.zapfw = "a530_zap.mdt",
+		.speedbins = ADRENO_SPEEDBINS(
+			{ 0, 0 },
+			{ 1, 1 },
+			{ 2, 2 },
+			{ 3, 3 },
+		),
 	}, {
 		.chip_ids = ADRENO_CHIP_IDS(0x05040001),
 		.family = ADRENO_5XX,

-- 
2.49.0

[PATCH RFT v6 4/5] drm/msm/adreno: Redo the speedbin assignment

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@linaro.org>

There is no need to reinvent the wheel for simple read-match-set logic.

Make speedbin discovery and assignment generation independent.

This implicitly removes the bogus 0x80 / BIT(7) speed bin on A5xx,
which has no representation in hardware whatshowever.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c   | 34 --------------
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 56 -----------------------
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 81 +++++++++++++++++++++++++++++----
 drivers/gpu/drm/msm/adreno/adreno_gpu.h |  3 --
 4 files changed, 71 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 650e5bac225f372e819130b891f1d020b464f17f..7b9e4292ff53aa608244525063577baae0314abd 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1717,38 +1717,6 @@ static const struct adreno_gpu_funcs funcs = {
 	.get_timestamp = a5xx_get_timestamp,
 };
 
-static void check_speed_bin(struct device *dev)
-{
-	struct nvmem_cell *cell;
-	u32 val;
-
-	/*
-	 * If the OPP table specifies a opp-supported-hw property then we have
-	 * to set something with dev_pm_opp_set_supported_hw() or the table
-	 * doesn't get populated so pick an arbitrary value that should
-	 * ensure the default frequencies are selected but not conflict with any
-	 * actual bins
-	 */
-	val = 0x80;
-
-	cell = nvmem_cell_get(dev, "speed_bin");
-
-	if (!IS_ERR(cell)) {
-		void *buf = nvmem_cell_read(cell, NULL);
-
-		if (!IS_ERR(buf)) {
-			u8 bin = *((u8 *) buf);
-
-			val = (1 << bin);
-			kfree(buf);
-		}
-
-		nvmem_cell_put(cell);
-	}
-
-	devm_pm_opp_set_supported_hw(dev, &val, 1);
-}
-
 struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
@@ -1771,8 +1739,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
 
 	a5xx_gpu->lm_leakage = 0x4E001A;
 
-	check_speed_bin(&pdev->dev);
-
 	nr_rings = 4;
 
 	if (config->info->revn == 510)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 0db57e4b7596b01c091ed82510cf14cf2a8e0d03..ac6a8acceffc5b5ea0262c8ba49d5774f03894d8 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2316,55 +2316,6 @@ static bool a6xx_progress(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 	return progress;
 }
 
-static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse)
-{
-	if (!info->speedbins)
-		return UINT_MAX;
-
-	for (int i = 0; info->speedbins[i].fuse != SHRT_MAX; i++)
-		if (info->speedbins[i].fuse == fuse)
-			return BIT(info->speedbins[i].speedbin);
-
-	return UINT_MAX;
-}
-
-static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu,
-				 struct device *dev,
-				 const struct adreno_info *info)
-{
-	u32 supp_hw;
-	u32 speedbin;
-	int ret;
-
-	ret = adreno_read_speedbin(adreno_gpu, dev, &speedbin);
-	/*
-	 * -ENOENT/EOPNOTSUPP means that the platform doesn't support speedbin
-	 * which is fine
-	 */
-	if (ret == -ENOENT || ret == -EOPNOTSUPP) {
-		return 0;
-	} else if (ret) {
-		dev_err_probe(dev, ret,
-			      "failed to read speed-bin. Some OPPs may not be supported by hardware\n");
-		return ret;
-	}
-
-	supp_hw = fuse_to_supp_hw(info, speedbin);
-
-	if (supp_hw == UINT_MAX) {
-		DRM_DEV_ERROR(dev,
-			"missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n",
-			speedbin);
-		supp_hw = BIT(0); /* Default */
-	}
-
-	ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
 static const struct adreno_gpu_funcs funcs = {
 	.base = {
 		.get_param = adreno_get_param,
@@ -2497,13 +2448,6 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
 
 	a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx);
 
-	ret = a6xx_set_supported_hw(adreno_gpu, &pdev->dev, config->info);
-	if (ret) {
-		a6xx_llc_slices_destroy(a6xx_gpu);
-		kfree(a6xx_gpu);
-		return ERR_PTR(ret);
-	}
-
 	if ((enable_preemption == 1) || (enable_preemption == -1 &&
 	    (config->info->quirks & ADRENO_QUIRK_PREEMPTION)))
 		ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs_a7xx, 4);
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index b0ec64e9a35591507f26e16b4ef60ec874dafe12..339e210db06398eae88f425574e684ea5b5ad68b 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -1093,13 +1093,13 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
 			   adreno_ocmem->hdl);
 }
 
-int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
-			 struct device *dev, u32 *fuse)
+static int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
+				struct device *dev, u32 *fuse)
 {
 	int ret;
 
 	/*
-	 * Try reading the speedbin via a nvmem cell first
+	 * Try reading the speedbin via a nvmem cell first.
 	 * -ENOENT means "no nvmem-cells" and essentially means "old DT" or
 	 * "nvmem fuse is irrelevant", simply assume it's fine.
 	 */
@@ -1114,8 +1114,7 @@ int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
 
 	/*
 	 * Only check the feature code - the product code only matters for
-	 * proto SoCs unavailable outside Qualcomm labs, as far as GPU bin
-	 * matching is concerned.
+	 * prototype SoCs, as far as GPU bin matching is concerned.
 	 *
 	 * Ignore EOPNOTSUPP, as not all SoCs expose this info through SMEM.
 	 */
@@ -1126,7 +1125,70 @@ int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
 		return dev_err_probe(dev, ret, "Couldn't get feature code from SMEM\n");
 #endif
 
-	return ret;
+	return 0;
+}
+
+#define ADRENO_SPEEDBIN_FUSE_NODATA	0xFFFF /* Made-up large value, expected by mesa */
+static int adreno_set_speedbin(struct adreno_gpu *adreno_gpu, struct device *dev)
+{
+	const struct adreno_info *info = adreno_gpu->info;
+	u32 fuse = ADRENO_SPEEDBIN_FUSE_NODATA;
+	u32 supp_hw = UINT_MAX;
+	int ret;
+
+	/* No speedbins defined for this GPU SKU => allow all defined OPPs. */
+	if (!info->speedbins) {
+		/*
+		 * Due to OPP framework's safety assumptions, the speedbin data
+		 * must be defined either in both the driver catalog and DT, or
+		 * in neither.
+		 *
+		 * This means that having a too old DT (with no opp-supported-hw)
+		 * or a "too new" one (with opp-supported-hw, but the kernel not
+		 * having the fuse matching tables) becomes a problem.
+		 *
+		 * We can only work around one of these at a time, so choose the
+		 * more real issue of outdated DT and skip setting compatibility
+		 * constraints so as not to trip off any alarms in _opp_is_supported()
+		 */
+		adreno_gpu->speedbin = ADRENO_SPEEDBIN_FUSE_NODATA;
+
+		return 0;
+	}
+
+	/*
+	 * If a real error (not counting older devicetrees having no nvmem references)
+	 * occurs when trying to get the fuse value, bail out.
+	 */
+	ret = adreno_read_speedbin(adreno_gpu, dev, &fuse);
+	if (ret) {
+		return ret;
+	} else if (fuse == ADRENO_SPEEDBIN_FUSE_NODATA) {
+		/* The info struct has speedbin data, but the DT doesn't => allow all OPPs */
+		DRM_DEV_INFO(dev, "No GPU speed bin fuse, please update your device tree\n");
+
+		/*
+		 * In this case, the DT doesn't point us to speedbin info.
+		 * We can then assume the opp-supported-hw properties are missing as well,
+		 * as having one in place but not the other would be rather invalid anyway.
+		 *
+		 * We're not calling set_supported_hw(ALLOW_ALL_OPPS) here, as OPP APIs
+		 * would then still expect opp-supported-hw, which we may lack
+		 */
+		return 0;
+	}
+
+	adreno_gpu->speedbin = fuse;
+
+	/* Traverse the known speedbins */
+	for (int i = 0; info->speedbins[i].fuse != SHRT_MAX; i++) {
+		if (info->speedbins[i].fuse == fuse) {
+			supp_hw = BIT(info->speedbins[i].speedbin);
+			return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
+		}
+	}
+
+	return dev_err_probe(dev, -EINVAL, "Unknown speed bin fuse value: 0x%x\n", fuse);
 }
 
 int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
@@ -1138,7 +1200,6 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 	struct msm_gpu_config adreno_gpu_config  = { 0 };
 	struct msm_gpu *gpu = &adreno_gpu->base;
 	const char *gpu_name;
-	u32 speedbin;
 	int ret;
 
 	adreno_gpu->funcs = funcs;
@@ -1166,9 +1227,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 			devm_pm_opp_set_clkname(dev, "core");
 	}
 
-	if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin)
-		speedbin = 0xffff;
-	adreno_gpu->speedbin = speedbin;
+	ret = adreno_set_speedbin(adreno_gpu, dev);
+	if (ret)
+		return ret;
 
 	gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
 			ADRENO_CHIPID_ARGS(config->chip_id));
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 3946b9e992b9a8e2fd81f3e03354f9f83717b270..bc46ed5608cd3f5c05142826a03b97d5cdab9eff 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -638,9 +638,6 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
 			 struct adreno_smmu_fault_info *info, const char *block,
 			 u32 scratch[4]);
 
-int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
-			 struct device *dev, u32 *speedbin);
-
 /*
  * For a5xx and a6xx targets load the zap shader that is used to pull the GPU
  * out of secure mode

-- 
2.49.0

[PATCH RFT v6 5/5] arm64: dts: qcom: sm8550: Wire up GPU speed bin & more OPPs

[Konrad Dybcio]

From: Konrad Dybcio <konrad.dybcio@linaro.org>

Add the speedbin masks to ensure only the desired OPPs are available on
chips of a given bin.

Using this, add the binned 719 MHz OPP and the non-binned 124.8 MHz.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index 82cabf777cd2c1dc87457aeede913873e7322ec2..1c006879bbfe01d7b20e6fab620affb61e31ecec 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -2460,56 +2460,75 @@ zap-shader {
 				memory-region = <&gpu_micro_code_mem>;
 			};
 
-			/* Speedbin needs more work on A740+, keep only lower freqs */
 			gpu_opp_table: opp-table {
 				compatible = "operating-points-v2";
 
+				opp-719000000 {
+					opp-hz = /bits/ 64 <719000000>;
+					opp-level = <RPMH_REGULATOR_LEVEL_SVS_L2>;
+					opp-supported-hw = <0x1>;
+				};
+
 				opp-680000000 {
 					opp-hz = /bits/ 64 <680000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS_L1>;
 					opp-peak-kBps = <16500000>;
+					opp-supported-hw = <0x3>;
 				};
 
 				opp-615000000 {
 					opp-hz = /bits/ 64 <615000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS_L0>;
 					opp-peak-kBps = <12449218>;
+					opp-supported-hw = <0x3>;
 				};
 
 				opp-550000000 {
 					opp-hz = /bits/ 64 <550000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
 					opp-peak-kBps = <10687500>;
+					opp-supported-hw = <0x3>;
 				};
 
 				opp-475000000 {
 					opp-hz = /bits/ 64 <475000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_L1>;
 					opp-peak-kBps = <6074218>;
+					opp-supported-hw = <0x3>;
 				};
 
 				opp-401000000 {
 					opp-hz = /bits/ 64 <401000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
 					opp-peak-kBps = <6074218>;
+					opp-supported-hw = <0x3>;
 				};
 
 				opp-348000000 {
 					opp-hz = /bits/ 64 <348000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D0>;
 					opp-peak-kBps = <6074218>;
+					opp-supported-hw = <0x3>;
 				};
 
 				opp-295000000 {
 					opp-hz = /bits/ 64 <295000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>;
 					opp-peak-kBps = <6074218>;
+					opp-supported-hw = <0x3>;
 				};
 
 				opp-220000000 {
 					opp-hz = /bits/ 64 <220000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D2>;
 					opp-peak-kBps = <2136718>;
+					opp-supported-hw = <0x3>;
+				};
+
+				opp-124800000 {
+					opp-hz = /bits/ 64 <124800000>;
+					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D2>;
+					opp-supported-hw = <0x3>;
 				};
 			};
 		};

-- 
2.49.0

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[neil.armstrong]

Hi,

On 30/04/2025 13:34, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Add speebin data for A740, as found on SM8550 and derivative SoCs.
> 
> For non-development SoCs it seems that "everything except FC_AC, FC_AF
> should be speedbin 1", but what the values are for said "everything" are
> not known, so that's an exercise left to the user..
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> @@ -11,6 +11,9 @@
>   #include "a6xx.xml.h"
>   #include "a6xx_gmu.xml.h"
>   
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/soc/qcom/socinfo.h>
> +
>   static const struct adreno_reglist a612_hwcg[] = {
>   	{REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>   	{REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>   		},
>   		.address_space_size = SZ_16G,
>   		.preempt_record_size = 4192 * SZ_1K,
> +		.speedbins = ADRENO_SPEEDBINS(
> +			{ ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
> +			{ ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
> +			/* Other feature codes (on prod SoCs) should match to speedbin 1 */

I'm trying to understand this sentence. because reading patch 4, when there's no match
devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?

Before this change the fallback was speedbin = BIT(0), but this disappeared.

Neil

> +		),
>   	}, {
>   		.chip_ids = ADRENO_CHIP_IDS(0x43050c01), /* "C512v2" */
>   		.family = ADRENO_7XX_GEN2,
> 

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Konrad Dybcio]

On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 30/04/2025 13:34, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>
>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>
>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>> should be speedbin 1", but what the values are for said "everything" are
>> not known, so that's an exercise left to the user..
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>> @@ -11,6 +11,9 @@
>>   #include "a6xx.xml.h"
>>   #include "a6xx_gmu.xml.h"
>>   +#include <linux/soc/qcom/smem.h>
>> +#include <linux/soc/qcom/socinfo.h>
>> +
>>   static const struct adreno_reglist a612_hwcg[] = {
>>       {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>       {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>           },
>>           .address_space_size = SZ_16G,
>>           .preempt_record_size = 4192 * SZ_1K,
>> +        .speedbins = ADRENO_SPEEDBINS(
>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>> +            /* Other feature codes (on prod SoCs) should match to speedbin 1 */
> 
> I'm trying to understand this sentence. because reading patch 4, when there's no match
> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?

What I'm saying is that all other entries that happen to be possibly
added down the line are expected to be speedbin 1 (i.e. BIT(1))

> Before this change the fallback was speedbin = BIT(0), but this disappeared.

No, the default was to allow speedbin mask ~(0U)

Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[neil.armstrong]

On 30/04/2025 14:35, Konrad Dybcio wrote:
> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>> Hi,
>>
>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>
>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>
>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>> should be speedbin 1", but what the values are for said "everything" are
>>> not known, so that's an exercise left to the user..
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>> @@ -11,6 +11,9 @@
>>>    #include "a6xx.xml.h"
>>>    #include "a6xx_gmu.xml.h"
>>>    +#include <linux/soc/qcom/smem.h>
>>> +#include <linux/soc/qcom/socinfo.h>
>>> +
>>>    static const struct adreno_reglist a612_hwcg[] = {
>>>        {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>        {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>            },
>>>            .address_space_size = SZ_16G,
>>>            .preempt_record_size = 4192 * SZ_1K,
>>> +        .speedbins = ADRENO_SPEEDBINS(
>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>> +            /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>
>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
> 
> What I'm saying is that all other entries that happen to be possibly
> added down the line are expected to be speedbin 1 (i.e. BIT(1))
> 
>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
> 
> No, the default was to allow speedbin mask ~(0U)

Hmm no:

	supp_hw = fuse_to_supp_hw(info, speedbin);

	if (supp_hw == UINT_MAX) {
		DRM_DEV_ERROR(dev,
			"missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n",
			speedbin);
		supp_hw = BIT(0); /* Default */
	}

	ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
	if (ret)
		return ret;

> 
> Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Konrad Dybcio]

On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 14:35, Konrad Dybcio wrote:
>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>> Hi,
>>>
>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>
>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>
>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>> should be speedbin 1", but what the values are for said "everything" are
>>>> not known, so that's an exercise left to the user..
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>> @@ -11,6 +11,9 @@
>>>>    #include "a6xx.xml.h"
>>>>    #include "a6xx_gmu.xml.h"
>>>>    +#include <linux/soc/qcom/smem.h>
>>>> +#include <linux/soc/qcom/socinfo.h>
>>>> +
>>>>    static const struct adreno_reglist a612_hwcg[] = {
>>>>        {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>        {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>            },
>>>>            .address_space_size = SZ_16G,
>>>>            .preempt_record_size = 4192 * SZ_1K,
>>>> +        .speedbins = ADRENO_SPEEDBINS(
>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>> +            /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>
>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>
>> What I'm saying is that all other entries that happen to be possibly
>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>
>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>
>> No, the default was to allow speedbin mask ~(0U)
> 
> Hmm no:
> 
>     supp_hw = fuse_to_supp_hw(info, speedbin);
> 
>     if (supp_hw == UINT_MAX) {
>         DRM_DEV_ERROR(dev,
>             "missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n",
>             speedbin);
>         supp_hw = BIT(0); /* Default */
>     }
> 
>     ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>     if (ret)
>         return ret;

Right, that's my own code even..

in any case, the kernel can't know about the speed bins that aren't
defined and here we only define bin0, which doesn't break things

the kernel isn't aware about hw with bin1 with or without this change
so it effectively doesn't matter

Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[neil.armstrong]

On 30/04/2025 15:09, Konrad Dybcio wrote:
> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>> Hi,
>>>>
>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>
>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>
>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>> not known, so that's an exercise left to the user..
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>     1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>> @@ -11,6 +11,9 @@
>>>>>     #include "a6xx.xml.h"
>>>>>     #include "a6xx_gmu.xml.h"
>>>>>     +#include <linux/soc/qcom/smem.h>
>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>> +
>>>>>     static const struct adreno_reglist a612_hwcg[] = {
>>>>>         {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>         {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>             },
>>>>>             .address_space_size = SZ_16G,
>>>>>             .preempt_record_size = 4192 * SZ_1K,
>>>>> +        .speedbins = ADRENO_SPEEDBINS(
>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>> +            /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>
>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>
>>> What I'm saying is that all other entries that happen to be possibly
>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>
>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>
>>> No, the default was to allow speedbin mask ~(0U)
>>
>> Hmm no:
>>
>>      supp_hw = fuse_to_supp_hw(info, speedbin);
>>
>>      if (supp_hw == UINT_MAX) {
>>          DRM_DEV_ERROR(dev,
>>              "missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n",
>>              speedbin);
>>          supp_hw = BIT(0); /* Default */
>>      }
>>
>>      ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>      if (ret)
>>          return ret;
> 
> Right, that's my own code even..
> 
> in any case, the kernel can't know about the speed bins that aren't
> defined and here we only define bin0, which doesn't break things
> 
> the kernel isn't aware about hw with bin1 with or without this change
> so it effectively doesn't matter

But it's regression for the other platforms, where before an unknown SKU
mapped to supp_hw=BIT(0)

Not calling devm_pm_opp_set_supported_hw() is a major regression,
if the opp-supported-hw is present, the OPP will be rejected:

https://elixir.bootlin.com/linux/v6.14.4/source/drivers/opp/of.c#L538

	if (!opp_table->supported_hw) {
		/*
		 * In the case that no supported_hw has been set by the
		 * platform but there is an opp-supported-hw value set for
		 * an OPP then the OPP should not be enabled as there is
		 * no way to see if the hardware supports it.
		 */
		if (of_property_present(np, "opp-supported-hw"))
			return false;
		else
			return true;
	}

Neil

> 
> Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Konrad Dybcio]

On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 15:09, Konrad Dybcio wrote:
>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>> Hi,
>>>>>
>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>
>>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>>
>>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>>> not known, so that's an exercise left to the user..
>>>>>>
>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>> ---
>>>>>>     drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>     1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>> @@ -11,6 +11,9 @@
>>>>>>     #include "a6xx.xml.h"
>>>>>>     #include "a6xx_gmu.xml.h"
>>>>>>     +#include <linux/soc/qcom/smem.h>
>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>> +
>>>>>>     static const struct adreno_reglist a612_hwcg[] = {
>>>>>>         {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>         {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>>             },
>>>>>>             .address_space_size = SZ_16G,
>>>>>>             .preempt_record_size = 4192 * SZ_1K,
>>>>>> +        .speedbins = ADRENO_SPEEDBINS(
>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>> +            /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>>
>>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>>
>>>> What I'm saying is that all other entries that happen to be possibly
>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>
>>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>>
>>>> No, the default was to allow speedbin mask ~(0U)
>>>
>>> Hmm no:
>>>
>>>      supp_hw = fuse_to_supp_hw(info, speedbin);
>>>
>>>      if (supp_hw == UINT_MAX) {
>>>          DRM_DEV_ERROR(dev,
>>>              "missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n",
>>>              speedbin);
>>>          supp_hw = BIT(0); /* Default */
>>>      }
>>>
>>>      ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>      if (ret)
>>>          return ret;
>>
>> Right, that's my own code even..
>>
>> in any case, the kernel can't know about the speed bins that aren't
>> defined and here we only define bin0, which doesn't break things
>>
>> the kernel isn't aware about hw with bin1 with or without this change
>> so it effectively doesn't matter
> 
> But it's regression for the other platforms, where before an unknown SKU
> mapped to supp_hw=BIT(0)
> 
> Not calling devm_pm_opp_set_supported_hw() is a major regression,
> if the opp-supported-hw is present, the OPP will be rejected:

A comment in patch 4 explains that. We can either be forwards or backwards
compatible (i.e. accept a limited amount of
speedbin_in_driver x speedbin_in_dt combinations)

Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[neil.armstrong]

On 30/04/2025 17:36, Konrad Dybcio wrote:
> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>
>>>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>>>
>>>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>>>> not known, so that's an exercise left to the user..
>>>>>>>
>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>>      1 file changed, 8 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>> @@ -11,6 +11,9 @@
>>>>>>>      #include "a6xx.xml.h"
>>>>>>>      #include "a6xx_gmu.xml.h"
>>>>>>>      +#include <linux/soc/qcom/smem.h>
>>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>>> +
>>>>>>>      static const struct adreno_reglist a612_hwcg[] = {
>>>>>>>          {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>>          {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>>>              },
>>>>>>>              .address_space_size = SZ_16G,
>>>>>>>              .preempt_record_size = 4192 * SZ_1K,
>>>>>>> +        .speedbins = ADRENO_SPEEDBINS(
>>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>>> +            /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>>>
>>>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>>>
>>>>> What I'm saying is that all other entries that happen to be possibly
>>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>>
>>>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>>>
>>>>> No, the default was to allow speedbin mask ~(0U)
>>>>
>>>> Hmm no:
>>>>
>>>>       supp_hw = fuse_to_supp_hw(info, speedbin);
>>>>
>>>>       if (supp_hw == UINT_MAX) {
>>>>           DRM_DEV_ERROR(dev,
>>>>               "missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n",
>>>>               speedbin);
>>>>           supp_hw = BIT(0); /* Default */
>>>>       }
>>>>
>>>>       ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>>       if (ret)
>>>>           return ret;
>>>
>>> Right, that's my own code even..
>>>
>>> in any case, the kernel can't know about the speed bins that aren't
>>> defined and here we only define bin0, which doesn't break things
>>>
>>> the kernel isn't aware about hw with bin1 with or without this change
>>> so it effectively doesn't matter
>>
>> But it's regression for the other platforms, where before an unknown SKU
>> mapped to supp_hw=BIT(0)
>>
>> Not calling devm_pm_opp_set_supported_hw() is a major regression,
>> if the opp-supported-hw is present, the OPP will be rejected:
> 
> A comment in patch 4 explains that. We can either be forwards or backwards
> compatible (i.e. accept a limited amount of
> speedbin_in_driver x speedbin_in_dt combinations)

I have a hard time understanding the change, please be much more verbose
in the cover letter and commit messages.

The fact that you do such a large change in the speedbin policy in patch 4
makes it hard to understand why it's needed in the first place.

Finally I'm very concerned that "old" SM8550 DT won't work on new kernels,
this is frankly unacceptable, and this should be addressed in the first
place.

The nvmem situation was much simple, where we considered we added the nvmem
property at the same time as opp-supported-hw in OPPs, but it's no more the
case.

So I think the OPP API should probably be extended to address this situation
first, since if we do not have the opp-supported-hw in OPPs, all OPPs are safe.

So this code:
	count = of_property_count_u32_elems(np, "opp-supported-hw");
	if (count <= 0 || count % levels) {
		dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
			__func__, count);
		return false;
	}
should return true in this specific case, like a supported_hw_failsafe mode.

Neil

> 
> Konrad

Re: [PATCH RFT v6 1/5] drm/msm/adreno: Implement SMEM-based speed bin

[neil.armstrong]

On 30/04/2025 13:34, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
> abstracted through SMEM, instead of being directly available in a fuse.
> 
> Add support for SMEM-based speed binning, which includes getting
> "feature code" and "product code" from said source and parsing them
> to form something that lets us match OPPs against.
> 
> Due to the product code being ignored in the context of Adreno on
> production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 14 +++++-----
>   drivers/gpu/drm/msm/adreno/adreno_device.c |  2 ++
>   drivers/gpu/drm/msm/adreno/adreno_gpu.c    | 42 +++++++++++++++++++++++++++---
>   drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  7 ++++-
>   4 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 242d02d48c0cd0972bb96a960872b73384fe043b..0db57e4b7596b01c091ed82510cf14cf2a8e0d03 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2328,18 +2328,20 @@ static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse)
>   	return UINT_MAX;
>   }
>   
> -static int a6xx_set_supported_hw(struct device *dev, const struct adreno_info *info)
> +static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu,
> +				 struct device *dev,
> +				 const struct adreno_info *info)
>   {
>   	u32 supp_hw;
>   	u32 speedbin;
>   	int ret;
>   
> -	ret = adreno_read_speedbin(dev, &speedbin);
> +	ret = adreno_read_speedbin(adreno_gpu, dev, &speedbin);
>   	/*
> -	 * -ENOENT means that the platform doesn't support speedbin which is
> -	 * fine
> +	 * -ENOENT/EOPNOTSUPP means that the platform doesn't support speedbin
> +	 * which is fine
>   	 */
> -	if (ret == -ENOENT) {
> +	if (ret == -ENOENT || ret == -EOPNOTSUPP) {
>   		return 0;
>   	} else if (ret) {
>   		dev_err_probe(dev, ret,
> @@ -2495,7 +2497,7 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>   
>   	a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx);
>   
> -	ret = a6xx_set_supported_hw(&pdev->dev, config->info);
> +	ret = a6xx_set_supported_hw(adreno_gpu, &pdev->dev, config->info);
>   	if (ret) {
>   		a6xx_llc_slices_destroy(a6xx_gpu);
>   		kfree(a6xx_gpu);
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 236b25c094cd5d462f4b6653de7b7910985cccb6..a8376574381abff90d4a56e86f3f05735027ca9f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -6,6 +6,8 @@
>    * Copyright (c) 2014,2017 The Linux Foundation. All rights reserved.
>    */
>   
> +#include <linux/soc/qcom/socinfo.h>
> +
>   #include "adreno_gpu.h"
>   
>   bool hang_debug = false;
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 26db1f4b5fb90930bdbd2f17682bf47e35870936..b0ec64e9a35591507f26e16b4ef60ec874dafe12 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -21,6 +21,9 @@
>   #include "msm_gem.h"
>   #include "msm_mmu.h"
>   
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/soc/qcom/socinfo.h>
> +
>   static u64 address_space_size = 0;
>   MODULE_PARM_DESC(address_space_size, "Override for size of processes private GPU address space");
>   module_param(address_space_size, ullong, 0600);
> @@ -1090,9 +1093,40 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
>   			   adreno_ocmem->hdl);
>   }
>   
> -int adreno_read_speedbin(struct device *dev, u32 *speedbin)
> +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
> +			 struct device *dev, u32 *fuse)
>   {
> -	return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin);
> +	int ret;
> +
> +	/*
> +	 * Try reading the speedbin via a nvmem cell first
> +	 * -ENOENT means "no nvmem-cells" and essentially means "old DT" or
> +	 * "nvmem fuse is irrelevant", simply assume it's fine.
> +	 */
> +	ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", fuse);
> +	if (!ret)
> +		return 0;
> +	else if (ret != -ENOENT)
> +		return dev_err_probe(dev, ret, "Couldn't read the speed bin fuse value\n");
> +
> +#ifdef CONFIG_QCOM_SMEM
> +	u32 fcode;
> +
> +	/*
> +	 * Only check the feature code - the product code only matters for
> +	 * proto SoCs unavailable outside Qualcomm labs, as far as GPU bin
> +	 * matching is concerned.
> +	 *
> +	 * Ignore EOPNOTSUPP, as not all SoCs expose this info through SMEM.
> +	 */
> +	ret = qcom_smem_get_feature_code(&fcode);
> +	if (!ret)
> +		*fuse = ADRENO_SKU_ID(fcode);
> +	else if (ret != -EOPNOTSUPP)
> +		return dev_err_probe(dev, ret, "Couldn't get feature code from SMEM\n");
> +#endif
> +
> +	return ret;
>   }
>   
>   int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> @@ -1132,9 +1166,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   			devm_pm_opp_set_clkname(dev, "core");
>   	}
>   
> -	if (adreno_read_speedbin(dev, &speedbin) || !speedbin)
> +	if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin)
>   		speedbin = 0xffff;
> -	adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
> +	adreno_gpu->speedbin = speedbin;
>   
>   	gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
>   			ADRENO_CHIPID_ARGS(config->chip_id));
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 92caba3584da0400b44a903e465814af165d40a3..3946b9e992b9a8e2fd81f3e03354f9f83717b270 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -80,6 +80,10 @@ struct adreno_reglist {
>   
>   struct adreno_speedbin {
>   	uint16_t fuse;
> +/* As of SM8650, PCODE on production SoCs is meaningless wrt the GPU bin */

This should be SM8550

> +#define ADRENO_SKU_ID_FCODE		GENMASK(15, 0)
> +#define ADRENO_SKU_ID(fcode)	(fcode)
> +
>   	uint16_t speedbin;
>   };
>   
> @@ -634,7 +638,8 @@ int adreno_fault_handler(struct msm_gpu *gpu, unsigned long iova, int flags,
>   			 struct adreno_smmu_fault_info *info, const char *block,
>   			 u32 scratch[4]);
>   
> -int adreno_read_speedbin(struct device *dev, u32 *speedbin);
> +int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
> +			 struct device *dev, u32 *speedbin);
>   
>   /*
>    * For a5xx and a6xx targets load the zap shader that is used to pull the GPU
> 

Re: [PATCH RFT v6 1/5] drm/msm/adreno: Implement SMEM-based speed bin

[Konrad Dybcio]

On 4/30/25 6:20 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 13:34, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>
>> On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
>> abstracted through SMEM, instead of being directly available in a fuse.
>>
>> Add support for SMEM-based speed binning, which includes getting
>> "feature code" and "product code" from said source and parsing them
>> to form something that lets us match OPPs against.
>>
>> Due to the product code being ignored in the context of Adreno on
>> production parts (as of SM8650), hardcode it to SOCINFO_PC_UNKNOWN.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---

[...]

>> +/* As of SM8650, PCODE on production SoCs is meaningless wrt the GPU bin */
> 
> This should be SM8550

No, this is 8650 to signify that this holds true even later
Looking into it, I can even say 8750 here now

Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Konrad Dybcio]

On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 17:36, Konrad Dybcio wrote:
>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>
>>>>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>>>>
>>>>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>>>>> not known, so that's an exercise left to the user..
>>>>>>>>
>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>>>      1 file changed, 8 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>> @@ -11,6 +11,9 @@
>>>>>>>>      #include "a6xx.xml.h"
>>>>>>>>      #include "a6xx_gmu.xml.h"
>>>>>>>>      +#include <linux/soc/qcom/smem.h>
>>>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>>>> +
>>>>>>>>      static const struct adreno_reglist a612_hwcg[] = {
>>>>>>>>          {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>>>          {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>>>>              },
>>>>>>>>              .address_space_size = SZ_16G,
>>>>>>>>              .preempt_record_size = 4192 * SZ_1K,
>>>>>>>> +        .speedbins = ADRENO_SPEEDBINS(
>>>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>>>> +            /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>>>>
>>>>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>>>>
>>>>>> What I'm saying is that all other entries that happen to be possibly
>>>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>>>
>>>>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>>>>
>>>>>> No, the default was to allow speedbin mask ~(0U)
>>>>>
>>>>> Hmm no:
>>>>>
>>>>>       supp_hw = fuse_to_supp_hw(info, speedbin);
>>>>>
>>>>>       if (supp_hw == UINT_MAX) {
>>>>>           DRM_DEV_ERROR(dev,
>>>>>               "missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n",
>>>>>               speedbin);
>>>>>           supp_hw = BIT(0); /* Default */
>>>>>       }
>>>>>
>>>>>       ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>>>       if (ret)
>>>>>           return ret;
>>>>
>>>> Right, that's my own code even..
>>>>
>>>> in any case, the kernel can't know about the speed bins that aren't
>>>> defined and here we only define bin0, which doesn't break things
>>>>
>>>> the kernel isn't aware about hw with bin1 with or without this change
>>>> so it effectively doesn't matter
>>>
>>> But it's regression for the other platforms, where before an unknown SKU
>>> mapped to supp_hw=BIT(0)
>>>
>>> Not calling devm_pm_opp_set_supported_hw() is a major regression,
>>> if the opp-supported-hw is present, the OPP will be rejected:
>>
>> A comment in patch 4 explains that. We can either be forwards or backwards
>> compatible (i.e. accept a limited amount of
>> speedbin_in_driver x speedbin_in_dt combinations)
> 
> I have a hard time understanding the change, please be much more verbose
> in the cover letter and commit messages.
> 
> The fact that you do such a large change in the speedbin policy in patch 4
> makes it hard to understand why it's needed in the first place.
> 
> Finally I'm very concerned that "old" SM8550 DT won't work on new kernels,
> this is frankly unacceptable, and this should be addressed in the first
> place.
> 
> The nvmem situation was much simple, where we considered we added the nvmem
> property at the same time as opp-supported-hw in OPPs, but it's no more the
> case.
> 
> So I think the OPP API should probably be extended to address this situation
> first, since if we do not have the opp-supported-hw in OPPs, all OPPs are safe.
> 
> So this code:
>     count = of_property_count_u32_elems(np, "opp-supported-hw");
>     if (count <= 0 || count % levels) {
>         dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
>             __func__, count);
>         return false;
>     }
> should return true in this specific case, like a supported_hw_failsafe mode.

Not really. opp-supported-hws = <BIT(0)> usually translates to the *fastest*
bin in our case, so perhaps that change I made previously to default to it
wasn't the wisest. In other words, all slower SKUs that weren't added to the
kernel catalog & dt are potentially getting overclocked, which is no bueno.
That is not always the case, but it most certainly has been for a number of
years.

Old DTs in this case would be DTs lacking opp-supported-hw with the kernel
having speedbin tables. The inverse ("too new DTs") case translates into
"someone put some unexpected stuff in dt and the kernel has no idea what
to do with it".
In this context, old DTs would continue to work after patch 4, as the first
early return in adreno_set_speedbin() takes care of that.

Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[neil.armstrong]

On 30/04/2025 18:39, Konrad Dybcio wrote:
> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
>> On 30/04/2025 17:36, Konrad Dybcio wrote:
>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>>
>>>>>>>>> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>>>>>>>>>
>>>>>>>>> For non-development SoCs it seems that "everything except FC_AC, FC_AF
>>>>>>>>> should be speedbin 1", but what the values are for said "everything" are
>>>>>>>>> not known, so that's an exercise left to the user..
>>>>>>>>>
>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>> ---
>>>>>>>>>       drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>>>>       1 file changed, 8 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>> index 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>> @@ -11,6 +11,9 @@
>>>>>>>>>       #include "a6xx.xml.h"
>>>>>>>>>       #include "a6xx_gmu.xml.h"
>>>>>>>>>       +#include <linux/soc/qcom/smem.h>
>>>>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>>>>> +
>>>>>>>>>       static const struct adreno_reglist a612_hwcg[] = {
>>>>>>>>>           {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>>>>           {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info a7xx_gpus[] = {
>>>>>>>>>               },
>>>>>>>>>               .address_space_size = SZ_16G,
>>>>>>>>>               .preempt_record_size = 4192 * SZ_1K,
>>>>>>>>> +        .speedbins = ADRENO_SPEEDBINS(
>>>>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>>>>> +            /* Other feature codes (on prod SoCs) should match to speedbin 1 */
>>>>>>>>
>>>>>>>> I'm trying to understand this sentence. because reading patch 4, when there's no match
>>>>>>>> devm_pm_opp_set_supported_hw() is simply never called so how can it match speedbin 1 ?
>>>>>>>
>>>>>>> What I'm saying is that all other entries that happen to be possibly
>>>>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>>>>
>>>>>>>> Before this change the fallback was speedbin = BIT(0), but this disappeared.
>>>>>>>
>>>>>>> No, the default was to allow speedbin mask ~(0U)
>>>>>>
>>>>>> Hmm no:
>>>>>>
>>>>>>        supp_hw = fuse_to_supp_hw(info, speedbin);
>>>>>>
>>>>>>        if (supp_hw == UINT_MAX) {
>>>>>>            DRM_DEV_ERROR(dev,
>>>>>>                "missing support for speed-bin: %u. Some OPPs may not be supported by hardware\n",
>>>>>>                speedbin);
>>>>>>            supp_hw = BIT(0); /* Default */
>>>>>>        }
>>>>>>
>>>>>>        ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>>>>        if (ret)
>>>>>>            return ret;
>>>>>
>>>>> Right, that's my own code even..
>>>>>
>>>>> in any case, the kernel can't know about the speed bins that aren't
>>>>> defined and here we only define bin0, which doesn't break things
>>>>>
>>>>> the kernel isn't aware about hw with bin1 with or without this change
>>>>> so it effectively doesn't matter
>>>>
>>>> But it's regression for the other platforms, where before an unknown SKU
>>>> mapped to supp_hw=BIT(0)
>>>>
>>>> Not calling devm_pm_opp_set_supported_hw() is a major regression,
>>>> if the opp-supported-hw is present, the OPP will be rejected:
>>>
>>> A comment in patch 4 explains that. We can either be forwards or backwards
>>> compatible (i.e. accept a limited amount of
>>> speedbin_in_driver x speedbin_in_dt combinations)
>>
>> I have a hard time understanding the change, please be much more verbose
>> in the cover letter and commit messages.
>>
>> The fact that you do such a large change in the speedbin policy in patch 4
>> makes it hard to understand why it's needed in the first place.
>>
>> Finally I'm very concerned that "old" SM8550 DT won't work on new kernels,
>> this is frankly unacceptable, and this should be addressed in the first
>> place.
>>
>> The nvmem situation was much simple, where we considered we added the nvmem
>> property at the same time as opp-supported-hw in OPPs, but it's no more the
>> case.
>>
>> So I think the OPP API should probably be extended to address this situation
>> first, since if we do not have the opp-supported-hw in OPPs, all OPPs are safe.
>>
>> So this code:
>>      count = of_property_count_u32_elems(np, "opp-supported-hw");
>>      if (count <= 0 || count % levels) {
>>          dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
>>              __func__, count);
>>          return false;
>>      }
>> should return true in this specific case, like a supported_hw_failsafe mode.
> 
> Not really. opp-supported-hws = <BIT(0)> usually translates to the *fastest*
> bin in our case, so perhaps that change I made previously to default to it
> wasn't the wisest. In other words, all slower SKUs that weren't added to the
> kernel catalog & dt are potentially getting overclocked, which is no bueno.
> That is not always the case, but it most certainly has been for a number of
> years.
> 
> Old DTs in this case would be DTs lacking opp-supported-hw with the kernel
> having speedbin tables. The inverse ("too new DTs") case translates into
> "someone put some unexpected stuff in dt and the kernel has no idea what
> to do with it".
> In this context, old DTs would continue to work after patch 4, as the first
> early return in adreno_set_speedbin() takes care of that.

No.

With only patches 1-4 applied (keep "old" DT) on today's -next:

SM8550-QRD:
[    7.574569] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops dsi_ops [msm])
[    7.586578] msm_dpu ae01000.display-controller: bound ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
[    7.597886] adreno 3d00000.gpu: error -EINVAL: Unknown speed bin fuse value: 0x2
[    7.605518] msm_dpu ae01000.display-controller: failed to load adreno gpu
[    7.612599] msm_dpu ae01000.display-controller: failed to bind 3d00000.gpu (ops a3xx_ops [msm]): -22

SM8550-HDK:
[   10.137558] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops dsi_ops [msm])
[   10.151796] msm_dpu ae01000.display-controller: bound ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
[   10.163358] adreno 3d00000.gpu: error -EINVAL: Unknown speed bin fuse value: 0x2
[   10.171066] msm_dpu ae01000.display-controller: failed to load adreno gpu
[   10.178118] msm_dpu ae01000.display-controller: failed to bind 3d00000.gpu (ops a3xx_ops [msm]): -22

With:
=================><==================
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
index 61daa3315679..7cac14a585a9 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
@@ -1435,6 +1435,7 @@ static const struct adreno_info a7xx_gpus[] = {
                 .address_space_size = SZ_16G,
                 .preempt_record_size = 4192 * SZ_1K,
                 .speedbins = ADRENO_SPEEDBINS(
+                       { ADRENO_SKU_ID(SOCINFO_FC_AB), 1 },
                         { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
                         { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
                         /* Other feature codes (on prod SoCs) should match to speedbin 1 */
=================><==================

SM8550-QRD:
[    7.681816] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops dsi_ops [msm])
[    7.694479] msm_dpu ae01000.display-controller: bound ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
[    7.705784] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[    7.714322] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[    7.722851] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[    7.722853] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[    7.722855] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[    7.722856] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[    7.722858] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[    7.722860] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[    7.722861] adreno 3d00000.gpu: _of_add_opp_table_v2: no supported OPPs
[    7.722863] adreno 3d00000.gpu: [drm:adreno_gpu_init [msm]] *ERROR* Unable to set the OPP table

SM8550-HDK:
[   10.119986] msm_dpu ae01000.display-controller: bound ae94000.dsi (ops dsi_ops [msm])
[   10.133872] msm_dpu ae01000.display-controller: bound ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
[   10.147377] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[   10.161640] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[   10.171198] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[   10.179756] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[   10.188313] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[   10.196868] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[   10.205424] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[   10.226025] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-supported-hw property (-22)
[   10.234589] adreno 3d00000.gpu: _of_add_opp_table_v2: no supported OPPs
[   10.247165] adreno 3d00000.gpu: [drm:adreno_gpu_init [msm]] *ERROR* Unable to set the OPP table

This behaves exactly as I said, so please fix it.

Neil

> 
> Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Akhil P Oommen]

On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote:
> On 30/04/2025 18:39, Konrad Dybcio wrote:
>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
>>> On 30/04/2025 17:36, Konrad Dybcio wrote:
>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 30/04/2025 13:34, Konrad Dybcio wrote:
>>>>>>>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>>>
>>>>>>>>>> Add speebin data for A740, as found on SM8550 and derivative
>>>>>>>>>> SoCs.
>>>>>>>>>>
>>>>>>>>>> For non-development SoCs it seems that "everything except
>>>>>>>>>> FC_AC, FC_AF
>>>>>>>>>> should be speedbin 1", but what the values are for said
>>>>>>>>>> "everything" are
>>>>>>>>>> not known, so that's an exercise left to the user..
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/gpu/drm/msm/adreno/a6xx_catalog.c | 8 ++++++++
>>>>>>>>>>       1 file changed, 8 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/
>>>>>>>>>> drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>>> index
>>>>>>>>>> 53e2ff4406d8f0afe474aaafbf0e459ef8f4577d..61daa331567925e529deae5e25d6fb63a8ba8375 100644
>>>>>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
>>>>>>>>>> @@ -11,6 +11,9 @@
>>>>>>>>>>       #include "a6xx.xml.h"
>>>>>>>>>>       #include "a6xx_gmu.xml.h"
>>>>>>>>>>       +#include <linux/soc/qcom/smem.h>
>>>>>>>>>> +#include <linux/soc/qcom/socinfo.h>
>>>>>>>>>> +
>>>>>>>>>>       static const struct adreno_reglist a612_hwcg[] = {
>>>>>>>>>>           {REG_A6XX_RBBM_CLOCK_CNTL_SP0, 0x22222222},
>>>>>>>>>>           {REG_A6XX_RBBM_CLOCK_CNTL2_SP0, 0x02222220},
>>>>>>>>>> @@ -1431,6 +1434,11 @@ static const struct adreno_info
>>>>>>>>>> a7xx_gpus[] = {
>>>>>>>>>>               },
>>>>>>>>>>               .address_space_size = SZ_16G,
>>>>>>>>>>               .preempt_record_size = 4192 * SZ_1K,
>>>>>>>>>> +        .speedbins = ADRENO_SPEEDBINS(
>>>>>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>>>>>>>>>> +            { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>>>>>>>>>> +            /* Other feature codes (on prod SoCs) should
>>>>>>>>>> match to speedbin 1 */
>>>>>>>>>
>>>>>>>>> I'm trying to understand this sentence. because reading patch
>>>>>>>>> 4, when there's no match
>>>>>>>>> devm_pm_opp_set_supported_hw() is simply never called so how
>>>>>>>>> can it match speedbin 1 ?
>>>>>>>>
>>>>>>>> What I'm saying is that all other entries that happen to be
>>>>>>>> possibly
>>>>>>>> added down the line are expected to be speedbin 1 (i.e. BIT(1))
>>>>>>>>
>>>>>>>>> Before this change the fallback was speedbin = BIT(0), but this
>>>>>>>>> disappeared.
>>>>>>>>
>>>>>>>> No, the default was to allow speedbin mask ~(0U)
>>>>>>>
>>>>>>> Hmm no:
>>>>>>>
>>>>>>>        supp_hw = fuse_to_supp_hw(info, speedbin);
>>>>>>>
>>>>>>>        if (supp_hw == UINT_MAX) {
>>>>>>>            DRM_DEV_ERROR(dev,
>>>>>>>                "missing support for speed-bin: %u. Some OPPs may
>>>>>>> not be supported by hardware\n",
>>>>>>>                speedbin);
>>>>>>>            supp_hw = BIT(0); /* Default */
>>>>>>>        }
>>>>>>>
>>>>>>>        ret = devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>>>>>>>        if (ret)
>>>>>>>            return ret;
>>>>>>
>>>>>> Right, that's my own code even..
>>>>>>
>>>>>> in any case, the kernel can't know about the speed bins that aren't
>>>>>> defined and here we only define bin0, which doesn't break things
>>>>>>
>>>>>> the kernel isn't aware about hw with bin1 with or without this change
>>>>>> so it effectively doesn't matter
>>>>>
>>>>> But it's regression for the other platforms, where before an
>>>>> unknown SKU
>>>>> mapped to supp_hw=BIT(0)
>>>>>
>>>>> Not calling devm_pm_opp_set_supported_hw() is a major regression,
>>>>> if the opp-supported-hw is present, the OPP will be rejected:
>>>>
>>>> A comment in patch 4 explains that. We can either be forwards or
>>>> backwards
>>>> compatible (i.e. accept a limited amount of
>>>> speedbin_in_driver x speedbin_in_dt combinations)
>>>
>>> I have a hard time understanding the change, please be much more verbose
>>> in the cover letter and commit messages.
>>>
>>> The fact that you do such a large change in the speedbin policy in
>>> patch 4
>>> makes it hard to understand why it's needed in the first place.
>>>
>>> Finally I'm very concerned that "old" SM8550 DT won't work on new
>>> kernels,
>>> this is frankly unacceptable, and this should be addressed in the first
>>> place.
>>>
>>> The nvmem situation was much simple, where we considered we added the
>>> nvmem
>>> property at the same time as opp-supported-hw in OPPs, but it's no
>>> more the
>>> case.
>>>
>>> So I think the OPP API should probably be extended to address this
>>> situation
>>> first, since if we do not have the opp-supported-hw in OPPs, all OPPs
>>> are safe.
>>>
>>> So this code:
>>>      count = of_property_count_u32_elems(np, "opp-supported-hw");
>>>      if (count <= 0 || count % levels) {
>>>          dev_err(dev, "%s: Invalid opp-supported-hw property (%d)\n",
>>>              __func__, count);
>>>          return false;
>>>      }
>>> should return true in this specific case, like a
>>> supported_hw_failsafe mode.
>>
>> Not really. opp-supported-hws = <BIT(0)> usually translates to the
>> *fastest*
>> bin in our case, so perhaps that change I made previously to default
>> to it
>> wasn't the wisest. In other words, all slower SKUs that weren't added
>> to the
>> kernel catalog & dt are potentially getting overclocked, which is no
>> bueno.
>> That is not always the case, but it most certainly has been for a
>> number of
>> years.
>>
>> Old DTs in this case would be DTs lacking opp-supported-hw with the
>> kernel
>> having speedbin tables. The inverse ("too new DTs") case translates into
>> "someone put some unexpected stuff in dt and the kernel has no idea what
>> to do with it".
>> In this context, old DTs would continue to work after patch 4, as the
>> first
>> early return in adreno_set_speedbin() takes care of that.
> 
> No.
> 
> With only patches 1-4 applied (keep "old" DT) on today's -next:
> 
> SM8550-QRD:
> [    7.574569] msm_dpu ae01000.display-controller: bound ae94000.dsi
> (ops dsi_ops [msm])
> [    7.586578] msm_dpu ae01000.display-controller: bound
> ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
> [    7.597886] adreno 3d00000.gpu: error -EINVAL: Unknown speed bin fuse
> value: 0x2
> [    7.605518] msm_dpu ae01000.display-controller: failed to load adreno
> gpu
> [    7.612599] msm_dpu ae01000.display-controller: failed to bind
> 3d00000.gpu (ops a3xx_ops [msm]): -22
> 
> SM8550-HDK:
> [   10.137558] msm_dpu ae01000.display-controller: bound ae94000.dsi
> (ops dsi_ops [msm])
> [   10.151796] msm_dpu ae01000.display-controller: bound
> ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
> [   10.163358] adreno 3d00000.gpu: error -EINVAL: Unknown speed bin fuse
> value: 0x2
> [   10.171066] msm_dpu ae01000.display-controller: failed to load adreno
> gpu
> [   10.178118] msm_dpu ae01000.display-controller: failed to bind
> 3d00000.gpu (ops a3xx_ops [msm]): -22
> 
> With:
> =================><==================
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c b/drivers/gpu/
> drm/msm/adreno/a6xx_catalog.c
> index 61daa3315679..7cac14a585a9 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_catalog.c
> @@ -1435,6 +1435,7 @@ static const struct adreno_info a7xx_gpus[] = {
>                 .address_space_size = SZ_16G,
>                 .preempt_record_size = 4192 * SZ_1K,
>                 .speedbins = ADRENO_SPEEDBINS(
> +                       { ADRENO_SKU_ID(SOCINFO_FC_AB), 1 },
>                         { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
>                         { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
>                         /* Other feature codes (on prod SoCs) should
> match to speedbin 1 */
> =================><==================
> 
> SM8550-QRD:
> [    7.681816] msm_dpu ae01000.display-controller: bound ae94000.dsi
> (ops dsi_ops [msm])
> [    7.694479] msm_dpu ae01000.display-controller: bound
> ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
> [    7.705784] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [    7.714322] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [    7.722851] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [    7.722853] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [    7.722855] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [    7.722856] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [    7.722858] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [    7.722860] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [    7.722861] adreno 3d00000.gpu: _of_add_opp_table_v2: no supported OPPs
> [    7.722863] adreno 3d00000.gpu: [drm:adreno_gpu_init [msm]] *ERROR*
> Unable to set the OPP table
> 
> SM8550-HDK:
> [   10.119986] msm_dpu ae01000.display-controller: bound ae94000.dsi
> (ops dsi_ops [msm])
> [   10.133872] msm_dpu ae01000.display-controller: bound
> ae90000.displayport-controller (ops msm_dp_display_comp_ops [msm])
> [   10.147377] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [   10.161640] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [   10.171198] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [   10.179756] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [   10.188313] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [   10.196868] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [   10.205424] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [   10.226025] adreno 3d00000.gpu: _opp_is_supported: Invalid opp-
> supported-hw property (-22)
> [   10.234589] adreno 3d00000.gpu: _of_add_opp_table_v2: no supported OPPs
> [   10.247165] adreno 3d00000.gpu: [drm:adreno_gpu_init [msm]] *ERROR*
> Unable to set the OPP table
> 
> This behaves exactly as I said, so please fix it.

Konrad,

iirc, we discussed this in one of the earlier revision. There is a
circular dependency between the driver change for SKU support and the dt
change that adds supported_hw bitmask in opp-table. Only scenario it
works is when you add these to the initial patches series of a new GPU.

It will be very useful if we can break this circular dependency.

-Akhil.

> 
> Neil
> 
>>
>> Konrad
> 
> 

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Konrad Dybcio]

On 5/1/25 11:29 AM, Akhil P Oommen wrote:
> On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote:
>> On 30/04/2025 18:39, Konrad Dybcio wrote:
>>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
>>>> On 30/04/2025 17:36, Konrad Dybcio wrote:
>>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:

[...]

>> This behaves exactly as I said, so please fix it.

Eh, I was so sure I tested things correctly..

> 
> Konrad,
> 
> iirc, we discussed this in one of the earlier revision. There is a
> circular dependency between the driver change for SKU support and the dt
> change that adds supported_hw bitmask in opp-table. Only scenario it
> works is when you add these to the initial patches series of a new GPU.
> 
> It will be very useful if we can break this circular dependency.

Right. Let's start with getting that in order

Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Akhil P Oommen]

On 5/1/2025 9:23 PM, Konrad Dybcio wrote:
> On 5/1/25 11:29 AM, Akhil P Oommen wrote:
>> On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote:
>>> On 30/04/2025 18:39, Konrad Dybcio wrote:
>>>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
>>>>> On 30/04/2025 17:36, Konrad Dybcio wrote:
>>>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
> 
> [...]
> 
>>> This behaves exactly as I said, so please fix it.
> 
> Eh, I was so sure I tested things correctly..
> 
>>
>> Konrad,
>>
>> iirc, we discussed this in one of the earlier revision. There is a
>> circular dependency between the driver change for SKU support and the dt
>> change that adds supported_hw bitmask in opp-table. Only scenario it
>> works is when you add these to the initial patches series of a new GPU.
>>
>> It will be very useful if we can break this circular dependency.
> 
> Right. Let's start with getting that in order

Another complication with the socinfo is that the value is unique for a
chipset, not for a GPU. So, it won't work if we keep this data in GPU
list in the driver.

Downstream solved this problem by keeping the PCODE/FCODE mappings in
the devicetree.

-Akhil.

> 
> Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Konrad Dybcio]

On 5/11/25 11:51 AM, Akhil P Oommen wrote:
> On 5/1/2025 9:23 PM, Konrad Dybcio wrote:
>> On 5/1/25 11:29 AM, Akhil P Oommen wrote:
>>> On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote:
>>>> On 30/04/2025 18:39, Konrad Dybcio wrote:
>>>>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
>>>>>> On 30/04/2025 17:36, Konrad Dybcio wrote:
>>>>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
>>>>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
>>>>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
>>>>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
>>>>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
>>
>> [...]
>>
>>>> This behaves exactly as I said, so please fix it.
>>
>> Eh, I was so sure I tested things correctly..
>>
>>>
>>> Konrad,
>>>
>>> iirc, we discussed this in one of the earlier revision. There is a
>>> circular dependency between the driver change for SKU support and the dt
>>> change that adds supported_hw bitmask in opp-table. Only scenario it
>>> works is when you add these to the initial patches series of a new GPU.
>>>
>>> It will be very useful if we can break this circular dependency.
>>
>> Right. Let's start with getting that in order
> 
> Another complication with the socinfo is that the value is unique for a
> chipset, not for a GPU. So, it won't work if we keep this data in GPU
> list in the driver.
> 
> Downstream solved this problem by keeping the PCODE/FCODE mappings in
> the devicetree.

Hmm.. that actually does not sound very bad.. it would allow for e.g.
new bins to appear without having to replace the kernel.. great for
backwards/forwards compat

Rob, WDYT?

Konrad

Re: [PATCH RFT v6 2/5] drm/msm/adreno: Add speedbin data for SM8550 / A740

[Rob Clark]

On Thu, May 22, 2025 at 9:03 AM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 5/11/25 11:51 AM, Akhil P Oommen wrote:
> > On 5/1/2025 9:23 PM, Konrad Dybcio wrote:
> >> On 5/1/25 11:29 AM, Akhil P Oommen wrote:
> >>> On 4/30/2025 10:26 PM, neil.armstrong@linaro.org wrote:
> >>>> On 30/04/2025 18:39, Konrad Dybcio wrote:
> >>>>> On 4/30/25 6:19 PM, neil.armstrong@linaro.org wrote:
> >>>>>> On 30/04/2025 17:36, Konrad Dybcio wrote:
> >>>>>>> On 4/30/25 4:49 PM, neil.armstrong@linaro.org wrote:
> >>>>>>>> On 30/04/2025 15:09, Konrad Dybcio wrote:
> >>>>>>>>> On 4/30/25 2:49 PM, neil.armstrong@linaro.org wrote:
> >>>>>>>>>> On 30/04/2025 14:35, Konrad Dybcio wrote:
> >>>>>>>>>>> On 4/30/25 2:26 PM, neil.armstrong@linaro.org wrote:
> >>
> >> [...]
> >>
> >>>> This behaves exactly as I said, so please fix it.
> >>
> >> Eh, I was so sure I tested things correctly..
> >>
> >>>
> >>> Konrad,
> >>>
> >>> iirc, we discussed this in one of the earlier revision. There is a
> >>> circular dependency between the driver change for SKU support and the dt
> >>> change that adds supported_hw bitmask in opp-table. Only scenario it
> >>> works is when you add these to the initial patches series of a new GPU.
> >>>
> >>> It will be very useful if we can break this circular dependency.
> >>
> >> Right. Let's start with getting that in order
> >
> > Another complication with the socinfo is that the value is unique for a
> > chipset, not for a GPU. So, it won't work if we keep this data in GPU
> > list in the driver.
> >
> > Downstream solved this problem by keeping the PCODE/FCODE mappings in
> > the devicetree.
>
> Hmm.. that actually does not sound very bad.. it would allow for e.g.
> new bins to appear without having to replace the kernel.. great for
> backwards/forwards compat
>
> Rob, WDYT?

Not against having it in dt if the dt maintainers can be convinced..

Alternatively, there is the optional machine string in adreno_info.
We've used that in a few other places where speedbin mappings are
different for multiple SoCs using the same gpu.

BR,
-R