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;
+	}
+
 	return MODE_OK;
 }
 
-- 
2.34.1

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.

Maxime

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.

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.

> 
> Maxime

-- 
Regards,
Liu Ying

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

On 10/11/2024, Maxime Ripard wrote:
> 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.

As kerneldoc of drm_crtc_helper_funcs::mode_valid mentions that
it is not allowed to look at anything else but the passed-in mode,
it doesn't know of the connected encoder(s)/bridge(s) and thus
cannot decide if it should do mode validation against pixel clock
or not.  Encoder/bridge drivers could adjust pixel clock rates
for display modes.  So, mode validation against pixel clock should
be done in this bridge driver.

In fact, the pixel clock should have been defined as a DT property
in fsl,ldb.yaml because the clock routes to LDB as an input signal.
However, it's too late...  If the DT property was defined in the
first place, then this driver can naturally do mode validation
against pixel clock instead of this workaround.

> 
> 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

Agree, but it's a workaround.

> you want. If it affects other clocks it shouldn't, it's a clock driver
> bug.

The sibling clocks are the same type of clocks from HW design
point of view and derived from the same clock parent/PLL.
That's the reason why the workaround works.

> 
> 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.

I can add some documentation in next version to clarify this
a bit.

> 
>> 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.

I'm not relying on that.  Instead, the PLL clock rate is not
supposed to change since IMX8MP_CLK_MEDIA_LDB clock("ldb" clock
parent clock) hasn't the CLK_SET_RATE_PARENT flag.  And, we don't
want to change the PLL clock rate at runtime because the PLL can
be used by i.MX8MP MIPI DSI and LDB display pipelines at the same
time, driven by two LCDIFv3 display controllers respectively with
two imx-lcdif KMS instances.  We don't want to see the two display
pipelines to step on each other.

> 
>   - 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.

IMX8MP_CLK_MEDIA_LDB clock hasn't the CLK_SET_RATE_PARENT flag.
I'm fine with the "ldb" clock tree from the current clock driver
PoV - just trying to validate pixel clock rate as a workaround.

> 
> Maxime

-- 
Regards,
Liu Ying

On Sat, Oct 12, 2024 at 02:18:16PM GMT, Liu Ying wrote:
> On 10/11/2024, Maxime Ripard wrote:
> > 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.
> 
> As kerneldoc of drm_crtc_helper_funcs::mode_valid mentions that
> it is not allowed to look at anything else but the passed-in mode,
> it doesn't know of the connected encoder(s)/bridge(s) and thus
> cannot decide if it should do mode validation against pixel clock
> or not.  Encoder/bridge drivers could adjust pixel clock rates
> for display modes.  So, mode validation against pixel clock should
> be done in this bridge driver.
> 
> In fact, the pixel clock should have been defined as a DT property
> in fsl,ldb.yaml because the clock routes to LDB as an input signal.
> However, it's too late...  If the DT property was defined in the
> first place, then this driver can naturally do mode validation
> against pixel clock instead of this workaround.
> 
> > 
> > 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
> 
> Agree, but it's a workaround.
> 
> > you want. If it affects other clocks it shouldn't, it's a clock driver
> > bug.
> 
> The sibling clocks are the same type of clocks from HW design
> point of view and derived from the same clock parent/PLL.
> That's the reason why the workaround works.
> 
> > 
> > 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.
> 
> I can add some documentation in next version to clarify this
> a bit.
> 
> > 
> >> 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.
> 
> I'm not relying on that.

Sure you do. If anything in the kernel changes the rate of the
VIDEO_PLL1 clock, then it's game over and "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." isn't true anymore.

> Instead, the PLL clock rate is not supposed to change since
> IMX8MP_CLK_MEDIA_LDB clock("ldb" clock parent clock) hasn't the
> CLK_SET_RATE_PARENT flag. And, we don't want to change the PLL clock
> rate at runtime because the PLL can be used by i.MX8MP MIPI DSI and
> LDB display pipelines at the same time, driven by two LCDIFv3 display
> controllers respectively with two imx-lcdif KMS instances. We don't
> want to see the two display pipelines to step on each other.
> 
> > 
> >   - 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.
> 
> IMX8MP_CLK_MEDIA_LDB clock hasn't the CLK_SET_RATE_PARENT flag.
> I'm fine with the "ldb" clock tree from the current clock driver
> PoV - just trying to validate pixel clock rate as a workaround.

