Recently David Weinehall has been investigating how we can make the resume
process in i915 faster, and poked me to take a look at why we were taking so
long while reprobing. As it turns out the main issue is just that we hold up
the entire resume process while we reprobe connectors, which isn't really
necessary. This fixes things so we can actually run the connector reprobing on
resume asynchronously.
Cc: David Weinehall <[email protected]>
Lyude (2):
drm/i915: Remove redundant reprobe in i915_drm_resume
drm/i915: Reinit polling before hpd when resuming
drivers/gpu/drm/i915/i915_drv.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
--
2.7.4
Weine's investigation on benchmarking the suspend/resume process pointed
out a lot of the time in suspend/resume is being spent reprobing. While
the reprobing process is a lengthy one for good reason, we don't need to
hold up the entire suspend/resume process while we wait for it to
finish. Luckily as it turns out, we already trigger a full connector
reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
i915_drm_resume() entirely.
This won't lead to less time spent resuming just yet since now the
bottleneck will be waiting for the mode_config lock in
drm_kms_helper_poll_enable(), since that will be held as long as
i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
address that in the next patch.
Signed-off-by: Lyude <[email protected]>
Cc: David Weinehall <[email protected]>
---
drivers/gpu/drm/i915/i915_drv.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index bfb2efd..532cc0f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
* notifications.
* */
intel_hpd_init(dev_priv);
- /* Config may have changed between suspend and resume */
- drm_helper_hpd_irq_event(dev);
intel_opregion_register(dev_priv);
--
2.7.4
Now that we don't run the connector reprobing from i915_drm_resume(), we
need to make it so we don't have to wait for reprobing to finish so that
we actually speed things up. In order to do this, we need to make sure
that i915_drm_resume() doesn't get blocked by i915_hpd_poll_init_work()
while trying to acquire the mode_config lock that
drm_kms_helper_poll_enable() needs to acquire.
The easiest way to do this is to just enable polling before hpd. This
shouldn't break anything since at that point we have everything else we
need for polling enabled.
As well, this should result in a rather significant improvement in how
quickly we can resume the system.
Signed-off-by: Lyude <[email protected]>
Cc: David Weinehall <[email protected]>
---
drivers/gpu/drm/i915/i915_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 532cc0f..f605dde 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1595,6 +1595,8 @@ static int i915_drm_resume(struct drm_device *dev)
intel_display_resume(dev);
+ drm_kms_helper_poll_enable(dev);
+
/*
* ... but also need to make sure that hotplug processing
* doesn't cause havoc. Like in the driver load code we don't
@@ -1614,7 +1616,6 @@ static int i915_drm_resume(struct drm_device *dev)
intel_opregion_notify_adapter(dev_priv, PCI_D0);
intel_autoenable_gt_powersave(dev_priv);
- drm_kms_helper_poll_enable(dev);
enable_rpm_wakeref_asserts(dev_priv);
--
2.7.4
On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> Weine's investigation on benchmarking the suspend/resume process pointed
> out a lot of the time in suspend/resume is being spent reprobing. While
> the reprobing process is a lengthy one for good reason, we don't need to
> hold up the entire suspend/resume process while we wait for it to
> finish. Luckily as it turns out, we already trigger a full connector
> reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
> i915_drm_resume() entirely.
>
> This won't lead to less time spent resuming just yet since now the
> bottleneck will be waiting for the mode_config lock in
> drm_kms_helper_poll_enable(), since that will be held as long as
> i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
> address that in the next patch.
>
> Signed-off-by: Lyude <[email protected]>
> Cc: David Weinehall <[email protected]>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index bfb2efd..532cc0f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
> * notifications.
> * */
> intel_hpd_init(dev_priv);
> - /* Config may have changed between suspend and resume */
> - drm_helper_hpd_irq_event(dev);
The comment is still apt. This code is known to be broken since it
doesn't detect a change in monitors (e.g. a change in external connectors
from docking) between suspend and resend. We still have to send the uevent.
+ drm_kms_helper_hotplug_event(dev);
which also depends upon us actually reseting the connector->status to
unknown in drm_mode_config_reset(), Daniel!
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
On Thu, 2016-11-03 at 16:02 +0000, Chris Wilson wrote:
> On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> >
> > Weine's investigation on benchmarking the suspend/resume process pointed
> > out a lot of the time in suspend/resume is being spent reprobing. While
> > the reprobing process is a lengthy one for good reason, we don't need to
> > hold up the entire suspend/resume process while we wait for it to
> > finish. Luckily as it turns out, we already trigger a full connector
> > reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
> > i915_drm_resume() entirely.
> >
> > This won't lead to less time spent resuming just yet since now the
> > bottleneck will be waiting for the mode_config lock in
> > drm_kms_helper_poll_enable(), since that will be held as long as
> > i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
> > address that in the next patch.
> >
> > Signed-off-by: Lyude <[email protected]>
> > Cc: David Weinehall <[email protected]>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index bfb2efd..532cc0f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
> > * notifications.
> > * */
> > intel_hpd_init(dev_priv);
> > - /* Config may have changed between suspend and resume */
> > - drm_helper_hpd_irq_event(dev);
>
> The comment is still apt. This code is known to be broken since it
> doesn't detect a change in monitors (e.g. a change in external connectors
> from docking) between suspend and resend. We still have to send the uevent.
>
> + drm_kms_helper_hotplug_event(dev);
I might not have explained myself very well. The way things should look with
this patch is like this:
i915_drm_resume()
-> intel_hpd_init()
-> sets dev_priv->hotplug.poll_enabled to true
-> schedules dev_priv->hotplug.poll_init_work
-> continue resume…
at the same time:
i915_hpd_poll_init_work() gets scheduled and starts
-> since dev_priv->hotplug.poll_enabled == false, drm_helper_hpd_irq_event() is called
-> drm_helper_hpd_irq_event() reprobes connectors
-> if anything changed, drm_kms_helper_hotplug_event() gets called.
So we're still polling the connectors when coming out of resume just like
before, except now we're doing it without needlessly making the whole resume
process stall until we're done. We're also no longer reprobing display
connectors twice…
>
> which also depends upon us actually reseting the connector->status to
> unknown in drm_mode_config_reset(), Daniel!
> -Chris
>
On Thu, 2016-11-03 at 12:11 -0400, Lyude Paul wrote:
> On Thu, 2016-11-03 at 16:02 +0000, Chris Wilson wrote:
> >
> > On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> > >
> > >
> > > Weine's investigation on benchmarking the suspend/resume process pointed
> > > out a lot of the time in suspend/resume is being spent reprobing. While
> > > the reprobing process is a lengthy one for good reason, we don't need to
> > > hold up the entire suspend/resume process while we wait for it to
> > > finish. Luckily as it turns out, we already trigger a full connector
> > > reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
> > > i915_drm_resume() entirely.
> > >
> > > This won't lead to less time spent resuming just yet since now the
> > > bottleneck will be waiting for the mode_config lock in
> > > drm_kms_helper_poll_enable(), since that will be held as long as
> > > i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
> > > address that in the next patch.
> > >
> > > Signed-off-by: Lyude <[email protected]>
> > > Cc: David Weinehall <[email protected]>
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.c | 2 --
> > > 1 file changed, 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index bfb2efd..532cc0f 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
> > > * notifications.
> > > * */
> > > intel_hpd_init(dev_priv);
> > > - /* Config may have changed between suspend and resume */
> > > - drm_helper_hpd_irq_event(dev);
> >
> > The comment is still apt. This code is known to be broken since it
> > doesn't detect a change in monitors (e.g. a change in external connectors
> > from docking) between suspend and resend. We still have to send the uevent.
> >
> > + drm_kms_helper_hotplug_event(dev);
>
> I might not have explained myself very well. The way things should look with
> this patch is like this:
>
> i915_drm_resume()
> -> intel_hpd_init()
> -> sets dev_priv->hotplug.poll_enabled to true
Whoops, s/true/false/
> -> schedules dev_priv->hotplug.poll_init_work
> -> continue resume…
>
> at the same time:
>
> i915_hpd_poll_init_work() gets scheduled and starts
> -> since dev_priv->hotplug.poll_enabled == false, drm_helper_hpd_irq_event()
> is called
> -> drm_helper_hpd_irq_event() reprobes connectors
> -> if anything changed, drm_kms_helper_hotplug_event() gets called.
>
> So we're still polling the connectors when coming out of resume just like
> before, except now we're doing it without needlessly making the whole resume
> process stall until we're done. We're also no longer reprobing display
> connectors twice…
>
> >
> >
> > which also depends upon us actually reseting the connector->status to
> > unknown in drm_mode_config_reset(), Daniel!
> > -Chris
> >
On Thu, Nov 03, 2016 at 12:11:55PM -0400, Lyude Paul wrote:
> On Thu, 2016-11-03 at 12:11 -0400, Lyude Paul wrote:
> > On Thu, 2016-11-03 at 16:02 +0000, Chris Wilson wrote:
> > >
> > > On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> > > >
> > > >
> > > > Weine's investigation on benchmarking the suspend/resume process pointed
> > > > out a lot of the time in suspend/resume is being spent reprobing. While
> > > > the reprobing process is a lengthy one for good reason, we don't need to
> > > > hold up the entire suspend/resume process while we wait for it to
> > > > finish. Luckily as it turns out, we already trigger a full connector
> > > > reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
> > > > i915_drm_resume() entirely.
> > > >
> > > > This won't lead to less time spent resuming just yet since now the
> > > > bottleneck will be waiting for the mode_config lock in
> > > > drm_kms_helper_poll_enable(), since that will be held as long as
> > > > i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
> > > > address that in the next patch.
> > > >
> > > > Signed-off-by: Lyude <[email protected]>
> > > > Cc: David Weinehall <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.c | 2 --
> > > > 1 file changed, 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index bfb2efd..532cc0f 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
> > > > * notifications.
> > > > * */
> > > > intel_hpd_init(dev_priv);
> > > > - /* Config may have changed between suspend and resume */
> > > > - drm_helper_hpd_irq_event(dev);
> > >
> > > The comment is still apt. This code is known to be broken since it
> > > doesn't detect a change in monitors (e.g. a change in external connectors
> > > from docking) between suspend and resend. We still have to send the uevent.
> > >
> > > + drm_kms_helper_hotplug_event(dev);
> >
> > I might not have explained myself very well. The way things should look with
> > this patch is like this:
> >
> > i915_drm_resume()
> > -> intel_hpd_init()
> > -> sets dev_priv->hotplug.poll_enabled to true
> Whoops, s/true/false/
>
> > -> schedules dev_priv->hotplug.poll_init_work
> > -> continue resume…
> >
> > at the same time:
> >
> > i915_hpd_poll_init_work() gets scheduled and starts
> > -> since dev_priv->hotplug.poll_enabled == false, drm_helper_hpd_irq_event()
> > is called
> > -> drm_helper_hpd_irq_event() reprobes connectors
> > -> if anything changed, drm_kms_helper_hotplug_event() gets called.
drm_helper_hpd_irq_event() does not detect a change in monitors, for
example, so there is no uevent in that case.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
On Thu, 2016-11-03 at 16:25 +0000, Chris Wilson wrote:
> On Thu, Nov 03, 2016 at 12:11:55PM -0400, Lyude Paul wrote:
> >
> > On Thu, 2016-11-03 at 12:11 -0400, Lyude Paul wrote:
> > >
> > > On Thu, 2016-11-03 at 16:02 +0000, Chris Wilson wrote:
> > > >
> > > >
> > > > On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> > > > >
> > > > >
> > > > >
> > > > > Weine's investigation on benchmarking the suspend/resume process
> > > > > pointed
> > > > > out a lot of the time in suspend/resume is being spent reprobing.
> > > > > While
> > > > > the reprobing process is a lengthy one for good reason, we don't need
> > > > > to
> > > > > hold up the entire suspend/resume process while we wait for it to
> > > > > finish. Luckily as it turns out, we already trigger a full connector
> > > > > reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing
> > > > > in
> > > > > i915_drm_resume() entirely.
> > > > >
> > > > > This won't lead to less time spent resuming just yet since now the
> > > > > bottleneck will be waiting for the mode_config lock in
> > > > > drm_kms_helper_poll_enable(), since that will be held as long as
> > > > > i915_hpd_poll_init_work() is reprobing all of the connectors. But
> > > > > we'll
> > > > > address that in the next patch.
> > > > >
> > > > > Signed-off-by: Lyude <[email protected]>
> > > > > Cc: David Weinehall <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_drv.c | 2 --
> > > > > 1 file changed, 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > > index bfb2efd..532cc0f 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device
> > > > > *dev)
> > > > > * notifications.
> > > > > * */
> > > > > intel_hpd_init(dev_priv);
> > > > > - /* Config may have changed between suspend and resume */
> > > > > - drm_helper_hpd_irq_event(dev);
> > > >
> > > > The comment is still apt. This code is known to be broken since it
> > > > doesn't detect a change in monitors (e.g. a change in external
> > > > connectors
> > > > from docking) between suspend and resend. We still have to send the
> > > > uevent.
> > > >
> > > > + drm_kms_helper_hotplug_event(dev);
> > >
> > > I might not have explained myself very well. The way things should look
> > > with
> > > this patch is like this:
> > >
> > > i915_drm_resume()
> > > -> intel_hpd_init()
> > > -> sets dev_priv->hotplug.poll_enabled to true
> > Whoops, s/true/false/
> >
> > >
> > > -> schedules dev_priv->hotplug.poll_init_work
> > > -> continue resume…
> > >
> > > at the same time:
> > >
> > > i915_hpd_poll_init_work() gets scheduled and starts
> > > -> since dev_priv->hotplug.poll_enabled == false,
> > > drm_helper_hpd_irq_event()
> > > is called
> > > -> drm_helper_hpd_irq_event() reprobes connectors
> > > -> if anything changed, drm_kms_helper_hotplug_event() gets called.
>
> drm_helper_hpd_irq_event() does not detect a change in monitors, for
> example, so there is no uevent in that case.
I still think you're mistaken (and I wouldn't blame you,
drm_helper_hpd_irq_event() has a rather misleading name). Contents
of drm_helper_hpd_irq_event()
bool drm_helper_hpd_irq_event(struct drm_device *dev)
{
struct drm_connector *connector;
enum drm_connector_status old_status;
bool changed = false;
if (!dev->mode_config.poll_enabled)
return false;
mutex_lock(&dev->mode_config.mutex);
drm_for_each_connector(connector, dev) {
/* Only handle HPD capable connectors. */
if (!(connector->polled & DRM_CONNECTOR_POLL_HPD))
continue;
old_status = connector->status;
connector->status = connector->funcs->detect(connector, false);
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to
%s\n",
connector->base.id,
connector->name,
drm_get_connector_status_name(old_status),
drm_get_connector_status_name(connector->status));
if (old_status != connector->status)
changed = true;
}
mutex_unlock(&dev->mode_config.mutex);
if (changed)
drm_kms_helper_hotplug_event(dev);
return changed;
}
Unless I made a mistake somewhere down the line of writing these: poll_enabled
will be set to true (due to the change done in the second patch where we move
the call for enabling polling up so that it gets called before
intel_hpd_init()), which causes it to go through all the connectors and call
connector->funcs->detect() (which will be intel_dp_detect(),
intel_hdmi_detect(), etc. And then send a uevent if their status changed.
This being said there's a couple of cases I know we don't handle correctly from
writing up igt tests with the chamelium: we don't simulate a disconnect +
connect when the EDID changed from what we had last, so if we suspend the system
with monitor A connected to port foo and resume it with monitor B connected to
port foo, it will still think we're connected to monitor A. However, we don't
handle this properly with the current code either.
As for connector->funcs->detect(), it does do a real connector reprobe as
opposed to just checking the status of the HPD pins, I'd look at the detect()
functions if you're still skeptical.
> -Chris
>
On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> Weine's investigation on benchmarking the suspend/resume process pointed
> out a lot of the time in suspend/resume is being spent reprobing. While
> the reprobing process is a lengthy one for good reason, we don't need to
> hold up the entire suspend/resume process while we wait for it to
> finish. Luckily as it turns out, we already trigger a full connector
> reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
> i915_drm_resume() entirely.
>
> This won't lead to less time spent resuming just yet since now the
> bottleneck will be waiting for the mode_config lock in
> drm_kms_helper_poll_enable(), since that will be held as long as
> i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
> address that in the next patch.
>
> Signed-off-by: Lyude <[email protected]>
> Cc: David Weinehall <[email protected]>
Tested-by: David Weinehall <[email protected]>
Reviewed-by: David Weinehall <[email protected]>
Testcase: analyze_suspend.py -config config/suspend-callgraph.cfg -filter i915
> ---
> drivers/gpu/drm/i915/i915_drv.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index bfb2efd..532cc0f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
> * notifications.
> * */
> intel_hpd_init(dev_priv);
> - /* Config may have changed between suspend and resume */
> - drm_helper_hpd_irq_event(dev);
>
> intel_opregion_register(dev_priv);
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Nov 03, 2016 at 11:42:38AM -0400, Lyude wrote:
> Now that we don't run the connector reprobing from i915_drm_resume(), we
> need to make it so we don't have to wait for reprobing to finish so that
> we actually speed things up. In order to do this, we need to make sure
> that i915_drm_resume() doesn't get blocked by i915_hpd_poll_init_work()
> while trying to acquire the mode_config lock that
> drm_kms_helper_poll_enable() needs to acquire.
>
> The easiest way to do this is to just enable polling before hpd. This
> shouldn't break anything since at that point we have everything else we
> need for polling enabled.
>
> As well, this should result in a rather significant improvement in how
> quickly we can resume the system.
>
> Signed-off-by: Lyude <[email protected]>
> Cc: David Weinehall <[email protected]>
Tested-by: David Weinehall <[email protected]>
Reviewed-by: David Weinehall <[email protected]>
Testcase: analyze_suspend.py -config config/suspend-callgraph.cfg -filter i915
> ---
> drivers/gpu/drm/i915/i915_drv.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 532cc0f..f605dde 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1595,6 +1595,8 @@ static int i915_drm_resume(struct drm_device *dev)
>
> intel_display_resume(dev);
>
> + drm_kms_helper_poll_enable(dev);
> +
> /*
> * ... but also need to make sure that hotplug processing
> * doesn't cause havoc. Like in the driver load code we don't
> @@ -1614,7 +1616,6 @@ static int i915_drm_resume(struct drm_device *dev)
> intel_opregion_notify_adapter(dev_priv, PCI_D0);
>
> intel_autoenable_gt_powersave(dev_priv);
> - drm_kms_helper_poll_enable(dev);
>
> enable_rpm_wakeref_asserts(dev_priv);
>
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx