Message-ID: <20201202163032.GG16856@jcrouse1-lnx.qualcomm.com>
Date:   Wed, 2 Dec 2020 09:30:32 -0700
From:   Jordan Crouse <jcrouse@...eaurora.org>
To:     Akhil P Oommen <akhilpo@...eaurora.org>
Cc:     freedreno@...ts.freedesktop.org, devicetree@...r.kernel.org,
        robh@...nel.org, dri-devel@...edesktop.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        mka@...omium.org, robdclark@...il.com, dianders@...omium.org
Subject: Re: [PATCH v2 1/3] drm/msm: adreno: Make speed-bin support generic

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@...eaurora.org>
> >>---
> >>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
> 
> -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@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists