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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
