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

Commit Message

John Harrison July 12, 2019, 7:07 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.

v2: Removed all use of 'rsvd' for read-only registers to avoid
ambiguous code or error messages.

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

Comments

Tvrtko Ursulin July 12, 2019, 8:48 a.m. UTC | #1

On 12/07/2019 08:07, [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.
> 
> v2: Removed all use of 'rsvd' for read-only registers to avoid
> ambiguous code or error messages.

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

Regards,

Tvrtko

> Signed-off-by: John Harrison <[e-mail]>
> CC: Tvrtko Ursulin <[e-mail]>
> ---
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 49 +++++++++++++------
>   1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index 466dcc8214c3..fd1d47ba4b10 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -485,12 +485,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;
> @@ -591,24 +591,35 @@ 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) {
> +			/* 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++;
> @@ -617,15 +628,22 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			pr_err("%s: %d mismatch between values written to whitelisted register [%x], and values read back!\n",
>   			       engine->name, err, reg);
>   
> -			pr_info("%s: Whitelisted register: %x, original value %08x, rsvd %08x\n",
> -				engine->name, reg, results[0], rsvd);
> +			if (ro_reg)
> +				pr_info("%s: Whitelisted read-only register: %x, original value %08x\n",
> +					engine->name, reg, results[0]);
> +			else
> +				pr_info("%s: Whitelisted register: %x, original value %08x, rsvd %08x\n",
> +					engine->name, reg, results[0], rsvd);
>   
>   			expect = results[0];
>   			idx = 1;
>   			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++;
> @@ -633,7 +651,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 12, 2019, 8:53 a.m. UTC | #2

On 12/07/2019 08:07, [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.
> 
> v2: Removed all use of 'rsvd' for read-only registers to avoid
> ambiguous code or error messages.

Work's never done. :) You can follow up with a patch which adds engine looping to live_reset_whitelist.

I was looking at your test results and wondering why no new whitelists:

<6> [486.665700] i915: Running intel_workarounds_live_selftests/live_reset_whitelist
<6> [486.665706] Checking 4 whitelisted registers on rcs0 (RING_NONPRIV) [engine]
<7> [486.666281] [drm:intel_power_well_enable [i915]] enabling always-on
<5> [486.668777] i915 0000:00:02.0: Resetting rcs0 for live_workarounds
<6> [486.669900] Checking 4 whitelisted registers on rcs0 (RING_NONPRIV) [device]
<5> [486.671042] i915 0000:00:02.0: Resetting chip for live_workarounds

Regards,

Tvrtko
 
> Signed-off-by: John Harrison <[e-mail]>
> CC: Tvrtko Ursulin <[e-mail]>
> ---
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 49 +++++++++++++------
>   1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index 466dcc8214c3..fd1d47ba4b10 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -485,12 +485,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;
> @@ -591,24 +591,35 @@ 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) {
> +			/* 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++;
> @@ -617,15 +628,22 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			pr_err("%s: %d mismatch between values written to whitelisted register [%x], and values read back!\n",
>   			       engine->name, err, reg);
>   
> -			pr_info("%s: Whitelisted register: %x, original value %08x, rsvd %08x\n",
> -				engine->name, reg, results[0], rsvd);
> +			if (ro_reg)
> +				pr_info("%s: Whitelisted read-only register: %x, original value %08x\n",
> +					engine->name, reg, results[0]);
> +			else
> +				pr_info("%s: Whitelisted register: %x, original value %08x, rsvd %08x\n",
> +					engine->name, reg, results[0], rsvd);
>   
>   			expect = results[0];
>   			idx = 1;
>   			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++;
> @@ -633,7 +651,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++;
>

John Harrison July 12, 2019, 4:23 p.m. UTC | #3

On 7/12/2019 01:53, Tvrtko Ursulin wrote:
> On 12/07/2019 08:07, [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.
>>
>> v2: Removed all use of 'rsvd' for read-only registers to avoid
>> ambiguous code or error messages.
> Work's never done. :) You can follow up with a patch which adds engine looping to live_reset_whitelist.

There is _always_ something more to be done!

I noticed that Lionel added usage of the RANGE field too. There needs to 
be looping support added for that. At the moment, the self-test simply 
masks off the range bits and ignores them. It really should be updated 
to test the entire range.

Lionel, any chance you would have time to pick up these extra changes?

Thanks,
John.

>
> I was looking at your test results and wondering why no new whitelists:
>
> <6> [486.665700] i915: Running intel_workarounds_live_selftests/live_reset_whitelist
> <6> [486.665706] Checking 4 whitelisted registers on rcs0 (RING_NONPRIV) [engine]
> <7> [486.666281] [drm:intel_power_well_enable [i915]] enabling always-on
> <5> [486.668777] i915 0000:00:02.0: Resetting rcs0 for live_workarounds
> <6> [486.669900] Checking 4 whitelisted registers on rcs0 (RING_NONPRIV) [device]
> <5> [486.671042] i915 0000:00:02.0: Resetting chip for live_workarounds
>
> Regards,
>
> Tvrtko
>

Lionel Landwerlin July 15, 2019, 12:49 p.m. UTC | #4

On 12/07/2019 19:23, John Harrison wrote:
> On 7/12/2019 01:53, Tvrtko Ursulin wrote:
>> On 12/07/2019 08:07, [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.
>>>
>>> v2: Removed all use of 'rsvd' for read-only registers to avoid
>>> ambiguous code or error messages.
>> Work's never done. :) You can follow up with a patch which adds 
>> engine looping to live_reset_whitelist.
>
> There is _always_ something more to be done!
>
> I noticed that Lionel added usage of the RANGE field too. There needs 
> to be looping support added for that. At the moment, the self-test 
> simply masks off the range bits and ignores them. It really should be 
> updated to test the entire range.
>
> Lionel, any chance you would have time to pick up these extra changes?


Unlikely in the next week or 2 :(


-Lionel


>
> Thanks,
> John.
>
>>
>> I was looking at your test results and wondering why no new whitelists:
>>
>> <6> [486.665700] i915: Running 
>> intel_workarounds_live_selftests/live_reset_whitelist
>> <6> [486.665706] Checking 4 whitelisted registers on rcs0 
>> (RING_NONPRIV) [engine]
>> <7> [486.666281] [drm:intel_power_well_enable [i915]] enabling always-on
>> <5> [486.668777] i915 0000:00:02.0: Resetting rcs0 for live_workarounds
>> <6> [486.669900] Checking 4 whitelisted registers on rcs0 
>> (RING_NONPRIV) [device]
>> <5> [486.671042] i915 0000:00:02.0: Resetting chip for live_workarounds
>>
>> Regards,
>>
>> Tvrtko
>>
>
>

Patch

diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index 466dcc8214c3..fd1d47ba4b10 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -485,12 +485,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;
@@ -591,24 +591,35 @@  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) {
+			/* 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++;
@@ -617,15 +628,22 @@  static int check_dirty_whitelist(struct i915_gem_context *ctx,
 			pr_err("%s: %d mismatch between values written to whitelisted register [%x], and values read back!\n",
 			       engine->name, err, reg);
 
-			pr_info("%s: Whitelisted register: %x, original value %08x, rsvd %08x\n",
-				engine->name, reg, results[0], rsvd);
+			if (ro_reg)
+				pr_info("%s: Whitelisted read-only register: %x, original value %08x\n",
+					engine->name, reg, results[0]);
+			else
+				pr_info("%s: Whitelisted register: %x, original value %08x, rsvd %08x\n",
+					engine->name, reg, results[0], rsvd);
 
 			expect = results[0];
 			idx = 1;
 			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++;
@@ -633,7 +651,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++;