As far as I can see, the ldb clock is IMX8MP_CLK_MEDIA_LDB_ROOT in
imx8mp.dtsi. That clock is defined using imx_clk_hw_gate2_shared2 that
does set CLK_SET_RATE_PARENT.

Maxime

On 10/14/2024, Maxime Ripard wrote:
> On Sat, Oct 12, 2024 at 02:18:16PM GMT, Liu Ying wrote:
>> On 10/11/2024, Maxime Ripard wrote:
>>> 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.
>>
>> As kerneldoc of drm_crtc_helper_funcs::mode_valid mentions that
>> it is not allowed to look at anything else but the passed-in mode,
>> it doesn't know of the connected encoder(s)/bridge(s) and thus
>> cannot decide if it should do mode validation against pixel clock
>> or not.  Encoder/bridge drivers could adjust pixel clock rates
>> for display modes.  So, mode validation against pixel clock should
>> be done in this bridge driver.
>>
>> In fact, the pixel clock should have been defined as a DT property
>> in fsl,ldb.yaml because the clock routes to LDB as an input signal.
>> However, it's too late...  If the DT property was defined in the
>> first place, then this driver can naturally do mode validation
>> against pixel clock instead of this workaround.
>>
>>>
>>> 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
>>
>> Agree, but it's a workaround.
>>
>>> you want. If it affects other clocks it shouldn't, it's a clock driver
>>> bug.
>>
>> The sibling clocks are the same type of clocks from HW design
>> point of view and derived from the same clock parent/PLL.
>> That's the reason why the workaround works.
>>
>>>
>>> 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.
>>
>> I can add some documentation in next version to clarify this
>> a bit.
>>
>>>
>>>> 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.
>>
>> I'm not relying on that.
> 
> Sure you do. If anything in the kernel changes the rate of the
> VIDEO_PLL1 clock, then it's game over and "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." isn't true anymore.

"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." implies
that IMX8MP_VIDEO_PLL1 is used only by certain display pipelines as DT
writer can deliberately assign it as the parent clock of clocks like
display controller's pixel clock, which means nothing else in the
kernel would change the rate of the IMX8MP_VIDEO_PLL1 clock.

> 
>> Instead, the PLL clock rate is not supposed to change since
>> IMX8MP_CLK_MEDIA_LDB clock("ldb" clock parent clock) hasn't the
>> CLK_SET_RATE_PARENT flag. And, we don't want to change the PLL clock
>> rate at runtime because the PLL can be used by i.MX8MP MIPI DSI and
>> LDB display pipelines at the same time, driven by two LCDIFv3 display
>> controllers respectively with two imx-lcdif KMS instances. We don't
>> want to see the two display pipelines to step on each other.
>>
>>>
>>>   - 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.
>>
>> IMX8MP_CLK_MEDIA_LDB clock hasn't the CLK_SET_RATE_PARENT flag.
>> I'm fine with the "ldb" clock tree from the current clock driver
>> PoV - just trying to validate pixel clock rate as a workaround.
> 
> As far as I can see, the ldb clock is IMX8MP_CLK_MEDIA_LDB_ROOT in
> imx8mp.dtsi. That clock is defined using imx_clk_hw_gate2_shared2 that
> does set CLK_SET_RATE_PARENT.

I said IMX8MP_CLK_MEDIA_LDB, not IMX8MP_CLK_MEDIA_LDB_ROOT.
IMX8MP_CLK_MEDIA_LDB clock is IMX8MP_CLK_MEDIA_LDB_ROOT clock's
parent clock.
IMX8MP_CLK_MEDIA_LDB clock hasn't the CLK_SET_RATE_PARENT flag.

IMX8MP_VIDEO_PLL1
  IMX8MP_VIDEO_PLL1_BYPASS
    IMX8MP_VIDEO_PLL1_OUT
      IMX8MP_CLK_MEDIA_LDB
        IMX8MP_CLK_MEDIA_LDB_ROOT  ->  "ldb" clock

> 
> Maxime

-- 
Regards,
Liu Ying