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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux