| Message ID | 20230320161636.24411-1-romanberanek@icloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG | expand |
Hi, On Mon, Mar 20, 2023 at 05:16:36PM +0100, Roman Beranek 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@icloud.com> This is similar to https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/ What's the story there? Also, how was it tested/confirmed? Maxime
Hello Maxime, On Tue Mar 21, 2023 at 3:56 PM CET, Maxime Ripard wrote: > > This is similar to > https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/ > > What's the story there? Yes, Frank Oltmanns wrote me recently in relation to a patch I wrote ~ 3 years ago that addressed the framerate issue, proposing to collaborate on pushing it upstream, however as I've been keeping up with my inbox rather sporadically these days, by the time I read his message, Frank had already taken the initiative and sent the patch. So that's how we've got to this slightly awkward situation with two patches on the same subject arriving 1 day apart of each other. The problem with the original patch was that it went around sun4i_dotclock by feeding it a rate adjusted such that the pll-mipi rate was set correctly. I couldn't quite figure out at the time of how big a portion of the tcon logic does the sun4i_dotclock code need to be made aware of. >Also, how was it tested/confirmed? By counting Vblank interrupts (GIC 118). Roman
On Tue Mar 21, 2023 at 5:50 PM CET, Roman Beranek wrote: > > Also, how was it tested/confirmed? > > By counting Vblank interrupts (GIC 118). Sorry, that was perhaps too abbreviated. To test this change, I set up an A64 board running kmscube on DSI-1 and verified that the rate of Vblank IRQs tracked with a video mode set on DSI-1, once with a 2-lane panel and once with a 4-lane panel. Roman
Hi, On 2023-03-20 at 17:16:36 +0100, Roman Beranek <romanberanek@icloud.com> 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@icloud.com> > — > 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@oltmanns.dev/> 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;
Hi, On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote: > On 2023-03-20 at 17:16:36 +0100, Roman Beranek <romanberanek@icloud.com> 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@icloud.com> > > — > > 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@oltmanns.dev/> Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it works with his testing, I'll be happy to merge it. > 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, > }; It's not clear to me what this patch is supposed to be doing, there's no mode_fixup implementation upstream? Maxime
On Tue, Mar 21, 2023 at 09:53:11PM +0100, Roman Beranek wrote: > On Tue Mar 21, 2023 at 5:50 PM CET, Roman Beranek wrote: > > > > Also, how was it tested/confirmed? > > > > By counting Vblank interrupts (GIC 118). > > Sorry, that was perhaps too abbreviated. To test this change, I set up > an A64 board running kmscube on DSI-1 and verified that the rate of > Vblank IRQs tracked with a video mode set on DSI-1, once with a 2-lane > panel and once with a 4-lane panel. Ok, as long as you confirmed it with multiple panel and multiple lanes count, I'm ok with it :) Maxime
On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote: > > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote: > > 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@oltmanns.dev/> > > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it > works with his testing, I'll be happy to merge it. > So I've just found out that my understanding of what sun4i_dotclock is was wrong the whole time. I treated it as a virtual clock representing the true CRTC pixel clock and only coincidentally also matching what A64 Reference Manual labels as TCON0 data clock (a coincidence to which DSI is an exception). Now that I finally see dotclock as 'what could dclk be an abbreviation to', I to agree that it's not only straightforward but also correct to keep the divider at 4 and adjust the rate as is done it the patch Frank submitted. In order to preserve semantic correctness however, I propose to preface the change with a patch that renames sun4i_dotclock and tcon-pixel-clock such that dot/pixel is replaced with d/data. What do you think? Roman
Hi, On 2023-03-27 at 22:20:45 +0200, Maxime Ripard <maxime@cerno.tech> wrote: > Hi, > > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote: [...] >> 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, >> }; > > It's not clear to me what this patch is supposed to be doing, there's no mode_fixup implementation > upstream? > Sorry, my mail client tried some fancy formatting. :( This is the patch again. --- 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, I still like the original patch better, but I'd be happy to submit this as a proper patch, if this is more to your liking. Thanks, Frank > Maxime > --
Hi, On Tue, Mar 28, 2023 at 09:28:19PM +0200, Frank Oltmanns wrote: > --- 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) So, mode_fixup is kind of deprecated in favour of atomic_check > +{ > + 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); And that's visible to the userspace, so it's not where we should store that value. I guess the best way to do something similar would be to store it into crtc_state, and then reuse it there. But it starts to make a lot of rather complicated code compared to your previous patch. Maxime
On Tue, Mar 28, 2023 at 01:48:33AM +0200, Roman Beranek wrote: > On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote: > > > > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote: > > > 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@oltmanns.dev/> > > > > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it > > works with his testing, I'll be happy to merge it. > > > > So I've just found out that my understanding of what sun4i_dotclock is > was wrong the whole time. I treated it as a virtual clock representing > the true CRTC pixel clock and only coincidentally also matching what > A64 Reference Manual labels as TCON0 data clock (a coincidence to which > DSI is an exception). > > Now that I finally see dotclock as 'what could dclk be an abbreviation > to', I to agree that it's not only straightforward but also correct to > keep the divider at 4 and adjust the rate as is done it the patch Frank > submitted. > > In order to preserve semantic correctness however, I propose to preface > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock > such that dot/pixel is replaced with d/data. What do you think? I don't think it's exposed to the userspace in any way so it makes sense to me Maxime
Hi, On 2023-03-29 at 21:56:39 +0200, Maxime Ripard <maxime@cerno.tech> wrote: > Hi, > > On Tue, Mar 28, 2023 at 09:28:19PM +0200, Frank Oltmanns wrote: >> --- 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) > > So, mode_fixup is kind of deprecated in favour of atomic_check I see. Thanks for pointing that out. >> +{ >> + 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); > > And that's visible to the userspace, so it's not where we should store > that value. I guess the best way to do something similar would be to > store it into crtc_state, and then reuse it there. But it starts to make > a lot of rather complicated code compared to your previous patch. Ah, interesting. But I agree, let's stick to the simpler aproach. Thanks, Frank > > Maxime > > [[End of PGP Signed Part]] --
Hi Roman, On 2023-03-29 at 21:58:02 +0200, Maxime Ripard <maxime@cerno.tech> wrote: > On Tue, Mar 28, 2023 at 01:48:33AM +0200, Roman Beranek wrote: >> On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote: >> > >> > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote: >> > > 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@oltmanns.dev/> >> > >> > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it >> > works with his testing, I'll be happy to merge it. >> > >> >> So I've just found out that my understanding of what sun4i_dotclock is >> was wrong the whole time. I treated it as a virtual clock representing >> the true CRTC pixel clock and only coincidentally also matching what >> A64 Reference Manual labels as TCON0 data clock (a coincidence to which >> DSI is an exception). >> >> Now that I finally see dotclock as 'what could dclk be an abbreviation >> to', I to agree that it's not only straightforward but also correct to >> keep the divider at 4 and adjust the rate as is done it the patch Frank >> submitted. >> >> In order to preserve semantic correctness however, I propose to preface >> the change with a patch that renames sun4i_dotclock and tcon-pixel-clock >> such that dot/pixel is replaced with d/data. What do you think? > > I don't think it's exposed to the userspace in any way so it makes sense to me Roman, will you please submit a V2 of the patch I submitted then? Or do you want me to do it? Thanks, Frank > > Maxime > --
Hello Frank, On Thu Mar 30, 2023 at 6:45 AM CEST, Frank Oltmanns wrote: > Roman, will you please submit a V2 of the patch I submitted then? Or do > you want me to do it? Yes, I'm already on it, only missing a cover letter. Roman
Hello Maxime, On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote: > > In order to preserve semantic correctness however, I propose to preface > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock > > such that dot/pixel is replaced with d/data. What do you think? > > I don't think it's exposed to the userspace in any way so it makes sense to me > Here's a new series that includes those renames: <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/> It turns out however that the new dclk rates can't be set exactly as requested without touching pll-video0*, tcon0 now therefore gets reparented from pll-mipi to pll-video0-2x which, as it further turns out, breaks DSI. While simply forbidding the video0-2x mux option seems to me as the right way to go because there's not much use for it with non-DSI interfaces either besides the opportunity to power pll-mipi down, I'd like to run by you first. Kind regards, Roman * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0 retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)? The driver actually asks for 297 MHz, clock_set_pll3 rounds it to 294 MHz though because it limits itself to 6 MHz steps.
On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote: > Hello Maxime, > > On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote: > > > In order to preserve semantic correctness however, I propose to preface > > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock > > > such that dot/pixel is replaced with d/data. What do you think? > > > > I don't think it's exposed to the userspace in any way so it makes sense to me > > > > Here's a new series that includes those renames: > <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/> > > It turns out however that the new dclk rates can't be set exactly as > requested without touching pll-video0*, tcon0 now therefore gets > reparented from pll-mipi to pll-video0-2x which, as it further turns > out, breaks DSI. While simply forbidding the video0-2x mux option seems > to me as the right way to go because there's not much use for it with > non-DSI interfaces either besides the opportunity to power pll-mipi > down, I'd like to run by you first. Sounds reasonable > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0 > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)? > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to > 294 MHz though because it limits itself to 6 MHz steps. We could also address that though Maxime
Dne sreda, 05. april 2023 ob 14:34:11 CEST je Roman Beranek napisal(a): > Hello Maxime, > > On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote: > > > In order to preserve semantic correctness however, I propose to preface > > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock > > > such that dot/pixel is replaced with d/data. What do you think? > > > > I don't think it's exposed to the userspace in any way so it makes sense > > to me > Here's a new series that includes those renames: > <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/> > > It turns out however that the new dclk rates can't be set exactly as > requested without touching pll-video0*, tcon0 now therefore gets > reparented from pll-mipi to pll-video0-2x which, as it further turns > out, breaks DSI. While simply forbidding the video0-2x mux option seems > to me as the right way to go because there's not much use for it with > non-DSI interfaces either besides the opportunity to power pll-mipi > down, I'd like to run by you first. It's been a long time since I looked at A64 HDMI clocks, but IIRC, pll-video0 is the only useful source for HDMI PHY (as opposed to HDMI controller.) So question remains how to properly support both displays at the same time. Have you ever tried to make HDMI and DSI work at the same time? This is one of issues of the PinePhone IIUC. > > Kind regards, > Roman > > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0 > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)? > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to > 294 MHz though because it limits itself to 6 MHz steps. Yeah, we added CLK_SET_RATE_PARENT flag to several clocks after initial driver was merged. Adding this flag sounds completely reasonable. Best regards, Jernej
On Sat Apr 8, 2023 at 9:07 AM CEST, Jernej Škrabec wrote: > Dne sreda, 05. april 2023 ob 14:34:11 CEST je Roman Beranek napisal(a): > > While simply forbidding the video0-2x mux option seems > > to me as the right way to go because there's not much use for it with > > non-DSI interfaces either besides the opportunity to power pll-mipi > > down, I'd like to run by you first. > > It's been a long time since I looked at A64 HDMI clocks, but IIRC, pll-video0 > is the only useful source for HDMI PHY (as opposed to HDMI controller.) > So question remains how to properly support both displays at the same time. > Correct. > Have you ever tried to make HDMI and DSI work at the same time? This is one of > issues of the PinePhone IIUC. > Yes, I have. Prusa3D's SL1 printer, on which I previously worked on, uses both outputs simultaneously. I had encountered the same reparenting problem back then but since I hadn't been able to identify it, I resorted to fiddling with the DSI pixelclock until it worked. DSI & HDMI co-existence is yet another reasoni though for forbidding the pll-video-2x parent. megi's kernel includes Mr. Zheng's commit which does the same. <https://github.com/megous/linux/commit/7374d57> Best wishes Roman Beranek
On Wed Apr 5, 2023 at 5:03 PM CEST, Maxime Ripard wrote: > On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote: > > It turns out however that the new dclk rates can't be set exactly as > > requested without touching pll-video0*, tcon0 now therefore gets > > reparented from pll-mipi to pll-video0-2x which, as it further turns > > out, breaks DSI. While simply forbidding the video0-2x mux option seems > > to me as the right way to go because there's not much use for it with > > non-DSI interfaces either besides the opportunity to power pll-mipi > > down, I'd like to run by you first. > > Sounds reasonable Okay, I'm unsure of how to denote that in the code however. Should I just comment the parent out of the table and put an explanation in a comment nearby? Or just erase it? I couldn't find an applicable precedent. > > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0 > > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver > > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)? > > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to > > 294 MHz though because it limits itself to 6 MHz steps. > > We could also address that though Should I include it in v2 of the series, or leave it for later? Thanks, Roman
On Wed, Apr 12, 2023 at 09:14:59AM +0200, Roman Beranek wrote: > On Wed Apr 5, 2023 at 5:03 PM CEST, Maxime Ripard wrote: > > On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote: > > > It turns out however that the new dclk rates can't be set exactly as > > > requested without touching pll-video0*, tcon0 now therefore gets > > > reparented from pll-mipi to pll-video0-2x which, as it further turns > > > out, breaks DSI. While simply forbidding the video0-2x mux option seems > > > to me as the right way to go because there's not much use for it with > > > non-DSI interfaces either besides the opportunity to power pll-mipi > > > down, I'd like to run by you first. > > > > Sounds reasonable > > Okay, I'm unsure of how to denote that in the code however. Should I > just comment the parent out of the table and put an explanation in > a comment nearby? Or just erase it? I couldn't find an applicable > precedent. I think that forcing the parent at boot, and adding the CLK_SET_RATE_NOREPARENT flag should be enough. > > > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0 > > > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver > > > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)? > > > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to > > > 294 MHz though because it limits itself to 6 MHz steps. > > > > We could also address that though > > Should I include it in v2 of the series, or leave it for later? I guess you can include it into this one too Maxime
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; 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;
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@icloud.com> --- 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(-)