| Message ID | 20250106-dpu-perf-rework-v4-4-00b248349476@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm/dpu: rework debugfs interface of dpu_core_perf | expand |
On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote: > The fix_core_ab_vote is an average bandwidth value, used for bandwidth > overrides in several cases. However there is an internal inconsistency: > fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined > in Bps. > > Fix that by changing the type of the variable to u32 and using * 1000ULL > multiplier when setting up the dpu_core_perf_params::bw_ctl value. > Actually after looking at this, I have another question. How did you conclude that fix_core_ib_vote is in KBps? min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote? It depends on the interpretation perhaps. If the debugfs was supposed to operate under the expectation that the provided value will be pre-multiplied by 1000 and given then that explains why it was not multiplied again. And I cross-checked some of the internal usages of the debugfs, the values provided to it were in Bps and not KBps. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +- > 2 files changed, 3 insertions(+), 3 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 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf, > perf->max_per_pipe_ib = 0; > perf->core_clk_rate = 0; > } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) { > - perf->bw_ctl = core_perf->fix_core_ab_vote; > + perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL; > perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; > perf->core_clk_rate = core_perf->fix_core_clk_rate; > } else { > @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent) > &perf->fix_core_clk_rate); > debugfs_create_u32("fix_core_ib_vote", 0600, entry, > &perf->fix_core_ib_vote); > - debugfs_create_u64("fix_core_ab_vote", 0600, entry, > + debugfs_create_u32("fix_core_ab_vote", 0600, entry, > &perf->fix_core_ab_vote); > > return 0; > 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 ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > @@ -51,7 +51,7 @@ struct dpu_core_perf { > u32 enable_bw_release; > u64 fix_core_clk_rate; > u32 fix_core_ib_vote; > - u64 fix_core_ab_vote; > + u32 fix_core_ab_vote; > }; > > int dpu_core_perf_crtc_check(struct drm_crtc *crtc, >
On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote: > > > On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote: > > The fix_core_ab_vote is an average bandwidth value, used for bandwidth > > overrides in several cases. However there is an internal inconsistency: > > fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined > > in Bps. > > > > Fix that by changing the type of the variable to u32 and using * 1000ULL > > multiplier when setting up the dpu_core_perf_params::bw_ctl value. > > > > Actually after looking at this, I have another question. > > How did you conclude that fix_core_ib_vote is in KBps? > > min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote? > > It depends on the interpretation perhaps. If the debugfs was supposed to > operate under the expectation that the provided value will be pre-multiplied > by 1000 and given then that explains why it was not multiplied again. > > And I cross-checked some of the internal usages of the debugfs, the values > provided to it were in Bps and not KBps. Well... As you wrote min_dram_ib is in KBps. So, by comparing the next two lines, fix_core_ib_vote should also be in kBps, as there is no premultiplier: perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; [...] perf->max_per_pipe_ib = perf_cfg->min_dram_ib; And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw() without any modifications: icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib); > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +- > > 2 files changed, 3 insertions(+), 3 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 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > > @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf, > > perf->max_per_pipe_ib = 0; > > perf->core_clk_rate = 0; > > } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) { > > - perf->bw_ctl = core_perf->fix_core_ab_vote; > > + perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL; > > perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; > > perf->core_clk_rate = core_perf->fix_core_clk_rate; > > } else { > > @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent) > > &perf->fix_core_clk_rate); > > debugfs_create_u32("fix_core_ib_vote", 0600, entry, > > &perf->fix_core_ib_vote); > > - debugfs_create_u64("fix_core_ab_vote", 0600, entry, > > + debugfs_create_u32("fix_core_ab_vote", 0600, entry, > > &perf->fix_core_ab_vote); > > return 0; > > 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 ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > > @@ -51,7 +51,7 @@ struct dpu_core_perf { > > u32 enable_bw_release; > > u64 fix_core_clk_rate; > > u32 fix_core_ib_vote; > > - u64 fix_core_ab_vote; > > + u32 fix_core_ab_vote; > > }; > > int dpu_core_perf_crtc_check(struct drm_crtc *crtc, > >
On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote: > On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote: >> >> >> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote: >>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth >>> overrides in several cases. However there is an internal inconsistency: >>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined >>> in Bps. >>> >>> Fix that by changing the type of the variable to u32 and using * 1000ULL >>> multiplier when setting up the dpu_core_perf_params::bw_ctl value. >>> >> >> Actually after looking at this, I have another question. >> >> How did you conclude that fix_core_ib_vote is in KBps? >> >> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote? >> >> It depends on the interpretation perhaps. If the debugfs was supposed to >> operate under the expectation that the provided value will be pre-multiplied >> by 1000 and given then that explains why it was not multiplied again. >> >> And I cross-checked some of the internal usages of the debugfs, the values >> provided to it were in Bps and not KBps. > > Well... As you wrote min_dram_ib is in KBps. So, by comparing the next > two lines, fix_core_ib_vote should also be in kBps, as there is no > premultiplier: > > perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; > [...] > perf->max_per_pipe_ib = perf_cfg->min_dram_ib; > > And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw() > without any modifications: > > icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib); > Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote is always in Bps and not in KBps because bw_ctl is in Bps. Is it really a discrepancy that fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps because they are just following the units in which bw_ctl and max_per_pipe_ib were defined in resp. > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++-- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +- >>> 2 files changed, 3 insertions(+), 3 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 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf, >>> perf->max_per_pipe_ib = 0; >>> perf->core_clk_rate = 0; >>> } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) { >>> - perf->bw_ctl = core_perf->fix_core_ab_vote; >>> + perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL; >>> perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; >>> perf->core_clk_rate = core_perf->fix_core_clk_rate; >>> } else { >>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent) >>> &perf->fix_core_clk_rate); >>> debugfs_create_u32("fix_core_ib_vote", 0600, entry, >>> &perf->fix_core_ib_vote); >>> - debugfs_create_u64("fix_core_ab_vote", 0600, entry, >>> + debugfs_create_u32("fix_core_ab_vote", 0600, entry, >>> &perf->fix_core_ab_vote); >>> return 0; >>> 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 ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >>> @@ -51,7 +51,7 @@ struct dpu_core_perf { >>> u32 enable_bw_release; >>> u64 fix_core_clk_rate; >>> u32 fix_core_ib_vote; >>> - u64 fix_core_ab_vote; >>> + u32 fix_core_ab_vote; >>> }; >>> int dpu_core_perf_crtc_check(struct drm_crtc *crtc, >>> >
On 11 January 2025 01:49:23 EET, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > >On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote: >> On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote: >>> >>> >>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote: >>>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth >>>> overrides in several cases. However there is an internal inconsistency: >>>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined >>>> in Bps. >>>> >>>> Fix that by changing the type of the variable to u32 and using * 1000ULL >>>> multiplier when setting up the dpu_core_perf_params::bw_ctl value. >>>> >>> >>> Actually after looking at this, I have another question. >>> >>> How did you conclude that fix_core_ib_vote is in KBps? >>> >>> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote? >>> >>> It depends on the interpretation perhaps. If the debugfs was supposed to >>> operate under the expectation that the provided value will be pre-multiplied >>> by 1000 and given then that explains why it was not multiplied again. >>> >>> And I cross-checked some of the internal usages of the debugfs, the values >>> provided to it were in Bps and not KBps. >> >> Well... As you wrote min_dram_ib is in KBps. So, by comparing the next >> two lines, fix_core_ib_vote should also be in kBps, as there is no >> premultiplier: >> >> perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; >> [...] >> perf->max_per_pipe_ib = perf_cfg->min_dram_ib; >> >> And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw() >> without any modifications: >> >> icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib); >> > >Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote is always in Bps and not in KBps because bw_ctl is in Bps. > >Is it really a discrepancy that fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps because they are just following the units in which bw_ctl and max_per_pipe_ib were defined in resp. Yes. They come in pair, as a part of the user interface. If one is in Bps and another one in KBps, it is very easy to forget that and misinterpret them or to make a mistake while programming them. Not to mention that the threshold files, which are related to AB, are in KBps. > >> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++-- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +- >>>> 2 files changed, 3 insertions(+), 3 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 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf, >>>> perf->max_per_pipe_ib = 0; >>>> perf->core_clk_rate = 0; >>>> } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) { >>>> - perf->bw_ctl = core_perf->fix_core_ab_vote; >>>> + perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL; >>>> perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; >>>> perf->core_clk_rate = core_perf->fix_core_clk_rate; >>>> } else { >>>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent) >>>> &perf->fix_core_clk_rate); >>>> debugfs_create_u32("fix_core_ib_vote", 0600, entry, >>>> &perf->fix_core_ib_vote); >>>> - debugfs_create_u64("fix_core_ab_vote", 0600, entry, >>>> + debugfs_create_u32("fix_core_ab_vote", 0600, entry, >>>> &perf->fix_core_ab_vote); >>>> return 0; >>>> 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 ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >>>> @@ -51,7 +51,7 @@ struct dpu_core_perf { >>>> u32 enable_bw_release; >>>> u64 fix_core_clk_rate; >>>> u32 fix_core_ib_vote; >>>> - u64 fix_core_ab_vote; >>>> + u32 fix_core_ab_vote; >>>> }; >>>> int dpu_core_perf_crtc_check(struct drm_crtc *crtc, >>>> >>
On 1/11/2025 5:08 AM, Dmitry Baryshkov wrote: > On 11 January 2025 01:49:23 EET, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote: >>> On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote: >>>> >>>> >>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote: >>>>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth >>>>> overrides in several cases. However there is an internal inconsistency: >>>>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined >>>>> in Bps. >>>>> >>>>> Fix that by changing the type of the variable to u32 and using * 1000ULL >>>>> multiplier when setting up the dpu_core_perf_params::bw_ctl value. >>>>> >>>> >>>> Actually after looking at this, I have another question. >>>> >>>> How did you conclude that fix_core_ib_vote is in KBps? >>>> >>>> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote? >>>> >>>> It depends on the interpretation perhaps. If the debugfs was supposed to >>>> operate under the expectation that the provided value will be pre-multiplied >>>> by 1000 and given then that explains why it was not multiplied again. >>>> >>>> And I cross-checked some of the internal usages of the debugfs, the values >>>> provided to it were in Bps and not KBps. >>> >>> Well... As you wrote min_dram_ib is in KBps. So, by comparing the next >>> two lines, fix_core_ib_vote should also be in kBps, as there is no >>> premultiplier: >>> >>> perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; >>> [...] >>> perf->max_per_pipe_ib = perf_cfg->min_dram_ib; >>> >>> And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw() >>> without any modifications: >>> >>> icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib); >>> >> >> Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote is always in Bps and not in KBps because bw_ctl is in Bps. >> >> Is it really a discrepancy that fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps because they are just following the units in which bw_ctl and max_per_pipe_ib were defined in resp. > > Yes. They come in pair, as a part of the user interface. If one is in Bps and another one in KBps, it is very easy to forget that and misinterpret them or to make a mistake while programming them. Not to mention that the threshold files, which are related to AB, are in KBps. > In that case, the documentation for both needs to be updated as well as it still says both are in bps not kbps. * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2 * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2 */ struct dpu_core_perf { const struct dpu_perf_cfg *perf_cfg; u64 core_clk_rate; u64 max_core_clk_rate; struct dpu_core_perf_tune perf_tune; u32 enable_bw_release; u64 fix_core_clk_rate; u64 fix_core_ib_vote; u64 fix_core_ab_vote; }; >> >>> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++-- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +- >>>>> 2 files changed, 3 insertions(+), 3 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 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>>>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf, >>>>> perf->max_per_pipe_ib = 0; >>>>> perf->core_clk_rate = 0; >>>>> } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) { >>>>> - perf->bw_ctl = core_perf->fix_core_ab_vote; >>>>> + perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL; >>>>> perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; >>>>> perf->core_clk_rate = core_perf->fix_core_clk_rate; >>>>> } else { >>>>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent) >>>>> &perf->fix_core_clk_rate); >>>>> debugfs_create_u32("fix_core_ib_vote", 0600, entry, >>>>> &perf->fix_core_ib_vote); >>>>> - debugfs_create_u64("fix_core_ab_vote", 0600, entry, >>>>> + debugfs_create_u32("fix_core_ab_vote", 0600, entry, >>>>> &perf->fix_core_ab_vote); >>>>> return 0; >>>>> 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 ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >>>>> @@ -51,7 +51,7 @@ struct dpu_core_perf { >>>>> u32 enable_bw_release; >>>>> u64 fix_core_clk_rate; >>>>> u32 fix_core_ib_vote; >>>>> - u64 fix_core_ab_vote; >>>>> + u32 fix_core_ab_vote; >>>>> }; >>>>> int dpu_core_perf_crtc_check(struct drm_crtc *crtc, >>>>> >>> >
On Tue, 14 Jan 2025 at 03:31, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/11/2025 5:08 AM, Dmitry Baryshkov wrote: > > On 11 January 2025 01:49:23 EET, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> On 1/9/2025 6:02 PM, Dmitry Baryshkov wrote: > >>> On Thu, Jan 09, 2025 at 05:40:23PM -0800, Abhinav Kumar wrote: > >>>> > >>>> > >>>> On 1/5/2025 7:07 PM, Dmitry Baryshkov wrote: > >>>>> The fix_core_ab_vote is an average bandwidth value, used for bandwidth > >>>>> overrides in several cases. However there is an internal inconsistency: > >>>>> fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined > >>>>> in Bps. > >>>>> > >>>>> Fix that by changing the type of the variable to u32 and using * 1000ULL > >>>>> multiplier when setting up the dpu_core_perf_params::bw_ctl value. > >>>>> > >>>> > >>>> Actually after looking at this, I have another question. > >>>> > >>>> How did you conclude that fix_core_ib_vote is in KBps? > >>>> > >>>> min_dram_ib is in KBps in the catalog but how is fix_core_ib_vote? > >>>> > >>>> It depends on the interpretation perhaps. If the debugfs was supposed to > >>>> operate under the expectation that the provided value will be pre-multiplied > >>>> by 1000 and given then that explains why it was not multiplied again. > >>>> > >>>> And I cross-checked some of the internal usages of the debugfs, the values > >>>> provided to it were in Bps and not KBps. > >>> > >>> Well... As you wrote min_dram_ib is in KBps. So, by comparing the next > >>> two lines, fix_core_ib_vote should also be in kBps, as there is no > >>> premultiplier: > >>> > >>> perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; > >>> [...] > >>> perf->max_per_pipe_ib = perf_cfg->min_dram_ib; > >>> > >>> And then, as a proof, perf->max_per_pipe_ib is passed to icc_set_bw() > >>> without any modifications: > >>> > >>> icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib); > >>> > >> > >> Understood max_per_pipe_ib. But then by the same logic, fix_core_ab_vote is always in Bps and not in KBps because bw_ctl is in Bps. > >> > >> Is it really a discrepancy that fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps because they are just following the units in which bw_ctl and max_per_pipe_ib were defined in resp. > > > > Yes. They come in pair, as a part of the user interface. If one is in Bps and another one in KBps, it is very easy to forget that and misinterpret them or to make a mistake while programming them. Not to mention that the threshold files, which are related to AB, are in KBps. > > > > In that case, the documentation for both needs to be updated as well as > it still says both are in bps not kbps. Ack, I missed it. > > * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2 > * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2 > */ > struct dpu_core_perf { > const struct dpu_perf_cfg *perf_cfg; > u64 core_clk_rate; > u64 max_core_clk_rate; > struct dpu_core_perf_tune perf_tune; > u32 enable_bw_release; > u64 fix_core_clk_rate; > u64 fix_core_ib_vote; > u64 fix_core_ab_vote; > }; > > >> > >>> > >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>>> --- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++-- > >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +- > >>>>> 2 files changed, 3 insertions(+), 3 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 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > >>>>> @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf, > >>>>> perf->max_per_pipe_ib = 0; > >>>>> perf->core_clk_rate = 0; > >>>>> } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) { > >>>>> - perf->bw_ctl = core_perf->fix_core_ab_vote; > >>>>> + perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL; > >>>>> perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; > >>>>> perf->core_clk_rate = core_perf->fix_core_clk_rate; > >>>>> } else { > >>>>> @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent) > >>>>> &perf->fix_core_clk_rate); > >>>>> debugfs_create_u32("fix_core_ib_vote", 0600, entry, > >>>>> &perf->fix_core_ib_vote); > >>>>> - debugfs_create_u64("fix_core_ab_vote", 0600, entry, > >>>>> + debugfs_create_u32("fix_core_ab_vote", 0600, entry, > >>>>> &perf->fix_core_ab_vote); > >>>>> return 0; > >>>>> 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 ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644 > >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > >>>>> @@ -51,7 +51,7 @@ struct dpu_core_perf { > >>>>> u32 enable_bw_release; > >>>>> u64 fix_core_clk_rate; > >>>>> u32 fix_core_ib_vote; > >>>>> - u64 fix_core_ab_vote; > >>>>> + u32 fix_core_ab_vote; > >>>>> }; > >>>>> int dpu_core_perf_crtc_check(struct drm_crtc *crtc, > >>>>> > >>> > >
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 7263ab63a692554cd51a7fd91bd6250330179240..7cabc8f26908cfd2dbbffebd7c70fc37d9159733 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c @@ -125,7 +125,7 @@ static void _dpu_core_perf_calc_crtc(const struct dpu_core_perf *core_perf, perf->max_per_pipe_ib = 0; perf->core_clk_rate = 0; } else if (core_perf->perf_tune.mode == DPU_PERF_MODE_FIXED) { - perf->bw_ctl = core_perf->fix_core_ab_vote; + perf->bw_ctl = core_perf->fix_core_ab_vote * 1000ULL; perf->max_per_pipe_ib = core_perf->fix_core_ib_vote; perf->core_clk_rate = core_perf->fix_core_clk_rate; } else { @@ -479,7 +479,7 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent) &perf->fix_core_clk_rate); debugfs_create_u32("fix_core_ib_vote", 0600, entry, &perf->fix_core_ib_vote); - debugfs_create_u64("fix_core_ab_vote", 0600, entry, + debugfs_create_u32("fix_core_ab_vote", 0600, entry, &perf->fix_core_ab_vote); return 0; 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 ca4595b4ec217697849af02446b23ed0857a0295..5e07119c14c6a9ed3413d0eaddbd93df5cc3f79d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h @@ -51,7 +51,7 @@ struct dpu_core_perf { u32 enable_bw_release; u64 fix_core_clk_rate; u32 fix_core_ib_vote; - u64 fix_core_ab_vote; + u32 fix_core_ab_vote; }; int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
The fix_core_ab_vote is an average bandwidth value, used for bandwidth overrides in several cases. However there is an internal inconsistency: fix_core_ib_vote is defined in KBps, while fix_core_ab_vote is defined in Bps. Fix that by changing the type of the variable to u32 and using * 1000ULL multiplier when setting up the dpu_core_perf_params::bw_ctl value. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 4 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)