| Message ID | 20241219105416.4AE0D1201@mail.steuer-voss.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] drm: bridge: fsl-ldb: fixup mode on freq mismatch | expand |
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
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,
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(+)