On Wed, Apr 28, 2021 at 5:27 AM Daniel Vetter daniel@ffwll.ch wrote:
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.
I'm not sure I agree with that bit. I don't think it was ever worth micro-optimizing like this. What does it gain us? Two fewer atomics? It's not like the bad old days when it took a lock.
ANV still calls reset_stats before every set of execbuf (sometimes more than one) but I've never once seen it show up on a perf trace. execbuf, on the other hand, that does show up and pretty heavy sometimes.
With that bit of more context added to the commit message:
I'd like to agree on what to add before adding something
--Jason
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Signed-off-by: Jason Ekstrand jason@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-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch