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

[Chris Wilson]

Quoting Lionel Landwerlin (2019-06-27 09:00:44)
> We can't run into issues with doing writing the global OA/NOA
> registers configuration from CPU so far, but HW engineers actually
> recommend doing this from the command streamer.
> 
> Since we have a command buffer prepared for the execbuffer side of
> things, we can reuse that approach here too.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_perf.c | 203 +++++++++++++++----------------
>  1 file changed, 100 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index bf4f5fee6764..7e636463e1f5 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -389,6 +389,19 @@ void i915_oa_config_release(struct kref *ref)
>         kfree(oa_config);
>  }
>  
> +static void i915_oa_config_dispose_buffers(struct drm_i915_private *i915)
> +{
> +       struct i915_oa_config *oa_config, *next;
> +
> +       mutex_lock(&i915->perf.metrics_lock);
> +       list_for_each_entry_safe(oa_config, next, &i915->perf.metrics_buffers, vma_link) {
> +               list_del(&oa_config->vma_link);
> +               i915_gem_object_put(oa_config->obj);
> +               oa_config->obj = NULL;
> +       }
> +       mutex_unlock(&i915->perf.metrics_lock);
> +}
> +
>  static u32 *write_cs_mi_lri(u32 *cs, const struct i915_oa_reg *reg_data, u32 n_regs)
>  {
>         u32 i;
> @@ -1813,67 +1826,86 @@ 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 config_oa_regs(struct drm_i915_private *i915,
> +                         struct i915_oa_config *oa_config)
>  {
> -       u32 i;
> +       struct i915_request *rq;
> +       struct i915_vma *vma;
> +       long timeout;
> +       u32 *cs;
> +       int err;
>  
> -       for (i = 0; i < n_regs; i++) {
> -               const struct i915_oa_reg *reg = regs + i;
> +       rq = i915_request_create(i915->engine[RCS0]->kernel_context);
> +       if (IS_ERR(rq))
> +               return PTR_ERR(rq);
>  
> -               I915_WRITE(reg->addr, reg->value);
> +       err = i915_active_request_set(&i915->engine[RCS0]->last_oa_config,
> +                                     rq);

This will need an explicit mutex as it is not serialised by any single
timeline.

i915->perf.metrics_lock viable? Needs to be taken here and in execbuf.

> +       if (err) {
> +               i915_request_add(rq);
> +               return err;
> +       }
> +
> +       vma = i915_vma_instance(oa_config->obj, &i915->ggtt.vm, NULL);
> +       if (unlikely(IS_ERR(vma))) {
> +               i915_request_add(rq);
> +               return PTR_ERR(vma);
> +       }
> +
> +       err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
> +       if (err) {
> +               i915_request_add(rq);
> +               return err;
>         }

This time there is no excuse for not pinning outside of the
timeline->mutex.

> +
> +       err = i915_vma_move_to_active(vma, rq, 0);
> +       if (err) {
> +               i915_vma_unpin(vma);
> +               i915_request_add(rq);
> +               return err;
> +       }

> +
> +       cs = intel_ring_begin(rq, INTEL_GEN(i915) >= 8 ? 4 : 2);
> +       if (IS_ERR(cs)) {
> +               i915_vma_unpin(vma);
> +               i915_request_add(rq);
> +               return PTR_ERR(cs);
> +       }
> +
> +       if (INTEL_GEN(i915) > 8) {
> +               *cs++ = MI_BATCH_BUFFER_START_GEN8;
> +               *cs++ = lower_32_bits(vma->node.start);
> +               *cs++ = upper_32_bits(vma->node.start);
> +               *cs++ = MI_NOOP;
> +       } else {
> +               *cs++ = MI_BATCH_BUFFER_START;
> +               *cs++ = vma->node.start;
> +       }
> +
> +       intel_ring_advance(rq, cs);
> +
> +       i915_vma_unpin(vma);
> +
> +       i915_request_add(rq);
> +
> +       i915_request_get(rq);

Strictly, before the add, as the add is the xfer of the ref.

> +       timeout = i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE | I915_WAIT_LOCKED,
> +                                   MAX_SCHEDULE_TIMEOUT);
> +       i915_request_put(rq);
> +
> +       return timeout < 0 ? err : 0;

Report err, but timeline carries the errno?
-Chris