Re: [PATCH V4 13/19] accel/amdxdna: Prepare for AIE4 command submission

[Lizhi Hou <lizhi.hou@amd.com>]

On 10/9/26 15:53, David Zhang wrote:
> Prepare the data structures and completion wait helpers required for
> AIE4 command submission:
> - Define struct amdxdna_cmd_start_dpu in amdxdna_ctx.h for the
>    ERT_START_DPU payload.
> - Extend union amdxdna_job_priv with an aie4 member for queue list
>    linkage and job state tracking.
> - Implement smp_rmb() ordering and non-sleeping retry in
>    get_read_index(), returning zero (no completion) when the sample
>    is still invalid after the retry.
> - Add cmd_done_or_unlinked() to wake on completion or on asynchronous
>    device disconnect and reset, detected via check_cert_comp_linked(),
>    and record completion, so aie4_cmd_wait() does not re-read state
>    that can change after the wait.
>
> read_index is written by firmware to host memory, so the wait path
> handles these cases:
> - A torn or invalid read_index is never reported as a completion.
> - -EAGAIN is returned only when the context is disconnected before
>    completion is seen; the command state and fence are not affected.
> - Completion is recorded inside the wait condition, so a context
>    unlinked and relinked during the wait cannot report a false
>    completion.
>
> Co-developed-by: Max Zhen <max.zhen@amd.com>
> Signed-off-by: Max Zhen <max.zhen@amd.com>
> Co-developed-by: Wendy Liang <wendy.liang@amd.com>
> Signed-off-by: Wendy Liang <wendy.liang@amd.com>
> Signed-off-by: David Zhang <yidong.zhang@amd.com>
> ---
>   drivers/accel/amdxdna/aie4_ctx.c    | 73 ++++++++++++++++++++---------
>   drivers/accel/amdxdna/amdxdna_ctx.h | 20 ++++++++
>   2 files changed, 71 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/aie4_ctx.c b/drivers/accel/amdxdna/aie4_ctx.c
> index 767ba6ecbfc6..bb126fbae10d 100644
> --- a/drivers/accel/amdxdna/aie4_ctx.c
> +++ b/drivers/accel/amdxdna/aie4_ctx.c
> @@ -399,56 +399,85 @@ static inline bool valid_queue_index(u64 read, u64 write, u32 capacity)
>   
>   static u64 get_read_index(struct amdxdna_hwctx *hwctx)
>   {
> -	u64 wi = READ_ONCE(*hwctx->priv->umq_write_index);
> -	u64 ri = READ_ONCE(*hwctx->priv->umq_read_index);
> +	struct amdxdna_hwctx_priv *priv = hwctx->priv;
>   	struct amdxdna_dev *xdna = hwctx->client->xdna;
> +	u64 ri, wi;
> +
> +	/* Sample read_index before write_index to guarantee wi >= ri. */
> +	ri = READ_ONCE(*priv->umq_read_index);
> +	/* Order the read_index sample before the write_index sample. */
> +	smp_rmb();
> +	wi = READ_ONCE(priv->write_index);
>   
> -	/*
> -	 * CERT cannot update read index as uint64 atomically. Driver may read
> -	 * half-updated read index when it has bits in high 32bit. In case read
> -	 * index is not valid, wait for some time and retry once. It should
> -	 * allow CERT to complete the read index update.
> -	 */
> +	/* Non-atomic 64-bit counter update by CERT; re-sample once if invalid. */
>   	if (!valid_queue_index(ri, wi, CTX_MAX_CMDS)) {
> -		XDNA_WARN(xdna, "Invalid index, ri %llu, wi %llu", ri, wi);
> -		usleep_range(100, 200);
> -		ri = READ_ONCE(*hwctx->priv->umq_read_index);
> +		ri = READ_ONCE(*priv->umq_read_index);
> +		/* Order the read_index sample before the write_index sample. */
> +		smp_rmb();
> +		wi = READ_ONCE(priv->write_index);
>   		if (!valid_queue_index(ri, wi, CTX_MAX_CMDS)) {
> -			XDNA_ERR(xdna, "Invalid index after retry, ri %llu, wi %llu", ri, wi);
> -			ri = 0;
> +			/* 0 <= any seq: callers always treat it as "not done" and retry. */
> +			XDNA_DBG(xdna, "Invalid index, ri %llu, wi %llu", ri, wi);
> +			return 0;
>   		}
>   	}
>   
>   	return ri;
>   }
>   
> -static inline bool check_cmd_done(struct amdxdna_hwctx *hwctx, u64 seq)
> +/* Verify cert_comp remains linked to detect disconnect or reset. */
> +static bool check_cert_comp_linked(struct amdxdna_hwctx *hwctx, struct cert_comp *comp)
>   {
> -	u64 read_idx = get_read_index(hwctx);
> +	/* READ_ONCE pairs with the link/unlink WRITE_ONCE. */
> +	return comp == READ_ONCE(hwctx->priv->cert_comp);
> +}
>   
> -	return read_idx > seq;
> +/*
> + * Lockless wait condition: wake on completion or disconnect. Record
> + * completion, so callers do not re-read state that can change after the
> + * wait.
> + */
> +static inline bool cmd_done_or_unlinked(struct amdxdna_hwctx *hwctx, u64 seq,
> +					struct cert_comp *comp, bool *done)
> +{
> +	*done = get_read_index(hwctx) > seq;
> +	return *done || !check_cert_comp_linked(hwctx, comp);
>   }
>   
> +/*
> + * Return 0 once seq completed, or -EAGAIN if the context is disconnected
> + * before seq completed.
> + */
>   int aie4_cmd_wait(struct amdxdna_hwctx *hwctx, u64 seq, u32 timeout)
>   {
>   	unsigned long wait_jifs = MAX_SCHEDULE_TIMEOUT;
>   	struct cert_comp *cert_comp = aie4_get_cert_comp(hwctx);
> +	bool done = false;
>   	long ret;
>   
> -	if (!cert_comp)
> +	if (!cert_comp) {
> +		/* Not connected: read_index in host memory is still valid. */
> +		if (get_read_index(hwctx) > seq)
> +			return 0;
>   		return -EAGAIN;
> +	}
>   
>   	if (timeout)
>   		wait_jifs = msecs_to_jiffies(timeout);
>   
>   	ret = wait_event_interruptible_timeout(cert_comp->waitq,
> -					       (check_cmd_done(hwctx, seq)),
> +					       cmd_done_or_unlinked(hwctx, seq, cert_comp,
> +								    &done),
>   					       wait_jifs);
> +	aie4_put_cert_comp(cert_comp);
>   
> +	if (ret < 0)
> +		return ret;
> +	if (done)
> +		return 0;
>   	if (!ret)
> -		ret = -ETIME;
> -
> -	aie4_put_cert_comp(cert_comp);
> +		return -ETIME;
>   
> -	return ret <= 0 ? ret : 0;
> +	/* Woken by disconnect before seq completed. */
> +	return -EAGAIN;
>   }
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.h b/drivers/accel/amdxdna/amdxdna_ctx.h
> index 9bbc3db4ebde..b3677851d1c5 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.h
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.h
> @@ -48,6 +48,18 @@ struct amdxdna_cmd_start_npu {
>   	u32 prop_args[];  /* properties and regular kernel arguments */
>   };
>   
> +/*
> + * struct amdxdna_cmd_start_dpu - interpretation of data payload for
> + * ERT_START_DPU in amdxdna_cmd.
> + */
> +struct amdxdna_cmd_start_dpu {
> +	u64 dtrace_buffer;		/* dtrace buffer address 2 words */
> +	u64 instruction_buffer;		/* buffer address 2 words */
> +	u32 instruction_buffer_size;	/* size of buffer in bytes */
> +	u16 uc_index;			/* microblaze controller index */
> +	u16 chained;			/* number of following amdxdna_cmd_start_dpu elements */
> +};
> +
>   /*
>    * Interpretation of the beginning of data payload for ERT_CMD_CHAIN in
>    * amdxdna_cmd. The rest of the payload in amdxdna_cmd is cmd BO handles.
> @@ -138,8 +150,14 @@ struct amdxdna_drv_cmd {
>   };
>   
>   struct app_health_report;
> +
>   union amdxdna_job_priv {
>   	struct app_health_report *aie2_health;
> +	/* aie4 kernel submission: queue linkage + job state */
> +	struct {
> +		struct list_head	list;
> +		u32			state;
> +	} aie4;
>   };
>   
>   struct amdxdna_sched_job {
> @@ -162,6 +180,8 @@ struct amdxdna_sched_job {
>   };
>   
>   #define aie2_job_health priv.aie2_health
> +#define aie4_job_list	priv.aie4.list
> +#define aie4_job_state	priv.aie4.state
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>   
>   static inline u32
>   amdxdna_cmd_get_op(struct amdxdna_gem_obj *abo)