On Tue, 2019-09-03 at 17:49 +0200, Daniel Vetter wrote: > On Tue, Sep 03, 2019 at 11:49:23AM +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? > > > > 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) Previously we were simply updating edid in intel_dp_set_edid > > without > > caring if it is the same or not and hotplug event was 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 make this > > function. > > update_edid is the thing which changes the userspace visible edid. We > can > check there with the previous userspace visible edid, and if it's > different, increase the epoch counter. If we never call update_edid > then > userspace won't see the changed edid (it might see the changed mode > list > or whatever), so doing that is pretty much a requirement for > correctness. > > The higher levels should notice the epoch counter change (you might > not > have captured all of them, there's a bunch between ioctl, poll worker > and > sysfs iirc), and send out the uevent. > > Why does that not work? Sure this will work, but still we need somehow to be able to determine this "if it's different" state. In your solution we just move that comparison to drm_connector_update_edid_property, which is quite fine for me. I would say that yes, this idea may be is even better because drivers won't need to implement this comparison in encoder->hotplug in each driver. However: we still need a comparison in drm_connector_update_edid_property(drm_edid_equal) and also I'm not sure we can send a hotplug event based on that as drm_connector_update_edid_property seems to get called only during connector init or during reprobe from userspace from connector->get_modes hook. Also it is called from drm_kms_helper_hotplug_event from, but this one is called from i915 only if connection status had changed. - Stanislav > -Daniel > > > > > Cheers, > > > > 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@xxxxxxxxxxxxxxxxxxxxx > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
