Re: [PATCH v7 2/7] Revert "clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate"

[Marek Vasut <marex@denx.de>]

On 11/19/24 9:18 AM, Ying Liu wrote:

[...]

>> The TC9595 can drive an DP output, for that the clock which have to be
>> set on the LCDIF cannot be predicted, as that information comes from the
>> monitor EDID/DPCD. That is why the LCDIF has to be able to configure the
>> Video PLL1 clock to accurate clock frequency.
>>
>> For the LVDS LDB, the use case is the other way around -- the pixel
>> clock which should be generated by LCDIF and fed to LDB are known from
>> the panel type listed in DT, but they should still be accurate.
> 
> Thanks for the information.  I think the key question is whether the
> alternative solution(*) you mentioned below stands or not, in other words,
> whether LCDIF1/LCDIF2/LDB drivers know that they are sharing a PLL
> or not.

I'll continue at the end ...

>>> You still may assign an accurate PLL rate in DT.
>>> This patch only makes the PLL rate be unchangeable dynamically in
>>> runtime.  That means the existing imx8m-dhcom-som.dtsi would use
>>> IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock
>>> of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes
>>> imx8mp.dsti.  I assume it should be able to support typical video modes
>>> like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz
>>> PLL rate.
>>
>> This will break multiple DP monitors I tested so far I'm afraid. And I
>> spent a LOT of time wrestling with the TC9595 bridge to make sure it
>> actually does work well.
> 
> If the DP monitors support typical video modes like 1080p60 with
> 148.5MHz pixel clock rate, I assume these typical video modes work
> still ok with this patch at least.  Please help confirm this, since if the
> alternative solution(*) doesn't stand, we would know those video
> modes still work ok with my solution(fixed PLL rate).

They do not work with the fixed PLL setting.

>>> Granted that less video modes read from DP monitor would
>>> be supported without dynamically changeable PLL rates, this is something
>>> we have to accept because some i.MX8MP platforms(like i.MX8MP EVK)
>>> have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI
>>> display pipelines.
>>
>> What I need is the use of two full PLL1443x (like Video PLL and Audio
>> PLL1/2) , one for each display output, and those PLLs have to be fully
>> configurable to produce accurate pixel clock for each connected panel.
>> Otherwise I cannot make proper use of the video output capabilities of
>> the MX8MP SoC.
> 
> Yeah, I understand your requirements.  However, it still depends on
> whether the alternative solution(*) stands or not.

I'll continue at the end ...

>>> The missing part is that we need to do mode validation
>>> for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c
>>> to filter unsupported video mode out.  Is this missing mode validation
>>> the cause of your failure case?
>>
>> I do want to support the various modes, I do not want to filter them
>> out. They can be supported, the only "problem" is the shared Video PLL
>> which is not really an actual problem in my case, because I do not use
>> shared Video PLL, I use two separate PLLs.
>>
>> I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out
>> whether they share the Video PLL at all (you already suggested the clock
>> subsystem can provide that information), and then if:
> 
> But, how to let LCDIF1/LCDIF2/LDB drivers to figure out that?
> 
> I didn't suggest that the clock subsystem can provide that information.

... by end I mean here.

One really nasty way I can think of is -- use find_node_by_compatible(), 
look up all the relevant DT nodes, parse their clock properties, and 
check whether they all point to the Video PLL or not.

Maybe the clock subsystem has a better way, like list "neighbor" 
consumers of some specific parent clock or something like that.

[...]

>> Can something like (*) above be implemented instead, so both Shared and
>> separate PLLs would be supported ? That should solve both of our use
>> cases, right ?
> 
> I don't see any clear way to implement something like(*).
> 
> Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif
> DRM instance?  Would it be too intrusive?

Yes, and I think unnecessary, one can simply traverse and parse the DT 
to determine the clock assignment?

> Use clk_get_parent() to determine if the pixel clocks of LCDIF1&2 are
> sharing PLL(note clk_get_parent() implementation contains a TODO:
> Create a per-user clk.)?

Maybe not necessary for this case.

> How to do mode validation for the shared PLL case(note mode_valid()
> callback is supposed to look at nothing more than passed-in mode)?
> Use clk_set_rate_range() to fix the PLL rate(min == max)?

This is a good question -- we can use fixed frequency set in DT for the 
PLL in case it is shared, and set whatever optimal frequency if the PLL 
is not shared. That would be a good first step I think (**).

The next step would be to find a way to negotiate acceptable PLL 
frequency between LCDIF1/LCDIF2/LDB in case the PLL is shared, but I do 
agree this is non-trivial, hence next step.

>>> I hope that we can agree on this solution first before spreading
>>> discussions across different threads and eventually the NAK can be
>>> taken back.
>>
>> I cannot really agree on a solution which breaks one of my use cases,
>> but maybe there is an alternative how to support both options, see (*)
>> above ?
> 
> I tend to say there is no any alternative solution to satisfy both
> separate PLLs and shared PLL use cases, or even if there is one, it won't
> be easy to implement.  If you know one, please shout it out.
Maybe (*) with first step (**) would be doable ?