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

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

On Thu, Jan 18, 2024 at 02:58:32PM -0600, Lucas De Marchi wrote:
> On Thu, Jan 18, 2024 at 07:10:27PM +0000, Matthew Brost wrote:
> > 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.
> 
> humn... Aside from that debug I mentioned, I don't remember really
> making use of this. If nobody is actively using, then maybe let's make
> it simpler and proceed with this patch. If the needs arise, then it
> should rather be added to devcoredump. Agreed?

yes, please! I will send a v3 removing that extra spurious line.

indeed devcoredump is the ideal place. but first we need to get some old
work from nvme folks adding support from multiple-files (other then 'data')
and then move the guc_log there. I can do that after I'm done with the rpm.

> 
> > 
> > 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.
> 
> 
> not sure I follow. Looking at the code now it seems the style became
> weird because you removed xe_guc_log_print(&guc->log, &p) without
> removing the line before
> 
> Lucas De Marchi