On 23.02.2023 00:16, Dmitry Baryshkov wrote:
> 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@xxxxxxxxxx>
>>>> ---
>>>> 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?
Sounds good!
Konrad
>
>>
>>>
>>>> + } 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,
>
>
