| Message ID | 20230707193942.3806526-5-dmitry.baryshkov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: cleanup dpu_core_perf module | expand |
On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote: > The values in struct dpu_core_perf_tune are fixed per the core perf > mode. Drop the 'tune' values and substitute them with known values when > performing perf management. > > Note: min_bus_vote was not used at all, so it is just silently dropped. > Interesting ..... should bring this back properly. Will take it up. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 ++++++++----------- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 --- > 2 files changed, 12 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > index 05d340aa18c5..348550ac7e51 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > @@ -235,7 +235,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms, > { > struct dpu_core_perf_params perf = { 0 }; > int i, ret = 0; > - u64 avg_bw; > + u32 avg_bw; > avg_bw seems unused in this patch, so unrelated change? > if (!kms->num_paths) > return 0; > @@ -291,10 +291,16 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc) > > static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms) > { > - u64 clk_rate = kms->perf.perf_tune.min_core_clk; > + u64 clk_rate; > struct drm_crtc *crtc; > struct dpu_crtc_state *dpu_cstate; > > + if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) > + return kms->perf.fix_core_clk_rate; > + > + if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) > + return kms->perf.max_core_clk_rate; > + > drm_for_each_crtc(crtc, kms->dev) { > if (crtc->enabled) { > dpu_cstate = to_dpu_crtc_state(crtc->state); > @@ -305,11 +311,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms) > } > } > > - if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) > - clk_rate = kms->perf.fix_core_clk_rate; > - > - DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate); > - Why dont you move both FIXED and MINIMUM handling below instead of above. So that they will just override the clk_rate and you can keep this useful log here and it matches where the function is. This chunk looks better that way. <skipping the rest as it LGTM>
On 7/10/2023 7:34 PM, Dmitry Baryshkov wrote: > On 11/07/2023 05:31, Abhinav Kumar wrote: >> >> >> On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote: >>> The values in struct dpu_core_perf_tune are fixed per the core perf >>> mode. Drop the 'tune' values and substitute them with known values when >>> performing perf management. >>> >>> Note: min_bus_vote was not used at all, so it is just silently dropped. >>> >> >> Interesting ..... should bring this back properly. Will take it up. > > Ack, thanks. > >> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 ++++++++----------- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 --- >>> 2 files changed, 12 insertions(+), 21 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>> index 05d340aa18c5..348550ac7e51 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>> @@ -235,7 +235,7 @@ static int _dpu_core_perf_crtc_update_bus(struct >>> dpu_kms *kms, >>> { >>> struct dpu_core_perf_params perf = { 0 }; >>> int i, ret = 0; >>> - u64 avg_bw; >>> + u32 avg_bw; >> >> avg_bw seems unused in this patch, so unrelated change? >> >>> if (!kms->num_paths) >>> return 0; >>> @@ -291,10 +291,16 @@ void dpu_core_perf_crtc_release_bw(struct >>> drm_crtc *crtc) >>> static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms) >>> { >>> - u64 clk_rate = kms->perf.perf_tune.min_core_clk; >>> + u64 clk_rate; >>> struct drm_crtc *crtc; >>> struct dpu_crtc_state *dpu_cstate; >>> + if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) >>> + return kms->perf.fix_core_clk_rate; >>> + >>> + if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) >>> + return kms->perf.max_core_clk_rate; >>> + >>> drm_for_each_crtc(crtc, kms->dev) { >>> if (crtc->enabled) { >>> dpu_cstate = to_dpu_crtc_state(crtc->state); >>> @@ -305,11 +311,6 @@ static u64 >>> _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms) >>> } >>> } >>> - if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) >>> - clk_rate = kms->perf.fix_core_clk_rate; >>> - >>> - DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate); >>> - >> >> Why dont you move both FIXED and MINIMUM handling below instead of above. >> >> So that they will just override the clk_rate and you can keep this >> useful log here and it matches where the function is. > > I can keep the log in the next version. The logic was quite simple: > there is no need to loop over CRTCs if we know that we are overriding > the value. > Yes I understood that part. I wanted to keep the log and the function together that way we are logging whats the value its going to return. This patch is logging it at the caller. Thats the only difference. I am fine either way. Once the avg_bw change is removed, I can ack this. >> >> This chunk looks better that way. >> >> <skipping the rest as it LGTM> >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c index 05d340aa18c5..348550ac7e51 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c @@ -235,7 +235,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms, { struct dpu_core_perf_params perf = { 0 }; int i, ret = 0; - u64 avg_bw; + u32 avg_bw; if (!kms->num_paths) return 0; @@ -291,10 +291,16 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc) static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms) { - u64 clk_rate = kms->perf.perf_tune.min_core_clk; + u64 clk_rate; struct drm_crtc *crtc; struct dpu_crtc_state *dpu_cstate; + if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) + return kms->perf.fix_core_clk_rate; + + if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) + return kms->perf.max_core_clk_rate; + drm_for_each_crtc(crtc, kms->dev) { if (crtc->enabled) { dpu_cstate = to_dpu_crtc_state(crtc->state); @@ -305,11 +311,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms) } } - if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) - clk_rate = kms->perf.fix_core_clk_rate; - - DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate); - return clk_rate; } @@ -397,6 +398,8 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc, if (update_clk) { clk_rate = _dpu_core_perf_get_core_clk_rate(kms); + DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate); + trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate); clk_rate = min(clk_rate, kms->perf.max_core_clk_rate); @@ -418,7 +421,6 @@ static ssize_t _dpu_core_perf_mode_write(struct file *file, const char __user *user_buf, size_t count, loff_t *ppos) { struct dpu_core_perf *perf = file->private_data; - const struct dpu_perf_cfg *cfg = perf->catalog->perf; u32 perf_mode = 0; int ret; @@ -433,14 +435,9 @@ static ssize_t _dpu_core_perf_mode_write(struct file *file, DRM_INFO("fix performance mode\n"); } else if (perf_mode == DPU_PERF_MODE_MINIMUM) { /* run the driver with max clk and BW vote */ - perf->perf_tune.min_core_clk = perf->max_core_clk_rate; - perf->perf_tune.min_bus_vote = - (u64) cfg->max_bw_high * 1000; DRM_INFO("minimum performance mode\n"); } else if (perf_mode == DPU_PERF_MODE_NORMAL) { /* reset the perf tune params to 0 */ - perf->perf_tune.min_core_clk = 0; - perf->perf_tune.min_bus_vote = 0; DRM_INFO("normal performance mode\n"); } perf->perf_tune.mode = perf_mode; @@ -456,10 +453,8 @@ static ssize_t _dpu_core_perf_mode_read(struct file *file, char buf[128]; len = scnprintf(buf, sizeof(buf), - "mode %d min_mdp_clk %llu min_bus_vote %llu\n", - perf->perf_tune.mode, - perf->perf_tune.min_core_clk, - perf->perf_tune.min_bus_vote); + "mode %d\n", + perf->perf_tune.mode); return simple_read_from_buffer(buff, count, ppos, buf, len); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h index 29bb8ee2bc26..c965dfbc3007 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h @@ -29,13 +29,9 @@ struct dpu_core_perf_params { /** * struct dpu_core_perf_tune - definition of performance tuning control * @mode: performance mode - * @min_core_clk: minimum core clock - * @min_bus_vote: minimum bus vote */ struct dpu_core_perf_tune { u32 mode; - u64 min_core_clk; - u64 min_bus_vote; }; /**
The values in struct dpu_core_perf_tune are fixed per the core perf mode. Drop the 'tune' values and substitute them with known values when performing perf management. Note: min_bus_vote was not used at all, so it is just silently dropped. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 ++++++++----------- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 --- 2 files changed, 12 insertions(+), 21 deletions(-)