| Message ID | 1412713523-14011-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 07, 2014 at 01:25:23PM -0700, Jesse Barnes wrote: > Gets the detect code (which may take awhile) out of the resume path, > speeding things up a bit. > > v2: use a delayed work queue instead (Daniel) > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_dma.c | 10 ++++++++++ > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > drivers/gpu/drm/i915/i915_drv.h | 1 + > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 85d14e1..633095d 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1303,6 +1303,15 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { > .can_switch = i915_switcheroo_can_switch, > }; > > +static void intel_resume_hotplug(struct work_struct *work) > +{ > + struct drm_i915_private *dev_priv = > + container_of(work, struct drm_i915_private, > + hotplug_resume_work.work); > + > + drm_helper_hpd_irq_event(dev_priv->dev); > +} > + > static int i915_load_modeset_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -1364,6 +1373,7 @@ static int i915_load_modeset_init(struct drm_device *dev) > > /* Only enable hotplug handling once the fbdev is fully set up. */ > intel_hpd_init(dev_priv); > + INIT_DELAYED_WORK(&dev_priv->hotplug_resume_work, intel_resume_hotplug); > > /* > * Some ports require correctly set-up hpd registers for detection to > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index a05a1d0..83075f9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -726,8 +726,12 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > * notifications. > * */ > intel_hpd_init(dev_priv); > - /* Config may have changed between suspend and resume */ > - drm_helper_hpd_irq_event(dev); > + /* Config may have changed between suspend and resume. Just > + * try to make sure the rest of driver resume is finished > + * before we start probing for config changes. This is a nice vague statement that scares me. If half of this comment is true, using a simple delay is periliously lackadaisical. The commit log just talks about speeding up resume, but this comment implies to me something more sinister. -Chris
On Wed, 8 Oct 2014 07:43:34 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, Oct 07, 2014 at 01:25:23PM -0700, Jesse Barnes wrote: > > Gets the detect code (which may take awhile) out of the resume path, > > speeding things up a bit. > > > > v2: use a delayed work queue instead (Daniel) > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_dma.c | 10 ++++++++++ > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 3 files changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > index 85d14e1..633095d 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1303,6 +1303,15 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { > > .can_switch = i915_switcheroo_can_switch, > > }; > > > > +static void intel_resume_hotplug(struct work_struct *work) > > +{ > > + struct drm_i915_private *dev_priv = > > + container_of(work, struct drm_i915_private, > > + hotplug_resume_work.work); > > + > > + drm_helper_hpd_irq_event(dev_priv->dev); > > +} > > + > > static int i915_load_modeset_init(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -1364,6 +1373,7 @@ static int i915_load_modeset_init(struct drm_device *dev) > > > > /* Only enable hotplug handling once the fbdev is fully set up. */ > > intel_hpd_init(dev_priv); > > + INIT_DELAYED_WORK(&dev_priv->hotplug_resume_work, intel_resume_hotplug); > > > > /* > > * Some ports require correctly set-up hpd registers for detection to > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index a05a1d0..83075f9 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -726,8 +726,12 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > * notifications. > > * */ > > intel_hpd_init(dev_priv); > > - /* Config may have changed between suspend and resume */ > > - drm_helper_hpd_irq_event(dev); > > + /* Config may have changed between suspend and resume. Just > > + * try to make sure the rest of driver resume is finished > > + * before we start probing for config changes. > > This is a nice vague statement that scares me. If half of this comment > is true, using a simple delay is periliously lackadaisical. The commit > log just talks about speeding up resume, but this comment implies to me > something more sinister. Nothing sinister; nothing should blow up if we end up firing this before resume completes. We mainly want to avoid hitting lock contention from the mode set above if it ever becomes async. So a rough estimate ought to be fine (there's really nothing that should take long after this point anyway). Jesse
On Wed, Oct 08, 2014 at 07:32:12AM -0700, Jesse Barnes wrote: > On Wed, 8 Oct 2014 07:43:34 +0100 > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > On Tue, Oct 07, 2014 at 01:25:23PM -0700, Jesse Barnes wrote: > > > Gets the detect code (which may take awhile) out of the resume path, > > > speeding things up a bit. > > > > > > v2: use a delayed work queue instead (Daniel) > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > --- > > > drivers/gpu/drm/i915/i915_dma.c | 10 ++++++++++ > > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > 3 files changed, 17 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > index 85d14e1..633095d 100644 > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > @@ -1303,6 +1303,15 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { > > > .can_switch = i915_switcheroo_can_switch, > > > }; > > > > > > +static void intel_resume_hotplug(struct work_struct *work) > > > +{ > > > + struct drm_i915_private *dev_priv = > > > + container_of(work, struct drm_i915_private, > > > + hotplug_resume_work.work); > > > + > > > + drm_helper_hpd_irq_event(dev_priv->dev); > > > +} > > > + > > > static int i915_load_modeset_init(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > @@ -1364,6 +1373,7 @@ static int i915_load_modeset_init(struct drm_device *dev) > > > > > > /* Only enable hotplug handling once the fbdev is fully set up. */ > > > intel_hpd_init(dev_priv); > > > + INIT_DELAYED_WORK(&dev_priv->hotplug_resume_work, intel_resume_hotplug); > > > > > > /* > > > * Some ports require correctly set-up hpd registers for detection to > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index a05a1d0..83075f9 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -726,8 +726,12 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > > * notifications. > > > * */ > > > intel_hpd_init(dev_priv); > > > - /* Config may have changed between suspend and resume */ > > > - drm_helper_hpd_irq_event(dev); > > > + /* Config may have changed between suspend and resume. Just > > > + * try to make sure the rest of driver resume is finished > > > + * before we start probing for config changes. /* Config may have changed between suspend and resume, queue a hotplug * notification for userspace to check when it wakes up. Delay the work * slightly so that the resume has time to finish, before userspace * starts pounding at the gates checking for changes. This speeds up the * resume process by reducing contention for resources. */
On Thu, 9 Oct 2014 11:11:32 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Oct 08, 2014 at 07:32:12AM -0700, Jesse Barnes wrote: > > On Wed, 8 Oct 2014 07:43:34 +0100 > > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > On Tue, Oct 07, 2014 at 01:25:23PM -0700, Jesse Barnes wrote: > > > > Gets the detect code (which may take awhile) out of the resume path, > > > > speeding things up a bit. > > > > > > > > v2: use a delayed work queue instead (Daniel) > > > > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > --- > > > > drivers/gpu/drm/i915/i915_dma.c | 10 ++++++++++ > > > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > 3 files changed, 17 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > > index 85d14e1..633095d 100644 > > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > > @@ -1303,6 +1303,15 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { > > > > .can_switch = i915_switcheroo_can_switch, > > > > }; > > > > > > > > +static void intel_resume_hotplug(struct work_struct *work) > > > > +{ > > > > + struct drm_i915_private *dev_priv = > > > > + container_of(work, struct drm_i915_private, > > > > + hotplug_resume_work.work); > > > > + > > > > + drm_helper_hpd_irq_event(dev_priv->dev); > > > > +} > > > > + > > > > static int i915_load_modeset_init(struct drm_device *dev) > > > > { > > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > @@ -1364,6 +1373,7 @@ static int i915_load_modeset_init(struct drm_device *dev) > > > > > > > > /* Only enable hotplug handling once the fbdev is fully set up. */ > > > > intel_hpd_init(dev_priv); > > > > + INIT_DELAYED_WORK(&dev_priv->hotplug_resume_work, intel_resume_hotplug); > > > > > > > > /* > > > > * Some ports require correctly set-up hpd registers for detection to > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index a05a1d0..83075f9 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -726,8 +726,12 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > > > * notifications. > > > > * */ > > > > intel_hpd_init(dev_priv); > > > > - /* Config may have changed between suspend and resume */ > > > > - drm_helper_hpd_irq_event(dev); > > > > + /* Config may have changed between suspend and resume. Just > > > > + * try to make sure the rest of driver resume is finished > > > > + * before we start probing for config changes. > > /* Config may have changed between suspend and resume, queue a hotplug > * notification for userspace to check when it wakes up. Delay the work > * slightly so that the resume has time to finish, before userspace > * starts pounding at the gates checking for changes. This speeds up the > * resume process by reducing contention for resources. > */ Yeah looks good and a bit less scary. Jesse
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 85d14e1..633095d 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1303,6 +1303,15 @@ static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { .can_switch = i915_switcheroo_can_switch, }; +static void intel_resume_hotplug(struct work_struct *work) +{ + struct drm_i915_private *dev_priv = + container_of(work, struct drm_i915_private, + hotplug_resume_work.work); + + drm_helper_hpd_irq_event(dev_priv->dev); +} + static int i915_load_modeset_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -1364,6 +1373,7 @@ static int i915_load_modeset_init(struct drm_device *dev) /* Only enable hotplug handling once the fbdev is fully set up. */ intel_hpd_init(dev_priv); + INIT_DELAYED_WORK(&dev_priv->hotplug_resume_work, intel_resume_hotplug); /* * Some ports require correctly set-up hpd registers for detection to diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index a05a1d0..83075f9 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -726,8 +726,12 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) * notifications. * */ intel_hpd_init(dev_priv); - /* Config may have changed between suspend and resume */ - drm_helper_hpd_irq_event(dev); + /* Config may have changed between suspend and resume. Just + * try to make sure the rest of driver resume is finished + * before we start probing for config changes. + */ + schedule_delayed_work(&dev_priv->hotplug_resume_work, + msecs_to_jiffies(50)); } intel_opregion_init(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1e476b5..6273ad6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1521,6 +1521,7 @@ struct drm_i915_private { } hpd_stats[HPD_NUM_PINS]; u32 hpd_event_bits; struct delayed_work hotplug_reenable_work; + struct delayed_work hotplug_resume_work; struct i915_fbc fbc; struct i915_drrs drrs;
Gets the detect code (which may take awhile) out of the resume path, speeding things up a bit. v2: use a delayed work queue instead (Daniel) Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_dma.c | 10 ++++++++++ drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- drivers/gpu/drm/i915/i915_drv.h | 1 + 3 files changed, 17 insertions(+), 2 deletions(-)