On Thu, Jun 3, 2021 at 12:55 AM Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote:
>
> On Mon, May 31, 2021 at 4:50 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
> >
> > On Thu, May 27, 2021 at 11:26:45AM -0500, Jason Ekstrand wrote:
> > > @@ -2692,16 +2792,41 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
> > > {
> > > struct drm_i915_file_private *file_priv = file->driver_priv;
> > > struct drm_i915_gem_context_param *args = data;
> > > + struct i915_gem_proto_context *pc;
> > > struct i915_gem_context *ctx;
> > > - int ret;
> > > + int ret = 0;
> > >
> > > - ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
> > > - if (IS_ERR(ctx))
> > > - return PTR_ERR(ctx);
> > > + ctx = __context_lookup(file_priv, args->ctx_id);
> > > + if (ctx)
> > > + goto set_ctx_param;
> > >
> > > - ret = ctx_setparam(file_priv, ctx, args);
> > > + mutex_lock(&file_priv->proto_context_lock);
> > > + ctx = __context_lookup(file_priv, args->ctx_id);
> >
> > Not unconditionally taking the mutex here feels a bit like overkill? Do we
> > really need that fast path?
>
> Probably not. If someone writes a setparam benchmark, I will actively not care.
So for the non-proto-ctx path you can drop the lock right after lookup
is done, if you haven't done so. Should still be clean, and really no
concerns about microbenchmarks because of that mutex_lock/unlock.
>
> > > + if (ctx)
> > > + goto unlock;
> > > +
> > > + pc = xa_load(&file_priv->proto_context_xa, args->ctx_id);
> > > + if (!pc) {
> > > + ret = -ENOENT;
> > > + goto unlock;
> > > + }
> > > +
> > > + /* FIXME: We should consider disallowing SET_CONTEXT_PARAM for most
> > > + * things on future platforms. Clients should be using
> > > + * CONTEXT_CREATE_EXT_PARAM instead.
> > > + */
> >
> > I think the way to do that is to finalize the context creation from
> > CONTEXT_CREATE_EXT on these platforms. That plugs this hole for good and
> > by design. Maybe on gen13+ or something like that. Or whatever it is we're
> > using for enumerating generations now.
>
> Yup. I agree. Do you want me to add that now?
Separate patch. Also use the new GRAPHICS_VER macro Lucas Demarchi is adding.
> > > + ret = set_proto_ctx_param(file_priv, pc, args);
> > > +
> > > +unlock:
> > > + mutex_unlock(&file_priv->proto_context_lock);
> > > +
> > > +set_ctx_param:
> > > + if (!ret && ctx)
> >
> > I don't think you need to check for ret here? It's not set by any path
> > leading to here where ctx != NULL.
>
> I think
>
> > Also mildly unhappy about the control flow here, we could simplify it if
> > we don't do the lockless faspath.
>
> Yeah, I think I've made it a lot better. You may want to re-review on
> the next go 'round though.
Yeah I need to also recheck the end result (gitlab lost your branch),
I have suddenly some questions about how we handle all the funny
combinations of engines_set vs sseu vs virtual engines in the proto
context again, so will do a final pass there.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
