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

[Dmitry Baryshkov]

On Sat, Nov 30, 2024 at 07:57:17PM +0100, Nikolaus Voss wrote:
> Hi Dmitry,
> 
> On Sat, 30 Nov 2024, Dmitry Baryshkov wrote:
> > On Tue, Nov 26, 2024 at 04:45:54PM +0100, 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 at vger.kernel.org>
> > > 
> > > Signed-off-by: Nikolaus Voss <nv at 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)
> > 
> > The driver uses atomic callbacks. Please use .atomic_check() instead.
> 
> So it is okay to modify drm_crtc_state->adjusted_mode in .atomic_check()? I

Yes. samsung-dsim, anx7625 do that (I stopped checking after the second
one).

> chose .mode_fixup() because the function name and args make it more obvious
> what is done there. Btw, the API reference doesn't say this call is
> deprecated.

It's not deprecated. But as the driver is using atomic calls (vs legacy
calls) it makes more sense to use atomic_check() too.

> A second thought:
> Maybe it would be a good idea to reject modes which result in an adjusted
> mode pclk that is not within certain boundaries, even if this patch doesn't
> do it yet. As I see it, that would be only possible in mode_fixup().

atomic_check() can definitely reject whatever is being stuffed to it.

> 
> > 
> > > +{
> > > +	const struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> > > +	unsigned long requested_link_freq =
> > > +		mode->clock * fsl_ldb_link_freq_factor(fsl_ldb);
> > > +	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);
> > > +
> > > +		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,
> > > --
> > > 2.43.0
> > > 
> > 
> > 
> 
> -- 
> Nikolaus Voss
> 

-- 
With best wishes
Dmitry