Re: [PATCH i-g-t 1/2] tests/intel/xe_oa: Fetch OA buffer size from observation IOCTL

["Pottumuttu, Sai Teja" <sai.teja.pottumuttu@intel.com>]

On 19-11-2024 01:21, Dixit, Ashutosh wrote:
> On Sun, 17 Nov 2024 22:32:22 -0800, Sai Teja Pottumuttu wrote:
>>
> 
> This one mostly looks good, just a couple of nits below.
> 
> Also how are you testing these patches? To test I suggest just run:
> 
> 	sudo ./build/tests/xe_oa

Yes, I ran all the tests that way multiple times and all of them passed.

> 
>> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
>> index 33e978a48..4e4d0fabc 100644
>> --- a/tests/intel/xe_oa.c
>> +++ b/tests/intel/xe_oa.c
>> @@ -63,9 +63,6 @@
>>   #define PIPE_CONTROL_PPGTT_WRITE	(0 << 2)
>>   #define PIPE_CONTROL_GLOBAL_GTT_WRITE   (1 << 2)
>>
>> -#define MAX_OA_BUF_SIZE (16 * 1024 * 1024)
>> -#define OA_BUFFER_SIZE MAX_OA_BUF_SIZE
>> -
>>   #define RING_FORCE_TO_NONPRIV_ADDRESS_MASK 0x03fffffc
>>   /*
>>    * Engine specific registers defined as offsets from engine->mmio_base. For
>> @@ -303,6 +300,7 @@ struct drm_xe_engine_class_instance default_hwe;
>>
>>   static struct intel_xe_perf *intel_xe_perf;
>>   static uint64_t oa_exp_1_millisec;
>> +static uint64_t oa_buffer_size;
> 
> s/size_t/uint64_t/
> 
> Also, for clarity, let's call this default_oa_buffer_size too. Because
> oa_buffer_size later can be changed per stream. So that later when we
> introduce different oa buffer sizes etc there won't be any confusion.
> 
>>   struct intel_mmio_data mmio_data;
>>   static igt_render_copyfunc_t render_copy;
>>
>> @@ -511,6 +509,30 @@ __perf_open(int fd, struct intel_xe_oa_open_prop *param, bool prevent_pm)
>> 	return ret;
>>   }
>>
>> +static u64 get_default_oa_buffer_size(int fd)
> 
> Let's make the return type size_t here too.
> 
> Rest lgtm. I'll take a quick look at the next version and should be able to
> R-b it.

Sure, will make all these changes and send a new revision.

Thanks for the review
- Sai Teja

> 
> Thanks.
> --
> Ashutosh