[Intel-gfx] [PATCH 11/21] drm/i915: Stop manually RCU banging in reset_stats_ioctl

[Jason Ekstrand]

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