Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression
On 5/4/2023 2:17 PM, Marijn Suijten wrote: 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 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). Hi Marijn, That's a fair point. Will change the pclk math accordingly. Thanks, Jessica Zhang - 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 1101920 0 0 5 Y gcc_cpuss_ahb_clk 1101920 0 0 5 Y bi_tcxo 6601920 0 0 5 Y dsi0vco_clk110 [-1873793994-]{+1249195898+} 0 0 5 Y dsi0_pll_out_div_clk110 [-1873793994-]{+624597949+} 0 0 5 Y dsi0_pll_post_out_div_clk 000 [-468448498-]{+156149487+} 0 0 5 Y dsi0_pll_bit_clk 220 [-1873793994-]{+624597949+} 0 0 5 Y dsi0_pclk_mux 110 [-1873793994-]{+624597949+} 0 0 5 Y dsi0_phy_pll_out_dsiclk 110 [-312298999-]{+104099659+} 0 0 5 Y disp_cc_mdss_pclk0_clk_src 110 [-312298999-]{+104099659+} 0 0 5 Y disp_cc_mdss_pclk0_clk 110 [-312298999-]{+104099659+} 0 0 5 Y dsi0_pll_by_2_bit_clk 000 [-936896997-]{+312298974+} 0 0 5 Y dsi0_phy_pll_out_byteclk 110 [-234224249-]{+78074743+} 0 0 5 Y disp_cc_mdss_byte0_clk_src 220 [-234224249-]{+78074743+} 0 0 5 Y disp_cc_mdss_byte0_div_clk_src 110 [-117112125-]{+39037372+} 0 0 5 Y disp_cc_mdss_byte0_intf_clk 110 [-117112125-]{+39037372+} 0 0 5 Y disp_cc_mdss_byte0_clk 110 [-234224249-]{+78074743+} 0 0 5 Y gpu_cc_pll1000 50097 0 0 5 N disp_cc_mdss_dp_pixel_clk_src 0001920 0 0 5 N disp_cc_mdss_dp_pixel_clk 0001920 0 0 5 N Reviewed-by: Marijn Suijten --- 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, +
Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression
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 > > 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 1101920 > 0 0 5 Y > gcc_cpuss_ahb_clk 1101920 > 0 0 5 Y > bi_tcxo 6601920 > 0 0 5 Y >dsi0vco_clk110 > [-1873793994-]{+1249195898+} 0 0 5 Y > dsi0_pll_out_div_clk110 > [-1873793994-]{+624597949+} 0 0 5 Y > dsi0_pll_post_out_div_clk 000 > [-468448498-]{+156149487+} 0 0 5 Y > dsi0_pll_bit_clk 220 > [-1873793994-]{+624597949+} 0 0 5 Y > dsi0_pclk_mux 110 > [-1873793994-]{+624597949+} 0 0 5 Y >dsi0_phy_pll_out_dsiclk 110 > [-312298999-]{+104099659+} 0 0 5 Y > disp_cc_mdss_pclk0_clk_src 110 > [-312298999-]{+104099659+} 0 0 5 Y > disp_cc_mdss_pclk0_clk 110 > [-312298999-]{+104099659+} 0 0 5 Y > dsi0_pll_by_2_bit_clk 000 > [-936896997-]{+312298974+} 0 0 5 Y > dsi0_phy_pll_out_byteclk 110 > [-234224249-]{+78074743+} 0 0 5 Y >disp_cc_mdss_byte0_clk_src 220 > [-234224249-]{+78074743+} 0 0 5 Y > disp_cc_mdss_byte0_div_clk_src 11 > 0[-117112125-]{+39037372+} 0 0 5 Y > disp_cc_mdss_byte0_intf_clk 11 > 0[-117112125-]{+39037372+} 0 0 5 Y > disp_cc_mdss_byte0_clk 110 > [-234224249-]{+78074743+} 0 0 5 Y >gpu_cc_pll1000 50097 > 0 0 5 N >disp_cc_mdss_dp_pixel_clk_src 0001920 > 0 0 5 N > disp_cc_mdss_dp_pixel_clk 0001920 > 0 0 5 N > > Reviewed-by: Marijn Suijten > > > --- > > 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(pcl
Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression
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 Thank you so much for sending this. The compression ratio was applied to hdisplay, 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 1101920 0 0 5 Y gcc_cpuss_ahb_clk 1101920 0 0 5 Y bi_tcxo 6601920 0 0 5 Y dsi0vco_clk110 [-1873793994-]{+1249195898+} 0 0 5 Y dsi0_pll_out_div_clk110 [-1873793994-]{+624597949+} 0 0 5 Y dsi0_pll_post_out_div_clk 000 [-468448498-]{+156149487+} 0 0 5 Y dsi0_pll_bit_clk 220 [-1873793994-]{+624597949+} 0 0 5 Y dsi0_pclk_mux 110 [-1873793994-]{+624597949+} 0 0 5 Y dsi0_phy_pll_out_dsiclk 110 [-312298999-]{+104099659+} 0 0 5 Y disp_cc_mdss_pclk0_clk_src 110 [-312298999-]{+104099659+} 0 0 5 Y disp_cc_mdss_pclk0_clk 110 [-312298999-]{+104099659+} 0 0 5 Y dsi0_pll_by_2_bit_clk 000 [-936896997-]{+312298974+} 0 0 5 Y dsi0_phy_pll_out_byteclk 110 [-234224249-]{+78074743+} 0 0 5 Y disp_cc_mdss_byte0_clk_src 220 [-234224249-]{+78074743+} 0 0 5 Y disp_cc_mdss_byte0_div_clk_src 110 [-117112125-]{+39037372+} 0 0 5 Y disp_cc_mdss_byte0_intf_clk 110 [-117112125-]{+39037372+} 0 0 5 Y disp_cc_mdss_byte0_clk 110 [-234224249-]{+78074743+} 0 0 5 Y gpu_cc_pll1000 50097 0 0 5 N disp_cc_mdss_dp_pixel_clk_src 0001920 0 0 5 N disp_cc_mdss_dp_pixel_clk 0001920 0 0 5 N Reviewed-by: Marijn Suijten > --- > 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-cb7f59f0f...@quicinc.com/ - 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
Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression
On 5/3/2023 1:33 AM, Dmitry Baryshkov wrote: On 03/05/2023 04:19, Jessica Zhang wrote: Divide the pclk rate by the compression ratio when DSC is enabled Signed-off-by: Jessica Zhang --- 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, + 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 we loose precision here? Would DIV_ROUND_UP(pclk_rate * bpp, dsc->bpc * 3) be better? Hi Dmitry, Acked. Thanks, Jessica Zhang 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; -- With best wishes Dmitry
Re: [PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression
On 03/05/2023 04:19, Jessica Zhang wrote: Divide the pclk rate by the compression ratio when DSC is enabled Signed-off-by: Jessica Zhang --- 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, + 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 we loose precision here? Would DIV_ROUND_UP(pclk_rate * bpp, dsc->bpc * 3) be better? 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; -- With best wishes Dmitry
[PATCH 1/4] drm/msm/dsi: Adjust pclk rate for compression
Divide the pclk rate by the compression ratio when DSC is enabled Signed-off-by: Jessica Zhang --- 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, + 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)); + 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