Re: [PATCH v6 13/14] drm/i915: Enable GuC firmware log

[Dave Gordon <david.s.gordon@intel.com>]

On 29/04/15 23:13, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
> 
> Allocate a gem obj to hold GuC log data. Also a debugfs interface
> (i915_guc_log_dump) is provided to print out the log content.
> 
> Issue: VIZ-4884
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     | 41 +++++++++++++++++----
>  drivers/gpu/drm/i915/i915_drv.h         |  1 +
>  drivers/gpu/drm/i915/i915_params.c      |  5 +++
>  drivers/gpu/drm/i915/intel_guc.h        |  1 +
>  drivers/gpu/drm/i915/intel_guc_loader.c | 64 ++++++++++++++++++++++++++++++++-
>  5 files changed, 104 insertions(+), 8 deletions(-)

[snip]

> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 9ad2e27..95e4eb7 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -54,6 +54,7 @@ struct i915_params i915 __read_mostly = {
>  	.verbose_state_checks = 1,
>  	.nuclear_pageflip = 0,
>  	.enable_guc_scheduling = false,
> +	.guc_log_level = 0,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -188,3 +189,7 @@ MODULE_PARM_DESC(nuclear_pageflip,
>  
>  module_param_named(enable_guc_scheduling, i915.enable_guc_scheduling, bool, 0400);
>  MODULE_PARM_DESC(enable_guc_scheduling, "Enable GuC scheduling (default:false)");
> +
> +module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
> +MODULE_PARM_DESC(guc_log_level,
> +	"GuC firmware logging level (0:disabled, 1~4:enabled)");

See below ...

> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index eb25cfb..a61d1df 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -51,6 +51,12 @@
>   * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects
>   * used by GuC is pinned with PIN_OFFSET_BIAS along with size of WOPCM.
>   *
> + * Firmware log:
> + * Firmware log is enabled by setting i915.guc_log_level to non-negative level.

This comment doesn't agree with the one above ...

> +static void create_guc_log(struct intel_guc *guc, u32 *params)
> +{
> +	struct drm_i915_private *dev_priv =
> +			container_of(guc, struct drm_i915_private, guc);
> +	struct drm_i915_gem_object *obj;
> +	u32 flags, size;
> +
> +	/* The first page is to save log buffer state. Allocate one
> +	 * extra page for others in case for overlap */
> +	size = (1 + GUC_LOG_DPC_PAGES + 1 +
> +		GUC_LOG_ISR_PAGES + 1 +
> +		GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
> +
> +	if (!guc->log_obj) {
> +		obj = intel_guc_allocate_gem_obj(dev_priv->dev, size);
> +		if (!obj) {
> +			/* logging will be off */
> +			*(params + GUC_CTL_LOG_PARAMS) = 0;
> +			i915.guc_log_level = 0;
> +			return;
> +		}
> +
> +		guc->log_obj = obj;
> +	}
> +	else
> +		obj = guc->log_obj;
> +
> +	/* each allocated unit is a page */
> +	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> +		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> +		(GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> +		(GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
> +
> +	size = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT; /* in pages */
> +	flags |= size << GUC_LOG_BUF_ADDR_SHIFT;
> +
> +	*(params + GUC_CTL_LOG_PARAMS) = flags;
> +
> +	i915.guc_log_level--;

This code modifies the global parameter, which will result in
(a) weird results if this code can be executed more than once
(b) confusion is the user reads it back and finds that it's
    not the same as given on the command line!

> +	if (i915.guc_log_level > GUC_LOG_VERBOSITY_ULTRA)
> +		i915.guc_log_level = GUC_LOG_VERBOSITY_ULTRA;
> +
> +	*(params + GUC_CTL_DEBUG) |= i915.guc_log_level;

These lines only happen to work because the VERBOSITY_SHIFT is 0.

Given the opportunities for confusion here, I think it would be better
to redefine the module parameter as signed, with -1 => disabled, and 0-3
for the levels of verbosity. This will mean the values used for the
module parameter will be the same as described in the GuC firmware
specification (i.e. verbosity levels of 0-3).

Then, the validation code above can legitimately limit the range to
(negative => off, 0-3 => level, 4+ => clipped to 3), and that will not
surprise the user too much when they read it back (it will be as they
set it, unless it was too large, in which case it shows what the maximum
useful value is).

With the parameter defined this way, we can add #defines for min and max
verbosity (0 and 3, without incorporating the shift left), and then
apply the shift only when setting the host-to-GuC-interface parameter,
rather than worrying about it when dealing with the user parameter.

>  static void set_guc_init_params(struct drm_i915_private *dev_priv)
>  {
>  	u32 params[GUC_CTL_MAX_DWORDS];
> @@ -227,7 +282,9 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
>  	params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
>  			GUC_CTL_VCS2_ENABLED;
>  
> -	/* XXX: Set up log buffer */
> +	/* Set up log buffer */
> +	if (i915.guc_log_level > 0)
> +		create_guc_log(&dev_priv->guc, params);

I would rather this was done during initial setup i.e. in the same place
as allocating the context pool and the execbuf client, rather than in
the middle of setting up the parameters. We can stash the values that
would have been passed back through params[] in the struct guc, so this
code can then just copy then to the parameter block.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx