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

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

[Chris Wilson]

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@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@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 */
+		str = strstr(line, "Workarounds applied:");
+		igt_assert(str);
+		sscanf(str, "Workarounds applied: %d", &num_wa_regs);
 		igt_require(num_wa_regs > 0);
 
 		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,
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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".

> +		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.

>   
>   		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.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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.

> > +             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
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx