Message-ID: <87h67uw92w.fsf@bootlin.com>
Date: Tue, 26 Nov 2024 12:03:35 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Liu Ying <victor.liu@....com>
Cc: Abel Vesa <abelvesa@...nel.org>,  Peng Fan <peng.fan@....com>,  Michael
 Turquette <mturquette@...libre.com>,  Stephen Boyd <sboyd@...nel.org>,
  Shawn Guo <shawnguo@...nel.org>,  Sascha Hauer <s.hauer@...gutronix.de>,
  Pengutronix Kernel Team <kernel@...gutronix.de>,  Fabio Estevam
 <festevam@...il.com>,  Marek Vasut <marex@...x.de>,  Laurent Pinchart
 <laurent.pinchart@...asonboard.com>,  linux-clk@...r.kernel.org,
  imx@...ts.linux.dev,  linux-arm-kernel@...ts.infradead.org,
  linux-kernel@...r.kernel.org,  dri-devel@...ts.freedesktop.org,  Abel
 Vesa <abel.vesa@...aro.org>,  Herve Codina <herve.codina@...tlin.com>,
  Luca Ceresoli <luca.ceresoli@...tlin.com>,  Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>,  Ian Ray <ian.ray@...com>,
  stable@...r.kernel.org
Subject: Re: [PATCH 0/5] clk: Fix simple video pipelines on i.MX8

Hi Liu,

>>> 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.
>
> Pixel clock is an input signal to LDB, which is reflected in LDB block
> diagram in i.MX8MP TRM(see Figure 13-70) where "CLOCK" goes into LDB
> along with "DATA", "HSYNC", "VSYNC", "DATA_EN" and "CONTROL".  So,
> fsl,ldb.yaml should have documented the pixel clock in clocks and
> clock-names properties, but unfortunately it doesn't and it's too late
> to do so.  The workaround is to get the pixel clock from LCDIF DT node
> in the LDB driver.  I would call it a workaround rather than a hack,
> since fsl,ldb.yaml should have documented the pixel clock in the first
> place.
>
>> 
>> 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.
>
> I'm still not a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock
> and pixel clocks.  If we look at the bigger picture, the first real
> problem is probably how to support both separated video PLLs and shared
> video PLL.
>
>> 
>>> 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
>
> I don't want fixed PLL rates, either...
>
>> external (hot-pluggable) displays with different needs: it just does not
>
> ... but, considering the problem of how to support separated/shared video
> PLLs, I think we have to accept the fixed PLL rates.  So, unfortunately
> some video modes read from EDID cannot fly.  If there is an feasible
> alternative solution, it will be good to implement it, but till now I
> don't see any.

Can you please remind me what your exact display setup (and their
required pixel clocks) is?

AFAIU, you don't want to use dynamic calculations for the PLL because it
breaks your use case with HDMI. Of course this is a very limited use
case, because using a static rate means almost a single type of display
can be plugged.

The clock subsystem will not recalculate the video_pll1 if you can
achieve a perfect rate change using the LDB/PIX divisors. So let me
propose the below addition to this series. With the below diff, your
setup should still work with assigned clock rates, while letting us
handle our calculations dynamically.

The addition I am now proposing is to remove CLK_SET_RATE_PARENT to both
media_disp[12]_pix clocks. This should actually fix your situation while
keeping pixel clocks accurate as far it is possible as the LDB clock
will change video_pll1 only if the PLL rate is not suitable for it in
the first place. And then, there will be no other clock messing with
this PLL. This is probably a safer approach, which should still allow
accurate dynamic rate calculations for "simple" setups *and* let the
static assignations work otherwise:

-       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
+       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
[...]
-       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT | CLK_NO_RATE_CHANGE_DURING_PROPAGATION);
+       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_NO_RATE_CHANGE_DURING_PROPAGATION);

Can you please give this proposal a try?

[...]

>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>> @@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
>>         mode = &crtc_state->adjusted_mode;
>>  
>>         requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
>> +       /*
>> +        * 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);
>
> I don't think i.MX6SX LDB needs this, because the "ldb" clock's parent
> is a mux clock with "ldb_di0_div_3_5" or "ldb_di0_div_7" parents.

Ah, you mean we should only do this in the imx8 case, right?

Thanks,
Miquèl

Powered by blists - more mailing lists