[PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload

[PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload

[Imre Deak]

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

Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload

[Ville Syrjälä]

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

Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload

[Imre Deak]

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

Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload

[Ville Syrjälä]

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

Re: [PATCH 2/4] drm/i915: cancel hotplug and dig_port work during suspend and unload

[Imre Deak]

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