| Message ID | 20190806125551.25761-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Send a hotplug when edid changes | expand |
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 ... -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 >
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.. However, please let me know, what do you think we should do for userspace. -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 > > > >
On Tue, Aug 6, 2019 at 4:06 PM Lisovskiy, Stanislav <stanislav.lisovskiy@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. 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 > > > > > > >
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@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. -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 > > > > > > > > > > > > >
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@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 > > -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 > > > > > > > > > > > > > > > > > > >
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@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. - 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 > > > > > > > > > > > > > > > > > > > > > > > > > > >
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@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 >>>>>>> >>>>>> >>>>>> >>>> >>>> >>>> >> >> >
On Mon, 2019-08-19 at 08:35 +0100, Peres, Martin wrote: > On 19/08/2019 10:23, Lisovskiy, Stanislav wrote: > > On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote: > > > > > > > > > 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 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. > > > > > - 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Mon, 2019-08-19 at 09:05 +0000, Lisovskiy, Stanislav wrote: > On Mon, 2019-08-19 at 08:35 +0100, Peres, Martin wrote: > > On 19/08/2019 10:23, Lisovskiy, Stanislav wrote: > > > On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote: > > > > > > > > > > > > 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 > > 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. > > > > > > > > - 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
On Tue, Sep 03, 2019 at 09:17:49AM +0000, Lisovskiy, Stanislav wrote: > On Mon, 2019-08-19 at 09:05 +0000, Lisovskiy, Stanislav wrote: > > On Mon, 2019-08-19 at 08:35 +0100, Peres, Martin wrote: > > > On 19/08/2019 10:23, Lisovskiy, Stanislav wrote: > > > > On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote: > > > > > > > > > > > > > > > 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 > > > > 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. 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? -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
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. 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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > >
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
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? -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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > >
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@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > >
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@lists.freedesktop.org > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > > > > > >
On Wed, 2019-09-04 at 11:23 +0200, Daniel Vetter wrote: > > > 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. So I guess the conclusion would be to try to use drm_connector_update_edid_property that way we will avoid duplicating drm_edid_equal code in all drivers. However this might require ensuring that drm_connector_update_edid_property is always called when we get a hotplug, so there we can check if edid had changed and send uevent, if needed. - Stanislav > -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_hotpl > > > > > > > > > > > > > > > ug.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 > > > > > > > > > > > > > > > > > >