| Message ID | 20170728104708.20635-11-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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
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);
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@codeaurora.org> --- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)