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. 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.
>
> 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.
Granted that most of the panel drivers do not implement get_timings()
and granted that there are no current users of that interface, I think
we should move away from it (and maybe even drop it completely from
drm_panel).
What about achieving the same via slightly different approach: register
a non-preferred mode with the clock equal to the max clock allowed. The
bridge driver can then use the default and the "max" mode to select PLL
clock.
I understand that this suggestion doesn't follow the DPI panel
specifics, which are closer to the continuous timings rather than fixed
set of modes, however I really don't think that it's worth keeping the
interface for the sake of a single driver. Original commit 2938931f3732
("drm/panel: Add display timing support") from 2014 mentions using those
timings for .mode_fixup(), but I couldn't find a trace of the
corresponding implementation.
Another possible option might be to impletent adjusting modes in
.atomic_check() / .mode_fixup().
>
> 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))
> + 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)));
> +
> 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,
> --
> 2.45.2
>
--
With best wishes
Dmitry
