[PATCH v2 6/7] drm/msm/adreno: Redo the speedbin assignment
There is no need to reinvent the wheel for simple read-match-set logic.
Make speedbin discovery and assignment generation independent.
This implicitly removes the bogus 0x80 / BIT(7) speed bin on A5xx,
which has no representation in hardware whatshowever.
Signed-off-by: Konrad Dybcio
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 56 -
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 51 ++
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 3 --
4 files changed, 45 insertions(+), 99 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index c003f970189b..eed6a2eb1731 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1704,38 +1704,6 @@ static const struct adreno_gpu_funcs funcs = {
.get_timestamp = a5xx_get_timestamp,
};
-static void check_speed_bin(struct device *dev)
-{
- struct nvmem_cell *cell;
- u32 val;
-
- /*
-* If the OPP table specifies a opp-supported-hw property then we have
-* to set something with dev_pm_opp_set_supported_hw() or the table
-* doesn't get populated so pick an arbitrary value that should
-* ensure the default frequencies are selected but not conflict with any
-* actual bins
-*/
- val = 0x80;
-
- cell = nvmem_cell_get(dev, "speed_bin");
-
- if (!IS_ERR(cell)) {
- void *buf = nvmem_cell_read(cell, NULL);
-
- if (!IS_ERR(buf)) {
- u8 bin = *((u8 *) buf);
-
- val = (1 << bin);
- kfree(buf);
- }
-
- nvmem_cell_put(cell);
- }
-
- devm_pm_opp_set_supported_hw(dev, , 1);
-}
-
struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
{
struct msm_drm_private *priv = dev->dev_private;
@@ -1763,8 +1731,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
a5xx_gpu->lm_leakage = 0x4E001A;
- check_speed_bin(>dev);
-
nr_rings = 4;
if (config->info->revn == 510)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 3f84417ff027..d256e27ee581 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2882,55 +2882,6 @@ static bool a6xx_progress(struct msm_gpu *gpu, struct
msm_ringbuffer *ring)
return progress;
}
-static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse)
-{
- if (!info->speedbins)
- return UINT_MAX;
-
- for (int i = 0; info->speedbins[i].fuse != SHRT_MAX; i++)
- if (info->speedbins[i].fuse == fuse)
- return BIT(info->speedbins[i].speedbin);
-
- return UINT_MAX;
-}
-
-static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu,
-struct device *dev,
-const struct adreno_info *info)
-{
- u32 supp_hw;
- u32 speedbin;
- int ret;
-
- ret = adreno_read_speedbin(adreno_gpu, dev, );
- /*
-* -ENOENT means that the platform doesn't support speedbin which is
-* fine
-*/
- if (ret == -ENOENT) {
- return 0;
- } else if (ret) {
- dev_err_probe(dev, ret,
- "failed to read speed-bin. Some OPPs may not be
supported by hardware\n");
- return ret;
- }
-
- supp_hw = fuse_to_supp_hw(info, speedbin);
-
- if (supp_hw == UINT_MAX) {
- DRM_DEV_ERROR(dev,
- "missing support for speed-bin: %u. Some OPPs may not
be supported by hardware\n",
- speedbin);
- supp_hw = BIT(0); /* Default */
- }
-
- ret = devm_pm_opp_set_supported_hw(dev, _hw, 1);
- if (ret)
- return ret;
-
- return 0;
-}
-
static const struct adreno_gpu_funcs funcs = {
.base = {
.get_param = adreno_get_param,
@@ -3062,13 +3013,6 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx);
- ret = a6xx_set_supported_hw(adreno_gpu, >dev, config->info);
- if (ret) {
- a6xx_llc_slices_destroy(a6xx_gpu);
- kfree(a6xx_gpu);
- return ERR_PTR(ret);
- }
-
if (is_a7xx)
ret = adreno_gpu_init(dev, pdev, adreno_gpu, _a7xx, 1);
else if (adreno_has_gmu_wrapper(adreno_gpu))
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 055072260b3d..8b2bc5f147e8 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -1060,8 +1060,8 @@ void adreno_gpu_ocmem_cleanup(struct adreno_ocmem
*adreno_ocmem)
Re: [PATCH v2 6/7] drm/msm/adreno: Redo the speedbin assignment
On Thu, Apr 18, 2024 at 11:57:35AM +0200, Konrad Dybcio wrote:
> On 18.04.2024 1:49 AM, Dmitry Baryshkov wrote:
> > On Wed, Apr 17, 2024 at 10:02:58PM +0200, Konrad Dybcio wrote:
> >> There is no need to reinvent the wheel for simple read-match-set logic.
> >>
> >> Make speedbin discovery and assignment generation independent.
> >>
> >> This implicitly removes the bogus 0x80 / BIT(7) speed bin on A5xx,
> >> which has no representation in hardware whatshowever.
> >>
> >> Signed-off-by: Konrad Dybcio
> >> ---
>
> [...]
>
> >> + /* No speedbins defined for this GPU SKU => allow all defined OPPs */
> >> + if (!info->speedbins) {
> >> + adreno_gpu->speedbin = ADRENO_SPEEDBIN_FUSE_NODATA;
> >> + return devm_pm_opp_set_supported_hw(dev, _hw, 1);
> >
> > BIT(0)
>
> You mean for _hw, or "1"?
>
> 1 is the "count" parameter, supp_hw is a "u32 supported_hw[count]"
I see. It confused me. This way it's getting set to UINT_MAX, which
will match against any non-zero opp-supported-hw. Ack.
>
> >
> >> + }
> >> +
> >> + /*
> >> + * If a real error (not counting older devicetrees having no nvmem
> >> references)
> >> + * occurs when trying to get the fuse value, bail out.
> >> + */
> >> + ret = adreno_read_speedbin(adreno_gpu, dev, );
> >> + if (ret) {
> >> + return ret;
> >> + } else if (fuse == ADRENO_SPEEDBIN_FUSE_NODATA) {
> >> + /* The info struct has speedbin data, but the DT is too old =>
> >> allow all OPPs */
> >
> > Missing assignment to adeno_gpu->speedbin ? Or is it fine?
>
> Good catch. Only mesa (and I suppose you :D) read this value.
>
> >
> >> + DRM_DEV_INFO(dev, "No GPU speed bin fuse, please update your
> >> device tree\n");
> >> + return devm_pm_opp_set_supported_hw(dev, _hw, 1);
> >
> > BIT(0)
> > maybe #define it?
>
> (ditto)
>
> Konrad
--
With best wishes
Dmitry
Re: [PATCH v2 6/7] drm/msm/adreno: Redo the speedbin assignment
On 18.04.2024 1:49 AM, Dmitry Baryshkov wrote:
> On Wed, Apr 17, 2024 at 10:02:58PM +0200, Konrad Dybcio wrote:
>> There is no need to reinvent the wheel for simple read-match-set logic.
>>
>> Make speedbin discovery and assignment generation independent.
>>
>> This implicitly removes the bogus 0x80 / BIT(7) speed bin on A5xx,
>> which has no representation in hardware whatshowever.
>>
>> Signed-off-by: Konrad Dybcio
>> ---
[...]
>> +/* No speedbins defined for this GPU SKU => allow all defined OPPs */
>> +if (!info->speedbins) {
>> +adreno_gpu->speedbin = ADRENO_SPEEDBIN_FUSE_NODATA;
>> +return devm_pm_opp_set_supported_hw(dev, _hw, 1);
>
> BIT(0)
You mean for _hw, or "1"?
1 is the "count" parameter, supp_hw is a "u32 supported_hw[count]"
>
>> +}
>> +
>> +/*
>> + * If a real error (not counting older devicetrees having no nvmem
>> references)
>> + * occurs when trying to get the fuse value, bail out.
>> + */
>> +ret = adreno_read_speedbin(adreno_gpu, dev, );
>> +if (ret) {
>> +return ret;
>> +} else if (fuse == ADRENO_SPEEDBIN_FUSE_NODATA) {
>> +/* The info struct has speedbin data, but the DT is too old =>
>> allow all OPPs */
>
> Missing assignment to adeno_gpu->speedbin ? Or is it fine?
Good catch. Only mesa (and I suppose you :D) read this value.
>
>> +DRM_DEV_INFO(dev, "No GPU speed bin fuse, please update your
>> device tree\n");
>> +return devm_pm_opp_set_supported_hw(dev, _hw, 1);
>
> BIT(0)
> maybe #define it?
(ditto)
Konrad
Re: [PATCH v2 6/7] drm/msm/adreno: Redo the speedbin assignment
On Wed, Apr 17, 2024 at 10:02:58PM +0200, Konrad Dybcio wrote:
> There is no need to reinvent the wheel for simple read-match-set logic.
>
> Make speedbin discovery and assignment generation independent.
>
> This implicitly removes the bogus 0x80 / BIT(7) speed bin on A5xx,
> which has no representation in hardware whatshowever.
>
> Signed-off-by: Konrad Dybcio
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 56 --
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 70
> +++--
> drivers/gpu/drm/msm/adreno/adreno_gpu.h | 3 --
> drivers/gpu/drm/msm/msm_gpu.c | 3 --
> 5 files changed, 57 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index c003f970189b..eed6a2eb1731 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1704,38 +1704,6 @@ static const struct adreno_gpu_funcs funcs = {
> .get_timestamp = a5xx_get_timestamp,
> };
>
> -static void check_speed_bin(struct device *dev)
> -{
> - struct nvmem_cell *cell;
> - u32 val;
> -
> - /*
> - * If the OPP table specifies a opp-supported-hw property then we have
> - * to set something with dev_pm_opp_set_supported_hw() or the table
> - * doesn't get populated so pick an arbitrary value that should
> - * ensure the default frequencies are selected but not conflict with any
> - * actual bins
> - */
> - val = 0x80;
> -
> - cell = nvmem_cell_get(dev, "speed_bin");
> -
> - if (!IS_ERR(cell)) {
> - void *buf = nvmem_cell_read(cell, NULL);
> -
> - if (!IS_ERR(buf)) {
> - u8 bin = *((u8 *) buf);
> -
> - val = (1 << bin);
> - kfree(buf);
> - }
> -
> - nvmem_cell_put(cell);
> - }
> -
> - devm_pm_opp_set_supported_hw(dev, , 1);
> -}
> -
> struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
> {
> struct msm_drm_private *priv = dev->dev_private;
> @@ -1763,8 +1731,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>
> a5xx_gpu->lm_leakage = 0x4E001A;
>
> - check_speed_bin(>dev);
> -
> nr_rings = 4;
>
> if (config->info->revn == 510)
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 60708c23ae4c..1242697d64a7 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -2878,55 +2878,6 @@ static bool a6xx_progress(struct msm_gpu *gpu, struct
> msm_ringbuffer *ring)
> return progress;
> }
>
> -static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse)
> -{
> - if (!info->speedbins)
> - return UINT_MAX;
> -
> - for (int i = 0; info->speedbins[i].fuse != SHRT_MAX; i++)
> - if (info->speedbins[i].fuse == fuse)
> - return BIT(info->speedbins[i].speedbin);
> -
> - return UINT_MAX;
> -}
> -
> -static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu,
> - struct device *dev,
> - const struct adreno_info *info)
> -{
> - u32 supp_hw;
> - u32 speedbin;
> - int ret;
> -
> - ret = adreno_read_speedbin(adreno_gpu, dev, );
> - /*
> - * -ENOENT means that the platform doesn't support speedbin which is
> - * fine
> - */
> - if (ret == -ENOENT) {
> - return 0;
> - } else if (ret) {
> - dev_err_probe(dev, ret,
> - "failed to read speed-bin. Some OPPs may not be
> supported by hardware\n");
> - return ret;
> - }
> -
> - supp_hw = fuse_to_supp_hw(info, speedbin);
> -
> - if (supp_hw == UINT_MAX) {
> - DRM_DEV_ERROR(dev,
> - "missing support for speed-bin: %u. Some OPPs may not
> be supported by hardware\n",
> - speedbin);
> - supp_hw = BIT(0); /* Default */
> - }
> -
> - ret = devm_pm_opp_set_supported_hw(dev, _hw, 1);
> - if (ret)
> - return ret;
> -
> - return 0;
> -}
> -
> static const struct adreno_gpu_funcs funcs = {
> .base = {
> .get_param = adreno_get_param,
> @@ -3058,13 +3009,6 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>
> a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx);
>
> - ret = a6xx_set_supported_hw(adreno_gpu, >dev, config->info);
> - if (ret) {
> - a6xx_llc_slices_destroy(a6xx_gpu);
> - kfree(a6xx_gpu);
> - return ERR_PTR(ret);
> - }
> -
> if (is_a7xx)
> ret = adreno_gpu_init(dev, pdev, adreno_gpu, _a7xx, 1);
> else if (adreno_has_gmu_wrapper(adreno_gpu))
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>
[PATCH v2 6/7] drm/msm/adreno: Redo the speedbin assignment
There is no need to reinvent the wheel for simple read-match-set logic.
Make speedbin discovery and assignment generation independent.
This implicitly removes the bogus 0x80 / BIT(7) speed bin on A5xx,
which has no representation in hardware whatshowever.
Signed-off-by: Konrad Dybcio
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 34
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 56 --
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 70 +++--
drivers/gpu/drm/msm/adreno/adreno_gpu.h | 3 --
drivers/gpu/drm/msm/msm_gpu.c | 3 --
5 files changed, 57 insertions(+), 109 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index c003f970189b..eed6a2eb1731 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1704,38 +1704,6 @@ static const struct adreno_gpu_funcs funcs = {
.get_timestamp = a5xx_get_timestamp,
};
-static void check_speed_bin(struct device *dev)
-{
- struct nvmem_cell *cell;
- u32 val;
-
- /*
-* If the OPP table specifies a opp-supported-hw property then we have
-* to set something with dev_pm_opp_set_supported_hw() or the table
-* doesn't get populated so pick an arbitrary value that should
-* ensure the default frequencies are selected but not conflict with any
-* actual bins
-*/
- val = 0x80;
-
- cell = nvmem_cell_get(dev, "speed_bin");
-
- if (!IS_ERR(cell)) {
- void *buf = nvmem_cell_read(cell, NULL);
-
- if (!IS_ERR(buf)) {
- u8 bin = *((u8 *) buf);
-
- val = (1 << bin);
- kfree(buf);
- }
-
- nvmem_cell_put(cell);
- }
-
- devm_pm_opp_set_supported_hw(dev, , 1);
-}
-
struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
{
struct msm_drm_private *priv = dev->dev_private;
@@ -1763,8 +1731,6 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
a5xx_gpu->lm_leakage = 0x4E001A;
- check_speed_bin(>dev);
-
nr_rings = 4;
if (config->info->revn == 510)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 60708c23ae4c..1242697d64a7 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -2878,55 +2878,6 @@ static bool a6xx_progress(struct msm_gpu *gpu, struct
msm_ringbuffer *ring)
return progress;
}
-static u32 fuse_to_supp_hw(const struct adreno_info *info, u32 fuse)
-{
- if (!info->speedbins)
- return UINT_MAX;
-
- for (int i = 0; info->speedbins[i].fuse != SHRT_MAX; i++)
- if (info->speedbins[i].fuse == fuse)
- return BIT(info->speedbins[i].speedbin);
-
- return UINT_MAX;
-}
-
-static int a6xx_set_supported_hw(struct adreno_gpu *adreno_gpu,
-struct device *dev,
-const struct adreno_info *info)
-{
- u32 supp_hw;
- u32 speedbin;
- int ret;
-
- ret = adreno_read_speedbin(adreno_gpu, dev, );
- /*
-* -ENOENT means that the platform doesn't support speedbin which is
-* fine
-*/
- if (ret == -ENOENT) {
- return 0;
- } else if (ret) {
- dev_err_probe(dev, ret,
- "failed to read speed-bin. Some OPPs may not be
supported by hardware\n");
- return ret;
- }
-
- supp_hw = fuse_to_supp_hw(info, speedbin);
-
- if (supp_hw == UINT_MAX) {
- DRM_DEV_ERROR(dev,
- "missing support for speed-bin: %u. Some OPPs may not
be supported by hardware\n",
- speedbin);
- supp_hw = BIT(0); /* Default */
- }
-
- ret = devm_pm_opp_set_supported_hw(dev, _hw, 1);
- if (ret)
- return ret;
-
- return 0;
-}
-
static const struct adreno_gpu_funcs funcs = {
.base = {
.get_param = adreno_get_param,
@@ -3058,13 +3009,6 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
a6xx_llc_slices_init(pdev, a6xx_gpu, is_a7xx);
- ret = a6xx_set_supported_hw(adreno_gpu, >dev, config->info);
- if (ret) {
- a6xx_llc_slices_destroy(a6xx_gpu);
- kfree(a6xx_gpu);
- return ERR_PTR(ret);
- }
-
if (is_a7xx)
ret = adreno_gpu_init(dev, pdev, adreno_gpu, _a7xx, 1);
else if (adreno_has_gmu_wrapper(adreno_gpu))
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 58fd70140685..08b2f08e2a14 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -1060,8 +1060,8 @@ void adreno_gpu_ocmem_cleanup(struct
