Re: [PATCH 1/9] drm/xe: Remove double new line in devcoredump

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Jan 22, 2024 at 06:40:14PM +0000, Souza, Jose wrote:
> On Mon, 2024-01-22 at 18:28 +0000, Summers, Stuart wrote:
> > On Mon, 2024-01-22 at 09:04 -0800, José Roberto de Souza wrote:
> > > Right now devcoredump has a new line between '**** GuC CT ****' and
> > > 'H2G CTB (all sizes in DW):' while other sections don't have.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Maarten Lankhorst <dev@lankhorst.se>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc_ct.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c
> > > b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > index ee5d99456aebc..0c07355690948 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > @@ -1366,7 +1366,7 @@ void xe_guc_ct_snapshot_print(struct
> > > xe_guc_ct_snapshot *snapshot,
> > >                 return;
> > >  
> > >         if (snapshot->ct_enabled) {
> > > -               drm_puts(p, "\nH2G CTB (all sizes in DW):\n");
> > > +               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");
> > 
> > Why not here too?
> > 
> > Also just visually IMO it is nice to have the extra spacing here. I
> > don't have a strong opinion though.
> 
> This sections outputs like below what looks fine to me:
> 
> **** GuC CT ****
> H2G CTB (all sizes in DW):
> 	size: 1024
> 	resv_space: 0
> 	head: 0
> 	tail: 424
> 	space: 599
> 	broken: 0
> 	head (memory): 424
> 	tail (memory): 424
> 	status (memory): 0x0
> 
> G2H CTB (all sizes in DW):
> 	size: 4096
> 	resv_space: 1024
> 	head: 137
> 	tail: 0
> 	space: 3071
> 	broken: 0
> 	head (memory): 137
> 	tail (memory): 137
> 	status (memory): 0x0
> 	g2h outstanding: 0
> 
> GuC ID: 2
> 	Name: rcs2
> 	Class: 0
> 	Logical mask: 0x1
> 	Width: 1
> 	Ref: 2
> 	Timeout: 5000 (ms)
> 	Timeslice: 1000 (us)
> 	Preempt timeout: 640000 (us)
> 	HW Context Desc: 0x00435000
> 	LRC Head: (memory) 416
> 	LRC Tail: (internal) 552, (memory) 552
> 	Start seqno: (memory) -124
> 	Seqno: (memory) -125
> 	Schedule State: 0x3
> 	Flags: 0x8

makes sense to me as well. But without the extra line it would
also work I guess.
up to you:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> 
> > 
> > Thanks,
> > Stuart
> > 
> > > @@ -1375,7 +1375,7 @@ void xe_guc_ct_snapshot_print(struct
> > > xe_guc_ct_snapshot *snapshot,
> > >                 drm_printf(p, "\tg2h outstanding: %d\n",
> > >                            snapshot->g2h_outstanding);
> > >         } else {
> > > -               drm_puts(p, "\nCT disabled\n");
> > > +               drm_puts(p, "CT disabled\n");
> > >         }
> > >  }
> > >  
> > 
>