Re: [PATCH v2 2/2] drm/bridge: tc358767: Improve DPI output pixel clock accuracy

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@denx.de>
> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> 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: 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: Simona Vetter <simona@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> ---
> 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