[PATCH] drm/msm/a6xx: restore previous freq on resume

[PATCH] drm/msm/a6xx: restore previous freq on resume

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Previously, if the freq were overriden (ie. via sysfs), it would get
reset to max on resume.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8 ++++++--
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 2ca470eb5cb8..b64867701e5a 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -149,6 +149,8 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq)
 		if (freq == gmu->gpu_freqs[perf_index])
 			break;
 
+	gmu->current_perf_index = perf_index;
+
 	__a6xx_gmu_set_freq(gmu, perf_index);
 }
 
@@ -741,8 +743,8 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
 	gmu_write(gmu, REG_A6XX_GMU_GMU2HOST_INTR_MASK, ~A6XX_HFI_IRQ_MASK);
 	enable_irq(gmu->hfi_irq);
 
-	/* Set the GPU to the highest power frequency */
-	__a6xx_gmu_set_freq(gmu, gmu->nr_gpu_freqs - 1);
+	/* Set the GPU to the current freq */
+	__a6xx_gmu_set_freq(gmu, gmu->current_perf_index);
 
 	/*
 	 * "enable" the GX power domain which won't actually do anything but it
@@ -1166,6 +1168,8 @@ static int a6xx_gmu_pwrlevels_probe(struct a6xx_gmu *gmu)
 	gmu->nr_gpu_freqs = a6xx_gmu_build_freq_table(&gpu->pdev->dev,
 		gmu->gpu_freqs, ARRAY_SIZE(gmu->gpu_freqs));
 
+	gmu->current_perf_index = gmu->nr_gpu_freqs - 1;
+
 	/* Build the list of RPMh votes that we'll send to the GMU */
 	return a6xx_gmu_rpmh_votes_init(gmu);
 }
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 39a26dd63674..2af91ed7ed0c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -63,6 +63,9 @@ struct a6xx_gmu {
 	struct clk_bulk_data *clocks;
 	struct clk *core_clk;
 
+	/* current performance index set externally */
+	int current_perf_index;
+
 	int nr_gpu_freqs;
 	unsigned long gpu_freqs[16];
 	u32 gx_arc_votes[16];
-- 
2.23.0

Re: [PATCH] drm/msm/a6xx: restore previous freq on resume

[Jordan Crouse]

On Mon, Nov 18, 2019 at 03:40:38PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Previously, if the freq were overriden (ie. via sysfs), it would get
> reset to max on resume.

Devfreq goes to sleep assuming that the hardware will still be at the same
frequency when it wakes up but the GMU sneaks out in the middle of the night
and takes the hardware for a joyride.

Reviewed-by: Jordan Crouse <jcrouse@codeaurora.org>

> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8 ++++++--
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 3 +++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 2ca470eb5cb8..b64867701e5a 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -149,6 +149,8 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq)
>  		if (freq == gmu->gpu_freqs[perf_index])
>  			break;
>  
> +	gmu->current_perf_index = perf_index;
> +
>  	__a6xx_gmu_set_freq(gmu, perf_index);
>  }
>  
> @@ -741,8 +743,8 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
>  	gmu_write(gmu, REG_A6XX_GMU_GMU2HOST_INTR_MASK, ~A6XX_HFI_IRQ_MASK);
>  	enable_irq(gmu->hfi_irq);
>  
> -	/* Set the GPU to the highest power frequency */
> -	__a6xx_gmu_set_freq(gmu, gmu->nr_gpu_freqs - 1);
> +	/* Set the GPU to the current freq */
> +	__a6xx_gmu_set_freq(gmu, gmu->current_perf_index);
>  
>  	/*
>  	 * "enable" the GX power domain which won't actually do anything but it
> @@ -1166,6 +1168,8 @@ static int a6xx_gmu_pwrlevels_probe(struct a6xx_gmu *gmu)
>  	gmu->nr_gpu_freqs = a6xx_gmu_build_freq_table(&gpu->pdev->dev,
>  		gmu->gpu_freqs, ARRAY_SIZE(gmu->gpu_freqs));
>  
> +	gmu->current_perf_index = gmu->nr_gpu_freqs - 1;
> +
>  	/* Build the list of RPMh votes that we'll send to the GMU */
>  	return a6xx_gmu_rpmh_votes_init(gmu);
>  }
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> index 39a26dd63674..2af91ed7ed0c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> @@ -63,6 +63,9 @@ struct a6xx_gmu {
>  	struct clk_bulk_data *clocks;
>  	struct clk *core_clk;
>  
> +	/* current performance index set externally */
> +	int current_perf_index;
> +
>  	int nr_gpu_freqs;
>  	unsigned long gpu_freqs[16];
>  	u32 gx_arc_votes[16];
> -- 
> 2.23.0
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] drm/msm/a6xx: restore previous freq on resume

[Stephen Boyd]

Quoting Rob Clark (2019-11-18 15:40:38)
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> index 39a26dd63674..2af91ed7ed0c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> @@ -63,6 +63,9 @@ struct a6xx_gmu {
>         struct clk_bulk_data *clocks;
>         struct clk *core_clk;
>  
> +       /* current performance index set externally */
> +       int current_perf_index;
> +

Is there a reason this isn't unsigned? It looks like
__a6xx_gmu_set_freq() takes an int, but maybe it should take a u16 or
something?

Re: [PATCH] drm/msm/a6xx: restore previous freq on resume

[Rob Clark]

On Mon, Nov 18, 2019 at 4:32 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rob Clark (2019-11-18 15:40:38)
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> > index 39a26dd63674..2af91ed7ed0c 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
> > @@ -63,6 +63,9 @@ struct a6xx_gmu {
> >         struct clk_bulk_data *clocks;
> >         struct clk *core_clk;
> >
> > +       /* current performance index set externally */
> > +       int current_perf_index;
> > +
>
> Is there a reason this isn't unsigned? It looks like
> __a6xx_gmu_set_freq() takes an int, but maybe it should take a u16 or
> something?
>

no particular reason, other than other things where already using an
int.. this is just an index into the table of opp's so it is never
going to be a large int.

Depending on GMU_DCVS_PERF_SETTING it could probably be a u8 (I'm not
*entirely* sure from the code how large that bitfield is)

BR,
-R