| Message ID | 20190411143632.26838-1-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: Add detection of changing of edid on between suspend and resume | expand |
On Thu, Apr 11, 2019 at 05:36:30PM +0300, Gwan-gyeong Mun wrote: > This patch series fix missed detection of changing of edid on between > suspend and resume. > First patch fixes drm_helper_hdp_irq_event() in order to fix a below use > case. > > Following scenario requires detection of changing of edid. > > 1) plug display device to a connector > 2) system suspend > 3) unplug 1)'s display device and plug the other display device to a > connector > 4) system resume > > It adds edid check routine when a connector status still remains as > "connector_status_connected". > > Second patch adds a missed update of edid property of drm connector on i915. > > v2: Add NULL check before comparing of EDIDs. > v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan, Mika) > v4: Rebased > v5: Use a cached edid property blob data of connector instead of adding > a new detected_edid variable. (Maarten) > Add an using of reference count for getting a cached edid property > blob data. (Maarten) > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend > > v1, v2: https://patchwork.freedesktop.org/series/47680/ > v3: https://patchwork.freedesktop.org/series/49298/ > v4: https://patchwork.freedesktop.org/series/57397/ I guess I missed this, but there's a few fundamental things here first imo: - patch 2 is fallout from the fairly abitrary split between ->detect and ->get_modes. There's not really any good reason for that, I think we can just always call ->get_modes if ->detect says the connector is connected. There's more than just dp and hdmi (that's simply the 2 things we test in CI using chamelium). - the problem is bigger than just the edid changing (e.g. when repeaters change, or when something else changes aside from the edid, e.g. in dp aux, like how many lanes the dp cable has). Plus we have this long-term idea to give userspace a better idea about which connector exactly has changed. Therefor: 1. add a new drm_connector->detect_epoque counter. 2. Increment that counter every time something has changed, i.e. from probe helpers (we can push this down into the detect wrappers) when connector->status has changed, or when we update the edid blob. 3. probe helpers look for changes in ->detect_epoque to decided whether to fire the uevent or not. 4. long term we could expose the ->detect_epoque as a read-only property, or at least supply information about which connector caused the uevent using the new drm_sysfs_connector_status_event() (see the patch from Ram "[PATCH v3 09/10] drm: uevent for connector status change"). I think that gives us a much better long term foundation than adding lots of hacks. Cheers, Daniel > > Gwan-gyeong Mun (2): > drm: Add detection of changing of edid on between suspend and resume > drm/i915: Add a missed update of edid property of drm connector > > drivers/gpu/drm/drm_probe_helper.c | 34 +++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_dp.c | 1 + > drivers/gpu/drm/i915/intel_hdmi.c | 1 + > 3 files changed, 35 insertions(+), 1 deletion(-) > > -- > 2.21.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2019-04-15 at 18:32 +0200, Daniel Vetter wrote: > On Thu, Apr 11, 2019 at 05:36:30PM +0300, Gwan-gyeong Mun wrote: > > This patch series fix missed detection of changing of edid on > > between > > suspend and resume. > > First patch fixes drm_helper_hdp_irq_event() in order to fix a > > below use > > case. > > > > Following scenario requires detection of changing of edid. > > > > 1) plug display device to a connector > > 2) system suspend > > 3) unplug 1)'s display device and plug the other display device > > to a > > connector > > 4) system resume > > > > It adds edid check routine when a connector status still remains as > > "connector_status_connected". > > > > Second patch adds a missed update of edid property of drm connector > > on i915. > > > > v2: Add NULL check before comparing of EDIDs. > > v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan, > > Mika) > > v4: Rebased > > v5: Use a cached edid property blob data of connector instead of > > adding > > a new detected_edid variable. (Maarten) > > Add an using of reference count for getting a cached edid > > property > > blob data. (Maarten) > > > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate > > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend > > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate > > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend > > > > v1, v2: https://patchwork.freedesktop.org/series/47680/ > > v3: https://patchwork.freedesktop.org/series/49298/ > > v4: https://patchwork.freedesktop.org/series/57397/ > > I guess I missed this, but there's a few fundamental things here > first > imo: > > - patch 2 is fallout from the fairly abitrary split between ->detect > and > ->get_modes. There's not really any good reason for that, I think > we can > just always call ->get_modes if ->detect says the connector is > connected. There's more than just dp and hdmi (that's simply the 2 > things we test in CI using chamelium). > Thank you for checking it. I agree. I'll modify first patch with your guide and will resend it. > - the problem is bigger than just the edid changing (e.g. when > repeaters > change, or when something else changes aside from the edid, e.g. in > dp > aux, like how many lanes the dp cable has). Plus we have this long- > term > idea to give userspace a better idea about which connector exactly > has > changed. Therefor: > Yes, you pointed an essential problem. > 1. add a new drm_connector->detect_epoque counter. > 2. Increment that counter every time something has changed, i.e. > from > probe helpers (we can push this down into the detect wrappers) when > connector->status has changed, or when we update the edid blob. > 3. probe helpers look for changes in ->detect_epoque to decided > whether > to fire the uevent or not. > 4. long term we could expose the ->detect_epoque as a read-only > property, or at least supply information about which connector > caused > the uevent using the new drm_sysfs_connector_status_event() (see > the > patch from Ram "[PATCH v3 09/10] drm: uevent for connector status > change"). > I'll make new patches which followes your guide. Thanks for reviewing it and giving me a new approach. Br, G.G. > I think that gives us a much better long term foundation than adding > lots > of hacks. > > Cheers, Daniel > > > Gwan-gyeong Mun (2): > > drm: Add detection of changing of edid on between suspend and > > resume > > drm/i915: Add a missed update of edid property of drm connector > > > > drivers/gpu/drm/drm_probe_helper.c | 34 > > +++++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > drivers/gpu/drm/i915/intel_hdmi.c | 1 + > > 3 files changed, 35 insertions(+), 1 deletion(-) > > > > -- > > 2.21.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
This patch series fix missed detection of changing of edid on between suspend and resume. First patch fixes drm_helper_hdp_irq_event() in order to fix a below use case. Following scenario requires detection of changing of edid. 1) plug display device to a connector 2) system suspend 3) unplug 1)'s display device and plug the other display device to a connector 4) system resume It adds edid check routine when a connector status still remains as "connector_status_connected". Second patch adds a missed update of edid property of drm connector on i915. v2: Add NULL check before comparing of EDIDs. v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan, Mika) v4: Rebased v5: Use a cached edid property blob data of connector instead of adding a new detected_edid variable. (Maarten) Add an using of reference count for getting a cached edid property blob data. (Maarten) Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate Testcase: igt/kms_chamelium/dp-edid-change-during-suspend v1, v2: https://patchwork.freedesktop.org/series/47680/ v3: https://patchwork.freedesktop.org/series/49298/ v4: https://patchwork.freedesktop.org/series/57397/ Gwan-gyeong Mun (2): drm: Add detection of changing of edid on between suspend and resume drm/i915: Add a missed update of edid property of drm connector drivers/gpu/drm/drm_probe_helper.c | 34 +++++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_dp.c | 1 + drivers/gpu/drm/i915/intel_hdmi.c | 1 + 3 files changed, 35 insertions(+), 1 deletion(-)