[Intel-gfx] [PATCH v2 8/9] drm/i915/perf: Add engine class instance parameters to perf

Fri Feb 24 19:37:01 UTC 2023

On Tue, Feb 21, 2023 at 03:53:57PM -0800, Dixit, Ashutosh wrote:
>On Thu, 16 Feb 2023 16:58:49 -0800, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>Patch is mostly ok but a few questions below:
>
>> Current implementation of perf defaults to render and configures the
>> default OAG unit. Since there are more OA units on newer hardware, allow
>> user to pass engine class and instance to program specific OA units.
>
>I think we should more clearly say here that the OA unit is identified by
>means of one of the engines (class/instance of that engine) associated with
>that OA unit. The engine -> OA unit mapping is a static mapping depending
>on the particular platform. Something like that.
>
>>
>> UMD specific changes for GPUvis support:
>> https://patchwork.freedesktop.org/patch/522827/?series=114023
>> https://patchwork.freedesktop.org/patch/522822/?series=114023
>> https://patchwork.freedesktop.org/patch/522826/?series=114023
>> https://patchwork.freedesktop.org/patch/522828/?series=114023
>> https://patchwork.freedesktop.org/patch/522816/?series=114023
>> https://patchwork.freedesktop.org/patch/522825/?series=114023
>>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_perf.c | 49 +++++++++++++++++++-------------
>>  include/uapi/drm/i915_drm.h      | 20 +++++++++++++
>>  2 files changed, 49 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index d3a1892c93be..f028df812067 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -4035,40 +4035,29 @@ static int read_properties_unlocked(struct i915_perf *perf,
>>	struct drm_i915_gem_context_param_sseu user_sseu;
>>	u64 __user *uprop = uprops;
>>	bool config_sseu = false;
>> +	u8 class, instance;
>>	u32 i;
>>	int ret;
>>
>>	memset(props, 0, sizeof(struct perf_open_properties));
>>	props->poll_oa_period = DEFAULT_POLL_PERIOD_NS;
>>
>> -	if (!n_props) {
>> -		drm_dbg(&perf->i915->drm,
>> -			"No i915 perf properties given\n");
>> -		return -EINVAL;
>> -	}
>> -
>> -	/* At the moment we only support using i915-perf on the RCS. */
>> -	props->engine = intel_engine_lookup_user(perf->i915,
>> -						 I915_ENGINE_CLASS_RENDER,
>> -						 0);
>> -	if (!props->engine) {
>> -		drm_dbg(&perf->i915->drm,
>> -			"No RENDER-capable engines\n");
>> -		return -EINVAL;
>> -	}
>> -
>>	/* Considering that ID = 0 is reserved and assuming that we don't
>>	 * (currently) expect any configurations to ever specify duplicate
>>	 * values for a particular property ID then the last _PROP_MAX value is
>>	 * one greater than the maximum number of properties we expect to get
>>	 * from userspace.
>>	 */
>> -	if (n_props >= DRM_I915_PERF_PROP_MAX) {
>> +	if (!n_props || n_props >= DRM_I915_PERF_PROP_MAX) {
>>		drm_dbg(&perf->i915->drm,
>> -			"More i915 perf properties specified than exist\n");
>> +			"Invalid no. of i915 perf properties given\n");
>
>Invalid number
>
>>		return -EINVAL;
>>	}
>>
>> +	/* Defaults when class:instance is not passed */
>> +	class = I915_ENGINE_CLASS_RENDER;
>> +	instance = 0;
>> +
>>	for (i = 0; i < n_props; i++) {
>>		u64 oa_period, oa_freq_hz;
>>		u64 id, value;
>> @@ -4189,7 +4178,13 @@ static int read_properties_unlocked(struct i915_perf *perf,
>>			}
>>			props->poll_oa_period = value;
>>			break;
>> -		case DRM_I915_PERF_PROP_MAX:
>> +		case DRM_I915_PERF_PROP_OA_ENGINE_CLASS:
>> +			class = (u8)value;
>> +			break;
>> +		case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE:
>> +			instance = (u8)value;
>> +			break;
>> +		default:
>>			MISSING_CASE(id);
>>			return -EINVAL;
>>		}
>> @@ -4197,6 +4192,17 @@ static int read_properties_unlocked(struct i915_perf *perf,
>>		uprop += 2;
>>	}
>>
>> +	props->engine = intel_engine_lookup_user(perf->i915, class, instance);
>> +	if (!props->engine) {
>> +		drm_dbg(&perf->i915->drm,
>> +			"OA engine class and instance invalid %d:%d\n",
>> +			class, instance);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!engine_supports_oa(props->engine))
>> +		return -EINVAL;
>
>Need drm_dbg here too?
>
>> +
>>	if (config_sseu) {
>>		ret = get_sseu_config(&props->sseu, props->engine, &user_sseu);
>>		if (ret) {
>> @@ -5208,8 +5214,11 @@ int i915_perf_ioctl_version(void)
>>	 *
>>	 * 5: Add DRM_I915_PERF_PROP_POLL_OA_PERIOD parameter that controls the
>>	 *    interval for the hrtimer used to check for OA data.
>> +	 *
>> +	 * 6: Add DRM_I915_PERF_PROP_OA_ENGINE_CLASS and
>> +	 *    DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE
>>	 */
>> -	return 5;
>> +	return 6;
>
>Do we need a separate revision for this? Maybe club it with OAM support
>since that is where this is getting introduced?

It's a separate revision to identify support for multiple GTs, even 
without OAM.
>
>>  }
>>
>>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index 8df261c5ab9b..b6922b52d85c 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -2758,6 +2758,26 @@ enum drm_i915_perf_property_id {
>>	 */
>>	DRM_I915_PERF_PROP_POLL_OA_PERIOD,
>>
>> +	/**
>> +	 * In platforms with multiple OA buffers, the engine class instance must
>> +	 * be passed to open a stream to a OA unit corresponding to the engine.
>> +	 * Multiple engines may be mapped to the same OA unit.
>> +	 *
>> +	 * In addition to the class:instance, if a gem context is also passed, then
>> +	 * 1) the report headers of OA reports from other engines are squashed.
>
>Other engines or you mean other contexts?
>
>> +	 * 2) OAR is enabled for the class:instance
>
>For render engine?
>
>Maybe the above comments need to be more crisp since they are in i915_drm.h
>or is it only I who is confused :)
>
>> +	 *
>> +	 * This property is available in perf revision 6.
>> +	 */
>> +	DRM_I915_PERF_PROP_OA_ENGINE_CLASS,
>> +
>> +	/**
>> +	 * This parameter specifies the engine instance.
>> +	 *
>> +	 * This property is available in perf revision 6.
>> +	 */
>> +	DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE,
>> +
>>	DRM_I915_PERF_PROP_MAX /* non-ABI */
>>  };
>>
>> --
>> 2.36.1
>>
>
>Thanks.
>--
>Ashutosh