[Intel-gfx] [PATCH v3 0/3] Send a hotplug when edid changes

[Peres, Martin]

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 at intel.com> 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
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
>>
>