Hi, On 2023-03-20 at 17:16:36 +0100, Roman Beranek <romanberanek@xxxxxxxxxx> wrote: > In the case of DSI output, the value of SUN4I_TCON0_DCLK_DIV (4) does > not represent the actual dotclock divider, PLL_MIPI instead runs at > (bpp / lanes )-multiple [1] of the dotclock. [2] Setting 4 as dotclock > divder thus leads to reduced frame rate, specifically by 1/3 on 4-lane > panels, and by 2/3 on 2-lane panels respectively. > > As sun4i_dotclock driver stores its calculated divider directly in > the register, conditional handling of the DSI output scenario is needed. > Instead of reading the divider from SUN4I_TCON0_DCLK_REG, retrieve > the value from tcon->dclk_min_div. > > [1] bits per pixel / number of DSI lanes > [2] <https://github.com/BPI-SINOVOIP/BPI-M64-bsp-4.4/blob/66bef0f2f30b367eb93b1cbad21ce85e0361f7ae/linux-sunxi/drivers/video/fbdev/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L322> > > Signed-off-by: Roman Beranek <romanberanek@xxxxxxxxxx> > — > drivers/gpu/drm/sun4i/sun4i_dotclock.c | 6 +++++- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 +++– > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff –git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c > index 417ade3d2565..26fa99aff590 100644 > — a/drivers/gpu/drm/sun4i/sun4i_dotclock.c > +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c > @@ -11,6 +11,7 @@ > > #include “sun4i_tcon.h” > #include “sun4i_dotclock.h” > +#include “sun6i_mipi_dsi.h” > > struct sun4i_dclk { > struct clk_hw hw; > @@ -56,6 +57,9 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_hw *hw, > struct sun4i_dclk *dclk = hw_to_dclk(hw); > u32 val; > > + if (dclk->tcon->is_dsi) > + return parent_rate / dclk->tcon->dclk_min_div; > + > regmap_read(dclk->regmap, SUN4I_TCON0_DCLK_REG, &val); > > val >>= SUN4I_TCON0_DCLK_DIV_SHIFT; > @@ -116,7 +120,7 @@ static int sun4i_dclk_set_rate(struct clk_hw *hw, unsigned long rate, > unsigned long parent_rate) > { > struct sun4i_dclk *dclk = hw_to_dclk(hw); > - u8 div = parent_rate / rate; > + u8 div = dclk->tcon->is_dsi ? SUN6I_DSI_TCON_DIV : parent_rate / rate; > > return regmap_update_bits(dclk->regmap, SUN4I_TCON0_DCLK_REG, > GENMASK(6, 0), div); > diff –git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c > index 523a6d787921..7f5d3c135058 100644 > — a/drivers/gpu/drm/sun4i/sun4i_tcon.c > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > @@ -367,8 +367,9 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon, > u32 block_space, start_delay; > u32 tcon_div; > > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV; > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV; > + tcon->is_dsi = true; > + tcon->dclk_min_div = bpp / lanes; > + tcon->dclk_max_div = bpp / lanes; Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@xxxxxxxxxxxx/> Actually, I had the following third patch prepared that adjusted the dotclock rate so that the required PLL rate is set. But again, this seems very indirect, so that’s why I refrained from submitting it and I submitted the linked patch above instead. Anyway, here is the third proposal: — a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder) regulator_disable(dsi->regulator); } +static bool sun6i_dsi_encoder_mode_fixup( ⁃ struct drm_encoder *encoder, ⁃ const struct drm_display_mode *mode, ⁃ struct drm_display_mode *adjusted_mode) +{ ⁃ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) { ⁃ /* ⁃ * For DSI the PLL rate has to respect the bits per pixel and ⁃ * number of lanes. ⁃ * ⁃ * According to the BSP code: ⁃ * PLL rate = DOTCLOCK * bpp / lanes ⁃ * ⁃ * Therefore, the clock has to be adjusted in order to set the ⁃ * correct PLL rate when actually setting the clock. ⁃ */ ⁃ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder); ⁃ struct mipi_dsi_device *device = dsi->device; ⁃ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format); ⁃ u8 lanes = device->lanes; ⁃ ⁃ adjusted_mode->crtc_clock = mode->crtc_clock ⁃ * bpp / (lanes * SUN6I_DSI_TCON_DIV); ⁃ } ⁃ ⁃ return true; +} ⁃ static int sun6i_dsi_get_modes(struct drm_connector *connector) { struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector); @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = { static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = { .disable = sun6i_dsi_encoder_disable, .enable = sun6i_dsi_encoder_enable, ⁃ .mode_fixup = sun6i_dsi_encoder_mode_fixup, }; static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi, Maxime, Roman, CC, what do you think? Can we achieve consensus? If I’m not mistaken, all of the three proposal are a step in the right direction, because they correct faulty behavior. Don’t you think? Thanks, Frank > > sun4i_tcon0_mode_set_common(tcon, mode); > > diff –git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h > index fa23aa23fe4a..d8150ba2f319 100644 > — a/drivers/gpu/drm/sun4i/sun4i_tcon.h > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h > @@ -271,6 +271,7 @@ struct sun4i_tcon { > struct clk *dclk; > u8 dclk_max_div; > u8 dclk_min_div; > + bool is_dsi; > > /* Reset control */ > struct reset_control *lcd_rst;
