On Thu, Jun 3, 2021 at 2:32 AM Daniel Vetter daniel@ffwll.ch wrote:
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?
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