* [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload @ 2014-08-11 18:54 Imre Deak 2014-08-12 12:13 ` Ville Syrjälä 0 siblings, 1 reply; 5+ messages in thread From: Imre Deak @ 2014-08-11 18:54 UTC (permalink / raw) To: intel-gfx Make sure these work handlers don't run after we system suspend or unload the driver. Note that we don't cancel the handlers during runtime suspend. That could lead to a lockup, since we take a runtime PM ref from the handlers themselves. Fortunaltely canceling there is not needed since the RPM ref itself provides for the needed serialization. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_display.c | 3 +-- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index ec96f9a..0653761 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -494,6 +494,13 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) return true; } +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) +{ + cancel_work_sync(&dev_priv->hotplug_work); + cancel_work_sync(&dev_priv->dig_port_work); + cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work); +} + static int i915_drm_freeze(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -538,6 +545,7 @@ static int i915_drm_freeze(struct drm_device *dev) flush_delayed_work(&dev_priv->rps.delayed_resume_work); intel_runtime_pm_disable_interrupts(dev); + intel_hpd_cancel_work(dev_priv); intel_suspend_gt_powersave(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 35d150a..8776847 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2218,6 +2218,7 @@ extern void i915_update_gfx_val(struct drm_i915_private *dev_priv); int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on); extern void intel_console_resume(struct work_struct *work); +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv); /* i915_irq.c */ void i915_queue_hangcheck(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b3a3168..ab90301 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13100,8 +13100,7 @@ void intel_modeset_cleanup(struct drm_device *dev) * experience fancy races otherwise. */ drm_irq_uninstall(dev); - cancel_work_sync(&dev_priv->hotplug_work); - cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work); + intel_hpd_cancel_work(dev_priv); dev_priv->pm._irqs_disabled = true; /* -- 1.8.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload 2014-08-11 18:54 [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload Imre Deak @ 2014-08-12 12:13 ` Ville Syrjälä 2014-08-12 12:36 ` Imre Deak 0 siblings, 1 reply; 5+ messages in thread From: Ville Syrjälä @ 2014-08-12 12:13 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Mon, Aug 11, 2014 at 09:54:15PM +0300, Imre Deak wrote: > Make sure these work handlers don't run after we system suspend or > unload the driver. Note that we don't cancel the handlers during runtime > suspend. That could lead to a lockup, since we take a runtime PM ref > from the handlers themselves. Fortunaltely canceling there is not needed > since the RPM ref itself provides for the needed serialization. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 3 +-- > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index ec96f9a..0653761 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -494,6 +494,13 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > return true; > } > > +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) > +{ > + cancel_work_sync(&dev_priv->hotplug_work); > + cancel_work_sync(&dev_priv->dig_port_work); Since dig_port_work can queue a hotplug_work shouldn't these two be swapped? I wonder if we should also clear hpd_event_bits and {long,short}_hpd_port_mask before cancelling the works? At least it might make the works end a bit quicker if the are already running. I also noticed that we don't seem to grab any rpm/powerwell references in ->hpd_pulse() or i915_digport_work_func(). That doesn't seem right. Or maybe you already addressed that in another patch? > + cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work); > +} > + > static int i915_drm_freeze(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -538,6 +545,7 @@ static int i915_drm_freeze(struct drm_device *dev) > flush_delayed_work(&dev_priv->rps.delayed_resume_work); > > intel_runtime_pm_disable_interrupts(dev); > + intel_hpd_cancel_work(dev_priv); > > intel_suspend_gt_powersave(dev); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 35d150a..8776847 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2218,6 +2218,7 @@ extern void i915_update_gfx_val(struct drm_i915_private *dev_priv); > int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on); > > extern void intel_console_resume(struct work_struct *work); > +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv); > > /* i915_irq.c */ > void i915_queue_hangcheck(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index b3a3168..ab90301 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13100,8 +13100,7 @@ void intel_modeset_cleanup(struct drm_device *dev) > * experience fancy races otherwise. > */ > drm_irq_uninstall(dev); > - cancel_work_sync(&dev_priv->hotplug_work); > - cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work); > + intel_hpd_cancel_work(dev_priv); > dev_priv->pm._irqs_disabled = true; > > /* > -- > 1.8.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload 2014-08-12 12:13 ` Ville Syrjälä @ 2014-08-12 12:36 ` Imre Deak 2014-08-12 12:53 ` Ville Syrjälä 0 siblings, 1 reply; 5+ messages in thread From: Imre Deak @ 2014-08-12 12:36 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 2345 bytes --] On Tue, 2014-08-12 at 15:13 +0300, Ville Syrjälä wrote: > On Mon, Aug 11, 2014 at 09:54:15PM +0300, Imre Deak wrote: > > Make sure these work handlers don't run after we system suspend or > > unload the driver. Note that we don't cancel the handlers during runtime > > suspend. That could lead to a lockup, since we take a runtime PM ref > > from the handlers themselves. Fortunaltely canceling there is not needed > > since the RPM ref itself provides for the needed serialization. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++ > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_display.c | 3 +-- > > 3 files changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index ec96f9a..0653761 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -494,6 +494,13 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > > return true; > > } > > > > +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) > > +{ > > + cancel_work_sync(&dev_priv->hotplug_work); > > + cancel_work_sync(&dev_priv->dig_port_work); > > Since dig_port_work can queue a hotplug_work shouldn't these two be > swapped? Right, will fix that. > I wonder if we should also clear hpd_event_bits and > {long,short}_hpd_port_mask before cancelling the works? At least it > might make the works end a bit quicker if the are already running. Makes sense for speed, will fix it. Another thing is that a final instance of these works can now run with interrupts disabled that could cause DP AUX timeouts for example. That could be avoided for example by adding a new dev_priv->hpd_irqs_disabled flag and setting it before disabling interrupts, but I didn't want to make things more complicated before getting some feedback. > I also noticed that we don't seem to grab any rpm/powerwell references > in ->hpd_pulse() or i915_digport_work_func(). That doesn't seem right. > Or maybe you already addressed that in another patch? No, I haven't. I thought it's enough that all low level functions like DPCD read, link training do take already the needed refs. Isn't that enough? --Imre [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 490 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload 2014-08-12 12:36 ` Imre Deak @ 2014-08-12 12:53 ` Ville Syrjälä 2014-08-12 13:39 ` Imre Deak 0 siblings, 1 reply; 5+ messages in thread From: Ville Syrjälä @ 2014-08-12 12:53 UTC (permalink / raw) To: Imre Deak; +Cc: intel-gfx On Tue, Aug 12, 2014 at 03:36:01PM +0300, Imre Deak wrote: > On Tue, 2014-08-12 at 15:13 +0300, Ville Syrjälä wrote: > > On Mon, Aug 11, 2014 at 09:54:15PM +0300, Imre Deak wrote: > > > Make sure these work handlers don't run after we system suspend or > > > unload the driver. Note that we don't cancel the handlers during runtime > > > suspend. That could lead to a lockup, since we take a runtime PM ref > > > from the handlers themselves. Fortunaltely canceling there is not needed > > > since the RPM ref itself provides for the needed serialization. > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++ > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_display.c | 3 +-- > > > 3 files changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index ec96f9a..0653761 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -494,6 +494,13 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > > > return true; > > > } > > > > > > +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) > > > +{ > > > + cancel_work_sync(&dev_priv->hotplug_work); > > > + cancel_work_sync(&dev_priv->dig_port_work); > > > > Since dig_port_work can queue a hotplug_work shouldn't these two be > > swapped? > > Right, will fix that. > > > I wonder if we should also clear hpd_event_bits and > > {long,short}_hpd_port_mask before cancelling the works? At least it > > might make the works end a bit quicker if the are already running. > > Makes sense for speed, will fix it. Another thing is that a final > instance of these works can now run with interrupts disabled that could > cause DP AUX timeouts for example. That could be avoided for example by > adding a new dev_priv->hpd_irqs_disabled flag and setting it before > disabling interrupts, but I didn't want to make things more complicated > before getting some feedback. > > > I also noticed that we don't seem to grab any rpm/powerwell references > > in ->hpd_pulse() or i915_digport_work_func(). That doesn't seem right. > > Or maybe you already addressed that in another patch? > > No, I haven't. I thought it's enough that all low level functions like > DPCD read, link training do take already the needed refs. Isn't that > enough? There's at least the call to ibx_digital_port_connected() which isn't covered by any rpm/powerwell reference. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload 2014-08-12 12:53 ` Ville Syrjälä @ 2014-08-12 13:39 ` Imre Deak 0 siblings, 0 replies; 5+ messages in thread From: Imre Deak @ 2014-08-12 13:39 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 2978 bytes --] On Tue, 2014-08-12 at 15:53 +0300, Ville Syrjälä wrote: > On Tue, Aug 12, 2014 at 03:36:01PM +0300, Imre Deak wrote: > > On Tue, 2014-08-12 at 15:13 +0300, Ville Syrjälä wrote: > > > On Mon, Aug 11, 2014 at 09:54:15PM +0300, Imre Deak wrote: > > > > Make sure these work handlers don't run after we system suspend or > > > > unload the driver. Note that we don't cancel the handlers during runtime > > > > suspend. That could lead to a lockup, since we take a runtime PM ref > > > > from the handlers themselves. Fortunaltely canceling there is not needed > > > > since the RPM ref itself provides for the needed serialization. > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++ > > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > > drivers/gpu/drm/i915/intel_display.c | 3 +-- > > > > 3 files changed, 10 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index ec96f9a..0653761 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -494,6 +494,13 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) > > > > return true; > > > > } > > > > > > > > +void intel_hpd_cancel_work(struct drm_i915_private *dev_priv) > > > > +{ > > > > + cancel_work_sync(&dev_priv->hotplug_work); > > > > + cancel_work_sync(&dev_priv->dig_port_work); > > > > > > Since dig_port_work can queue a hotplug_work shouldn't these two be > > > swapped? > > > > Right, will fix that. > > > > > I wonder if we should also clear hpd_event_bits and > > > {long,short}_hpd_port_mask before cancelling the works? At least it > > > might make the works end a bit quicker if the are already running. > > > > Makes sense for speed, will fix it. Another thing is that a final > > instance of these works can now run with interrupts disabled that could > > cause DP AUX timeouts for example. That could be avoided for example by > > adding a new dev_priv->hpd_irqs_disabled flag and setting it before > > disabling interrupts, but I didn't want to make things more complicated > > before getting some feedback. > > > > > I also noticed that we don't seem to grab any rpm/powerwell references > > > in ->hpd_pulse() or i915_digport_work_func(). That doesn't seem right. > > > Or maybe you already addressed that in another patch? > > > > No, I haven't. I thought it's enough that all low level functions like > > DPCD read, link training do take already the needed refs. Isn't that > > enough? > > There's at least the call to ibx_digital_port_connected() which isn't > covered by any rpm/powerwell reference. Right, haven't noticed :/ I can fix this by taking the display port power domain around ->hpd_pulse(), similar to all the other DP encoder/connector handlers (get_modes, detect etc.). --Imre [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 490 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-08-12 13:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-11 18:54 [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload Imre Deak 2014-08-12 12:13 ` Ville Syrjälä 2014-08-12 12:36 ` Imre Deak 2014-08-12 12:53 ` Ville Syrjälä 2014-08-12 13:39 ` Imre Deak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox