On Fri, Sep 30, 2022 at 04:09:36PM -0700, Dixit, Ashutosh wrote:
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).
It controls TIMER for OAG. I think this is a bug. I will post a separate patch for this since there is some inconsistency in how OAR vs OAG is handled in this code. In short:
- we need to either enable OAG always OR disable all reports into OAG based on SAMPLE_OA/stream->periodic config - oa_ctx_ctrl_offset should accordingly program the timer and periodicity bits here.
So maybe letting this fail unconditionally is correct? But I am not sure.
Agree on returning failure if ret is error. Thanks, Umesh
Thanks. -- Ashutosh
