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

[Lucas De Marchi]

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?

>
>Matt
>
>> Lucas De Marchi
>>
>> >
>> > >
>> > > >
>> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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