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

[Tvrtko Ursulin]

On 12/07/2019 08:07, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
> 
> 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 <John.C.Harrison at Intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   .../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++;
>