[PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

[Kieran Bingham]

Quoting Marek Vasut (2024-10-12 21:37:59)
> On 10/11/24 5:10 AM, Liu Ying wrote:
> 
> Hi,
> 
> >>>> This Video PLL1 configuration since moved to &media_blk_ctrl {} , but it is still in the imx8mp.dtsi . Therefore, to make your panel work at the correct desired pixel clock frequency instead of some random one inherited from imx8mp.dtsi, add the following to the pollux DT, I believe that will fix the problem and is the correct fix:
> >>>>
> >>>> &media_blk_ctrl {
> >>>>      // 506800000 = 72400000 * 7 (for single-link LVDS, this is enough)
> >>>>      // there is no need to multiply the clock by * 2
> >>>>      assigned-clock-rates = <500000000>, <200000000>, <0>, <0>, <500000000>, <506800000>;
> >>>
> >>> This assigns "video_pll1" clock rate to 506.8MHz which is currently not
> >>> listed in imx_pll1443x_tbl[].
> >>
> >> Since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") the 1443x PLLs can be configured to arbitrary rates which for video PLL is desirable as those should produce accurate clock.
> > 
> > Ack.
> > 
> >>
> >>> Does the below patch[1] fix the regression issue? It explicitly sets
> >>> the clock frequency of the panel timing to 74.25MHz.
> >>>
> >>> [1] https://patchwork.freedesktop.org/patch/616905/?series=139266&rev=1
> >> That patch is wrong, there is an existing entry for this panel in panel-simple.c which is correct and precise, please do not add that kind of imprecise duplicate timings into DT.
> > 
> > At least the patch[1] is legitimate now to override the display
> > timing of the panel because the override mode is something
> > panel-simple.c supports.
> 
> It may be possible to override the mode, but why would this be the 
> desired if the panel-simple.c already contains valid accurate timings 
> for this particular panel ?

I'm confused a little here. Why is it that setting the panel timings in
the DT as per [1] make the display work, while the panel timeings in
panel-simple alone are not enough?

Is there some difference in code path for 'how' the panel timings are
set as to whether they will apply fully or not ?


--
Kieran


> 
> > And, pixel clock @74.25MHz is not out
> > of the panel specification since edt_etml1010g3dra_timing
> > indicates the minimum as 66.3MHz and the maximum as 78.9MHz.
> 
> The panel-simple.c already contains timings for this panel, which are 
> accurate and follow the panel datasheet. If the goal here is to support 
> approximate panel timings between the currently available three options 
> (min/typ/max) listed in panel-simple, then that is another discussion 
> for another patch.
> 
> > Furthermore, if "PHYTEC phyBOARD-Pollux i.MX8MP" also supports
> > something like MIPI DSI to HDMI, then 74.25MHz panel pixel clock
> > rate is more desirable because the LVDS display and the MIPI DSI
> > display pipeline with typical 148.5MHz/74.25MHz pixel clock rates
> > may use one single "video_pll1" clock.
> 
> I actually do have exactly this use case on one system -- one LVDS panel 
> and one MIPI DSI panel -- the solution is really easy, source the LVDS 
> pixel clock from Video PLL and the DSI clock from e.g. PLL3 .
> 
> > Anyway, I think it is ok to use the patch[1] or assigning
> > "video_pll1" clock rate to 506.8MHz in DT(no things like MIPI
> > DSI to HDMI in existing DT).
> I believe for the current release, it is better to update the Video PLL 
> clock in this one board DT, because that is minimum impact change 
> isolated to this board and fixes a real issue where the LVDS panel 
> worked within specification only by sheer chance.