On Thu, Aug 23, 2018 at 02:48:30PM +0530, Sharat Masetty wrote:
> devfreq framework requires the drivers to provide busy time estimations.
It would help if you added an article to this sentence, i.e: "The devfreq
framework..."
> The GPU driver relies on the hardware performance counteres for the busy time
> estimations, but different hardware revisions have counters which can be
> sourced from different clocks. So the busy time estimation will be target
> dependent. Additionally on targets where the clocks are completely controlled
> by the on chip microcontroller, fetching and setting the current GPU frequency
> will be different. This patch aims to embrace these differences by re-factoring
> the devfreq code a bit.
Other than that, the code looks good. A bit of churn, but for a good cause.
Reviewed-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
>
> Signed-off-by: Sharat Masetty <smasetty@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 16 +++++++++---
> drivers/gpu/drm/msm/msm_gpu.c | 49 ++++++++++++++++++++---------------
> drivers/gpu/drm/msm/msm_gpu.h | 5 +++-
> 3 files changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 897f3e2..043e680 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1369,12 +1369,20 @@ static struct msm_ringbuffer *a5xx_active_ring(struct msm_gpu *gpu)
> return a5xx_gpu->cur_ring;
> }
>
> -static int a5xx_gpu_busy(struct msm_gpu *gpu, uint64_t *value)
> +static unsigned long a5xx_gpu_busy(struct msm_gpu *gpu)
> {
> - *value = gpu_read64(gpu, REG_A5XX_RBBM_PERFCTR_RBBM_0_LO,
> - REG_A5XX_RBBM_PERFCTR_RBBM_0_HI);
> + u64 busy_cycles;
> + unsigned long busy_time;
>
> - return 0;
> + busy_cycles = gpu_read64(gpu, REG_A5XX_RBBM_PERFCTR_RBBM_0_LO,
> + REG_A5XX_RBBM_PERFCTR_RBBM_0_HI);
> +
> + busy_time = (busy_cycles - gpu->devfreq.busy_cycles) /
> + (clk_get_rate(gpu->core_clk) / 1000000);
> +
> + gpu->devfreq.busy_cycles = busy_cycles;
> +
> + return busy_time;
> }
>
> static const struct adreno_gpu_funcs funcs = {
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 83fd602..32269ef 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -36,12 +36,16 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
> struct msm_gpu *gpu = platform_get_drvdata(to_platform_device(dev));
> struct dev_pm_opp *opp;
>
> - opp = dev_pm_opp_find_freq_ceil(dev, freq);
> + opp = devfreq_recommended_opp(dev, freq, flags);
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
>
> - if (!IS_ERR(opp)) {
> + if (gpu->funcs->gpu_set_freq)
> + gpu->funcs->gpu_set_freq(gpu, (u64)*freq);
> + else
> clk_set_rate(gpu->core_clk, *freq);
> - dev_pm_opp_put(opp);
> - }
> +
> + dev_pm_opp_put(opp);
>
> return 0;
> }
> @@ -50,16 +54,14 @@ static int msm_devfreq_get_dev_status(struct device *dev,
> struct devfreq_dev_status *status)
> {
> struct msm_gpu *gpu = platform_get_drvdata(to_platform_device(dev));
> - u64 cycles;
> ktime_t time;
>
> - status->current_frequency = (unsigned long) clk_get_rate(gpu->core_clk);
> - gpu->funcs->gpu_busy(gpu, &cycles);
> -
> - status->busy_time = (cycles - gpu->devfreq.busy_cycles) /
> - (status->current_frequency / 1000000);
> + if (gpu->funcs->gpu_get_freq)
> + status->current_frequency = gpu->funcs->gpu_get_freq(gpu);
> + else
> + status->current_frequency = clk_get_rate(gpu->core_clk);
>
> - gpu->devfreq.busy_cycles = cycles;
> + status->busy_time = gpu->funcs->gpu_busy(gpu);
>
> time = ktime_get();
> status->total_time = ktime_us_delta(time, gpu->devfreq.time);
> @@ -72,7 +74,10 @@ static int msm_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> {
> struct msm_gpu *gpu = platform_get_drvdata(to_platform_device(dev));
>
> - *freq = (unsigned long) clk_get_rate(gpu->core_clk);
> + if (gpu->funcs->gpu_get_freq)
> + *freq = gpu->funcs->gpu_get_freq(gpu);
> + else
> + *freq = clk_get_rate(gpu->core_clk);
>
> return 0;
> }
> @@ -87,7 +92,7 @@ static int msm_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> static void msm_devfreq_init(struct msm_gpu *gpu)
> {
> /* We need target support to do devfreq */
> - if (!gpu->funcs->gpu_busy || !gpu->core_clk)
> + if (!gpu->funcs->gpu_busy)
> return;
>
> msm_devfreq_profile.initial_freq = gpu->fast_rate;
> @@ -185,6 +190,14 @@ static int disable_axi(struct msm_gpu *gpu)
> return 0;
> }
>
> +void msm_gpu_resume_devfreq(struct msm_gpu *gpu)
> +{
> + gpu->devfreq.busy_cycles = 0;
> + gpu->devfreq.time = ktime_get();
> +
> + devfreq_resume_device(gpu->devfreq.devfreq);
> +}
> +
> int msm_gpu_pm_resume(struct msm_gpu *gpu)
> {
> int ret;
> @@ -203,12 +216,7 @@ int msm_gpu_pm_resume(struct msm_gpu *gpu)
> if (ret)
> return ret;
>
> - if (gpu->devfreq.devfreq) {
> - gpu->devfreq.busy_cycles = 0;
> - gpu->devfreq.time = ktime_get();
> -
> - devfreq_resume_device(gpu->devfreq.devfreq);
> - }
> + msm_gpu_resume_devfreq(gpu);
>
> gpu->needs_hw_init = true;
>
> @@ -221,8 +229,7 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
>
> DBG("%s", gpu->name);
>
> - if (gpu->devfreq.devfreq)
> - devfreq_suspend_device(gpu->devfreq.devfreq);
> + devfreq_suspend_device(gpu->devfreq.devfreq);
>
> ret = disable_axi(gpu);
> if (ret)
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 2ae34e3..2446066 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -68,9 +68,11 @@ struct msm_gpu_funcs {
> void (*show)(struct msm_gpu *gpu, struct msm_gpu_state *state,
> struct drm_printer *p);
> #endif
> - int (*gpu_busy)(struct msm_gpu *gpu, uint64_t *value);
> + unsigned long (*gpu_busy)(struct msm_gpu *gpu);
> struct msm_gpu_state *(*gpu_state_get)(struct msm_gpu *gpu);
> int (*gpu_state_put)(struct msm_gpu_state *state);
> + unsigned long (*gpu_get_freq)(struct msm_gpu *gpu);
> + int (*gpu_set_freq)(struct msm_gpu *gpu, unsigned long freq);
> };
>
> struct msm_gpu {
> @@ -262,6 +264,7 @@ static inline void gpu_write64(struct msm_gpu *gpu, u32 lo, u32 hi, u64 val)
>
> int msm_gpu_pm_suspend(struct msm_gpu *gpu);
> int msm_gpu_pm_resume(struct msm_gpu *gpu);
> +void msm_gpu_resume_devfreq(struct msm_gpu *gpu);
>
> int msm_gpu_hw_init(struct msm_gpu *gpu);
>
> --
> 1.9.1
>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
