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

[Ying Liu]

On 11/19/24, Marek Vasut wrote:
> On 11/18/24 4:54 AM, Ying Liu wrote:
> > Hi Marek,
> 
> Hi,
> 
> >>> media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3
> >>> display controller, while media_disp2_pix clock is the pixel clock of
> >>> the second i.MX8MP LCDIFv3 display controller.  The two display
> >>> controllers connect with Samsung MIPI DSI controller and LVDS Display
> >>> Bridge(LDB) respectively.  Since the two display controllers are driven
> >>> by separate DRM driver instances and the two pixel clocks may be
> derived
> >>> from the same video_pll1_out clock(sys_pll3_out clock could be already
> >>> used to derive audio_axi clock), there is no way to negotiate a
> dynamically
> >>> changeable video_pll1_out clock rate to satisfy both of the two display
> >>> controllers.  In this case, the only solution to drive them with the
> >>> single video_pll1_out clock is to assign a sensible/unchangeable clock
> >>> rate for video_pll1_out clock.  Thus, there is no need to set the
> >>> CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then.
> >>>
> >>> Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock
> >> reconfigure parent rate")
> >>> Signed-off-by: Liu Ying <victor.liu at nxp.com>
> >> Uh, I almost missed this revert between all the LDB patches.
> >>
> >> This revert will break my usecase on MX8MP where I need to operate two
> >> disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I
> >> need accurate pixel clock for both. Not being able to configure accurate
> >> pixel clock will make the displays not work, so from my side, this is a
> >> NAK, sorry.
> >
> > Is your usecase in upstream kernel? If yes, which DT file implements the
> > usecase?  I guess it's im8mp-dhcom-som.dtsi authored by you, but it only
> > contains the DT node for TC358767, but not LVDS panel.
> >
> > Can you please elaborate about the failure case?
> 
> 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.

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

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

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

> - yes, agree on some sort of middle-ground frequency to configure into
>    the Video PLL, frequency which somehow fits all three consumers
>    (LCDIF1,LCDIF2,LDB)
> - no, configure each consumer upstream clock to generate accurate pixel
>    clock for that consumer
> 
> Something like ^ would make MX8MP EVK (the "yes" case) with shared Video
> PLL work, without breaking my use case (the "no" case), right ? (*)

In an ideal world, right.  But, again how to let LCDIF1/LCDIF2/LDB drivers
to figure out that they are sharing a PLL?  

> 
> >> There has to be some better solution which still allows the PLL
> >> reconfiguration to achieve accurate pixel clock.
> >
> > As I mentioned in cover letter, the only solution to support LVDS and
> > MIPI DSI displays on all i.MX8MP platforms is to assign a sensible and
> > unchangeable PLL rate in DT.
> 
> I am currently using Video PLL and Audio PLL to drive DSI and LVDS
> outputs from each, so no, fixed Video PLL assignment in DT is not the
> only solution.
> 
> > Some platforms may use two separate
> > PLLs for the LVDS and MIPI DSI display pipelines, while some others
> > have to use only the single IMX8MP_VIDEO_PLL1_OUT because
> > all other eligible PLLs are used up.  That's all fine, just being platforms
> > dependent.  The only limitation of the solution is that some platforms
> > couldn't support some particular LVDS and MIPI DSI displays at the
> > same time due to lack of PLLs, but this has to be accepted since
> > the shared IMX8MP_VIDEO_PLL1_OUT case needs to be supported and
> > the two display pipelines are not aware of each other from kernel's
> > point of view.
> 
> 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?

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

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

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

Regards,
Liu Ying