Re: [Intel-gfx] [PATCH v2 04/15] drm/i915/perf: Determine gen12 oa ctx offset at runtime

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

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