[Intel-gfx] [PATCH v5 0/2] drm: Add detection of changing of edid on between suspend and resume

[Mun, Gwan-gyeong]

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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx