| 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
| ||
|
Date: Thu, 23 Feb 2023 01:16:56 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Sean Paul <sean@...rly.run>, David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>
Cc: linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation
On 23/02/2023 00:40, Konrad Dybcio wrote:
>
>
> On 22.02.2023 23:38, Dmitry Baryshkov wrote:
>> On 22/02/2023 23:47, Konrad Dybcio wrote:
>>> Some older GPUs (namely a2xx with no opp tables at all and a320 with
>>> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
>>> both however had just one frequency defined, making it extremely easy
>>> to construct such an OPP table from within the driver if need be.
>>>
>>> Do so and switch all clk_set_rate calls on core_clk to their OPP
>>> counterparts.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
>>> ---
>>> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>>> drivers/gpu/drm/msm/msm_gpu.c | 4 +-
>>> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 2 +-
>>> 3 files changed, 45 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> index ce6b76c45b6f..9b940c0f063f 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>>> ring->id);
>>> }
>>> -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
>>> -static int adreno_get_legacy_pwrlevels(struct device *dev)
>>> -{
>>> - struct device_node *child, *node;
>>> - int ret;
>>> -
>>> - node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
>>> - if (!node) {
>>> - DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
>>> - return -ENXIO;
>>> - }
>>> -
>>> - for_each_child_of_node(node, child) {
>>> - unsigned int val;
>>> -
>>> - ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
>>> - if (ret)
>>> - continue;
>>> -
>>> - /*
>>> - * Skip the intentionally bogus clock value found at the bottom
>>> - * of most legacy frequency tables
>>> - */
>>> - if (val != 27000000)
>>> - dev_pm_opp_add(dev, val, 0);
>>> - }
>>> -
>>> - of_node_put(node);
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static void adreno_get_pwrlevels(struct device *dev,
>>> +static int adreno_get_pwrlevels(struct device *dev,
>>> struct msm_gpu *gpu)
>>> {
>>> + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>> unsigned long freq = ULONG_MAX;
>>> struct dev_pm_opp *opp;
>>> int ret;
>>> gpu->fast_rate = 0;
>>> - /* You down with OPP? */
>>> - if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>>> - ret = adreno_get_legacy_pwrlevels(dev);
>>> - else {
>>> - ret = devm_pm_opp_of_add_table(dev);
>>> - if (ret)
>>> - DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>> - }
>>> -
>>> - if (!ret) {
>>> + /* devm_pm_opp_of_add_table may error out but will still create an OPP table */
>>> + ret = devm_pm_opp_of_add_table(dev);
>>> + if (ret == -ENODEV) {
>>> + /* Special cases for ancient hw with ancient DT bindings */
>>> + if (adreno_is_a2xx(adreno_gpu)) {
>>> + dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
>>> + dev_pm_opp_add(dev, 200000000, 0);
>>> + gpu->fast_rate = 200000000;
>>
>> We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will get it from our freshly generated opp table.
> It's not reached in this code path.
I see. I got lost in all the ifs. What do you think about turning it
into the main code path, since after this code block we always have a
valid OPP table?
>
>>
>>> + } else if (adreno_is_a320(adreno_gpu)) {
>>> + dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
>>> + dev_pm_opp_add(dev, 450000000, 0);
>>> + gpu->fast_rate = 450000000;
>>> + } else {
>>> + DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
>>> + return -ENODEV;
>>> + }
>>> + } else if (ret) {
>>> + DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>> + return ret;
>>> + } else {
>>> /* Find the fastest defined rate */
>>> opp = dev_pm_opp_find_freq_floor(dev, &freq);
>>> - if (!IS_ERR(opp)) {
>>> +
>>> + if (IS_ERR(opp))
>>> + return PTR_ERR(opp);
>>> + else {
>>> gpu->fast_rate = freq;
>>> dev_pm_opp_put(opp);
>>> }
>>> }
>>> - if (!gpu->fast_rate) {
>>> - dev_warn(dev,
>>> - "Could not find a clock rate. Using a reasonable default\n");
>>> - /* Pick a suitably safe clock speed for any target */
>>> - gpu->fast_rate = 200000000;
>>> - }
>>> -
>>> DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
>>> +
>>> + return 0;
>>> }
>>> int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
--
With best wishes
Dmitry
Powered by blists - more mailing lists