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

[Sagar Arun Kamble]

On 9/25/2017 3:31 PM, Joonas Lahtinen wrote:
> 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.
Agree.
>
>>> 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
Sure. Will request for this change to be made in the firmware.