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

[Chris Wilson]

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