On Fri, 30 Sep 2022 14:42:07 -0700, Umesh Nerlige Ramappa wrote:
>
> >> +static int __set_oa_ctx_ctrl_offset(struct intel_context *ce)
> >
> > I have seen people complain about unnecessary double underscores in front
> > of function names ;-)
>
> will remove/change to oa_*.
>
> >
> >> +{
> >> + i915_reg_t reg = GEN12_OACTXCONTROL(ce->engine->mmio_base);
> >> + struct i915_perf *perf = &ce->engine->i915->perf;
> >> + u32 saved_offset = perf->ctx_oactxctrl_offset;
> >> + u32 offset;
> >> +
> >> + /* Do this only once. Failure is stored as offset of U32_MAX */
> >> + if (saved_offset)
> >> + return 0;
> >
> > But if saved_offset is U32_MAX we should be returning -ENODEV?
>
> correct, the above if block should be:
>
> if (__valid_oactxctrl_offset(offset))
> return 0;
>
> if (saved_offset == U32_MAX)
> return -ENODEV;
I would just do:
u32 offset = perf->ctx_oactxctrl_offset;
if (offset)
goto exit;
...
exit:
return __valid_oactxctrl_offset(offset) ? 0 : -ENODEV;
}
>
> >
> >> +
> >> + offset = __context_image_offset(ce, i915_mmio_reg_offset(reg));
> >> + perf->ctx_oactxctrl_offset = offset;
> >> +
> >> + drm_dbg(&ce->engine->i915->drm,
> >> + "%s oa ctx control at 0x%08x dword offset\n",
> >> + ce->engine->name, offset);
> >> +
> >> + return __valid_oactxctrl_offset(offset) ? 0 : -ENODEV;
> >> +}
> >> +
> >> +static bool engine_supports_mi_query(struct intel_engine_cs *engine)
> >> +{
> >> + return engine->class == RENDER_CLASS;
> >> +}
> >> +
> >> /**
> >> * oa_get_render_ctx_id - determine and hold ctx hw id
> >> * @stream: An i915-perf stream opened for OA metrics
> >> @@ -1377,6 +1435,17 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
> >> if (IS_ERR(ce))
> >> return PTR_ERR(ce);
> >>
> >> + if (engine_supports_mi_query(stream->engine)) {
> >> + ret = __set_oa_ctx_ctrl_offset(ce);
> >> + if (ret && !(stream->sample_flags & SAMPLE_OA_REPORT)) {
> >
> > This is not a problem in SAMPLE_OA_REPORT case?
>
> SAMPLE_OA_REPORT is OAG use case.
>
> Actually, I did not know how to treat this condition. The current interface
> will configure both OAR and OAG. If we have an error configuring OAR,
> should we fail or let the OAG use case work?
>
> I am now leaning towards failing this unconditionally. Thoughts?
Sorry I didn't follow. What does the oa_ctx_ctrl_offset in the context
image do or control? Looks like oa_ctx_ctrl register controls the OA HW
timer which dumps data into the OA buffer so should be programmed correctly
for OAG (and possibly also for OAR). So maybe letting this fail
unconditionally is correct? But I am not sure.
Thanks.
--
Ashutosh
