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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 ?



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux