[PATCH v1 07/26] drm/panel: remove get_timings

[Laurent Pinchart]

Hi Maxime,

On Wed, Dec 04, 2019 at 09:16:50AM +0100, Maxime Ripard wrote:
> On Tue, Dec 03, 2019 at 04:20:24PM +0100, Linus Walleij wrote:
> > On Tue, Dec 3, 2019 at 8:47 AM Maxime Ripard wrote:
> >
> > > Using only the mode as we do currently has a bunch of shortcomings as
> > > almost no encoder will be able to provide the typical pixel clock, and
> > > that situation leads to multiple things:
> > >
> > >   - If someone working on one encoder wants to upstream a panel they
> > >     have tested, chances are this will not be the typical pixel clock
> > >     / timings being used but rather the one that will match what that
> > >     SoC is capable of. Trouble comes when a second user comes in with
> > >     a different encoder and different capabilities, and then we have a
> > >     maintainance fight over which timing is the true timing (with a
> > >     significant chance that none of them are).
> > >
> > >   - If we can't match the pixel clock, we currently have no easy way
> > >     to make the usual measures of reducing / growing the porches and
> > >     blankings areas to match the pixel clock we can provide, since we
> > >     don't have an easy way to get the tolerance on those timings for a
> > >     given panel. There's some ad hoc solutions on some drivers (I
> > >     think vc4 has that?) to ignore the panel and just play around with
> > >     the timings, but I think this should be generalised.
> >
> > I've been confused with these things as they look today and it seems
> > the whole struct drm_display_mode could need some improvement?
> >
> > If .clock is supposed to be htotal * vtotal * vrefresh, what is the
> > .clock doing there anyway.
> 
> It's one thing I wonder as well. I guess it's just more convenient for
> everyone, since it's exposed by the VESA modes (iirc) and a lot of
> drivers really care about the clock.

My understanding is that the clock is the authoritative parameter, while
vrefresh is offered as a convenience to avoid calculating it manually
through drivers.

> > Sadly I am too inexperienced to realize where the tolerances should
> > be stated, but I guess just stating that hsync_start etc are typical,
> > then specify some tolerance for each would help a bit?
> 
> The timings structure discussed in the patch that started this
> discussion is actually doing this nicely, you have for each timing the
> minimum, typical and maximum value. The current issue with it though
> is that it's pretty difficult to use it, since it's not really tied to
> any of the panel code (or DRM helpers). The only driver that was
> supporting it was omapdrm and it was removed recently.
> 
> If we really wanted to support it, one path forward I can see would be
> to make the timings structure the primary one, and only use
> drm_display_mode for userspace facing code, and generate it from the
> timings. This would be a pretty invasive change though...
> 
> > On the DSI displays in video mode there is also this EOL area
> > which seems to be where the logic is normally just idling for a
> > while, that can be adjusted on some hardware as well, but
> > I don't quite understand it admittedly. Sometimes I wonder if
> > anyone really understands DSI... :/
> 
> I'm not aware of any EOL area in MIPI-DSI that would make the hardware
> idle, don't you mean LP-11?

-- 
Regards,

Laurent Pinchart