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

[Marek Vasut]

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.

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

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

> 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:
- 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 ? (*)

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