Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Sagar Arun Kamble <sagar.a.kamble@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Fry Gregory P <gregory.p.fry@intel.com>,
	Chheda Harsh J <harsh.j.chheda@intel.com>
Subject: Re: [PATCH v7 2/2] drm/i915/guc: Enable default/critical logging in GuC by default from GuC v9
Date: Mon, 25 Sep 2017 13:01:34 +0300	[thread overview]
Message-ID: <1506333694.5593.22.camel@linux.intel.com> (raw)
In-Reply-To: <a786e3ca-1fb0-698c-5e59-015261bf19ea@intel.com>

On Fri, 2017-09-22 at 20:16 +0530, Sagar Arun Kamble wrote:
> 
> On 9/22/2017 5:34 PM, Joonas Lahtinen wrote:
> > On Fri, 2017-09-22 at 15:37 +0530, Sagar Arun Kamble wrote:
> > > With GuC v9, new type of Default/critical logging in GuC to enable
> > > capturing minimal important logs in production systems efficiently.
> > > This patch enables this logging in GuC by default always. It should
> > > be noted that streaming support with half-full interrupt mechanism
> > > that is present for normal logging is not present for this type of
> > > logging.
> > 
> > The commit message would be a good place to debrief the user impact. Do
> > we have the tools to capture the new style of log?
> 
> This has been verified to have minor impact by GuC team. Greg, Harsh can 
> clarify further.
> Goal was to allow to get GuC logs from production systems.
> These are the subset of same GuC logs available currently at all GuC log 
> levels and are needed to be captured
> always whenever we want to capture GuC logs at i915.guc_log_level >=0 
> and <=3.
> Chris had suggested on why this is not made on of the log levels and on 
> discussion with Harsh it is
> concluded that this will not be supported as log level as GuC is using 
> fast functions to log the new
> critical logs. Also there will not be half-full streaming support for 
> these type of logs.
> I did have a patch earlier to disable this by default or emulate it as 
> log level to the end user but recommendation
> from GuC team is to always turn this ON.

I see little point in detecting firmware version and unconditionally
setting a flag up at boot from kernel driver, when the firmware can do
it themselves knowing their own version.

We should not be polluting driver codebase with such constructs. It's
firmware, a new version can be provided easily unlike with hardware
which might need W/As.

> > And question is why do we enable any logging by default, let it have
> > much or little impact on performance? This flag should probably be set
> > through debugfs or some other means when we know we're going to want
> > debugging output.
> 
> In my earlier patch we did have ability to turn this ON/OFF 
> https://patchwork.freedesktop.org/patch/173884/

Yeah, I think we might want to extend the existing .enable_guc_logging
parameter once we understand the relation between critical and regular
logging in a released firmware.

Could be as simple as;

(...).enable_critical_logging = (i915_modparams.guc_log_level >= 0);

For the time being, we shall wait for a fixed firmware.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx