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

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

On 6/27/2016 7:01 PM, Jani Nikula wrote:
> 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.
>

Sorry for not choosing a suitable name in first place.
I agree the name should be indicative of the unit.
As you would have seen, the parameter provides number of snapshots of 
the Log buffer which can be stored on Driver side.
The size of one snapshot or Log buffer is not so important here and can
change in future.

Please suggest an appropriate name ('guc_log_buffer_nr' ?)

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