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@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.
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
Attachment:
signature.asc
Description: PGP signature
