Re: [PATCH 21/29] drm/i915/gem: Use the proto-context to handle create parameters (v2)

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 3, 2021 at 12:23 AM Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Mon, May 31, 2021 at 4:12 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, May 27, 2021 at 11:26:42AM -0500, Jason Ekstrand wrote:
> > > +static int set_proto_ctx_engines(struct drm_i915_file_private *fpriv,
> > > +                              struct i915_gem_proto_context *pc,
> > > +                              const struct drm_i915_gem_context_param *args)
> > > +{
> > > +     struct drm_i915_private *i915 = fpriv->dev_priv;
> > > +     struct set_proto_ctx_engines set = { .i915 = i915 };
> > > +     struct i915_context_param_engines __user *user =
> > > +             u64_to_user_ptr(args->value);
> > > +     unsigned int n;
> > > +     u64 extensions;
> > > +     int err;
> > > +
> > > +     if (!args->size) {
> > > +             proto_context_free_user_engines(pc);
> > > +             memset(&pc->legacy_rcs_sseu, 0, sizeof(pc->legacy_rcs_sseu));
> >
> > Hm I wonder whether we shouldn't put this into the cleanup helper, and
> > then maybe call it proto_context_reset_user_engines()? I think that makes
> > the entire user engines vs sseu flow a notch clearer again.
>
> I fought with myself over this.  The other two callers of
> free_user_engines() would be fine with clearing out the SSEU as well,
> I think, but neither of them need it.  I erred on the side of putting
> it in the one place it's actually needed to make it clear what's going
> on here.  I can move it if you'd like.

So I'm wondering about semantics here a bit, and whether this is all
real, as in, used in real userspace:

Instead of resetting engines here, shouldn't we just complain if
there's more than one engines_set command, ever, on a context?

> As a bit of a P.S., I really hate the SSEU handling.  It's horrible.
> If I had it to do all over again, SSEU would be a purly dynamic
> context param that you aren't allowed to set at create time.  But,
> sadly, we're in the mess we're in. :-(

Yeah it's rather annoying. If we go with "only one engines_set per ctx
create", then maybe we could streamline the SSEU stuff some more too?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch