Message-ID: <7ceae5ec-200d-46fc-acc3-a4aff371937e@nxp.com>
Date: Wed, 27 Nov 2024 15:24:51 +0800
From: Liu Ying <victor.liu@....com>
To: Miquel Raynal <miquel.raynal@...tlin.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

On 11/26/2024, Miquel Raynal wrote:
> Hi Liu,

Hi,

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

i.MX8MP EVK[1] supports MIPI DSI, LVDS and native HDMI outputs.
MIPI DSI and LVDS outputs use MINI-SAS connectors and HDMI output
uses a HDMI type A connector.
You may easily find the connectors in the EVK pictures in [1].
IMX-MIPI-HDMI(ADV7535), IMX-LVDS-HDMI(IT6263 single LVDS link) and
MX8-DSI-OLED1A(RAYDIUM RM67199 DSI panel) accessory boards[2] can
be connected/plugged to the EVK through the MINI-SAS connectors.
In addition, MX8-DSI-OLED1(RAYDIUM RM67191 DSI panel),
IMX-DLVDS-HDMI(IT6263 single DLVDS link) and
MX8-DLVDS-LCD1(koe,tx26d202vm0bwa dual-link LVDS panel) can also
be connected to the EVK through the MINI-SAS connectors, though
not listed in [2].

So, putting native HDMI output aside, there are quite a few MIPI
DSI + LVDS display device combinations to be supported with
a single video PLL.

For ADV7535 and IT6263, they support typical 1080p/720p video modes
read from EDID with 148.5MHz and 74.25MHz pixel clock rates.

For MX8-DLVDS-LCD1 LVDS panel, it is supported with in-tree
imx8mp-evk-mx8-dlvds-lcd1.dtso where nominal 156.72MHz pixel clock
rate is overridden to be 148.5MHz with a panel-timing DT node,
considering the fixed video PLL rate @1.0395GHz assigned in
imx8mp.dtsi.

For MX8-DSI-OLED1 and MX8-DSI-OLED1A MIPI DSI panels, both have
nominal 132MHz pixel clock rate(see panel-raydium-rm67191.c). NXP
downstream kernel uses 148.5MHz pixel clock rate for the two panel.

In short, I use 148.5MHz or 74.25MHz for ADV7535, IT6263 and the
LVDS panel.  I haven't tried the MIPI DSI panels with upstream
kernel yet.

[1] https://www.nxp.com/design/design-center/development-boards-and-designs/i-mx-evaluation-and-development-boards/evaluation-kit-for-the-i-mx-8m-plus-applications-processor:8MPLUSLPD4-EVK
[2] https://www.nxp.com/design/design-center/development-boards-and-designs/i-mx-evaluation-and-development-boards/i-mx-8-series-accessory-boards:i.MX8-ACCESSORY-BOARDS

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

Removing CLK_SET_RATE_PARENT from the two pixel clocks is also done
with my patch[3]. 

[3] https://patchwork.freedesktop.org/patch/624509/?series=139266&rev=7

> 
> Can you please give this proposal a try?

I tried this/your patch set + your additional patch(remove
CLK_SET_RATE_PARENT for pixel clocks) + your additional patch(set LDB
clock rate to 2x requested link rate for dual-link LVDS)[4] + my two
patches[5][6] with ADV7535 + IT6263 on i.MX8MP EVK.  I don't see
any particular issue for both single and dual link LVDS cases. Some
typical video modes(1080p60/50, 720p60) work ok for ADV7535, though
there is still no display for a few video modes(should be not caused
by your patch set because there is no mode validation logic for the
MIPI DSI display pipeline).  So, basically, this has the same test
result if simply only applying my patch set[7].

I think the key change for your patch set to work for my test case
is your additional patch(remove CLK_SET_RATE_PARENT for pixel clocks).
Together with [4][5][6], the video PLL rate is fixed to 1.0395GHz for
all video modes.

Back to the separated/shared PLLs problem, I still don't see any
feasible alternative solution till now, since you are proposing to
remove CLK_SET_RATE_PARENT for pixel clocks which defeats the idea
of dynamically changing PLL rate(at least for the MIPI DSI display
pipeline).

Also, I feel that CLK_NO_RATE_CHANGE_DURING_PROPAGATION is not
needed, because the pixel clock rate can be set in
fsl_ldb_atomic_enable() as I mentioned before.

[5] https://patchwork.freedesktop.org/patch/624511/?series=139266&rev=7
[6] https://patchwork.freedesktop.org/patch/624512/?series=139266&rev=7
[7] https://patchwork.freedesktop.org/series/139266/#rev7

> 
> [...]
> 
>>> --- 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?

I think that doing this does no harm for both i.MX8MP LDB and i.MX93
LDB.

> 
> Thanks,
> Miquèl

-- 
Regards,
Liu Ying

Powered by blists - more mailing lists