diff mbox series

[4/4] drm/i915: Update workarounds selftest for read only regs

Message ID 20190618010108.27499-5-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series Update whitelist support for new hardware | expand

Commit Message

John Harrison June 18, 2019, 1:01 a.m. UTC 
From: "Robert M. Fosha" <robert.m.fosha@intel.com>

Updates the live_workarounds selftest to handle whitelisted
registers that are flagged as read only.

Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++++++--
 1 file changed, 39 insertions(+), 4 deletions(-)

Comments

Tvrtko Ursulin June 18, 2019, 6:42 a.m. UTC | #1 
On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: "Robert M. Fosha" <robert.m.fosha@intel.com>
> 
> Updates the live_workarounds selftest to handle whitelisted
> registers that are flagged as read only.
> 
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++++++--
>   1 file changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index c8d335d63f9c..eb6d3aa2c8cc 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -408,6 +408,29 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>   	return false;
>   }
>   
> +static bool ro_register(u32 reg)
> +{
> +	if (reg & RING_FORCE_TO_NONPRIV_RD)
> +		return true;
> +
> +	return false;
> +}
> +
> +static int whitelist_writable_count(struct intel_engine_cs *engine)
> +{
> +	int count = engine->whitelist.count;
> +	int i;
> +
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +
> +		if (ro_register(reg))
> +			count--;
> +	}
> +
> +	return count;
> +}
> +
>   static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   				 struct intel_engine_cs *engine)
>   {
> @@ -463,6 +486,9 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		if (wo_register(engine, reg))
>   			continue;
>   
> +		if (ro_register(reg))
> +			continue;
> +
>   		srm = MI_STORE_REGISTER_MEM;
>   		lrm = MI_LOAD_REGISTER_MEM;
>   		if (INTEL_GEN(ctx->i915) >= 8)
> @@ -734,9 +760,13 @@ static int read_whitelisted_registers(struct i915_gem_context *ctx,
>   
>   	for (i = 0; i < engine->whitelist.count; i++) {
>   		u64 offset = results->node.start + sizeof(u32) * i;
> +		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +
> +		/* Clear RD only and WR only flags */
> +		reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
>   
>   		*cs++ = srm;
> -		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +		*cs++ = reg;
>   		*cs++ = lower_32_bits(offset);
>   		*cs++ = upper_32_bits(offset);
>   	}
> @@ -769,9 +799,14 @@ static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
>   		goto err_batch;
>   	}
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
> +	*cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
>   	for (i = 0; i < engine->whitelist.count; i++) {
> -		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +
> +		if (ro_register(reg))
> +			continue;
> +

Are we not able to test the read-only property at all?

Regards,

Tvrtko

> +		*cs++ = reg;
>   		*cs++ = 0xffffffff;
>   	}
>   	*cs++ = MI_BATCH_BUFFER_END;
> @@ -956,7 +991,7 @@ static int live_isolated_whitelist(void *arg)
>   	}
>   
>   	for_each_engine(engine, i915, id) {
> -		if (!engine->whitelist.count)
> +		if (!whitelist_writable_count(engine))
>   			continue;
>   
>   		/* Read default values */
>
John Harrison June 18, 2019, 1:43 p.m. UTC | #2 
On 6/17/2019 23:42, Tvrtko Ursulin wrote:
> On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
>> From: "Robert M. Fosha" <robert.m.fosha@intel.com>
>>
>> Updates the live_workarounds selftest to handle whitelisted
>> registers that are flagged as read only.
>>
>> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++++++--
>>   1 file changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> index c8d335d63f9c..eb6d3aa2c8cc 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> @@ -408,6 +408,29 @@ static bool wo_register(struct intel_engine_cs 
>> *engine, u32 reg)
>>       return false;
>>   }
>>   +static bool ro_register(u32 reg)
>> +{
>> +    if (reg & RING_FORCE_TO_NONPRIV_RD)
>> +        return true;
>> +
>> +    return false;
>> +}
>> +
>> +static int whitelist_writable_count(struct intel_engine_cs *engine)
>> +{
>> +    int count = engine->whitelist.count;
>> +    int i;
>> +
>> +    for (i = 0; i < engine->whitelist.count; i++) {
>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +
>> +        if (ro_register(reg))
>> +            count--;
>> +    }
>> +
>> +    return count;
>> +}
>> +
>>   static int check_dirty_whitelist(struct i915_gem_context *ctx,
>>                    struct intel_engine_cs *engine)
>>   {
>> @@ -463,6 +486,9 @@ static int check_dirty_whitelist(struct 
>> i915_gem_context *ctx,
>>           if (wo_register(engine, reg))
>>               continue;
>>   +        if (ro_register(reg))
>> +            continue;
>> +
>>           srm = MI_STORE_REGISTER_MEM;
>>           lrm = MI_LOAD_REGISTER_MEM;
>>           if (INTEL_GEN(ctx->i915) >= 8)
>> @@ -734,9 +760,13 @@ static int read_whitelisted_registers(struct 
>> i915_gem_context *ctx,
>>         for (i = 0; i < engine->whitelist.count; i++) {
>>           u64 offset = results->node.start + sizeof(u32) * i;
>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +
>> +        /* Clear RD only and WR only flags */
>> +        reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
>>             *cs++ = srm;
>> -        *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +        *cs++ = reg;
>>           *cs++ = lower_32_bits(offset);
>>           *cs++ = upper_32_bits(offset);
>>       }
>> @@ -769,9 +799,14 @@ static int scrub_whitelisted_registers(struct 
>> i915_gem_context *ctx,
>>           goto err_batch;
>>       }
>>   -    *cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
>> +    *cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
>>       for (i = 0; i < engine->whitelist.count; i++) {
>> -        *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +
>> +        if (ro_register(reg))
>> +            continue;
>> +
>
> Are we not able to test the read-only property at all?
>
I am sure it would be possible to make such work. But can that wait 
until the next round when we add support for ranges? And write only 
access too if any registers actually use that and there is a way to test 
that it really does do something?

I believe Robert was looking at getting something going but it wasn't 
immediately working and we urgently need to get the HUC whitelist 
updates merged to hit the next release window. So right now, it is 
sufficient to say that the user land media driver works with these 
changes and therefore the whitelisting must be working. The kernel 
selftest is just a belt and braces check on top of that and therefore 
can wait until later.

John.
Tvrtko Ursulin June 18, 2019, 4:14 p.m. UTC | #3 
On 18/06/2019 14:43, John Harrison wrote:
> On 6/17/2019 23:42, Tvrtko Ursulin wrote:
>> On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
>>> From: "Robert M. Fosha" <robert.m.fosha@intel.com>
>>>
>>> Updates the live_workarounds selftest to handle whitelisted
>>> registers that are flagged as read only.
>>>
>>> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++++++--
>>>   1 file changed, 39 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
>>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> index c8d335d63f9c..eb6d3aa2c8cc 100644
>>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> @@ -408,6 +408,29 @@ static bool wo_register(struct intel_engine_cs 
>>> *engine, u32 reg)
>>>       return false;
>>>   }
>>>   +static bool ro_register(u32 reg)
>>> +{
>>> +    if (reg & RING_FORCE_TO_NONPRIV_RD)
>>> +        return true;
>>> +
>>> +    return false;
>>> +}
>>> +
>>> +static int whitelist_writable_count(struct intel_engine_cs *engine)
>>> +{
>>> +    int count = engine->whitelist.count;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < engine->whitelist.count; i++) {
>>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>>> +
>>> +        if (ro_register(reg))
>>> +            count--;
>>> +    }
>>> +
>>> +    return count;
>>> +}
>>> +
>>>   static int check_dirty_whitelist(struct i915_gem_context *ctx,
>>>                    struct intel_engine_cs *engine)
>>>   {
>>> @@ -463,6 +486,9 @@ static int check_dirty_whitelist(struct 
>>> i915_gem_context *ctx,
>>>           if (wo_register(engine, reg))
>>>               continue;
>>>   +        if (ro_register(reg))
>>> +            continue;
>>> +
>>>           srm = MI_STORE_REGISTER_MEM;
>>>           lrm = MI_LOAD_REGISTER_MEM;
>>>           if (INTEL_GEN(ctx->i915) >= 8)
>>> @@ -734,9 +760,13 @@ static int read_whitelisted_registers(struct 
>>> i915_gem_context *ctx,
>>>         for (i = 0; i < engine->whitelist.count; i++) {
>>>           u64 offset = results->node.start + sizeof(u32) * i;
>>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>>> +
>>> +        /* Clear RD only and WR only flags */
>>> +        reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
>>>             *cs++ = srm;
>>> -        *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>>> +        *cs++ = reg;
>>>           *cs++ = lower_32_bits(offset);
>>>           *cs++ = upper_32_bits(offset);
>>>       }
>>> @@ -769,9 +799,14 @@ static int scrub_whitelisted_registers(struct 
>>> i915_gem_context *ctx,
>>>           goto err_batch;
>>>       }
>>>   -    *cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
>>> +    *cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
>>>       for (i = 0; i < engine->whitelist.count; i++) {
>>> -        *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>>> +
>>> +        if (ro_register(reg))
>>> +            continue;
>>> +
>>
>> Are we not able to test the read-only property at all?
>>
> I am sure it would be possible to make such work. But can that wait 
> until the next round when we add support for ranges? And write only 
> access too if any registers actually use that and there is a way to test 
> that it really does do something?
> 
> I believe Robert was looking at getting something going but it wasn't 
> immediately working and we urgently need to get the HUC whitelist 
> updates merged to hit the next release window. So right now, it is 
> sufficient to say that the user land media driver works with these 
> changes and therefore the whitelisting must be working. The kernel 
> selftest is just a belt and braces check on top of that and therefore 
> can wait until later.

I don't agree that it is just belt and braces but in fact a way to check 
that the thing really is read-only like it says on the tin. Lesson here 
is that history keeps repeating panics which could have been avoided by 
running the pre-existing tests during development.

However since I don't think there is any risk to driver or system 
stability, even if the read-only property happened to be broken, which 
it probably isn't, you can have a grumpy:

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

Regards,

Tvrtko
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index c8d335d63f9c..eb6d3aa2c8cc 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -408,6 +408,29 @@  static bool wo_register(struct intel_engine_cs *engine, u32 reg)
 	return false;
 }
 
+static bool ro_register(u32 reg)
+{
+	if (reg & RING_FORCE_TO_NONPRIV_RD)
+		return true;
+
+	return false;
+}
+
+static int whitelist_writable_count(struct intel_engine_cs *engine)
+{
+	int count = engine->whitelist.count;
+	int i;
+
+	for (i = 0; i < engine->whitelist.count; i++) {
+		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+		if (ro_register(reg))
+			count--;
+	}
+
+	return count;
+}
+
 static int check_dirty_whitelist(struct i915_gem_context *ctx,
 				 struct intel_engine_cs *engine)
 {
@@ -463,6 +486,9 @@  static int check_dirty_whitelist(struct i915_gem_context *ctx,
 		if (wo_register(engine, reg))
 			continue;
 
+		if (ro_register(reg))
+			continue;
+
 		srm = MI_STORE_REGISTER_MEM;
 		lrm = MI_LOAD_REGISTER_MEM;
 		if (INTEL_GEN(ctx->i915) >= 8)
@@ -734,9 +760,13 @@  static int read_whitelisted_registers(struct i915_gem_context *ctx,
 
 	for (i = 0; i < engine->whitelist.count; i++) {
 		u64 offset = results->node.start + sizeof(u32) * i;
+		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+		/* Clear RD only and WR only flags */
+		reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
 
 		*cs++ = srm;
-		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		*cs++ = reg;
 		*cs++ = lower_32_bits(offset);
 		*cs++ = upper_32_bits(offset);
 	}
@@ -769,9 +799,14 @@  static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
 		goto err_batch;
 	}
 
-	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
+	*cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
 	for (i = 0; i < engine->whitelist.count; i++) {
-		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+		if (ro_register(reg))
+			continue;
+
+		*cs++ = reg;
 		*cs++ = 0xffffffff;
 	}
 	*cs++ = MI_BATCH_BUFFER_END;
@@ -956,7 +991,7 @@  static int live_isolated_whitelist(void *arg)
 	}
 
 	for_each_engine(engine, i915, id) {
-		if (!engine->whitelist.count)
+		if (!whitelist_writable_count(engine))
 			continue;
 
 		/* Read default values */