| Message ID | 20190313144401.17735-13-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/39] drm/i915: Hold a ref to the ring while retiring | expand |
On 13/03/2019 14:43, Chris Wilson wrote: > Allow the user to specify a local engine index (as opposed to > class:index) that they can use to refer to a preset engine inside the > ctx->engine[] array defined by an earlier I915_CONTEXT_PARAM_ENGINES. > This will be useful for setting SSEU parameters on virtual engines that > are local to the context and do not have a valid global class:instance > lookup. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 24 ++++++++++++++++++++---- > include/uapi/drm/i915_drm.h | 3 ++- > 2 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 07377b75b563..7ae28622b709 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -1375,6 +1375,7 @@ static int set_sseu(struct i915_gem_context *ctx, > struct drm_i915_gem_context_param_sseu user_sseu; > struct intel_engine_cs *engine; > struct intel_sseu sseu; > + unsigned long lookup; I'd prefer unsigned int. > int ret; > > if (args->size < sizeof(user_sseu)) > @@ -1387,10 +1388,17 @@ static int set_sseu(struct i915_gem_context *ctx, > sizeof(user_sseu))) > return -EFAULT; > > - if (user_sseu.flags || user_sseu.rsvd) > + if (user_sseu.rsvd) > return -EINVAL; > > - engine = lookup_user_engine(ctx, 0, > + if (user_sseu.flags & ~(I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX)) > + return -EINVAL; > + > + lookup = 0; > + if (user_sseu.flags & I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX) > + lookup |= LOOKUP_USER_INDEX; > + > + engine = lookup_user_engine(ctx, lookup, > user_sseu.engine_class, > user_sseu.engine_instance); > if (!engine) > @@ -1899,6 +1907,7 @@ static int get_sseu(struct i915_gem_context *ctx, > struct drm_i915_gem_context_param_sseu user_sseu; > struct intel_engine_cs *engine; > struct intel_context *ce; > + unsigned long lookup; > > if (args->size == 0) > goto out; > @@ -1909,10 +1918,17 @@ static int get_sseu(struct i915_gem_context *ctx, > sizeof(user_sseu))) > return -EFAULT; > > - if (user_sseu.flags || user_sseu.rsvd) > + if (user_sseu.rsvd) > return -EINVAL; > > - engine = lookup_user_engine(ctx, 0, > + if (user_sseu.flags & ~(I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX)) > + return -EINVAL; > + > + lookup = 0; > + if (user_sseu.flags & I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX) > + lookup |= LOOKUP_USER_INDEX; > + > + engine = lookup_user_engine(ctx, lookup, > user_sseu.engine_class, > user_sseu.engine_instance); > if (!engine) > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 6dde864e14e7..e17c7375248c 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1567,9 +1567,10 @@ struct drm_i915_gem_context_param_sseu { > __u16 engine_instance; > > /* > - * Unused for now. Must be cleared to zero. > + * Unknown flags must be cleared to zero. > */ > __u32 flags; > +#define I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX (1u << 0) > > /* > * Mask of slices to enable for the context. Valid values are a subset > Which patch has this hunk: --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -1185,7 +1185,6 @@ i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx, int ret = 0; GEM_BUG_ON(INTEL_GEN(ctx->i915) < 8); - GEM_BUG_ON(engine->id != RCS0); I think it should go into this one. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-03-14 16:49:54) > > On 13/03/2019 14:43, Chris Wilson wrote: > > Allow the user to specify a local engine index (as opposed to > > class:index) that they can use to refer to a preset engine inside the > > ctx->engine[] array defined by an earlier I915_CONTEXT_PARAM_ENGINES. > > This will be useful for setting SSEU parameters on virtual engines that > > are local to the context and do not have a valid global class:instance > > lookup. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 24 ++++++++++++++++++++---- > > include/uapi/drm/i915_drm.h | 3 ++- > > 2 files changed, 22 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > index 07377b75b563..7ae28622b709 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -1375,6 +1375,7 @@ static int set_sseu(struct i915_gem_context *ctx, > > struct drm_i915_gem_context_param_sseu user_sseu; > > struct intel_engine_cs *engine; > > struct intel_sseu sseu; > > + unsigned long lookup; > > I'd prefer unsigned int. Why not register size for what should only be in a register and matches usage with BIT()? > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index 6dde864e14e7..e17c7375248c 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -1567,9 +1567,10 @@ struct drm_i915_gem_context_param_sseu { > > __u16 engine_instance; > > > > /* > > - * Unused for now. Must be cleared to zero. > > + * Unknown flags must be cleared to zero. > > */ > > __u32 flags; > > +#define I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX (1u << 0) > > > > /* > > * Mask of slices to enable for the context. Valid values are a subset > > > > Which patch has this hunk: > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -1185,7 +1185,6 @@ i915_gem_context_reconfigure_sseu(struct > i915_gem_context *ctx, > int ret = 0; > > GEM_BUG_ON(INTEL_GEN(ctx->i915) < 8); > - GEM_BUG_ON(engine->id != RCS0); > > I think it should go into this one. That's in load balancing (introduction of virtual engine) itself. Consider it done. -Chris
On 14/03/2019 17:04, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-14 16:49:54) >> >> On 13/03/2019 14:43, Chris Wilson wrote: >>> Allow the user to specify a local engine index (as opposed to >>> class:index) that they can use to refer to a preset engine inside the >>> ctx->engine[] array defined by an earlier I915_CONTEXT_PARAM_ENGINES. >>> This will be useful for setting SSEU parameters on virtual engines that >>> are local to the context and do not have a valid global class:instance >>> lookup. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem_context.c | 24 ++++++++++++++++++++---- >>> include/uapi/drm/i915_drm.h | 3 ++- >>> 2 files changed, 22 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c >>> index 07377b75b563..7ae28622b709 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_context.c >>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >>> @@ -1375,6 +1375,7 @@ static int set_sseu(struct i915_gem_context *ctx, >>> struct drm_i915_gem_context_param_sseu user_sseu; >>> struct intel_engine_cs *engine; >>> struct intel_sseu sseu; >>> + unsigned long lookup; >> >> I'd prefer unsigned int. > > Why not register size for what should only be in a register and matches > usage with BIT()? My bad! >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >>> index 6dde864e14e7..e17c7375248c 100644 >>> --- a/include/uapi/drm/i915_drm.h >>> +++ b/include/uapi/drm/i915_drm.h >>> @@ -1567,9 +1567,10 @@ struct drm_i915_gem_context_param_sseu { >>> __u16 engine_instance; >>> >>> /* >>> - * Unused for now. Must be cleared to zero. >>> + * Unknown flags must be cleared to zero. >>> */ >>> __u32 flags; >>> +#define I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX (1u << 0) >>> >>> /* >>> * Mask of slices to enable for the context. Valid values are a subset >>> >> >> Which patch has this hunk: >> >> --- a/drivers/gpu/drm/i915/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >> @@ -1185,7 +1185,6 @@ i915_gem_context_reconfigure_sseu(struct >> i915_gem_context *ctx, >> int ret = 0; >> >> GEM_BUG_ON(INTEL_GEN(ctx->i915) < 8); >> - GEM_BUG_ON(engine->id != RCS0); >> >> I think it should go into this one. > > That's in load balancing (introduction of virtual engine) itself. > Consider it done. And again. You are right, it belongs to virtual engine. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 07377b75b563..7ae28622b709 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -1375,6 +1375,7 @@ static int set_sseu(struct i915_gem_context *ctx, struct drm_i915_gem_context_param_sseu user_sseu; struct intel_engine_cs *engine; struct intel_sseu sseu; + unsigned long lookup; int ret; if (args->size < sizeof(user_sseu)) @@ -1387,10 +1388,17 @@ static int set_sseu(struct i915_gem_context *ctx, sizeof(user_sseu))) return -EFAULT; - if (user_sseu.flags || user_sseu.rsvd) + if (user_sseu.rsvd) return -EINVAL; - engine = lookup_user_engine(ctx, 0, + if (user_sseu.flags & ~(I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX)) + return -EINVAL; + + lookup = 0; + if (user_sseu.flags & I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX) + lookup |= LOOKUP_USER_INDEX; + + engine = lookup_user_engine(ctx, lookup, user_sseu.engine_class, user_sseu.engine_instance); if (!engine) @@ -1899,6 +1907,7 @@ static int get_sseu(struct i915_gem_context *ctx, struct drm_i915_gem_context_param_sseu user_sseu; struct intel_engine_cs *engine; struct intel_context *ce; + unsigned long lookup; if (args->size == 0) goto out; @@ -1909,10 +1918,17 @@ static int get_sseu(struct i915_gem_context *ctx, sizeof(user_sseu))) return -EFAULT; - if (user_sseu.flags || user_sseu.rsvd) + if (user_sseu.rsvd) return -EINVAL; - engine = lookup_user_engine(ctx, 0, + if (user_sseu.flags & ~(I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX)) + return -EINVAL; + + lookup = 0; + if (user_sseu.flags & I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX) + lookup |= LOOKUP_USER_INDEX; + + engine = lookup_user_engine(ctx, lookup, user_sseu.engine_class, user_sseu.engine_instance); if (!engine) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 6dde864e14e7..e17c7375248c 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1567,9 +1567,10 @@ struct drm_i915_gem_context_param_sseu { __u16 engine_instance; /* - * Unused for now. Must be cleared to zero. + * Unknown flags must be cleared to zero. */ __u32 flags; +#define I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX (1u << 0) /* * Mask of slices to enable for the context. Valid values are a subset
Allow the user to specify a local engine index (as opposed to class:index) that they can use to refer to a preset engine inside the ctx->engine[] array defined by an earlier I915_CONTEXT_PARAM_ENGINES. This will be useful for setting SSEU parameters on virtual engines that are local to the context and do not have a valid global class:instance lookup. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem_context.c | 24 ++++++++++++++++++++---- include/uapi/drm/i915_drm.h | 3 ++- 2 files changed, 22 insertions(+), 5 deletions(-)