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

[Marek Vasut]

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,
   ...
};

?

Maybe some mode flag to specify which mode is MIN/TYP/MAX would be 
useful with that change too ?

Finally, the TC358767 driver would test all modes and find the most 
accurate one ?