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

[Liu Ying]

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