[Intel-gfx] [PATCH 4/5] drm/i915: add a new perf configuration execbuf parameter

[Chris Wilson]

Quoting Lionel Landwerlin (2019-05-21 15:08:54)
> +       if (eb->oa_config &&
> +           eb->oa_config != eb->i915->perf.oa.exclusive_stream->oa_config) {

But the oa_config is not ordered with respect to requests, and the
registers changed here are not context saved and so may be changed by a
third party before execution. The first party must presumably dropped
the perf_fd before then, so maybe you don't care? Hmm, doesn't even take
a third party as the perf_fd owner may change their mind for different
contexts and be surprised when the wrong set is used.

> +               struct i915_vma *oa_vma;
> +
> +               oa_vma = i915_vma_instance(eb->oa_bo,
> +                                             &eb->engine->i915->ggtt.vm, NULL);
> +               if (unlikely(IS_ERR(oa_vma))) {
> +                       err = PTR_ERR(oa_vma);
> +                       return err;
> +               }
> +
> +               err = i915_vma_pin(oa_vma, 0, 0, PIN_GLOBAL);
> +               if (err)
> +                       return err;
> +
> +               err = eb->engine->emit_bb_start(eb->request,
> +                                               oa_vma->node.start,
> +                                               0, I915_DISPATCH_SECURE);
> +               if (err) {
> +                       i915_vma_unpin(oa_vma);
> +                       return err;
> +               }
> +
> +               err = i915_vma_move_to_active(oa_vma, eb->request, 0);

Move to active first, so that the vma is not in use if the move fails.

> +               if (err) {
> +                       i915_vma_unpin(oa_vma);
> +                       return err;
> +               }
> +
> +
> +               i915_vma_unpin(oa_vma);
> +
> +
> +               swap(eb->oa_config, eb->i915->perf.oa.exclusive_stream->oa_config);
> +       }
> +
>         err = eb->engine->emit_bb_start(eb->request,
>                                         eb->batch->node.start +
>                                         eb->batch_start_offset,
> @@ -2341,6 +2410,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>         eb.buffer_count = args->buffer_count;
>         eb.batch_start_offset = args->batch_start_offset;
>         eb.batch_len = args->batch_len;
> +       eb.oa_config = NULL;
>  
>         eb.batch_flags = 0;
>         if (args->flags & I915_EXEC_SECURE) {
> @@ -2385,17 +2455,29 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>          */
>         intel_gt_pm_get(eb.i915);
>  
> -       err = i915_mutex_lock_interruptible(dev);
> -       if (err)
> -               goto err_rpm;
> -
>         err = eb_select_engine(&eb, file, args);

Lost the lock.

>         if (unlikely(err))
> -               goto err_unlock;
> +               goto err_rpm;
> +
> +       if (args->flags & I915_EXEC_PERF_CONFIG) {
> +               if (!intel_engine_has_oa(eb.engine)) {
> +                       err = -ENODEV;
> +                       goto err_engine;
> +               }
> +
> +               err = get_execbuf_oa_config(eb.i915, args->DR1, args->DR4,
> +                                           &eb.oa_config, &eb.oa_bo);
> +               if (err)
> +                       goto err_engine;
> +       }
> +
> +       err = i915_mutex_lock_interruptible(dev);
> +       if (err)
> +               goto err_oa;
>  
>         err = eb_wait_for_ring(&eb); /* may temporarily drop struct_mutex */
>         if (unlikely(err))
> -               goto err_engine;
> +               goto err_unlock;
>  
>         err = eb_relocate(&eb);
>         if (err) {
> @@ -2541,10 +2623,15 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>  err_vma:
>         if (eb.exec)
>                 eb_release_vmas(&eb);
> -err_engine:
> -       eb_unpin_context(&eb);
>  err_unlock:
>         mutex_unlock(&dev->struct_mutex);
> +err_oa:
> +       if (eb.oa_config) {
> +               i915_gem_object_put(eb.oa_bo);
> +               i915_oa_config_put(eb.oa_config);
> +       }
> +err_engine:
> +       eb_unpin_context(&eb);

Lost the lock.

>  err_rpm:
>         intel_gt_pm_put(eb.i915);
>         i915_gem_context_put(eb.gem_context);