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
