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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3240 bytes --]

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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]