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.
...
> @@ -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.
> - 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
