Re: [PATCH 4/8] drm/bridge: fsl-ldb: Use clk_round_rate() to validate "ldb" clock rate

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4711 bytes --]

On Mon, Sep 30, 2024 at 03:55:30PM GMT, Liu Ying wrote:
> On 09/30/2024, Maxime Ripard wrote:
> > On Mon, Sep 30, 2024 at 01:28:59PM GMT, Liu Ying wrote:
> >> Multiple display modes could be read from a display device's EDID.
> >> Use clk_round_rate() to validate the "ldb" clock rate for each mode
> >> in drm_bridge_funcs::mode_valid() to filter unsupported modes out.
> >>
> >> Also, if the "ldb" clock and the pixel clock are sibling in clock
> >> tree, use clk_round_rate() to validate the pixel clock rate against
> >> the "ldb" clock.  This is not done in display controller driver
> >> because drm_crtc_helper_funcs::mode_valid() may not decide to do
> >> the validation or not if multiple encoders are connected to the CRTC,
> >> e.g., i.MX93 LCDIF may connect with MIPI DSI controller, LDB and
> >> parallel display output simultaneously.
> >>
> >> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> >> ---
> >>  drivers/gpu/drm/bridge/fsl-ldb.c | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> >> index b559f3e0bef6..ee8471c86617 100644
> >> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> >> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> >> @@ -11,6 +11,7 @@
> >>  #include <linux/of_graph.h>
> >>  #include <linux/platform_device.h>
> >>  #include <linux/regmap.h>
> >> +#include <linux/units.h>
> >>  
> >>  #include <drm/drm_atomic_helper.h>
> >>  #include <drm/drm_bridge.h>
> >> @@ -64,6 +65,7 @@ struct fsl_ldb_devdata {
> >>  	u32 lvds_ctrl;
> >>  	bool lvds_en_bit;
> >>  	bool single_ctrl_reg;
> >> +	bool ldb_clk_pixel_clk_sibling;
> >>  };
> >>  
> >>  static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
> >> @@ -74,11 +76,13 @@ static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
> >>  	[IMX8MP_LDB] = {
> >>  		.ldb_ctrl = 0x5c,
> >>  		.lvds_ctrl = 0x128,
> >> +		.ldb_clk_pixel_clk_sibling = true,
> >>  	},
> >>  	[IMX93_LDB] = {
> >>  		.ldb_ctrl = 0x20,
> >>  		.lvds_ctrl = 0x24,
> >>  		.lvds_en_bit = true,
> >> +		.ldb_clk_pixel_clk_sibling = true,
> >>  	},
> >>  };
> >>  
> >> @@ -269,11 +273,29 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
> >>  		   const struct drm_display_info *info,
> >>  		   const struct drm_display_mode *mode)
> >>  {
> >> +	unsigned long link_freq, pclk_rate, rounded_pclk_rate;
> >>  	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> >>  
> >>  	if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 160000 : 80000))
> >>  		return MODE_CLOCK_HIGH;
> >>  
> >> +	/* Validate "ldb" clock rate. */
> >> +	link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> >> +	if (link_freq != clk_round_rate(fsl_ldb->clk, link_freq))
> >> +		return MODE_NOCLOCK;
> >> +
> >> +	/*
> >> +	 * Use "ldb" clock to validate pixel clock rate,
> >> +	 * if the two clocks are sibling.
> >> +	 */
> >> +	if (fsl_ldb->devdata->ldb_clk_pixel_clk_sibling) {
> >> +		pclk_rate = mode->clock * HZ_PER_KHZ;
> >> +
> >> +		rounded_pclk_rate = clk_round_rate(fsl_ldb->clk, pclk_rate);
> >> +		if (rounded_pclk_rate != pclk_rate)
> >> +			return MODE_NOCLOCK;
> >> +	}
> >> +
> > 
> > I guess this is to workaround the fact that the parent rate would be
> > changed, and thus the sibling rate as well? This should be documented in
> > a comment if so.
> 
> This is to workaround the fact that the display controller driver
> (lcdif_kms.c) cannot do the mode validation against pixel clock, as
> the commit message mentions.

That part is still not super clear to me, but it's also not super
important to the discussion.

My point is: from a clock API standpoint, there's absolutely no reason
to consider sibling clocks. clk_round_rate() should give you the rate
you want. If it affects other clocks it shouldn't, it's a clock driver
bug.

You might want to workaround it, but this is definitely not something
you should gloss over: it's a hack, it needs to be documented as such.

> The parent clock is IMX8MP_VIDEO_PLL1_OUT and it's clock rate is not
> supposed to be changed any more once IMX8MP_VIDEO_PLL1 clock rate is
> set by using DT assigned-clock-rates property.  For i.MX8MP EVK, the
> clock rate is assigned to 1039500000Hz in imx8mp.dtsi in media_blk_ctrl
> node.

There's two things wrong with what you just described:

  - assigned-clock-rates never provided the guarantee that the clock
    rate wouldn't change later on. So if you rely on that, here's your
    first bug.

  - If the parent clock rate must not change, why does that clock has
    SET_RATE_PARENT then? Because that's the bug you're trying to work
    around.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]