[igt-dev] [PATCH i-g-t] i915/gem_workarounds: Adapt to change in file format for per-engine wa

[Tvrtko Ursulin]

On 04/07/2019 10:58, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-07-04 10:51:44)
>>
>> On 03/07/2019 17:59, Chris Wilson wrote:
>>> To reduce the assumptions of RCS0 in the kernel, we want to make the
>>> debugfs engine agnostic and so we need to adapt the igt parser for
>>> flexibility.
>>>
>>> If we could just adequately simulate S3/S4 in the kernel we could forgo
>>> this test entirely...
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> ---
>>>    tests/i915/gem_workarounds.c | 12 ++++++++++--
>>>    1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/i915/gem_workarounds.c b/tests/i915/gem_workarounds.c
>>> index 403863c0b..81c356f06 100644
>>> --- a/tests/i915/gem_workarounds.c
>>> +++ b/tests/i915/gem_workarounds.c
>>> @@ -249,6 +249,7 @@ igt_main
>>>        igt_fixture {
>>>                FILE *file;
>>>                char *line = NULL;
>>> +             char *str;
>>>                size_t line_size;
>>>                int i, fd;
>>>    
>>> @@ -261,9 +262,13 @@ igt_main
>>>    
>>>                fd = igt_debugfs_open(device, "i915_wa_registers", O_RDONLY);
>>>                file = fdopen(fd, "r");
>>> -             igt_assert(getline(&line, &line_size, file) > 0);
>>> +             igt_require(getline(&line, &line_size, file) > 0);
>>>                igt_debug("i915_wa_registers: %s", line);
>>> -             sscanf(line, "Workarounds applied: %d", &num_wa_regs);
>>> +
>>> +             /* We assume that the first batch is for rcs */
>>
>> I think you mean "first line".
> 
> I was thinking of batch as in set of workarounds.

Ok :)
 
>>> +             str = strstr(line, "Workarounds applied:");
>>> +             igt_assert(str);
>>> +             sscanf(str, "Workarounds applied: %d", &num_wa_regs);
>>>                igt_require(num_wa_regs > 0);
>>
>> I have a suspicion all the above section could be simplified using
>> fscanf and just checking return value and errno but that would be asking
>> for too much work for what this is.
> 
> Speaking my language :)
> 
>>>    
>>>                wa_regs = malloc(num_wa_regs * sizeof(*wa_regs));
>>> @@ -271,6 +276,9 @@ igt_main
>>>    
>>>                i = 0;
>>>                while (getline(&line, &line_size, file) > 0) {
>>> +                     if (strstr(line, "Workarounds applied:"))
>>> +                             break;
>>> +
>>>                        igt_debug("%s", line);
>>>                        if (sscanf(line, "0x%X: 0x%08X, mask: 0x%08X",
>>>                                   &wa_regs[i].addr,
>>>
>>
>> I just had a thought that since we are fiddling with this we should add
>> engine names into strings. And/or class:instance pairs.
> 
> Expand upon that thought: where do you want this? I presume you are
> thinking of an igt helper for rcs0 -> class:RENDER, inst:0

I now see you already did it in the i915 patch:

+		seq_printf(m, "%s: Workarounds applied: %u\n",
+			   engine->name, count);

So nothing really. IGT can be extended later if needed.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko