[Intel-gfx] [PATCH v6 10/11] drm/i915/perf: execute OA configuration from command stream

[Chris Wilson]

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