| Message ID | 20241212-filter-mode-clock-v1-1-f4441988d6aa@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm/dpu: Filter modes based on adjusted mode clock | expand |
On Thu, Dec 12, 2024 at 11:11:54AM -0800, Jessica Zhang wrote: > Filter out modes that have a clock rate greater than the max core clock > rate when adjusted for the perf clock factor > > This is especially important for chipsets such as QCS615 that have lower > limits for the MDP max core clock. > > Since the core CRTC clock is at least the mode clock (adjusted for the > perf clock factor) [1], the modes supported by the driver should be less > than the max core clock rate. > > [1] https://elixir.bootlin.com/linux/v6.12.4/source/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c#L83 > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 +++++++++++++++++++-------- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 3 +++ > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++++++++++ > 3 files changed, 36 insertions(+), 8 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 6f0a37f954fe8797a4e3a34e7876a93d5e477642..0afd7c81981c722a1a9176062250c418255fe6d0 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > @@ -31,6 +31,26 @@ enum dpu_perf_mode { > DPU_PERF_MODE_MAX > }; > > +/** > + * dpu_core_perf_adjusted_crtc_clk - Adjust given crtc clock rate according to Nit: CRTC (here and further) > + * the perf clock factor. > + * @crtc_clk_rate - Unadjusted crtc clock rate > + * @perf_cfg: performance configuration > + */ > +u64 dpu_core_perf_adjusted_crtc_clk(u64 crtc_clk_rate, > + const struct dpu_perf_cfg *perf_cfg) It's not just the CRTC clocks > +{ > + u32 clk_factor; > + > + clk_factor = perf_cfg->clk_inefficiency_factor; > + if (clk_factor) { > + crtc_clk_rate *= clk_factor; > + do_div(crtc_clk_rate, 100); > + } > + > + return crtc_clk_rate; > +} > + > /** > * _dpu_core_perf_calc_bw() - to calculate BW per crtc > * @perf_cfg: performance configuration > @@ -76,7 +96,6 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, > struct dpu_plane_state *pstate; > struct drm_display_mode *mode; > u64 crtc_clk; While you are at it, could you please also add a patch, replacing height * vidth * vrefresh with mode->clock * 1000? The former one has limited precision. > - u32 clk_factor; > > mode = &state->adjusted_mode; > > @@ -90,13 +109,7 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, > crtc_clk = max(pstate->plane_clk, crtc_clk); > } This function calculates crtc_clk as max(plane_clk, crtc_clk). Shouldn't we also reject the atomic_state if for any of the planes the corrected clock is lower than max_core_clk_rate > > - clk_factor = perf_cfg->clk_inefficiency_factor; > - if (clk_factor) { > - crtc_clk *= clk_factor; > - do_div(crtc_clk, 100); > - } > - > - return crtc_clk; > + return dpu_core_perf_adjusted_crtc_clk(crtc_clk, perf_cfg); > } > > static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) > 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 451bf8021114d9d4a2dfdbb81ed4150fc559c681..c3bcd567cdfb66647c83682d1feedd69e33f0680 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > @@ -54,6 +54,9 @@ struct dpu_core_perf { > u64 fix_core_ab_vote; > }; > > +u64 dpu_core_perf_adjusted_crtc_clk(u64 clk_rate, > + const struct dpu_perf_cfg *perf_cfg); > + > int dpu_core_perf_crtc_check(struct drm_crtc *crtc, > struct drm_crtc_state *state); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index ad3462476a143ec01a3b8817a2c85b0f50435a9e..cd7b84ab57a7526948c2beb7c5cefdddcbe4f6d9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1257,6 +1257,7 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, > const struct drm_display_mode *mode) > { > struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > + u64 adjusted_mode_clk; > > /* if there is no 3d_mux block we cannot merge LMs so we cannot > * split the large layer into 2 LMs, filter out such modes > @@ -1264,6 +1265,17 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, > if (!dpu_kms->catalog->caps->has_3d_merge && > mode->hdisplay > dpu_kms->catalog->caps->max_mixer_width) > return MODE_BAD_HVALUE; > + > + adjusted_mode_clk = dpu_core_perf_adjusted_crtc_clk(mode->clock, > + dpu_kms->perf.perf_cfg); > + > + /* > + * The given mode, adjusted for the perf clock factor, should not exceed > + * the max core clock rate > + */ > + if (adjusted_mode_clk > dpu_kms->perf.max_core_clk_rate / 1000) > + return MODE_CLOCK_HIGH; > + > /* > * max crtc width is equal to the max mixer width * 2 and max height is 4K > */ > > --- > base-commit: 423c1c96d6b2d3bb35072e33a5fdd8db6d2c0a74 > change-id: 20241212-filter-mode-clock-8cb2e769f05b > > Best regards, > -- > Jessica Zhang <quic_jesszhan@quicinc.com> >
On 12/12/2024 5:05 PM, Dmitry Baryshkov wrote: > On Thu, Dec 12, 2024 at 11:11:54AM -0800, Jessica Zhang wrote: >> Filter out modes that have a clock rate greater than the max core clock >> rate when adjusted for the perf clock factor >> >> This is especially important for chipsets such as QCS615 that have lower >> limits for the MDP max core clock. >> >> Since the core CRTC clock is at least the mode clock (adjusted for the >> perf clock factor) [1], the modes supported by the driver should be less >> than the max core clock rate. >> >> [1] https://elixir.bootlin.com/linux/v6.12.4/source/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c#L83 >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 +++++++++++++++++++-------- >> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 3 +++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++++++++++ >> 3 files changed, 36 insertions(+), 8 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 6f0a37f954fe8797a4e3a34e7876a93d5e477642..0afd7c81981c722a1a9176062250c418255fe6d0 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >> @@ -31,6 +31,26 @@ enum dpu_perf_mode { >> DPU_PERF_MODE_MAX >> }; >> >> +/** >> + * dpu_core_perf_adjusted_crtc_clk - Adjust given crtc clock rate according to > > Nit: CRTC (here and further) > >> + * the perf clock factor. >> + * @crtc_clk_rate - Unadjusted crtc clock rate >> + * @perf_cfg: performance configuration >> + */ >> +u64 dpu_core_perf_adjusted_crtc_clk(u64 crtc_clk_rate, >> + const struct dpu_perf_cfg *perf_cfg) > > It's not just the CRTC clocks > Do you mean we should use adjusted mode clock here? >> +{ >> + u32 clk_factor; >> + >> + clk_factor = perf_cfg->clk_inefficiency_factor; >> + if (clk_factor) { >> + crtc_clk_rate *= clk_factor; >> + do_div(crtc_clk_rate, 100); >> + } >> + >> + return crtc_clk_rate; >> +} >> + >> /** >> * _dpu_core_perf_calc_bw() - to calculate BW per crtc >> * @perf_cfg: performance configuration >> @@ -76,7 +96,6 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, >> struct dpu_plane_state *pstate; >> struct drm_display_mode *mode; >> u64 crtc_clk; > > While you are at it, could you please also add a patch, replacing height > * vidth * vrefresh with mode->clock * 1000? The former one has limited > precision. > >> - u32 clk_factor; >> >> mode = &state->adjusted_mode; >> >> @@ -90,13 +109,7 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, >> crtc_clk = max(pstate->plane_clk, crtc_clk); >> } > > This function calculates crtc_clk as max(plane_clk, crtc_clk). Shouldn't > we also reject the atomic_state if for any of the planes the corrected > clock is lower than max_core_clk_rate > You mean higher than max_core_clk_rate? If so, yes we can fix that up. >> >> - clk_factor = perf_cfg->clk_inefficiency_factor; >> - if (clk_factor) { >> - crtc_clk *= clk_factor; >> - do_div(crtc_clk, 100); >> - } >> - >> - return crtc_clk; >> + return dpu_core_perf_adjusted_crtc_clk(crtc_clk, perf_cfg); >> } >> >> static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) >> 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 451bf8021114d9d4a2dfdbb81ed4150fc559c681..c3bcd567cdfb66647c83682d1feedd69e33f0680 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >> @@ -54,6 +54,9 @@ struct dpu_core_perf { >> u64 fix_core_ab_vote; >> }; >> >> +u64 dpu_core_perf_adjusted_crtc_clk(u64 clk_rate, >> + const struct dpu_perf_cfg *perf_cfg); >> + >> int dpu_core_perf_crtc_check(struct drm_crtc *crtc, >> struct drm_crtc_state *state); >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> index ad3462476a143ec01a3b8817a2c85b0f50435a9e..cd7b84ab57a7526948c2beb7c5cefdddcbe4f6d9 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> @@ -1257,6 +1257,7 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, >> const struct drm_display_mode *mode) >> { >> struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); >> + u64 adjusted_mode_clk; >> >> /* if there is no 3d_mux block we cannot merge LMs so we cannot >> * split the large layer into 2 LMs, filter out such modes >> @@ -1264,6 +1265,17 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, >> if (!dpu_kms->catalog->caps->has_3d_merge && >> mode->hdisplay > dpu_kms->catalog->caps->max_mixer_width) >> return MODE_BAD_HVALUE; >> + >> + adjusted_mode_clk = dpu_core_perf_adjusted_crtc_clk(mode->clock, >> + dpu_kms->perf.perf_cfg); >> + >> + /* >> + * The given mode, adjusted for the perf clock factor, should not exceed >> + * the max core clock rate >> + */ >> + if (adjusted_mode_clk > dpu_kms->perf.max_core_clk_rate / 1000) >> + return MODE_CLOCK_HIGH; >> + >> /* >> * max crtc width is equal to the max mixer width * 2 and max height is 4K >> */ >> >> --- >> base-commit: 423c1c96d6b2d3bb35072e33a5fdd8db6d2c0a74 >> change-id: 20241212-filter-mode-clock-8cb2e769f05b >> >> Best regards, >> -- >> Jessica Zhang <quic_jesszhan@quicinc.com> >> >
On Fri, 13 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 12/12/2024 5:05 PM, Dmitry Baryshkov wrote: > > On Thu, Dec 12, 2024 at 11:11:54AM -0800, Jessica Zhang wrote: > >> Filter out modes that have a clock rate greater than the max core clock > >> rate when adjusted for the perf clock factor > >> > >> This is especially important for chipsets such as QCS615 that have lower > >> limits for the MDP max core clock. > >> > >> Since the core CRTC clock is at least the mode clock (adjusted for the > >> perf clock factor) [1], the modes supported by the driver should be less > >> than the max core clock rate. > >> > >> [1] https://elixir.bootlin.com/linux/v6.12.4/source/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c#L83 > >> > >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 +++++++++++++++++++-------- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 3 +++ > >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++++++++++ > >> 3 files changed, 36 insertions(+), 8 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 6f0a37f954fe8797a4e3a34e7876a93d5e477642..0afd7c81981c722a1a9176062250c418255fe6d0 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > >> @@ -31,6 +31,26 @@ enum dpu_perf_mode { > >> DPU_PERF_MODE_MAX > >> }; > >> > >> +/** > >> + * dpu_core_perf_adjusted_crtc_clk - Adjust given crtc clock rate according to > > > > Nit: CRTC (here and further) > > > >> + * the perf clock factor. > >> + * @crtc_clk_rate - Unadjusted crtc clock rate > >> + * @perf_cfg: performance configuration > >> + */ > >> +u64 dpu_core_perf_adjusted_crtc_clk(u64 crtc_clk_rate, > >> + const struct dpu_perf_cfg *perf_cfg) > > > > It's not just the CRTC clocks > > > > Do you mean we should use adjusted mode clock here? This also applies, etc. But my point was that you can not name it just "adjusted CRTC clock" if you also add the plane clocks handling. > > >> +{ > >> + u32 clk_factor; > >> + > >> + clk_factor = perf_cfg->clk_inefficiency_factor; > >> + if (clk_factor) { > >> + crtc_clk_rate *= clk_factor; > >> + do_div(crtc_clk_rate, 100); > >> + } > >> + > >> + return crtc_clk_rate; > >> +} > >> + > >> /** > >> * _dpu_core_perf_calc_bw() - to calculate BW per crtc > >> * @perf_cfg: performance configuration > >> @@ -76,7 +96,6 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, > >> struct dpu_plane_state *pstate; > >> struct drm_display_mode *mode; > >> u64 crtc_clk; > > > > While you are at it, could you please also add a patch, replacing height > > * vidth * vrefresh with mode->clock * 1000? The former one has limited > > precision. > > > >> - u32 clk_factor; > >> > >> mode = &state->adjusted_mode; > >> > >> @@ -90,13 +109,7 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, > >> crtc_clk = max(pstate->plane_clk, crtc_clk); > >> } > > > > This function calculates crtc_clk as max(plane_clk, crtc_clk). Shouldn't > > we also reject the atomic_state if for any of the planes the corrected > > clock is lower than max_core_clk_rate > > > > You mean higher than max_core_clk_rate? If so, yes we can fix that up. Yes > > >> > >> - clk_factor = perf_cfg->clk_inefficiency_factor; > >> - if (clk_factor) { > >> - crtc_clk *= clk_factor; > >> - do_div(crtc_clk, 100); > >> - } > >> - > >> - return crtc_clk; > >> + return dpu_core_perf_adjusted_crtc_clk(crtc_clk, perf_cfg); > >> } > >> > >> static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) > >> 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 451bf8021114d9d4a2dfdbb81ed4150fc559c681..c3bcd567cdfb66647c83682d1feedd69e33f0680 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > >> @@ -54,6 +54,9 @@ struct dpu_core_perf { > >> u64 fix_core_ab_vote; > >> }; > >> > >> +u64 dpu_core_perf_adjusted_crtc_clk(u64 clk_rate, > >> + const struct dpu_perf_cfg *perf_cfg); > >> + > >> int dpu_core_perf_crtc_check(struct drm_crtc *crtc, > >> struct drm_crtc_state *state); > >> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >> index ad3462476a143ec01a3b8817a2c85b0f50435a9e..cd7b84ab57a7526948c2beb7c5cefdddcbe4f6d9 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >> @@ -1257,6 +1257,7 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, > >> const struct drm_display_mode *mode) > >> { > >> struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > >> + u64 adjusted_mode_clk; > >> > >> /* if there is no 3d_mux block we cannot merge LMs so we cannot > >> * split the large layer into 2 LMs, filter out such modes > >> @@ -1264,6 +1265,17 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, > >> if (!dpu_kms->catalog->caps->has_3d_merge && > >> mode->hdisplay > dpu_kms->catalog->caps->max_mixer_width) > >> return MODE_BAD_HVALUE; > >> + > >> + adjusted_mode_clk = dpu_core_perf_adjusted_crtc_clk(mode->clock, > >> + dpu_kms->perf.perf_cfg); > >> + > >> + /* > >> + * The given mode, adjusted for the perf clock factor, should not exceed > >> + * the max core clock rate > >> + */ > >> + if (adjusted_mode_clk > dpu_kms->perf.max_core_clk_rate / 1000) > >> + return MODE_CLOCK_HIGH; > >> + > >> /* > >> * max crtc width is equal to the max mixer width * 2 and max height is 4K > >> */ > >> > >> --- > >> base-commit: 423c1c96d6b2d3bb35072e33a5fdd8db6d2c0a74 > >> change-id: 20241212-filter-mode-clock-8cb2e769f05b > >> > >> Best regards, > >> -- > >> Jessica Zhang <quic_jesszhan@quicinc.com> > >> > >
On 12/13/2024 12:38 PM, Dmitry Baryshkov wrote: > On Fri, 13 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 12/12/2024 5:05 PM, Dmitry Baryshkov wrote: >>> On Thu, Dec 12, 2024 at 11:11:54AM -0800, Jessica Zhang wrote: >>>> Filter out modes that have a clock rate greater than the max core clock >>>> rate when adjusted for the perf clock factor >>>> >>>> This is especially important for chipsets such as QCS615 that have lower >>>> limits for the MDP max core clock. >>>> >>>> Since the core CRTC clock is at least the mode clock (adjusted for the >>>> perf clock factor) [1], the modes supported by the driver should be less >>>> than the max core clock rate. >>>> >>>> [1] https://elixir.bootlin.com/linux/v6.12.4/source/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c#L83 >>>> >>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 +++++++++++++++++++-------- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 3 +++ >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++++++++++ >>>> 3 files changed, 36 insertions(+), 8 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 6f0a37f954fe8797a4e3a34e7876a93d5e477642..0afd7c81981c722a1a9176062250c418255fe6d0 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c >>>> @@ -31,6 +31,26 @@ enum dpu_perf_mode { >>>> DPU_PERF_MODE_MAX >>>> }; >>>> >>>> +/** >>>> + * dpu_core_perf_adjusted_crtc_clk - Adjust given crtc clock rate according to >>> >>> Nit: CRTC (here and further) >>> >>>> + * the perf clock factor. >>>> + * @crtc_clk_rate - Unadjusted crtc clock rate >>>> + * @perf_cfg: performance configuration >>>> + */ >>>> +u64 dpu_core_perf_adjusted_crtc_clk(u64 crtc_clk_rate, >>>> + const struct dpu_perf_cfg *perf_cfg) >>> >>> It's not just the CRTC clocks >>> >> >> Do you mean we should use adjusted mode clock here? > > This also applies, etc. But my point was that you can not name it just > "adjusted CRTC clock" if you also add the plane clocks handling. > _dpu_plane_calc_clk() already handles the plane_clk calculation so we dont need to add it here. adjusted_mode_clk sounds fine to me in that case. >> >>>> +{ >>>> + u32 clk_factor; >>>> + >>>> + clk_factor = perf_cfg->clk_inefficiency_factor; >>>> + if (clk_factor) { >>>> + crtc_clk_rate *= clk_factor; >>>> + do_div(crtc_clk_rate, 100); >>>> + } >>>> + >>>> + return crtc_clk_rate; >>>> +} >>>> + >>>> /** >>>> * _dpu_core_perf_calc_bw() - to calculate BW per crtc >>>> * @perf_cfg: performance configuration >>>> @@ -76,7 +96,6 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, >>>> struct dpu_plane_state *pstate; >>>> struct drm_display_mode *mode; >>>> u64 crtc_clk; >>> >>> While you are at it, could you please also add a patch, replacing height >>> * vidth * vrefresh with mode->clock * 1000? The former one has limited >>> precision. >>> >>>> - u32 clk_factor; >>>> >>>> mode = &state->adjusted_mode; >>>> >>>> @@ -90,13 +109,7 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, >>>> crtc_clk = max(pstate->plane_clk, crtc_clk); >>>> } >>> >>> This function calculates crtc_clk as max(plane_clk, crtc_clk). Shouldn't >>> we also reject the atomic_state if for any of the planes the corrected >>> clock is lower than max_core_clk_rate >>> >> >> You mean higher than max_core_clk_rate? If so, yes we can fix that up. > > Yes > I cross-checked the src code, we do already have the protection for plane_clk going beyond max_core_clk /* max clk check */ if (_dpu_plane_calc_clk(mode, pipe_cfg) > kms->perf.max_core_clk_rate) { DPU_DEBUG_PLANE(pdpu, "plane exceeds max mdp core clk limits\n"); return -E2BIG; } So this should be sufficient for the case you are referring to. >> >>>> >>>> - clk_factor = perf_cfg->clk_inefficiency_factor; >>>> - if (clk_factor) { >>>> - crtc_clk *= clk_factor; >>>> - do_div(crtc_clk, 100); >>>> - } >>>> - >>>> - return crtc_clk; >>>> + return dpu_core_perf_adjusted_crtc_clk(crtc_clk, perf_cfg); >>>> } >>>> >>>> static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) >>>> 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 451bf8021114d9d4a2dfdbb81ed4150fc559c681..c3bcd567cdfb66647c83682d1feedd69e33f0680 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h >>>> @@ -54,6 +54,9 @@ struct dpu_core_perf { >>>> u64 fix_core_ab_vote; >>>> }; >>>> >>>> +u64 dpu_core_perf_adjusted_crtc_clk(u64 clk_rate, >>>> + const struct dpu_perf_cfg *perf_cfg); >>>> + >>>> int dpu_core_perf_crtc_check(struct drm_crtc *crtc, >>>> struct drm_crtc_state *state); >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> index ad3462476a143ec01a3b8817a2c85b0f50435a9e..cd7b84ab57a7526948c2beb7c5cefdddcbe4f6d9 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>>> @@ -1257,6 +1257,7 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, >>>> const struct drm_display_mode *mode) >>>> { >>>> struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); >>>> + u64 adjusted_mode_clk; >>>> >>>> /* if there is no 3d_mux block we cannot merge LMs so we cannot >>>> * split the large layer into 2 LMs, filter out such modes >>>> @@ -1264,6 +1265,17 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, >>>> if (!dpu_kms->catalog->caps->has_3d_merge && >>>> mode->hdisplay > dpu_kms->catalog->caps->max_mixer_width) >>>> return MODE_BAD_HVALUE; >>>> + >>>> + adjusted_mode_clk = dpu_core_perf_adjusted_crtc_clk(mode->clock, >>>> + dpu_kms->perf.perf_cfg); >>>> + >>>> + /* >>>> + * The given mode, adjusted for the perf clock factor, should not exceed >>>> + * the max core clock rate >>>> + */ >>>> + if (adjusted_mode_clk > dpu_kms->perf.max_core_clk_rate / 1000) >>>> + return MODE_CLOCK_HIGH; >>>> + >>>> /* >>>> * max crtc width is equal to the max mixer width * 2 and max height is 4K >>>> */ >>>> >>>> --- >>>> base-commit: 423c1c96d6b2d3bb35072e33a5fdd8db6d2c0a74 >>>> change-id: 20241212-filter-mode-clock-8cb2e769f05b >>>> >>>> Best regards, >>>> -- >>>> Jessica Zhang <quic_jesszhan@quicinc.com> >>>> >>> > > >
On Mon, Dec 16, 2024 at 11:16:11AM -0800, Abhinav Kumar wrote: > > > On 12/13/2024 12:38 PM, Dmitry Baryshkov wrote: > > On Fri, 13 Dec 2024 at 21:15, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > > > > > > > On 12/12/2024 5:05 PM, Dmitry Baryshkov wrote: > > > > On Thu, Dec 12, 2024 at 11:11:54AM -0800, Jessica Zhang wrote: > > > > > Filter out modes that have a clock rate greater than the max core clock > > > > > rate when adjusted for the perf clock factor > > > > > > > > > > This is especially important for chipsets such as QCS615 that have lower > > > > > limits for the MDP max core clock. > > > > > > > > > > Since the core CRTC clock is at least the mode clock (adjusted for the > > > > > perf clock factor) [1], the modes supported by the driver should be less > > > > > than the max core clock rate. > > > > > > > > > > [1] https://elixir.bootlin.com/linux/v6.12.4/source/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c#L83 > > > > > > > > > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > > > > > --- > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 +++++++++++++++++++-------- > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 3 +++ > > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++++++++++ > > > > > 3 files changed, 36 insertions(+), 8 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 6f0a37f954fe8797a4e3a34e7876a93d5e477642..0afd7c81981c722a1a9176062250c418255fe6d0 100644 > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c > > > > > @@ -31,6 +31,26 @@ enum dpu_perf_mode { > > > > > DPU_PERF_MODE_MAX > > > > > }; > > > > > > > > > > +/** > > > > > + * dpu_core_perf_adjusted_crtc_clk - Adjust given crtc clock rate according to > > > > > > > > Nit: CRTC (here and further) > > > > > > > > > + * the perf clock factor. > > > > > + * @crtc_clk_rate - Unadjusted crtc clock rate > > > > > + * @perf_cfg: performance configuration > > > > > + */ > > > > > +u64 dpu_core_perf_adjusted_crtc_clk(u64 crtc_clk_rate, > > > > > + const struct dpu_perf_cfg *perf_cfg) > > > > > > > > It's not just the CRTC clocks > > > > > > > > > > Do you mean we should use adjusted mode clock here? > > > > This also applies, etc. But my point was that you can not name it just > > "adjusted CRTC clock" if you also add the plane clocks handling. > > > > _dpu_plane_calc_clk() already handles the plane_clk calculation so we dont > need to add it here. > > adjusted_mode_clk sounds fine to me in that case. > > > > > > > > > +{ > > > > > + u32 clk_factor; > > > > > + > > > > > + clk_factor = perf_cfg->clk_inefficiency_factor; > > > > > + if (clk_factor) { > > > > > + crtc_clk_rate *= clk_factor; > > > > > + do_div(crtc_clk_rate, 100); > > > > > + } > > > > > + > > > > > + return crtc_clk_rate; > > > > > +} > > > > > + > > > > > /** > > > > > * _dpu_core_perf_calc_bw() - to calculate BW per crtc > > > > > * @perf_cfg: performance configuration > > > > > @@ -76,7 +96,6 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, > > > > > struct dpu_plane_state *pstate; > > > > > struct drm_display_mode *mode; > > > > > u64 crtc_clk; > > > > > > > > While you are at it, could you please also add a patch, replacing height > > > > * vidth * vrefresh with mode->clock * 1000? The former one has limited > > > > precision. > > > > > > > > > - u32 clk_factor; > > > > > > > > > > mode = &state->adjusted_mode; > > > > > > > > > > @@ -90,13 +109,7 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, > > > > > crtc_clk = max(pstate->plane_clk, crtc_clk); > > > > > } > > > > > > > > This function calculates crtc_clk as max(plane_clk, crtc_clk). Shouldn't > > > > we also reject the atomic_state if for any of the planes the corrected > > > > clock is lower than max_core_clk_rate > > > > > > > > > > You mean higher than max_core_clk_rate? If so, yes we can fix that up. > > > > Yes > > > > I cross-checked the src code, we do already have the protection for > plane_clk going beyond max_core_clk > > /* max clk check */ > if (_dpu_plane_calc_clk(mode, pipe_cfg) > > kms->perf.max_core_clk_rate) { > DPU_DEBUG_PLANE(pdpu, "plane exceeds max mdp core clk > limits\n"); > return -E2BIG; > } > > So this should be sufficient for the case you are referring to. But the quoted piece of code doesn't take the 'inefficiency' into account. But it is probably a separate issue to be fixed. For this patch: Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > > > > > > > > - clk_factor = perf_cfg->clk_inefficiency_factor; > > > > > - if (clk_factor) { > > > > > - crtc_clk *= clk_factor; > > > > > - do_div(crtc_clk, 100); > > > > > - } > > > > > - > > > > > - return crtc_clk; > > > > > + return dpu_core_perf_adjusted_crtc_clk(crtc_clk, perf_cfg); > > > > > } > > > > > > > > > > static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) > > > > > 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 451bf8021114d9d4a2dfdbb81ed4150fc559c681..c3bcd567cdfb66647c83682d1feedd69e33f0680 100644 > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h > > > > > @@ -54,6 +54,9 @@ struct dpu_core_perf { > > > > > u64 fix_core_ab_vote; > > > > > }; > > > > > > > > > > +u64 dpu_core_perf_adjusted_crtc_clk(u64 clk_rate, > > > > > + const struct dpu_perf_cfg *perf_cfg); > > > > > + > > > > > int dpu_core_perf_crtc_check(struct drm_crtc *crtc, > > > > > struct drm_crtc_state *state); > > > > > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > > index ad3462476a143ec01a3b8817a2c85b0f50435a9e..cd7b84ab57a7526948c2beb7c5cefdddcbe4f6d9 100644 > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > > @@ -1257,6 +1257,7 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, > > > > > const struct drm_display_mode *mode) > > > > > { > > > > > struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > > > > > + u64 adjusted_mode_clk; > > > > > > > > > > /* if there is no 3d_mux block we cannot merge LMs so we cannot > > > > > * split the large layer into 2 LMs, filter out such modes > > > > > @@ -1264,6 +1265,17 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, > > > > > if (!dpu_kms->catalog->caps->has_3d_merge && > > > > > mode->hdisplay > dpu_kms->catalog->caps->max_mixer_width) > > > > > return MODE_BAD_HVALUE; > > > > > + > > > > > + adjusted_mode_clk = dpu_core_perf_adjusted_crtc_clk(mode->clock, > > > > > + dpu_kms->perf.perf_cfg); > > > > > + > > > > > + /* > > > > > + * The given mode, adjusted for the perf clock factor, should not exceed > > > > > + * the max core clock rate > > > > > + */ > > > > > + if (adjusted_mode_clk > dpu_kms->perf.max_core_clk_rate / 1000) > > > > > + return MODE_CLOCK_HIGH; > > > > > + > > > > > /* > > > > > * max crtc width is equal to the max mixer width * 2 and max height is 4K > > > > > */ > > > > > > > > > > --- > > > > > base-commit: 423c1c96d6b2d3bb35072e33a5fdd8db6d2c0a74 > > > > > change-id: 20241212-filter-mode-clock-8cb2e769f05b > > > > > > > > > > Best regards, > > > > > -- > > > > > Jessica Zhang <quic_jesszhan@quicinc.com> > > > > > > > > > > > > > > >
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 6f0a37f954fe8797a4e3a34e7876a93d5e477642..0afd7c81981c722a1a9176062250c418255fe6d0 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c @@ -31,6 +31,26 @@ enum dpu_perf_mode { DPU_PERF_MODE_MAX }; +/** + * dpu_core_perf_adjusted_crtc_clk - Adjust given crtc clock rate according to + * the perf clock factor. + * @crtc_clk_rate - Unadjusted crtc clock rate + * @perf_cfg: performance configuration + */ +u64 dpu_core_perf_adjusted_crtc_clk(u64 crtc_clk_rate, + const struct dpu_perf_cfg *perf_cfg) +{ + u32 clk_factor; + + clk_factor = perf_cfg->clk_inefficiency_factor; + if (clk_factor) { + crtc_clk_rate *= clk_factor; + do_div(crtc_clk_rate, 100); + } + + return crtc_clk_rate; +} + /** * _dpu_core_perf_calc_bw() - to calculate BW per crtc * @perf_cfg: performance configuration @@ -76,7 +96,6 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, struct dpu_plane_state *pstate; struct drm_display_mode *mode; u64 crtc_clk; - u32 clk_factor; mode = &state->adjusted_mode; @@ -90,13 +109,7 @@ static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg, crtc_clk = max(pstate->plane_clk, crtc_clk); } - clk_factor = perf_cfg->clk_inefficiency_factor; - if (clk_factor) { - crtc_clk *= clk_factor; - do_div(crtc_clk, 100); - } - - return crtc_clk; + return dpu_core_perf_adjusted_crtc_clk(crtc_clk, perf_cfg); } static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) 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 451bf8021114d9d4a2dfdbb81ed4150fc559c681..c3bcd567cdfb66647c83682d1feedd69e33f0680 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h @@ -54,6 +54,9 @@ struct dpu_core_perf { u64 fix_core_ab_vote; }; +u64 dpu_core_perf_adjusted_crtc_clk(u64 clk_rate, + const struct dpu_perf_cfg *perf_cfg); + int dpu_core_perf_crtc_check(struct drm_crtc *crtc, struct drm_crtc_state *state); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index ad3462476a143ec01a3b8817a2c85b0f50435a9e..cd7b84ab57a7526948c2beb7c5cefdddcbe4f6d9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1257,6 +1257,7 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) { struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); + u64 adjusted_mode_clk; /* if there is no 3d_mux block we cannot merge LMs so we cannot * split the large layer into 2 LMs, filter out such modes @@ -1264,6 +1265,17 @@ static enum drm_mode_status dpu_crtc_mode_valid(struct drm_crtc *crtc, if (!dpu_kms->catalog->caps->has_3d_merge && mode->hdisplay > dpu_kms->catalog->caps->max_mixer_width) return MODE_BAD_HVALUE; + + adjusted_mode_clk = dpu_core_perf_adjusted_crtc_clk(mode->clock, + dpu_kms->perf.perf_cfg); + + /* + * The given mode, adjusted for the perf clock factor, should not exceed + * the max core clock rate + */ + if (adjusted_mode_clk > dpu_kms->perf.max_core_clk_rate / 1000) + return MODE_CLOCK_HIGH; + /* * max crtc width is equal to the max mixer width * 2 and max height is 4K */
Filter out modes that have a clock rate greater than the max core clock rate when adjusted for the perf clock factor This is especially important for chipsets such as QCS615 that have lower limits for the MDP max core clock. Since the core CRTC clock is at least the mode clock (adjusted for the perf clock factor) [1], the modes supported by the driver should be less than the max core clock rate. [1] https://elixir.bootlin.com/linux/v6.12.4/source/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c#L83 Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 +++++++++++++++++++-------- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 3 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 +++++++++++ 3 files changed, 36 insertions(+), 8 deletions(-) --- base-commit: 423c1c96d6b2d3bb35072e33a5fdd8db6d2c0a74 change-id: 20241212-filter-mode-clock-8cb2e769f05b Best regards,