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

[Isaac Scott <isaac.scott@ideasonboard.com>]

On Wed, 2024-10-09 at 17:47 +0200, Marek Vasut wrote:
> On 10/9/24 11:55 AM, Isaac Scott wrote:
> > On Tue, 2024-10-08 at 23:48 +0200, Marek Vasut wrote:
> > > On 10/8/24 12:07 PM, Isaac Scott wrote:
> > > > On Mon, 2024-10-07 at 20:06 +0200, Marek Vasut wrote:
> > > > > On 10/7/24 7:01 PM, Isaac Scott wrote:
> > > > > > Hi Marek,
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > > On Sat, 2024-07-06 at 02:16 +0200, Marek Vasut wrote:
> > > > > > > On 6/24/24 11:19 AM, Alexander Stein wrote:
> > > > > > > > Am Freitag, 31. Mai 2024, 22:27:21 CEST schrieb Marek
> > > > > > > > Vasut:
> > > > > > > > > In case an upstream bridge modified the required
> > > > > > > > > clock
> > > > > > > > > frequency
> > > > > > > > > in its .atomic_check callback by setting
> > > > > > > > > adjusted_mode.clock
> > > > > > > > > ,
> > > > > > > > > make sure that clock frequency is generated by the
> > > > > > > > > LCDIFv3
> > > > > > > > > block.
> > > > > > > > > 
> > > > > > > > > This is useful e.g. when LCDIFv3 feeds DSIM which
> > > > > > > > > feeds
> > > > > > > > > TC358767
> > > > > > > > > with (e)DP output, where the TC358767 expects precise
> > > > > > > > > timing
> > > > > > > > > on
> > > > > > > > > its input side, the precise timing must be generated
> > > > > > > > > by
> > > > > > > > > the
> > > > > > > > > LCDIF.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > > > > 
> > > > > > > > With the other rc358767 patches in place, this does the
> > > > > > > > trick.
> > > > > > > > Reviewed-by: Alexander Stein
> > > > > > > > <alexander.stein@ew.tq-group.com>
> > > > > > > 
> > > > > > > I'll pick this up next week if there is no objection.
> > > > > > 
> > > > > > Unfortunately, this has caused a regression that is present
> > > > > > in
> > > > > > v6.12-
> > > > > > rc1 on the i.MX8MP PHYTEC Pollux using the
> > > > > > arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-
> > > > > > rdk.dts.
> > > > > > The
> > > > > > display is the edt,etml1010g3dra panel, as per the upstream
> > > > > > dts. We
> > > > > > bisected to this commit, and reverting this change fixed
> > > > > > the
> > > > > > screen.
> > > > > > 
> > > > > > We then tried to retest this on top of v6.12-rc2, and found
> > > > > > we
> > > > > > also
> > > > > > had
> > > > > > to revert commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155
> > > > > > ("clk:
> > > > > > imx:
> > > > > > clk-imx8mp: Allow media_disp pixel clock reconfigure parent
> > > > > > rate")
> > > > > > alongside this. Reverting these two commits makes the
> > > > > > display
> > > > > > work
> > > > > > again at -rc2.
> > > > > > 
> > > > > > Do you have any suggestions on anything we might be missing
> > > > > > on
> > > > > > our
> > > > > > end?
> > > > > > Please let me know if there's anything you'd like me to
> > > > > > test as
> > > > > > I'm
> > > > > > not
> > > > > > sure what the underlying fault was here.
> > > > > I believe what is going on is that the LCDIF cannot configure
> > > > > its
> > > > > upstream clock because something else is already using those
> > > > > clock
> > > > > and
> > > > > it set those clock to a specific frequency. LCDIF is now
> > > > > trying
> > > > > to
> > > > > configure those clock to match the LVDS panel, and it fails,
> > > > > so
> > > > > it
> > > > > tries
> > > > > to set some approximate clock and that is not good enough for
> > > > > the
> > > > > LVDS
> > > > > panel.
> > > > > 
> > > > > Can you share dump of /sys/kernel/debug/clk/clk_summary on
> > > > > failing
> > > > > and
> > > > > working system ? You might see the difference around the
> > > > > "video"
> > > > > clock.
> > > > > 
> > > > > (I have seen this behavior before, the fix was usually a
> > > > > matter
> > > > > of
> > > > > moving one of the LCDIFs to another upstream clock like PLL3,
> > > > > so
> > > > > it
> > > > > can
> > > > > pick well matching output clock instead of some horrid
> > > > > approximation
> > > > > which then drives the panel likely out of specification)
> > > > 
> > > > Hi Marek,
> > > > 
> > > > Please find attached the clk_summary for v6.12-rc2 before and
> > > > after
> > > > the
> > > > reversion (the one after the reversion is 6.12-
> > > > rc2_summary_postfix).
> > > Thank you, this helped greatly.
> > > 
> > > I believe I know why it used to kind-of work for you, but I'm
> > > afraid
> > > this used to work by sheer chance and it does not really work
> > > correctly
> > > for the panel you use, even if the panel likely does show the
> > > correct
> > > content. But, there is a way to make it work properly for the
> > > panel
> > > you use.
> > > 
> > > First of all, the pixel clock never really matched the panel-
> > > simple.c
> > > pixel clock for the edt_etml1010g3dra_timing:
> > > 
> > > $ grep '\<media_disp2_pix\>' 6.12-rc2_summary_postfix
> > >     media_disp2_pix  1  1  0  74250000 ...
> > >                               ^^^^^^^^
> > > 
> > > $ grep -A 1 edt_etml1010g3dra_timing drivers/gpu/drm/panel/panel-
> > > simple.c
> > > static const struct display_timing edt_etml1010g3dra_timing = {
> > >           .pixelclock = { 66300000, 72400000, 78900000 },
> > >                                     ^^^^^^^^
> > > 
> > > The pixel clock are within tolerance, but there is a discrepancy
> > > 74250000 != 72400000 .
> > > 
> > > Since commit 94e6197dadc9 ("arm64: dts: imx8mp: Add LCDIF2 & LDB
> > > nodes")
> > > the IMX8MP_VIDEO_PLL1_OUT is set to a very specific frequency of
> > > 1039500000 Hz, which tidily divides by 2 to 519750000 Hz (which
> > > is
> > > your
> > > LVDS serializer frequency) and divides by 7 to 74250000 Hz which
> > > is
> > > your
> > > LCDIF pixel clock.
> > > 
> > > 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>;
> > > };
> > > 
> > > Can you please test whether this works and the pixel clock are
> > > accurate
> > > in /sys/kernel/debug/clk/clk_summary ?
> > 
> > Interestingly, after making the change you suggested to imx8mp-
> > phyboard-pollux-rdk.dts before the two reversions, the display now
> > seems to work. Please see below for the relevant section of the new
> > clk_summary referring to media_disp2_pix:
> > 
> > video_pll1_ref_sel               1       1        0        24000000
> > 0          0     50000      Y      deviceless
> > no_connection_id
> >         video_pll1                    1       1        0
> > 506800000   0          0     50000      Y         deviceless
> > no_connection_
> >            video_pll1_bypass          1       1        0
> > 506800000   0          0     50000      Y            deviceless
> > no_connecti
> >               video_pll1_out          2       2        0
> > 506800000   0          0     50000      Y               deviceless
> > no_conne
> >                  media_ldb            1       1        0
> > 506800000   0          0     50000      Y                 
> > deviceless
> > no_co
> >                     media_ldb_root_clk 1       1        0
> > 506800000   0          0     50000      Y
> > 32ec0000.blk-ctrl:bridge@5c     l
> >                                                                    
> >     
> > deviceless                      no
> >                  media_disp2_pix      1       1        0       
> > 72400000
> > 0          0     50000      Y                  deviceless
> > no_co
> >                     media_disp2_pix_root_clk 1       1        0
> > 72400000    0          0     50000      Y
> > 32e90000.display-controller
> >                                                                    
> >     
> > 32ec0000.blk-ctrl               di
> >                                                                    
> >     
> > deviceless                      no
> >                  media_disp1_pix      0       0        0
> > 506800000   0          0     50000      N                 
> > deviceless
> > no_co
> >                     media_disp1_pix_root_clk 0       0        0
> > 506800000   0          0     50000      N
> > 32ec0000.blk-ctrl
> >                                                                    
> >     
> > deviceless                      no
> >                  media_mipi_phy1_ref  0       0        0       
> > 23036364
> > 0          0     50000      N                  deviceless
> > no_co
> >                     media_mipi_phy1_ref_root 0       0        0
> > 23036364    0          0     50000      N
> > 32ec0000.blk-ctrl
> > 
> > The media_disp2_pix clock now seems to be correct at 724000000
> > after
> > your changes.
> 
> Do you want to submit the DT patch with correct Fixes: tag ? :)

I thought this wasn't needed with the other two patches?