[PATCH v4] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[PATCH v4] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Nikolaus Voss]

LDB clock has to be a fixed multiple of the pixel clock.
Although LDB and pixel clock have a common source, this
constraint cannot be satisfied for any pixel clock at a
fixed source clock.

Violating this constraint leads to flickering and distorted
lines on the attached display.

To overcome this, there are these approches:

1. Modify the base PLL clock statically by changing the
   device tree, implies calculating the PLL clock by
   hand, e.g. commit 4fbb73416b10 ("arm64: dts:
   imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz")

2. Walk down the clock tree and modify the source clock.
   Proposed patch series by Miquel Raynal:
   [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

3. This patch: check constraint violation in
   drm_bridge_funcs.atomic_check() and adapt the pixel
   clock in drm_display_mode.adjusted_mode accordingly.

Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale i.MX8MP LDB bridge")
Cc: <stable@vger.kernel.org> # 6.12.x, 6.6.x
Signed-off-by: Nikolaus Voss <nv@vosn.de>

---
v2:
- use .atomic_check() instead of .mode_fixup() (Dmitry Baryshkov)
- add Fixes tag (Liu Ying)
- use fsl_ldb_link_frequency() and drop const qualifier for
  struct fsl_ldb* (Liu Ying)

v3:
- fix kernel test robot warning: fsl-ldb.c:125:30:
  warning: omitting the parameter name in a function definition
  is a C23 extension [-Wc23-extensions]
- fix/rephrase commit text due to discussion with Marek Vasut,
  Liu Ying and Miquel Raynal
- only calculate and set pixel clock if ldb is not already
  configured to the matching frequency

v4:
- handle mode changes correctly: recalculate pixel clock when
  mode changes occur
- tested on 6.12.x and 6.6.x stable branches

 drivers/gpu/drm/bridge/fsl-ldb.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 0e4bac7dd04ff..97ca522556718 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -121,6 +121,36 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
 				 bridge, flags);
 }
 
