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

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 21/05/2019 18:07, Chris Wilson wrote:
> 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.


The OA config batch should be ordered with regard to the MI_BBS we're 
doing just below right?

It's written before in the ring buffer.


That essentially all we need so that as the perf queries run in the 
batch supplied by the application runs with the configuration needed.

If the application shares the perf FD and someone else runs another 
configuration, it's the application fault.

It needs to hold the perf FD for as long as it's doing perf queries and 
not allow anybody else to interact with the OA configuration.


This mechanism is unfortunately what we have resolve to because we don't 
have per context performance counters.

The alternative is post processing the OA buffer (which we do in GL) 
from the CPU which is not really compatible with Vulkan queries.


-Lionel


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


Whoops... I'll split the engine_has_oa() check away.

Thanks,


-Lionel


>
>>          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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx