Commit f616f283 authored by Lionel Landwerlin's avatar Lionel Landwerlin Committed by Rodrigo Vivi

drm/i915/perf: fix perf stream opening lock

We're seeing on CI that some contexts don't have the programmed OA
period timer that directs the OA unit on how often to write reports.

The issue is that we're not holding the drm lock from when we edit the
context images down to when we set the exclusive_stream variable. This
leaves a window for the deferred context allocation to call
i915_oa_init_reg_state() that will not program the expected OA timer
value, because we haven't set the exclusive_stream yet.

v2: Drop need_lock from gen8_configure_all_contexts() (Matt)
Signed-off-by: default avatarLionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: default avatarMatthew Auld <matthew.auld@intel.com>
Reviewed-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
Fixes: 701f8231 ("drm/i915/perf: prune OA configs")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102254
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103715
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103755
Link: https://patchwork.freedesktop.org/patch/msgid/20180301110613.1737-1-lionel.g.landwerlin@intel.com
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.14+
(cherry picked from commit 41d3fdcd)
Signed-off-by: default avatarRodrigo Vivi <rodrigo.vivi@intel.com>
parent 661e50bc
...@@ -1303,9 +1303,8 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream) ...@@ -1303,9 +1303,8 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
*/ */
mutex_lock(&dev_priv->drm.struct_mutex); mutex_lock(&dev_priv->drm.struct_mutex);
dev_priv->perf.oa.exclusive_stream = NULL; dev_priv->perf.oa.exclusive_stream = NULL;
mutex_unlock(&dev_priv->drm.struct_mutex);
dev_priv->perf.oa.ops.disable_metric_set(dev_priv); dev_priv->perf.oa.ops.disable_metric_set(dev_priv);
mutex_unlock(&dev_priv->drm.struct_mutex);
free_oa_buffer(dev_priv); free_oa_buffer(dev_priv);
...@@ -1756,22 +1755,13 @@ static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_pr ...@@ -1756,22 +1755,13 @@ static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_pr
* Note: it's only the RCS/Render context that has any OA state. * Note: it's only the RCS/Render context that has any OA state.
*/ */
static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv, static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
const struct i915_oa_config *oa_config, const struct i915_oa_config *oa_config)
bool interruptible)
{ {
struct i915_gem_context *ctx; struct i915_gem_context *ctx;
int ret; int ret;
unsigned int wait_flags = I915_WAIT_LOCKED; unsigned int wait_flags = I915_WAIT_LOCKED;
if (interruptible) { lockdep_assert_held(&dev_priv->drm.struct_mutex);
ret = i915_mutex_lock_interruptible(&dev_priv->drm);
if (ret)
return ret;
wait_flags |= I915_WAIT_INTERRUPTIBLE;
} else {
mutex_lock(&dev_priv->drm.struct_mutex);
}
/* Switch away from any user context. */ /* Switch away from any user context. */
ret = gen8_switch_to_updated_kernel_context(dev_priv, oa_config); ret = gen8_switch_to_updated_kernel_context(dev_priv, oa_config);
...@@ -1819,8 +1809,6 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv, ...@@ -1819,8 +1809,6 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
} }
out: out:
mutex_unlock(&dev_priv->drm.struct_mutex);
return ret; return ret;
} }
...@@ -1863,7 +1851,7 @@ static int gen8_enable_metric_set(struct drm_i915_private *dev_priv, ...@@ -1863,7 +1851,7 @@ static int gen8_enable_metric_set(struct drm_i915_private *dev_priv,
* to make sure all slices/subslices are ON before writing to NOA * to make sure all slices/subslices are ON before writing to NOA
* registers. * registers.
*/ */
ret = gen8_configure_all_contexts(dev_priv, oa_config, true); ret = gen8_configure_all_contexts(dev_priv, oa_config);
if (ret) if (ret)
return ret; return ret;
...@@ -1878,7 +1866,7 @@ static int gen8_enable_metric_set(struct drm_i915_private *dev_priv, ...@@ -1878,7 +1866,7 @@ static int gen8_enable_metric_set(struct drm_i915_private *dev_priv,
static void gen8_disable_metric_set(struct drm_i915_private *dev_priv) static void gen8_disable_metric_set(struct drm_i915_private *dev_priv)
{ {
/* Reset all contexts' slices/subslices configurations. */ /* Reset all contexts' slices/subslices configurations. */
gen8_configure_all_contexts(dev_priv, NULL, false); gen8_configure_all_contexts(dev_priv, NULL);
I915_WRITE(GDT_CHICKEN_BITS, (I915_READ(GDT_CHICKEN_BITS) & I915_WRITE(GDT_CHICKEN_BITS, (I915_READ(GDT_CHICKEN_BITS) &
~GT_NOA_ENABLE)); ~GT_NOA_ENABLE));
...@@ -1888,7 +1876,7 @@ static void gen8_disable_metric_set(struct drm_i915_private *dev_priv) ...@@ -1888,7 +1876,7 @@ static void gen8_disable_metric_set(struct drm_i915_private *dev_priv)
static void gen10_disable_metric_set(struct drm_i915_private *dev_priv) static void gen10_disable_metric_set(struct drm_i915_private *dev_priv)
{ {
/* Reset all contexts' slices/subslices configurations. */ /* Reset all contexts' slices/subslices configurations. */
gen8_configure_all_contexts(dev_priv, NULL, false); gen8_configure_all_contexts(dev_priv, NULL);
/* Make sure we disable noa to save power. */ /* Make sure we disable noa to save power. */
I915_WRITE(RPM_CONFIG1, I915_WRITE(RPM_CONFIG1,
...@@ -2138,6 +2126,10 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream, ...@@ -2138,6 +2126,10 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
if (ret) if (ret)
goto err_oa_buf_alloc; goto err_oa_buf_alloc;
ret = i915_mutex_lock_interruptible(&dev_priv->drm);
if (ret)
goto err_lock;
ret = dev_priv->perf.oa.ops.enable_metric_set(dev_priv, ret = dev_priv->perf.oa.ops.enable_metric_set(dev_priv,
stream->oa_config); stream->oa_config);
if (ret) if (ret)
...@@ -2145,23 +2137,17 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream, ...@@ -2145,23 +2137,17 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
stream->ops = &i915_oa_stream_ops; stream->ops = &i915_oa_stream_ops;
/* Lock device for exclusive_stream access late because
* enable_metric_set() might lock as well on gen8+.
*/
ret = i915_mutex_lock_interruptible(&dev_priv->drm);
if (ret)
goto err_lock;
dev_priv->perf.oa.exclusive_stream = stream; dev_priv->perf.oa.exclusive_stream = stream;
mutex_unlock(&dev_priv->drm.struct_mutex); mutex_unlock(&dev_priv->drm.struct_mutex);
return 0; return 0;
err_lock: err_enable:
dev_priv->perf.oa.ops.disable_metric_set(dev_priv); dev_priv->perf.oa.ops.disable_metric_set(dev_priv);
mutex_unlock(&dev_priv->drm.struct_mutex);
err_enable: err_lock:
free_oa_buffer(dev_priv); free_oa_buffer(dev_priv);
err_oa_buf_alloc: err_oa_buf_alloc:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment