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

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

On 10/25/24 10:55, Jeffrey Hugo wrote:
> 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.

Ok. I will  remove the wait ioctl and use syncobj instead.


Thanks,

Lizhi

>
> -Jeff