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
>
> >
