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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux