diff mbox series

[v2] drm/i915/guc: Do not complain about stale reset notifications

Message ID 20220212010425.3643192-1-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] drm/i915/guc: Do not complain about stale reset notifications | expand

Commit Message

John Harrison Feb. 12, 2022, 1:04 a.m. UTC
From: John Harrison <John.C.Harrison@Intel.com>

It is possible for reset notifications to arrive for a context that is
in the process of being banned. So don't flag these as an error, just
report it as informational (because it is still useful to know that
resets are happening even if they are being ignored).

v2: Better wording for the message (review feedback from Tvrtko).

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Daniele Ceraolo Spurio Feb. 23, 2022, 1:39 a.m. UTC | #1
On 2/11/2022 5:04 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> It is possible for reset notifications to arrive for a context that is
> in the process of being banned. So don't flag these as an error, just
> report it as informational (because it is still useful to know that
> resets are happening even if they are being ignored).
>
> v2: Better wording for the message (review feedback from Tvrtko).
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index b3a429a92c0d..3afff24b8f24 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -4022,10 +4022,10 @@ static void guc_handle_context_reset(struct intel_guc *guc,
>   		capture_error_state(guc, ce);
>   		guc_context_replay(ce);
>   	} else {
> -		drm_err(&guc_to_gt(guc)->i915->drm,
> -			"Invalid GuC engine reset notificaion for 0x%04X on %s: banned = %d, blocked = %d",
> -			ce->guc_id.id, ce->engine->name, intel_context_is_banned(ce),
> -			context_blocked(ce));
> +		drm_info(&guc_to_gt(guc)->i915->drm,
> +			 "Ignoring context reset notification for 0x%04X on %s: banned = %d, blocked = %d",

The if statement above checks for !banned, so if we're here we're banned 
for sure, no need to print it as if it was conditional. I'd reword it as 
something like: "Ignoring reset notification for banned context 0x%04X 
...". With that:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> +			 ce->guc_id.id, ce->engine->name, intel_context_is_banned(ce),
> +			 context_blocked(ce));
>   	}
>   }
>
John Harrison Feb. 23, 2022, 1:51 a.m. UTC | #2
On 2/22/2022 17:39, Ceraolo Spurio, Daniele wrote:
> On 2/11/2022 5:04 PM, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> It is possible for reset notifications to arrive for a context that is
>> in the process of being banned. So don't flag these as an error, just
>> report it as informational (because it is still useful to know that
>> resets are happening even if they are being ignored).
>>
>> v2: Better wording for the message (review feedback from Tvrtko).
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> index b3a429a92c0d..3afff24b8f24 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> @@ -4022,10 +4022,10 @@ static void guc_handle_context_reset(struct 
>> intel_guc *guc,
>>           capture_error_state(guc, ce);
>>           guc_context_replay(ce);
>>       } else {
>> -        drm_err(&guc_to_gt(guc)->i915->drm,
>> -            "Invalid GuC engine reset notificaion for 0x%04X on %s: 
>> banned = %d, blocked = %d",
>> -            ce->guc_id.id, ce->engine->name, 
>> intel_context_is_banned(ce),
>> -            context_blocked(ce));
>> +        drm_info(&guc_to_gt(guc)->i915->drm,
>> +             "Ignoring context reset notification for 0x%04X on %s: 
>> banned = %d, blocked = %d",
>
> The if statement above checks for !banned, so if we're here we're 
> banned for sure, no need to print it as if it was conditional. I'd 
> reword it as something like: "Ignoring reset notification for banned 
> context 0x%04X ...". With that:
Hmm. The patch was based on an older tree that had an extra term in the 
if. Seems like the patch applied cleanly and I didn't check the 
surrounding code! Will update it to drop the banned and blocked values.

John.


>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> Daniele
>
>> +             ce->guc_id.id, ce->engine->name, 
>> intel_context_is_banned(ce),
>> +             context_blocked(ce));
>>       }
>>   }
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index b3a429a92c0d..3afff24b8f24 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -4022,10 +4022,10 @@  static void guc_handle_context_reset(struct intel_guc *guc,
 		capture_error_state(guc, ce);
 		guc_context_replay(ce);
 	} else {
-		drm_err(&guc_to_gt(guc)->i915->drm,
-			"Invalid GuC engine reset notificaion for 0x%04X on %s: banned = %d, blocked = %d",
-			ce->guc_id.id, ce->engine->name, intel_context_is_banned(ce),
-			context_blocked(ce));
+		drm_info(&guc_to_gt(guc)->i915->drm,
+			 "Ignoring context reset notification for 0x%04X on %s: banned = %d, blocked = %d",
+			 ce->guc_id.id, ce->engine->name, intel_context_is_banned(ce),
+			 context_blocked(ce));
 	}
 }