* [PATCH] drm/msm: adreno: Make speed-bin support generic
@ 2020-11-12 15:49 Akhil P Oommen
2020-11-12 16:35 ` Jordan Crouse
2020-11-12 16:37 ` Rob Clark
0 siblings, 2 replies; 9+ messages in thread
From: Akhil P Oommen @ 2020-11-12 15:49 UTC (permalink / raw)
To: freedreno
Cc: dri-devel, linux-arm-msm, linux-kernel, jcrouse, mka, robdclark,
dianders
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@codeaurora.org>
---
This patch is rebased on top of msm-next-staging branch in rob's tree.
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..cdd0c11 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);
+
+ /* A speedbin table is must if the platform supports speedbin */
+ if (!speedbins) {
+ DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
+ return -ENOENT;
+ }
+
+ 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);
+ nvmem_cell_put(cell);
+
+ if (!val) {
+ DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
+ return -ENOENT;
+ }
+
+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm: adreno: Make speed-bin support generic
2020-11-12 15:49 [PATCH] drm/msm: adreno: Make speed-bin support generic Akhil P Oommen
@ 2020-11-12 16:35 ` Jordan Crouse
2020-11-16 14:10 ` Akhil P Oommen
2020-11-12 16:37 ` Rob Clark
1 sibling, 1 reply; 9+ messages in thread
From: Jordan Crouse @ 2020-11-12 16:35 UTC (permalink / raw)
To: Akhil P Oommen
Cc: freedreno, dri-devel, linux-arm-msm, linux-kernel, mka,
robdclark, dianders
On Thu, Nov 12, 2020 at 09:19:04PM +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@codeaurora.org>
> ---
> This patch is rebased on top of msm-next-staging branch in rob's tree.
>
> 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..cdd0c11 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;
We don't need to make this generic and put it in the table. Just call the
function from the target specific code and pass the speedbin array and size from
there.
> + 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);
> +
> + /* A speedbin table is must if the platform supports speedbin */
> + if (!speedbins) {
> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
> + return -ENOENT;
> + }
> +
> + 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);
> + nvmem_cell_put(cell);
> +
> + if (!val) {
> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
> + return -ENOENT;
> + }
> +
> + 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;
This bit should be in the target specific code
> +
> 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);
And this bit too, though it would be easier to just call the put function
directly without having a intermediate function. Also the OPP function should
be NULL aware but thats a different story.
Jordan
> }
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm: adreno: Make speed-bin support generic
2020-11-12 15:49 [PATCH] drm/msm: adreno: Make speed-bin support generic Akhil P Oommen
2020-11-12 16:35 ` Jordan Crouse
@ 2020-11-12 16:37 ` Rob Clark
2020-11-16 14:34 ` Akhil P Oommen
1 sibling, 1 reply; 9+ messages in thread
From: Rob Clark @ 2020-11-12 16:37 UTC (permalink / raw)
To: Akhil P Oommen
Cc: freedreno, dri-devel, linux-arm-msm, Linux Kernel Mailing List,
Jordan Crouse, Matthias Kaehlcke, Douglas Anderson
On Thu, Nov 12, 2020 at 7:49 AM Akhil P Oommen <akhilpo@codeaurora.org> 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@codeaurora.org>
> ---
> This patch is rebased on top of msm-next-staging branch in rob's tree.
>
> 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..cdd0c11 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);
> +
> + /* A speedbin table is must if the platform supports speedbin */
> + if (!speedbins) {
> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
> + return -ENOENT;
Hmm, this means that hw which supports speed-bin, but for which we
haven't yet added a speedbin table, will start failing. Which seems
not great. Maybe it would be better to keep the DRM_DEV_ERROR() (so
people realize something is missing), but return 0?
Or do you think we could add the speed-bin tables for all supported hw
immediately?
BR,
-R
> + }
> +
> + 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);
> + nvmem_cell_put(cell);
> +
> + if (!val) {
> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
> + return -ENOENT;
> + }
> +
> + 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm: adreno: Make speed-bin support generic
2020-11-12 16:35 ` Jordan Crouse
@ 2020-11-16 14:10 ` Akhil P Oommen
2020-11-16 17:14 ` [Freedreno] " Jordan Crouse
0 siblings, 1 reply; 9+ messages in thread
From: Akhil P Oommen @ 2020-11-16 14:10 UTC (permalink / raw)
To: freedreno, dri-devel, linux-arm-msm, linux-kernel, mka,
robdclark, dianders
On 11/12/2020 10:05 PM, Jordan Crouse wrote:
> On Thu, Nov 12, 2020 at 09:19:04PM +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@codeaurora.org>
>> ---
>> This patch is rebased on top of msm-next-staging branch in rob's tree.
>>
>> 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..cdd0c11 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);jjjj
>> }
>>
>> +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;
>
> We don't need to make this generic and put it in the table. Just call the
> function from the target specific code and pass the speedbin array and size from
> there.
>
I didn't get you entirely. Do you mean we should avoid keeping speedbin
array in the adreno_gpu->info table?
-Akhil.
>> + 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);
>> +
>> + /* A speedbin table is must if the platform supports speedbin */
>> + if (!speedbins) {
>> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
>> + return -ENOENT;
>> + }
>> +
>> + 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);
>> + nvmem_cell_put(cell);
>> +
>> + if (!val) {
>> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
>> + return -ENOENT;
>> + }
>> +
>> + 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;
>
> This bit should be in the target specific code
>> +
>> 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);
>
> And this bit too, though it would be easier to just call the put function
> directly without having a intermediate function. Also the OPP function should
> be NULL aware but thats a different story.
>
> Jordan
>> }
>> 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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm: adreno: Make speed-bin support generic
2020-11-12 16:37 ` Rob Clark
@ 2020-11-16 14:34 ` Akhil P Oommen
2020-11-16 16:22 ` Rob Clark
0 siblings, 1 reply; 9+ messages in thread
From: Akhil P Oommen @ 2020-11-16 14:34 UTC (permalink / raw)
To: Rob Clark
Cc: freedreno, dri-devel, linux-arm-msm, Linux Kernel Mailing List,
Jordan Crouse, Matthias Kaehlcke, Douglas Anderson
On 11/12/2020 10:07 PM, Rob Clark wrote:
> On Thu, Nov 12, 2020 at 7:49 AM Akhil P Oommen <akhilpo@codeaurora.org> 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@codeaurora.org>
>> ---
>> This patch is rebased on top of msm-next-staging branch in rob's tree.
>>
>> 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..cdd0c11 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);
>> +
>> + /* A speedbin table is must if the platform supports speedbin */
>> + if (!speedbins) {
>> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
>> + return -ENOENT;
>
> Hmm, this means that hw which supports speed-bin, but for which we
> haven't yet added a speedbin table, will start failing. Which seems
> not great. Maybe it would be better to keep the DRM_DEV_ERROR() (so
> people realize something is missing), but return 0?
We can't because if the gpu opp table has "opp-supported-hw" property,
opp driver expects us to call dev_pm_opp_set_supported_hw() to select
the supported hardware. I think we can just pick a default one and also
print a detailed warning, will that work for you?
-Akhil.
>
> Or do you think we could add the speed-bin tables for all supported hw
> immediately?
>
> BR,
> -R
>
>> + }
>> +
>> + 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);
>> + nvmem_cell_put(cell);
>> +
>> + if (!val) {
>> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
>> + return -ENOENT;
>> + }
>> +
>> + 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
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm: adreno: Make speed-bin support generic
2020-11-16 14:34 ` Akhil P Oommen
@ 2020-11-16 16:22 ` Rob Clark
2020-11-26 15:38 ` Akhil P Oommen
0 siblings, 1 reply; 9+ messages in thread
From: Rob Clark @ 2020-11-16 16:22 UTC (permalink / raw)
To: Akhil P Oommen
Cc: freedreno, dri-devel, linux-arm-msm, Linux Kernel Mailing List,
Jordan Crouse, Matthias Kaehlcke, Douglas Anderson
On Mon, Nov 16, 2020 at 6:34 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>
> On 11/12/2020 10:07 PM, Rob Clark wrote:
> > On Thu, Nov 12, 2020 at 7:49 AM Akhil P Oommen <akhilpo@codeaurora.org> 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@codeaurora.org>
> >> ---
> >> This patch is rebased on top of msm-next-staging branch in rob's tree.
> >>
> >> 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..cdd0c11 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);
> >> +
> >> + /* A speedbin table is must if the platform supports speedbin */
> >> + if (!speedbins) {
> >> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
> >> + return -ENOENT;
> >
> > Hmm, this means that hw which supports speed-bin, but for which we
> > haven't yet added a speedbin table, will start failing. Which seems
> > not great. Maybe it would be better to keep the DRM_DEV_ERROR() (so
> > people realize something is missing), but return 0?
> We can't because if the gpu opp table has "opp-supported-hw" property,
> opp driver expects us to call dev_pm_opp_set_supported_hw() to select
> the supported hardware. I think we can just pick a default one and also
> print a detailed warning, will that work for you?
That seems like it could work.. or maybe just skip all this if there
is no opp table?
BR,
-R
> -Akhil.
> >
> > Or do you think we could add the speed-bin tables for all supported hw
> > immediately?
> >
> > BR,
> > -R
> >
> >> + }
> >> +
> >> + 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);
> >> + nvmem_cell_put(cell);
> >> +
> >> + if (!val) {
> >> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
> >> + return -ENOENT;
> >> + }
> >> +
> >> + 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
> >>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: adreno: Make speed-bin support generic
2020-11-16 14:10 ` Akhil P Oommen
@ 2020-11-16 17:14 ` Jordan Crouse
2020-11-27 12:51 ` Akhil P Oommen
0 siblings, 1 reply; 9+ messages in thread
From: Jordan Crouse @ 2020-11-16 17:14 UTC (permalink / raw)
To: Akhil P Oommen
Cc: freedreno, dri-devel, linux-arm-msm, linux-kernel, mka,
robdclark, dianders
On Mon, Nov 16, 2020 at 07:40:03PM +0530, Akhil P Oommen wrote:
> On 11/12/2020 10:05 PM, Jordan Crouse wrote:
> >On Thu, Nov 12, 2020 at 09:19:04PM +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@codeaurora.org>
> >>---
> >>This patch is rebased on top of msm-next-staging branch in rob's tree.
> >>
> >> 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..cdd0c11 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);jjjj
> >> }
> >>+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;
> >
> >We don't need to make this generic and put it in the table. Just call the
> >function from the target specific code and pass the speedbin array and size from
> >there.
> >
> I didn't get you entirely. Do you mean we should avoid keeping speedbin
> array in the adreno_gpu->info table?
Exactly.
Jordan
> -Akhil.
> >>+ 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);
> >>+
> >>+ /* A speedbin table is must if the platform supports speedbin */
> >>+ if (!speedbins) {
> >>+ DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
> >>+ return -ENOENT;
> >>+ }
> >>+
> >>+ 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);
> >>+ nvmem_cell_put(cell);
> >>+
> >>+ if (!val) {
> >>+ DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
> >>+ return -ENOENT;
> >>+ }
> >>+
> >>+ 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;
> >
> >This bit should be in the target specific code
> >>+
> >> 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);
> >
> >And this bit too, though it would be easier to just call the put function
> >directly without having a intermediate function. Also the OPP function should
> >be NULL aware but thats a different story.
> >
> >Jordan
> >> }
> >>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
> >>
> >
>
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm: adreno: Make speed-bin support generic
2020-11-16 16:22 ` Rob Clark
@ 2020-11-26 15:38 ` Akhil P Oommen
0 siblings, 0 replies; 9+ messages in thread
From: Akhil P Oommen @ 2020-11-26 15:38 UTC (permalink / raw)
To: Rob Clark
Cc: linux-arm-msm, Linux Kernel Mailing List, Douglas Anderson,
Matthias Kaehlcke, dri-devel, freedreno
On 11/16/2020 9:52 PM, Rob Clark wrote:
> On Mon, Nov 16, 2020 at 6:34 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>>
>> On 11/12/2020 10:07 PM, Rob Clark wrote:
>>> On Thu, Nov 12, 2020 at 7:49 AM Akhil P Oommen <akhilpo@codeaurora.org> 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@codeaurora.org>
>>>> ---
>>>> This patch is rebased on top of msm-next-staging branch in rob's tree.
>>>>
>>>> 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..cdd0c11 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);
>>>> +
>>>> + /* A speedbin table is must if the platform supports speedbin */
>>>> + if (!speedbins) {
>>>> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
>>>> + return -ENOENT;
>>>
>>> Hmm, this means that hw which supports speed-bin, but for which we
>>> haven't yet added a speedbin table, will start failing. Which seems
>>> not great. Maybe it would be better to keep the DRM_DEV_ERROR() (so
>>> people realize something is missing), but return 0?
>> We can't because if the gpu opp table has "opp-supported-hw" property,
>> opp driver expects us to call dev_pm_opp_set_supported_hw() to select
>> the supported hardware. I think we can just pick a default one and also
>> print a detailed warning, will that work for you?
>
> That seems like it could work.. or maybe just skip all this if there
> is no opp table?
>
> BR,
> -R
Rob, I will share a new patchset shortly.
-Akhil.
>
>> -Akhil.
>>>
>>> Or do you think we could add the speed-bin tables for all supported hw
>>> immediately?
>>>
>>> BR,
>>> -R
>>>
>>>> + }
>>>> +
>>>> + 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);
>>>> + nvmem_cell_put(cell);
>>>> +
>>>> + if (!val) {
>>>> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
>>>> + return -ENOENT;
>>>> + }
>>>> +
>>>> + 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
>>>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm: adreno: Make speed-bin support generic
2020-11-16 17:14 ` [Freedreno] " Jordan Crouse
@ 2020-11-27 12:51 ` Akhil P Oommen
0 siblings, 0 replies; 9+ messages in thread
From: Akhil P Oommen @ 2020-11-27 12:51 UTC (permalink / raw)
To: freedreno, dri-devel, linux-arm-msm, linux-kernel, mka,
robdclark, dianders
On 11/16/2020 10:44 PM, Jordan Crouse wrote:
> On Mon, Nov 16, 2020 at 07:40:03PM +0530, Akhil P Oommen wrote:
>> On 11/12/2020 10:05 PM, Jordan Crouse wrote:
>>> On Thu, Nov 12, 2020 at 09:19:04PM +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@codeaurora.org>
>>>> ---
>>>> This patch is rebased on top of msm-next-staging branch in rob's tree.
>>>>
>>>> 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..cdd0c11 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);jjjj
>>>> }
>>>> +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;
>>>
>>> We don't need to make this generic and put it in the table. Just call the
>>> function from the target specific code and pass the speedbin array and size from
>>> there.
>>>
>> I didn't get you entirely. Do you mean we should avoid keeping speedbin
>> array in the adreno_gpu->info table?
>
> Exactly.
>
> Jordan
But why duplicate this code if it can be made generic? Could you please
check the v2 version?
-Akhil.
>
>> -Akhil.
>>>> + 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);
>>>> +
>>>> + /* A speedbin table is must if the platform supports speedbin */
>>>> + if (!speedbins) {
>>>> + DRM_DEV_ERROR(dev, "speed-bin table is missing\n");
>>>> + return -ENOENT;
>>>> + }
>>>> +
>>>> + 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);
>>>> + nvmem_cell_put(cell);
>>>> +
>>>> + if (!val) {
>>>> + DRM_DEV_ERROR(dev, "missing support for speed-bin: %u\n", bin);
>>>> + return -ENOENT;
>>>> + }
>>>> +
>>>> + 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;
>>>
>>> This bit should be in the target specific code
>>>> +
>>>> 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);
>>>
>>> And this bit too, though it would be easier to just call the put function
>>> directly without having a intermediate function. Also the OPP function should
>>> be NULL aware but thats a different story.
>>>
>>> Jordan
>>>> }
>>>> 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
>>>>
>>>
>>
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-27 12:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-12 15:49 [PATCH] drm/msm: adreno: Make speed-bin support generic Akhil P Oommen
2020-11-12 16:35 ` Jordan Crouse
2020-11-16 14:10 ` Akhil P Oommen
2020-11-16 17:14 ` [Freedreno] " Jordan Crouse
2020-11-27 12:51 ` Akhil P Oommen
2020-11-12 16:37 ` Rob Clark
2020-11-16 14:34 ` Akhil P Oommen
2020-11-16 16:22 ` Rob Clark
2020-11-26 15:38 ` Akhil P Oommen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).