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@xxxxxxxxx> > > > > --- > > > > 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:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx