Re: [PATCH V5 07/10] accel/amdxdna: Add command execution

[Jeffrey Hugo <quic_jhugo@quicinc.com>]

On 10/21/2024 10:19 AM, Lizhi Hou wrote:
> diff --git a/include/uapi/drm/amdxdna_accel.h b/include/uapi/drm/amdxdna_accel.h
> index 3792750834b2..08f3ec7146ab 100644
> --- a/include/uapi/drm/amdxdna_accel.h
> +++ b/include/uapi/drm/amdxdna_accel.h
> @@ -13,6 +13,7 @@
>   extern "C" {
>   #endif
>   
> +#define AMDXDNA_INVALID_CMD_HANDLE	(~0UL)
>   #define AMDXDNA_INVALID_ADDR		(~0UL)
>   #define AMDXDNA_INVALID_CTX_HANDLE	0
>   #define AMDXDNA_INVALID_BO_HANDLE	0
> @@ -29,6 +30,8 @@ enum amdxdna_drm_ioctl_id {
>   	DRM_AMDXDNA_CREATE_BO,
>   	DRM_AMDXDNA_GET_BO_INFO,
>   	DRM_AMDXDNA_SYNC_BO,
> +	DRM_AMDXDNA_EXEC_CMD,
> +	DRM_AMDXDNA_WAIT_CMD,
>   };
>   
>   /**
> @@ -201,6 +204,54 @@ struct amdxdna_drm_sync_bo {
>   	__u64 size;
>   };
>   
> +enum amdxdna_cmd_type {
> +	AMDXDNA_CMD_SUBMIT_EXEC_BUF = 0,
> +	AMDXDNA_CMD_SUBMIT_DEPENDENCY,
> +	AMDXDNA_CMD_SUBMIT_SIGNAL,
> +};
> +
> +/**
> + * struct amdxdna_drm_exec_cmd - Execute command.
> + * @ext: MBZ.
> + * @ext_flags: MBZ.
> + * @hwctx: Hardware context handle.
> + * @type: One of command type in enum amdxdna_cmd_type.
> + * @cmd_handles: Array of command handles or the command handle itself
> + *               in case of just one.
> + * @args: Array of arguments for all command handles.
> + * @cmd_count: Number of command handles in the cmd_handles array.
> + * @arg_count: Number of arguments in the args array.
> + * @seq: Returned sequence number for this command.
> + */
> +struct amdxdna_drm_exec_cmd {
> +	__u64 ext;
> +	__u64 ext_flags;
> +	__u32 hwctx;
> +	__u32 type;
> +	__u64 cmd_handles;
> +	__u64 args;
> +	__u32 cmd_count;
> +	__u32 arg_count;
> +	__u64 seq;
> +};
> +
> +/**
> + * struct amdxdna_drm_wait_cmd - Wait exectuion command.
> + *
> + * @hwctx: hardware context handle.
> + * @timeout: timeout in ms, 0 implies infinite wait.
> + * @seq: sequence number of the command returned by execute command.
> + *
> + * Wait a command specified by seq to be completed.
> + * Using AMDXDNA_INVALID_CMD_HANDLE as seq means wait till there is a free slot
> + * to submit a new command.
> + */
> +struct amdxdna_drm_wait_cmd {
> +	__u32 hwctx;
> +	__u32 timeout;
> +	__u64 seq;
> +};
> +
>   #define DRM_IOCTL_AMDXDNA_CREATE_HWCTX \
>   	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDXDNA_CREATE_HWCTX, \
>   		 struct amdxdna_drm_create_hwctx)
> @@ -225,6 +276,14 @@ struct amdxdna_drm_sync_bo {
>   	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDXDNA_SYNC_BO, \
>   		 struct amdxdna_drm_sync_bo)
>   
> +#define DRM_IOCTL_AMDXDNA_EXEC_CMD \
> +	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDXDNA_EXEC_CMD, \
> +		 struct amdxdna_drm_exec_cmd)
> +
> +#define DRM_IOCTL_AMDXDNA_WAIT_CMD \
> +	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDXDNA_WAIT_CMD, \
> +		 struct amdxdna_drm_wait_cmd)
> +

Nope.  This looks like a driver private wait ioctl on a specific BO. 
That is not the modern way to do things per Vetter -

https://lore.kernel.org/dri-devel/ZC75%2Fq34YnDDsGpB@phenom.ffwll.local/

Skimming the implementation, it looks like you are already using fences 
and the drm scheduler, so plumbing in syncobjs is not much more than 
what you are already doing, I think.

-Jeff