Re: [PATCH] Revert "drm: Stop resetting connector state to unknown"

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Dec 15, 2015 at 01:23:06PM -0500, Rob Clark wrote:
> On Tue, Dec 15, 2015 at 1:07 PM, Lyude <cpaul@redhat.com> wrote:
> > This reverts commit 5677d67ae394 ("drm: Stop resetting connector state to
> > unknown")
> >
> > Unfortunately, not resetting the connector status to unknown actually
> > breaks reprobing on suspend/resume in i915, which is important to have
> > working since it means a user docking their laptop in suspend won't have
> > their monitors work after resume. This commit was originally pushed to fix
> > a bug with systemd[1], however said bug has already been fixed in
> > userspace.
> >
> > Since "unknown" is technically a valid option to return to userspace for a
> > connector's status, I would think that this sort of behavior should
> > probably be expected from userspace. Some good examples of this are the
> > radeon driver reporting "unknown" for connectors that have done something
> > wonky during a hotplug event (e.g. part of the initialization of the
> > connector failed), and the omapdrm driver returns "unknown" for certain
> > connector types by default.
> >
> > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=100641
> >
> > Signed-off-by: Lyude <cpaul@redhat.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 24c5434..474e636 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -5312,11 +5312,12 @@ void drm_mode_config_reset(struct drm_device *dev)
> >                 if (encoder->funcs->reset)
> >                         encoder->funcs->reset(encoder);
> >
> > -       mutex_lock(&dev->mode_config.mutex);
> > -       drm_for_each_connector(connector, dev)
> > +       list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> > +               connector->status = connector_status_unknown;
> > +
> >                 if (connector->funcs->reset)
> >                         connector->funcs->reset(connector);
> > -       mutex_unlock(&dev->mode_config.mutex);
> > +       }
> >  }
> 
> looks like git-revert might have been a bit over-ambitious and
> clobbered a couple subsequent changes.. but that is easy enough to fix
> once we figure out what the right thing to do is.
> 
> Beyond that.. I'm not really sure how to apply the "do not break
> userspace" rule here.. since prior to
> c484f02d0f02fbbfc6decc945a69aae011041a27 userspace could see "unknown"
> for certain hardware.  But after that commit it could start seeing
> "unknown" for drivers/connectors that never would have returned
> "unknown" before.  If userspace had a problem with "unknown", it
> sounds like a userspace bug that was just unnoticed because no one
> tested on the right hardware.
> 
> But anyways, one idea to revert things to original behavior prior to
> c484f02d0f02fbbfc6decc945a69aae011041a27 (so at least userspace
> doesn't see 'unknown' for drivers/connectors that never used to report
> 'unknown') would be to do something roughly like this in
> status_show():
> 
>   if (status == unknown)
>      status = connector->funcs->detect(connector)
> 
> So I could go with either just reverting this commit, or reverting
> commit plus above change.  My $0.02 anyways..

Hmm. Or maybe leave the states alone, and just fire off the uevent
unconditionally and let userspace initiate the probe. That way we could
skip all ->detect() calls during resume for a bit of extra speed.

Not 100% if that would be safe. Maybe something internal depends on the
->detect() having been called? DP stuff perhaps? Which makes me wonder
how i915 copes with a DP monitor geting unplugged/change while
suspended. I should probably try that.

-- 
Ville Syrjälä
Intel OTC