Re: [PATCH v5 06/10] drm/xe: Improve unexpected state error messages

[John Harrison <john.c.harrison@intel.com>]

On 6/10/2024 07:18, Matthew Brost wrote:
> Include G2H handler name when an unexpected error state messages.
>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_guc_submit.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 4464ba337d12..766ff8e48dde 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -1632,8 +1632,8 @@ int xe_guc_sched_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
>   
>   	if (unlikely(!exec_queue_pending_enable(q) &&
>   		     !exec_queue_pending_disable(q))) {
> -		drm_err(&xe->drm, "Unexpected engine state 0x%04x",
> -			atomic_read(&q->guc->state));
> +		drm_err(&xe->drm, "SCHED_DONE: Unexpected engine state 0x%04x, guc_id=%d",
> +			atomic_read(&q->guc->state), q->guc->id);
>   		return -EPROTO;
>   	}
My earlier point was that this G2H notification provides two words of 
data - the context id and the runnable state. The code is currently not 
looking at that second word. It is only looking at the internal KMD 
state and assuming that the notification will match. Instead, the code 
should read out the state from msg[1] and pass that in to 
handle_sched_done(). Which should complain if the notification is for an 
enable but only a disable is pending and vice versa. And by extension, 
should not do the disable processing if the notification was actually 
for an enable, and vice versa.

And as per Michal's comment, any update to a drm_err|warn|info|dbg|etc. 
should take the opportunity to convert it to an xe_gt_ equivalent.

John.

>   
> @@ -1671,8 +1671,8 @@ int xe_guc_deregister_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
>   
>   	if (!exec_queue_destroyed(q) || exec_queue_pending_disable(q) ||
>   	    exec_queue_pending_enable(q) || exec_queue_enabled(q)) {
> -		drm_err(&xe->drm, "Unexpected engine state 0x%04x",
> -			atomic_read(&q->guc->state));
> +		drm_err(&xe->drm, "DEREGISTER_DONE: Unexpected engine state 0x%04x, guc_id=%d",
> +			atomic_read(&q->guc->state), q->guc->id);
>   		return -EPROTO;
>   	}
>