On 2023-05-04 22:33:17, Marijn Suijten wrote:
> Title suggestion: use the wording "reduce pclk rate" :)
>
> (Eventually "when DSC is enabled", instead of "for compression")
>
> On 2023-05-02 18:19:12, Jessica Zhang wrote:
> > Divide the pclk rate by the compression ratio when DSC is enabled
> >
> > Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
>
> Thank you so much for sending this. The compression ratio was applied
> to hdisplay
In hindsight, on the note of hdisplay, dsi_timing_setup() actually only
divides the visual portion - that is hdisplay out of htotal - without
affecting the back and front porch.
Since this clock inside the mode is based on the full htotal * vtotal *
..., should we compensate for that and only divide the visual portion of
the clock signal by 3? Otherwise we might not have enough clockticks to
perform the front and back porch (even though CMD mode doesn't really
have porches, I have yet to properly understand that part of the
signal).
- Marijn
> , but not the clocks yet, and with this patch I get a massive
> reduction in clock speeds on the Xperia XZ3, without regressions nor
> affecting performance/fps:
>
> gcc_sys_noc_cpuss_ahb_clk 1 1 0 19200000 0 0 50000 Y
> gcc_cpuss_ahb_clk 1 1 0 19200000 0 0 50000 Y
> bi_tcxo 6 6 0 19200000 0 0 50000 Y
> dsi0vco_clk 1 1 0 [-1873793994-]{+1249195898+} 0 0 50000 Y
> dsi0_pll_out_div_clk 1 1 0 [-1873793994-]{+624597949+} 0 0 50000 Y
> dsi0_pll_post_out_div_clk 0 0 0 [-468448498-]{+156149487+} 0 0 50000 Y
> dsi0_pll_bit_clk 2 2 0 [-1873793994-]{+624597949+} 0 0 50000 Y
> dsi0_pclk_mux 1 1 0 [-1873793994-]{+624597949+} 0 0 50000 Y
> dsi0_phy_pll_out_dsiclk 1 1 0 [-312298999-]{+104099659+} 0 0 50000 Y
> disp_cc_mdss_pclk0_clk_src 1 1 0 [-312298999-]{+104099659+} 0 0 50000 Y
> disp_cc_mdss_pclk0_clk 1 1 0 [-312298999-]{+104099659+} 0 0 50000 Y
> dsi0_pll_by_2_bit_clk 0 0 0 [-936896997-]{+312298974+} 0 0 50000 Y
> dsi0_phy_pll_out_byteclk 1 1 0 [-234224249-]{+78074743+} 0 0 50000 Y
> disp_cc_mdss_byte0_clk_src 2 2 0 [-234224249-]{+78074743+} 0 0 50000 Y
> disp_cc_mdss_byte0_div_clk_src 1 1 0 [-117112125-]{+39037372+} 0 0 50000 Y
> disp_cc_mdss_byte0_intf_clk 1 1 0 [-117112125-]{+39037372+} 0 0 50000 Y
> disp_cc_mdss_byte0_clk 1 1 0 [-234224249-]{+78074743+} 0 0 50000 Y
> gpu_cc_pll1 0 0 0 500000097 0 0 50000 N
> disp_cc_mdss_dp_pixel_clk_src 0 0 0 19200000 0 0 50000 N
> disp_cc_mdss_dp_pixel_clk 0 0 0 19200000 0 0 50000 N
>
> Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
>
> > ---
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 43a5ec33eee8..35c69dbe5f6f 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -561,7 +561,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
> > clk_disable_unprepare(msm_host->byte_clk);
> > }
> >
> > -static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
> > +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode,
>
> It is a bit unfortunate that this function is called so often with the
> same parameters, doing the same calculation over and over.
>
> > + struct drm_dsc_config *dsc, bool is_bonded_dsi)
> > {
> > unsigned long pclk_rate;
> >
> > @@ -576,6 +577,11 @@ static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool
> > if (is_bonded_dsi)
> > pclk_rate /= 2;
> >
> > + /* If DSC is enabled, divide pclk by compression ratio */
> > + if (dsc)
> > + pclk_rate = DIV_ROUND_UP(pclk_rate,
> > + dsc->bits_per_component * 3 / msm_dsc_get_bpp_int(dsc));
>
> Don't forget to mention that this series depends on the DSC helpers. I
> don't think the linked DSC 1.2 series depends on it (at least it doesn't
> mention it):
>
> https://lore.kernel.org/linux-arm-msm/20230329-rfc-msm-dsc-helper-v6-2-cb7f59f0f7fb@xxxxxxxxxxx/
>
> - Marijn
>
> > +
> > return pclk_rate;
> > }
> >
> > @@ -585,7 +591,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
> > struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> > u8 lanes = msm_host->lanes;
> > u32 bpp = dsi_get_bpp(msm_host->format);
> > - unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
> > + unsigned long pclk_rate = dsi_get_pclk_rate(mode, msm_host->dsc, is_bonded_dsi);
> > u64 pclk_bpp = (u64)pclk_rate * bpp;
> >
> > if (lanes == 0) {
> > @@ -604,7 +610,7 @@ unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_d
> >
> > static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > {
> > - msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
> > + msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi);
> > msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
> > msm_host->mode);
> >
> > @@ -634,7 +640,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >
> > dsi_calc_pclk(msm_host, is_bonded_dsi);
> >
> > - pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
> > + pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, msm_host->dsc, is_bonded_dsi) * bpp;
> > do_div(pclk_bpp, 8);
> > msm_host->src_clk_rate = pclk_bpp;
> >
> >
> > --
> > 2.40.1
> >
