Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

Hello,

quoted

quoted

quoted

[After applying PATCH "clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate"]

This is a commit from Marek, which is, I believe going in the right
direction, so I am including it. Just with this change, the situation is
slightly different, but the result is the same:
- media_disp[12]_pix is set to freq A by using a divisor of 1 and
  setting video_pll1 to freq A.
- media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
  to freq 7*A.
  /!\ This as the side effect of changing media_disp[12]_pix from freq A
  to freq 7*A.

Although I'm not of a fan of setting CLK_SET_RATE_PARENT flag to the
LDB clock and pixel clocks,

I haven't commented much on this. For me, inaccurate pixel clocks mostly
work fine (if not too inaccurate), but it is true that having very
powerful PLL like the PLL1443, it is a pity not to use them at their
highest capabilities. However, I consider "not breaking users" more
important than having "perfect clock rates".

Whether an inaccurate clock "works" depends on the context. A .5%
deviation will be out of spec for HDMI for example. An inaccurate VBLANK
frequency might also break some use cases.

So that your display still works is not enough to prove it works.

Well, my display used to work. And now it no longer does. The perfect
has become the enemy of the good.

quoted

This series has one unique goal: accepting more accurate frequencies
*and* not breaking users in the most simplest cases.

quoted

would it work if the pixel clock rate is
set after the LDB clock rate is set in fsl_ldb_atomic_enable()?

The situation would be:
- media_ldb is set to 7*A by using a divisor of 1 and setting video_pll1
  to freq 7*A.
- media_disp[12]_pix is set to freq A by using a divisor of 7.

So yes, and the explanation of why is there:
https://elixir.bootlin.com/linux/v6.11.8/source/drivers/clk/clk-divider.c#L322

quoted

The
pixel clock can be got from LDB's remote input LCDIF DT node by
calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
does. Similar to setting pixel clock rate, I think a chance is that
pixel clock enablement can be moved from LCDIF driver to
fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.

TBH, this sounds like a hack and is no longer required with this series.

You are just trying to circumvent the fact that until now, applying a
rate in an upper clock would unconfigure the downstream rates, and I
think this is our first real problem.

quoted

https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/

Actually, one sibling patch of the above patch reverts ff06ea04e4cf
because I thought "fixed PLL rate" is the only solution, though I'm
discussing any alternative solution of "dynamically changeable PLL
rate" with Marek in the thread of the sibling patch.

I don't think we want fixed PLL rates. Especially if you start using
external (hot-pluggable) displays with different needs: it just does not
fly. There is one situation that cannot yet be handled and needs
manual reparenting: using 3 displays with a non-divisible pixel
frequency.

Funnily, external interfaces (ie, HDMI, DP) tend to be easier to work
with than panels. HDMI for example works with roughly three frequencies:
148.5MHz, 297MHz and 594MHz. If you set the PLL to 594MHz and the
downstream clock has a basic divider, it works just fine.

quoted

FYI we managed this specific "advanced" case with assigned-clock-parents
using an audio PLL as hinted by Marek. It mostly works, event though the
PLL1416 are less precise and offer less accurate pixel clocks.

Note that assigned-clock-parents doesn't provide any guarantee on
whether the parent will change in the future or not. It's strictly
equivalent to calling clk_set_parent in the driver's probe.

Oh yeah, but here I'm mentioning en even more complex case where we
connect three panels with pixel clocks that cannot be all three derived
from the same parent. There has never been any upstream support for
that and I doubt we will have any anytime soon because we need a central
(drm) place to be aware of the clock limitations and make these
decisions.

[...]

quoted

+       /*
+        * Dual cases require a 3.5 rate division on the pixel clocks, which
+        * cannot be achieved with regular single divisors. Instead, double the
+        * parent PLL rate in the first place. In order to do that, we first
+        * require twice the target clock rate, which will program the upper
+        * PLL. Then, we ask for the actual targeted rate, which can be achieved
+        * by dividing by 2 the already configured upper PLL rate, without
+        * making further changes to it.
+        */
+       if (fsl_ldb_is_dual(fsl_ldb))
+               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);
        clk_set_rate(fsl_ldb->clk, requested_link_freq);

This has nothing to do in a DRM driver. The clock driver logic needs
to handle it.

The approach I am proposing in this series can sadly not work, because
it is not possible to slightly modify a clock after the crtc has been
set up without getting back into drm for further tuning. I observed
that my series was dependent on the probe order, because the exact same
clock tree would work and not work depending on the order.

To get back to your comment, unfortunately, there will be no other
choice than having drm drivers being aware of clock limitations, just
because the clock subsystem alone, even if it would do the right thing
behind the hood, would still sometimes break displays. This means drm
drivers will have to care about clock constraints.

As an example here, I am fine arguing about the way (calling
clk_set_rate twice on the same clock), but the fact that the parent
clock must be multiplied: this is drm business, because it is a drm
requirement.

Thanks,
Miquèl