Re: [PATCH 2/3] drm/msm/a6xx: Move power counter selectable to resume()

From: Jordan Crouse <jcrouse-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
To: Sharat Masetty <smasetty-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 2/3] drm/msm/a6xx: Move power counter selectable to resume()
Date: Wed, 17 Oct 2018 08:09:43 -0600	[thread overview]
Message-ID: <20181017140943.GC4751@jcrouse-lnx.qualcomm.com> (raw)
In-Reply-To: <1539781441-13076-3-git-send-email-smasetty-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

On Wed, Oct 17, 2018 at 06:34:00PM +0530, Sharat Masetty wrote:
> Moving this to resume helps with both GPU DCVS and the performance
> sampling. For GPU DCVS this makes sure that the frequency does not scale
> when there are no GPU submissions. In the case of performance profiling,
> the GPU is UP, but its possible that the hw_init() was not called yet,
> so we need this to get sane perf values in all cases.

This description is worded oddly to me but I think the gist is that you
want the counters running before you start sampling. I agree which is why I
think that the /* FIXME: */ is right and this should go into in the GMU code
(a6xx_rpmh_start to be exact). The timing should still work out and the GMU code
can stay with the GMU code which is always a bonus.

> Signed-off-by: Sharat Masetty <smasetty@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index c0cd3ac..2c52b7c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -387,14 +387,6 @@ static int a6xx_hw_init(struct msm_gpu *gpu)
>  	/* Select CP0 to always count cycles */
>  	gpu_write(gpu, REG_A6XX_CP_PERFCTR_CP_SEL_0, PERF_CP_ALWAYS_COUNT);
>  
> -	/* FIXME: not sure if this should live here or in a6xx_gmu.c */
> -	gmu_write(&a6xx_gpu->gmu,  REG_A6XX_GPU_GMU_AO_GPU_CX_BUSY_MASK,
> -		0xff000000);
> -	gmu_rmw(&a6xx_gpu->gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_SELECT_0,
> -		0xff, 0x20);
> -	gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_ENABLE,
> -		0x01);
> -
>  	gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL, 2 << 1);
>  	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, 2 << 1);
>  	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, 2 << 1);
> @@ -655,6 +647,14 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
>  
>  	gpu->needs_hw_init = true;
>  
> +	/* FIXME: not sure if this should live here or in a6xx_gmu.c */

If in case you don't move this,  this line needs to go away.

> +	gmu_write(&a6xx_gpu->gmu,  REG_A6XX_GPU_GMU_AO_GPU_CX_BUSY_MASK,
> +		0xff000000);
> +	gmu_rmw(&a6xx_gpu->gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_SELECT_0,
> +		0xff, 0x20);
> +	gmu_write(&a6xx_gpu->gmu, REG_A6XX_GMU_CX_GMU_POWER_COUNTER_ENABLE,
> +		0x01);
> +
>  	msm_gpu_resume_devfreq(gpu);
>  
>  	return ret;
> -- 
> 1.9.1
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno