On Thu, Jun 3, 2021 at 2:32 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Thu, Jun 3, 2021 at 12:23 AM Jason Ekstrand <jason@xxxxxxxxxxxxxx> wrote:
> >
> > On Mon, May 31, 2021 at 4:12 AM Daniel Vetter <daniel@xxxxxxxx> 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?
I don't think it's ever used. Let's kill it.
> > 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?
It certainly gets rid of this weird corner.
--Jason
