Re: [igt-dev] [PATCH i-g-t 10/31] i915/perf: Add OAM format type

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

On Tue, Mar 07, 2023 at 02:45:06PM +0100, Kamil Konieczny wrote:
>Hi Umesh,
>
>On 2023-02-14 at 16:46:27 -0800, Umesh Nerlige Ramappa wrote:
>> Don't check for B and C counters sanity for OAM formats.
>
>Please improve description here, like:
>
>Add new OAM format, where OAM stands for ObservAbility of Media.
>Also while at it, don't check for B and C counters sanity for
>OAM formats.
>
>Note: it is my guess what OAM means so please correct me, btw
>by media do we mean video (image) stream ?

Correct, OA reports for media engines - vcs/vecs.
>It would also help if you write why you don't check that counters
>here or maybe in code comment before if condition.
>
>>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  tests/i915/perf.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/i915/perf.c b/tests/i915/perf.c
>> index c3c5663b..e9613dc9 100644
>> --- a/tests/i915/perf.c
>> +++ b/tests/i915/perf.c
>> @@ -104,6 +104,7 @@ struct accumulator {
>
>It would be nice if you could add here description of the meaning
>of OAG, OAR and OAM.

Will add the comments,

Thanks,
Umesh
>
>With improved comments you can add:
>Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>
>Regards,
>Kamil
>
>>  enum {
>>  	OAG,
>>  	OAR,
>> +	OAM,
>>
>>  	MAX_OA_TYPE,
>>  };
>> @@ -1035,7 +1036,7 @@ gen8_sanity_check_test_oa_reports(const uint32_t *oa_report0,
>>  	/* The TestOa metric set defines all B counters to be a
>>  	 * multiple of the gpu clock
>>  	 */
>> -	if (format.n_b) {
>> +	if (format.n_b && (format.oa_type == OAG || format.oa_type == OAR)) {
>>  		if (clock_delta > 0) {
>>  			b = rpt1_b[0] - rpt0_b[0];
>>  			igt_debug("B0: delta = %"PRIu32"\n", b);
>> --
>> 2.36.1
>>