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

["Goel, Akash" <akash.goel@intel.com>]

On 7/18/2016 6:36 PM, Tvrtko Ursulin wrote:
>
> 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@intel.com wrote:
>>>>>> From: Akash Goel <akash.goel@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@chris-wilson.co.uk>
>>>>>> Signed-off-by: Akash Goel <akash.goel@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.
>
Sorry that's what I unable to say right now, a default value which can 
cater to most workloads and is not too high also.

> 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?
>
It might be needed in certain cases, probably while enabling new platforms.

>>>>> 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?
>

Yes 4 doesn't seems to be enough for runtime logging with certain 
benchmarks at least.

>> 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.
>

Yes if a reasonable default value can be figured out, which can cover 
most workloads, then no need of this parameter.

>>> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx