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

[Tvrtko Ursulin]

On 03/07/2019 03:06, 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.
> 
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

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

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

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