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

[Marek Vasut <marex@denx.de>]

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 .