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@xxxxxxxxxxxxxxx>
> > >
> > > Signed-off-by: Nikolaus Voss <nv@xxxxxxx>
> > > ---
> > > 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
