On 01/07/2019 16:32, Chris Wilson wrote:
Quoting Lionel Landwerlin (2019-07-01 12:34:36)
@@ -1860,23 +1893,55 @@ static int alloc_noa_wait(struct drm_i915_private *i915)
return ret;
}
-static void config_oa_regs(struct drm_i915_private *dev_priv,
- const struct i915_oa_reg *regs,
- u32 n_regs)
+static int emit_oa_config(struct drm_i915_private *i915,
+ struct i915_perf_stream *stream)
{
- u32 i;
+ struct i915_oa_config *oa_config = stream->oa_config;
+ struct i915_request *rq = stream->initial_config_rq;
+ struct i915_vma *vma;
+ u32 *cs;
+ int err;
- for (i = 0; i < n_regs; i++) {
- const struct i915_oa_reg *reg = regs + i;
+ vma = i915_vma_instance(oa_config->obj, &i915->ggtt.vm, NULL);
+ if (unlikely(IS_ERR(vma)))
+ return PTR_ERR(vma);
+
+ err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
+ if (err)
+ return err;
No pinning underneath the timeline->mutex.
...
Hmm... But in this change emit_oa_config() is called by the
enable_metricset() vfunc from i915_oa_stream_init() without holding
drm.struct_mutex.
That doesn't seem to be under the timeline->mutex.
@@ -2455,47 +2466,90 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
if (ret)
goto err_oa_buf_alloc;
+ ret = i915_perf_get_oa_config(dev_priv, props->metrics_set,
+ &stream->oa_config, &obj);
+ if (ret) {
+ DRM_DEBUG("Invalid OA config id=%i\n", props->metrics_set);
+ goto err_config;
+ }
+
+ /*
+ * We just need the buffer to be created, but not our own reference on
+ * it as the oa_config already has one.
+ */
+ i915_gem_object_put(obj);
+
+ stream->initial_config_rq =
+ i915_request_create(dev_priv->engine[RCS0]->kernel_context);
+ if (IS_ERR(stream->initial_config_rq)) {
+ ret = PTR_ERR(stream->initial_config_rq);
+ goto err_initial_config;
+ }
+
+ stream->ops = &i915_oa_stream_ops;
+
ret = i915_mutex_lock_interruptible(&dev_priv->drm);
if (ret)
goto err_lock;
This locking is inverted as timeline->mutex is not a complete guard for
request allocation yet.
So intel_context_lock_pinned() around the request allocation and setting
the active request then?
With the struct_mutex lock taken around it?
- stream->ops = &i915_oa_stream_ops;
+ ret = i915_active_request_set(&dev_priv->engine[RCS0]->last_oa_config,
+ stream->initial_config_rq);
I'm not convinced you want this (and the missing mutex) on the engine,
as it is just describing the perf oa_config timeline. I think it's
better to put that at the same granularity as it is used.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
