Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed
> > > 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. NOTE: John and I had an offline conversation and we are aware that there will still be a case where we can miss new-platform updates for guc-error-capture without being alerted by a warning: Let's take the example of the empty blitter's engine-class-register-list. We dont have such a thing on today's hardware.. we only have blitter's engine-register-list ... i.e. HEAD, TAIL etc. But if a future platform were to introduce a blitter engine-class-register-list, we wont get a warning since the empty list is there to prevent unnecessary warning for today's hardware. But we know this is better than having to explain unnecessary warnings (which was the reason why a more verbose version of this code was removed in the past). I believe we are good with this solution here for now.
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed
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
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed
On 17/10/2022 18:46, Teres Alexis, Alan Previn wrote:
On Mon, 2022-10-17 at 09:42 +0100, Tvrtko Ursulin wrote:
On 15/10/2022 04:59, Alan Previn wrote:
If GuC is being used and we initialized GuC-error-capture,
we need to be warning if we don't provide an error-capture
register list in the firmware ADS, for valid GT engines.
A warning makes sense as this would impact debugability
without realizing why a reglist wasn't retrieved and reported
by GuC.>
However, depending on the platform, we might have certain
engines that have a register list for engine instance error state
but not for engine class. Thus, add a check only to warn if the
register list was non existent vs an empty list (use the
empty lists to skip the warning).
Signed-off-by: Alan Previn
---
.../gpu/drm/i915/gt/uc/intel_guc_capture.c| 55 ++-
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index 8f1165146013..290c1e1343dd 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -419,6 +419,44 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
return default_lists;
}
+static const char *
+__stringify_type(u32 type)
+{
+ switch (type) {
+ case GUC_CAPTURE_LIST_TYPE_GLOBAL:
+ return "Global";
+ case GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS:
+ return "Class";
+ case GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE:
+ return "Instance";
+ default:
+ return "unknown";
+ }
+
+ return "";
What's the point of these returns? Gcc complains about not returning a type
from const char * return function?
Sorry i am not sure I saw any complain for Gcc. If you are referring to "" then
i can re-rev without the default and
just let the path outside return the unknown. Is that what you are referring to?
Yes, it is an unreachable path, handled by default switch branch already.
+}
+
+static const char *
+__stringify_engclass(u32 class)
+{
+ switch (class) {
+ case GUC_RENDER_CLASS:
+ return "Render";
+ case GUC_VIDEO_CLASS:
+ return "Video";
+ case GUC_VIDEOENHANCE_CLASS:
+ return "VideoEnhance";
+ case GUC_BLITTER_CLASS:
+ return "Blitter";
+ case GUC_COMPUTE_CLASS:
+ return "Compute";
+ default:
+ return "unknown";
+ }
+
+ return "";
+}
+
static int
guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32
classid,
struct guc_mmio_reg *ptr, u16 num_entries)
@@ -487,19 +525,32 @@ intel_guc_capture_getlistsize(struct intel_guc *guc, u32
owner, u32 type, u32 cl
size_t *size)
{
struct intel_guc_state_capture *gc = guc->capture;
+ struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
struct __guc_capture_ads_cache *cache =
&gc->ads_cache[owner][type][classid];
int num_regs;
- if (!gc->reglists)
+ if (!gc->reglists) {
+ drm_warn(&i915->drm, "GuC-capture: No reglist on this
device\n");
return -ENODEV;
+ }
if (cache->is_valid) {
*size = cache->size;
return cache->status;
}
+ 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 kee
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed
On 10/17/2022 16:36, Teres Alexis, Alan Previn wrote:
Agreed on all the others (as per my other reply to Tvrtko), but on that 2nd
last one:
On Mon, 2022-10-17 at 12:33 -0700, Harrison, John C wrote:
On 10/14/2022 20:59, Alan Previn wrote:
If GuC is being used and we initialized GuC-error-capture,
we need to be warning if we don't provide an error-capture
register list in the firmware ADS, for valid GT engines.
A warning makes sense as this would impact debugability
without realizing why a reglist wasn't retrieved and reported
by GuC.
+ 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)
+ drm_warn(&i915->drm, "GuC-capture: missing regiist
type(%d)-%s : "
+"%s(%d)-Engine\n", type,
__stringify_type(type),
What Tvrtko is meaning here is to not split the string at all. You can
ignore a line length warning message if the only alternatives are either
to split the string or to obfuscate the code with unreadable/unnecessary
construction methods.
I dont see how not splitting the string makes the grep work as per the reason
Tvrtko was bringing up... but sure,..
ignoring a checkpatch here is fine by me - as i do agree having a single line
is better to read.
I don't think Tvrtko was meaning anything other than the line wrap.
Having %d, %s, etc. in a string is fine if that's what you are meaning.
You really don't want to go the route of expanding all possible options
of those. And you can still grep for 'missing reglist.*Engine' for
example. But yeah, with this particular one I think it is more about
code readability than greppability for me at least.
John.
...alan
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed
Agreed on all the others (as per my other reply to Tvrtko), but on that 2nd
last one:
On Mon, 2022-10-17 at 12:33 -0700, Harrison, John C wrote:
> On 10/14/2022 20:59, Alan Previn wrote:
> > If GuC is being used and we initialized GuC-error-capture,
> > we need to be warning if we don't provide an error-capture
> > register list in the firmware ADS, for valid GT engines.
> > A warning makes sense as this would impact debugability
> > without realizing why a reglist wasn't retrieved and reported
> > by GuC.
> >
> > + 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)
> > + drm_warn(&i915->drm, "GuC-capture: missing regiist
> > type(%d)-%s : "
> > +"%s(%d)-Engine\n", type,
> > __stringify_type(type),
> What Tvrtko is meaning here is to not split the string at all. You can
> ignore a line length warning message if the only alternatives are either
> to split the string or to obfuscate the code with unreadable/unnecessary
> construction methods.
>
>
I dont see how not splitting the string makes the grep work as per the reason
Tvrtko was bringing up... but sure,..
ignoring a checkpatch here is fine by me - as i do agree having a single line
is better to read.
...alan
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed
On 10/14/2022 20:59, Alan Previn wrote:
If GuC is being used and we initialized GuC-error-capture,
we need to be warning if we don't provide an error-capture
register list in the firmware ADS, for valid GT engines.
A warning makes sense as this would impact debugability
without realizing why a reglist wasn't retrieved and reported
by GuC.
However, depending on the platform, we might have certain
engines that have a register list for engine instance error state
but not for engine class. Thus, add a check only to warn if the
register list was non existent vs an empty list (use the
empty lists to skip the warning).
Signed-off-by: Alan Previn
---
.../gpu/drm/i915/gt/uc/intel_guc_capture.c| 55 ++-
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index 8f1165146013..290c1e1343dd 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -419,6 +419,44 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
return default_lists;
}
+static const char *
+__stringify_type(u32 type)
+{
+ switch (type) {
+ case GUC_CAPTURE_LIST_TYPE_GLOBAL:
+ return "Global";
+ case GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS:
+ return "Class";
+ case GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE:
+ return "Instance";
+ default:
+ return "unknown";
+ }
+
+ return "";
As per Tvrtko's comment, this is dead code and unnecessary. A blank
'default:' that falls through to 'return "Unknown";' would be better.
+}
+
+static const char *
+__stringify_engclass(u32 class)
+{
+ switch (class) {
+ case GUC_RENDER_CLASS:
+ return "Render";
+ case GUC_VIDEO_CLASS:
+ return "Video";
+ case GUC_VIDEOENHANCE_CLASS:
+ return "VideoEnhance";
+ case GUC_BLITTER_CLASS:
+ return "Blitter";
+ case GUC_COMPUTE_CLASS:
+ return "Compute";
+ default:
+ return "unknown";
+ }
+
+ return "";
As above.
+}
+
static int
guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
struct guc_mmio_reg *ptr, u16 num_entries)
@@ -487,19 +525,32 @@ intel_guc_capture_getlistsize(struct intel_guc *guc, u32
owner, u32 type, u32 cl
size_t *size)
{
struct intel_guc_state_capture *gc = guc->capture;
+ struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
struct __guc_capture_ads_cache *cache =
&gc->ads_cache[owner][type][classid];
int num_regs;
- if (!gc->reglists)
+ if (!gc->reglists) {
+ drm_warn(&i915->drm, "GuC-capture: No reglist on this
device\n");
return -ENODEV;
+ }
if (cache->is_valid) {
*size = cache->size;
return cache->status;
}
+ 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)
+ drm_warn(&i915->drm, "GuC-capture: missing regiist
type(%d)-%s : "
+"%s(%d)-Engine\n", type,
__stringify_type(type),
What Tvrtko is meaning here is to not split the string at all. You can
ignore a line length warning message if the only alternatives are either
to split the string or to obfuscate the code with unreadable/unnecessary
construction methods.
+__stringify_engclass(classid), classid);
+ return -ENODATA;
+ }
+
num_regs = guc_cap_list_num_regs(gc, owner, type, classid);
- if (!num_regs)
+ if (!num_regs) /* intentional empty lists can exist depending on hw
config */
Not sure if this is proper formatting for a comment? I would either put
it on the line before or inside the if with the addition of braces.
John.
return -ENODATA;
*size = PAGE_ALIGN((sizeof(struct guc_debug_capture_list)) +
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed
On Mon, 2022-10-17 at 09:42 +0100, Tvrtko Ursulin wrote:
> On 15/10/2022 04:59, Alan Previn wrote:
> > If GuC is being used and we initialized GuC-error-capture,
> > we need to be warning if we don't provide an error-capture
> > register list in the firmware ADS, for valid GT engines.
> > A warning makes sense as this would impact debugability
> > without realizing why a reglist wasn't retrieved and reported
> > by GuC.>
> > However, depending on the platform, we might have certain
> > engines that have a register list for engine instance error state
> > but not for engine class. Thus, add a check only to warn if the
> > register list was non existent vs an empty list (use the
> > empty lists to skip the warning).
> >
> > Signed-off-by: Alan Previn
> > ---
> > .../gpu/drm/i915/gt/uc/intel_guc_capture.c| 55 ++-
> > 1 file changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > index 8f1165146013..290c1e1343dd 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > @@ -419,6 +419,44 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
> > return default_lists;
> > }
> >
> > +static const char *
> > +__stringify_type(u32 type)
> > +{
> > + switch (type) {
> > + case GUC_CAPTURE_LIST_TYPE_GLOBAL:
> > + return "Global";
> > + case GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS:
> > + return "Class";
> > + case GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE:
> > + return "Instance";
> > + default:
> > + return "unknown";
> > + }
> > +
> > + return "";
>
> What's the point of these returns? Gcc complains about not returning a type
> from const char * return function?
>
Sorry i am not sure I saw any complain for Gcc. If you are referring to "" then
i can re-rev without the default and
just let the path outside return the unknown. Is that what you are referring to?
> > +}
> > +
> > +static const char *
> > +__stringify_engclass(u32 class)
> > +{
> > + switch (class) {
> > + case GUC_RENDER_CLASS:
> > + return "Render";
> > + case GUC_VIDEO_CLASS:
> > + return "Video";
> > + case GUC_VIDEOENHANCE_CLASS:
> > + return "VideoEnhance";
> > + case GUC_BLITTER_CLASS:
> > + return "Blitter";
> > + case GUC_COMPUTE_CLASS:
> > + return "Compute";
> > + default:
> > + return "unknown";
> > + }
> > +
> > + return "";
> > +}
> > +
> > static int
> > guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32
> > classid,
> > struct guc_mmio_reg *ptr, u16 num_entries)
> > @@ -487,19 +525,32 @@ intel_guc_capture_getlistsize(struct intel_guc *guc,
> > u32 owner, u32 type, u32 cl
> > size_t *size)
> > {
> > struct intel_guc_state_capture *gc = guc->capture;
> > + struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > struct __guc_capture_ads_cache *cache =
> > &gc->ads_cache[owner][type][classid];
> > int num_regs;
> >
> > - if (!gc->reglists)
> > + if (!gc->reglists) {
> > + drm_warn(&i915->drm, "GuC-capture: No reglist on this
> > device\n");
> > return -ENODEV;
> > + }
> >
> > if (cache->is_valid) {
> > *size = cache->size;
> > return cache->status;
> > }
> >
> > + 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
> t
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Add error-capture init warnings when needed
On 15/10/2022 04:59, Alan Previn wrote:
If GuC is being used and we initialized GuC-error-capture,
we need to be warning if we don't provide an error-capture
register list in the firmware ADS, for valid GT engines.
A warning makes sense as this would impact debugability
without realizing why a reglist wasn't retrieved and reported
by GuC.>
However, depending on the platform, we might have certain
engines that have a register list for engine instance error state
but not for engine class. Thus, add a check only to warn if the
register list was non existent vs an empty list (use the
empty lists to skip the warning).
Signed-off-by: Alan Previn
---
.../gpu/drm/i915/gt/uc/intel_guc_capture.c| 55 ++-
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index 8f1165146013..290c1e1343dd 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -419,6 +419,44 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
return default_lists;
}
+static const char *
+__stringify_type(u32 type)
+{
+ switch (type) {
+ case GUC_CAPTURE_LIST_TYPE_GLOBAL:
+ return "Global";
+ case GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS:
+ return "Class";
+ case GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE:
+ return "Instance";
+ default:
+ return "unknown";
+ }
+
+ return "";
What's the point of these returns? Gcc complains about not returning a type
from const char * return function?
+}
+
+static const char *
+__stringify_engclass(u32 class)
+{
+ switch (class) {
+ case GUC_RENDER_CLASS:
+ return "Render";
+ case GUC_VIDEO_CLASS:
+ return "Video";
+ case GUC_VIDEOENHANCE_CLASS:
+ return "VideoEnhance";
+ case GUC_BLITTER_CLASS:
+ return "Blitter";
+ case GUC_COMPUTE_CLASS:
+ return "Compute";
+ default:
+ return "unknown";
+ }
+
+ return "";
+}
+
static int
guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
struct guc_mmio_reg *ptr, u16 num_entries)
@@ -487,19 +525,32 @@ intel_guc_capture_getlistsize(struct intel_guc *guc, u32
owner, u32 type, u32 cl
size_t *size)
{
struct intel_guc_state_capture *gc = guc->capture;
+ struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
struct __guc_capture_ads_cache *cache =
&gc->ads_cache[owner][type][classid];
int num_regs;
- if (!gc->reglists)
+ if (!gc->reglists) {
+ drm_warn(&i915->drm, "GuC-capture: No reglist on this
device\n");
return -ENODEV;
+ }
if (cache->is_valid) {
*size = cache->size;
return cache->status;
}
+ 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?
Btw what's with the PF and VF (cover letter) references while SRIOV does not
exists upstream?
+ drm_warn(&i915->drm, "GuC-capture: missing regiist
type(%d)-%s : "
reglist
+"%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.
"""
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.
+ }
+
num_regs = guc_cap_list_num_regs(gc, owner, type, classid);
- if (!num_regs)
+ if (!num_regs) /* intentional empty lists can exist depending on hw
config */
return -ENODATA;
*size = PAGE_ALIGN((sizeof(struct guc_debug_capture_list)) +
Regards,
Tvrtko
