[Intel-gfx] [PATCH 10/17] drm/i915: New module param to control the size of buffer used for storing GuC firmware logs

[Tvrtko Ursulin]

On 18/07/16 13:19, Goel, Akash wrote:
> On 7/18/2016 3:36 PM, Tvrtko Ursulin wrote:
>> On 15/07/16 16:36, Goel, Akash wrote:
>>> On 7/15/2016 4:45 PM, Tvrtko Ursulin wrote:
>>>>
>>>> On 10/07/16 14:41, akash.goel at intel.com wrote:
>>>>> From: Akash Goel <akash.goel at intel.com>
>>>>>
>>>>> On recieving the log buffer flush interrupt from GuC firmware, Driver
>>>>> stores the snapshot of the log buffer in a local buffer, from which
>>>>> Userspace can pull the logs. By default Driver store, up to, 4
>>>>> snapshots
>>>>> of the log buffer in a local buffer (managed by relay).
>>>>> Added a new module (read only) param, 'guc_log_size', through which
>>>>> User
>>>>> can specify the number of snapshots of log buffer to be stored in
>>>>> local
>>>>> buffer. This can be used to ensure capturing of all boot time logs
>>>>> even
>>>>> with high verbosity level.
>>>>>
>>>>> v2: Rename module param to more apt name 'guc_log_buffer_nr'. (Nikula)
>>>>>
>>>>> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>>> Signed-off-by: Akash Goel <akash.goel at intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
>>>>>   drivers/gpu/drm/i915/i915_params.c         | 5 +++++
>>>>>   drivers/gpu/drm/i915/i915_params.h         | 1 +
>>>>>   3 files changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> index 2e3b723..009d7c0 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>>>>> @@ -1046,8 +1046,7 @@ static int guc_create_log_relay_file(struct
>>>>> intel_guc *guc)
>>>>>
>>>>>       /* Keep the size of sub buffers same as shared log buffer */
>>>>>       subbuf_size = guc->log.obj->base.size;
>>>>> -    /* TODO: Decide based on the User's input */
>>>>> -    n_subbufs = 4;
>>>>> +    n_subbufs = i915.guc_log_buffer_nr;
>>>>>
>>>>>       guc_log_relay_chan = relay_open("guc_log", log_dir,
>>>>>               subbuf_size, n_subbufs, &relay_callbacks, dev);
>>>>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>>>>> b/drivers/gpu/drm/i915/i915_params.c
>>>>> index 8b13bfa..d30c972 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>>>> @@ -57,6 +57,7 @@ struct i915_params i915 __read_mostly = {
>>>>>       .enable_guc_loading = -1,
>>>>>       .enable_guc_submission = -1,
>>>>>       .guc_log_level = -1,
>>>>> +    .guc_log_buffer_nr = 4,
>>>>>       .enable_dp_mst = true,
>>>>>       .inject_load_failure = 0,
>>>>>       .enable_dpcd_backlight = false,
>>>>> @@ -214,6 +215,10 @@ module_param_named(guc_log_level,
>>>>> i915.guc_log_level, int, 0400);
>>>>>   MODULE_PARM_DESC(guc_log_level,
>>>>>       "GuC firmware logging level (-1:disabled (default),
>>>>> 0-3:enabled)");
>>>>>
>>>>> +module_param_named(guc_log_buffer_nr, i915.guc_log_buffer_nr, int,
>>>>> 0400);
>>>>> +MODULE_PARM_DESC(guc_log_buffer_nr,
>>>>> +    "Number of sub buffers to store GuC firmware logs (default: 4)");
>>>>> +
>>>>>   module_param_named_unsafe(enable_dp_mst, i915.enable_dp_mst, bool,
>>>>> 0600);
>>>>>   MODULE_PARM_DESC(enable_dp_mst,
>>>>>       "Enable multi-stream transport (MST) for new DisplayPort sinks.
>>>>> (default: true)");
>>>>> diff --git a/drivers/gpu/drm/i915/i915_params.h
>>>>> b/drivers/gpu/drm/i915/i915_params.h
>>>>> index 0ad020b..14ca855 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_params.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_params.h
>>>>> @@ -48,6 +48,7 @@ struct i915_params {
>>>>>       int enable_guc_loading;
>>>>>       int enable_guc_submission;
>>>>>       int guc_log_level;
>>>>> +    int guc_log_buffer_nr;
>>>>>       int use_mmio_flip;
>>>>>       int mmio_debug;
>>>>>       int edp_vswing;
>>>>>
>>>>
>>>> I did not figure out after a quick read of
>>>> Documentation/filesystems/relay.txt whether we really need this to be
>>>> configurable?
>>>>
>>>> If I got it right number of sub-buffers here only has a relation to the
>>>> userspace relay consumer latency. If the userspace is responsive should
>>>> just two be enough? Or the existing default of four was shown in
>>>> practice that it is better and good enough?
>>>>
>>> Yes one of the use of this module parameter is to give User some leeway
>>> i.e. more time to collect logs from the relay buffer. User may not be
>>> always able to match the rate at which logs are being produced from the
>>> GuC side.
>>>
>>> 2 could be too less.
>>> Even 4, when running a benchmark, was proving less and not able to match
>>> the Driver rate (this might change after some optimization is done from
>>> User space side also, like splice).
>>
>> Okay, it makes sense for it to be bigger than four by default then,
>> correct?
>>
>>> The other use is to ensure capturing of all boot time logs, even with
>>> maximum verbosity level. The default number of sub buffers may not
>>> always be sufficient to store all the logs from boot, by the time User
>>> is ready to capture the logs.
>>> Saw about 8 flush interrupts coming from GuC during the boot.
>>
>> How important it is for a default value to capture all activity since
>> boot?
>>
>> I think we need to keep in mind here that amount of that activity may be
>> a lot different with different setups so it might not be that
>> interesting after all.
>>
>> Someone will log in via a display manager, which may generate a widely
>> differing amount of GPU activity, until they start the logger. Someone
>> else on the other hand might be booting to vt only, starting the logger,
>> and only then starting the graphical UI.
>>
>
> Agree, that's why its useful to have a provision for altering the
> size of buffer.

Maybe. :) I have to be critical here since bar for adding new params is 
high. If we can have a default value which works for everyone it would 
be better. Of course if the size is not too big then.

That's why I asked how important is being able to capture everything 
since boot. Is that a real use case and how important it is?

>>>> I am just not sure this is a useful module parameter without some more
>>>> data.
>>>>
>>>> Even if it is needed, as minimum I think the name should reflect
>>>> this is
>>>> about the relay side of things and not the GuC log buffer itself. So
>>>> something like i915.guc_relay_log_subbuf_nr or something.
>>> Fine will use this name.
>>>
>>>> With the matching description of course.
>>>>
>>> Is the current description not apt ?
>>> "Number of sub buffers to store GuC firmware logs (default: 4)");"
>>
>> My thinking is, what will this parameter mostly be used for? I think the
>> default needs to be such that a reasonable implementation of an
>> userspace logger can cope with the flush rate. I am not so sure that
>> capturing boot time activity by default is that critical.
>>
> The default value can be kept as 2 also, as you suggested .

But you said even 4 was not enough to ensure userspace logger can keep up?

> Actually since logging would generally be disabled by default, so if
> User has a need to capture all boot time logs, it will have to anyways
> enable the logging (i915.guc_log_level >= 0) first and so at that time
> it can also chose a suitable i915.guc_relay_log_subbuf_nr value.
> The logging stats from the output of 'i915_guc_info' debugfs can be used
> to know how many sub buffers will be required to capture all boot time
> logs.
>
> Similarly for run time logging, User can tune the value according to the
> benchmarks, as for some of the benchmarks the amount of logs
> generated is comparatively much more.

I don't see why would they need to tune this depending on the benchmark. 
My assumption is they start the logger and then start the benchmark. The 
only important thing it that number of sub-buffers is large enough for 
userspace to cope with the flush rate.

>> Assuming you find a value to satisfy the above, what are the scenarios
>> someone will need/want to tweak it and what description of the tunable
>> would help them understand what it is for?
>>
>> Should we say, in the description, something like "increase this if you
>> want to capture all boot time activity until you can start the logging"
>> and/or "increase this if experiencing logging drops" ?
>>
> Yes this would be much better description, in fact I myself wanted to
> use a similar description initially, but for brevity sake did not use it
> (though sorry could have captured that in commit message).
> Will use this only now.

Cool.

Regards,

Tvrtko