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

[Marek Vasut]

On 11/20/24 7:38 AM, Ying Liu wrote:

[...]

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

Whatever was in imx8mp.dtsi does not really work for all the panels.
Please keep in mind that the use case I have does not include only 
1920x1080 "standard" panels, but also other resolutions.

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

I'm afraid I do not need to support only typical video modes, but also 
the other "atypical" modes.

[...]

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

The check can be skipped based on compatible string.

I agree it is nasty, but it is a start. Are there better ideas ?

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

One other option came to my mind -- place a virtual clock between the 
Video PLL and consumers (LCDIF1/2/LDB), and then have the virtual clock 
driver do the clock rate negotiation in some .round_rate callback. That 
is also nasty, but it is another idea. If there is a clock specifically 
implemented to negotiate best upstream clock rate for all of its 
consumers, and it is aware of the consumer behavior details and 
requirements, maybe that could work ?

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

Would single virtual clock which do the frequency negotiation between 
multiple DRM consumers work too ?

I do not have much to add to the points below.