Re: [Intel-gfx] [PATCH v2 2/6] drm/i915/xehp: Drop GETPARAM lookups of I915_PARAM_[SUB]SLICE_MASK

[Matt Roper <matthew.d.roper@intel.com>]

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

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation