On Fri, Aug 24, 2018 at 03:24:04PM +0530, Sharat Masetty wrote:
>
>
> On 8/23/2018 9:30 PM, Jordan Crouse wrote:
> >On Thu, Aug 23, 2018 at 02:48:31PM +0530, Sharat Masetty wrote:
> >>Implement routines to estimate GPU busy time and fetching the
> >>current frequency for the polling interval. This is required by
> >>the devfreq framework which recommends a frequency change if needed.
> >>The driver code then tries to set this new frequency on the GPU by
> >>sending an Out Of Band(OOB) request.
> >
> >"sending an Out of Band (OOB) request _to the GMU_". Otherwise it is a little
> >confusing as to who is doing what.
> >
> >>
> >>Signed-off-by: Sharat Masetty <smasetty@xxxxxxxxxxxxxx>
> >>---
> >> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 46 +++++++++++++++++++++++++++++++----
> >> drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 2 ++
> >> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 27 ++++++++++++++++++++
> >> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 2 ++
> >> 4 files changed, 72 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >>index f6634c0..92ff48b 100644
> >>--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >>+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> >>@@ -67,8 +67,10 @@ static bool a6xx_gmu_gx_is_on(struct a6xx_gmu *gmu)
> >> A6XX_GMU_SPTPRAC_PWR_CLK_STATUS_GX_HM_CLK_OFF));
> >> }
> >>-static int a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)
> >>+static int __a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)
> >> {
> >>+ int ret;
> >
> >Should be a u32 since we are doing a gmu_read().
> >
> >>+
> >> gmu_write(gmu, REG_A6XX_GMU_DCVS_ACK_OPTION, 0);
> >> gmu_write(gmu, REG_A6XX_GMU_DCVS_PERF_SETTING,
> >>@@ -84,7 +86,41 @@ static int a6xx_gmu_set_freq(struct a6xx_gmu *gmu, int index)
> >> a6xx_gmu_set_oob(gmu, GMU_OOB_DCVS_SET);
> >> a6xx_gmu_clear_oob(gmu, GMU_OOB_DCVS_SET);
> >>- return gmu_read(gmu, REG_A6XX_GMU_DCVS_RETURN);
> >>+ ret = gmu_read(gmu, REG_A6XX_GMU_DCVS_RETURN);
> >>+ if (!ret)
> >>+ gmu->cur_freq = gmu->gpu_freqs[index];
> >>+
> >
> >'ret' from the register read won't be an appropriate Unix error message so it
> >should be translated - otherwise it will be confusing because
> >'a6xx_gmu_set_freq' otherwise returns 0 or valid error messages.
> >
> >>+ return ret;
> >>+}
> >>+
> >>+int a6xx_gmu_set_freq(struct msm_gpu *gpu, unsigned long freq)
> >>+{
> >>+ struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> >>+ struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> >>+ struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
> >>+ u32 perf_index = 0;
> >>+
> >>+ if (freq == gmu->cur_freq)
> >>+ return 0;
> >>+
> >>+ //TODO: Use a hashmap instead? This gets called potentially every ~10 ms
> >
> >Please don't use C++ style comments. A TODO is okay, but I would prefer if you
> >solved this question. I'm not sure if walking a short list of 10 items is a big
> >concern if it happens every 10ms or so.
> Sure, I will take care of this...
> >
> >>+ for (perf_index = 0; perf_index < gmu->nr_gpu_freqs; perf_index++)
> >>+ if (freq == gmu->gpu_freqs[perf_index])
> >>+ break;
> >
> >Are you positive we don't need to worry about rounding here - will devfreq
> >*always* give you an exact frequency value? I know the clock subsystem allows
> >for rounding. You might want to double check just to be sure that we don't need
> >to worry about that here.
> >
> >In particular, I would be concerned about the userspace governor for devfreq
> >where the user can set anything they want. I'm not 100% sure that gets vetted
> >against the OPP table before we get to this point.
> >
> >>+ if (perf_index == gmu->nr_gpu_freqs)
> >>+ return -EINVAL;
> >
> >Related to the previous comment slightly, if devfreq wants to set a frequency of
> >a hundred million HZ is it an error or should we just clamp to the highest
> >available frequency and call it good?
> For this and the comment above, we use the devfreq_recommended_opp()
> function to get a proper OPP from our OPP list in the dt for the GPU
> device.
So if we are sure the incoming frequency is always valid then perf_index will
always match and we know this if statement will never be true. So we should
get rid of it.
If you are paranoid about the list being wrong you could do change the for loop
so that it always defaulted to the highest priority and then remove the if
statement:
- for (perf_index = 0; perf_index < gmu->nr_gpu_freqs; perf_index++)
+ for (perf_index = 0; perf_index < gmu->nr_gpu_freqs - 1; perf_index++)
Jordan
--
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
