On 10/23/2024, Marek Vasut wrote: > On 10/22/24 8:13 AM, Liu Ying wrote: > > [...] > >>>>>> This patch would cause the below in-flight LDB bridge driver >>>>>> patch[1] fail to do display mode validation upon display modes >>>>>> read from LVDS to HDMI converter IT6263's DDC I2C bus. >>>>> >>>>> Why ? >>>> >>>> Mode validation is affected only for dual LVDS link mode. >>>> For single LVDS link mode, this patch does open more display >>>> modes read from the DDC I2C bus. The reason behind is that >>>> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS >>>> link mode, while it's 7 for single LVDS link mode. >>>> >>>> In my system, "video_pll1" clock rate is assigned to 1.0395GHz >>>> in imx8mp.dtsi. For 1920x1080-60.00Hz with 148.5MHz pixel >>>> clock rate, "media_ldb" clock rate is 519.75MHz and >>>> "media_disp2_pix" clock rate is 148.5MHz, which is fine for >>>> dual LVDS link mode(x3.5). For newly opened up 1920x1080-59.94Hz >>>> with 148.352MHz pixel clock rate, "video_pll1" clock rate will >>>> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz >>>> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz >>>> too because "media_disp2_pix" clock cannot handle the 3.5 >>>> division ratio from "video_pll1_out" clock running at >>>> 519.232MHz. See the below clk_summary. >>> >>> Shouldn't this patch help exactly with that ? >> >> No, it doesn't help but only makes clk_round_rate() called in >> LDB driver's .mode_valid() against 148.352MHz return 148.352MHz >> which allows the unexpected 1920x1080-59.94Hz display mode. > > Why is 1920x1080-59.94Hz mode unexpected in the first place ? Because video PLL1 is supposed to be shared by LDB and Samsung MIPI DSI display pipelines on i.MX8MP EVK and video PLL1 clock rate won't be changed once it is assigned to 1.0395GHz in imx8mp.dtsi at least for i.MX8MP EVK. This 1.0395GHz satisfies the need to drive typical 1920x1080@60 display mode read from HDMI monitors connected to LVDS to HDMI(IT6263) and MIPI DSI to HDMI(ADV7535) transmitters. The transmitters can be connected to i.MX8MP EVK through MINI-SAS connectors. If the MINI-SAS connectors can also connect to a LVDS panel or MIPI DSI panel if the transmitters are not connected. This 1.0395GHz just doesn't satisfy 1920x1080-59.94Hz display mode with 148.352MHz pixel clock rate. > I assume your display device reports that it supports this mode, and now the scanout engine and LDB can generate this mode too ? Or does the display device NOT support this mode ? This display mode is read from a HDMI monitor's EDID, so the display device supports it for sure. The scanout engine and LDB cannot support this display mode with the fixed 1.0395GHz video PLL1 clock rate. With other particular video PLL1 clock rate, they can. > >>> It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ? >> >> Yes, it allows that for single-link LVDS use cases. >> And, __no__, for dual-link LVDS use cases because the >> video_pll1_out clock rate needs to be 2x the LVDS serial clock >> rate. > > Can't the LDB still set the Video PLL frequency to whatever it needs first, fixate it, and only then let the LCDIFv3 divide the frequency down ? (sorry, I am a bit tired today, maybe I am missing the obvious) Yes, LDB driver can set video PLL clock rate directly, but it needs to get the video PLL clock first by calling clk_get_parent(), which doesn't look nice. Let LCDIFv3 driver divide the rate down? Not easy... You know it needs to make LDB driver's atomic_enable() happen before LCDIFv3 driver's atomic_enable(). > >>>> video_pll1_ref_sel 1 1 0 24000000 0 0 50000 Y deviceless no_connection_id >>>> video_pll1 1 1 0 519232000 0 0 50000 Y deviceless no_connection_id >>>> video_pll1_bypass 1 1 0 519232000 0 0 50000 Y deviceless no_connection_id >>>> video_pll1_out 2 2 0 519232000 0 0 50000 Y deviceless no_connection_id >>>> media_ldb 1 1 0 519232000 0 0 50000 Y deviceless no_connection_id >>>> media_ldb_root_clk 1 1 0 519232000 0 0 50000 Y 32ec0000.blk-ctrl:bridge@5c ldb >>>> deviceless no_connection_id >>>> media_disp1_pix 0 0 0 129808000 0 0 50000 N deviceless no_connection_id >>>> media_disp1_pix_root_clk 0 0 0 129808000 0 0 50000 N 32e80000.display-controller pix >>>> 32ec0000.blk-ctrl disp1 >>>> deviceless no_connection_id >>>> media_disp2_pix 1 1 0 519232000 0 0 50000 Y deviceless no_connection_id >>>> media_disp2_pix_root_clk 1 1 0 519232000 0 0 50000 Y 32e90000.display-controller pix >>>> 32ec0000.blk-ctrl disp2 >>>> deviceless no_connection_id >>>> >>>> Single LVDS link mode is not affected because "media_disp2_pix" >>>> clock can handle the 7 division ratio. >>>> >>>> To support the dual LVDS link mode, "video_pll1" clock rate needs >>>> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock >>>> can use 7 division ratio to achieve the /3.5 clock rate comparing >>>> to "media_ldb" clock rate. However, "video_pll1" is not seen by >>>> LDB driver thus not directly controlled by it. This is another >>>> reason why assigning a reasonable "video_pll1" clock rate in DT >>>> makes sense. >>> >>> I agree that _right_now_, the DT clock assignment is the only option. >>> I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves. >> >> I think we'll live with the assigned clock rate in DT, because the >> i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a >> video PLL, like I mentioned in comments for patch 2. > > They do NOT need to share a PLL, you can use e.g. PLL3 for one and Video PLL for the other if the requirement is accurate pixel clock . I need to share the video PLL clock on i.MX8MP EVK for LDB and Samsung MIPI DSI display pipelines. PLL3 is supposed to be the parent clock of audio AXI clock at least on i.MX8MP EVK. All other clocks in imx8mp_media_disp_pix_sels[] are not appropriate to be used by the display pipelines, except "video_pll1_out", at least for i.MX8MP EVK. > > [...] -- Regards, Liu Ying
