[Intel-gfx] [PATCH 07/17] drm/i915/perf: Schedule oa_config after modifying the contexts

[Lionel Landwerlin]

On 06/03/2020 15:38, Chris Wilson wrote:
> We wish that the scheduler emit the context modification commands prior
> to enabling the oa_config, for which we must explicitly inform it of the
> ordering constraints. This is especially important as we now wait for
> the final oa_config setup to be completed and as this wait may be on a
> distinct context to the state modifications, we need that command packet
> to be always last in the queue.
>
> We borrow the i915_active for its ability to track multiple timelines
> and the last dma_fence on each; a flexible dma_resv. Keeping track of
> each dma_fence is important for us so that we can efficiently schedule
> the requests and reprioritise as required.
>
> Reported-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_overlay.c  |   8 +-
>   drivers/gpu/drm/i915/gt/intel_context_param.c |   2 +-
>   drivers/gpu/drm/i915/i915_active.c            |   6 +-
>   drivers/gpu/drm/i915/i915_active.h            |   2 +-
>   drivers/gpu/drm/i915/i915_perf.c              | 154 +++++++++++-------
>   drivers/gpu/drm/i915/i915_perf_types.h        |   5 +-
>   drivers/gpu/drm/i915/i915_vma.h               |   2 +-
>   drivers/gpu/drm/i915/selftests/i915_active.c  |   4 +-
>   8 files changed, 115 insertions(+), 68 deletions(-)
>
...
> @@ -2729,16 +2772,19 @@ static const struct i915_perf_stream_ops i915_oa_stream_ops = {
>   
>   static int i915_perf_stream_enable_sync(struct i915_perf_stream *stream)
>   {
> -	struct i915_request *rq;
> +	struct i915_active *active;
> +	int err;
>   
> -	rq = stream->perf->ops.enable_metric_set(stream);
> -	if (IS_ERR(rq))
> -		return PTR_ERR(rq);
> +	active = i915_active_create();
> +	if (!active)
> +		return -ENOMEM;
>   
> -	i915_request_wait(rq, 0, MAX_SCHEDULE_TIMEOUT);
> -	i915_request_put(rq);
> +	err = stream->perf->ops.enable_metric_set(stream, active);
> +	if (err == 0)
> +		i915_active_wait(active, TASK_UNINTERRUPTIBLE);


Why not? :


err = i915_active_wait(active, TASK_INTERRUPTIBLE);


-Lionel


>   
> -	return 0;
> +	i915_active_put(active);
> +	return err;
>   }
>   
>   /**
> @@ -3192,7 +3238,7 @@ static long i915_perf_config_locked(struct i915_perf_stream *stream,
>   		return -EINVAL;
>   
>   	if (config != stream->oa_config) {
> -		struct i915_request *rq;
> +		int err;
>   
>   		/*
>   		 * If OA is bound to a specific context, emit the
> @@ -3203,13 +3249,11 @@ static long i915_perf_config_locked(struct i915_perf_stream *stream,
>   		 * When set globally, we use a low priority kernel context,
>   		 * so it will effectively take effect when idle.
>   		 */
> -		rq = emit_oa_config(stream, config, oa_context(stream));
> -		if (!IS_ERR(rq)) {
> +		err = emit_oa_config(stream, config, oa_context(stream), NULL);
> +		if (!err)
>   			config = xchg(&stream->oa_config, config);
> -			i915_request_put(rq);
> -		} else {
> -			ret = PTR_ERR(rq);
> -		}
> +		else
> +			ret = err;
>   	}
>   
>   	i915_oa_config_put(config);
> diff --git a/drivers/gpu/drm/i915/i915_perf_types.h b/drivers/gpu/drm/i915/i915_perf_types.h
> index a0e22f00f6cf..5eaf874a0d25 100644
> --- a/drivers/gpu/drm/i915/i915_perf_types.h
> +++ b/drivers/gpu/drm/i915/i915_perf_types.h
> @@ -21,6 +21,7 @@
>   
>   struct drm_i915_private;
>   struct file;
> +struct i915_active;
>   struct i915_gem_context;
>   struct i915_perf;
>   struct i915_vma;
> @@ -339,8 +340,8 @@ struct i915_oa_ops {
>   	 * counter reports being sampled. May apply system constraints such as
>   	 * disabling EU clock gating as required.
>   	 */
> -	struct i915_request *
> -		(*enable_metric_set)(struct i915_perf_stream *stream);
> +	int (*enable_metric_set)(struct i915_perf_stream *stream,
> +				 struct i915_active *active);
>   
>   	/**
>   	 * @disable_metric_set: Remove system constraints associated with using
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index e1ced1df13e1..3baa98fa5009 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -380,7 +380,7 @@ int i915_vma_wait_for_bind(struct i915_vma *vma);
>   static inline int i915_vma_sync(struct i915_vma *vma)
>   {
>   	/* Wait for the asynchronous bindings and pending GPU reads */
> -	return i915_active_wait(&vma->active);
> +	return i915_active_wait(&vma->active, TASK_INTERRUPTIBLE);
>   }
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
> index 68bbb1580162..7357d2130024 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_active.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_active.c
> @@ -153,7 +153,7 @@ static int live_active_wait(void *arg)
>   	if (IS_ERR(active))
>   		return PTR_ERR(active);
>   
> -	i915_active_wait(&active->base);
> +	i915_active_wait(&active->base, TASK_UNINTERRUPTIBLE);
>   	if (!READ_ONCE(active->retired)) {
>   		struct drm_printer p = drm_err_printer(__func__);
>   
> @@ -230,7 +230,7 @@ static int live_active_barrier(void *arg)
>   	i915_active_release(&active->base);
>   
>   	if (err == 0)
> -		err = i915_active_wait(&active->base);
> +		err = i915_active_wait(&active->base, TASK_UNINTERRUPTIBLE);
>   
>   	if (err == 0 && !READ_ONCE(active->retired)) {
>   		pr_err("i915_active not retired after flushing barriers!\n");