Re: [Intel-gfx] [PATCH 18/24] drm/i915: Convert i915_perf to ww locking as well

["Thomas Hellström (Intel)" <thomas_os@shipmail.org>]

On 8/10/20 12:30 PM, Maarten Lankhorst wrote:
> We have the ordering of timeline->mutex vs resv_lock wrong,
> convert the i915_pin_vma and intel_context_pin as well to
> future-proof this.
>
> We may need to do future changes to do this more transaction-like,
> and only get down to a single i915_gem_ww_ctx, but for now this
> should work.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c | 57 +++++++++++++++++++++++---------
>   1 file changed, 42 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index c6f6370283cf..e94976976571 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1195,24 +1195,39 @@ static struct intel_context *oa_pin_context(struct i915_perf_stream *stream)
>   	struct i915_gem_engines_iter it;
>   	struct i915_gem_context *ctx = stream->ctx;
>   	struct intel_context *ce;
> -	int err;
> +	struct i915_gem_ww_ctx ww;
> +	int err = -ENODEV;
>   
>   	for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
>   		if (ce->engine != stream->engine) /* first match! */
>   			continue;
>   
> -		/*
> -		 * As the ID is the gtt offset of the context's vma we
> -		 * pin the vma to ensure the ID remains fixed.
> -		 */
> -		err = intel_context_pin(ce);
> -		if (err == 0) {
> -			stream->pinned_ctx = ce;
> -			break;
> -		}
> +		err = 0;
> +		break;
>   	}
>   	i915_gem_context_unlock_engines(ctx);
>   
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	i915_gem_ww_ctx_init(&ww, true);
> +retry:
> +	/*
> +	 * As the ID is the gtt offset of the context's vma we
> +	 * pin the vma to ensure the ID remains fixed.
> +	 */
> +	err = intel_context_pin_ww(ce, &ww);
> +	if (err == -EDEADLK) {
> +		err = i915_gem_ww_ctx_backoff(&ww);
> +		if (!err)
> +			goto retry;
> +	}
> +	i915_gem_ww_ctx_fini(&ww);
> +

Hmm. Didn't we keep an intel_context_pin() that does exactly the above 
without recoding the whole ww transaction? Or do you plan to remove that?

With that taken into account,

Reviewed-by: Thomas Hellström <thomas.hellstrom@intel.com>


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx