[Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed

[Teres Alexis, Alan Previn]

Update: One additional change needed... after more testing i have come to realize that
intel_guc_capture_getlistsize is also being triggered before ADS-guc-error-capture
register-list population during initialization of the guc-error-capture module itself 
(intel_guc_capture_init). Its getting called as part of a check on the size of the
guc-log-buffer-error-capture-region to verify its big enough for the current platform
(assuming all engine masks + all steered-register permutations). So at that early
point, we do encounter the "OTHER ENGINE" showing up as a possible engine but in
fact none of the current hardware has that (yet). So to ensure this warning is not printed
during this early size estimation check:

i shall make "intel_guc_capture_getlistsize" a wrapper around a new function
"static int guc_capture_getlistsize(...[same-params]..., bool is_purpose_estimation)"
which contains all the original logic and uses new boolean for the additional check
on whether to print the warning or not.

	current code:
		if (!guc_capture_get_one_list(gc->reglists, owner, type, classid)) {
			if (owner == GUC_CAPTURE_LIST_INDEX_PF && type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
				drm_warn(&i915->drm, "Missing GuC reglist Global\n");
			...			...
			...
	new code: 
		if (!is_purpose_estimation && owner == GUC_CAPTURE_LIST_INDEX_PF &&
			!guc_capture_get_one_list(gc->reglists, owner, type, classid)) {
			if (tpe == GUC_CAPTURE_LIST_TYPE_GLOBAL)
				drm_warn(&i915->drm, "Missing GuC reglist Global\n");
			...
			...


On Tue, 2022-10-18 at 09:00 +0100, Tvrtko Ursulin wrote:
> > > > +	if (!guc_capture_get_one_list(gc->reglists, owner, type, classid)) {
> > > > +		if (owner == GUC_CAPTURE_LIST_INDEX_PF && type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
> > > > +			drm_warn(&i915->drm, "GuC-capture: missing reglist type-Global\n");
> > > > +		if (owner == GUC_CAPTURE_LIST_INDEX_PF)
> > > 
> > > GUC_CAPTURE_LIST_INDEX_PF could be made once on the enclosing if statement?
> > Sure - will do.
> > > 
> > > Btw what's with the PF and VF (cover letter) references while SRIOV does not exists upstream?
> > To maintain a scalable code flow across both the ADS code and guc-error-capture code, we do have to skip over this enum
> > else we'll encounter lots of warnings about missing VF-reglist support (which we cant check for since we dont even have
> > support - i.e we dont even have a "is not supported" check) but the GuC firmware ADS buffer allocation includes an entry
> > for VFs so we have to skip over it. This is the cleanest way i can think of without impacting other code areas and also
> > while adding the ability to warn when its important.
> > > > +			drm_warn(&i915->drm, "GuC-capture: missing regiist type(%d)-%s : "
> > > 
> > > reglist
> > thanks - will fix
> > > 
> > > > +				 "%s(%d)-Engine\n", type, __stringify_type(type),
> > > > +				 __stringify_engclass(classid), classid);
> > > 
> > > One details to consider from Documentation/process/coding-style.rst
> > > """
> > > However, never break user-visible strings such as printk messages because that breaks the ability to grep for them.
> > > """
> > > 
> > I totally agree with you but i cant find a way to keep totally grep-able way without creating a whole set of error
> > strings for the various list-types, list-owners and class-types. However i did ensure the first part of the message
> > is grep-able : "GuC-capture: missing reglist type". Do you an alternative proposal?
> 
> Yeah it is not very greppable being largely constructed at runtime, but 
> just don't break the string. IMO no gain to diverge from coding style here.
> 
> Or maybe with one level of indentation less as discussed, and maybe 
> remove "GuC-capture" if it can be implied (are there other reglists not 
> relating to error capture?), maybe it becomes short enough?
> 
> "Missing GuC reglist %s(%u):%s(%u)!", ...
> 
> ?
> 
Yes. this will work well - will use this.

> Type will never be unknown I suspect since it should always be added 
> very early during development. So type and engine suffixes may be 
> redundant? Or keep it verbose if that fits better with existing GuC 
> error capture logging, I don't know.
> 
above is good. :)
> > 
> > > Also commit message you can aim to wrap at 75 chars as per submitting-patches.rst.
> > > 
> > > > +		return -ENODATA;
> > > 
> > > Is this a new exit condition or the thing would exit on the !num_regs check below anyway? Just wondering if the series is only about logging changes or there is more to it.
> > Its no different from previous behavior - and yes its about logging the missing reg lists for hw that needs it as we are
> > missing this for DG2 we we didn't notice (we did a previous revert to remove these warnings but that time the warnings
> > was too verbose - even complaining for the intentional empty lists and for VF cases that isnt supported).
> 
> Okay think I get it, thanks. If the "positive match" logging of empty 
> list is more future proof than "negative - don't log these" you will 
> know better.
> 
> Regards,
> 
> Tvrtko
> 
> >