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

Jani Nikula
2016-06-27T13:31:56Z

On Mon, 27 Jun 2016, 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.
>
> 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 fd26a9e..8c0fd83 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -999,8 +999,7 @@ static void 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_size;
>
> 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..14ce0c4 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_size = 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_size, i915.guc_log_size, int, 0400);
> +MODULE_PARM_DESC(guc_log_size,
> + "Number of sub buffers to store GuC firmware logs (default: 4)");
> +

I guess my battle against adding all sorts of module parameters all the
time is a futile and lost one. :(

Please at least make it clear what the unit of the size is. It's not
obvious to me, and I shouldn't have to look at the source for that.

BR,
Jani.

> 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..89fa832 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_size;
> int use_mmio_flip;
> int mmio_debug;
> int edp_vswing;

--
Jani Nikula, Intel Open Source Technology Center