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

[Ying Liu]

On 11/20/24, Marek Vasut wrote:
> 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.

Why?  Did you assign a sensible fixed PLL rate in DT?
Can you please compare clk_summary output for the failing cases
before and after this patch is applied? I assume that if you use
the fixed PLL rate same to the rate which works before this patch is
applied, the typical video modes still just work after this patch is
applied.

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

That's nasty.  It looks even more nasty when considering the fact that
i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF
needs the nasty check, because i.MX93 SoC embeds only one LCDIF.

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

What will imx-lcdif DRM look like by using this way? Get the ancestor PLL
clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks
(media_disp1_pix and/or media_disp2_pix + other possible clocks) of the
PLL clock in a string array and find media_disp1_pix + media_disp2_pix
in it?

Doesn't look nice, either.

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

Yes, people can traverse and parse DT, but it's nasty.

In addition, one may argue that now that CLK_SET_RATE_PARENT flag
is set for the pixel clocks, all potential video modes read from EDID
should be supported when only either LVDS display pipeline or MIPI DSI
display pipeline is active in the shared PLL case.  This requires one
single DRM instance to detect single or dual active display pipelines
dynamically, hence this single DRM instance becomes necessary.

> 
> > 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 above requirement of dynamical active display pipeline number
detection defeats the fixed PLL rate for in the shared PLL case. 
And, it makes mode validation kind of undoable, because mode_valid()
is not supposed to know that active number.

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

Maybe it's not doable if the above requirement of dynamical active display
pipeline number detection needs to be supported.

Considering 1) the way of getting separate/shared PLL information and
2) mode validation, I don't think your overall alternative solution is good.

Regards,
Liu Ying