On Fri, Apr 23, 2021 at 05:31:21PM -0500, Jason Ekstrand wrote:
> As far as I can tell, the only real reason for this is to avoid taking a
> reference to the i915_gem_context. The cost of those two atomics
> probably pales in comparison to the cost of the ioctl itself so we're
> really not buying ourselves anything here. We're about to make context
> lookup a tiny bit more complicated, so let's get rid of the one hand-
> rolled case.
I think the historical reason here is that i965_brw checks this before
every execbuf call, at least for arb_robustness contexts with the right
flag. But we've fixed that hotpath problem by adding non-recoverable
contexts. The kernel will tell you now automatically, for proper userspace
at least (I checked iris and anv, assuming I got it correct), and
reset_stats ioctl isn't a hot path worth micro-optimizing anymore.
With that bit of more context added to the commit message:
Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
>
> Signed-off-by: Jason Ekstrand <ja...@jlekstrand.net>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 13 ++++---------
> drivers/gpu/drm/i915/i915_drv.h | 8 +-------
> 2 files changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index ecb3bf5369857..941fbf78267b4 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -2090,16 +2090,13 @@ int i915_gem_context_reset_stats_ioctl(struct
> drm_device *dev,
> struct drm_i915_private *i915 = to_i915(dev);
> struct drm_i915_reset_stats *args = data;
> struct i915_gem_context *ctx;
> - int ret;
>
> if (args->flags || args->pad)
> return -EINVAL;
>
> - ret = -ENOENT;
> - rcu_read_lock();
> - ctx = __i915_gem_context_lookup_rcu(file->driver_priv, args->ctx_id);
> + ctx = i915_gem_context_lookup(file->driver_priv, args->ctx_id);
> if (!ctx)
> - goto out;
> + return -ENOENT;
>
> /*
> * We opt for unserialised reads here. This may result in tearing
> @@ -2116,10 +2113,8 @@ int i915_gem_context_reset_stats_ioctl(struct
> drm_device *dev,
> args->batch_active = atomic_read(&ctx->guilty_count);
> args->batch_pending = atomic_read(&ctx->active_count);
>
> - ret = 0;
> -out:
> - rcu_read_unlock();
> - return ret;
> + i915_gem_context_put(ctx);
> + return 0;
> }
>
> /* GEM context-engines iterator: for_each_gem_engine() */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0b44333eb7033..8571c5c1509a7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1840,19 +1840,13 @@ struct drm_gem_object *i915_gem_prime_import(struct
> drm_device *dev,
>
> struct dma_buf *i915_gem_prime_export(struct drm_gem_object *gem_obj, int
> flags);
>
> -static inline struct i915_gem_context *
> -__i915_gem_context_lookup_rcu(struct drm_i915_file_private *file_priv, u32
> id)
> -{
> - return xa_load(&file_priv->context_xa, id);
> -}
> -
> static inline struct i915_gem_context *
> i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id)
> {
> struct i915_gem_context *ctx;
>
> rcu_read_lock();
> - ctx = __i915_gem_context_lookup_rcu(file_priv, id);
> + ctx = xa_load(&file_priv->context_xa, id);
> if (ctx && !kref_get_unless_zero(&ctx->ref))
> ctx = NULL;
> rcu_read_unlock();
> --
> 2.31.1
>
> _______________________________________________
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
