On Wed, Apr 28, 2021 at 5:27 AM Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxx>
>
> >
> > Signed-off-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
