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

[Dmitry Baryshkov]

On Tue, 26 Nov 2024 at 18:00, Maxime Ripard <mripard at kernel.org> wrote:
>
> On Tue, Nov 26, 2024 at 12:07:10AM +0200, Dmitry Baryshkov wrote:
> > On Mon, 25 Nov 2024 at 15:17, Maxime Ripard <mripard at kernel.org> wrote:
> > >
> > > 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.
> >
> > Well... I think it's messy because we have so many different
> > interfaces. Some encoders can poke directly into the panel, some
> > drivers use bridge chains and panel bridge. Some do even a messier
> > thing and try both at the same time.
> > I think that in the long-term nobody should be using the drm_panel
> > interface directly.
>
> There's a few corner cases that are doable with the panel API that
> aren't possible with the bridge API still. Being able to get the pixel
> clock you're going to get from the encoder and adjust the timings to
> match the panel tolerance is one for example, and we have a couple of
> drivers doing that.

My initial point was that only panel-simple (and panel-edp) implement
the get_timings().
And for the existing encoder drivers I think we should be migrating to
using bridge API instead of panel API.

>
> > > 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.
> >
> > ... and then it might help with the command-mode DSI panels with DSC...
>
> How so?

We had troubles with CMD panels, because we need to get the mode
timings based on FPS and compression ratio (so it can  not be
pregrorammed, like we usually do for most of the panels).

>
> > >
> > > > 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*.
> >
> > Sounds good to me too.
> > I'm not sure what it should look like though. We barely scratched our
> > heads when looking at the DSC / CMD stuff, but nothing came out of it.
> >
> > Ideally... there should be some kind of get_timings being available
> > through the full bridge chain, so that the encoders can use it. But
> > I'm not sure how that should work, because some bridges would like to
> > manipulate those timings. And some bridges will completely drop
> > get_timings() and produce raw modes even after consuming the timings
> > set.
>
> the drm_display_mode -> timings conversion is fairly easy to do: just
> use the same values for min, typ and max.

-- 
With best wishes
Dmitry