Re: [Intel-gfx] [PATCH 14/21] drm/i915/gem: Return an error ptr from context_lookup

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 23, 2021 at 05:31:24PM -0500, Jason Ekstrand wrote:
> We're about to start doing lazy context creation which means contexts
> get created in i915_gem_context_lookup and we may start having more
> errors than -ENOENT.
> 
> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c    | 12 ++++++------
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c |  4 ++--
>  drivers/gpu/drm/i915/i915_drv.h                |  2 +-
>  drivers/gpu/drm/i915/i915_perf.c               |  4 ++--
>  4 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 3e883daab93bf..7929d5a8be449 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -2105,8 +2105,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>  	int ret = 0;
>  
>  	ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
> -	if (!ctx)
> -		return -ENOENT;
> +	if (IS_ERR(ctx))
> +		return PTR_ERR(ctx);
>  
>  	switch (args->param) {
>  	case I915_CONTEXT_PARAM_GTT_SIZE:
> @@ -2174,8 +2174,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>  	int ret;
>  
>  	ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
> -	if (!ctx)
> -		return -ENOENT;
> +	if (IS_ERR(ctx))
> +		return PTR_ERR(ctx);
>  
>  	ret = ctx_setparam(file_priv, ctx, args);
>  
> @@ -2194,8 +2194,8 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
>  		return -EINVAL;
>  
>  	ctx = i915_gem_context_lookup(file->driver_priv, args->ctx_id);
> -	if (!ctx)
> -		return -ENOENT;
> +	if (IS_ERR(ctx))
> +		return PTR_ERR(ctx);
>  
>  	/*
>  	 * We opt for unserialised reads here. This may result in tearing
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 7024adcd5cf15..de14b26f3b2d5 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -739,8 +739,8 @@ static int eb_select_context(struct i915_execbuffer *eb)
>  	struct i915_gem_context *ctx;
>  
>  	ctx = i915_gem_context_lookup(eb->file->driver_priv, eb->args->rsvd1);
> -	if (unlikely(!ctx))
> -		return -ENOENT;
> +	if (unlikely(IS_ERR(ctx)))
> +		return PTR_ERR(ctx);
>  
>  	eb->gem_context = ctx;
>  	if (rcu_access_pointer(ctx->vm))
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8571c5c1509a7..004ed0e59c999 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h

I just realized that I think __i915_gem_context_lookup_rcu doesn't have
users anymore. Please make sure it's deleted.

> @@ -1851,7 +1851,7 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id)
>  		ctx = NULL;
>  	rcu_read_unlock();
>  
> -	return ctx;
> +	return ctx ? ctx : ERR_PTR(-ENOENT);
>  }
>  
>  /* i915_gem_evict.c */
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 85ad62dbabfab..b86ed03f6a705 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -3414,10 +3414,10 @@ i915_perf_open_ioctl_locked(struct i915_perf *perf,
>  		struct drm_i915_file_private *file_priv = file->driver_priv;
>  
>  		specific_ctx = i915_gem_context_lookup(file_priv, ctx_handle);
> -		if (!specific_ctx) {
> +		if (IS_ERR(specific_ctx)) {
>  			DRM_DEBUG("Failed to look up context with ID %u for opening perf stream\n",
>  				  ctx_handle);
> -			ret = -ENOENT;
> +			ret = PTR_ERR(specific_ctx);

Yeah this looks like a nice place to integrate this.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

One thing we need to make sure in the next patch or thereabouts is that
lookup can only return ENOENT or ENOMEM, but never EINVAL. I'll drop some
bikesheds on that :-)
-Daniel

>  			goto err;
>  		}
>  	}
> -- 
> 2.31.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel