[PATCH v2 2/2] drm/bridge: tc358767: Improve DPI output pixel clock accuracy

[Marek Vasut]

On 11/26/24 4:56 PM, Maxime Ripard wrote:
> On Tue, Nov 26, 2024 at 12:48:20AM +0100, Marek Vasut wrote:
>> On 11/22/24 2:32 PM, Dmitry Baryshkov wrote:
>>> On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote:
>>>> The Pixel PLL is not very capable and may come up with wildly inaccurate
>>>> clock. Since DPI panels are often tolerant to slightly higher pixel clock
>>>> without being operated outside of specification, calculate two Pixel PLL
>>>> from either mode clock or display_timing .pixelclock.max , whichever is
>>>> higher. Since the Pixel PLL output clock frequency calculation always
>>>> returns lower frequency than the requested clock frequency, passing in
>>>> the higher clock frequency should result in output clock frequency which
>>>> is closer to the expected pixel clock.
>>>>
>>>> For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency
>>>> without this patch is 65 MHz which is out of the panel specification of
>>>> 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within
>>>> the specification and far more accurate.
>>>
>>> Granted that most of the panel drivers do not implement get_timings()
>>> and granted that there are no current users of that interface, I think
>>> we should move away from it (and maybe even drop it completely from
>>> drm_panel).
>>
>> It does fit DPI and LVDS panels and their descriptions in datasheets the
>> best.
>>
>>> What about achieving the same via slightly different approach: register
>>> a non-preferred mode with the clock equal to the max clock allowed. The
>>> bridge driver can then use the default and the "max" mode to select PLL
>>> clock.
>>>
>>> I understand that this suggestion doesn't follow the DPI panel
>>> specifics, which are closer to the continuous timings rather than fixed
>>> set of modes, however I really don't think that it's worth keeping the
>>> interface for the sake of a single driver. Original commit 2938931f3732
>>> ("drm/panel: Add display timing support") from 2014 mentions using those
>>> timings for .mode_fixup(), but I couldn't find a trace of the
>>> corresponding implementation.
>>>
>>> Another possible option might be to impletent adjusting modes in
>>> .atomic_check() / .mode_fixup().
>> Something like this ?
>>
>> static const struct display_timing chefree_ch101olhlwh_002_timing = {
>>    .pixelclock = { 68900000, 71100000, 73400000 },
>>    ...
>> };
>>
>> static const struct panel_desc chefree_ch101olhlwh_002 = {
>>    .timings = &chefree_ch101olhlwh_002_timing,
>>    .num_timings = 1,
>>    ...
>> };
>>
>> ... would turn into ...
>>
>> static const struct drm_display_mode chefree_ch101olhlwh_002_mode[3] = {
>>    {
>>      .clock = 68900000,
>>      ...
>>    }, {
>>      .clock = 71100000,
>>      ...
>>    }, {
>>      .clock = 73400000,
>>      ...
>>    }
>> };
>>
>> static const struct panel_desc chefree_ch101olhlwh_002 = {
>>    .modes = &chefree_ch101olhlwh_002_mode,
>>    .num_timings = 3,
>>    ...
>> };
>>
>> ?
> 
> Except that doesn't work if you want to keep your driver at the expected
> framerate. To reduce the pixel clock, you also need to reduce the
> blanking period within the acceptable boundaries if you want to keep the
> same framerate.
Each mode entry would have to have its own full set of timings of 
course, not just .clock . Those can be converted from display_timing 
just like clock.