| Message ID | 20240513021607.129111-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: tc358767: Enable FRMSYNC timing generator | expand |
On Mon, May 13, 2024 at 4:16 AM Marek Vasut <marex@denx.de> 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@denx.de> > --- > Cc: Adam Ford <aford173@gmail.com> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: David Airlie <airlied@gmail.com> > Cc: Frieder Schrempf <frieder.schrempf@kontron.de> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Michael Walle <mwalle@kernel.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Robert Foss <rfoss@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > Cc: kernel@dh-electronics.com > --- > 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@kernel.org>
On Mon, 13 May 2024 04:16:04 +0200, Marek Vasut 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. > > [...] Applied, thanks! [1/1] drm/bridge: tc358767: Enable FRMSYNC timing generator https://cgit.freedesktop.org/drm/drm-misc/commit/?id=d9ca4b760ef6 Rob
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); +} + 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;
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@denx.de> --- Cc: Adam Ford <aford173@gmail.com> Cc: Alexander Stein <alexander.stein@ew.tq-group.com> Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Frieder Schrempf <frieder.schrempf@kontron.de> Cc: Jernej Skrabec <jernej.skrabec@gmail.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Michael Walle <mwalle@kernel.org> Cc: Neil Armstrong <neil.armstrong@linaro.org> Cc: Robert Foss <rfoss@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: dri-devel@lists.freedesktop.org Cc: kernel@dh-electronics.com --- drivers/gpu/drm/bridge/tc358767.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)