[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f7575353-760c-0f6d-5569-cd7f691f6af9@quicinc.com>
Date: Fri, 9 Aug 2024 10:32:36 -0600
From: Jeffrey Hugo <quic_jhugo@...cinc.com>
To: Lizhi Hou <lizhi.hou@....com>, <ogabbay@...nel.org>,
<dri-devel@...ts.freedesktop.org>
CC: <linux-kernel@...r.kernel.org>, <min.ma@....com>, <max.zhen@....com>,
<sonal.santan@....com>, <king.tam@....com>,
George Yang <George.Yang@....com>
Subject: Re: [PATCH V2 02/10] accel/amdxdna: Support hardware mailbox
On 8/5/2024 11:39 AM, Lizhi Hou wrote:
> +enum aie2_msg_status {
> + AIE2_STATUS_SUCCESS = 0x0,
> + /* AIE Error codes */
> + AIE2_STATUS_AIE_SATURATION_ERROR = 0x1000001,
> + AIE2_STATUS_AIE_FP_ERROR = 0x1000002,
> + AIE2_STATUS_AIE_STREAM_ERROR = 0x1000003,
> + AIE2_STATUS_AIE_ACCESS_ERROR = 0x1000004,
> + AIE2_STATUS_AIE_BUS_ERROR = 0x1000005,
> + AIE2_STATUS_AIE_INSTRUCTION_ERROR = 0x1000006,
> + AIE2_STATUS_AIE_ECC_ERROR = 0x1000007,
> + AIE2_STATUS_AIE_LOCK_ERROR = 0x1000008,
> + AIE2_STATUS_AIE_DMA_ERROR = 0x1000009,
> + AIE2_STATUS_AIE_MEM_PARITY_ERROR = 0x100000a,
> + AIE2_STATUS_AIE_PWR_CFG_ERROR = 0x100000b,
> + AIE2_STATUS_AIE_BACKTRACK_ERROR = 0x100000c,
> + AIE2_STATUS_MAX_AIE_STATUS_CODE,
> + /* MGMT ERT Error codes */
> + AIE2_STATUS_MGMT_ERT_SELF_TEST_FAILURE = 0x2000001,
> + AIE2_STATUS_MGMT_ERT_HASH_MISMATCH,
> + AIE2_STATUS_MGMT_ERT_NOAVAIL,
> + AIE2_STATUS_MGMT_ERT_INVALID_PARAM,
> + AIE2_STATUS_MGMT_ERT_ENTER_SUSPEND_FAILURE,
> + AIE2_STATUS_MGMT_ERT_BUSY,
> + AIE2_STATUS_MGMT_ERT_APPLICATION_ACTIVE,
> + MAX_MGMT_ERT_STATUS_CODE,
> + /* APP ERT Error codes */
> + AIE2_STATUS_APP_ERT_FIRST_ERROR = 0x3000001,
> + AIE2_STATUS_APP_INVALID_INSTR,
> + AIE2_STATUS_APP_LOAD_PDI_FAIL,
> + MAX_APP_ERT_STATUS_CODE,
> + /* NPU RTOS Error Codes */
> + AIE2_STATUS_INVALID_INPUT_BUFFER = 0x4000001,
> + AIE2_STATUS_INVALID_COMMAND,
> + AIE2_STATUS_INVALID_PARAM,
> + AIE2_STATUS_INVALID_OPERATION = 0x4000006,
Looks like your alignment is off here
> + AIE2_STATUS_ASYNC_EVENT_MSGS_FULL,
> + AIE2_STATUS_MAX_RTOS_STATUS_CODE,
> + MAX_AIE2_STATUS_CODE
> +};
> +
> +struct assign_mgmt_pasid_req {
> + u16 pasid;
> + u16 reserved;
> +} __packed;
> +
> +struct assign_mgmt_pasid_resp {
> + enum aie2_msg_status status;
> +} __packed;
> +
> +struct map_host_buffer_req {
> + u32 context_id;
> + u64 buf_addr;
> + u64 buf_size;
> +} __packed;
You define a bunch of structures, but don't use them. Seems like a lot
of dead code to me.
Hard to say since you are not using these, but I'm guessing these are
all the message structs to the device (fw). They should be using __
types, like __u64, since the messages are crossing boundaries.
> +#define MAX_CHAIN_CMDBUF_SIZE 0x1000
SZ_ macro please (here and a few other places)
> +
> +struct xdna_msg_header {
> + u32 total_size;
> + u32 size : 11;
> + u32 rsvd0 : 5;
> + u32 protocol_version : 8;
> + u32 rsvd1 : 8;
This bitwise syntax is a really bad idea because it depends on compiler
behavior. You should use FIELD_PREP
> + u32 id;
> + u32 opcode;
> +} __packed;
> +
> +static_assert(sizeof(struct xdna_msg_header) == 16);
> +
> +struct mailbox_pkg {
> + struct xdna_msg_header header;
> + u32 payload[];
> +};
> +
> +/* The protocol version. */
> +#define MSG_PROTOCOL_VERSION 0x1
> +/* The tombstone value. */
> +#define TOMBSTONE 0xDEADFACE
> +
> +struct mailbox_msg {
> + void *handle;
> + int (*notify_cb)(void *handle, const u32 *data, size_t size);
> + size_t pkg_size; /* package size in bytes */
> + struct mailbox_pkg pkg;
> +};
> +
> +static void mailbox_reg_write(struct mailbox_channel *mb_chann, u32 mbox_reg, u32 data)
> +{
> + struct xdna_mailbox_res *mb_res = &mb_chann->mb->res;
> + u64 ringbuf_addr = mb_res->mbox_base + mbox_reg;
> +
> + iowrite32(data, (void *)ringbuf_addr);
Why iowrite32() over writel()?
> +static int mailbox_acquire_msgid(struct mailbox_channel *mb_chann, struct mailbox_msg *mb_msg)
> +{
> + unsigned long flags;
> + int msg_id;
> +
> + spin_lock_irqsave(&mb_chann->chan_idr_lock, flags);
> + msg_id = idr_alloc_cyclic(&mb_chann->chan_idr, mb_msg, 0,
> + MAX_MSG_ID_ENTRIES, GFP_NOWAIT);
> + spin_unlock_irqrestore(&mb_chann->chan_idr_lock, flags);
I think an xa struct would be preferred.
> + if (msg_id < 0)
> + return msg_id;
> +
> + /*
> + * The IDR becomes less efficient when dealing with larger IDs.
> + * Thus, add MAGIC_VAL to the higher bits.
> + */
> + msg_id |= MAGIC_VAL;
> + return msg_id;
> +}
> +
Powered by blists - more mailing lists