Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Nikolaus Voss]

Hi Marek,

On 09.12.2024 22:51, Marek Vasut wrote:
> On 12/9/24 10:27 AM, Nikolaus Voss wrote:
> > On 07.12.2024 12:46, Marek Vasut wrote:
> > > On 12/4/24 11:40 AM, Nikolaus Voss wrote:
> > > > > > LDB clock has to be a fixed multiple of the pixel clock.
> > > > > > As LDB and pixel clock are derived from different clock sources
> > > > >
> > > > > Can you please share the content of 
> > > > > /sys/kernel/debug/clk/clk_summary ?
> > > >
> > > > Sure. Without my patch:
> > > >
> > > >      video_pll1_ref_sel               1       1        0 24000000    
> > > > 0          0     50000      Y      deviceless no_connection_id
> > > >         video_pll1                    1       1        0 1039500000 
> > > > 0          0     50000      Y         deviceless no_connection_id
> > > >            video_pll1_bypass          1       1        0 1039500000 
> > > > 0          0     50000      Y            deviceless no_connection_id
> > > >               video_pll1_out          2       2        0 1039500000 
> > > > 0          0     50000      Y               deviceless 
> > > > no_connection_id
> > > >                  media_ldb            1       1        0 346500000   
> > > > 0          0     50000      Y 32ec0000.blk- ctrl:bridge@5c     ldb
> > > >                                                   deviceless 
> > > > no_connection_id
> > > >                     media_ldb_root_clk 0       0        0 346500000 
> > > > 0          0     50000      Y                     deviceless 
> > > >                      no_connection_id
> > > >                  media_disp2_pix      1       1        0 51975000    
> > > > 0          0     50000      Y                  deviceless        
> > > > no_connection_id
> > > >                     media_disp2_pix_root_clk 1       1        0 
> > > > 51975000    0          0     50000      Y 32e90000.display- 
> > > > controller     pix
> > > >
> > > > Here 346500000 (media_ldb) != 7 * 51975000 (media_disp2_pix)
> > > >    -> distorted panel image (if any).
> > > > The requested panel pixel clock from EDID is 51200000.
> > >
> > > Right, this is what Miquel is trying to solve with their series.
> > >
> > > > This is the same with my patch:
> > > >
> > > >      video_pll1_ref_sel               1       1        0 24000000    
> > > > 0          0     50000      Y      deviceless no_connection_id
> > > >         video_pll1                    1       1        0 1039500000 
> > > > 0          0     50000      Y         deviceless no_connection_id
> > > >            video_pll1_bypass          1       1        0 1039500000 
> > > > 0          0     50000      Y            deviceless no_connection_id
> > > >               video_pll1_out          2       2        0 1039500000 
> > > > 0          0     50000      Y               deviceless 
> > > > no_connection_id
> > > >                  media_ldb            1       1        0 346500000   
> > > > 0          0     50000      Y 32ec0000.blk- ctrl:bridge@5c     ldb
> > > >                                                   deviceless 
> > > > no_connection_id
> > > >                     media_ldb_root_clk 0       0        0 346500000 
> > > > 0          0     50000      Y                     deviceless 
> > > >                      no_connection_id
> > > >                  media_disp2_pix      1       1        0 49500000    
> > > > 0          0     50000      Y                  deviceless        
> > > > no_connection_id
> > > >                     media_disp2_pix_root_clk 1       1        0 
> > > > 49500000    0          0     50000      Y 32e90000.display- 
> > > > controller     pix
> > > >
> > > > So, here 346500000 (media_ldb) = 7 * 49500000 (media_disp2_pix).
> > > >    -> stable panel image, but pixel clock reduced to 49.5 MHz from 
> > > > requested 51.2 MHz.
> > >
> > > Inaccurate pixel clock and non-60Hz frame rate is not a win either.
> >
> > Some percents of deviation is usually not visible.
>
> The PLL is accurate, so this kind of non-60 Hz frame rate compromise
> really should not be necessary.
>
> > > > My conclusion: The clock source is the same
> > >
> > > I agree .
> > >
> > > You wrote "derived from different clock sources" above,
> > > keyword:different, which is not correct.
> > >
> > > > , nevertheless the
> > > > ldb/pixel clock constraint cannot be satisfied without either
> > > > modifying the pll clock or the pixel clock.
> > > In this particular case, you surely do want to modify the PLL 
> > > settings
> > > to achieve accurate pixel clock.
> >
> > No, in this case there is a 3 percent deviation, resulting in 58 Hz
> > frame rate instead of 60 Hz.
> Consider e.g. 60 FPS video playback, on 58 Hz refresh panel it will
> suffer from some stutter . It is better to aim for the 60 Hz then .

This is a relevant use case, I agree. But even with a dedicated video
PLL (what a luxury in the embedded world!) you will eventually have to
drop or double a frame as the clocks are asynchronous. And the sync
problem still exists with 25 or 50 FPS videos.

--
Nikolaus Voss