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

[Jani Nikula <jani.nikula@intel.com>]

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:
>> 
>> 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?

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

BR,
Jani.



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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx