Re: [Intel-gfx] [Intel-gfx 3/6] drm/i915/guc: Add a helper for log buffer size

From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
To: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [Intel-gfx 3/6] drm/i915/guc: Add a helper for log buffer size
Date: Tue, 19 Jul 2022 19:49:47 -0700	[thread overview]
Message-ID: <87mtd4bir8.wl-ashutosh.dixit@intel.com> (raw)
In-Reply-To: <20220509210151.1843173-4-alan.previn.teres.alexis@intel.com>

On Mon, 09 May 2022 14:01:48 -0700, Alan Previn wrote:
>
> Add a helper to get GuC log buffer size.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 49 ++++++++++++----------
>  1 file changed, 27 insertions(+), 22 deletions(-)
>
> 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 d902b40ded0e..f454d53a8bca 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -15,6 +15,32 @@
>
>  static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log);
>
> +static u32 intel_guc_log_size(struct intel_guc_log *log)
> +{
> +	/*
> +	 *  GuC Log buffer Layout:
> +	 *
> +	 *  NB: Ordering must follow "enum guc_log_buffer_type".
> +	 *
> +	 *  +===============================+ 00B
> +	 *  |      Debug state header       |
> +	 *  +-------------------------------+ 32B
> +	 *  |    Crash dump state header    |
> +	 *  +-------------------------------+ 64B
> +	 *  |     Capture state header      |
> +	 *  +-------------------------------+ 96B
> +	 *  |                               |
> +	 *  +===============================+ PAGE_SIZE (4KB)
> +	 *  |          Debug logs           |
> +	 *  +===============================+ + DEBUG_SIZE
> +	 *  |        Crash Dump logs        |
> +	 *  +===============================+ + CRASH_SIZE
> +	 *  |         Capture logs          |
> +	 *  +===============================+ + CAPTURE_SIZE
> +	 */
> +	return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE + CAPTURE_BUFFER_SIZE;
> +}
> +
>  /**
>   * DOC: GuC firmware log
>   *
> @@ -464,32 +490,11 @@ int intel_guc_log_create(struct intel_guc_log *log)
>
>	GEM_BUG_ON(log->vma);
>
> -	/*
> -	 *  GuC Log buffer Layout
> -	 * (this ordering must follow "enum guc_log_buffer_type" definition)
> -	 *
> -	 *  +===============================+ 00B
> -	 *  |      Debug state header       |
> -	 *  +-------------------------------+ 32B
> -	 *  |    Crash dump state header    |
> -	 *  +-------------------------------+ 64B
> -	 *  |     Capture state header      |
> -	 *  +-------------------------------+ 96B
> -	 *  |                               |
> -	 *  +===============================+ PAGE_SIZE (4KB)
> -	 *  |          Debug logs           |
> -	 *  +===============================+ + DEBUG_SIZE
> -	 *  |        Crash Dump logs        |
> -	 *  +===============================+ + CRASH_SIZE
> -	 *  |         Capture logs          |
> -	 *  +===============================+ + CAPTURE_SIZE
> -	 */
>	if (intel_guc_capture_output_min_size_est(guc) > CAPTURE_BUFFER_SIZE)
>		DRM_WARN("GuC log buffer for state_capture maybe too small. %d < %d\n",
>			 CAPTURE_BUFFER_SIZE, intel_guc_capture_output_min_size_est(guc));
>
> -	guc_log_size = PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE +
> -		       CAPTURE_BUFFER_SIZE;
> +	guc_log_size = intel_guc_log_size(log);
>
>	vma = intel_guc_allocate_vma(guc, guc_log_size);
>	if (IS_ERR(vma)) {

My nit-pick suggestions are:

* Call the static function guc_log_size() (don't append intel_ prefix for
  static/internal functions)
* Eliminate the guc_log_size variable and just do
* vma = intel_guc_allocate_vma(guc, guc_log_size());

Otherwise this is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>