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

[Joonas Lahtinen]

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