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

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Jun 10, 2024 at 05:09:01PM -0700, John Harrison wrote:
> 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

I was unaware, msg[1] contained runnable state. Will include in message
plus add a patch to assert this in the handle_sched_done.

> 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.
>

Already done.

Matt
 
> 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;
> >   	}
>