[2/2] drm/i915: Implement read-only support in whitelist selftest

Commit Message

John Harrison July 3, 2019, 2:06 a.m. UTC

From: John Harrison <[e-mail]>

Newer hardware supports extra feature in the whitelist registers. This
patch updates the selftest to test that entries marked as read only
are actually read only.

Signed-off-by: John Harrison <[e-mail]>
CC: Tvrtko Ursulin <[e-mail]>
---
 .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++------
 1 file changed, 31 insertions(+), 12 deletions(-)

Comments

Chris Wilson July 3, 2019, 8:32 a.m. UTC | #1

Quoting [e-mail] (2019-07-03 03:06:04)
> From: John Harrison <[e-mail]>
> 
> Newer hardware supports extra feature in the whitelist registers. This
> patch updates the selftest to test that entries marked as read only
> are actually read only.
> 
> Signed-off-by: John Harrison <[e-mail]>
> CC: Tvrtko Ursulin <[e-mail]>
> ---
>  .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++------
>  1 file changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index f8151d5946c8..5cd2b17105ba 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -482,12 +482,12 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>                 u32 srm, lrm, rsvd;
>                 u32 expect;
>                 int idx;
> +               bool ro_reg;
>  
>                 if (wo_register(engine, reg))
>                         continue;
>  
> -               if (ro_register(reg))
> -                       continue;
> +               ro_reg = ro_register(reg);
>  
>                 srm = MI_STORE_REGISTER_MEM;
>                 lrm = MI_LOAD_REGISTER_MEM;
> @@ -588,24 +588,37 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>                 }
>  
>                 GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
> -               rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> -               if (!rsvd) {
> -                       pr_err("%s: Unable to write to whitelisted register %x\n",
> -                              engine->name, reg);
> -                       err = -EINVAL;
> -                       goto out_unpin;
> +               if (ro_reg) {
> +                       rsvd = 0xFFFFFFFF;

rsvd = 0;

reg_write() will then dtrt.

Does this not replace the skip placed in check_whitelisted_registers()?

We still need a way to verify that the register exists, as even writing
from a secure batch fails (not tried ring though). Do we load a spinner,
tweak via mmio?
-Chris

John Harrison July 3, 2019, 7:43 p.m. UTC | #2

On 7/3/2019 01:32, Chris Wilson wrote:
> Quoting [e-mail] (2019-07-03 03:06:04)
>> From: John Harrison <[e-mail]>
>>
>> Newer hardware supports extra feature in the whitelist registers. This
>> patch updates the selftest to test that entries marked as read only
>> are actually read only.
>>
>> Signed-off-by: John Harrison <[e-mail]>
>> CC: Tvrtko Ursulin <[e-mail]>
>> ---
>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++------
>>   1 file changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> index f8151d5946c8..5cd2b17105ba 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> @@ -482,12 +482,12 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>>                  u32 srm, lrm, rsvd;
>>                  u32 expect;
>>                  int idx;
>> +               bool ro_reg;
>>   
>>                  if (wo_register(engine, reg))
>>                          continue;
>>   
>> -               if (ro_register(reg))
>> -                       continue;
>> +               ro_reg = ro_register(reg);
>>   
>>                  srm = MI_STORE_REGISTER_MEM;
>>                  lrm = MI_LOAD_REGISTER_MEM;
>> @@ -588,24 +588,37 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>>                  }
>>   
>>                  GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
>> -               rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
>> -               if (!rsvd) {
>> -                       pr_err("%s: Unable to write to whitelisted register %x\n",
>> -                              engine->name, reg);
>> -                       err = -EINVAL;
>> -                       goto out_unpin;
>> +               if (ro_reg) {
>> +                       rsvd = 0xFFFFFFFF;
> rsvd = 0;
>
> reg_write() will then dtrt.
It seemed too suspiciously broken to have the test claim a read-only 
register was successfully written to. This way makes it clear that the 
test expects read-only to always return the first value read.

> Does this not replace the skip placed in check_whitelisted_registers()?
The two versions of that test looks like they need to be able to set 
values. So they can't be run on read-only registers.

> We still need a way to verify that the register exists, as even writing
> from a secure batch fails (not tried ring though). Do we load a spinner,
> tweak via mmio?

I don't think there is a reliable, generic mechanism to test that you 
can actually read from a read only register. You need to know what 
content it should provide. Even the current test (that it always returns 
the same value) would break if the register changes dynamically (e.g. 
it's a hardware counter).

John.

Tvrtko Ursulin July 4, 2019, 10:10 a.m. UTC | #3

On 03/07/2019 03:06, [e-mail] wrote:
> From: John Harrison <[e-mail]>
> 
> Newer hardware supports extra feature in the whitelist registers. This
> patch updates the selftest to test that entries marked as read only
> are actually read only.
> 
> Signed-off-by: John Harrison <[e-mail]>
> CC: Tvrtko Ursulin <[e-mail]>

I think I gave my r-b for this in the last round.

Reviewed-by: Tvrtko Ursulin <[e-mail]>

Regards,

Tvrtko

P.S. I don't have a strong opinion on whether to have it like it is, or 
to do what Chris suggested and to cheat with rsvd = 0. Both are a bit 
difficult to figure out when reviewing. 0xffffffff solution is also 
misleading in a way that the value is only used in a log message for no 
real effect. So I guess this means slight preference to rsvd = 0 
solution after all.

> ---
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++------
>   1 file changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index f8151d5946c8..5cd2b17105ba 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -482,12 +482,12 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		u32 srm, lrm, rsvd;
>   		u32 expect;
>   		int idx;
> +		bool ro_reg;
>   
>   		if (wo_register(engine, reg))
>   			continue;
>   
> -		if (ro_register(reg))
> -			continue;
> +		ro_reg = ro_register(reg);
>   
>   		srm = MI_STORE_REGISTER_MEM;
>   		lrm = MI_LOAD_REGISTER_MEM;
> @@ -588,24 +588,37 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		}
>   
>   		GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
> -		rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> -		if (!rsvd) {
> -			pr_err("%s: Unable to write to whitelisted register %x\n",
> -			       engine->name, reg);
> -			err = -EINVAL;
> -			goto out_unpin;
> +		if (ro_reg) {
> +			rsvd = 0xFFFFFFFF;
> +		} else {
> +			/* detect write masking */
> +			rsvd = results[ARRAY_SIZE(values)];
> +			if (!rsvd) {
> +				pr_err("%s: Unable to write to whitelisted register %x\n",
> +				       engine->name, reg);
> +				err = -EINVAL;
> +				goto out_unpin;
> +			}
>   		}
>   
>   		expect = results[0];
>   		idx = 1;
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
>   		}
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, ~values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, ~values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
> @@ -622,7 +635,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -630,7 +646,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = ~values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
>

Tvrtko Ursulin July 10, 2019, 8:21 a.m. UTC | #4

On 03/07/2019 20:43, John Harrison wrote:
> On 7/3/2019 01:32, Chris Wilson wrote:
>> Quoting [e-mail] (2019-07-03 03:06:04)
>>> From: John Harrison <[e-mail]>
>>>
>>> Newer hardware supports extra feature in the whitelist registers. This
>>> patch updates the selftest to test that entries marked as read only
>>> are actually read only.
>>>
>>> Signed-off-by: John Harrison <[e-mail]>
>>> CC: Tvrtko Ursulin <[e-mail]>
>>> ---
>>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++------
>>>   1 file changed, 31 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
>>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> index f8151d5946c8..5cd2b17105ba 100644
>>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> @@ -482,12 +482,12 @@ static int check_dirty_whitelist(struct 
>>> i915_gem_context *ctx,
>>>                  u32 srm, lrm, rsvd;
>>>                  u32 expect;
>>>                  int idx;
>>> +               bool ro_reg;
>>>                  if (wo_register(engine, reg))
>>>                          continue;
>>> -               if (ro_register(reg))
>>> -                       continue;
>>> +               ro_reg = ro_register(reg);
>>>                  srm = MI_STORE_REGISTER_MEM;
>>>                  lrm = MI_LOAD_REGISTER_MEM;
>>> @@ -588,24 +588,37 @@ static int check_dirty_whitelist(struct 
>>> i915_gem_context *ctx,
>>>                  }
>>>                  GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 
>>> 0xffffffff);
>>> -               rsvd = results[ARRAY_SIZE(values)]; /* detect write 
>>> masking */
>>> -               if (!rsvd) {
>>> -                       pr_err("%s: Unable to write to whitelisted 
>>> register %x\n",
>>> -                              engine->name, reg);
>>> -                       err = -EINVAL;
>>> -                       goto out_unpin;
>>> +               if (ro_reg) {
>>> +                       rsvd = 0xFFFFFFFF;
>> rsvd = 0;
>>
>> reg_write() will then dtrt.
> It seemed too suspiciously broken to have the test claim a read-only 
> register was successfully written to. This way makes it clear that the 
> test expects read-only to always return the first value read.

I suggest we go with this version if it is not too-disagreeable. Chris?

John can only hope it still applies.

Regards,

Tvrtko

>> Does this not replace the skip placed in check_whitelisted_registers()?
> The two versions of that test looks like they need to be able to set 
> values. So they can't be run on read-only registers.
> 
>> We still need a way to verify that the register exists, as even writing
>> from a secure batch fails (not tried ring though). Do we load a spinner,
>> tweak via mmio?
> 
> I don't think there is a reliable, generic mechanism to test that you 
> can actually read from a read only register. You need to know what 
> content it should provide. Even the current test (that it always returns 
> the same value) would break if the register changes dynamically (e.g. 
> it's a hardware counter).
> 
> John.
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index f8151d5946c8..5cd2b17105ba 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -482,12 +482,12 @@  static int check_dirty_whitelist(struct i915_gem_context *ctx,
 		u32 srm, lrm, rsvd;
 		u32 expect;
 		int idx;
+		bool ro_reg;
 
 		if (wo_register(engine, reg))
 			continue;
 
-		if (ro_register(reg))
-			continue;
+		ro_reg = ro_register(reg);
 
 		srm = MI_STORE_REGISTER_MEM;
 		lrm = MI_LOAD_REGISTER_MEM;
@@ -588,24 +588,37 @@  static int check_dirty_whitelist(struct i915_gem_context *ctx,
 		}
 
 		GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
-		rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
-		if (!rsvd) {
-			pr_err("%s: Unable to write to whitelisted register %x\n",
-			       engine->name, reg);
-			err = -EINVAL;
-			goto out_unpin;
+		if (ro_reg) {
+			rsvd = 0xFFFFFFFF;
+		} else {
+			/* detect write masking */
+			rsvd = results[ARRAY_SIZE(values)];
+			if (!rsvd) {
+				pr_err("%s: Unable to write to whitelisted register %x\n",
+				       engine->name, reg);
+				err = -EINVAL;
+				goto out_unpin;
+			}
 		}
 
 		expect = results[0];
 		idx = 1;
 		for (v = 0; v < ARRAY_SIZE(values); v++) {
-			expect = reg_write(expect, values[v], rsvd);
+			if (ro_reg)
+				expect = results[0];
+			else
+				expect = reg_write(expect, values[v], rsvd);
+
 			if (results[idx] != expect)
 				err++;
 			idx++;
 		}
 		for (v = 0; v < ARRAY_SIZE(values); v++) {
-			expect = reg_write(expect, ~values[v], rsvd);
+			if (ro_reg)
+				expect = results[0];
+			else
+				expect = reg_write(expect, ~values[v], rsvd);
+
 			if (results[idx] != expect)
 				err++;
 			idx++;
@@ -622,7 +635,10 @@  static int check_dirty_whitelist(struct i915_gem_context *ctx,
 			for (v = 0; v < ARRAY_SIZE(values); v++) {
 				u32 w = values[v];
 
-				expect = reg_write(expect, w, rsvd);
+				if (ro_reg)
+					expect = results[0];
+				else
+					expect = reg_write(expect, w, rsvd);
 				pr_info("Wrote %08x, read %08x, expect %08x\n",
 					w, results[idx], expect);
 				idx++;
@@ -630,7 +646,10 @@  static int check_dirty_whitelist(struct i915_gem_context *ctx,
 			for (v = 0; v < ARRAY_SIZE(values); v++) {
 				u32 w = ~values[v];
 
-				expect = reg_write(expect, w, rsvd);
+				if (ro_reg)
+					expect = results[0];
+				else
+					expect = reg_write(expect, w, rsvd);
 				pr_info("Wrote %08x, read %08x, expect %08x\n",
 					w, results[idx], expect);
 				idx++;