| Message ID | 20220517032005.2694737-3-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915: SSEU handling updates | expand |
On 17/05/2022 04:20, Matt Roper wrote: > Slice/subslice/EU information should be obtained via the topology > queries provided by the I915_QUERY interface; let's turn off support for > the old GETPARAM lookups on Xe_HP and beyond where we can't return > meaningful values. > > The slice mask lookup is meaningless since Xe_HP doesn't support > traditional slices (and we make no attempt to return the various new > units like gslices, cslices, mslices, etc.) here. > > The subslice mask lookup is even more problematic; given the distinct > masks for geometry vs compute purposes, the combined mask returned here > is likely not what userspace would want to act upon anyway. The value > is also limited to 32-bits by the nature of the GETPARAM ioctl which is > sufficient for the initial Xe_HP platforms, but is unable to convey the > larger masks that will be needed on other upcoming platforms. Finally, > the value returned here becomes even less meaningful when used on > multi-tile platforms where each tile will have its own masks. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/i915_getparam.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index c12a0adefda5..ac9767c56619 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -148,11 +148,19 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > value = intel_engines_has_context_isolation(i915); > break; > case I915_PARAM_SLICE_MASK: > + /* Not supported from Xe_HP onward; use topology queries */ > + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) > + return -EINVAL; > + > value = sseu->slice_mask; > if (!value) > return -ENODEV; > break; > case I915_PARAM_SUBSLICE_MASK: > + /* Not supported from Xe_HP onward; use topology queries */ > + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) > + return -EINVAL; > + > /* Only copy bits from the first slice */ > memcpy(&value, sseu->subslice_mask, > min(sseu->ss_stride, (u8)sizeof(value))); Just in case lets run this by Jordan and Lionel since it affects DG2. Anyone else on the userspace side who might be affected? Regards, Tvrtko
On Fri, May 20, 2022 at 10:15:32AM +0100, Tvrtko Ursulin wrote: > > On 17/05/2022 04:20, Matt Roper wrote: > > Slice/subslice/EU information should be obtained via the topology > > queries provided by the I915_QUERY interface; let's turn off support for > > the old GETPARAM lookups on Xe_HP and beyond where we can't return > > meaningful values. > > > > The slice mask lookup is meaningless since Xe_HP doesn't support > > traditional slices (and we make no attempt to return the various new > > units like gslices, cslices, mslices, etc.) here. > > > > The subslice mask lookup is even more problematic; given the distinct > > masks for geometry vs compute purposes, the combined mask returned here > > is likely not what userspace would want to act upon anyway. The value > > is also limited to 32-bits by the nature of the GETPARAM ioctl which is > > sufficient for the initial Xe_HP platforms, but is unable to convey the > > larger masks that will be needed on other upcoming platforms. Finally, > > the value returned here becomes even less meaningful when used on > > multi-tile platforms where each tile will have its own masks. > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > --- > > drivers/gpu/drm/i915/i915_getparam.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > > index c12a0adefda5..ac9767c56619 100644 > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > @@ -148,11 +148,19 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > value = intel_engines_has_context_isolation(i915); > > break; > > case I915_PARAM_SLICE_MASK: > > + /* Not supported from Xe_HP onward; use topology queries */ > > + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) > > + return -EINVAL; > > + > > value = sseu->slice_mask; > > if (!value) > > return -ENODEV; > > break; > > case I915_PARAM_SUBSLICE_MASK: > > + /* Not supported from Xe_HP onward; use topology queries */ > > + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) > > + return -EINVAL; > > + > > /* Only copy bits from the first slice */ > > memcpy(&value, sseu->subslice_mask, > > min(sseu->ss_stride, (u8)sizeof(value))); > > Just in case lets run this by Jordan and Lionel since it affects DG2. Anyone > else on the userspace side who might be affected? When I grep'd Mesa, I found two uses of I915_PARAM_SLICE_MASK and I915_PARAM_SUBSLICE_MASK: * oa_metrics_kernel_support: The topology query is used on gen10+ so the getparam code is only called on gen9 and below * getparam_topology: Invoked via intel_get_device_info_from_fd(). The topology query is attempted first. Only if that fails _and_ we're on a pre-gen10 platform does it fall back to GETPARAM. I also checked https://github.com/intel/compute-runtime and only see these being issued in one place: * HwInfoConfig::configureHwInfoDrm: Only used if drm->queryTopology() returns a failure first. I think those are the only relevant userspace for SSEU topology, so as far as I can tell nobody is still relying on the legacy getparams by the time we get to Xe_HP hardware. Matt > > Regards, > > Tvrtko
On 20/05/2022 21:42, Matt Roper wrote: > On Fri, May 20, 2022 at 10:15:32AM +0100, Tvrtko Ursulin wrote: >> >> On 17/05/2022 04:20, Matt Roper wrote: >>> Slice/subslice/EU information should be obtained via the topology >>> queries provided by the I915_QUERY interface; let's turn off support for >>> the old GETPARAM lookups on Xe_HP and beyond where we can't return >>> meaningful values. >>> >>> The slice mask lookup is meaningless since Xe_HP doesn't support >>> traditional slices (and we make no attempt to return the various new >>> units like gslices, cslices, mslices, etc.) here. >>> >>> The subslice mask lookup is even more problematic; given the distinct >>> masks for geometry vs compute purposes, the combined mask returned here >>> is likely not what userspace would want to act upon anyway. The value >>> is also limited to 32-bits by the nature of the GETPARAM ioctl which is >>> sufficient for the initial Xe_HP platforms, but is unable to convey the >>> larger masks that will be needed on other upcoming platforms. Finally, >>> the value returned here becomes even less meaningful when used on >>> multi-tile platforms where each tile will have its own masks. >>> >>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_getparam.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c >>> index c12a0adefda5..ac9767c56619 100644 >>> --- a/drivers/gpu/drm/i915/i915_getparam.c >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c >>> @@ -148,11 +148,19 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, >>> value = intel_engines_has_context_isolation(i915); >>> break; >>> case I915_PARAM_SLICE_MASK: >>> + /* Not supported from Xe_HP onward; use topology queries */ >>> + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) >>> + return -EINVAL; >>> + >>> value = sseu->slice_mask; >>> if (!value) >>> return -ENODEV; >>> break; >>> case I915_PARAM_SUBSLICE_MASK: >>> + /* Not supported from Xe_HP onward; use topology queries */ >>> + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) >>> + return -EINVAL; >>> + >>> /* Only copy bits from the first slice */ >>> memcpy(&value, sseu->subslice_mask, >>> min(sseu->ss_stride, (u8)sizeof(value))); >> >> Just in case lets run this by Jordan and Lionel since it affects DG2. Anyone >> else on the userspace side who might be affected? > > When I grep'd Mesa, I found two uses of I915_PARAM_SLICE_MASK and > I915_PARAM_SUBSLICE_MASK: > > * oa_metrics_kernel_support: The topology query is used on gen10+ so > the getparam code is only called on gen9 and below > > * getparam_topology: Invoked via intel_get_device_info_from_fd(). The > topology query is attempted first. Only if that fails _and_ we're on > a pre-gen10 platform does it fall back to GETPARAM. > > I also checked https://github.com/intel/compute-runtime and only see > these being issued in one place: > > * HwInfoConfig::configureHwInfoDrm: Only used if drm->queryTopology() > returns a failure first. > > > I think those are the only relevant userspace for SSEU topology, so as > far as I can tell nobody is still relying on the legacy getparams by the > time we get to Xe_HP hardware. Sounds good to me - I also had a look at the media and compute drivers and couldn't find any use. However I still think it is best if UMD teams would ack this patch. Regards, Tvrtko
On 17/05/2022 06:20, Matt Roper wrote: > Slice/subslice/EU information should be obtained via the topology > queries provided by the I915_QUERY interface; let's turn off support for > the old GETPARAM lookups on Xe_HP and beyond where we can't return > meaningful values. > > The slice mask lookup is meaningless since Xe_HP doesn't support > traditional slices (and we make no attempt to return the various new > units like gslices, cslices, mslices, etc.) here. > > The subslice mask lookup is even more problematic; given the distinct > masks for geometry vs compute purposes, the combined mask returned here > is likely not what userspace would want to act upon anyway. The value > is also limited to 32-bits by the nature of the GETPARAM ioctl which is > sufficient for the initial Xe_HP platforms, but is unable to convey the > larger masks that will be needed on other upcoming platforms. Finally, > the value returned here becomes even less meaningful when used on > multi-tile platforms where each tile will have its own masks. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Sounds fair. We've been relying on the topology query in Mesa since it's available and it's a requirement for Gfx10+. FYI, we're also not using I915_PARAM_EU_TOTAL on Gfx10+ for the same reason. Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_getparam.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index c12a0adefda5..ac9767c56619 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -148,11 +148,19 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > value = intel_engines_has_context_isolation(i915); > break; > case I915_PARAM_SLICE_MASK: > + /* Not supported from Xe_HP onward; use topology queries */ > + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) > + return -EINVAL; > + > value = sseu->slice_mask; > if (!value) > return -ENODEV; > break; > case I915_PARAM_SUBSLICE_MASK: > + /* Not supported from Xe_HP onward; use topology queries */ > + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) > + return -EINVAL; > + > /* Only copy bits from the first slice */ > memcpy(&value, sseu->subslice_mask, > min(sseu->ss_stride, (u8)sizeof(value)));
diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c index c12a0adefda5..ac9767c56619 100644 --- a/drivers/gpu/drm/i915/i915_getparam.c +++ b/drivers/gpu/drm/i915/i915_getparam.c @@ -148,11 +148,19 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, value = intel_engines_has_context_isolation(i915); break; case I915_PARAM_SLICE_MASK: + /* Not supported from Xe_HP onward; use topology queries */ + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) + return -EINVAL; + value = sseu->slice_mask; if (!value) return -ENODEV; break; case I915_PARAM_SUBSLICE_MASK: + /* Not supported from Xe_HP onward; use topology queries */ + if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) + return -EINVAL; + /* Only copy bits from the first slice */ memcpy(&value, sseu->subslice_mask, min(sseu->ss_stride, (u8)sizeof(value)));
Slice/subslice/EU information should be obtained via the topology queries provided by the I915_QUERY interface; let's turn off support for the old GETPARAM lookups on Xe_HP and beyond where we can't return meaningful values. The slice mask lookup is meaningless since Xe_HP doesn't support traditional slices (and we make no attempt to return the various new units like gslices, cslices, mslices, etc.) here. The subslice mask lookup is even more problematic; given the distinct masks for geometry vs compute purposes, the combined mask returned here is likely not what userspace would want to act upon anyway. The value is also limited to 32-bits by the nature of the GETPARAM ioctl which is sufficient for the initial Xe_HP platforms, but is unable to convey the larger masks that will be needed on other upcoming platforms. Finally, the value returned here becomes even less meaningful when used on multi-tile platforms where each tile will have its own masks. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/i915_getparam.c | 8 ++++++++ 1 file changed, 8 insertions(+)