On Mon, Jun 15, 2015 at 07:36:22PM +0100, Dave Gordon wrote:
> From: Alex Dai <yu.dai@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
