[PATCH v2 1/2] drm/bridge/panel: Add drm_bridge_get_panel to extract panel from last bridge

[PATCH v2 1/2] drm/bridge/panel: Add drm_bridge_get_panel to extract panel from last bridge

[Marek Vasut]

Add drm_bridge_get_panel() function to extract drm_panel pointer from
panel_bridge. This can be used by bridges in the middle to look up and
access drm_panel at the end, and e.g. extract display_timings from it.

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: - New patch
---
 drivers/gpu/drm/bridge/panel.c | 18 ++++++++++++++++++
 include/drm/drm_bridge.h       |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 6e88339dec0f5..0e5937dc7ab21 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -235,6 +235,24 @@ bool drm_bridge_is_panel(const struct drm_bridge *bridge)
 }
 EXPORT_SYMBOL(drm_bridge_is_panel);
 
+/**
+ * drm_bridge_get_panel - Return panel if a drm_bridge is a panel_bridge.
+ *
+ * @bridge: The drm_bridge.
+ *
+ * Returns drm_panel pointer if the bridge is a panel bridge, or NULL otherwise.
+ */
+struct drm_panel *drm_bridge_get_panel(struct drm_bridge *bridge)
+{
+	struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+
+	if (!drm_bridge_is_panel(bridge))
+		return NULL;
+
+	return panel_bridge->panel;
+}
+EXPORT_SYMBOL(drm_bridge_get_panel);
+
 /**
  * drm_panel_bridge_add - Creates a &drm_bridge and &drm_connector that
  * just calls the appropriate functions from &drm_panel.
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index e8d735b7f6a48..4b64ab47c9d7f 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -984,6 +984,7 @@ void drm_bridge_hpd_notify(struct drm_bridge *bridge,
 
 #ifdef CONFIG_DRM_PANEL_BRIDGE
 bool drm_bridge_is_panel(const struct drm_bridge *bridge);
+struct drm_panel *drm_bridge_get_panel(struct drm_bridge *bridge);
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
 struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
 					      u32 connector_type);
@@ -1004,6 +1005,11 @@ static inline bool drm_bridge_is_panel(const struct drm_bridge *bridge)
 	return false;
 }
 
+static struct drm_panel *drm_bridge_get_panel(struct drm_bridge *bridge)
+{
+	return NULL;
+}
+
 static inline int drm_panel_bridge_set_orientation(struct drm_connector *connector,
 						   struct drm_bridge *bridge)
 {
-- 
2.45.2

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

[Marek Vasut]

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.

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))
+			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

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

[Laurent Pinchart]

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

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

[Dmitry Baryshkov]

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@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))
> +			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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2809 bytes --]

On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote:
> 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).

I think we should do the opposite :)

Panels usually have a pretty large operating window, and the current
construct only works because nobody really uses the same panels (or
we're hiding that behind different compatibles) across SoCs or
generation. Or we're working around it.

It's kind of a mess, and it gets messy in encoders too: some will filter
out panel modes, some won't. Some will adjust timings to fit their
clocks, some won't. etc.

Going forward, switching everyone to a timing-like interface and
providing a set of helpers to adjust timings within possible boundaries
to fit a clock rate is doable and should be done imo.

> 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().

It's unused because we don't have an easy API for encoders to use it. We
should fix *that*.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Dmitry Baryshkov]

On Mon, 25 Nov 2024 at 15:17, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote:
> > 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).
>
> I think we should do the opposite :)
>
> Panels usually have a pretty large operating window, and the current
> construct only works because nobody really uses the same panels (or
> we're hiding that behind different compatibles) across SoCs or
> generation. Or we're working around it.
>
> It's kind of a mess, and it gets messy in encoders too: some will filter
> out panel modes, some won't. Some will adjust timings to fit their
> clocks, some won't. etc.

Well... I think it's messy because we have so many different
interfaces. Some encoders can poke directly into the panel, some
drivers use bridge chains and panel bridge. Some do even a messier
thing and try both at the same time.
I think that in the long-term nobody should be using the drm_panel
interface directly.

>
> Going forward, switching everyone to a timing-like interface and
> providing a set of helpers to adjust timings within possible boundaries
> to fit a clock rate is doable and should be done imo.

... and then it might help with the command-mode DSI panels with DSC...

>
> > 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().
>
> It's unused because we don't have an easy API for encoders to use it. We
> should fix *that*.

Sounds good to me too.
I'm not sure what it should look like though. We barely scratched our
heads when looking at the DSC / CMD stuff, but nothing came out of it.

Ideally... there should be some kind of get_timings being available
through the full bridge chain, so that the encoders can use it. But
I'm not sure how that should work, because some bridges would like to
manipulate those timings. And some bridges will completely drop
get_timings() and produce raw modes even after consuming the timings
set.


-- 
With best wishes
Dmitry

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

[Marek Vasut]

On 11/22/24 2:32 PM, Dmitry Baryshkov wrote:
> 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).

It does fit DPI and LVDS panels and their descriptions in datasheets the 
best.

> 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().
Something like this ?

static const struct display_timing chefree_ch101olhlwh_002_timing = {
   .pixelclock = { 68900000, 71100000, 73400000 },
   ...
};

static const struct panel_desc chefree_ch101olhlwh_002 = {
   .timings = &chefree_ch101olhlwh_002_timing,
   .num_timings = 1,
   ...
};

... would turn into ...

static const struct drm_display_mode chefree_ch101olhlwh_002_mode[3] = {
   {
     .clock = 68900000,
     ...
   }, {
     .clock = 71100000,
     ...
   }, {
     .clock = 73400000,
     ...
   }
};

static const struct panel_desc chefree_ch101olhlwh_002 = {
   .modes = &chefree_ch101olhlwh_002_mode,
   .num_timings = 3,
   ...
};

?

Maybe some mode flag to specify which mode is MIN/TYP/MAX would be 
useful with that change too ?

Finally, the TC358767 driver would test all modes and find the most 
accurate one ?

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

[Marek Vasut]

On 11/25/24 2:17 PM, Maxime Ripard wrote:
> On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote:
>> 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).
> 
> I think we should do the opposite :)
> 
> Panels usually have a pretty large operating window, and the current
> construct only works because nobody really uses the same panels (or
> we're hiding that behind different compatibles) across SoCs or
> generation. Or we're working around it.

I am using the same panels on NXP MX6/MX8 and STM32MP1 with wildly 
different pipeline setups too.

> It's kind of a mess, and it gets messy in encoders too: some will filter
> out panel modes, some won't. Some will adjust timings to fit their
> clocks, some won't. etc.
> 
> Going forward, switching everyone to a timing-like interface and
> providing a set of helpers to adjust timings within possible boundaries
> to fit a clock rate is doable and should be done imo.
Maybe it would be better to have multiple modes and mark them with 
MIN/TYP/MAX flag , so at least there wouldn't be two sets of structures 
describing the same thing, but only one structure (drm_display_mode) ?

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3240 bytes --]

On Tue, Nov 26, 2024 at 12:48:20AM +0100, Marek Vasut wrote:
> On 11/22/24 2:32 PM, Dmitry Baryshkov wrote:
> > 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).
> 
> It does fit DPI and LVDS panels and their descriptions in datasheets the
> best.
> 
> > 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().
> Something like this ?
> 
> static const struct display_timing chefree_ch101olhlwh_002_timing = {
>   .pixelclock = { 68900000, 71100000, 73400000 },
>   ...
> };
> 
> static const struct panel_desc chefree_ch101olhlwh_002 = {
>   .timings = &chefree_ch101olhlwh_002_timing,
>   .num_timings = 1,
>   ...
> };
> 
> ... would turn into ...
> 
> static const struct drm_display_mode chefree_ch101olhlwh_002_mode[3] = {
>   {
>     .clock = 68900000,
>     ...
>   }, {
>     .clock = 71100000,
>     ...
>   }, {
>     .clock = 73400000,
>     ...
>   }
> };
> 
> static const struct panel_desc chefree_ch101olhlwh_002 = {
>   .modes = &chefree_ch101olhlwh_002_mode,
>   .num_timings = 3,
>   ...
> };
> 
> ?

Except that doesn't work if you want to keep your driver at the expected
framerate. To reduce the pixel clock, you also need to reduce the
blanking period within the acceptable boundaries if you want to keep the
same framerate.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4553 bytes --]

On Tue, Nov 26, 2024 at 12:07:10AM +0200, Dmitry Baryshkov wrote:
> On Mon, 25 Nov 2024 at 15:17, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote:
> > > 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).
> >
> > I think we should do the opposite :)
> >
> > Panels usually have a pretty large operating window, and the current
> > construct only works because nobody really uses the same panels (or
> > we're hiding that behind different compatibles) across SoCs or
> > generation. Or we're working around it.
> >
> > It's kind of a mess, and it gets messy in encoders too: some will filter
> > out panel modes, some won't. Some will adjust timings to fit their
> > clocks, some won't. etc.
> 
> Well... I think it's messy because we have so many different
> interfaces. Some encoders can poke directly into the panel, some
> drivers use bridge chains and panel bridge. Some do even a messier
> thing and try both at the same time.
> I think that in the long-term nobody should be using the drm_panel
> interface directly.

There's a few corner cases that are doable with the panel API that
aren't possible with the bridge API still. Being able to get the pixel
clock you're going to get from the encoder and adjust the timings to
match the panel tolerance is one for example, and we have a couple of
drivers doing that.

> > Going forward, switching everyone to a timing-like interface and
> > providing a set of helpers to adjust timings within possible boundaries
> > to fit a clock rate is doable and should be done imo.
> 
> ... and then it might help with the command-mode DSI panels with DSC...

How so?

> >
> > > 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().
> >
> > It's unused because we don't have an easy API for encoders to use it. We
> > should fix *that*.
> 
> Sounds good to me too.
> I'm not sure what it should look like though. We barely scratched our
> heads when looking at the DSC / CMD stuff, but nothing came out of it.
> 
> Ideally... there should be some kind of get_timings being available
> through the full bridge chain, so that the encoders can use it. But
> I'm not sure how that should work, because some bridges would like to
> manipulate those timings. And some bridges will completely drop
> get_timings() and produce raw modes even after consuming the timings
> set.

the drm_display_mode -> timings conversion is fairly easy to do: just
use the same values for min, typ and max.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Marek Vasut]

On 11/26/24 4:56 PM, Maxime Ripard wrote:
> On Tue, Nov 26, 2024 at 12:48:20AM +0100, Marek Vasut wrote:
>> On 11/22/24 2:32 PM, Dmitry Baryshkov wrote:
>>> 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).
>>
>> It does fit DPI and LVDS panels and their descriptions in datasheets the
>> best.
>>
>>> 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().
>> Something like this ?
>>
>> static const struct display_timing chefree_ch101olhlwh_002_timing = {
>>    .pixelclock = { 68900000, 71100000, 73400000 },
>>    ...
>> };
>>
>> static const struct panel_desc chefree_ch101olhlwh_002 = {
>>    .timings = &chefree_ch101olhlwh_002_timing,
>>    .num_timings = 1,
>>    ...
>> };
>>
>> ... would turn into ...
>>
>> static const struct drm_display_mode chefree_ch101olhlwh_002_mode[3] = {
>>    {
>>      .clock = 68900000,
>>      ...
>>    }, {
>>      .clock = 71100000,
>>      ...
>>    }, {
>>      .clock = 73400000,
>>      ...
>>    }
>> };
>>
>> static const struct panel_desc chefree_ch101olhlwh_002 = {
>>    .modes = &chefree_ch101olhlwh_002_mode,
>>    .num_timings = 3,
>>    ...
>> };
>>
>> ?
> 
> Except that doesn't work if you want to keep your driver at the expected
> framerate. To reduce the pixel clock, you also need to reduce the
> blanking period within the acceptable boundaries if you want to keep the
> same framerate.
Each mode entry would have to have its own full set of timings of 
course, not just .clock . Those can be converted from display_timing 
just like clock.

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

[Dmitry Baryshkov]

On Tue, 26 Nov 2024 at 18:00, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Tue, Nov 26, 2024 at 12:07:10AM +0200, Dmitry Baryshkov wrote:
> > On Mon, 25 Nov 2024 at 15:17, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote:
> > > > 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).
> > >
> > > I think we should do the opposite :)
> > >
> > > Panels usually have a pretty large operating window, and the current
> > > construct only works because nobody really uses the same panels (or
> > > we're hiding that behind different compatibles) across SoCs or
> > > generation. Or we're working around it.
> > >
> > > It's kind of a mess, and it gets messy in encoders too: some will filter
> > > out panel modes, some won't. Some will adjust timings to fit their
> > > clocks, some won't. etc.
> >
> > Well... I think it's messy because we have so many different
> > interfaces. Some encoders can poke directly into the panel, some
> > drivers use bridge chains and panel bridge. Some do even a messier
> > thing and try both at the same time.
> > I think that in the long-term nobody should be using the drm_panel
> > interface directly.
>
> There's a few corner cases that are doable with the panel API that
> aren't possible with the bridge API still. Being able to get the pixel
> clock you're going to get from the encoder and adjust the timings to
> match the panel tolerance is one for example, and we have a couple of
> drivers doing that.

My initial point was that only panel-simple (and panel-edp) implement
the get_timings().
And for the existing encoder drivers I think we should be migrating to
using bridge API instead of panel API.

>
> > > Going forward, switching everyone to a timing-like interface and
> > > providing a set of helpers to adjust timings within possible boundaries
> > > to fit a clock rate is doable and should be done imo.
> >
> > ... and then it might help with the command-mode DSI panels with DSC...
>
> How so?

We had troubles with CMD panels, because we need to get the mode
timings based on FPS and compression ratio (so it can  not be
pregrorammed, like we usually do for most of the panels).

>
> > >
> > > > 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().
> > >
> > > It's unused because we don't have an easy API for encoders to use it. We
> > > should fix *that*.
> >
> > Sounds good to me too.
> > I'm not sure what it should look like though. We barely scratched our
> > heads when looking at the DSC / CMD stuff, but nothing came out of it.
> >
> > Ideally... there should be some kind of get_timings being available
> > through the full bridge chain, so that the encoders can use it. But
> > I'm not sure how that should work, because some bridges would like to
> > manipulate those timings. And some bridges will completely drop
> > get_timings() and produce raw modes even after consuming the timings
> > set.
>
> the drm_display_mode -> timings conversion is fairly easy to do: just
> use the same values for min, typ and max.

-- 
With best wishes
Dmitry