On Thu, Aug 23, 2018 at 02:48:31PM +0530, Sharat Masetty wrote:
> Implement routines to estimate GPU busy time and fetching the
> current frequency for the polling interval. This is required by
> the devfreq framework which recommends a frequency change if needed.
> The driver code then tries to set this new frequency on the GPU by
> sending an Out Of Band(OOB) request.
"sending an Out of Band (OOB) request _to the GMU_". Otherwise it is a little
confusing as to who is doing what.
>
> Signed-off-by: Sharat Masetty <smasetty@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 46 +++++++++++++++++++++++++++++++----
> drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 2 ++
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 27 ++++++++++++++++++++
> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 ++
> 4 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index f6634c0..92ff48b 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -67,8 +67,10 @@ static bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu)
> A6XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_CLK_OFF));
> }
>
> -static int a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)
> +static int __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)
> {
> + int ret;
Should be a u32 since we are doing a gmu_read().
> +
> gmu_write(gmu, REG_A6XX_GMU_DCVS_ACK_OPTION, 0);
>
> gmu_write(gmu, REG_A6XX_GMU_DCVS_PERF_SETTING,
> @@ -84,7 +86,41 @@ static int a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)
> a6xx_gmu_set_oob(gmu, GMU_OOB_DCVS_SET);
> a6xx_gmu_clear_oob(gmu, GMU_OOB_DCVS_SET);
>
> - return gmu_read(gmu, REG_A6XX_GMU_DCVS_RETURN);
> + ret = gmu_read(gmu, REG_A6XX_GMU_DCVS_RETURN);
> + if (!ret)
> + gmu->cur_freq = gmu->gpu_freqs[index];
> +
'ret' from the register read won't be an appropriate Unix error message so it
should be translated - otherwise it will be confusing because
'a6xx_gmu_set_freq' otherwise returns 0 or valid error messages.
> + return ret;
> +}
> +
> +int a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq)
> +{
> + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> + struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
> + u32 perf_index = 0;
> +
> + if (freq == gmu->cur_freq)
> + return 0;
> +
> + //TODO: Use a hashmap instead? This gets called potentially every ~10 ms
Please don't use C++ style comments. A TODO is okay, but I would prefer if you
solved this question. I'm not sure if walking a short list of 10 items is a big
concern if it happens every 10ms or so.
> + for (perf_index = 0; perf_index < gmu->nr_gpu_freqs; perf_index++)
> + if (freq == gmu->gpu_freqs[perf_index])
> + break;
Are you positive we don't need to worry about rounding here - will devfreq
*always* give you an exact frequency value? I know the clock subsystem allows
for rounding. You might want to double check just to be sure that we don't need
to worry about that here.
In particular, I would be concerned about the userspace governor for devfreq
where the user can set anything they want. I'm not 100% sure that gets vetted
against the OPP table before we get to this point.
> + if (perf_index == gmu->nr_gpu_freqs)
> + return -EINVAL;
Related to the previous comment slightly, if devfreq wants to set a frequency of
a hundred million HZ is it an error or should we just clamp to the highest
available frequency and call it good?
> + return __a6xx_gmu_set_freq(gmu, perf_index);
> +}
> +
> +unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu)
> +{
> + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> + struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
> +
> + return gmu->cur_freq;
> }
>
> static bool a6xx_gmu_check_idle_level(struct a6xx_gmu *gmu)
> @@ -629,8 +665,8 @@ int a6xx_gmu_reset(struct a6xx_gpu *a6xx_gpu)
> if (!ret)
> ret = a6xx_hfi_start(gmu, GMU_COLD_BOOT);
>
> - /* Set the GPU back to the highest power frequency */
> - a6xx_gmu_set_freq(gmu, gmu->nr_gpu_freqs - 1);
> + /* Save the current frequency for devfreq */
> + gmu->cur_freq = gmu->gpu_freqs[gmu->nr_gpu_freqs - 1];
I'm not sure I understand this change - don't we need to set the frequency
GPU immediately out of reset even if DCVS is expected to change it soon?
>
> out:
> if (ret)
> @@ -671,7 +707,7 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
> ret = a6xx_hfi_start(gmu, status);
>
> /* Set the GPU to the highest power frequency */
> - a6xx_gmu_set_freq(gmu, gmu->nr_gpu_freqs - 1);
> + __a6xx_gmu_set_freq(gmu, gmu->nr_gpu_freqs - 1);
>
> out:
> /* Make sure to turn off the boot OOB request on error */
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> index f9e4dfe..ce6e5ca 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> @@ -77,6 +77,8 @@ struct a6xx_gmu {
> unsigned long gmu_freqs[4];
> u32 cx_arc_votes[4];
>
> + unsigned long cur_freq;
> +
This could just be 'freq'.
> struct a6xx_hfi_queue queues[2];
>
> struct tasklet_struct hfi_tasklet;
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 3429d33a..af90706 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -7,6 +7,8 @@
> #include "a6xx_gpu.h"
> #include "a6xx_gmu.xml.h"
>
> +#include <linux/devfreq.h>
> +
> static inline bool _a6xx_check_idle(struct msm_gpu *gpu)
> {
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> @@ -682,6 +684,8 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
>
> gpu->needs_hw_init = true;
>
> + msm_gpu_resume_devfreq(gpu);
> +
> return ret;
> }
>
> @@ -690,6 +694,8 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu)
> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
>
> + devfreq_suspend_device(gpu->devfreq.devfreq);
> +
> /*
> * Make sure the GMU is idle before continuing (because some transitions
> * may use VBIF
> @@ -753,6 +759,24 @@ static void a6xx_destroy(struct msm_gpu *gpu)
> kfree(a6xx_gpu);
> }
>
> +static unsigned long a6xx_gpu_busy(struct msm_gpu *gpu)
> +{
> + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> + u64 busy_cycles;
> + unsigned long busy_time;
> +
> + busy_cycles = gmu_read64(&a6xx_gpu->gmu,
> + REG_A6XX_GMU_CX_GMU_POWER_COUNTER_XOCLK_0_L,
> + REG_A6XX_GMU_CX_GMU_POWER_COUNTER_XOCLK_0_H);
> +
> + busy_time = ((busy_cycles - gpu->devfreq.busy_cycles) * 10) / 192;
> +
> + gpu->devfreq.busy_cycles = busy_cycles;
> +
> + return busy_time;
> +}
> +
> static const struct adreno_gpu_funcs funcs = {
> .base = {
> .get_param = adreno_get_param,
> @@ -768,6 +792,9 @@ static void a6xx_destroy(struct msm_gpu *gpu)
> #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
> .show = a6xx_show,
> #endif
> + .gpu_busy = a6xx_gpu_busy,
> + .gpu_get_freq = a6xx_gmu_get_freq,
> + .gpu_set_freq = a6xx_gmu_set_freq,
> },
> .get_timestamp = a6xx_get_timestamp,
> };
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index 32c2501..f236767 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -56,5 +56,7 @@ struct a6xx_gpu {
>
> int a6xx_gmu_probe(struct a6xx_gpu *a6xx_gpu, struct device_node *node, struct platform_device *gpu_pdev);
> void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu);
> +int a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq);
> +unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu);
>
> #endif /* __A6XX_GPU_H__ */
> --
> 1.9.1
>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
