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

["Souza, Jose" <jose.souza@intel.com>]

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@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@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@Intel.com>
> > > Reviewed-by: Michal Wajdeczko <michal.wajdeczko@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.

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@intel.com>

having a break line between sub-sections is good for readability:


**** 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",
>