Re: [PATCH 02/11] drm/i915: Add GuC ukernel logging related fields to fw interface file

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 27/06/16 13:16, akash.goel@intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>
> The first page of the GuC log buffer contains state info or meta data
> which is required to parse the logs contained in the subsequent pages.
> The structure representing the state info is added to interface file
> as Driver would need to handle log buffer flush interrupts from GuC.
> Added an enum for the different message/event types that can be send
> by the GuC ukernel to Host.
> Also added 2 new Host to GuC action types to inform GuC when Host has
> flushed the log buffer and forcefuly cause the GuC to send a new
> log buffer flush interrupt.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_fwif.h | 53 +++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
> index 944786d..eb10324 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
> @@ -418,15 +418,62 @@ struct guc_ads {
>   	u32 reserved2[4];
>   } __packed;
>
> +/* GuC logging structures */
> +
> +enum guc_log_buffer_type {
> +	GUC_ISR_LOG_BUFFER,
> +	GUC_DPC_LOG_BUFFER,
> +	GUC_CRASH_DUMP_LOG_BUFFER,
> +	GUC_MAX_LOG_BUFFER
> +};
> +
> +/*
> + * Below state is used for coordination of retrival of GuC logs
> + * by i915. read_ptr is where i915 read last. GuC keeps incrementing
> + * write_ptr on each log entry. When buffer gets half filled and i915
> + * has requested interrupt, GuC will set flush_to_file field and raise the
> + * interrupt. i915 should read the buffer and clear flush_to_file field.
> + * i915 should also update the read_ptr.
> +*/
> +struct guc_log_buffer_state {
> +	u32 marker[2];
> +	u32 read_ptr;
> +	u32 write_ptr;
> +	u32 size;
> +	u32 sampled_write_ptr;
> +	union {
> +		struct {
> +			u32 flush_to_file:1;
> +			u32 buffer_full_cnt:4;
> +			u32 reserved:27;
> +		};
> +		u32 flags;
> +	};
> +	u32 version;
> +} __packed;

Suggest documenting things like the unit of read/write_ptr, size, which 
files are NA/RO/RW for i915, what is sampled_write_ptr, what is the 
marker etc.

> +
> +union guc_log_control {
> +	struct {
> +		u32 logging_enabled:1;
> +		u32 reserved1:3;
> +		u32 verbosity:4;
> +		u32 reserved2:24;
> +	};
> +	u32 value;
> +} __packed;
> +
>   /* This Action will be programmed in C180 - SOFT_SCRATCH_O_REG */
>   enum host2guc_action {
>   	HOST2GUC_ACTION_DEFAULT = 0x0,
>   	HOST2GUC_ACTION_SAMPLE_FORCEWAKE = 0x6,
>   	HOST2GUC_ACTION_ALLOCATE_DOORBELL = 0x10,
>   	HOST2GUC_ACTION_DEALLOCATE_DOORBELL = 0x20,
> +	HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE = 0x30,
> +	HOST2GUC_ACTION_FORCE_LOGBUFFERFLUSH = 0x302,

Not that important but just strikes me it would be better to settle for 
either LOG_BUFFER_FLUSH or LOGBUFFERFLUSH and not use both conventions.

>   	HOST2GUC_ACTION_ENTER_S_STATE = 0x501,
>   	HOST2GUC_ACTION_EXIT_S_STATE = 0x502,
>   	HOST2GUC_ACTION_SLPC_REQUEST = 0x3003,
> +	HOST2GUC_ACTION_UK_LOG_ENABLE_LOGGING = 0x0E000,
>   	HOST2GUC_ACTION_LIMIT
>   };
>
> @@ -448,4 +495,10 @@ enum guc2host_status {
>   	GUC2HOST_STATUS_GENERIC_FAIL = GUC2HOST_STATUS(0x0000F000)
>   };
>
> +/* This action will be programmed in C1BC - SOFT_SCRATCH_15_REG */
> +enum guc2host_message {
> +	GUC2HOST_MSG_CRASH_DUMP_POSTED = (1 << 1),
> +	GUC2HOST_MSG_FLUSH_LOG_BUFFER = (1 << 3)
> +};
> +
>   #endif
>

Regards,

Tvrtko

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