On Wed, Dec 28, 2016 at 11:23:42AM +0200, Jani Nikula wrote:
On Tue, 27 Dec 2016, Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Dec 27, 2016 at 06:21:47PM +0200, Jani Nikula wrote:
Hi all -
This series aims at three goals:
- 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.
- 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.
- 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).
- 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