Re: [PATCH] drm/xe: Add engine name to the engine reset and cat-err log

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

On Mon, Apr 29, 2024 at 02:17:31PM -0700, John Harrison wrote:
> On 4/25/2024 14:16, Nirmoy Das wrote:
> > Hi Matt,
> > 
> > On 4/25/2024 10:31 PM, Matthew Brost wrote:
> > > On Thu, Apr 25, 2024 at 09:11:22PM +0200, Nirmoy Das wrote:
> > > > Hi Matt,
> > > > 
> > > > On 4/25/2024 7:46 PM, Matthew Brost wrote:
> > > > > On Thu, Apr 25, 2024 at 02:18:56PM +0200, Nirmoy Das wrote:
> > > > > > Add engine name to the engine reset and cat error log
> > > > > > which should be useful while debugging.
> > > > > > 
> > > > > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > > > > > ---
> > > > > >    drivers/gpu/drm/xe/xe_guc_submit.c | 5 +++--
> > > > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > > > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > > > index c7d38469fb46..245e29d095c0 100644
> > > > > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > > > @@ -1655,7 +1655,7 @@ int
> > > > > > xe_guc_exec_queue_reset_handler(struct xe_guc *guc, u32
> > > > > > *msg, u32 len)
> > > > > >        if (unlikely(!q))
> > > > > >            return -EPROTO;
> > > > > > -    drm_info(&xe->drm, "Engine reset: guc_id=%d", guc_id);
> > > > > > +    drm_info(&xe->drm, "Engine reset: name=%s,
> > > > > > guc_id=%d", q->hwe->name, guc_id);
> > > > > I don't think q->hwe->name name is useful as it might not actually be
> > > > > exec queue is running. I'd drop that, and replace with
> > > > > string indicating
> Not following this. What is q->hwe->name if it is not the name of the exec
> queue which owns the given guc_id?
> 

A hwe is a 'struct xe_hw_engine'.

> Note that the notification is officially about a context reset not an engine
> reset. The actual implementation mechanism might be an engine reset but the
> intent and purpose is to reset the specific context as identified by the
> guc_id field. That is, the error report is not that BCS37 failed and needed
> to be reset, but that context 43 failed and needed to be reset and the fact
> that it happened to executing on BCS37 at the time is more coincidence than
> cause.
> 
> If the q name is not meaningful and just some generic string then maybe the
> better fix would be to make that name more useful?
> 

Again that is a pointer to 'struct xe_hw_engine'. We could use q->name
here I guess which is the class + guc_id.

> I would also change the base string to be 'Context reset' rather than
> 'Engine reset'.
> 

That old nomenclature, 'engine'. 'Context' isn't used in Xe either.
'Exec queue' would be correct here.

Matt 

> 
> > > > > the hardware engine class.
> > > > I will resend with engine class instead.
> > > > 
> > > Maybe include the logical mask of exec queue too.
> > 
> > Will do that!
> > 
> > Nirmoy
> > 
> > > 
> > > Matt
> > > 
> > > > Thanks,
> > > > 
> > > > Nirmoy
> > > > 
> > > > > >        /* FIXME: Do error capture, most likely async */
> > > > > > @@ -1690,7 +1690,8 @@ int
> > > > > > xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc
> > > > > > *guc, u32 *msg,
> > > > > >        if (unlikely(!q))
> > > > > >            return -EPROTO;
> > > > > > -    drm_dbg(&xe->drm, "Engine memory cat error: guc_id=%d", guc_id);
> > > > > > +    drm_dbg(&xe->drm, "Engine memory cat error: name=%s, guc_id=%d",
> > > > > > +        q->hwe->name, guc_id);
> > > > > Same here.
> > > > > 
> Indeed. This is also not about an engine failing and create a memory error.
> It is about a context attempting to access an invalid address.
> 
> John.
> 
> > > > > Matt
> > > > > 
> > > > > > trace_xe_exec_queue_memory_cat_error(q);
> > > > > >        /* Treat the same as engine reset */
> > > > > > -- 
> > > > > > 2.42.0
> > > > > > 
>