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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
