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

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. With the 
matching description of course.

Regards,

Tvrtko