On Wed, Dec 28, 2016 at 11:23:42AM +0200, Jani Nikula wrote: > On Tue, 27 Dec 2016, Daniel Vetter <daniel@xxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
