Hi Marek,
Thank you for the patch.
On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote:
> The Pixel PLL is not very capable and may come up with wildly inaccurate
> clock. Since DPI panels are often tolerant to slightly higher pixel clock
> without being operated outside of specification, calculate two Pixel PLL
> from either mode clock or display_timing .pixelclock.max , whichever is
> higher.
Maybe this is a leftover from v1 in the commit message, but I don't
think the code computes two pixel PLL.
> Since the Pixel PLL output clock frequency calculation always
> returns lower frequency than the requested clock frequency, passing in
> the higher clock frequency should result in output clock frequency which
> is closer to the expected pixel clock.
Is that guaranteed ?
> For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency
> without this patch is 65 MHz which is out of the panel specification of
> 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within
> the specification and far more accurate.
I'm a bit concerned that this patch is quite a bit of a hack, but fixing
the problem correctly would be too much yak shaving :-S
>
> Keep the change isolated to DPI output.
>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> ---
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxxx>
> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> Cc: Jonas Karlman <jonas@xxxxxxxxx>
> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> Cc: Robert Foss <rfoss@xxxxxxxxxx>
> Cc: Simona Vetter <simona@xxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> ---
> V2: - Isolate the change to DPI only, split tc_bridge_mode_set()
> - Look up display_timings and use .pixelclock.max as input
> into the PLL calculation if available. That should yield
> more accurate results for DPI panels.
> ---
> drivers/gpu/drm/bridge/tc358767.c | 47 +++++++++++++++++++++++++------
> 1 file changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 0d523322fdd8e..fe9ab06d82d91 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -39,6 +39,8 @@
> #include <drm/drm_print.h>
> #include <drm/drm_probe_helper.h>
>
> +#include <video/display_timing.h>
> +
> /* Registers */
>
> /* DSI D-PHY Layer registers */
> @@ -1681,13 +1683,33 @@ static int tc_dpi_atomic_check(struct drm_bridge *bridge,
> struct drm_crtc_state *crtc_state,
> struct drm_connector_state *conn_state)
> {
> + u32 mode_clock = crtc_state->mode.clock * 1000;
> struct tc_data *tc = bridge_to_tc(bridge);
> - int adjusted_clock = 0;
> + struct drm_bridge *nb = bridge;
> + struct display_timing timings;
> + struct drm_panel *panel;
> + int adjusted_clock;
> int ret;
>
> + do {
> + if (!drm_bridge_is_panel(nb))
drm_bridge_get_panel() already checks if the bridge is related to a
panel, so I think you can drop this check.
> + continue;
> +
> + panel = drm_bridge_get_panel(nb);
> + if (!panel || !panel->funcs || !panel->funcs->get_timings)
> + continue;
> +
> + ret = panel->funcs->get_timings(panel, 1, &timings);
> + if (ret <= 0)
> + break;
> +
> + if (timings.pixelclock.max > mode_clock)
> + mode_clock = timings.pixelclock.max;
> + break;
> + } while ((nb = drm_bridge_get_next_bridge(nb)));
Can the panel be anything that the last bridge in the chain ?
> +
> ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk),
> - crtc_state->mode.clock * 1000,
> - &adjusted_clock, NULL);
> + mode_clock, &adjusted_clock, NULL);
> if (ret)
> return ret;
>
> @@ -1758,9 +1780,18 @@ tc_edp_mode_valid(struct drm_bridge *bridge,
> return MODE_OK;
> }
>
> -static void tc_bridge_mode_set(struct drm_bridge *bridge,
> - const struct drm_display_mode *mode,
> - const struct drm_display_mode *adj)
> +static void tc_dpi_bridge_mode_set(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + const struct drm_display_mode *adj)
> +{
> + struct tc_data *tc = bridge_to_tc(bridge);
> +
> + drm_mode_copy(&tc->mode, adj);
> +}
> +
> +static void tc_edp_bridge_mode_set(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + const struct drm_display_mode *adj)
> {
> struct tc_data *tc = bridge_to_tc(bridge);
>
> @@ -1977,7 +2008,7 @@ tc_edp_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
> static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
> .attach = tc_dpi_bridge_attach,
> .mode_valid = tc_dpi_mode_valid,
> - .mode_set = tc_bridge_mode_set,
> + .mode_set = tc_dpi_bridge_mode_set,
> .atomic_check = tc_dpi_atomic_check,
> .atomic_enable = tc_dpi_bridge_atomic_enable,
> .atomic_disable = tc_dpi_bridge_atomic_disable,
> @@ -1991,7 +2022,7 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = {
> .attach = tc_edp_bridge_attach,
> .detach = tc_edp_bridge_detach,
> .mode_valid = tc_edp_mode_valid,
> - .mode_set = tc_bridge_mode_set,
> + .mode_set = tc_edp_bridge_mode_set,
> .atomic_check = tc_edp_atomic_check,
> .atomic_enable = tc_edp_bridge_atomic_enable,
> .atomic_disable = tc_edp_bridge_atomic_disable,
--
Regards,
Laurent Pinchart
