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

[Dave Gordon]

On 15/06/15 21:20, Chris Wilson wrote:
> On Mon, Jun 15, 2015 at 07:36:22PM +0100, Dave Gordon wrote:
>> From: Alex Dai <yu.dai at 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.

Done.

>> +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?

All converted to stdint now.

>> +};
>> +
>> +#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.

No, we need it for reloading. It's not very big anyway.

>> +	/* 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.

Firstly, they're used to pass the designation for the version of the f/w
interface that this revision of the driver can work with.

Secondly we want to print them in messages, so I've split these into
(major,minor) that we wanted and (major,minor) that we found. These were
already printed in a NOTICE, but not in the debugfs output.

>> +	spinlock_t host2guc_lock;
> 
> Seems overly specific, no comment as to what it locks and lack of
> implementation to be able to confirm.

Now reorganised so it's adjacent to the protected data :)

>> +	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.

Now added in separate patches.

>> +	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

Not really worth making and naming a struct. There's only one instance
of this whole thing; the code that updates these touches them
individually, and the debugfs code that prints them can't really make
use of them collectively either.

.Dave.