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

[Chris Wilson]

Quoting Lionel Landwerlin (2019-08-30 15:47:23)
>  err_unpin:
> -       __i915_vma_unpin(vma);
> +       mutex_lock(&i915->drm.struct_mutex);
> +       i915_vma_unpin_and_release(&vma, 0);
> +       mutex_unlock(&i915->drm.struct_mutex);

Strangely unpin_and_release() doesn't need the mutex. But I can clean
that up later.

>  
>  err_unref:
>         i915_gem_object_put(bo);
> @@ -1947,50 +1961,69 @@ static int alloc_noa_wait(struct i915_perf_stream *stream)
>         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_request *rq;
> +       struct i915_vma *vma;
> +       u32 *cs;
> +       int err;
>  
> -       for (i = 0; i < n_regs; i++) {
> -               const struct i915_oa_reg *reg = regs + i;
> +       lockdep_assert_held(&stream->config_mutex);
>  
> -               I915_WRITE(reg->addr, reg->value);
> +       rq = i915_request_create(stream->engine->kernel_context);
> +       if (IS_ERR(rq))
> +               return PTR_ERR(rq);
> +
> +       err = i915_active_request_set(&stream->active_config_rq,
> +                                     rq);
> +       if (err)
> +               goto err_add_request;
> +
> +       vma = i915_vma_instance(stream->initial_oa_config_bo,
> +                               &i915->ggtt.vm, NULL);

Safer with stream->engine->gt->ggtt.vm

> +       if (unlikely(IS_ERR(vma))) {
> +               err = PTR_ERR(vma);
> +               goto err_add_request;
>         }
> -}
>  
> -static void delay_after_mux(void)
> -{
> -       /*
> -        * It apparently takes a fairly long time for a new MUX
> -        * configuration to be be applied after these register writes.
> -        * This delay duration was derived empirically based on the
> -        * render_basic config but hopefully it covers the maximum
> -        * configuration latency.
> -        *
> -        * As a fallback, the checks in _append_oa_reports() to skip
> -        * invalid OA reports do also seem to work to discard reports
> -        * generated before this config has completed - albeit not
> -        * silently.
> -        *
> -        * Unfortunately this is essentially a magic number, since we
> -        * don't currently know of a reliable mechanism for predicting
> -        * how long the MUX config will take to apply and besides
> -        * seeing invalid reports we don't know of a reliable way to
> -        * explicitly check that the MUX config has landed.
> -        *
> -        * It's even possible we've miss characterized the underlying
> -        * problem - it just seems like the simplest explanation why
> -        * a delay at this location would mitigate any invalid reports.
> -        */
> -       usleep_range(15000, 20000);
> +       err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
> +       if (err)
> +               goto err_add_request;

Don't pin inside a request. do the pining before i915_request_create().
One day, we may need to allocate a request to do the pin.

Be safe,

i915_vma_lock(vma);
err = i915_request_await_object(rq, vma->obj, 0);
(yes, we need a better wrapper here)
if (err)
> +       err = i915_vma_move_to_active(vma, rq, 0);
i915_vma_unlock(vma);
> +       if (err)
> +               goto err_vma_unpin;
> +


> @@ -2658,16 +2684,31 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>         stream->ops = &i915_oa_stream_ops;
>         dev_priv->perf.exclusive_stream = stream;
>  
> +       mutex_lock(&stream->config_mutex);
>         ret = dev_priv->perf.ops.enable_metric_set(stream);
>         if (ret) {
>                 DRM_DEBUG("Unable to enable metric set\n");
> -               goto err_enable;
> +               /*
> +                * Ignore the return value since we already have an error from
> +                * the enable vfunc.
> +                */
> +               i915_active_request_retire(&stream->active_config_rq,
> +                                          &stream->config_mutex);
> +       } else {
> +               ret = i915_active_request_retire(&stream->active_config_rq,
> +                                                &stream->config_mutex);

This function doesn't exist anymore. It's basically just waiting for the
old request. Why do you need it? (Your request flow is otherwise ordered.)

> -       DRM_DEBUG("opening stream oa config uuid=%s\n", stream->oa_config->uuid);
> -
> +       mutex_unlock(&stream->config_mutex);
>         mutex_unlock(&dev_priv->drm.struct_mutex);
>  
> +       i915_gem_object_put(stream->initial_oa_config_bo);
> +       stream->initial_oa_config_bo = NULL;
> +       if (ret)
> +               goto err_enable;

Is it because of this err that may end up freeing the stream? I would
expect a similar wait-before-free on stream destroy. In which case I
would have the active request hold a reference on the stream. (And you
might find it more convenient to use i915_active rather than the raw
i915_active_request. i915_active is geared to tracking multiple
timelines, so definitely overkill for you use case, just has more
utility/mid-layer! built in)
-Chris