Re: [PATCH 4/5] drm/msm: re-factor devfreq code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux