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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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