Re: [PATCH 04/15] drm/i915: Add GuC-related header files

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 04/15] drm/i915: Add GuC-related header files
Date: Mon, 15 Jun 2015 21:20:37 +0100	[thread overview]
Message-ID: <20150615202037.GF28462@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1434393394-21002-5-git-send-email-david.s.gordon@intel.com>

On Mon, Jun 15, 2015 at 07:36:22PM +0100, Dave Gordon wrote:
> From: Alex Dai <yu.dai@intel.com>
> 
> intel_guc_api.h contains the subset of the GuC interface that we
> will need for submission of commands through the GuC. These MUST
> be kept in sync with the definitions used by the GuC firmware.

intel_guc_hw.h or intel_guc_abi.h then. Calling it API doesn't make it
clear whose API you are talking about.

> intel_guc.h defines structures and parameters relevant to loading
> the GuC firmware and setting it running. Some of these also need
> to be kept in sync with the firmware.

intel_guc.h should be developed organically as features are added in the
series so that it is possible to track against implementation. Certainly
not in a patch that adds the entirety of the firmware ABI.

> +struct i915_guc_client {
> +	spinlock_t wq_lock;
> +	struct drm_i915_gem_object *client_obj;
> +	u32 priority;
> +	off_t doorbell_offset;
> +	off_t proc_desc_offset;
> +	off_t wq_offset;
> +	uint16_t doorbell_id;
> +	uint32_t ctx_index;
> +	uint32_t wq_size;
> +	uint32_t wq_tail;
> +	uint32_t cookie;
> +
> +	/* GuC submission statistics & status */
> +	uint64_t submissions;
> +	uint32_t q_fail;
> +	uint32_t b_fail;
> +	int retcode;

Mixture of classic kernel types and stdint types. And off_t! What size
exactly do you mean there?

> +};
> +
> +#define I915_MAX_DOORBELLS	256
> +#define INVALID_DOORBELL_ID	I915_MAX_DOORBELLS
> +
> +#define INVALID_CTX_ID		(MAX_GUC_GPU_CONTEXTS+1)
> +
> +struct intel_guc {
> +	/* Generic uC firmware management */
> +	struct intel_uc_fw guc_fw;

Haven't checked for size, but I guess this is going to be an init only
structure that we could discard.

> +	/* GuC-specific additions */
> +	uint32_t fw_ver_major;
> +	uint32_t fw_ver_minor;

I have no idea why you would want to keep these around.

> +	spinlock_t host2guc_lock;

Seems overly specific, no comment as to what it locks and lack of
implementation to be able to confirm.
> +
> +	struct drm_i915_gem_object *ctx_pool_obj;
> +	struct drm_i915_gem_object *log_obj;
> +	struct i915_guc_client *execbuf_client;

I expect these will want modification based on patches to be reviewed.

> +	struct ida ctx_ids;
> +	uint32_t log_flags;
> +	int db_cacheline;
> +	DECLARE_BITMAP(doorbell_bitmap, I915_MAX_DOORBELLS);
> +
> +	/* Action status & statistics */
> +	uint64_t action_count;		/* Total commands issued	*/
> +	uint32_t action_cmd;		/* Last command word		*/
> +	uint32_t action_status;		/* Last return status		*/
> +	uint32_t action_fail;		/* Total number of failures	*/
> +	int32_t action_err;		/* Last error code		*/

Any group of prefix_ immediately raises the question of "why isn't this
a struct?"
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx