On Tue, 2019-09-03 at 11:49 +0000, Lisovskiy, Stanislav wrote:
On Tue, 2019-09-03 at 11:40 +0200, Daniel Vetter wrote:
In fact I was wrong - when it worked, it was using exactly those patches :). With clean drm-tip - it seems to work ocassionally and it doesn't update the actual display edid and other stuff, so even when displays are changed we still see the old info/edid from userspace.
We always get a hpd irq when suspend/resume however it doesn't always result in uevent being sent. So there is a real need in those patches.
Just decided to "ping" this discussion again. The issue is already some years old and still nothing is fixed. I do agree that may be something needs to be fixed/changed here in those patches, but something must be agreed at least I guess, as discussions themself do not fix bugs. Currently those patches address a particular issue which occurs, if display is changed during suspend. On ocassional basis, userspace might not get a hotplug event at all, causing different kind of problems(like wrong mode set on display or diaply not working at all). Also some kms_chamelium hotplug tests fail because of that.
I still think we'll long-term regret this if we just duct-tape more stuff on top, instead of giving userspace a more informative uevent. This will send more uevents to userspace, so maybe then userspace tries to filter more and be clever, which never works, and we're back to tears.
But here we actually do need a uevent as currently we don't get any at all, if edid changes during suspend. If userspace will try to filter this out - it's just stupid, however we still need to do things correctly.
Anyway, on the approach itself: It's extremely i915 specific, and it requires that all drivers roll out drm_edid_equal checks and not forget to increment the epoch counter.
What I had in mind is that when we set the edid for a connector with drm_connector_update_edid_property() or whatever, then the epoch counter would auto-increment if anything has changed. Similarly (long-term idea at least) if anything important with DP registers has changed.
Can't we do that, instead of this sub-optimal solution of requiring all drivers to roll out lots of code?
So once again, just to summarize things:
1) We update edid in intel_dp_set_edid, which is called from intel_dp_detect(drm_connector_helper_funcs->detect_ctx hook) which is called from drm_helper_probe_detect. That one is called either from specific intel_encoder->hotplug hook in i915_hotplug_work_func or by userspace request during reprobe.
2) Currently we are simply updating edid in intel_dp_set_edid without caring if it is the same or not and hotplug event is sent only once connection_status had changed.
3) drm_connector_update_edid_property is called from connector-
get_modes hook(lets say intel_dp_get_modes fo dp) however it simply
uses results of drm_helper_probe_detect so without actual comparison it would not be able to detect if we really need to update epoch_counter or not.
Because as I said currently intel_dp_set_edid simply assigns it without checking, so that way you will get epoch_counter updated every time, i.e exactly what you wanted to avoid here.
So we really need someway to determine if edid had changed, instead of simply assigning it all the time - that is why I had to implement drm_edid_equal function.
- Stanislav
-Daniel
- Stanislav
> > > > > > -Stanislav > > > > > > > > Cheers, Daniel > > > > > > > > > > > > > > > -Stanislav > > > > > > > > > > > > > -Daniel > > > > > > > > > > > > > > > > > Stanislav Lisovskiy (3): > > > > > > drm: Add helper to compare edids. > > > > > > drm: Introduce change counter to > > > > > > drm_connector > > > > > > drm/i915: Send hotplug event if edid had > > > > > > changed. > > > > > > > > > > > > drivers/gpu/drm/drm_connector.c | > > > > > > > > > > > > 1 + > > > > > > drivers/gpu/drm/drm_edid.c | > > > > > > 33 > > > > > > ++++++++++++++++++++ > > > > > > drivers/gpu/drm/drm_probe_helper.c | > > > > > > 29 > > > > > > +++++++++++++++- > > > > > > - > > > > > > drivers/gpu/drm/i915/display/intel_dp.c | > > > > > > 16 > > > > > > +++++++++- > > > > > > drivers/gpu/drm/i915/display/intel_hdmi.c | > > > > > > 16 > > > > > > ++++++++-- > > > > > > drivers/gpu/drm/i915/display/intel_hotplug.c | > > > > > > 21 > > > > > > ++++++++++ > > > > > > --- > > > > > > include/drm/drm_connector.h | > > > > > > > > > > > > 3 ++ > > > > > > include/drm/drm_edid.h | > > > > > > > > > > > > 9 > > > > > > ++++++ > > > > > > 8 files changed, 117 insertions(+), 11 > > > > > > deletions(-) > > > > > > > > > > > > -- > > > > > > 2.17.1 > > > > > > > > > > > > > > > > > > > > > > > > > > >
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel