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

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jul 31, 2024 at 08:14:23PM +0000, 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@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.
> 

I agree with John H that we can't have devcoredump output be UABI as
over time this will change as the KMD internals change. Perhaps in
devcoredump, we include a version number or something which indicates
the format? I know version numbers for uAPI is a no go, but maybe this
works devcoredump.

Matt

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