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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

I can refactor my code to enforce an owner module to only return the size and an interim
buffer pointer (kzalloc, not io mem) and have ADS memcpy from that pointer into the ADS
substructure pointer.. 

But I hope we can make it a rule that its okay for an external owner-module to 
know and define the layout of the 2nd level substructure layout. This would allow
future new ADS substructure to have separate owner-modules handle the definition
of the substructure layout

something like this:

guc_ads_top_fwif.h

ADS owns top level ADS
{
    u32 ptr1;// substruct_foo1
    u32 ptr2;// substruct_foo2
    u32 ptr3;// substruct_foo3
	...
}

guc_capture_private.h

substruct_foo



So putting aside the fact that already have ADS containing the knowledge for
golden context,  register-save-restore, and others, but moving forward i am hoping
we can avoid piling on more and more unrelated low level knowledge inside of ADS.

The the error capture substructure layout has awareness of per PF-vs-VF, global vs
engine-class vs engine-instance and other fuse-specific awareness. So i am hoping
we can allow other moules to own the definition and parsing of the substructure
layout.

...alan


On Thu, 2022-02-03 at 12:04 -0800, Lucas De Marchi wrote:
> On Thu, Feb 03, 2022 at 11:03:24AM -0800, Matthew Brost wrote:
> > 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?
> 
> yeah, sounds good to me. My worry is exporting ADS struct layout to
> other compilation units. Asking for the entire ads blob
> (or what would be dma_buf_map in my patches, or iosys_map in the new
> version I will send soon) could quickly spread too much knowledge about it to
> the rest of the driver.
> 
I'm in partial disagreement with above. Based on above statement, are we enforcing
that we must always continue to only have ADS know the 2nd level blobl structure layout?
Doesn't that also force that intelligence of knowing its substructure contents to
always be in ADS only? So ADS C file continues to grow larger and larger with completely
orthogonal domain specific knowledge? (golden context: engine info,
error-capture: debug registers, etc..).
I believe ADS should really let the substructure headers be accessible to external
modules to deal with the parsing, size determination and preparing its contents.

NOTE: see my next comment specifically regarding the pointer access.

> I think we should at most export the speficic offset inside the buffer
> another compilation unit can fill out. In that case with the
> iosys_buf API we would return a shallow copy of our guc->ads_map +
> offset.
> 
> And the other alternative would be as you suggested: save the list in a
> temporary buffer and when needed call intel_guc_ads() to populate that
> into ads in the right place.
> 

Alan: The first part of above is already what is happening in my series today,
we have a seperate register list  in the intel_guc_capture module
that also determines (based on device and fusing) which registers
to include or exclude. There is are static global lists and
per-gt-allocated lists (based on fuse masks). The latter
is not in current rev but already commented as planned change.

So in response to the original review comment, I can change this
patch so that the ADS gets a regular heap-kzalloc-allocated pointer and
size from the error-capture module and ADS do the copying into the
io-mem ptr but i want to ensure the layout of the error-capture 
lists are not private to ADS only.

Are we okay with that?

> The integration with iosys-map I can figure out if my patch series lands
> after this one, or I can help rebasing this otherwise. But IMO we
> should not pass the plain blob pointer around regardless of iosys-map.
> 
> 
> thanks
> Lucas De Marchi
> 
> > Specific patch Lucas's is referencing:
> > https://patchwork.freedesktop.org/patch/471051/?series=99378&rev=1
> > 
> > Matt
> > 
> > > Lucas De Marchi