[PATCH v5 1/8] drm/xe/guc: Remove spurious line feed in debug print

[John Harrison]

On 7/31/2024 13:14, Souza, Jose wrote:
> On Wed, 2024-07-31 at 12:56 -0700, John Harrison wrote:
>> On 7/30/2024 02:14, Michal Wajdeczko wrote:
>>> On 30.07.2024 01:17, John.C.Harrison at Intel.com wrote:
>>>> From: John Harrison <John.C.Harrison at Intel.com>
>>>>
>>>> Including line feeds at the start of a debug print messes up the
>>>> output when sent to dmesg. The break actually appears between all the
>>>> usefu
>>> typo
>>>
>>>> prefix information and the actual string being printed. In this
>>>> case, each block of data has a very clear start line and an extra
>>>> delimeter is really not necessary. So don't do it.
>>>>
>>>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>>>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> there was some discussion about merging this one without a conclusion
>>>
>>> [1] https://patchwork.freedesktop.org/patch/601018/?series=135447&rev=1
>> The last comment was for Mesa people to shout if it would be a problem
>> and no-one shouted, so...
>>
>> However, I would strongly argue that devcoredump exact layout and
>> content cannot be considered UABI because it is going to change as the
>> driver changes. Some of the information being printed is internal driver
>> state. Driver internals can never be UABI. If there are userland tools
>> parsing the dump then those tools have to be able to adapt to changing
>> core dump formats. There is also the argument that we are still in
>> force-probe so there is no fixed UABI yet anyway. So now is the time to
>> get the formatting as good as possible before officially going live.
> I don't think KMD can freely break fundamental UMD tools, during my time in KMD team it was not even accepted to break the behavior of a sysfs only
> use by IGT display tests.
My point is not that the KMD can do what it likes and break tools on a 
whim. My point is that the content is going to change because the driver 
itself is going to change - different hardware, different software 
algorithms, etc. There is nothing we can do about that. But what we can 
do is write those userland tools to be flexible and cope with unexpected 
changes in the core dump file.

E.g. use the '*** XXX ***' headers as section delimiters rather than 
assuming the presence and meaning of white space.

>
> Like I said in the previous version, I agree with the change if you add one line breaker in the end of guc_ctb_snapshot_print(), with that: Reviewed-
> by: José Roberto de Souza <jose.souza at intel.com>
Sorry, missed that comment.

We can't put a an extra blank line inside the helper function itself. 
That would break between "status (memory)...." and "g2h outstanding...".

> having a break line between sub-sections is good for readability:
But that is what the indentation is for.

Each sub-section is indented below a header line so that you can easily 
see exactly what goes with what. A blank line between main sections is 
maybe helpful but for subsections, IMO, it breaks things up too much. 
Sub-sections should be kept together to show that they are all the 
related to the same entity.

John.

>
>
> **** Xe Device Coredump ****
> kernel: 6.9.0-rc6-zeh-xe+
> module: xe
> Snapshot time: 1715877420.647211377
> Uptime: 70684.605982665
> PCI ID: 0x9a49
> PCI revision: 0x01
> GT id: 0
> 	Type: main
> 	IP ver: 0.0.0
> 	CS reference clock: 19200000
>
> **** GuC CT ****
> H2G CTB (all sizes in DW):
> 	size: 1024
> 	resv_space: 0
> 	head: 978
> 	tail: 599
> 	space: 378
> 	broken: 0
> 	head (memory): 599
> 	tail (memory): 599
> 	status (memory): 0x0
>
> G2H CTB (all sizes in DW):
> 	size: 4096
> 	resv_space: 1024
> 	head: 626
> 	tail: 0
> 	space: 3071
> 	broken: 0
> 	head (memory): 626
> 	tail (memory): 626
> 	status (memory): 0x0
> 	g2h outstanding: 0
>
> GuC ID: 9
> 	Name: rcs9
> 	Class: 0
> 	Logical mask: 0x1
> 	Width: 1
> 	Ref: 4
> 	Timeout: 0 (ms)
> 	Timeslice: 1000 (us)
> 	Preempt timeout: 640000 (us)
> 	HW Context Desc: 0x01480000
> 	LRC Head: (memory) 280
> 	LRC Tail: (internal) 552, (memory) 552
> 	Start seqno: (memory) -125
> 	Seqno: (memory) -126
> 	[HWSP].length: 0x1000
>
>
>> John.
>>
>>
>>>> ---
>>>>    drivers/gpu/drm/xe/xe_guc_ct.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> index beeeb120d1fc..422c3f5c87d8 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
>>>> @@ -1515,7 +1515,7 @@ void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot,
>>>>    		drm_puts(p, "H2G CTB (all sizes in DW):\n");
>>>>    		guc_ctb_snapshot_print(&snapshot->h2g, p);
>>>>    
>>>> -		drm_puts(p, "\nG2H CTB (all sizes in DW):\n");
>>>> +		drm_puts(p, "G2H CTB (all sizes in DW):\n");
>>>>    		guc_ctb_snapshot_print(&snapshot->g2h, p);
>>>>    
>>>>    		drm_printf(p, "\tg2h outstanding: %d\n",