2020-11-27 12:54:11

by Akhil P Oommen

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

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 <[email protected]>
---
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;
+
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

2020-11-27 12:56:02

by Akhil P Oommen

Subject: [PATCH v2 2/3] drm/msm: Add speed-bin support for a618 gpu

Extend speed-bin support to a618 gpu.

Signed-off-by: Akhil P Oommen <[email protected]>
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
index e0ff16c..21db7ae 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -18,6 +18,7 @@ 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 a618_speedbins[] = {0, 169, 174};
const u32 a530v2_speedbins[] = {0, 1, 2, 3, 4, 5, 6, 7};

static const struct adreno_info gpulist[] = {
@@ -196,6 +197,8 @@ static const struct adreno_info gpulist[] = {
.gmem = SZ_512K,
.inactive_period = DRM_MSM_INACTIVE_PERIOD,
.init = a6xx_gpu_init,
+ .speedbins = a618_speedbins,
+ .speedbins_count = ARRAY_SIZE(a618_speedbins),
}, {
.rev = ADRENO_REV(6, 3, 0, ANY_ID),
.revn = 630,
--
2.7.4

2020-11-27 18:31:28

by Akhil P Oommen

Subject: [PATCH v2 3/3] arm: dts: sc7180: Add support for gpu fuse

Add support for gpu fuse to help identify the supported opps.

Signed-off-by: Akhil P Oommen <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 6678f1e..8cae3eb 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -675,6 +675,11 @@
reg = <0x25b 0x1>;
bits = <1 3>;
};
+
+ gpu_speed_bin: gpu_speed_bin@1d2 {
+ reg = <0x1d2 0x2>;
+ bits = <5 8>;
+ };
};

sdhc_1: sdhci@7c4000 {
@@ -1907,52 +1912,69 @@
operating-points-v2 = <&gpu_opp_table>;
qcom,gmu = <&gmu>;

+ nvmem-cells = <&gpu_speed_bin>;
+ nvmem-cell-names = "speed_bin";
+
interconnects = <&gem_noc MASTER_GFX3D 0 &mc_virt SLAVE_EBI1 0>;
interconnect-names = "gfx-mem";

gpu_opp_table: opp-table {
compatible = "operating-points-v2";

+ opp-825000000 {
+ opp-hz = /bits/ 64 <825000000>;
+ opp-level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
+ opp-peak-kBps = <8532000>;
+ opp-supported-hw = <0x04>;
+ };
+
opp-800000000 {
opp-hz = /bits/ 64 <800000000>;
opp-level = <RPMH_REGULATOR_LEVEL_TURBO>;
opp-peak-kBps = <8532000>;
+ opp-supported-hw = <0x07>;
};

opp-650000000 {
opp-hz = /bits/ 64 <650000000>;
opp-level = <RPMH_REGULATOR_LEVEL_NOM_L1>;
opp-peak-kBps = <7216000>;
+ opp-supported-hw = <0x07>;
};

opp-565000000 {
opp-hz = /bits/ 64 <565000000>;
opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
opp-peak-kBps = <5412000>;
+ opp-supported-hw = <0x07>;
};

opp-430000000 {
opp-hz = /bits/ 64 <430000000>;
opp-level = <RPMH_REGULATOR_LEVEL_SVS_L1>;
opp-peak-kBps = <5412000>;
+ opp-supported-hw = <0x07>;
};

opp-355000000 {
opp-hz = /bits/ 64 <355000000>;
opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
opp-peak-kBps = <3072000>;
+ opp-supported-hw = <0x07>;
};

opp-267000000 {
opp-hz = /bits/ 64 <267000000>;
opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
opp-peak-kBps = <3072000>;
+ opp-supported-hw = <0x07>;
};

opp-180000000 {
opp-hz = /bits/ 64 <180000000>;
opp-level = <RPMH_REGULATOR_LEVEL_MIN_SVS>;
opp-peak-kBps = <1804000>;
+ opp-supported-hw = <0x07>;
};
};
};
--
2.7.4

2020-11-30 17:05:29

by Jordan Crouse

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

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 <[email protected]>
> ---
> 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

> 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
>

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

2020-12-02 15:27:21

by Akhil P Oommen

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

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 <[email protected]>
>> ---
>> 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?

-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
>>
>

2020-12-02 15:32:56

by Akhil P Oommen

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

<< Resending since Jordan wasn't in the CC list >>

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 <[email protected]>
>> ---
>> 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?

-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
>>
>

2020-12-02 16:34:28

by Jordan Crouse

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 <[email protected]>
> >>---
> >>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
> [email protected]
> 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

2020-12-03 16:05:09

by Akhil P Oommen

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

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 <[email protected]>
>>>> ---
>>>> 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>