Re: [PATCH 09/10] drm/i915: add support for checking RPM atomic sections

[Imre Deak <imre.deak@intel.com>]

On Wed, 2015-12-16 at 12:18 +0100, Daniel Vetter wrote:
> On Tue, Dec 15, 2015 at 08:10:37PM +0200, Imre Deak wrote:
> > In some cases we want to check whether we hold an RPM wakelock
> > reference
> > for the whole duration of a sequence. To achieve this add a new RPM
> > atomic sequence
> > counter that we increment any time the wakelock refcount drops to
> > zero.
> > Check whether the sequence number stays the same during the atomic
> > section and that we hold the wakelock at the beginning of the
> > section.
> > 
> > Motivated by Chris.
> > 
> > v2-v3:
> > - unchanged
> > v4:
> > - swap the order of atomic_read() and assert_rpm_wakelock_held() in
> >   assert_rpm_atomic_begin() to avoid race
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v3)
> 
> Can we employ lockdep for some of this? In general lockdep doesn't
> mesh
> with rpm references, since rpm references can be transferred between
> processes. And lockdep doesn't like that.
> 
> But in many cases, and this one here sounds like one, we don't do
> that.
> And those cases could be annotated/validated with the help of
> lockdep.
> The bonus of lockdep is that we could then also tell lockdep that the
> rpm
> suspend/resume functionsa acquire this lock context, which would
> catch
> bugs like mutex_lock(dev->struct_mutex); before rpm_get().
> 
> Just a thought really.

Yes, as I mentioned I also played with the idea and it could be done by
separating the RPM wakelock users into "prolonged" and short time
users. Yea, couldn't think of a better name. I explained what I mean in
the top three patch of:
https://github.com/ideak/linux/commits/rpm-lockdep-annotations

But I would still need to do more testing with this, making sure the
annotations work as expected, in particular that I call lock_acquire()
with the proper parameters. So until that we could use this simpler
check that could reveal some issues already now.

--Imre

> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |  1 +
> >  drivers/gpu/drm/i915/intel_drv.h        | 17 +++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.c         |  1 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++-
> >  4 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 2894716..00ce627 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1603,6 +1603,7 @@ struct skl_wm_level {
> >   */
> >  struct i915_runtime_pm {
> >  	atomic_t wakeref_count;
> > +	atomic_t atomic_seq;
> >  	bool suspended;
> >  	bool irqs_enabled;
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d8e4aca..88d37eb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1446,6 +1446,23 @@ assert_rpm_wakelock_held(struct
> > drm_i915_private *dev_priv)
> >  		  "RPM wakelock ref not held during HW access");
> >  }
> >  
> > +static inline int
> > +assert_rpm_atomic_begin(struct drm_i915_private *dev_priv)
> > +{
> > +	int seq = atomic_read(&dev_priv->pm.atomic_seq);
> > +
> > +	assert_rpm_wakelock_held(dev_priv);
> > +
> > +	return seq;
> > +}
> > +
> > +static inline void
> > +assert_rpm_atomic_end(struct drm_i915_private *dev_priv, int
> > begin_seq)
> > +{
> > +	WARN_ONCE(atomic_read(&dev_priv->pm.atomic_seq) !=
> > begin_seq,
> > +		 "HW access outside of RPM atomic section\n");
> > +}
> > +
> >  /**
> >   * disable_rpm_wakeref_asserts - disable the RPM assert checks
> >   * @dev_priv: i915 device instance
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 6c08537..6eb9606 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7248,4 +7248,5 @@ void intel_pm_setup(struct drm_device *dev)
> >  
> >  	dev_priv->pm.suspended = false;
> >  	atomic_set(&dev_priv->pm.wakeref_count, 0);
> > +	atomic_set(&dev_priv->pm.atomic_seq, 0);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 82c55a9..cee54ea 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -2283,7 +2283,8 @@ void intel_runtime_pm_put(struct
> > drm_i915_private *dev_priv)
> >  	struct device *device = &dev->pdev->dev;
> >  
> >  	assert_rpm_wakelock_held(dev_priv);
> > -	atomic_dec(&dev_priv->pm.wakeref_count);
> > +	if (atomic_dec_and_test(&dev_priv->pm.wakeref_count))
> > +		atomic_inc(&dev_priv->pm.atomic_seq);
> >  
> >  	pm_runtime_mark_last_busy(device);
> >  	pm_runtime_put_autosuspend(device);
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx