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

[Ying Liu]

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

It looks like you are still sticking to the idea of supporting all potentially
valid video modes by trying to find an "alternative" solution, while
neglecting that the solution *could be* never working. 

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

If the "alternative" solution doesn't work, we'll end up using the "fixed
PLL rate" solution.  It that case, some video modes would be filtered
out as a sacrifice. 

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

No good idea from me.

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

A mighty virtual clock?  I'm not sure if that would work or not.

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

Not sure if it would work or not, but I'm sure that one single DRM instance
means atomic check/commit for the display pipelines as a whole, hence
awareness of active display pipeline number in an atomic way.

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

Regards,
Liu Ying