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

Chris Wilson
2015-06-15T20:20:41Z

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