[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 at 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 at 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