| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8c215bdf-b739-4b30-6cb0-fa521fde2784@codeaurora.org>
Date: Mon, 16 Nov 2020 19:40:03 +0530
From: Akhil P Oommen <akhilpo@...eaurora.org>
To: freedreno@...ts.freedesktop.org, dri-devel@...edesktop.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
mka@...omium.org, robdclark@...il.com, dianders@...omium.org
Subject: Re: [PATCH] drm/msm: adreno: Make speed-bin support generic
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@...eaurora.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
>>
>
Powered by blists - more mailing lists