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

[Maxime Ripard]

On Sun, Oct 20, 2024 at 04:49:29AM +0200, Marek Vasut wrote:
> On 10/19/24 11:49 PM, Kieran Bingham wrote:
> > 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 ?
> Because [1] sets inaccurate pixel clock of 74.25 MHz, which can be divided
> down from random default Video PLL setting of 1039.5 MHz set in imx8mp.dtsi
> media_blk_ctrl , 1039.5 / 74.25 = 14 .
> 
> The panel-simple pixel clock are 72.4 MHz, to achieve that accurately, it is
> necessary to reconfigure the Video PLL frequency to 506.8 MHz , which
> LCDIFv3 can do, but LDB can not, hence it has to be done in DT for now.

That the clock driver is broken and thus should be fixed through the DT is a
bit backward, don't you think?

AFAIU, the clock can't reach the ideal pixel clock panel-simple has. Do
you have the datasheet for that panel?

If so, using display_timings and shortening/extending the blanking
timings to match the clock that can be provided seems like a more robust
solution.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20241021/bb2f9107/attachment.sig>