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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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