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..
Patches 5 and 6 coooould be omitted, but I'd reaaally like them to land
and soon at that. This would enable even more overdue and necessary
cleanups/feature prepwork sooner than later.
The dt patch can only be picked if the drm patches are there.
Depends on:
https://lore.kernel.org/linux-arm-msm/20240412-topic-adreno_nullptr_supphw-v1-1-eb30a1c1292f@linaro.org/
Signed-off-by: Konrad Dybcio <[email protected]>
---
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, might matter in the future or w/ other peripherals)
- Rework the speedbin logic to be hopefully saner (accidental support
for A2xx-A4xx, I guess!)
- Reorder some existing function calls to avoid crazy nullptrs
- ""fix"" the smem dependency inconvenience
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Konrad Dybcio (7):
soc: qcom: Move some socinfo defines to the header
soc: qcom: smem: Add a feature code getter
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_gpu.c | 34 ----------
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 54 ---------------
drivers/gpu/drm/msm/adreno/adreno_device.c | 13 ++++
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 103 +++++++++++++++++++++++++----
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 11 +--
drivers/gpu/drm/msm/msm_gpu.c | 3 -
drivers/soc/qcom/smem.c | 33 +++++++++
drivers/soc/qcom/socinfo.c | 8 ---
include/linux/soc/qcom/smem.h | 1 +
include/linux/soc/qcom/socinfo.h | 34 ++++++++++
11 files changed, 198 insertions(+), 117 deletions(-)
---
base-commit: b13768266bf3a129adf5bbd0bad28e23a74329a2
change-id: 20240404-topic-smem_speedbin-8deecd0bef0e
Best regards,
--
Konrad Dybcio <[email protected]>
Recent (SM8550+ ish) Qualcomm SoCs have a new mechanism for precisely
identifying the specific SKU and the precise speed bin (in the general
meaning of this word, anyway): a pair of values called Product Code
and Feature Code.
Based on this information, we can deduce the available frequencies for
things such as Adreno. In the case of Adreno specifically, Pcode is
useless for non-prototype SoCs.
Introduce a getter for the feature code and export it.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/soc/qcom/smem.c | 33 +++++++++++++++++++++++++++++++++
include/linux/soc/qcom/smem.h | 1 +
include/linux/soc/qcom/socinfo.h | 26 ++++++++++++++++++++++++++
3 files changed, 60 insertions(+)
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 7191fa0c087f..29e708789eec 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -795,6 +795,39 @@ int qcom_smem_get_soc_id(u32 *id)
}
EXPORT_SYMBOL_GPL(qcom_smem_get_soc_id);
+/**
+ * qcom_smem_get_feature_code() - return the feature code
+ * @code: On success, return the feature code here.
+ *
+ * Look up the feature code identifier from SMEM and return it.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int qcom_smem_get_feature_code(u32 *code)
+{
+ struct socinfo *info;
+ u32 raw_code;
+
+ info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
+ if (IS_ERR(info))
+ return PTR_ERR(info);
+
+ /* This only makes sense for socinfo >= 16 */
+ if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
+ return -EOPNOTSUPP;
+
+ raw_code = __le32_to_cpu(info->feature_code);
+
+ /* Ensure the value makes sense */
+ if (raw_code >= SOCINFO_FC_INT_MAX)
+ raw_code = SOCINFO_FC_UNKNOWN;
+
+ *code = raw_code;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(qcom_smem_get_feature_code);
+
static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
{
struct smem_header *header;
diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
index a36a3b9d4929..0943bf419e11 100644
--- a/include/linux/soc/qcom/smem.h
+++ b/include/linux/soc/qcom/smem.h
@@ -13,5 +13,6 @@ int qcom_smem_get_free_space(unsigned host);
phys_addr_t qcom_smem_virt_to_phys(void *p);
int qcom_smem_get_soc_id(u32 *id);
+int qcom_smem_get_feature_code(u32 *code);
#endif
diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
index 10e0a4c287f4..52439f48428f 100644
--- a/include/linux/soc/qcom/socinfo.h
+++ b/include/linux/soc/qcom/socinfo.h
@@ -3,6 +3,8 @@
#ifndef __QCOM_SOCINFO_H__
#define __QCOM_SOCINFO_H__
+#include <linux/types.h>
+
/*
* SMEM item id, used to acquire handles to respective
* SMEM region.
@@ -82,4 +84,28 @@ struct socinfo {
__le32 boot_core;
};
+/* Internal feature codes */
+enum qcom_socinfo_feature_code {
+ /* External feature codes */
+ SOCINFO_FC_UNKNOWN = 0x0,
+ SOCINFO_FC_AA,
+ SOCINFO_FC_AB,
+ SOCINFO_FC_AC,
+ SOCINFO_FC_AD,
+ SOCINFO_FC_AE,
+ SOCINFO_FC_AF,
+ SOCINFO_FC_AG,
+ SOCINFO_FC_AH,
+};
+
+/* Internal feature codes */
+/* Valid values: 0 <= n <= 0xf */
+#define SOCINFO_FC_Yn(n) (0xf1 + n)
+#define SOCINFO_FC_INT_MAX SOCINFO_FC_Yn(0x10)
+
+/* Product codes */
+#define SOCINFO_PC_UNKNOWN 0
+#define SOCINFO_PCn(n) (n + 1)
+#define SOCINFO_PC_RESERVE (BIT(31) - 1)
+
#endif
--
2.44.0
Add speebin data for A740, as found on SM8550 and derivative SoCs.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 901ef767e491..3b631445c541 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -570,6 +570,11 @@ static const struct adreno_info gpulist[] = {
.zapfw = "a740_zap.mdt",
.hwcg = a740_hwcg,
.address_space_size = SZ_16G,
+ .speedbins = ADRENO_SPEEDBINS(
+ { ADRENO_SKU_ID(SOCINFO_FC_AB), 1 },
+ { ADRENO_SKU_ID(SOCINFO_FC_AC), 0 },
+ { ADRENO_SKU_ID(SOCINFO_FC_AF), 0 },
+ ),
}, {
.chip_ids = ADRENO_CHIP_IDS(0x43051401), /* "C520v2" */
.family = ADRENO_7XX_GEN3,
--
2.44.0
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 <[email protected]>
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 +++---
drivers/gpu/drm/msm/adreno/adreno_device.c | 2 ++
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++++---
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 12 ++++++---
4 files changed, 53 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index d10323f15d40..60708c23ae4c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2890,13 +2890,15 @@ 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
@@ -3056,7 +3058,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 c3703a51287b..901ef767e491 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 074fb498706f..58fd70140685 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);
@@ -1057,9 +1060,39 @@ 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);
+ u32 fcode;
+ 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
+ /*
+ * 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 0;
}
int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
@@ -1098,9 +1131,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 77526892eb8c..8f2b70eaf6ad 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -81,7 +81,12 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
extern const struct adreno_reglist a660_hwcg[], a690_hwcg[], a702_hwcg[], a730_hwcg[], a740_hwcg[];
struct adreno_speedbin {
- uint16_t fuse;
+ /* <= 16-bit for NVMEM fuses, 32b for SOCID values */
+ uint32_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) (SOCINFO_PC_UNKNOWN << 16 | fcode)
+
uint16_t speedbin;
};
@@ -136,7 +141,7 @@ struct adreno_gpu {
struct msm_gpu base;
const struct adreno_info *info;
uint32_t chip_id;
- uint16_t speedbin;
+ uint32_t speedbin;
const struct adreno_gpu_funcs *funcs;
/* interesting register offsets to dump: */
@@ -519,7 +524,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.44.0
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 <[email protected]>
---
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 | 70 +++++++++++++++++++++++++++------
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 3 --
drivers/gpu/drm/msm/msm_gpu.c | 3 --
5 files changed, 57 insertions(+), 109 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index c003f970189b..eed6a2eb1731 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1704,38 +1704,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;
@@ -1763,8 +1731,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 60708c23ae4c..1242697d64a7 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2878,55 +2878,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 means that the platform doesn't support speedbin which is
- * fine
- */
- if (ret == -ENOENT) {
- 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,
@@ -3058,13 +3009,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 (is_a7xx)
ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs_a7xx, 1);
else if (adreno_has_gmu_wrapper(adreno_gpu))
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 58fd70140685..08b2f08e2a14 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -1060,8 +1060,8 @@ 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)
{
u32 fcode;
int ret;
@@ -1095,6 +1095,46 @@ int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
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) {
+ adreno_gpu->speedbin = ADRENO_SPEEDBIN_FUSE_NODATA;
+ return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
+ }
+
+ /*
+ * 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 is too old => allow all OPPs */
+ DRM_DEV_INFO(dev, "No GPU speed bin fuse, please update your device tree\n");
+ return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
+ }
+
+ 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,
struct adreno_gpu *adreno_gpu,
const struct adreno_gpu_funcs *funcs, int nr_rings)
@@ -1104,7 +1144,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;
@@ -1131,29 +1170,34 @@ 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;
-
gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
ADRENO_CHIPID_ARGS(config->chip_id));
if (!gpu_name)
return -ENOMEM;
adreno_gpu_config.ioname = "kgsl_3d0_reg_memory";
-
adreno_gpu_config.nr_rings = nr_rings;
+ pm_runtime_set_autosuspend_delay(dev, adreno_gpu->info->inactive_period);
+ pm_runtime_use_autosuspend(dev);
+
+ ret = msm_gpu_init(drm, pdev, &adreno_gpu->base, &funcs->base,
+ gpu_name, &adreno_gpu_config);
+ if (ret)
+ return ret;
+
+ ret = adreno_set_speedbin(adreno_gpu, dev);
+ if (ret)
+ return ret;
+
ret = adreno_get_pwrlevels(dev, gpu);
if (ret)
return ret;
- pm_runtime_set_autosuspend_delay(dev,
- adreno_gpu->info->inactive_period);
- pm_runtime_use_autosuspend(dev);
+ /* Devfreq can only be initialized after OPP */
+ msm_devfreq_init(gpu);
- return msm_gpu_init(drm, pdev, &adreno_gpu->base, &funcs->base,
- gpu_name, &adreno_gpu_config);
+ return 0;
}
void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index 8f2b70eaf6ad..30e8b9919adb 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -524,9 +524,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
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 655002b21b0d..8504eaab8ef6 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -932,9 +932,6 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
gpu->pdev = pdev;
platform_set_drvdata(pdev, &gpu->adreno_smmu);
- msm_devfreq_init(gpu);
-
-
gpu->aspace = gpu->funcs->create_address_space(gpu, pdev);
if (gpu->aspace == NULL)
--
2.44.0
In preparation for commonizing the speedbin handling code.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 3b631445c541..53302995fc81 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -258,6 +258,12 @@ static const struct adreno_info gpulist[] = {
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.44.0
In preparation for parsing the chip "feature code" (FC) and "product
code" (PC) (essentially the parameters that let us conclusively
characterize the sillicon we're running on, including various speed
bins), move the socinfo version defines to the public header.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/soc/qcom/socinfo.c | 8 --------
include/linux/soc/qcom/socinfo.h | 8 ++++++++
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index 277c07a6603d..cf4616a468f2 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -21,14 +21,6 @@
#include <dt-bindings/arm/qcom,ids.h>
-/*
- * SoC version type with major number in the upper 16 bits and minor
- * number in the lower 16 bits.
- */
-#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff)
-#define SOCINFO_MINOR(ver) ((ver) & 0xffff)
-#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))
-
/* Helper macros to create soc_id table */
#define qcom_board_id(id) QCOM_ID_ ## id, __stringify(id)
#define qcom_board_id_named(id, name) QCOM_ID_ ## id, (name)
diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
index e78777bb0f4a..10e0a4c287f4 100644
--- a/include/linux/soc/qcom/socinfo.h
+++ b/include/linux/soc/qcom/socinfo.h
@@ -12,6 +12,14 @@
#define SMEM_SOCINFO_BUILD_ID_LENGTH 32
#define SMEM_SOCINFO_CHIP_ID_LENGTH 32
+/*
+ * SoC version type with major number in the upper 16 bits and minor
+ * number in the lower 16 bits.
+ */
+#define SOCINFO_MAJOR(ver) (((ver) >> 16) & 0xffff)
+#define SOCINFO_MINOR(ver) ((ver) & 0xffff)
+#define SOCINFO_VERSION(maj, min) ((((maj) & 0xffff) << 16)|((min) & 0xffff))
+
/* Socinfo SMEM item structure */
struct socinfo {
__le32 fmt;
--
2.44.0
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.
Signed-off-by: Konrad Dybcio <[email protected]>
---
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 5cae8d773cec..2f6842f6a5b7 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -2087,48 +2087,67 @@ 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-supported-hw = <0x3>;
};
opp-615000000 {
opp-hz = /bits/ 64 <615000000>;
opp-level = <RPMH_REGULATOR_LEVEL_SVS_L0>;
+ opp-supported-hw = <0x3>;
};
opp-550000000 {
opp-hz = /bits/ 64 <550000000>;
opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
+ opp-supported-hw = <0x3>;
};
opp-475000000 {
opp-hz = /bits/ 64 <475000000>;
opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_L1>;
+ opp-supported-hw = <0x3>;
};
opp-401000000 {
opp-hz = /bits/ 64 <401000000>;
opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+ opp-supported-hw = <0x3>;
};
opp-348000000 {
opp-hz = /bits/ 64 <348000000>;
opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D0>;
+ opp-supported-hw = <0x3>;
};
opp-295000000 {
opp-hz = /bits/ 64 <295000000>;
opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>;
+ opp-supported-hw = <0x3>;
};
opp-220000000 {
opp-hz = /bits/ 64 <220000000>;
opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D2>;
+ opp-supported-hw = <0x3>;
+ };
+
+ opp-124800000 {
+ opp-hz = /bits/ 64 <124800000>;
+ opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D2>;
+ opp-supported-hw = <0x3>;
};
};
};
--
2.44.0
On Wed, Apr 17, 2024 at 10:02:54PM +0200, Konrad Dybcio wrote:
> Recent (SM8550+ ish) Qualcomm SoCs have a new mechanism for precisely
> identifying the specific SKU and the precise speed bin (in the general
> meaning of this word, anyway): a pair of values called Product Code
> and Feature Code.
>
> Based on this information, we can deduce the available frequencies for
> things such as Adreno. In the case of Adreno specifically, Pcode is
> useless for non-prototype SoCs.
>
> Introduce a getter for the feature code and export it.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/soc/qcom/smem.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/soc/qcom/smem.h | 1 +
> include/linux/soc/qcom/socinfo.h | 26 ++++++++++++++++++++++++++
> 3 files changed, 60 insertions(+)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 7191fa0c087f..29e708789eec 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -795,6 +795,39 @@ int qcom_smem_get_soc_id(u32 *id)
> }
> EXPORT_SYMBOL_GPL(qcom_smem_get_soc_id);
>
> +/**
> + * qcom_smem_get_feature_code() - return the feature code
> + * @code: On success, return the feature code here.
> + *
> + * Look up the feature code identifier from SMEM and return it.
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +int qcom_smem_get_feature_code(u32 *code)
> +{
> + struct socinfo *info;
> + u32 raw_code;
> +
> + info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID, NULL);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + /* This only makes sense for socinfo >= 16 */
> + if (__le32_to_cpu(info->fmt) < SOCINFO_VERSION(0, 16))
> + return -EOPNOTSUPP;
> +
> + raw_code = __le32_to_cpu(info->feature_code);
> +
> + /* Ensure the value makes sense */
> + if (raw_code >= SOCINFO_FC_INT_MAX)
> + raw_code = SOCINFO_FC_UNKNOWN;
> +
> + *code = raw_code;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_get_feature_code);
> +
> static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
> {
> struct smem_header *header;
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index a36a3b9d4929..0943bf419e11 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -13,5 +13,6 @@ int qcom_smem_get_free_space(unsigned host);
> phys_addr_t qcom_smem_virt_to_phys(void *p);
>
> int qcom_smem_get_soc_id(u32 *id);
> +int qcom_smem_get_feature_code(u32 *code);
>
> #endif
> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
> index 10e0a4c287f4..52439f48428f 100644
> --- a/include/linux/soc/qcom/socinfo.h
> +++ b/include/linux/soc/qcom/socinfo.h
> @@ -3,6 +3,8 @@
> #ifndef __QCOM_SOCINFO_H__
> #define __QCOM_SOCINFO_H__
>
> +#include <linux/types.h>
> +
> /*
> * SMEM item id, used to acquire handles to respective
> * SMEM region.
> @@ -82,4 +84,28 @@ struct socinfo {
> __le32 boot_core;
> };
>
> +/* Internal feature codes */
> +enum qcom_socinfo_feature_code {
> + /* External feature codes */
> + SOCINFO_FC_UNKNOWN = 0x0,
> + SOCINFO_FC_AA,
> + SOCINFO_FC_AB,
> + SOCINFO_FC_AC,
> + SOCINFO_FC_AD,
> + SOCINFO_FC_AE,
> + SOCINFO_FC_AF,
> + SOCINFO_FC_AG,
> + SOCINFO_FC_AH,
> +};
> +
> +/* Internal feature codes */
> +/* Valid values: 0 <= n <= 0xf */
> +#define SOCINFO_FC_Yn(n) (0xf1 + n)
> +#define SOCINFO_FC_INT_MAX SOCINFO_FC_Yn(0x10)
This is 0x101 rather than 0x100 or 0xff. Is that expected?
> +
> +/* Product codes */
> +#define SOCINFO_PC_UNKNOWN 0
> +#define SOCINFO_PCn(n) (n + 1)
> +#define SOCINFO_PC_RESERVE (BIT(31) - 1)
This patch works on fcodes, why do we have PCode defines here?
> +
> #endif
>
> --
> 2.44.0
>
--
With best wishes
Dmitry
On Wed, Apr 17, 2024 at 10:02:55PM +0200, Konrad Dybcio wrote:
> 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 <[email protected]>
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 +++---
> drivers/gpu/drm/msm/adreno/adreno_device.c | 2 ++
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 41 +++++++++++++++++++++++++++---
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 12 ++++++---
> 4 files changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index d10323f15d40..60708c23ae4c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2890,13 +2890,15 @@ 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
> @@ -3056,7 +3058,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 c3703a51287b..901ef767e491 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>
> +
Stray leftover?
> #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 074fb498706f..58fd70140685 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);
> @@ -1057,9 +1060,39 @@ 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);
> + u32 fcode;
> + 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
Please extract to a separate function and put the function under ifdef
(providing a stub otherwise). Having #ifndefs inside funciton body is
frowned upon.
> + /*
> + * 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 0;
> }
>
> int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> @@ -1098,9 +1131,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 77526892eb8c..8f2b70eaf6ad 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -81,7 +81,12 @@ extern const struct adreno_reglist a612_hwcg[], a615_hwcg[], a630_hwcg[], a640_h
> extern const struct adreno_reglist a660_hwcg[], a690_hwcg[], a702_hwcg[], a730_hwcg[], a740_hwcg[];
>
> struct adreno_speedbin {
> - uint16_t fuse;
> + /* <= 16-bit for NVMEM fuses, 32b for SOCID values */
> + uint32_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) (SOCINFO_PC_UNKNOWN << 16 | fcode)
If we got rid of PCode matching, is there a need to actually use
SOCINFO_PC_UNKNOWN here? Or just 0 would be fine?
> +
> uint16_t speedbin;
> };
>
> @@ -136,7 +141,7 @@ struct adreno_gpu {
> struct msm_gpu base;
> const struct adreno_info *info;
> uint32_t chip_id;
> - uint16_t speedbin;
> + uint32_t speedbin;
> const struct adreno_gpu_funcs *funcs;
>
> /* interesting register offsets to dump: */
> @@ -519,7 +524,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.44.0
>
--
With best wishes
Dmitry
On Wed, Apr 17, 2024 at 10:02:56PM +0200, Konrad Dybcio wrote:
> Add speebin data for A740, as found on SM8550 and derivative SoCs.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/gpu/drm/msm/adreno/adreno_device.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Wed, Apr 17, 2024 at 10:02:57PM +0200, Konrad Dybcio wrote:
> In preparation for commonizing the speedbin handling code.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/gpu/drm/msm/adreno/adreno_device.c | 6 ++++++
> 1 file changed, 6 insertions(+)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Wed, Apr 17, 2024 at 10:02:58PM +0200, Konrad Dybcio wrote:
> 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 <[email protected]>
> ---
> 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 | 70 +++++++++++++++++++++++++++------
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 3 --
> drivers/gpu/drm/msm/msm_gpu.c | 3 --
> 5 files changed, 57 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index c003f970189b..eed6a2eb1731 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1704,38 +1704,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;
> @@ -1763,8 +1731,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 60708c23ae4c..1242697d64a7 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2878,55 +2878,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 means that the platform doesn't support speedbin which is
> - * fine
> - */
> - if (ret == -ENOENT) {
> - 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,
> @@ -3058,13 +3009,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 (is_a7xx)
> ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs_a7xx, 1);
> else if (adreno_has_gmu_wrapper(adreno_gpu))
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 58fd70140685..08b2f08e2a14 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -1060,8 +1060,8 @@ 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)
> {
> u32 fcode;
> int ret;
> @@ -1095,6 +1095,46 @@ int adreno_read_speedbin(struct adreno_gpu *adreno_gpu,
> 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) {
> + adreno_gpu->speedbin = ADRENO_SPEEDBIN_FUSE_NODATA;
> + return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
BIT(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 is too old => allow all OPPs */
Missing assignment to adeno_gpu->speedbin ? Or is it fine?
> + DRM_DEV_INFO(dev, "No GPU speed bin fuse, please update your device tree\n");
> + return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
BIT(0)
maybe #define it?
> + }
> +
> + 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,
> struct adreno_gpu *adreno_gpu,
> const struct adreno_gpu_funcs *funcs, int nr_rings)
> @@ -1104,7 +1144,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;
> @@ -1131,29 +1170,34 @@ 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;
> -
> gpu_name = devm_kasprintf(dev, GFP_KERNEL, "%"ADRENO_CHIPID_FMT,
> ADRENO_CHIPID_ARGS(config->chip_id));
> if (!gpu_name)
> return -ENOMEM;
>
> adreno_gpu_config.ioname = "kgsl_3d0_reg_memory";
> -
> adreno_gpu_config.nr_rings = nr_rings;
>
> + pm_runtime_set_autosuspend_delay(dev, adreno_gpu->info->inactive_period);
> + pm_runtime_use_autosuspend(dev);
> +
> + ret = msm_gpu_init(drm, pdev, &adreno_gpu->base, &funcs->base,
> + gpu_name, &adreno_gpu_config);
> + if (ret)
> + return ret;
> +
> + ret = adreno_set_speedbin(adreno_gpu, dev);
> + if (ret)
> + return ret;
> +
> ret = adreno_get_pwrlevels(dev, gpu);
> if (ret)
> return ret;
>
> - pm_runtime_set_autosuspend_delay(dev,
> - adreno_gpu->info->inactive_period);
> - pm_runtime_use_autosuspend(dev);
> + /* Devfreq can only be initialized after OPP */
> + msm_devfreq_init(gpu);
>
> - return msm_gpu_init(drm, pdev, &adreno_gpu->base, &funcs->base,
> - gpu_name, &adreno_gpu_config);
> + return 0;
> }
>
> void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index 8f2b70eaf6ad..30e8b9919adb 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -524,9 +524,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
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 655002b21b0d..8504eaab8ef6 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -932,9 +932,6 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> gpu->pdev = pdev;
> platform_set_drvdata(pdev, &gpu->adreno_smmu);
>
> - msm_devfreq_init(gpu);
> -
> -
> gpu->aspace = gpu->funcs->create_address_space(gpu, pdev);
>
> if (gpu->aspace == NULL)
>
> --
> 2.44.0
>
--
With best wishes
Dmitry
On Wed, Apr 17, 2024 at 10:02:59PM +0200, Konrad Dybcio wrote:
> 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.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sm8550.dtsi | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 18.04.2024 1:43 AM, Dmitry Baryshkov wrote:
> On Wed, Apr 17, 2024 at 10:02:55PM +0200, Konrad Dybcio wrote:
>> 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 <[email protected]>
>> ---
[...]
>> --- 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>
>> +
>
> Stray leftover?
Looks like
[...]
>> +
>> +#ifdef CONFIG_QCOM_SMEM
>
> Please extract to a separate function and put the function under ifdef
> (providing a stub otherwise). Having #ifndefs inside funciton body is
> frowned upon.
Hm, this looked quite sparse and straightforward, but I can do that.
[...]
>> +/* 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) (SOCINFO_PC_UNKNOWN << 16 | fcode)
>
> If we got rid of PCode matching, is there a need to actually use
> SOCINFO_PC_UNKNOWN here? Or just 0 would be fine?
The IDs need to stay constant for mesa
I used the define here to:
a) define the SKU_ID structure so that it's clear what it's comprised of
b) make it easy to add back Pcode in case it becomes useful with future SoCs
c) avoid mistakes - PC_UNKNOWN happens to be zero, but that's a lucky
coincidence
We don't *match* based on PCODE, but still need to construct the ID properly
Another option would be to pass the real pcode and add some sort of
"pcode_invalid" property that if found would ignore this part of the
SKU_ID in mesa, but that sounds overly and unnecessarily complex.
Konrad
Konrad
On 18.04.2024 1:39 AM, Dmitry Baryshkov wrote:
> On Wed, Apr 17, 2024 at 10:02:54PM +0200, Konrad Dybcio wrote:
>> Recent (SM8550+ ish) Qualcomm SoCs have a new mechanism for precisely
>> identifying the specific SKU and the precise speed bin (in the general
>> meaning of this word, anyway): a pair of values called Product Code
>> and Feature Code.
>>
>> Based on this information, we can deduce the available frequencies for
>> things such as Adreno. In the case of Adreno specifically, Pcode is
>> useless for non-prototype SoCs.
>>
>> Introduce a getter for the feature code and export it.
>>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
[...]
>> +/* Internal feature codes */
>> +/* Valid values: 0 <= n <= 0xf */
>> +#define SOCINFO_FC_Yn(n) (0xf1 + n)
>> +#define SOCINFO_FC_INT_MAX SOCINFO_FC_Yn(0x10)
>
> This is 0x101 rather than 0x100 or 0xff. Is that expected?
Yes, this is "the first invalid one", similar to ENUMNAME_NUM
>
>> +
>> +/* Product codes */
>> +#define SOCINFO_PC_UNKNOWN 0
>> +#define SOCINFO_PCn(n) (n + 1)
>> +#define SOCINFO_PC_RESERVE (BIT(31) - 1)
>
> This patch works on fcodes, why do we have PCode defines here?
I decided they're useful to keep.. Didn't want to split them to a separate
patch for no reason.
Konrad
On 18.04.2024 1:49 AM, Dmitry Baryshkov wrote:
> On Wed, Apr 17, 2024 at 10:02:58PM +0200, Konrad Dybcio wrote:
>> 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 <[email protected]>
>> ---
[...]
>> + /* No speedbins defined for this GPU SKU => allow all defined OPPs */
>> + if (!info->speedbins) {
>> + adreno_gpu->speedbin = ADRENO_SPEEDBIN_FUSE_NODATA;
>> + return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>
> BIT(0)
You mean for &supp_hw, or "1"?
1 is the "count" parameter, supp_hw is a "u32 supported_hw[count]"
>
>> + }
>> +
>> + /*
>> + * 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 is too old => allow all OPPs */
>
> Missing assignment to adeno_gpu->speedbin ? Or is it fine?
Good catch. Only mesa (and I suppose you :D) read this value.
>
>> + DRM_DEV_INFO(dev, "No GPU speed bin fuse, please update your device tree\n");
>> + return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
>
> BIT(0)
> maybe #define it?
(ditto)
Konrad
On Thu, Apr 18, 2024 at 11:53:31AM +0200, Konrad Dybcio wrote:
> On 18.04.2024 1:39 AM, Dmitry Baryshkov wrote:
> > On Wed, Apr 17, 2024 at 10:02:54PM +0200, Konrad Dybcio wrote:
> >> Recent (SM8550+ ish) Qualcomm SoCs have a new mechanism for precisely
> >> identifying the specific SKU and the precise speed bin (in the general
> >> meaning of this word, anyway): a pair of values called Product Code
> >> and Feature Code.
> >>
> >> Based on this information, we can deduce the available frequencies for
> >> things such as Adreno. In the case of Adreno specifically, Pcode is
> >> useless for non-prototype SoCs.
> >>
> >> Introduce a getter for the feature code and export it.
> >>
> >> Signed-off-by: Konrad Dybcio <[email protected]>
> >> ---
>
> [...]
>
> >> +/* Internal feature codes */
> >> +/* Valid values: 0 <= n <= 0xf */
> >> +#define SOCINFO_FC_Yn(n) (0xf1 + n)
> >> +#define SOCINFO_FC_INT_MAX SOCINFO_FC_Yn(0x10)
> >
> > This is 0x101 rather than 0x100 or 0xff. Is that expected?
>
> Yes, this is "the first invalid one", similar to ENUMNAME_NUM
>
> >
> >> +
> >> +/* Product codes */
> >> +#define SOCINFO_PC_UNKNOWN 0
> >> +#define SOCINFO_PCn(n) (n + 1)
> >> +#define SOCINFO_PC_RESERVE (BIT(31) - 1)
> >
> > This patch works on fcodes, why do we have PCode defines here?
>
> I decided they're useful to keep.. Didn't want to split them to a separate
> patch for no reason.
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Thu, Apr 18, 2024 at 11:51:16AM +0200, Konrad Dybcio wrote:
> On 18.04.2024 1:43 AM, Dmitry Baryshkov wrote:
> > On Wed, Apr 17, 2024 at 10:02:55PM +0200, Konrad Dybcio wrote:
> >> 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 <[email protected]>
> >> ---
>
> [...]
>
> >> --- 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>
> >> +
> >
> > Stray leftover?
>
> Looks like
>
> [...]
>
> >> +
> >> +#ifdef CONFIG_QCOM_SMEM
> >
> > Please extract to a separate function and put the function under ifdef
> > (providing a stub otherwise). Having #ifndefs inside funciton body is
> > frowned upon.
>
> Hm, this looked quite sparse and straightforward, but I can do that.
>
> [...]
>
> >> +/* 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) (SOCINFO_PC_UNKNOWN << 16 | fcode)
> >
> > If we got rid of PCode matching, is there a need to actually use
> > SOCINFO_PC_UNKNOWN here? Or just 0 would be fine?
>
> The IDs need to stay constant for mesa
>
> I used the define here to:
>
> a) define the SKU_ID structure so that it's clear what it's comprised of
> b) make it easy to add back Pcode in case it becomes useful with future SoCs
> c) avoid mistakes - PC_UNKNOWN happens to be zero, but that's a lucky
> coincidence
>
> We don't *match* based on PCODE, but still need to construct the ID properly
>
> Another option would be to pass the real pcode and add some sort of
> "pcode_invalid" property that if found would ignore this part of the
> SKU_ID in mesa, but that sounds overly and unnecessarily complex.
It's fine, just add a comment please. Maybe we can rename PC_UNKNOWN to
PC_PRODUCTION?
>
> Konrad
>
> Konrad
--
With best wishes
Dmitry
On Thu, Apr 18, 2024 at 11:57:35AM +0200, Konrad Dybcio wrote:
> On 18.04.2024 1:49 AM, Dmitry Baryshkov wrote:
> > On Wed, Apr 17, 2024 at 10:02:58PM +0200, Konrad Dybcio wrote:
> >> 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 <[email protected]>
> >> ---
>
> [...]
>
> >> + /* No speedbins defined for this GPU SKU => allow all defined OPPs */
> >> + if (!info->speedbins) {
> >> + adreno_gpu->speedbin = ADRENO_SPEEDBIN_FUSE_NODATA;
> >> + return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
> >
> > BIT(0)
>
> You mean for &supp_hw, or "1"?
>
> 1 is the "count" parameter, supp_hw is a "u32 supported_hw[count]"
I see. It confused me. This way it's getting set to UINT_MAX, which
will match against any non-zero opp-supported-hw. Ack.
>
> >
> >> + }
> >> +
> >> + /*
> >> + * 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 is too old => allow all OPPs */
> >
> > Missing assignment to adeno_gpu->speedbin ? Or is it fine?
>
> Good catch. Only mesa (and I suppose you :D) read this value.
>
> >
> >> + DRM_DEV_INFO(dev, "No GPU speed bin fuse, please update your device tree\n");
> >> + return devm_pm_opp_set_supported_hw(dev, &supp_hw, 1);
> >
> > BIT(0)
> > maybe #define it?
>
> (ditto)
>
> Konrad
--
With best wishes
Dmitry
On 18.04.2024 1:07 PM, Dmitry Baryshkov wrote:
> On Thu, Apr 18, 2024 at 11:51:16AM +0200, Konrad Dybcio wrote:
>> On 18.04.2024 1:43 AM, Dmitry Baryshkov wrote:
>>> On Wed, Apr 17, 2024 at 10:02:55PM +0200, Konrad Dybcio wrote:
>>>> 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 <[email protected]>
>>>> ---
>>
>> [...]
>>
>>>> --- 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>
>>>> +
>>>
>>> Stray leftover?
>>
>> Looks like
>>
>> [...]
>>
>>>> +
>>>> +#ifdef CONFIG_QCOM_SMEM
>>>
>>> Please extract to a separate function and put the function under ifdef
>>> (providing a stub otherwise). Having #ifndefs inside funciton body is
>>> frowned upon.
>>
>> Hm, this looked quite sparse and straightforward, but I can do that.
>>
>> [...]
>>
>>>> +/* 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) (SOCINFO_PC_UNKNOWN << 16 | fcode)
>>>
>>> If we got rid of PCode matching, is there a need to actually use
>>> SOCINFO_PC_UNKNOWN here? Or just 0 would be fine?
>>
>> The IDs need to stay constant for mesa
>>
>> I used the define here to:
>>
>> a) define the SKU_ID structure so that it's clear what it's comprised of
>> b) make it easy to add back Pcode in case it becomes useful with future SoCs
>> c) avoid mistakes - PC_UNKNOWN happens to be zero, but that's a lucky
>> coincidence
>>
>> We don't *match* based on PCODE, but still need to construct the ID properly
>>
>> Another option would be to pass the real pcode and add some sort of
>> "pcode_invalid" property that if found would ignore this part of the
>> SKU_ID in mesa, but that sounds overly and unnecessarily complex.
>
> It's fine, just add a comment please. Maybe we can rename PC_UNKNOWN to
> PC_PRODUCTION?
I don't think that's right. The SoC "product code" may actually mean something
(again, not necessarily for Adreno specifically), and with Adreno in mind, it
being only meaningful for engineering samples may change in the future.
Konrad
On Thu, 18 Apr 2024 at 14:31, Konrad Dybcio <[email protected]> wrote:
>
> On 18.04.2024 1:07 PM, Dmitry Baryshkov wrote:
> > On Thu, Apr 18, 2024 at 11:51:16AM +0200, Konrad Dybcio wrote:
> >> On 18.04.2024 1:43 AM, Dmitry Baryshkov wrote:
> >>> On Wed, Apr 17, 2024 at 10:02:55PM +0200, Konrad Dybcio wrote:
> >>>> 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 <[email protected]>
> >>>> ---
> >>
> >> [...]
> >>
> >>>> --- 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>
> >>>> +
> >>>
> >>> Stray leftover?
> >>
> >> Looks like
> >>
> >> [...]
> >>
> >>>> +
> >>>> +#ifdef CONFIG_QCOM_SMEM
> >>>
> >>> Please extract to a separate function and put the function under ifdef
> >>> (providing a stub otherwise). Having #ifndefs inside funciton body is
> >>> frowned upon.
> >>
> >> Hm, this looked quite sparse and straightforward, but I can do that.
> >>
> >> [...]
> >>
> >>>> +/* 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) (SOCINFO_PC_UNKNOWN << 16 | fcode)
> >>>
> >>> If we got rid of PCode matching, is there a need to actually use
> >>> SOCINFO_PC_UNKNOWN here? Or just 0 would be fine?
> >>
> >> The IDs need to stay constant for mesa
> >>
> >> I used the define here to:
> >>
> >> a) define the SKU_ID structure so that it's clear what it's comprised of
> >> b) make it easy to add back Pcode in case it becomes useful with future SoCs
> >> c) avoid mistakes - PC_UNKNOWN happens to be zero, but that's a lucky
> >> coincidence
> >>
> >> We don't *match* based on PCODE, but still need to construct the ID properly
> >>
> >> Another option would be to pass the real pcode and add some sort of
> >> "pcode_invalid" property that if found would ignore this part of the
> >> SKU_ID in mesa, but that sounds overly and unnecessarily complex.
> >
> > It's fine, just add a comment please. Maybe we can rename PC_UNKNOWN to
> > PC_PRODUCTION?
>
> I don't think that's right. The SoC "product code" may actually mean something
> (again, not necessarily for Adreno specifically), and with Adreno in mind, it
> being only meaningful for engineering samples may change in the future.
Ack
--
With best wishes
Dmitry
On Wed, Apr 17, 2024 at 10:02:54PM GMT, Konrad Dybcio wrote:
[..]
> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
> index 10e0a4c287f4..52439f48428f 100644
> --- a/include/linux/soc/qcom/socinfo.h
> +++ b/include/linux/soc/qcom/socinfo.h
> @@ -3,6 +3,8 @@
> #ifndef __QCOM_SOCINFO_H__
> #define __QCOM_SOCINFO_H__
>
> +#include <linux/types.h>
> +
> /*
> * SMEM item id, used to acquire handles to respective
> * SMEM region.
> @@ -82,4 +84,28 @@ struct socinfo {
> __le32 boot_core;
> };
>
> +/* Internal feature codes */
> +enum qcom_socinfo_feature_code {
> + /* External feature codes */
> + SOCINFO_FC_UNKNOWN = 0x0,
> + SOCINFO_FC_AA,
> + SOCINFO_FC_AB,
> + SOCINFO_FC_AC,
> + SOCINFO_FC_AD,
> + SOCINFO_FC_AE,
> + SOCINFO_FC_AF,
> + SOCINFO_FC_AG,
> + SOCINFO_FC_AH,
> +};
> +
> +/* Internal feature codes */
> +/* Valid values: 0 <= n <= 0xf */
> +#define SOCINFO_FC_Yn(n) (0xf1 + n)
Please wrap that 'n' in some () here and below...
> +#define SOCINFO_FC_INT_MAX SOCINFO_FC_Yn(0x10)
"MAX" sounds inclusive, but the value is exclusive.
Regards,
Bjorn
> +
> +/* Product codes */
> +#define SOCINFO_PC_UNKNOWN 0
> +#define SOCINFO_PCn(n) (n + 1)
> +#define SOCINFO_PC_RESERVE (BIT(31) - 1)
> +
> #endif
>
> --
> 2.44.0
>