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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 07 Mar 2023 12:16:10 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> +	/* 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;
> @@ -4174,7 +4156,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;

I am wondering since this is uapi we should make it robust. So if the user
passes either class or instance he must pass both and we should check for
that. If only one is passed we should not implicitly assume the other as we
are doing here (if only instance is passed here we will assume RCS and if
only class is passed we will assume instance 0). I think making this
explicit will avoid confusion later. Thoughts?

> +		default:
>			MISSING_CASE(id);
>			return -EINVAL;
>		}
> @@ -4182,6 +4170,21 @@ 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;
> +	}

Thanks.
--
Ashutosh