Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Marek, Abel,

+Cc: Miquèl

On Fri, 31 May 2024 22:26:26 +0200
Marek Vasut <marex@denx.de> wrote:

> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
> Allow these clocks to reconfigure the Video PLL, as that results in
> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
> clock accuracy is low.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

I'm afraid I just found this patch broke my previously working setup
with a panel connected on the LDB. As we are at 6.12-rc7, and this
patch got merged at 6.12-rc1, I'm reporting it immediately after a
quick preliminary analysis in order to hopefully find an appropriate
solution before 6.12.

Problem: with this patch, my LDB picture is horizontally shrunk by a
factor of about 7, measured empirically.

Configuration:
 - lcdif1 and lcdif3 disabled
 - a single-channel LVDS panel on lcdif2/ldb channel 2

So this problem looks like different from the one reported by Adam as
there a single panel and still it stops working.

The relevant rates in my case are as follows:

                            video_pll1   media_disp2_pix    media_ldb
                          ~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~
at boot, panel still off: 1.039.500.000    1.039.500.000  1.039.500.000
requested LDB clock:                                        503.300.000
before this patch:        1.039.500.000       74.250.000    519.750.000
after this patch:            71.900.000       71.900.000     71.900.000

Previously, the resulting media_ldb clock was not precise, but close
enough. Now the value is clearly too wrong to work. Also (but my memory
might be wrong here) there must be a ratio of 7 between media_ldb and
media_disp2_pix, which has become 1 after this patch, and this perhaps
explains the shrink factor of about 7.

I double checked the rate that fsl_ldb_atomic_enable() is requesting at
[0] and it is 503.3 MHz with and without the patch. This is proved by
the logged line, before and after the patch:

fsl-ldb 32ec0000.blk-ctrl:bridge@5c: Configured LDB clock (519750000 Hz) does not match requested LVDS clock: 503300000 Hz
fsl-ldb 32ec0000.blk-ctrl:bridge@5c: Configured LDB clock (71900000 Hz) does not match requested LVDS clock: 503300000 Hz

My preliminary conclusions:

 * it looks like a bug leading to an incorrect computation of the
   video_pll1 rate when media_ldb is requesting its rate
 * the bug does not look like due to this patch, but exposed by it

I still have no idea how the video_pll1 gets configured to such a low
value when its child needs a 500+ MHz clock.

Any clues or ideas would be welcome.

Best regards,
Luca

[0] https://elixir.bootlin.com/linux/v6.12-rc7/source/drivers/gpu/drm/bridge/fsl-ldb.c#L180

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com