Re: [PATCH v3 3/6] drm/xe: Store xe_he_engine in xe_hw_engine_snapshot

["Souza, Jose" <jose.souza@intel.com>]

On Mon, 2024-01-29 at 13:16 -0800, Matt Roper wrote:
> On Mon, Jan 29, 2024 at 10:17:39AM -0800, José Roberto de Souza wrote:
> > A future patch will require gt and xe device structs, so here
> > replacing class by hwe.
> 
> I thought this didn't work because in cases like a failure during probe
> the the devcoredump can persist for a while after the xe_device (and
> xe_gt, and xe_hw_engine) have all been kfree'd?
> 
> I.e., we can use the GT or HWE during the capture phase, but by the time
> we get to the print phase they may already have been deallocated and we
> can't touch anything except the data we've already stashed away locally?

No, devcoredump needs to be removed before xe_device.
Rodrigo is even fixing devcoredump to do that: https://lore.kernel.org/all/20240117195349.343083-1-rodrigo.vivi@intel.com/T/

> 
> 
> Matt
> 
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_hw_engine.c       | 6 +++---
> >  drivers/gpu/drm/xe/xe_hw_engine_types.h | 4 ++--
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index 0d17e32d70c87..61b0c18d8b624 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -760,7 +760,7 @@ xe_hw_engine_snapshot_capture(struct xe_hw_engine *hwe)
> >  		return NULL;
> >  
> >  	snapshot->name = kstrdup(hwe->name, GFP_ATOMIC);
> > -	snapshot->class = hwe->class;
> > +	snapshot->hwe = hwe;
> >  	snapshot->logical_instance = hwe->logical_instance;
> >  	snapshot->forcewake.domain = hwe->domain;
> >  	snapshot->forcewake.ref = xe_force_wake_ref(gt_to_fw(hwe->gt),
> > @@ -807,7 +807,7 @@ xe_hw_engine_snapshot_capture(struct xe_hw_engine *hwe)
> >  	snapshot->reg.ring_eir = hw_engine_mmio_read32(hwe, RING_EIR(0));
> >  	snapshot->reg.ipehr = hw_engine_mmio_read32(hwe, RING_IPEHR(0));
> >  
> > -	if (snapshot->class == XE_ENGINE_CLASS_COMPUTE)
> > +	if (snapshot->hwe->class == XE_ENGINE_CLASS_COMPUTE)
> >  		snapshot->reg.rcu_mode = xe_mmio_read32(hwe->gt, RCU_MODE);
> >  
> >  	return snapshot;
> > @@ -852,7 +852,7 @@ void xe_hw_engine_snapshot_print(struct xe_hw_engine_snapshot *snapshot,
> >  	drm_printf(p, "\tBBADDR: 0x%016llx\n", snapshot->reg.ring_bbaddr);
> >  	drm_printf(p, "\tDMA_FADDR: 0x%016llx\n", snapshot->reg.ring_dma_fadd);
> >  	drm_printf(p, "\tIPEHR: 0x%08x\n", snapshot->reg.ipehr);
> > -	if (snapshot->class == XE_ENGINE_CLASS_COMPUTE)
> > +	if (snapshot->hwe->class == XE_ENGINE_CLASS_COMPUTE)
> >  		drm_printf(p, "\tRCU_MODE: 0x%08x\n",
> >  			   snapshot->reg.rcu_mode);
> >  }
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > index d7f828c76cc5f..27deaa31efd31 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > @@ -158,8 +158,8 @@ struct xe_hw_engine {
> >  struct xe_hw_engine_snapshot {
> >  	/** @name: name of the hw engine */
> >  	char *name;
> > -	/** @class: class of this hw engine */
> > -	enum xe_engine_class class;
> > +	/** @hwe: hw engine */
> > +	struct xe_hw_engine *hwe;
> >  	/** @logical_instance: logical instance of this hw engine */
> >  	u16 logical_instance;
> >  	/** @forcewake: Force Wake information snapshot */
> > -- 
> > 2.43.0
> > 
>