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

[John Harrison <john.c.harrison@intel.com>]

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
>