On Mon, May 13, 2024 at 4:16 AM Marek Vasut <marex@xxxxxxx> wrote:
>
> TC9595 datasheet Video Path0 Control (VPCTRL0) Register bit FRMSYNC description
> says "This bit should be disabled only in video mode transmission where Host
> transmits video timing together with video data and where pixel clock source
> is from DSI clock." . This driver always sources pixel clock from external xtal,
> therefore the FRMSYNC bit must always be enabled, enable it.
>
> This fixes an actual issue with DSI-to-DPI mode, where the display would
> randomly show subtle pixel flickering, or wobble, or shimmering. This is
> visible on solid gray color, but the degree of the shimmering differs
> between boots, which makes it hard to debug.
>
> There is a caveat to the FRMSYNC and this bridge pixel PLL, which can only
> generate pixel clock with limited accuracy, it may therefore be necessary
> to reduce the HFP to fit into line length of input pixel data, to avoid any
> possible overflows, which make the output video look striped horizontally.
>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> ---
> Cc: Adam Ford <aford173@xxxxxxxxx>
> Cc: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxxx>
> Cc: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> Cc: Jonas Karlman <jonas@xxxxxxxxx>
> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Michael Walle <mwalle@xxxxxxxxxx>
> Cc: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> Cc: Robert Foss <rfoss@xxxxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: kernel@xxxxxxxxxxxxxxxxxx
> ---
> drivers/gpu/drm/bridge/tc358767.c | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 166f9a3e9622d..fe2b93546eaef 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -382,6 +382,9 @@ struct tc_data {
>
> /* HPD pin number (0 or 1) or -ENODEV */
> int hpd_pin;
> +
> + /* Number of pixels to subtract from a line due to pixel clock delta */
> + u32 line_pixel_subtract;
> };
>
> static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
> @@ -577,6 +580,11 @@ static int tc_pllupdate(struct tc_data *tc, unsigned int pllctrl)
> return 0;
> }
>
> +static u32 div64_round_up(u64 v, u32 d)
> +{
> + return div_u64(v + d - 1, d);
> +}
> +
Could the DIV_ROUND_UP macro in math,h replace this function?
> static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
> {
> int ret;
> @@ -658,8 +666,11 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
> return -EINVAL;
> }
>
> - dev_dbg(tc->dev, "PLL: got %d, delta %d\n", best_pixelclock,
> - best_delta);
> + tc->line_pixel_subtract = tc->mode.htotal -
> + div64_round_up(tc->mode.htotal * (u64)best_pixelclock, pixelclock);
> +
> + dev_dbg(tc->dev, "PLL: got %d, delta %d (subtract %d px)\n", best_pixelclock,
> + best_delta, tc->line_pixel_subtract);
> dev_dbg(tc->dev, "PLL: %d / %d / %d * %d / %d\n", refclk,
> ext_div[best_pre], best_div, best_mul, ext_div[best_post]);
>
> @@ -885,6 +896,12 @@ static int tc_set_common_video_mode(struct tc_data *tc,
> upper_margin, lower_margin, vsync_len);
> dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal);
>
> + if (right_margin > tc->line_pixel_subtract) {
> + right_margin -= tc->line_pixel_subtract;
> + } else {
> + dev_err(tc->dev, "Bridge pixel clock too slow for mode\n");
> + right_margin = 0;
> + }
>
> /*
> * LCD Ctl Frame Size
> @@ -894,7 +911,7 @@ static int tc_set_common_video_mode(struct tc_data *tc,
> */
> ret = regmap_write(tc->regmap, VPCTRL0,
> FIELD_PREP(VSDELAY, right_margin + 10) |
> - OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED);
> + OPXLFMT_RGB888 | FRMSYNC_ENABLED | MSF_DISABLED);
> if (ret)
> return ret;
>
> --
> 2.43.0
>
With the above question resolved, feel free to add my r-b. I will
snooze this for a week before looking at applying this.
Reviewed-by: Robert Foss <rfoss@xxxxxxxxxx>
