[Intel-gfx] [PATCH v5 07/10] drm/i915: add a new perf configuration execbuf parameter

[Lionel Landwerlin]

On 27/06/2019 15:53, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-06-27 13:32:13)
>> On 27/06/2019 12:45, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2019-06-27 09:00:42)
>>>> +       /*
>>>> +        * If the config hasn't changed, skip reconfiguring the HW (this is
>>>> +        * subject to a delay we want to avoid has much as possible).
>>>> +        */
>>>> +       if (eb->oa_config == eb->i915->perf.oa.exclusive_stream->oa_config)
>>>> +               return 0;
>>>> +
>>>> +       oa_vma = i915_vma_instance(eb->oa_bo,
>>>> +                                  &eb->engine->i915->ggtt.vm, NULL);
>>>> +       if (unlikely(IS_ERR(oa_vma)))
>>>> +               return PTR_ERR(oa_vma);
>>>> +
>>>> +       err = i915_vma_pin(oa_vma, 0, 0, PIN_GLOBAL);
>>>> +       if (err)
>>>> +               return err;
>>> Ugh. We should not be pinned after creating the request. Might not
>>> matter -- it's just reservation under its own lock that must not be
>>> crossed with the timeline lock currently held here.
>>
>> Should I move this into get_execbuf_oa_config() ?
> That would be save me fretting about the lock nesting.
>
>>>> @@ -2651,9 +2760,23 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>>>>           if (unlikely(err))
>>>>                   goto err_unlock;
>>>>    
>>>> +       if (eb.extensions.flags & BIT(DRM_I915_GEM_EXECBUFFER_EXT_PERF)) {
>>>> +               if (!intel_engine_has_oa(eb.engine)) {
>>>> +                       err = -ENODEV;
>>>> +                       goto err_engine;
>>>> +               }
>>>> +
>>>> +               err = get_execbuf_oa_config(eb.i915,
>>>> +                                           eb.extensions.perf_config.perf_fd,
>>>> +                                           eb.extensions.perf_config.oa_config,
>>>> +                                           &eb.oa_config, &eb.oa_bo);
>>>> +               if (err)
>>>> +                       goto err_engine;
>>> Why is this under the struct_mutex?
>>
>> Access to dev_priv->perf.oa.exclusive_stream must be under struct_mutex.
>>
>> Also because we verify that the engine actually has OA support.
>>
>> I could split the getting the configuration part away.
> I'm about 10^W 50^W certainly less than a 100 patches away from killing
> struct_mutex for execbuf...
> -Chris
>
I'm sorry. Dealing with all this OA stuff is underwhelming.

I think an engine lock would be enough if that's not too bad for you.


-Lionel