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

[Maxime Ripard]

On Fri, Nov 22, 2024 at 03:32:57PM +0200, 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).

I think we should do the opposite :)

Panels usually have a pretty large operating window, and the current
construct only works because nobody really uses the same panels (or
we're hiding that behind different compatibles) across SoCs or
generation. Or we're working around it.

It's kind of a mess, and it gets messy in encoders too: some will filter
out panel modes, some won't. Some will adjust timings to fit their
clocks, some won't. etc.

Going forward, switching everyone to a timing-like interface and
providing a set of helpers to adjust timings within possible boundaries
to fit a clock rate is doable and should be done imo.

> 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().

It's unused because we don't have an easy API for encoders to use it. We
should fix *that*.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20241125/b08f3438/attachment.sig>