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

Daniel Vetter
2016-12-27T18:41:52Z

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 ...

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.
-Daniel

>
> BR,
> Jani.
>
>
> Jani Nikula (7):
> drm: reset ELD if NULL edid is passed to drm_edid_to_eld
> drm: move edid property update and add modes out of edid firmware
> loader
> drm: abstract override and firmware edid
> drm: export load edid firmware
> drm: make drm_get_displayid() available outside of drm_edid.c
> drm: add new drm_mode_connector_get_edid to do drm_get_edid and
> friends
> drm/i915: replace intel_ddc_get_modes with drm_mode_connector_get_edid
>
> drivers/gpu/drm/drm_connector.c | 60 ++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_edid.c | 10 +++----
> drivers/gpu/drm/drm_edid_load.c | 18 ++++--------
> drivers/gpu/drm/drm_probe_helper.c | 45 +++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_drv.h | 1 -
> drivers/gpu/drm/i915/intel_dvo.c | 5 ++--
> drivers/gpu/drm/i915/intel_modes.c | 23 ---------------
> drivers/gpu/drm/i915/intel_sdvo.c | 2 +-
> include/drm/drm_connector.h | 6 ++++
> include/drm/drm_edid.h | 8 +++--
> 10 files changed, 120 insertions(+), 58 deletions(-)
>
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation