[PATCH 10/10] drm/msm/adreno: Prevent unclocked access when retrieving timestamps

[Jordan Crouse]

On Fri, Jul 28, 2017 at 04:17:08PM +0530, Archit Taneja wrote:
> msm_gpu's get_timestamp() op (called by the MSM_GET_PARAM ioctl) can
> result in register accesses. We need our power domain and clocks to
> be active for that. Make sure they are enabled here.
> 
> Signed-off-by: Archit Taneja <architt at codeaurora.org>
> ---
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index f1ab2703674a..7414c6bbd582 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -48,8 +48,15 @@ int adreno_get_param(struct msm_gpu *gpu, uint32_t param, uint64_t *value)
>  		*value = adreno_gpu->base.fast_rate;
>  		return 0;
>  	case MSM_PARAM_TIMESTAMP:
> -		if (adreno_gpu->funcs->get_timestamp)
> -			return adreno_gpu->funcs->get_timestamp(gpu, value);
> +		if (adreno_gpu->funcs->get_timestamp) {
> +			int ret;
> +
> +			pm_runtime_get_sync(&gpu->pdev->dev);
> +			ret = adreno_gpu->funcs->get_timestamp(gpu, value);
> +			pm_runtime_put_autosuspend(&gpu->pdev->dev);
> +
> +			return ret;
> +		}
>  		return -EINVAL;
>  	default:
>  		DBG("%s: invalid param: %u", gpu->name, param);

This is clearly correct from a stability standpoint but it does raise
a few side questions.

When the system is idle it is not interesting to go to all the work of bringing
up the system to read a timestamp that will be 0 (or very close to it) and then
take the system down again.  Furthermore when the system is going up and down
repeatedly between submits the counter will keep getting reset to zero and
makes it less useful over the long term.

Downstream had these problems too and has an elaborate mechanism to save the
value of the counters at power down and (on 4XX and newer) reload the
counters with the previous value when power comes back. This has the advantage
of consistent numbers with the downside of a reasonable amount of work to
maintain the database of allocated counters and taking the to save them on
power down and restore them on power up.

This patch should be obviously merged because system hangs are bad but I just
wanted to toss out more food for thought as we get more power aggressive and
more interested in performance monitoring.

Jordan

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