[Intel-gfx] [PATCH v6 10/11] drm/i915/perf: execute OA configuration from command stream

[Lionel Landwerlin]

On 01/07/2019 16:32, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-07-01 12:34:36)
>> @@ -1860,23 +1893,55 @@ static int alloc_noa_wait(struct drm_i915_private *i915)
>>          return ret;
>>   }
>>   
>> -static void config_oa_regs(struct drm_i915_private *dev_priv,
>> -                          const struct i915_oa_reg *regs,
>> -                          u32 n_regs)
>> +static int emit_oa_config(struct drm_i915_private *i915,
>> +                         struct i915_perf_stream *stream)
>>   {
>> -       u32 i;
>> +       struct i915_oa_config *oa_config = stream->oa_config;
>> +       struct i915_request *rq = stream->initial_config_rq;
>> +       struct i915_vma *vma;
>> +       u32 *cs;
>> +       int err;
>>   
>> -       for (i = 0; i < n_regs; i++) {
>> -               const struct i915_oa_reg *reg = regs + i;
>> +       vma = i915_vma_instance(oa_config->obj, &i915->ggtt.vm, NULL);
>> +       if (unlikely(IS_ERR(vma)))
>> +               return PTR_ERR(vma);
>> +
>> +       err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
>> +       if (err)
>> +               return err;
> No pinning underneath the timeline->mutex.
>
> ...


Hmm... But in this change emit_oa_config() is called by the 
enable_metricset() vfunc from i915_oa_stream_init() without holding 
drm.struct_mutex.

That doesn't seem to be under the timeline->mutex.


>
>> @@ -2455,47 +2466,90 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>>          if (ret)
>>                  goto err_oa_buf_alloc;
>>   
>> +       ret = i915_perf_get_oa_config(dev_priv, props->metrics_set,
>> +                                     &stream->oa_config, &obj);
>> +       if (ret) {
>> +               DRM_DEBUG("Invalid OA config id=%i\n", props->metrics_set);
>> +               goto err_config;
>> +       }
>> +
>> +       /*
>> +        * We just need the buffer to be created, but not our own reference on
>> +        * it as the oa_config already has one.
>> +        */
>> +       i915_gem_object_put(obj);
>> +
>> +       stream->initial_config_rq =
>> +               i915_request_create(dev_priv->engine[RCS0]->kernel_context);
>> +       if (IS_ERR(stream->initial_config_rq)) {
>> +               ret = PTR_ERR(stream->initial_config_rq);
>> +               goto err_initial_config;
>> +       }
>> +
>> +       stream->ops = &i915_oa_stream_ops;
>> +
>>          ret = i915_mutex_lock_interruptible(&dev_priv->drm);
>>          if (ret)
>>                  goto err_lock;
> This locking is inverted as timeline->mutex is not a complete guard for
> request allocation yet.


So intel_context_lock_pinned() around the request allocation and setting 
the active request then?

With the struct_mutex lock taken around it?


>
>> -       stream->ops = &i915_oa_stream_ops;
>> +       ret = i915_active_request_set(&dev_priv->engine[RCS0]->last_oa_config,
>> +                                     stream->initial_config_rq);
> I'm not convinced you want this (and the missing mutex) on the engine,
> as it is just describing the perf oa_config timeline. I think it's
> better to put that at the same granularity as it is used.
> -Chris
>