On Wed, Sep 04, 2019 at 08:36:46AM +0000, Lisovskiy, Stanislav wrote: > 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. Yes we need to compare edid somewhere, that much is clear. I'm not disputing that. I just want something where we don't have to roll this out over all the drivers, because that's a hopeless endeavour. > 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. So ditch the optimization to only call ->get_modes when called from userspace? We've been talking about this one too in the past ... I'd really like a solution where it will work for most drivers out of the box. -Daniel > > - 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 > > > > > > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
