| Message ID | 20240531204130.277800-3-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] drm/bridge: tc358767: Split tc_pxl_pll_en() into parameter calculation and enablement | expand |
Hi Marek, Am Freitag, 31. Mai 2024, 22:39:49 CEST schrieb Marek Vasut: > This line_pixel_subtract is no longer needed now that the bridge can > request and obtain specific pixel clock on input to the bridge, with > clock frequency that matches the Pixel PLL frequency. > > The line_pixel_subtract is now always 0, so drop it entirely. > > The line_pixel_subtract was not reliable as it never worked when the > Pixel PLL and input clock were off just so that the required amount > of pixels to subtract would not be whole integer. I think this is based on [1], no? I was wondering because it was not stated. Best regards, Alexander [1] https://lore.kernel.org/all/20240514004759.230431-1-marex@denx.de/ > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: David Airlie <airlied@gmail.com> > 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: 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 | 16 +--------------- > 1 file changed, 1 insertion(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 252cc08dcc4a8..f16728256991a 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -382,9 +382,6 @@ 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) > @@ -661,11 +658,7 @@ static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, u32 pixelclock, > return -EINVAL; > } > > - tc->line_pixel_subtract = tc->mode.htotal - > - DIV_ROUND_UP(tc->mode.htotal * (u64)best_pixelclock, (u64)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: got %d, delta %d\n", best_pixelclock, best_delta); > dev_dbg(tc->dev, "PLL: %d / %d / %d * %d / %d\n", refclk, > ext_div[best_pre], best_div, best_mul, ext_div[best_post]); > > @@ -909,13 +902,6 @@ 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 > * datasheet is not clear of vsdelay in case of DPI >
On 6/3/24 2:18 PM, Alexander Stein wrote: > Hi Marek, Hi, > Am Freitag, 31. Mai 2024, 22:39:49 CEST schrieb Marek Vasut: >> This line_pixel_subtract is no longer needed now that the bridge can >> request and obtain specific pixel clock on input to the bridge, with >> clock frequency that matches the Pixel PLL frequency. >> >> The line_pixel_subtract is now always 0, so drop it entirely. >> >> The line_pixel_subtract was not reliable as it never worked when the >> Pixel PLL and input clock were off just so that the required amount >> of pixels to subtract would not be whole integer. > > I think this is based on [1], no? It is. > I was wondering because it was not stated. I thought [1] was already applied, but it seems it was only RBd. I can either apply [1] and then add this on top, so the two commits can be reverted separately if this one breaks something, or squash [1] into this patch and send V2. Which one do you prefer ? The [1] fixes a real nasty issue with DPI output which causes visible image corruption, so I would like to have [1] in, but it is obviously not perfect.
Hi Marek, Am Montag, 3. Juni 2024, 23:25:43 CEST schrieb Marek Vasut: > On 6/3/24 2:18 PM, Alexander Stein wrote: > > Hi Marek, > > Hi, > > > Am Freitag, 31. Mai 2024, 22:39:49 CEST schrieb Marek Vasut: > >> This line_pixel_subtract is no longer needed now that the bridge can > >> request and obtain specific pixel clock on input to the bridge, with > >> clock frequency that matches the Pixel PLL frequency. > >> > >> The line_pixel_subtract is now always 0, so drop it entirely. > >> > >> The line_pixel_subtract was not reliable as it never worked when the > >> Pixel PLL and input clock were off just so that the required amount > >> of pixels to subtract would not be whole integer. > > > > I think this is based on [1], no? > > It is. Thanks for confirmation. > > I was wondering because it was not stated. > > I thought [1] was already applied, but it seems it was only RBd. > > I can either apply [1] and then add this on top, so the two commits can > be reverted separately if this one breaks something, or squash [1] into > this patch and send V2. Which one do you prefer ? > > The [1] fixes a real nasty issue with DPI output which causes visible > image corruption, so I would like to have [1] in, but it is obviously > not perfect. I can't use DPI anyway, but if [1] actually fixes something for DPI I'm okay with having [1] in, which gets mostly reverted in this series. But that's up to the maintainers. Best regards, Alexander
On 6/4/24 1:12 PM, Alexander Stein wrote: > Hi Marek, Hi, > Am Montag, 3. Juni 2024, 23:25:43 CEST schrieb Marek Vasut: >> On 6/3/24 2:18 PM, Alexander Stein wrote: >>> Hi Marek, >> >> Hi, >> >>> Am Freitag, 31. Mai 2024, 22:39:49 CEST schrieb Marek Vasut: >>>> This line_pixel_subtract is no longer needed now that the bridge can >>>> request and obtain specific pixel clock on input to the bridge, with >>>> clock frequency that matches the Pixel PLL frequency. >>>> >>>> The line_pixel_subtract is now always 0, so drop it entirely. >>>> >>>> The line_pixel_subtract was not reliable as it never worked when the >>>> Pixel PLL and input clock were off just so that the required amount >>>> of pixels to subtract would not be whole integer. >>> >>> I think this is based on [1], no? >> >> It is. > > Thanks for confirmation. > >>> I was wondering because it was not stated. >> >> I thought [1] was already applied, but it seems it was only RBd. >> >> I can either apply [1] and then add this on top, so the two commits can >> be reverted separately if this one breaks something, or squash [1] into >> this patch and send V2. Which one do you prefer ? >> >> The [1] fixes a real nasty issue with DPI output which causes visible >> image corruption, so I would like to have [1] in, but it is obviously >> not perfect. > > I can't use DPI anyway, but if [1] actually fixes something for DPI > I'm okay with having [1] in, which gets mostly reverted in this series. > But that's up to the maintainers. It is the FRMSYNC enablement (as suggested by the TC9595 datasheet) which fixes the issue. This just makes FRMSYNC work well with other modes.
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 252cc08dcc4a8..f16728256991a 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -382,9 +382,6 @@ 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) @@ -661,11 +658,7 @@ static int tc_pxl_pll_calc(struct tc_data *tc, u32 refclk, u32 pixelclock, return -EINVAL; } - tc->line_pixel_subtract = tc->mode.htotal - - DIV_ROUND_UP(tc->mode.htotal * (u64)best_pixelclock, (u64)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: got %d, delta %d\n", best_pixelclock, best_delta); dev_dbg(tc->dev, "PLL: %d / %d / %d * %d / %d\n", refclk, ext_div[best_pre], best_div, best_mul, ext_div[best_post]); @@ -909,13 +902,6 @@ 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 * datasheet is not clear of vsdelay in case of DPI
This line_pixel_subtract is no longer needed now that the bridge can request and obtain specific pixel clock on input to the bridge, with clock frequency that matches the Pixel PLL frequency. The line_pixel_subtract is now always 0, so drop it entirely. The line_pixel_subtract was not reliable as it never worked when the Pixel PLL and input clock were off just so that the required amount of pixels to subtract would not be whole integer. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: David Airlie <airlied@gmail.com> 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: 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 | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)