On Wed, Apr 28, 2021 at 01:22:14PM -0500, Jason Ekstrand wrote:
> 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.
Huh I thought I checked, but I guess got lost.
> > With that bit of more context added to the commit message:
>
> I'd like to agree on what to add before adding something
Yeah in this case maybe just mention that with non-recoverable ctx there's
no need for userspace to check before every execbuf, so if this ever shows
up there's a proper fix which avoids the ioctl entirely. Like iris does.
Or something like that. I just want to make it clear that if this ever
does show up (once we've made execbuf faster with vm_bind and all that)
then the correct fix isn't to make this ioctl faster. But to just not
call it :-)
Cheers, Daniel
>
> --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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
