Re: [PATCH i-g-t v4 1/1] tests/intel/xe_exec_capture: Add xe_exec_capture test

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

On 2024-11-14 5:23 p.m., Dong, Zhanjun wrote:
> Please see my comments inline below.
> 
> Regards,
> Zhanjun Dong
> 
> On 2024-11-13 2:48 a.m., Teres Alexis, Alan Previn wrote:
>> On Tue, 2024-10-22 at 09:33 -0700, Zhanjun Dong wrote:
>>> Test with GuC reset, check if devcoredump register dump is within the
>> alan: nit: more clarification? -> "Submit cmds to the GPU that result 
>> in a GuC engine reset and check that devcoredump
>> register dump is generated, by the GuC, and includes the full register 
>> range"?
> sure
>>> range.
>>>
>>> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
>>> ---
>>>   tests/intel/xe_exec_capture.c | 365 ++++++++++++++++++++++++++++++++++
>>>   tests/meson.build             |   1 +
>>>   2 files changed, 366 insertions(+)
>>>   create mode 100644 tests/intel/xe_exec_capture.c
>>>
>>> diff --git a/tests/intel/xe_exec_capture.c b/tests/intel/ 
>>> xe_exec_capture.c
>>> new file mode 100644
>>> index 000000000..3a8fa68f2
...

>>> +#define BASE_ADDRESS                   0x1a0000
>>> +/* Batch buffer element count, in number of dwords(uint32_t) */
>>> +#define BATCH_DW_COUNT                 16
>>> +
>>> +#define MAX_PATH_NAME_LEN              512
>>> +#define MAX_LINES                      4096
>>> +/* Keep line content up to this length */
>>> +#define MAX_LINE_LEN                   1024
>>> +#define MAIN_BUF_SIZE                  (MAX_LINES * MAX_LINE_LEN * 
>>> sizeof(char))
>>> +/* Buffer size to read a line */
>>> +#define LINE_BUF_SIZE                  (64 * 1024)
>>> +
>> alan: the above set of definitions are being used for the sole purpose of
>> trying to get all of the devcoredump file into a buffer that you can 
>> then go through
>> an find some specific keywords using strstr in get_coredump_item. 
>> However, instead
>> of using all these definitions and creating alloc_lines_buffer, 
>> load_all and get_coredump_item,
>> why not reuse igt_sysfs_get function? is there a reason we are not 
>> using that?
> Yes, that idea comes from previous version review comments.
> By load all into line buffers, then lines buffer index is the 
> line_number, make debug easier.
>>
>> Alternatively, i am wondering if we can consider the top part of the 
>> devcoredump layout
>> is persistent and so maybe we dont even need to load so much of the 
>> devcoredump?
>> i.e. if we dont get any variable length printouts or buffer object dumps
>> before the guc error capture then we could simply read the minimal 
>> buffer size needed
>> to get the initial keywords of the guc error capture dump.
> That looks like what I do in V3: only data content between
> "**** Job ****"
> and
> "**** VM state ****"
> will be load into buffer and parsed
This rev loads all data into buffer, up to MAX_LINES of 4096, with full 
16MB GuC log buffer+debug settings, the devcoredump could be very large 
and 4096 lines can not hold all. On the other hand, the fixed max lines 
is not scalable at all.
Back to the purpose of this igt test, it focus on engine register 
capture, we don't care all other parts in the dump.
So in the next rev, I will only load contents of 2 sections:
**** Job ****
and
**** HW Engines ****
which is up to tag of
**** VM state ****

If we want to have line number, it could be line_number_of_start_tag + 
index to get real line number, if we need it.

Regards,
Zhanjun Dong

> 
>>
>>
>>> +#define DUMP_PATH                      "/sys/class/drm/card%d/ 
>>> device/devcoredump/data"
>>> +#define START_TAG                      "**** Job ****"
>>> +#define REGEX_KEY_VALUE_PAIR           "^[ \t]*([^:]+):[ \t]*([^ 
>>> \t]+)[ \t]*$"
>>> +#define REGEX_KEY_INDEX                        1
>>> +#define REGEX_VALUE_INDEX              2
>>> +#define REGEX_KEY_VALUE_GROUP_COUNT    3
>>> +
...