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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 27, 2016 at 07:41:47PM +0100, Daniel Vetter 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 ...
> 
> 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.

5) Also integrate the connector status lifetime counter thing I discussed
with Chris, so that we can start to catch&report changes besides
connector->status (which is atm the only thing the probe helpers bother to
track). E.g. if you have hdmi repeaters, they indicate downstream changes
in a changed edid. Currently we fail to send out a hdp event for that.
-Daniel

> 
> > 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
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel