| Message ID | 20180425114521.7524-2-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Lionel Landwerlin (2018-04-25 12:45:14) > This function will be used later by the per (context,engine) power > programming interface. No. This is not the appropriate uABI, please see intel_engine_lookup_user(). -Chris
On 25/04/18 12:50, Chris Wilson wrote: > Quoting Lionel Landwerlin (2018-04-25 12:45:14) >> This function will be used later by the per (context,engine) power >> programming interface. > No. This is not the appropriate uABI, please see > intel_engine_lookup_user(). > -Chris > uAPI wise, does this sound okay? : #define I915_CONTEXT_PARAM_SSEU 0x7 __u64 value; struct drm_i915_gem_context_param_sseu { /* * Engine to be configured or queried. */ __u32 class; __u32 instance; /* * Setting slice_mask or subslice_mask to 0 will make the context use * masks reported respectively by I915_PARAM_SLICE_MASK or * I915_PARAM_SUBSLICE_MASK. */ union { struct { __u8 slice_mask; __u8 subslice_mask; __u8 min_eus_per_subslice; __u8 max_eus_per_subslice; } packed; __u64 value; }; };
Quoting Lionel Landwerlin (2018-04-30 15:37:42) > On 25/04/18 12:50, Chris Wilson wrote: > > Quoting Lionel Landwerlin (2018-04-25 12:45:14) > >> This function will be used later by the per (context,engine) power > >> programming interface. > > No. This is not the appropriate uABI, please see > > intel_engine_lookup_user(). > > -Chris > > > uAPI wise, does this sound okay? : > > #define I915_CONTEXT_PARAM_SSEU 0x7 > __u64 value; > > struct drm_i915_gem_context_param_sseu { > /* > * Engine to be configured or queried. > */ > __u32 class; > __u32 instance; Tvrtko hasn't yet complained about me limiting class/instance to U32_MAX, so I think this limit will do for the forseeable future ;) > /* > * Setting slice_mask or subslice_mask to 0 will make the > context use > * masks reported respectively by I915_PARAM_SLICE_MASK or > * I915_PARAM_SUBSLICE_MASK. > */ > union { > struct { > __u8 slice_mask; > __u8 subslice_mask; > __u8 min_eus_per_subslice; > __u8 max_eus_per_subslice; > } packed; > __u64 value; > }; The union isn't required any more as we aren't packing into the u64 value. I would include perhaps a rsvd field? And then make the interface accept an array of these. -Chris
On 30/04/2018 15:37, Lionel Landwerlin wrote: > On 25/04/18 12:50, Chris Wilson wrote: >> Quoting Lionel Landwerlin (2018-04-25 12:45:14) >>> This function will be used later by the per (context,engine) power >>> programming interface. >> No. This is not the appropriate uABI, please see >> intel_engine_lookup_user(). >> -Chris >> > uAPI wise, does this sound okay? : > > #define I915_CONTEXT_PARAM_SSEU 0x7 > __u64 value; > > struct drm_i915_gem_context_param_sseu { > /* > * Engine to be configured or queried. > */ > __u32 class; > __u32 instance; > > /* > * Setting slice_mask or subslice_mask to 0 will make the > context use > * masks reported respectively by I915_PARAM_SLICE_MASK or > * I915_PARAM_SUBSLICE_MASK. > */ > union { > struct { > __u8 slice_mask; > __u8 subslice_mask; > __u8 min_eus_per_subslice; > __u8 max_eus_per_subslice; > } packed; > __u64 value; > }; > }; Is it possible (now or in the future) to have different configs per slice or subslice? And if so, is the way this uAPI handles that passing these packets multiple times every time with different slices and subslice mask? For instance: class=0, instance=0, slice_mask=0x1, subslice=0x10 class=0, instance=0, slice_mask=0x10, subslice=0x1 Is this something we should support? Regards, Tvrtko
On 03/05/18 18:12, Tvrtko Ursulin wrote: > > On 30/04/2018 15:37, Lionel Landwerlin wrote: >> On 25/04/18 12:50, Chris Wilson wrote: >>> Quoting Lionel Landwerlin (2018-04-25 12:45:14) >>>> This function will be used later by the per (context,engine) power >>>> programming interface. >>> No. This is not the appropriate uABI, please see >>> intel_engine_lookup_user(). >>> -Chris >>> >> uAPI wise, does this sound okay? : >> >> #define I915_CONTEXT_PARAM_SSEU 0x7 >> __u64 value; >> >> struct drm_i915_gem_context_param_sseu { >> /* >> * Engine to be configured or queried. >> */ >> __u32 class; >> __u32 instance; >> >> /* >> * Setting slice_mask or subslice_mask to 0 will make the >> context use >> * masks reported respectively by I915_PARAM_SLICE_MASK or >> * I915_PARAM_SUBSLICE_MASK. >> */ >> union { >> struct { >> __u8 slice_mask; >> __u8 subslice_mask; >> __u8 min_eus_per_subslice; >> __u8 max_eus_per_subslice; >> } packed; >> __u64 value; >> }; >> }; > > Is it possible (now or in the future) to have different configs per > slice or subslice? And if so, is the way this uAPI handles that > passing these packets multiple times every time with different slices > and subslice mask? > > For instance: > > class=0, instance=0, slice_mask=0x1, subslice=0x10 > class=0, instance=0, slice_mask=0x10, subslice=0x1 > > Is this something we should support? It's not supported in any configuration I'm aware of. It's always a uniform subslice programming across slices. It's actually a number of slice/subslice to enable (through the register interface), not a mask (i.e. we can't choose which ones get used) which is what this interface exposes. It's a fair comment though. We didn't plan for asymmetric slice fusing initially and it just happened. One other thing I've been wondering is whether we should strictly validate the input from userspace. Right now we min/max the numbers and never raise any error. Thanks, - Lionel > > Regards, > > Tvrtko > > >
On 03/05/2018 18:31, Lionel Landwerlin wrote: > On 03/05/18 18:12, Tvrtko Ursulin wrote: >> >> On 30/04/2018 15:37, Lionel Landwerlin wrote: >>> On 25/04/18 12:50, Chris Wilson wrote: >>>> Quoting Lionel Landwerlin (2018-04-25 12:45:14) >>>>> This function will be used later by the per (context,engine) power >>>>> programming interface. >>>> No. This is not the appropriate uABI, please see >>>> intel_engine_lookup_user(). >>>> -Chris >>>> >>> uAPI wise, does this sound okay? : >>> >>> #define I915_CONTEXT_PARAM_SSEU 0x7 >>> __u64 value; >>> >>> struct drm_i915_gem_context_param_sseu { >>> /* >>> * Engine to be configured or queried. >>> */ >>> __u32 class; >>> __u32 instance; >>> >>> /* >>> * Setting slice_mask or subslice_mask to 0 will make the >>> context use >>> * masks reported respectively by I915_PARAM_SLICE_MASK or >>> * I915_PARAM_SUBSLICE_MASK. >>> */ >>> union { >>> struct { >>> __u8 slice_mask; >>> __u8 subslice_mask; >>> __u8 min_eus_per_subslice; >>> __u8 max_eus_per_subslice; >>> } packed; >>> __u64 value; >>> }; >>> }; >> >> Is it possible (now or in the future) to have different configs per >> slice or subslice? And if so, is the way this uAPI handles that >> passing these packets multiple times every time with different slices >> and subslice mask? >> >> For instance: >> >> class=0, instance=0, slice_mask=0x1, subslice=0x10 >> class=0, instance=0, slice_mask=0x10, subslice=0x1 >> >> Is this something we should support? > > It's not supported in any configuration I'm aware of. It's always a > uniform subslice programming across slices. > It's actually a number of slice/subslice to enable (through the register > interface), not a mask (i.e. we can't choose which ones get used) which > is what this interface exposes. I thought for Gen11 it cannot be just number of slices but must be a mask? since media will want to make sure some slices are not enabled, and some are, for its workloads. But in general I guess I don't understand the semantics of slice and subslice masks (and eu counts). Say if you pass in slice_mask=0x1 subslice_mask=0x1 - this means you are only enabling subslice 0x1 on slice 0x1 - but what happens on other slices? They are all disabled? > It's a fair comment though. We didn't plan for asymmetric slice fusing > initially and it just happened. Okay so you think uAPI should support it to be more future proof? > One other thing I've been wondering is whether we should strictly > validate the input from userspace. > Right now we min/max the numbers and never raise any error. It does feel low level enough that we should reject userspace trying to feed in nonsense for its own good. Regards, Tvrtko
On 03/05/18 19:00, Tvrtko Ursulin wrote: > > On 03/05/2018 18:31, Lionel Landwerlin wrote: >> On 03/05/18 18:12, Tvrtko Ursulin wrote: >>> >>> On 30/04/2018 15:37, Lionel Landwerlin wrote: >>>> On 25/04/18 12:50, Chris Wilson wrote: >>>>> Quoting Lionel Landwerlin (2018-04-25 12:45:14) >>>>>> This function will be used later by the per (context,engine) power >>>>>> programming interface. >>>>> No. This is not the appropriate uABI, please see >>>>> intel_engine_lookup_user(). >>>>> -Chris >>>>> >>>> uAPI wise, does this sound okay? : >>>> >>>> #define I915_CONTEXT_PARAM_SSEU 0x7 >>>> __u64 value; >>>> >>>> struct drm_i915_gem_context_param_sseu { >>>> /* >>>> * Engine to be configured or queried. >>>> */ >>>> __u32 class; >>>> __u32 instance; >>>> >>>> /* >>>> * Setting slice_mask or subslice_mask to 0 will make the >>>> context use >>>> * masks reported respectively by I915_PARAM_SLICE_MASK or >>>> * I915_PARAM_SUBSLICE_MASK. >>>> */ >>>> union { >>>> struct { >>>> __u8 slice_mask; >>>> __u8 subslice_mask; >>>> __u8 min_eus_per_subslice; >>>> __u8 max_eus_per_subslice; >>>> } packed; >>>> __u64 value; >>>> }; >>>> }; >>> >>> Is it possible (now or in the future) to have different configs per >>> slice or subslice? And if so, is the way this uAPI handles that >>> passing these packets multiple times every time with different >>> slices and subslice mask? >>> >>> For instance: >>> >>> class=0, instance=0, slice_mask=0x1, subslice=0x10 >>> class=0, instance=0, slice_mask=0x10, subslice=0x1 >>> >>> Is this something we should support? >> >> It's not supported in any configuration I'm aware of. It's always a >> uniform subslice programming across slices. > It's actually a number >> of slice/subslice to enable (through the register >> interface), not a mask (i.e. we can't choose which ones get used) >> which is what this interface exposes. > > I thought for Gen11 it cannot be just number of slices but must be a > mask? since media will want to make sure some slices are not enabled, > and some are, for its workloads. Interesting question. I'll try to get an understanding on that. I'm guessing that you program it to half the number of subslices and it just works. > > But in general I guess I don't understand the semantics of slice and > subslice masks (and eu counts). Say if you pass in slice_mask=0x1 > subslice_mask=0x1 - this means you are only enabling subslice 0x1 on > slice 0x1 - but what happens on other slices? They are all disabled? slice_mask=0x1 means you only enable the first slice subslice_mask=0x1 means for each slice, you only enable the first subslice. So essentially on a GT4 (3 slices), slices 1 & 2 would be disable completely (all subslices included) and only subslice0 in slice0 would be powered. > >> It's a fair comment though. We didn't plan for asymmetric slice >> fusing initially and it just happened. > > Okay so you think uAPI should support it to be more future proof? I don't feel like HW engineers want to give us that amount of say in what can be powered. So I wouldn't make it more flexible than it currently is. We explicitly say that subslice_mask is uniform for all slices. And if hardware ever grows more capability we add a new enum. > >> One other thing I've been wondering is whether we should strictly >> validate the input from userspace. >> Right now we min/max the numbers and never raise any error. > > It does feel low level enough that we should reject userspace trying > to feed in nonsense for its own good. Okay, adding that. Thanks! > > Regards, > > Tvrtko >
Quoting Lionel Landwerlin (2018-05-03 21:09:01) > On 03/05/18 19:00, Tvrtko Ursulin wrote: > >> One other thing I've been wondering is whether we should strictly > >> validate the input from userspace. > >> Right now we min/max the numbers and never raise any error. > > > > It does feel low level enough that we should reject userspace trying > > to feed in nonsense for its own good. > > Okay, adding that. As a general rule, validate input to protect the kernel and not the user. It's a fine line between strict validation and preventing the user from doing something you didn't consider/appreciate but is perfectly valid (and turns out to be the preferred way down the line). (Not enough validation hits the headlines; too much and we just curse you as we have to carry two interfaces to do the job of one ;) -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2c72c596057f..fdf71164ee24 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2895,6 +2895,9 @@ void i915_gem_cleanup_early(struct drm_i915_private *dev_priv); void i915_gem_load_init_fences(struct drm_i915_private *dev_priv); int i915_gem_freeze(struct drm_i915_private *dev_priv); int i915_gem_freeze_late(struct drm_i915_private *dev_priv); +struct intel_engine_cs *i915_gem_engine_from_flags(struct drm_i915_private *dev_priv, + struct drm_file *file, + u64 flags); void *i915_gem_object_alloc(struct drm_i915_private *dev_priv); void i915_gem_object_free(struct drm_i915_gem_object *obj); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index c74f5df3fb5a..9c13ec39d87b 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -2032,12 +2032,12 @@ static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = { [I915_EXEC_VEBOX] = VECS }; -static struct intel_engine_cs * -eb_select_engine(struct drm_i915_private *dev_priv, - struct drm_file *file, - struct drm_i915_gem_execbuffer2 *args) +struct intel_engine_cs * +i915_gem_engine_from_flags(struct drm_i915_private *dev_priv, + struct drm_file *file, + u64 flags) { - unsigned int user_ring_id = args->flags & I915_EXEC_RING_MASK; + unsigned int user_ring_id = flags & I915_EXEC_RING_MASK; struct intel_engine_cs *engine; if (user_ring_id > I915_USER_RINGS) { @@ -2046,14 +2046,14 @@ eb_select_engine(struct drm_i915_private *dev_priv, } if ((user_ring_id != I915_EXEC_BSD) && - ((args->flags & I915_EXEC_BSD_MASK) != 0)) { + ((flags & I915_EXEC_BSD_MASK) != 0)) { DRM_DEBUG("execbuf with non bsd ring but with invalid " - "bsd dispatch flags: %d\n", (int)(args->flags)); + "bsd dispatch flags: %d\n", (int)(flags)); return NULL; } if (user_ring_id == I915_EXEC_BSD && HAS_BSD2(dev_priv)) { - unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK; + unsigned int bsd_idx = flags & I915_EXEC_BSD_MASK; if (bsd_idx == I915_EXEC_BSD_DEFAULT) { bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file); @@ -2256,7 +2256,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, if (args->flags & I915_EXEC_IS_PINNED) eb.batch_flags |= I915_DISPATCH_PINNED; - eb.engine = eb_select_engine(eb.i915, file, args); + eb.engine = i915_gem_engine_from_flags(eb.i915, file, args->flags); if (!eb.engine) return -EINVAL;
This function will be used later by the per (context,engine) power programming interface. Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 3 +++ drivers/gpu/drm/i915/i915_gem_execbuffer.c | 18 +++++++++--------- 2 files changed, 12 insertions(+), 9 deletions(-)