On 19/08/2019 10:23, Lisovskiy, Stanislav wrote: > On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote: >> On Wed, Aug 07, 2019 at 07:43:18AM +0000, Lisovskiy, Stanislav wrote: >>> On Tue, 2019-08-06 at 19:43 +0200, Daniel Vetter wrote: >>>> On Tue, Aug 6, 2019 at 4:06 PM Lisovskiy, Stanislav >>>> <stanislav.lisovskiy@xxxxxxxxx> wrote: >>>>> >>>>> On Tue, 2019-08-06 at 15:51 +0200, Daniel Vetter wrote: >>>>>> On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy >>>>>> wrote: >>>>>>> This series introduce to drm a way to determine if >>>>>>> something >>>>>>> else >>>>>>> except connection_status had changed during probing, which >>>>>>> can be used by other drivers as well. Another i915 specific >>>>>>> part >>>>>>> uses this approach to determine if edid had changed without >>>>>>> changing the connection status and send a hotplug event. >>>>>> >>>>>> Did you read through the entire huge thread on all this (with >>>>>> I >>>>>> think >>>>>> Paul, Pekka, Ram and some others)? I feel like this is mostly >>>>>> taking >>>>>> that >>>>>> idea, but not taking a lot of the details we've discussed >>>>>> there. >>>>>> Specifically I'm not sure how userspace should handle this >>>>>> without >>>>>> also >>>>>> exposing the epoch counter, or at least generating a hotplug >>>>>> event >>>>>> for the >>>>>> specific connector. All that and more we discussed there. >>>>>> >>>>>> And then there's the follow-up question: What's the userspace >>>>>> for >>>>>> this? >>>>>> Existing expectations are a minefield here, so even if you >>>>>> don't >>>>>> plan >>>>>> to >>>>>> change userspace we need to know what this is aimed for, and >>>>>> see >>>>>> above I >>>>>> don't think this is possible to use without userspace changes >>>>>> ... >>>>> >>>>> Yes, sure I agree about userspace. However I guess we must >>>>> start >>>>> from >>>>> something at least. >>>>> I think I have seen some discussion regarding exposing this >>>>> epoch >>>>> counter as a property. Was even implementing this at some point >>>>> but >>>>> didn't dare to send to mailing list :) >>>>> >>>>> My idea was just to expose this epoch counter as a drm >>>>> property, to >>>>> let >>>>> userspace then to figure out, what has happened, as imho adding >>>>> different events for this and that is a bit of an overkill and >>>>> brings >>>>> additional complexity.. >>>> >>>> Adding Ram and link to the (warning, huge!) thread: >>>> >>>> > https://patchwork.freedesktop.org/patch/303905/?series=57232&rev=9 >>>> >>>>> However, please let me know, what do you think we should do for >>>>> userspace. >>>> >>>> That seems backwards, since this is an uapi change I'd expect >>>> you're >>>> solving some specific issue in some specific userspace? If we're >>>> doing >>>> this just because I'm not really following. >>> >>> Specifically, I'm solving an issue of changed edid during suspend, >>> like >>> we for example have in kms_chamelium hotplug tests(some of which >>> now >>> fail because of that). >>> So even if connection status hasn't changed, we still need to send >>> hotplug event and userspace needs to be able to understand that >>> something had changed and whether we need to do a full reprobe or >>> not. >>> Epoch counter property would be responsible for this. >>> As I understand in general there might be other connector updates, >>> except edid which we need propagate in a similar way. >> >> So igt isn't valid userspace (it's just good testcases). Can we repro >> the >> same on real userspace? Does this work with real userspace? We've had >> userspace which tries to be clever and filters out what looks like >> redundant hotplug events. And then gets it wrong in cases like this. >> >> Also, we've had forever an unconditional uevent on resume, exactly >> because >> anything could have changed. Did we loose this one on the way >> somewhere? >> Or maybe I misremember ... >> >> If all we care about is resume re-adding that uncondtional uevent on >> resume is going to be a lot easier than this here. >> -Daniel > > Sorry for long reply(was on vacation), that is a good question > regarding reproducing this in real life scenario. My obvious guess > was to suspend the machine and meanwhile change connected display to > another one. However this situation seems to be already handled by > kernel nicely(tried few times and we always get a hotplug event). So > that edid change during suspend chamelium test case seems to be > a bit different. I will talk to our guys who wrote this about what is > the real life scenario for this, because I'm now curious as well. Thanks Daniel for the feedback. I also now wonder why our IGT test (chamelium-based) does not pass if a uevent is sent on resume automatically and all the test is expecting is a uevent... Martin > > - 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
