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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index f12bbee..f47714c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2332,14 +2332,14 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data)
>  
>  	tmp = I915_READ(GUC_STATUS);
>  
> -	seq_puts(m, "\nResponse from GuC:\n");
> +	seq_printf(m, "\nGuC status 0x%08x:\n", tmp);
>  	seq_printf(m, "\tBootrom status = 0x%x\n",
>  		(tmp & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT);
>  	seq_printf(m, "\tuKernel status = 0x%x\n",
>  		(tmp & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT);
>  	seq_printf(m, "\tMIA Core status = 0x%x\n",
>  		(tmp & GS_MIA_MASK) >> GS_MIA_SHIFT);
> -	seq_puts(m, "Scratch registers value:\n");
> +	seq_puts(m, "\nScratch registers value:\n");
>  	for (i = 0; i < 16; i++)
>  		seq_printf(m, "\t%2d: \t0x%x\n", i, I915_READ(SOFT_SCRATCH(i)));
>  
> @@ -2352,13 +2352,11 @@ static int i915_guc_info(struct seq_file *m, void *data)
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_guc guc;
> -	struct i915_guc_client client;
> +	struct i915_guc_client client = { 0 };

This line gives a warning, because the preceding patch [12/14],
added a new first member which is not a scalar, so can't be initialised
with a zero.

> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 892f974..f8065cf 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -31,6 +31,7 @@
>  #define GUC_WQ_SIZE	(PAGE_SIZE * 2)
>  
>  struct i915_guc_client {
> +	spinlock_t wq_lock;
>  	struct drm_i915_gem_object *client_obj;
>  	u32 priority;
>  	off_t doorbell_offset;

GCC allows '{}' as a set-everything-to-default initialiser, but kernel
coding standards may not. Or, we could put in an explicit member name, i.e.

+	struct i915_guc_client client = { .client_obj = 0 };

But in any case I was thinking of reordering the i915_guc_client
structure members for better packing, and/or better logical grouping
and/or ordering. So the client_obj member may well end up at the top,
and then the struct initialisation will work as-is :)

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