Re: [Intel-gfx] [PATCH v3 9/9] drm/i915/perf: Add support for OA media units

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

On Mon, 27 Feb 2023 18:23:29 -0800, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> @@ -1632,11 +1647,28 @@ free_noa_wait(struct i915_perf_stream *stream)
>	i915_vma_unpin_and_release(&stream->noa_wait, 0);
>  }
>
> +/*
> + * intel_engine_lookup_user ensures that most of engine specific checks are
> + * taken care of, however, we can run into a case where the OA unit catering to
> + * the engine passed by the user is disabled for some reason. In such cases,
> + * ensure oa unit corresponding to an engine is functional. If there are no
> + * engines in the group, the unit is disabled.
> + */
> +static bool oa_unit_functional(const struct intel_engine_cs *engine)
> +{
> +	return engine->oa_group && engine->oa_group->num_engines;

This doesn't add anything above engine_supports_oa() below: if
engine->oa_group is true for engine X, engine->oa_group->num_engines must
at least be 1 because the oa_group must at least contain X. (Of course
oa_group->num_engines can still be 0 but engine->oa_group->num_engines must
be > 0).

We can see this in oa_init_gt where num_engines++ and oa_group assignment
is done together:

static int oa_init_gt(struct intel_gt *gt)
{
	...

	for_each_engine_masked(engine, gt, ALL_ENGINES, tmp) {
		u32 index = __oa_engine_group(engine);

		engine->oa_group = NULL;
		if (index < num_groups) {
			g[index].num_engines++;
			engine->oa_group = &g[index];
		}
	}
}

Therefore in read_properties_unlocked these checks are sufficient:

	props->engine = intel_engine_lookup_user(perf->i915, class, instance);
	if (!props->engine) {
		return -EINVAL;
	}

	if (!engine_supports_oa(props->engine)) {
		return -EINVAL;
	}

The oa_unit_functional check is not needed.


> +}
> +
>  static bool engine_supports_oa(const struct intel_engine_cs *engine)
>  {
>	return engine->oa_group;
>  }
>
> +static bool engine_supports_oa_format(struct intel_engine_cs *engine, int type)
> +{
> +	return engine->oa_group && engine->oa_group->type == type;
> +}
> +

/snip/

> @@ -4186,6 +4227,17 @@ static int read_properties_unlocked(struct i915_perf *perf,
>		return -EINVAL;
>	}
>
> +	if (!oa_unit_functional(props->engine))
> +		return -ENODEV;
> +
> +	i = array_index_nospec(props->oa_format, I915_OA_FORMAT_MAX);
> +	f = &perf->oa_formats[i];
> +	if (!engine_supports_oa_format(props->engine, f->type)) {
> +		DRM_DEBUG("Invalid OA format %d for class %d\n",
> +			  f->type, props->engine->class);
> +		return -EINVAL;
> +	}
> +

Thanks.
--
Ashutosh