Re: [Intel-gfx] [Intel-gfx 4/6] drm/i915/guc: Provide debugfs for log relay sub-buf info

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 09 May 2022 14:01:49 -0700, Alan Previn wrote:
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index f454d53a8bca..35709202b09c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -15,7 +15,7 @@
>
>  static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log);
>
> -static u32 intel_guc_log_size(struct intel_guc_log *log)
> +u32 intel_guc_log_size(struct intel_guc_log *log)
>  {
>	/*
>	 *  GuC Log buffer Layout:
> @@ -41,6 +41,12 @@ static u32 intel_guc_log_size(struct intel_guc_log *log)
>	return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE + CAPTURE_BUFFER_SIZE;
>  }
>
> +#define GUC_LOG_RELAY_SUBBUF_COUNT 8
> +u32 intel_guc_log_relay_subbuf_count(struct intel_guc_log *log)
> +{
> +	return GUC_LOG_RELAY_SUBBUF_COUNT;
> +}

uapi wise, instead of exposing guc_log_size and subbuf_count, why not
expose subbuf_size and subbuf_count?

> +static int guc_log_relay_buf_size_get(void *data, u64 *val)
> +{
> +	struct intel_guc_log *log = data;
> +
> +	if (!log)
> +		return -ENODEV;
> +	if (!log->vma)
> +		return -ENODEV;

Why are these checks needed? Hasn't log been initialized and attached at
intel_gt_debugfs_register_files()/debugfs_create_file() time itself?

Also if needed let's do:

	if (!log || !log->vma)
		return -ENODEV;

> +
> +	*val = (u64)intel_guc_log_size(log);
> +
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(guc_log_relay_buf_size_fops,
> +			guc_log_relay_buf_size_get, NULL,
> +			"%lld\n");
> +
> +static int guc_log_relay_subbuf_count_get(void *data, u64 *val)
> +{
> +	struct intel_guc_log *log = data;
> +
> +	if (!log)
> +		return -ENODEV;
> +	if (!log->vma)
> +		return -ENODEV;

Same issue as above.

> +
> +	*val = intel_guc_log_relay_subbuf_count(log);
> +
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(guc_log_relay_subbuf_count_fops,
> +			guc_log_relay_subbuf_count_get, NULL,
> +			"%lld\n");
> +
>  static int guc_log_relay_open(struct inode *inode, struct file *file)
>  {
>	struct intel_guc_log *log = inode->i_private;
>
> +	if (!log)
> +		return -ENODEV;
> +

Same issue as above.

>	if (!intel_guc_is_ready(log_to_guc(log)))
>		return -ENODEV;
>
> @@ -166,6 +205,8 @@ void intel_guc_log_debugfs_register(struct intel_guc_log *log,
>		{ "guc_load_err_log_dump", &guc_load_err_log_dump_fops, NULL },
>		{ "guc_log_level", &guc_log_level_fops, NULL },
>		{ "guc_log_relay", &guc_log_relay_fops, NULL },
> +		{ "guc_log_relay_buf_size", &guc_log_relay_buf_size_fops, NULL },
> +		{ "guc_log_relay_subbuf_count", &guc_log_relay_subbuf_count_fops, NULL },
>	};
>
>	if (!intel_guc_is_supported(log_to_guc(log)))
> --
> 2.25.1
>