| 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: <b5074339-21a2-06e8-7703-6bdc905944c8@codeaurora.org>
Date: Thu, 26 Nov 2020 21:08:57 +0530
From: Akhil P Oommen <akhilpo@...eaurora.org>
To: Rob Clark <robdclark@...il.com>
Cc: linux-arm-msm <linux-arm-msm@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Douglas Anderson <dianders@...omium.org>,
Matthias Kaehlcke <mka@...omium.org>,
dri-devel@...edesktop.org,
freedreno <freedreno@...ts.freedesktop.org>
Subject: Re: [PATCH] drm/msm: adreno: Make speed-bin support generic
On 11/16/2020 9:52 PM, Rob Clark wrote:
> On Mon, Nov 16, 2020 at 6:34 AM Akhil P Oommen <akhilpo@...eaurora.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@...eaurora.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@...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);
>>>> }
>>>>
>>>> +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@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Powered by blists - more mailing lists