[PATCH V1] accel/amdxdna: Support submit commands without arguments

Lizhi Hou posted 1 patch 9 months ago
drivers/accel/amdxdna/amdxdna_ctx.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
[PATCH V1] accel/amdxdna: Support submit commands without arguments
Posted by Lizhi Hou 9 months ago
The latest userspace runtime allows generating commands which do not
have any argument. Remove the corresponding check in driver IOCTL to
enable this use case.

Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
---
 drivers/accel/amdxdna/amdxdna_ctx.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
index 43442b9e273b..be073224bd69 100644
--- a/drivers/accel/amdxdna/amdxdna_ctx.c
+++ b/drivers/accel/amdxdna/amdxdna_ctx.c
@@ -496,11 +496,11 @@ static int amdxdna_drm_submit_execbuf(struct amdxdna_client *client,
 				      struct amdxdna_drm_exec_cmd *args)
 {
 	struct amdxdna_dev *xdna = client->xdna;
-	u32 *arg_bo_hdls;
+	u32 *arg_bo_hdls = NULL;
 	u32 cmd_bo_hdl;
 	int ret;
 
-	if (!args->arg_count || args->arg_count > MAX_ARG_COUNT) {
+	if (args->arg_count > MAX_ARG_COUNT) {
 		XDNA_ERR(xdna, "Invalid arg bo count %d", args->arg_count);
 		return -EINVAL;
 	}
@@ -512,14 +512,16 @@ static int amdxdna_drm_submit_execbuf(struct amdxdna_client *client,
 	}
 
 	cmd_bo_hdl = (u32)args->cmd_handles;
-	arg_bo_hdls = kcalloc(args->arg_count, sizeof(u32), GFP_KERNEL);
-	if (!arg_bo_hdls)
-		return -ENOMEM;
-	ret = copy_from_user(arg_bo_hdls, u64_to_user_ptr(args->args),
-			     args->arg_count * sizeof(u32));
-	if (ret) {
-		ret = -EFAULT;
-		goto free_cmd_bo_hdls;
+	if (args->arg_count) {
+		arg_bo_hdls = kcalloc(args->arg_count, sizeof(u32), GFP_KERNEL);
+		if (!arg_bo_hdls)
+			return -ENOMEM;
+		ret = copy_from_user(arg_bo_hdls, u64_to_user_ptr(args->args),
+				     args->arg_count * sizeof(u32));
+		if (ret) {
+			ret = -EFAULT;
+			goto free_cmd_bo_hdls;
+		}
 	}
 
 	ret = amdxdna_cmd_submit(client, cmd_bo_hdl, arg_bo_hdls,
-- 
2.34.1
Re: [PATCH V1] accel/amdxdna: Support submit commands without arguments
Posted by Mario Limonciello 9 months ago
On 5/7/2025 11:15 AM, Lizhi Hou wrote:
> The latest userspace runtime allows generating commands which do not
> have any argument. Remove the corresponding check in driver IOCTL to
> enable this use case.
> 
> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>

Can the userspace handle discovery of the difference?  Or does this need 
any sort of ABI discovery command introduced too?

The code change itself below looks good to me though.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/accel/amdxdna/amdxdna_ctx.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 43442b9e273b..be073224bd69 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -496,11 +496,11 @@ static int amdxdna_drm_submit_execbuf(struct amdxdna_client *client,
>   				      struct amdxdna_drm_exec_cmd *args)
>   {
>   	struct amdxdna_dev *xdna = client->xdna;
> -	u32 *arg_bo_hdls;
> +	u32 *arg_bo_hdls = NULL;
>   	u32 cmd_bo_hdl;
>   	int ret;
>   
> -	if (!args->arg_count || args->arg_count > MAX_ARG_COUNT) {
> +	if (args->arg_count > MAX_ARG_COUNT) {
>   		XDNA_ERR(xdna, "Invalid arg bo count %d", args->arg_count);
>   		return -EINVAL;
>   	}
> @@ -512,14 +512,16 @@ static int amdxdna_drm_submit_execbuf(struct amdxdna_client *client,
>   	}
>   
>   	cmd_bo_hdl = (u32)args->cmd_handles;
> -	arg_bo_hdls = kcalloc(args->arg_count, sizeof(u32), GFP_KERNEL);
> -	if (!arg_bo_hdls)
> -		return -ENOMEM;
> -	ret = copy_from_user(arg_bo_hdls, u64_to_user_ptr(args->args),
> -			     args->arg_count * sizeof(u32));
> -	if (ret) {
> -		ret = -EFAULT;
> -		goto free_cmd_bo_hdls;
> +	if (args->arg_count) {
> +		arg_bo_hdls = kcalloc(args->arg_count, sizeof(u32), GFP_KERNEL);
> +		if (!arg_bo_hdls)
> +			return -ENOMEM;
> +		ret = copy_from_user(arg_bo_hdls, u64_to_user_ptr(args->args),
> +				     args->arg_count * sizeof(u32));
> +		if (ret) {
> +			ret = -EFAULT;
> +			goto free_cmd_bo_hdls;
> +		}
>   	}
>   
>   	ret = amdxdna_cmd_submit(client, cmd_bo_hdl, arg_bo_hdls,
Re: [PATCH V1] accel/amdxdna: Support submit commands without arguments
Posted by Lizhi Hou 9 months ago
On 5/7/25 09:29, Mario Limonciello wrote:
> On 5/7/2025 11:15 AM, Lizhi Hou wrote:
>> The latest userspace runtime allows generating commands which do not
>> have any argument. Remove the corresponding check in driver IOCTL to
>> enable this use case.
>>
>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>
> Can the userspace handle discovery of the difference?  Or does this 
> need any sort of ABI discovery command introduced too?
>
> The code change itself below looks good to me though.

Runtime will capture and handle the error with current driver.

Even if runtime have a way to explicitly discovery the different, it 
will just report an error and fail the case.


Thanks,

Lizhi

>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>
>> ---
>>   drivers/accel/amdxdna/amdxdna_ctx.c | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c 
>> b/drivers/accel/amdxdna/amdxdna_ctx.c
>> index 43442b9e273b..be073224bd69 100644
>> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
>> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
>> @@ -496,11 +496,11 @@ static int amdxdna_drm_submit_execbuf(struct 
>> amdxdna_client *client,
>>                         struct amdxdna_drm_exec_cmd *args)
>>   {
>>       struct amdxdna_dev *xdna = client->xdna;
>> -    u32 *arg_bo_hdls;
>> +    u32 *arg_bo_hdls = NULL;
>>       u32 cmd_bo_hdl;
>>       int ret;
>>   -    if (!args->arg_count || args->arg_count > MAX_ARG_COUNT) {
>> +    if (args->arg_count > MAX_ARG_COUNT) {
>>           XDNA_ERR(xdna, "Invalid arg bo count %d", args->arg_count);
>>           return -EINVAL;
>>       }
>> @@ -512,14 +512,16 @@ static int amdxdna_drm_submit_execbuf(struct 
>> amdxdna_client *client,
>>       }
>>         cmd_bo_hdl = (u32)args->cmd_handles;
>> -    arg_bo_hdls = kcalloc(args->arg_count, sizeof(u32), GFP_KERNEL);
>> -    if (!arg_bo_hdls)
>> -        return -ENOMEM;
>> -    ret = copy_from_user(arg_bo_hdls, u64_to_user_ptr(args->args),
>> -                 args->arg_count * sizeof(u32));
>> -    if (ret) {
>> -        ret = -EFAULT;
>> -        goto free_cmd_bo_hdls;
>> +    if (args->arg_count) {
>> +        arg_bo_hdls = kcalloc(args->arg_count, sizeof(u32), 
>> GFP_KERNEL);
>> +        if (!arg_bo_hdls)
>> +            return -ENOMEM;
>> +        ret = copy_from_user(arg_bo_hdls, u64_to_user_ptr(args->args),
>> +                     args->arg_count * sizeof(u32));
>> +        if (ret) {
>> +            ret = -EFAULT;
>> +            goto free_cmd_bo_hdls;
>> +        }
>>       }
>>         ret = amdxdna_cmd_submit(client, cmd_bo_hdl, arg_bo_hdls,
>
Re: [PATCH V1] accel/amdxdna: Support submit commands without arguments
Posted by Lizhi Hou 9 months ago
Merged to drm-misc-next

On 5/7/25 10:31, Lizhi Hou wrote:
>
> On 5/7/25 09:29, Mario Limonciello wrote:
>> On 5/7/2025 11:15 AM, Lizhi Hou wrote:
>>> The latest userspace runtime allows generating commands which do not
>>> have any argument. Remove the corresponding check in driver IOCTL to
>>> enable this use case.
>>>
>>> Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
>>
>> Can the userspace handle discovery of the difference?  Or does this 
>> need any sort of ABI discovery command introduced too?
>>
>> The code change itself below looks good to me though.
>
> Runtime will capture and handle the error with current driver.
>
> Even if runtime have a way to explicitly discovery the different, it 
> will just report an error and fail the case.
>
>
> Thanks,
>
> Lizhi
>
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>>> ---
>>>   drivers/accel/amdxdna/amdxdna_ctx.c | 22 ++++++++++++----------
>>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c 
>>> b/drivers/accel/amdxdna/amdxdna_ctx.c
>>> index 43442b9e273b..be073224bd69 100644
>>> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
>>> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
>>> @@ -496,11 +496,11 @@ static int amdxdna_drm_submit_execbuf(struct 
>>> amdxdna_client *client,
>>>                         struct amdxdna_drm_exec_cmd *args)
>>>   {
>>>       struct amdxdna_dev *xdna = client->xdna;
>>> -    u32 *arg_bo_hdls;
>>> +    u32 *arg_bo_hdls = NULL;
>>>       u32 cmd_bo_hdl;
>>>       int ret;
>>>   -    if (!args->arg_count || args->arg_count > MAX_ARG_COUNT) {
>>> +    if (args->arg_count > MAX_ARG_COUNT) {
>>>           XDNA_ERR(xdna, "Invalid arg bo count %d", args->arg_count);
>>>           return -EINVAL;
>>>       }
>>> @@ -512,14 +512,16 @@ static int amdxdna_drm_submit_execbuf(struct 
>>> amdxdna_client *client,
>>>       }
>>>         cmd_bo_hdl = (u32)args->cmd_handles;
>>> -    arg_bo_hdls = kcalloc(args->arg_count, sizeof(u32), GFP_KERNEL);
>>> -    if (!arg_bo_hdls)
>>> -        return -ENOMEM;
>>> -    ret = copy_from_user(arg_bo_hdls, u64_to_user_ptr(args->args),
>>> -                 args->arg_count * sizeof(u32));
>>> -    if (ret) {
>>> -        ret = -EFAULT;
>>> -        goto free_cmd_bo_hdls;
>>> +    if (args->arg_count) {
>>> +        arg_bo_hdls = kcalloc(args->arg_count, sizeof(u32), 
>>> GFP_KERNEL);
>>> +        if (!arg_bo_hdls)
>>> +            return -ENOMEM;
>>> +        ret = copy_from_user(arg_bo_hdls, u64_to_user_ptr(args->args),
>>> +                     args->arg_count * sizeof(u32));
>>> +        if (ret) {
>>> +            ret = -EFAULT;
>>> +            goto free_cmd_bo_hdls;
>>> +        }
>>>       }
>>>         ret = amdxdna_cmd_submit(client, cmd_bo_hdl, arg_bo_hdls,
>>