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

[Liu Ying <victor.liu@oss.nxp.com>]

On 11/27/2024, Nikolaus Voss wrote:
> LDB clock has to be a fixed multiple of the pixel clock.
> As LDB and pixel clock are derived from different clock sources
> (at least on imx8mp), this constraint cannot be satisfied for
> any pixel clock, which leads to flickering and incomplete
> lines on the attached display.
> 
> To overcome this, check this condition in mode_fixup() and
> adapt the pixel clock accordingly.
> 
> Cc: <stable@vger.kernel.org>

It looks like stable is not effectively Cc'ed.
Need a Fixes tag?

> 
> Signed-off-by: Nikolaus Voss <nv@vosn.de>
> ---
>  drivers/gpu/drm/bridge/fsl-ldb.c | 40 ++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 0e4bac7dd04ff..e341341b8c600 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -104,12 +104,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct drm_bridge *bridge)
>  	return container_of(bridge, struct fsl_ldb, bridge);
>  }
>  
> +static unsigned int fsl_ldb_link_freq_factor(const struct fsl_ldb *fsl_ldb)
> +{
> +	return fsl_ldb_is_dual(fsl_ldb) ? 3500 : 7000;
> +}
> +
>  static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
>  {
> -	if (fsl_ldb_is_dual(fsl_ldb))
> -		return clock * 3500;
> -	else
> -		return clock * 7000;
> +	return clock * fsl_ldb_link_freq_factor(fsl_ldb);
>  }
>  
>  static int fsl_ldb_attach(struct drm_bridge *bridge,
> @@ -121,6 +123,35 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
>  				 bridge, flags);
>  }
>  
> +static bool fsl_ldb_mode_fixup(struct drm_bridge *bridge,
> +				const struct drm_display_mode *mode,
> +				struct drm_display_mode *adjusted_mode)
> +{
> +	const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);

Why const?
If no const, then ...

> +	unsigned long requested_link_freq =
> +		mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);

... this could be
        unsigned long requested_link_freq =                                      
                                fsl_ldb_link_frequency(fsl_ldb, mode->clock); 

> +	unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq);
> +
> +	if (freq != requested_link_freq) {
> +		/*
> +		 * this will lead to flicker and incomplete lines on
> +		 * the attached display, adjust the CRTC clock
> +		 * accordingly.
> +		 */
> +		int pclk = freq / fsl_ldb_link_freq_factor(fsl_ldb);

I doubt that pixel clock tree cannot find appropriate division ratios
for some pixel clock rates, especially for dual-link LVDS on i.MX8MP
and i.MX93 platforms, because PLL clock rate should be 7x faster than
pixel clock rate and 2x faster than "ldb" clock rate so that the 3.5
folder between "ldb" clock and pixel clock can be met. That means the
PLL clock rate needs to be explicitly set first for this case. 

Can you assign the PLL clock rate in DT to satisfy the "ldb" and pixel
clock rates like the below commit does, if you use a LVDS panel?

4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
frequency to 506.8 MHz")

> +
> +		if (adjusted_mode->clock != pclk) {
> +			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 true;
> +}
> +
>  static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>  				  struct drm_bridge_state *old_bridge_state)
>  {
> @@ -280,6 +311,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>  
>  static const struct drm_bridge_funcs funcs = {
>  	.attach = fsl_ldb_attach,
> +	.mode_fixup = fsl_ldb_mode_fixup,
>  	.atomic_enable = fsl_ldb_atomic_enable,
>  	.atomic_disable = fsl_ldb_atomic_disable,
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
--
Regards,
Liu Ying