Re: [PATCH v2 1/3] drm/msm: adreno: Make speed-bin support generic

From: Akhil P Oommen
Date: Thu Dec 03 2020 - 11:03:03 EST

On 12/2/2020 10:00 PM, Jordan Crouse wrote:
On Wed, Dec 02, 2020 at 08:53:51PM +0530, Akhil P Oommen wrote:
On 11/30/2020 10:32 PM, Jordan Crouse wrote:
On Fri, Nov 27, 2020 at 06:19:44PM +0530, Akhil P Oommen wrote:
So far a530v2 gpu has support for detecting its supported opps
based on a fuse value called speed-bin. This patch makes this
support generic across gpu families. This is in preparation to
extend speed-bin support to a6x family.

Signed-off-by: Akhil P Oommen <akhilpo@xxxxxxxxxxxxxx>
---
Changes from v1:
1. Added the changes to support a618 sku to the series.
2. Avoid failing probe in case of an unsupported sku. (Rob)

drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34 --------------
drivers/gpu/drm/msm/adreno/adreno_device.c | 4 ++
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 71 ++++++++++++++++++++++++++++++
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 5 +++
4 files changed, 80 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 8fa5c91..7d42321 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1531,38 +1531,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);
- }
-
- dev_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;
@@ -1588,8 +1556,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
a5xx_gpu->lm_leakage = 0x4E001A;
- check_speed_bin(&pdev->dev);
-
ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 4);
if (ret) {
a5xx_destroy(&(a5xx_gpu->base.base));
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 87c8b03..e0ff16c 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -18,6 +18,8 @@ bool snapshot_debugbus = false;
MODULE_PARM_DESC(snapshot_debugbus, "Include debugbus sections in GPU devcoredump (if not fused off)");
module_param_named(snapshot_debugbus, snapshot_debugbus, bool, 0600);
+const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7};
+
static const struct adreno_info gpulist[] = {
{
.rev = ADRENO_REV(2, 0, 0, 0),
@@ -163,6 +165,8 @@ static const struct adreno_info gpulist[] = {
ADRENO_QUIRK_FAULT_DETECT_MASK,
.init = a5xx_gpu_init,
.zapfw = "a530_zap.mdt",
+ .speedbins = a530v2_speedbins,
+ .speedbins_count = ARRAY_SIZE(a530v2_speedbins),
}, {
.rev = ADRENO_REV(5, 4, 0, 2),
.revn = 540,
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index f21561d..b342fa4 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -14,6 +14,7 @@
#include <linux/pm_opp.h>
#include <linux/slab.h>
#include <linux/soc/qcom/mdt_loader.h>
+#include <linux/nvmem-consumer.h>
#include <soc/qcom/ocmem.h>
#include "adreno_gpu.h"
#include "msm_gem.h"
@@ -891,6 +892,69 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem *adreno_ocmem)
adreno_ocmem->hdl);
}
+static int adreno_set_supported_hw(struct device *dev,
+ struct adreno_gpu *adreno_gpu)
+{
+ u8 speedbins_count = adreno_gpu->info->speedbins_count;
+ const u32 *speedbins = adreno_gpu->info->speedbins;
+ struct nvmem_cell *cell;
+ u32 bin, i;
+ u32 val = 0;
+ void *buf, *opp_table;
+
+ cell = nvmem_cell_get(dev, "speed_bin");
+ /*
+ * -ENOENT means that the platform doesn't support speedbin which is
+ * fine
+ */
+ if (PTR_ERR(cell) == -ENOENT)
+ return 0;
+ else if (IS_ERR(cell))
+ return PTR_ERR(cell);
+
+ if (!speedbins)
+ goto done;
+
+ buf = nvmem_cell_read(cell, NULL);
+ if (IS_ERR(buf)) {
+ nvmem_cell_put(cell);
+ return PTR_ERR(buf);
+ }
+
+ bin = *((u32 *) buf);
+
+ for (i = 0; i < speedbins_count; i++) {
+ if (bin == speedbins[i]) {
+ val = (1 << i);
+ break;
+ }
+ }
+
+ kfree(buf);
+done:
+ nvmem_cell_put(cell);
+
+ if (!val) {
+ DRM_DEV_ERROR(dev,
+ "missing support for speed-bin: %u. Some OPPs may not be supported by hardware",
+ bin);
+ val = ~0U;
+ }
+
+ opp_table = dev_pm_opp_set_supported_hw(dev, &val, 1);
+ if (IS_ERR(opp_table))
+ return PTR_ERR(opp_table);
+
+ adreno_gpu->opp_table = opp_table;
+ return 0;
+}
+
+static void adreno_put_supported_hw(struct opp_table *opp_table)
+{
+ if (opp_table)
+ dev_pm_opp_put_supported_hw(opp_table);
+}
+
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)
@@ -899,6 +963,7 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
struct adreno_platform_config *config = dev->platform_data;
struct msm_gpu_config adreno_gpu_config = { 0 };
struct msm_gpu *gpu = &adreno_gpu->base;
+ int ret;
adreno_gpu->funcs = funcs;
adreno_gpu->info = adreno_info(config->rev);
@@ -910,6 +975,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
adreno_gpu_config.nr_rings = nr_rings;
+ ret = adreno_set_supported_hw(dev, adreno_gpu);
+ if (ret)
+ return ret;
+

I still don't understand why we are doing this here instead of a5xx_gpu.c and
a6xx_gpu.c.

Jordan

Could you please clarify why you prefer so?

Putting this support in the target specific code avoids declaring more global
variables and skips a bit of extra code for the vast majority of targets that do
not have speed bins. I don't mind sharing the common helper function but a5xx
has shown that this can be safely done in the target specific code and I don't
see any reason to deviate from that.

Jordan
Alright. Then it seems better to move everything to target specific code. Will post another patch shortly.

-Akhil.

-Akhil

adreno_get_pwrlevels(dev, gpu);
pm_runtime_set_autosuspend_delay(dev,
@@ -936,4 +1005,6 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
icc_put(gpu->icc_path);
icc_put(gpu->ocmem_icc_path);
+
+ adreno_put_supported_hw(adreno_gpu->opp_table);
}
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
index c3775f7..a756ad7 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
@@ -55,6 +55,7 @@ struct adreno_reglist {
};
extern const struct adreno_reglist a630_hwcg[], a640_hwcg[], a650_hwcg[];
+extern const u32 a618_speedbins[];
struct adreno_info {
struct adreno_rev rev;
@@ -67,6 +68,8 @@ struct adreno_info {
const char *zapfw;
u32 inactive_period;
const struct adreno_reglist *hwcg;
+ const u32 *speedbins;
+ const u8 speedbins_count;
};
const struct adreno_info *adreno_info(struct adreno_rev rev);
@@ -112,6 +115,8 @@ struct adreno_gpu {
* code (a3xx_gpu.c) and stored in this common location.
*/
const unsigned int *reg_offsets;
+
+ struct opp_table *opp_table;
};
#define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
--
2.7.4



_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel