[PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Jonathan Marek]

Initializing sensors requires attaching to pd 2. Add an ioctl for that.

This corresponds to FASTRPC_INIT_ATTACH_SENSORS in the downstream driver.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/misc/fastrpc.c      | 9 ++++++---
 include/uapi/misc/fastrpc.h | 5 +++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 7939c55daceb..ea5e9ca0d705 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1276,7 +1276,7 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
 	return 0;
 }
 
-static int fastrpc_init_attach(struct fastrpc_user *fl)
+static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
 {
 	struct fastrpc_invoke_args args[1];
 	int tgid = fl->tgid;
@@ -1287,7 +1287,7 @@ static int fastrpc_init_attach(struct fastrpc_user *fl)
 	args[0].fd = -1;
 	args[0].reserved = 0;
 	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
-	fl->pd = 0;
+	fl->pd = pd;
 
 	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
 				       sc, &args[0]);
@@ -1477,7 +1477,10 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
 		err = fastrpc_invoke(fl, argp);
 		break;
 	case FASTRPC_IOCTL_INIT_ATTACH:
-		err = fastrpc_init_attach(fl);
+		err = fastrpc_init_attach(fl, 0);
+		break;
+	case FASTRPC_IOCTL_INIT_ATTACH_SNS:
+		err = fastrpc_init_attach(fl, 2);
 		break;
 	case FASTRPC_IOCTL_INIT_CREATE:
 		err = fastrpc_init_create_process(fl, argp);
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index 07de2b7aac85..0a89f95463f6 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -10,8 +10,9 @@
 #define FASTRPC_IOCTL_INVOKE		_IOWR('R', 3, struct fastrpc_invoke)
 #define FASTRPC_IOCTL_INIT_ATTACH	_IO('R', 4)
 #define FASTRPC_IOCTL_INIT_CREATE	_IOWR('R', 5, struct fastrpc_init_create)
-#define FASTRPC_IOCTL_MMAP              _IOWR('R', 6, struct fastrpc_req_mmap)
-#define FASTRPC_IOCTL_MUNMAP            _IOWR('R', 7, struct fastrpc_req_munmap)
+#define FASTRPC_IOCTL_MMAP		_IOWR('R', 6, struct fastrpc_req_mmap)
+#define FASTRPC_IOCTL_MUNMAP		_IOWR('R', 7, struct fastrpc_req_munmap)
+#define FASTRPC_IOCTL_INIT_ATTACH_SNS	_IO('R', 8)
 
 struct fastrpc_invoke_args {
 	__u64 ptr;
-- 
2.26.1

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Greg Kroah-Hartman]

On Mon, Aug 31, 2020 at 08:32:59PM -0400, Jonathan Marek wrote:
> Initializing sensors requires attaching to pd 2. Add an ioctl for that.
> 
> This corresponds to FASTRPC_INIT_ATTACH_SENSORS in the downstream driver.
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  drivers/misc/fastrpc.c      | 9 ++++++---
>  include/uapi/misc/fastrpc.h | 5 +++--
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7939c55daceb..ea5e9ca0d705 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1276,7 +1276,7 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
>  	return 0;
>  }
>  
> -static int fastrpc_init_attach(struct fastrpc_user *fl)
> +static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>  {
>  	struct fastrpc_invoke_args args[1];
>  	int tgid = fl->tgid;
> @@ -1287,7 +1287,7 @@ static int fastrpc_init_attach(struct fastrpc_user *fl)
>  	args[0].fd = -1;
>  	args[0].reserved = 0;
>  	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
> -	fl->pd = 0;
> +	fl->pd = pd;
>  
>  	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>  				       sc, &args[0]);
> @@ -1477,7 +1477,10 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
>  		err = fastrpc_invoke(fl, argp);
>  		break;
>  	case FASTRPC_IOCTL_INIT_ATTACH:
> -		err = fastrpc_init_attach(fl);
> +		err = fastrpc_init_attach(fl, 0);
> +		break;
> +	case FASTRPC_IOCTL_INIT_ATTACH_SNS:
> +		err = fastrpc_init_attach(fl, 2);

Shouldn't you have #defines for those magic numbers somewhere?  What
does 0 and 2 mean?

thanks,

greg k-h

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Jonathan Marek]

On 9/7/20 8:33 AM, Greg Kroah-Hartman wrote:
> On Mon, Aug 31, 2020 at 08:32:59PM -0400, Jonathan Marek wrote:
>> Initializing sensors requires attaching to pd 2. Add an ioctl for that.
>>
>> This corresponds to FASTRPC_INIT_ATTACH_SENSORS in the downstream driver.
>>
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   drivers/misc/fastrpc.c      | 9 ++++++---
>>   include/uapi/misc/fastrpc.h | 5 +++--
>>   2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 7939c55daceb..ea5e9ca0d705 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1276,7 +1276,7 @@ static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
>>   	return 0;
>>   }
>>   
>> -static int fastrpc_init_attach(struct fastrpc_user *fl)
>> +static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
>>   {
>>   	struct fastrpc_invoke_args args[1];
>>   	int tgid = fl->tgid;
>> @@ -1287,7 +1287,7 @@ static int fastrpc_init_attach(struct fastrpc_user *fl)
>>   	args[0].fd = -1;
>>   	args[0].reserved = 0;
>>   	sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0);
>> -	fl->pd = 0;
>> +	fl->pd = pd;
>>   
>>   	return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE,
>>   				       sc, &args[0]);
>> @@ -1477,7 +1477,10 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd,
>>   		err = fastrpc_invoke(fl, argp);
>>   		break;
>>   	case FASTRPC_IOCTL_INIT_ATTACH:
>> -		err = fastrpc_init_attach(fl);
>> +		err = fastrpc_init_attach(fl, 0);
>> +		break;
>> +	case FASTRPC_IOCTL_INIT_ATTACH_SNS:
>> +		err = fastrpc_init_attach(fl, 2);
> 
> Shouldn't you have #defines for those magic numbers somewhere?  What
> does 0 and 2 mean?
> 

This is based off a downstream driver which also uses magic numbers, 
although I can make an educated guess about the meaning.

Srini do you have any suggestions for how to name these values?

> thanks,
> 
> greg k-h
> 

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Srinivas Kandagatla]

On 07/09/2020 14:51, Jonathan Marek wrote:
>>> @@ -1477,7 +1477,10 @@ static long fastrpc_device_ioctl(struct file 
>>> *file, unsigned int cmd,
>>>           err = fastrpc_invoke(fl, argp);
>>>           break;
>>>       case FASTRPC_IOCTL_INIT_ATTACH:
>>> -        err = fastrpc_init_attach(fl);
>>> +        err = fastrpc_init_attach(fl, 0);
>>> +        break;
>>> +    case FASTRPC_IOCTL_INIT_ATTACH_SNS:
>>> +        err = fastrpc_init_attach(fl, 2);
>>
>> Shouldn't you have #defines for those magic numbers somewhere?  What
>> does 0 and 2 mean?
>>
> 
> This is based off a downstream driver which also uses magic numbers, 
> although I can make an educated guess about the meaning.
> 
> Srini do you have any suggestions for how to name these values?

These are domain id corresponding to each core.
you can use SDSP_DOMAIN_ID in here!
these are already defined in the file as:

#define ADSP_DOMAIN_ID (0)
#define MDSP_DOMAIN_ID (1)
#define SDSP_DOMAIN_ID (2)
#define CDSP_DOMAIN_ID (3)


--srini

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Jonathan Marek]

On 9/7/20 9:58 AM, Srinivas Kandagatla wrote:
> 
> 
> On 07/09/2020 14:51, Jonathan Marek wrote:
>>>> @@ -1477,7 +1477,10 @@ static long fastrpc_device_ioctl(struct file 
>>>> *file, unsigned int cmd,
>>>>           err = fastrpc_invoke(fl, argp);
>>>>           break;
>>>>       case FASTRPC_IOCTL_INIT_ATTACH:
>>>> -        err = fastrpc_init_attach(fl);
>>>> +        err = fastrpc_init_attach(fl, 0);
>>>> +        break;
>>>> +    case FASTRPC_IOCTL_INIT_ATTACH_SNS:
>>>> +        err = fastrpc_init_attach(fl, 2);
>>>
>>> Shouldn't you have #defines for those magic numbers somewhere?  What
>>> does 0 and 2 mean?
>>>
>>
>> This is based off a downstream driver which also uses magic numbers, 
>> although I can make an educated guess about the meaning.
>>
>> Srini do you have any suggestions for how to name these values?
> 
> These are domain id corresponding to each core.
> you can use SDSP_DOMAIN_ID in here!
> these are already defined in the file as:
> 
> #define ADSP_DOMAIN_ID (0)
> #define MDSP_DOMAIN_ID (1)
> #define SDSP_DOMAIN_ID (2)
> #define CDSP_DOMAIN_ID (3)
> 

I don't think this is right:

FASTRPC_IOCTL_INIT_ATTACH uses pd = 0
FASTRPC_IOCTL_INIT_CREATE uses pd = 1

And these two ioctl are used with all DSP cores. So it wouldn't make 
sense for the pd value to correspond to the domain id.

> 
> --srini

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Srinivas Kandagatla]

On 07/09/2020 15:02, Jonathan Marek wrote:
>>>
>>> Srini do you have any suggestions for how to name these values?
>>
>> These are domain id corresponding to each core.
>> you can use SDSP_DOMAIN_ID in here!
>> these are already defined in the file as:
>>
>> #define ADSP_DOMAIN_ID (0)
>> #define MDSP_DOMAIN_ID (1)
>> #define SDSP_DOMAIN_ID (2)
>> #define CDSP_DOMAIN_ID (3)
>>
> 
> I don't think this is right:
> 
> FASTRPC_IOCTL_INIT_ATTACH uses pd = 0
> FASTRPC_IOCTL_INIT_CREATE uses pd = 1
> 
> And these two ioctl are used with all DSP cores. So it wouldn't make 
> sense for the pd value to correspond to the domain id.
> 
You are right, values are pretty much similar to domain ids but not 
exactly the same as Protection Domain(PD) ids.

I spoke to qcom guys about this, and this is what I have.

0 is Audio Process PD
1 is Dynamic User PD, cases like SNPE or CV
2 is Sensor Process PD.


Hope this helps!

--srini

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Srinivas Kandagatla]

On 01/09/2020 01:32, Jonathan Marek wrote:
> -#define FASTRPC_IOCTL_MMAP              _IOWR('R', 6, struct fastrpc_req_mmap)
> -#define FASTRPC_IOCTL_MUNMAP            _IOWR('R', 7, struct fastrpc_req_munmap)
> +#define FASTRPC_IOCTL_MMAP		_IOWR('R', 6, struct fastrpc_req_mmap)
> +#define FASTRPC_IOCTL_MUNMAP		_IOWR('R', 7, struct fastrpc_req_munmap)

Looks like changes that do not belong to this patch!

I wanted to try this patch on SM8250.
How do you test attaching fastrpc to sensor core?, I mean which 
userspace lib/tool do you use?

--srini

> +#define FASTRPC_IOCTL_INIT_ATTACH_SNS	_IO('R', 8)

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Jonathan Marek]

On 9/7/20 8:36 AM, Srinivas Kandagatla wrote:
> 
> 
> On 01/09/2020 01:32, Jonathan Marek wrote:
>> -#define FASTRPC_IOCTL_MMAP              _IOWR('R', 6, struct 
>> fastrpc_req_mmap)
>> -#define FASTRPC_IOCTL_MUNMAP            _IOWR('R', 7, struct 
>> fastrpc_req_munmap)
>> +#define FASTRPC_IOCTL_MMAP        _IOWR('R', 6, struct fastrpc_req_mmap)
>> +#define FASTRPC_IOCTL_MUNMAP        _IOWR('R', 7, struct 
>> fastrpc_req_munmap)
> 
> Looks like changes that do not belong to this patch!
> 
> I wanted to try this patch on SM8250.
> How do you test attaching fastrpc to sensor core?, I mean which 
> userspace lib/tool do you use?
> 
> --srini
> 

I pushed my sdsprpcd implementation to github, which is responsible for 
initializing the sensors, and uses this ioctl:

https://github.com/flto/fastrpc

Note: it uses my own WIP fastrpc "library" instead of the one from 
linaro, I also have other related code, like a sensor client, and 
cDSP/aDSP compute examples, but need to confirm that I can share them

Also, the corresponding dts patch I sent has a problem, the label = 
"dsps"; should be label = "sdsp"; (copied the "dsps" from downstream, 
but upstream expects "sdsp"), will send a v2 later today.

>> +#define FASTRPC_IOCTL_INIT_ATTACH_SNS    _IO('R', 8)

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Srinivas Kandagatla]

On 07/09/2020 14:47, Jonathan Marek wrote:
> On 9/7/20 8:36 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 01/09/2020 01:32, Jonathan Marek wrote:
>>> -#define FASTRPC_IOCTL_MMAP              _IOWR('R', 6, struct 
>>> fastrpc_req_mmap)
>>> -#define FASTRPC_IOCTL_MUNMAP            _IOWR('R', 7, struct 
>>> fastrpc_req_munmap)
>>> +#define FASTRPC_IOCTL_MMAP        _IOWR('R', 6, struct 
>>> fastrpc_req_mmap)
>>> +#define FASTRPC_IOCTL_MUNMAP        _IOWR('R', 7, struct 
>>> fastrpc_req_munmap)
>>
>> Looks like changes that do not belong to this patch!
>>
>> I wanted to try this patch on SM8250.
>> How do you test attaching fastrpc to sensor core?, I mean which 
>> userspace lib/tool do you use?
>>
>> --srini
>>
> 
> I pushed my sdsprpcd implementation to github, which is responsible for 
> initializing the sensors, and uses this ioctl:
> 
> https://github.com/flto/fastrpc

Thanks!, I can take a look and see if I can try it out with linaro 
fastrpc library!
> 
> Note: it uses my own WIP fastrpc "library" instead of the one from 
> linaro, I also have other related code, like a sensor client, and 
> cDSP/aDSP compute examples, but need to confirm that I can share them
> 
> Also, the corresponding dts patch I sent has a problem, the label = 
> "dsps"; should be label = "sdsp"; (copied the "dsps" from downstream, 
> but upstream expects "sdsp"), will send a v2 later today.
Also the dts patch will fail to apply as it is, as it seems me that you 
have based the patch after adding audio dts patch!


--srini
> 
>>> +#define FASTRPC_IOCTL_INIT_ATTACH_SNS    _IO('R', 8)

Re: [PATCH] misc: fastrpc: add ioctl for attaching to sensors pd

[Jonathan Marek]

On 9/7/20 10:01 AM, Srinivas Kandagatla wrote:
> 
> 
> On 07/09/2020 14:47, Jonathan Marek wrote:
>> On 9/7/20 8:36 AM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 01/09/2020 01:32, Jonathan Marek wrote:
>>>> -#define FASTRPC_IOCTL_MMAP              _IOWR('R', 6, struct 
>>>> fastrpc_req_mmap)
>>>> -#define FASTRPC_IOCTL_MUNMAP            _IOWR('R', 7, struct 
>>>> fastrpc_req_munmap)
>>>> +#define FASTRPC_IOCTL_MMAP        _IOWR('R', 6, struct 
>>>> fastrpc_req_mmap)
>>>> +#define FASTRPC_IOCTL_MUNMAP        _IOWR('R', 7, struct 
>>>> fastrpc_req_munmap)
>>>
>>> Looks like changes that do not belong to this patch!
>>>
>>> I wanted to try this patch on SM8250.
>>> How do you test attaching fastrpc to sensor core?, I mean which 
>>> userspace lib/tool do you use?
>>>
>>> --srini
>>>
>>
>> I pushed my sdsprpcd implementation to github, which is responsible 
>> for initializing the sensors, and uses this ioctl:
>>
>> https://github.com/flto/fastrpc
> 
> Thanks!, I can take a look and see if I can try it out with linaro 
> fastrpc library!

You don't need linaro fastrpc library to try it, everything you need is 
in that repo.

>>
>> Note: it uses my own WIP fastrpc "library" instead of the one from 
>> linaro, I also have other related code, like a sensor client, and 
>> cDSP/aDSP compute examples, but need to confirm that I can share them
>>
>> Also, the corresponding dts patch I sent has a problem, the label = 
>> "dsps"; should be label = "sdsp"; (copied the "dsps" from downstream, 
>> but upstream expects "sdsp"), will send a v2 later today.
> Also the dts patch will fail to apply as it is, as it seems me that you 
> have based the patch after adding audio dts patch!
> 

Thanks for pointing it out, will make sure the v2 applies cleanly 
without audio dts patches applied.

> 
> --srini
>>
>>>> +#define FASTRPC_IOCTL_INIT_ATTACH_SNS    _IO('R', 8)