Re: Re: [PATCH] drm/xe: Do not flood dmesg with guc log

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

On Thu, Jan 18, 2024 at 12:16:27PM -0600, Lucas De Marchi wrote:
> On Thu, Jan 18, 2024 at 09:31:09AM -0500, Rodrigo Vivi wrote:
> > On Wed, Jan 17, 2024 at 05:42:00PM -0600, Lucas De Marchi wrote:
> > > On Wed, Jan 17, 2024 at 03:40:59PM -0500, Rodrigo Vivi wrote:
> > > > This information is already present at
> > > > /sys/kernel/debug/dri/0/gt0/uc/guc_log if needed.
> > > 
> > > but is it persisted if we couldn't load guc?
> > 
> > well, it was there here with the same content that was spit to dmesg.
> > Maybe just because I got this after a resume?
> 
> not sure. I remember past debugs I had to rely on the guc log being
> relayed to to kernel log when debugging issues on the golden LRC
> submission. Honestly I don't remember if that was with i915 or xe.
> 
> commit 4bc3a34f1237 ("drm/xe: Dump GuC log to dmesg on load / auth failure")
> seems to be the one adding it before drm-xe-next creation, but maybe
> we just didn't have a better alternative at the time.
> 
> > 
> > If so I would still prefer to make that persistent instead
> > of the flooded dmesg that gets really messed up and hard
> > to navigate to find the real useful information of the
> > failures.
> 
> that would be my preference too. If it works, I'm fine with that. At
> least the end user should never be exposed to that amount of info we
> print.
> 
> could we add the guc log to a devcoredump if we failed earlier?
>

I agree that if we can avoid flooding dmesg either via debugfs being
available or creating a devcoredump that is the preference.

I added this code originally to debug failures on driver load during
early Xe bring up. I haven't used this in a very long time.

Matt
 
> Lucas De Marchi
> 
> > 
> > > 
> > > >
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_guc.c | 1 -
> > > > 1 file changed, 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> > > > index 235d27b17ff99..2a71348c5deda 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc.c
> > > > @@ -466,7 +466,6 @@ static int guc_wait_ucode(struct xe_guc *guc)
> > > > 			ret = -ENXIO;
> > > > 		}
> > > >
> > > 
> > > trailing newline above
> > 
> > not actually a newline, but I can remove that extra line there
> > while removing this.
> > Wonder if I also should remove the spaces between the if/else
> > above this block as well... The "style" there looked strange,
> > but I decided to not touch.
> > 
> > > 
> > > Lucas De Marchi
> > > 
> > > > -		xe_guc_log_print(&guc->log, &p);
> > > > 	} else {
> > > > 		drm_dbg(&xe->drm, "GuC successfully loaded");
> > > > 	}
> > > > --
> > > > 2.43.0
> > > >