[v3,7/7] drm/i915/perf: remove perf.hook_lock

Commit Message

Robert Bragg April 5, 2017, 4:23 p.m. UTC

In earlier iterations of the i915-perf driver we had a number of
callbacks/hooks from other parts of the i915 driver to e.g. notify us
when a legacy context was pinned and these could run asynchronously with
respect to the stream file operations and might also run in atomic
context.

dev_priv->perf.hook_lock had been for serialising access to state needed
within these callbacks, but as the code has evolved some of the hooks
have gone away or are implemented to avoid needing to lock any state.

The remaining use of this lock was actually redundant considering how
the gen7 oacontrol state used to be updated as part of a context pin
hook.

Signed-off-by: Robert Bragg <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 --
 drivers/gpu/drm/i915/i915_perf.c | 32 ++++++++++----------------------
 2 files changed, 10 insertions(+), 24 deletions(-)

Comments

Matthew Auld April 12, 2017, 11:36 a.m. UTC | #1

On 04/05, Robert Bragg wrote:
> In earlier iterations of the i915-perf driver we had a number of
> callbacks/hooks from other parts of the i915 driver to e.g. notify us
> when a legacy context was pinned and these could run asynchronously with
> respect to the stream file operations and might also run in atomic
> context.
> 
> dev_priv->perf.hook_lock had been for serialising access to state needed
> within these callbacks, but as the code has evolved some of the hooks
> have gone away or are implemented to avoid needing to lock any state.
> 
> The remaining use of this lock was actually redundant considering how
> the gen7 oacontrol state used to be updated as part of a context pin
> hook.
> 
> Signed-off-by: Robert Bragg <[e-mail]>
Thanks for following up with a patch.

Reviewed-by: Matthew Auld <[e-mail]>

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 48b07d706f06..67ac4e6dbccb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2444,8 +2444,6 @@  struct drm_i915_private {
 		struct mutex lock;
 		struct list_head streams;
 
-		spinlock_t hook_lock;
-
 		struct {
 			struct i915_perf_stream *exclusive_stream;
 
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 87c0d1ce1b9f..63a1152766f8 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1677,9 +1677,17 @@  static void gen8_disable_metric_set(struct drm_i915_private *dev_priv)
 	/* NOP */
 }
 
-static void gen7_update_oacontrol_locked(struct drm_i915_private *dev_priv)
+static void gen7_oa_enable(struct drm_i915_private *dev_priv)
 {
-	lockdep_assert_held(&dev_priv->perf.hook_lock);
+	/* Reset buf pointers so we don't forward reports from before now.
+	 *
+	 * Think carefully if considering trying to avoid this, since it
+	 * also ensures status flags and the buffer itself are cleared
+	 * in error paths, and we have checks for invalid reports based
+	 * on the assumption that certain fields are written to zeroed
+	 * memory which this helps maintains.
+	 */
+	gen7_init_oa_buffer(dev_priv);
 
 	if (dev_priv->perf.oa.exclusive_stream->enabled) {
 		struct i915_gem_context *ctx =
@@ -1702,25 +1710,6 @@  static void gen7_update_oacontrol_locked(struct drm_i915_private *dev_priv)
 		I915_WRITE(GEN7_OACONTROL, 0);
 }
 
-static void gen7_oa_enable(struct drm_i915_private *dev_priv)
-{
-	unsigned long flags;
-
-	/* Reset buf pointers so we don't forward reports from before now.
-	 *
-	 * Think carefully if considering trying to avoid this, since it
-	 * also ensures status flags and the buffer itself are cleared
-	 * in error paths, and we have checks for invalid reports based
-	 * on the assumption that certain fields are written to zeroed
-	 * memory which this helps maintains.
-	 */
-	gen7_init_oa_buffer(dev_priv);
-
-	spin_lock_irqsave(&dev_priv->perf.hook_lock, flags);
-	gen7_update_oacontrol_locked(dev_priv);
-	spin_unlock_irqrestore(&dev_priv->perf.hook_lock, flags);
-}
-
 static void gen8_oa_enable(struct drm_i915_private *dev_priv)
 {
 	u32 report_format = dev_priv->perf.oa.oa_buffer.format;
@@ -2999,7 +2988,6 @@  void i915_perf_init(struct drm_i915_private *dev_priv)
 
 		INIT_LIST_HEAD(&dev_priv->perf.streams);
 		mutex_init(&dev_priv->perf.lock);
-		spin_lock_init(&dev_priv->perf.hook_lock);
 		spin_lock_init(&dev_priv->perf.oa.oa_buffer.ptr_lock);
 
 		dev_priv->perf.sysctl_header = register_sysctl_table(dev_root);