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

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

[Lyude]

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);
+	}
 }
 EXPORT_SYMBOL(drm_mode_config_reset);
 
-- 
2.5.0

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

[Rob Clark]

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..

BR,
-R

>  EXPORT_SYMBOL(drm_mode_config_reset);
>
> --
> 2.5.0
>

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

[Ville Syrjälä]

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

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

[Rob Clark]

On Tue, Dec 15, 2015 at 1:33 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> 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.

That is, iirc, something that Lyude suggested when we discussed in
person (but I asked him to send revert patch just to start broader
discussion on what the right solution would be).  Depending on
userspace to reprobe seems like at least the wrong thing for fbcon..
which as much as I wish it would go away, as long as it is enabled in
kernel config, I don't think we can ignore.

BR,
-R

> 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

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

[Lyude]

On Tue, 2015-12-15 at 13:42 -0500, Rob Clark wrote:
> On Tue, Dec 15, 2015 at 1:33 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > 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.
The current purpose of all of the detect() calls is to check whether or not it
would be appropriate to send the hotplug event (assuming you mean
drm_kms_helper_hotplug_event()), just so that we can save reprobing a display if
nothing changed. Anyway, if speed were an issue here we could just fire off a
worker so that the reprobing doesn't hold up the resume process.

While I'd be fine with just firing off a probe, I don't think it makes sense to
avoid having an unknown state here when numerous drivers already use unknown
states regularly, unless there was a reason other then the one mentioned in the
bugzilla for doing this.

> That is, iirc, something that Lyude suggested when we discussed in
> person (but I asked him to send revert patch just to start broader
*her please :)

> discussion on what the right solution would be).  Depending on
> userspace to reprobe seems like at least the wrong thing for fbcon..
> which as much as I wish it would go away, as long as it is enabled in
> kernel config, I don't think we can ignore.
> 
> BR,
> -R
> 
> > 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
-- 
Cheers,
	Lyude

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

[Ville Syrjälä]

On Tue, Dec 15, 2015 at 03:34:18PM -0500, Lyude wrote:
> On Tue, 2015-12-15 at 13:42 -0500, Rob Clark wrote:
> > On Tue, Dec 15, 2015 at 1:33 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > > 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.
> The current purpose of all of the detect() calls is to check whether or not it
> would be appropriate to send the hotplug event (assuming you mean
> drm_kms_helper_hotplug_event()), just so that we can save reprobing a display if
> nothing changed. Anyway, if speed were an issue here we could just fire off a
> worker so that the reprobing doesn't hold up the resume process.

AFAICS if you force everything to unknown you are effectively forcing it
to send the uevent anyway (unless the probe actually returns "unknown"
for everything). Seems a bit pointless to do the probe at all when you
really know that you're going to send the event anyway.

> 
> While I'd be fine with just firing off a probe, I don't think it makes sense to
> avoid having an unknown state here when numerous drivers already use unknown
> states regularly, unless there was a reason other then the one mentioned in the
> bugzilla for doing this.
> 
> > That is, iirc, something that Lyude suggested when we discussed in
> > person (but I asked him to send revert patch just to start broader
> *her please :)
> 
> > discussion on what the right solution would be).  Depending on
> > userspace to reprobe seems like at least the wrong thing for fbcon..
> > which as much as I wish it would go away, as long as it is enabled in
> > kernel config, I don't think we can ignore.
> > 
> > BR,
> > -R
> > 
> > > 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
> -- 
> Cheers,
> 	Lyude

-- 
Ville Syrjälä
Intel OTC

[PATCH v2] drm/i915: Send a hotplug event on resume instead of an IRQ event

[Lyude]

We currently call drm_helper_hpd_irq_event() to handle reprobing
displays on resume, however drm_helper_hpd_irq_event() only checks the
status of hpd. HPD doesn't update if the displays connected changed
before resuming the system, and as such causes us not to detect any
connection changes after resume.

This originally wasn't the case since drm would reset all of the
connectors to unknown during resume and cause a reprobe (since the
connector statuses would change), however this behavior changed in
5677d67ae394 ('drm: Stop resetting connector state to unknown').

Signed-off-by: Lyude <cpaul@redhat.com>
---
I did some research into why setting the connector state to unknown was
causing a reprobe and discovered that the fact that this reprobing ever
worked at all was just a coincidence (i915's IRQ handler gets called by the
irq event, notices the state of all the connectors changed since they were
set to unknown, causes a reprobe). This seems to fix everything (including
monitor detection in normal VTs), and I can't figure out any real use now
for us to set everything to unknown so I'm going to go the route of just
sending the hotplug event instead.

 drivers/gpu/drm/i915/i915_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 760e0ce..27a6c1b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -792,7 +792,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	 * */
 	intel_hpd_init(dev_priv);
 	/* Config may have changed between suspend and resume */
-	drm_helper_hpd_irq_event(dev);
+	drm_kms_helper_hotplug_event(dev);
 
 	intel_opregion_init(dev);
 
-- 
2.5.0

Re: [PATCH v2] drm/i915: Send a hotplug event on resume instead of an IRQ event

[Daniel Vetter]

On Tue, Dec 15, 2015 at 05:25:56PM -0500, Lyude wrote:
> We currently call drm_helper_hpd_irq_event() to handle reprobing
> displays on resume, however drm_helper_hpd_irq_event() only checks the
> status of hpd. HPD doesn't update if the displays connected changed
> before resuming the system, and as such causes us not to detect any
> connection changes after resume.
> 
> This originally wasn't the case since drm would reset all of the
> connectors to unknown during resume and cause a reprobe (since the
> connector statuses would change), however this behavior changed in
> 5677d67ae394 ('drm: Stop resetting connector state to unknown').
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
> I did some research into why setting the connector state to unknown was
> causing a reprobe and discovered that the fact that this reprobing ever
> worked at all was just a coincidence (i915's IRQ handler gets called by the
> irq event, notices the state of all the connectors changed since they were
> set to unknown, causes a reprobe). This seems to fix everything (including
> monitor detection in normal VTs), and I can't figure out any real use now
> for us to set everything to unknown so I'm going to go the route of just
> sending the hotplug event instead.
> 
>  drivers/gpu/drm/i915/i915_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 760e0ce..27a6c1b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -792,7 +792,7 @@ static int i915_drm_resume(struct drm_device *dev)
>  	 * */
>  	intel_hpd_init(dev_priv);
>  	/* Config may have changed between suspend and resume */
> -	drm_helper_hpd_irq_event(dev);
> +	drm_kms_helper_hotplug_event(dev);

So there's a bunch of things broken in this area, but just from your
description it's unclear what exactly. Problems I'm aware of:

- hpd_irq_event() only reprobes connectors for which hpd is enabled, which
  are not all of them. We broke this in

  commit 816da85a0990c2b52cfffa77637d1c770d6790e9
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>
  Date:   Tue Oct 23 18:23:33 2012 +0000
  
      drm: handle HPD and polled connectors separately

  which is positively ages ago. Also note that any other driver not using
  vt switching also suffers from this (atm radeon, nouveau, rockchip,
  amdgpu). otoh at least from a quick those drivers also don't really
  bother with reprobing on resume really.

- DP MST reprobing is busted in i915 since

  commit e7d6f7d708290da1b7c92f533444b042c79412e0
  Author: Dave Airlie <airlied@redhat.com>
  Date:   Mon Dec 8 13:23:37 2014 +1000
  
      drm/i915: resume MST after reading back hw state

  There's some bug reports iirc from Takashi and Ted Tso about this. Your
  patch won't help for DP MST since we never reprobe MST state from
  userspace, it only ever happens through interrupt handling. Well mostly,
  if you completely unplug something then we detect that and throw all the
  MST stuff over board, but if you e.g. just change a connection in your
  dock or go from one dock to another we won't detect this.

Which one of the above is it? And to what extend do you care about this
working everywhere?

Sorry that I'm opening a can of worms here.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch