Re: [Intel-gfx] [PATCH v5 01/10] drm/i915/guc: Update GuC ADS size for error capture lists

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

On Wed, Jan 26, 2022 at 02:46:19PM -0800, Lucas De Marchi wrote:
> On Wed, Jan 26, 2022 at 02:48:13AM -0800, Alan Previn wrote:
> > Update GuC ADS size allocation to include space for
> > the lists of error state capture register descriptors.
> > 
> > Also, populate the lists of registers we want GuC to report back to
> > Host on engine reset events. This list should include global,
> > engine-class and engine-instance registers for every engine-class
> > type on the current hardware.
> > 
> > NOTE: Start with a sample table of register lists to layout the
> > framework before adding real registers in subsequent patch.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> 
> ...
> 
> > static void __guc_ads_init(struct intel_guc *guc)
> > {
> > 	struct intel_gt *gt = guc_to_gt(guc);
> > @@ -573,9 +553,9 @@ static void __guc_ads_init(struct intel_guc *guc)
> > 
> > 	base = intel_guc_ggtt_offset(guc, guc->ads_vma);
> > 
> > -	/* Capture list for hang debug */
> > -	guc_capture_list_init(guc, blob);
> > -
> > +	/* Lists for error capture debug */
> > +	intel_guc_capture_prep_lists(guc, (struct guc_ads *)blob, base,
> 
> no, please don't cast/export struct guc_ads like this. We can't really
> dereference it since it may be in IO memory.
> 
> See https://patchwork.freedesktop.org/series/99378/ with the huge
> refactor in this file to make it conform to the rules of accessing IO
> memory.
> 
> Maybe this list could be appended in the same reglist buffer and we just
> copy it once to its final location, like we are doing with the reglist?
> 

Agree with Lucas here, let's create the capture list on probe and store
it somewhere in the device data. On ADS population this more or less
turns into a memcpy (or after Lucas's series a dma-buf-map call). And on
fini, just free to stored data. The create capture list IMO is fine to
be done in an external file and return the data to the ADS code but
let's make sure everyone is on board with that. Maybe ping Lucas
directly about this?

Specific patch Lucas's is referencing:
https://patchwork.freedesktop.org/patch/471051/?series=99378&rev=1

Matt

> Lucas De Marchi