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