[Intel-gfx] [RFC PATCH 0/7] drm: facilitate driver unification around edid read and override

[Daniel Vetter]

On Wed, Dec 28, 2016 at 11:23:42AM +0200, Jani Nikula wrote:
> On Tue, 27 Dec 2016, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Tue, Dec 27, 2016 at 06:21:47PM +0200, Jani Nikula wrote:
> >> Hi all -
> >> 
> >> This series aims at three goals:
> >> 
> >> 1) Most drivers do similar things around drm_get_edid (namely convert
> >> edid to eld, add modes, and update edid blob property). Add a helper for
> >> the drivers, to reduce code and unify.
> >
> > I like.
> >
> >> 2) Move override and firmware EDID handling to a lower level, in the
> >> helper. This makes them more transparent to the rest of the stack,
> >> avoiding special casing. For example, any drm_detect_hdmi_monitor calls
> >> typically use the EDID from the display, not the override EDID.
> >
> > I replied to that patch, I think we need to rethink the helper callbacks
> > to make this work. The general direction make sense to me though.
> >
> >> 3) Make EDID caching easier and unified across drivers. Currently,
> >> plenty of drivers have their own hacks for caching EDID reads. This
> >> could be made more transparent and unified at the helper level.
> >
> > Caching is Real Hard, and I don't think something generic will work:
> > - On DP, hpd works and will reliable tell you when you need to invalidate
> >   stuff. Except when you have a downstream port to something else (hdmi,
> >   vga, whatever). I think this is best solved by making the shared helpers
> >   for DP a lot better.
> >
> > - HDMI is supposed to work, except it's not. You can't rely on hpd, any
> >   caching needs to have a time limit. I guess we could share some code
> >   here.
> >
> > - Everything else is much worse, and caching is a no-go. At most a
> >   time-based cache that invalidates and re-probes.
> >
> > - Built-in panels are special.
> >
> > - One issue with all this is that currently the hpd helpers we have (not
> >   the stuff in i915) don't even forward which hpd signalled which
> >   connector.
> >
> > - Another fun is handling invalidations in general. System suspend/resume
> >   is real fun that way ...
> 
> So I don't disagree. But looking at all the code that uses
> drm_get_edid(), there's quite a bit of EDID caching around. I don't
> think it's all that great as it is. Mostly I'm just saying, perhaps the
> EDID property should be the place to store the EDID if drivers want to
> use a cached value? Why keep several copies around, making the
> invalidation even more complicated? And really, updating of the EDID
> property seems to be bonkers all over the place too. Perhaps we should
> just have helpers for the drivers to make the caching easier?

I think a big part for all that caching is the split between ->detect and
->get_modes. That simply seems to cause pain all over. Caching the EDID
beyond that is where it gets really tricky, and afaik (haven't re-read all
the drivers) i915 is the only one that tries to do that somewhat. Or at
least did (not sure again, since detect/get_modes + hotplug makes a fine
mess).

> > 4) Fix the locking (well, entire lack thereof) between the probe paths
> > (protected by mode_config.mutex), and the atomic modeset paths (protected
> > by mode_config.connection_mutex).
> >
> > Yes that's feature creep but I think any redesign of the probe code should
> > have a answer to that too.
> >
> >> All of this is opt-in, using the helper from patch 6/7. This is mostly
> >> because converting everything at one go is pretty much impossible. The
> >> main wrinkle is having to leave override/firmware EDID handling in two
> >> places for now, but this could be fixed once enough drivers have
> >> switched to using the common helper.
> >
> > I feel like we'd need a bit more conversion when merging, to make sure it
> > all makes sense. Ending up with 2 not really useful ways to handle probing
> > in the helpers would be worse than what we have now.
> 
> Sure, I just threw in something for the discussion. Unfortunately it
> doesn't look easy to convert everything in one go, at least it requires
> digging into the internals of too many drivers for any single person to
> easily handle.
> 
> Perhaps one outcome of the discussion should be a better idea what the
> aim for the override/firmware EDID really is. Currently, you can use it
> to use specific modes and timings, but that's about it. You can use it
> to test a specific part of the EDID parser, but not all. Everything else
> still gets used from the display EDID. So should we try to make them
> complete and transparent replacements of the EDID or not?

Very much +1 on that goal, since it would allow us to test fun stuff like
audio, screen bpp limits and kinds of fun. I want this since years. I'm
just not sure how to do it properly, since indeed this entire area has a
lot of "organically evolved" taste to it ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch