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