+static int fsl_ldb_atomic_check(struct drm_bridge *bridge,
+				struct drm_bridge_state *bridge_state,
+				struct drm_crtc_state *crtc_state,
+				struct drm_connector_state *connector_state)
+{
+	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+	const struct drm_display_mode *mode = &crtc_state->mode;
+	unsigned long requested_freq =
+		fsl_ldb_link_frequency(fsl_ldb, mode->clock);
+	unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_freq);
+
+	if (crtc_state->mode_changed && (freq != requested_freq)) {
+		/*
+		 * this will lead to flicker and incomplete lines on
+		 * the attached display, adjust the CRTC clock
+		 * accordingly.
+		 */
+		struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+		int pclk = freq / fsl_ldb_link_frequency(fsl_ldb, 1);
+
+		dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n",
+			 adjusted_mode->clock, pclk);
+
+		adjusted_mode->clock = pclk;
+		adjusted_mode->crtc_clock = pclk;
+	}
+
+	return 0;
+}
+
 static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
 				  struct drm_bridge_state *old_bridge_state)
 {
@@ -280,6 +310,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
 
 static const struct drm_bridge_funcs funcs = {
 	.attach = fsl_ldb_attach,
+	.atomic_check = fsl_ldb_atomic_check,
 	.atomic_enable = fsl_ldb_atomic_enable,
 	.atomic_disable = fsl_ldb_atomic_disable,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
-- 
2.43.0

Re: [PATCH v4] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Martin Kepplinger]

Am Donnerstag, dem 19.12.2024 um 11:54 +0100 schrieb Nikolaus Voss:
> LDB clock has to be a fixed multiple of the pixel clock.
> Although LDB and pixel clock have a common source, this
> constraint cannot be satisfied for any pixel clock at a
> fixed source clock.
> 
> Violating this constraint leads to flickering and distorted
> lines on the attached display.
> 
> To overcome this, there are these approches:
> 
> 1. Modify the base PLL clock statically by changing the
>    device tree, implies calculating the PLL clock by
>    hand, e.g. commit 4fbb73416b10 ("arm64: dts:
>    imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz")
> 
> 2. Walk down the clock tree and modify the source clock.
>    Proposed patch series by Miquel Raynal:
>    [PATCH 0/5] clk: Fix simple video pipelines on i.MX8
> 
> 3. This patch: check constraint violation in
>    drm_bridge_funcs.atomic_check() and adapt the pixel
>    clock in drm_display_mode.adjusted_mode accordingly.
> 
> Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale
> i.MX8MP LDB bridge")
> Cc: <stable@vger.kernel.org> # 6.12.x, 6.6.x
> Signed-off-by: Nikolaus Voss <nv@vosn.de>
> 
> ---
> v2:
> - use .atomic_check() instead of .mode_fixup() (Dmitry Baryshkov)
> - add Fixes tag (Liu Ying)
> - use fsl_ldb_link_frequency() and drop const qualifier for
>   struct fsl_ldb* (Liu Ying)
> 
> v3:
> - fix kernel test robot warning: fsl-ldb.c:125:30:
>   warning: omitting the parameter name in a function definition
>   is a C23 extension [-Wc23-extensions]
> - fix/rephrase commit text due to discussion with Marek Vasut,
>   Liu Ying and Miquel Raynal
> - only calculate and set pixel clock if ldb is not already
>   configured to the matching frequency
> 
> v4:
> - handle mode changes correctly: recalculate pixel clock when
>   mode changes occur
> - tested on 6.12.x and 6.6.x stable branches
> 
>  drivers/gpu/drm/bridge/fsl-ldb.c | 31
> +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
> b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 0e4bac7dd04ff..97ca522556718 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -121,6 +121,36 @@ static int fsl_ldb_attach(struct drm_bridge
> *bridge,
>                                  bridge, flags);
>  }
>  
> +static int fsl_ldb_atomic_check(struct drm_bridge *bridge,
> +                               struct drm_bridge_state
> *bridge_state,
> +                               struct drm_crtc_state *crtc_state,
> +                               struct drm_connector_state
> *connector_state)
> +{
> +       struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> +       const struct drm_display_mode *mode = &crtc_state->mode;
> +       unsigned long requested_freq =
> +               fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> +       unsigned long freq = clk_round_rate(fsl_ldb->clk,
> requested_freq);
> +
> +       if (crtc_state->mode_changed && (freq != requested_freq)) {
> +               /*
> +                * this will lead to flicker and incomplete lines on
> +                * the attached display, adjust the CRTC clock
> +                * accordingly.
> +                */
> +               struct drm_display_mode *adjusted_mode = &crtc_state-
> >adjusted_mode;
> +               int pclk = freq / fsl_ldb_link_frequency(fsl_ldb, 1);
> +
> +               dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match
> LDB clk (%d kHz -> %d kHz)!\n",
> +                        adjusted_mode->clock, pclk);
> +
> +               adjusted_mode->clock = pclk;
> +               adjusted_mode->crtc_clock = pclk;
> +       }
> +
> +       return 0;
> +}
> +
>  static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>                                   struct drm_bridge_state
> *old_bridge_state)
>  {
> @@ -280,6 +310,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>  
>  static const struct drm_bridge_funcs funcs = {
>         .attach = fsl_ldb_attach,
> +       .atomic_check = fsl_ldb_atomic_check,
>         .atomic_enable = fsl_ldb_atomic_enable,
>         .atomic_disable = fsl_ldb_atomic_disable,
>         .atomic_duplicate_state =
> drm_atomic_helper_bridge_duplicate_state,

without judging the approaches you list above, this works for me on
imx8mp as well,

Tested-by: Martin Kepplinger <martink@posteo.de>

thank you,

                              martin

Re: [PATCH v4] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Miquel Raynal]

Hello Martin,

On 19/12/2024 at 12:39:05 GMT, Martin Kepplinger <martink@posteo.de> wrote:

> Am Donnerstag, dem 19.12.2024 um 11:54 +0100 schrieb Nikolaus Voss:
>> LDB clock has to be a fixed multiple of the pixel clock.
>> Although LDB and pixel clock have a common source, this
>> constraint cannot be satisfied for any pixel clock at a
>> fixed source clock.
>> 
>> Violating this constraint leads to flickering and distorted
>> lines on the attached display.
>> 
>> To overcome this, there are these approches:
>> 
>> 1. Modify the base PLL clock statically by changing the
>>    device tree, implies calculating the PLL clock by
>>    hand, e.g. commit 4fbb73416b10 ("arm64: dts:
>>    imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz")
>> 
>> 2. Walk down the clock tree and modify the source clock.
>>    Proposed patch series by Miquel Raynal:
>>    [PATCH 0/5] clk: Fix simple video pipelines on i.MX8
>> 
>> 3. This patch: check constraint violation in
>>    drm_bridge_funcs.atomic_check() and adapt the pixel
>>    clock in drm_display_mode.adjusted_mode accordingly.
>> 
>> Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale
>> i.MX8MP LDB bridge")
>> Cc: <stable@vger.kernel.org> # 6.12.x, 6.6.x
>> Signed-off-by: Nikolaus Voss <nv@vosn.de>
>> 
>> ---

I didn't investigate further, but FYI this approach does not fix my
situation with iMX8MP. I am not saying it's wrong, because I am not
experienced enough with drm, but at least that it is not a replacement
of point #2 above.

Cheers,
Miquèl