[PATCH v2 0/3] Add support for gpdsp remoteproc on sa8775p

[PATCH v2 0/3] Add support for gpdsp remoteproc on sa8775p

[Ling Xu]

The fastrpc driver has support for 5 types of remoteprocs. There are
some products which support GPDSP remoteprocs. GPDSPs are General
Purpose DSPs where tasks can be offloaded. Add changes to support GPDSP
remoteprocs and also add GPDSP fastrpc nodes.
Patch [v1]: https://lore.kernel.org/all/20250320051645.2254904-1-quic_lxu5@quicinc.com/

Changes in v2:
  - Add GPDSP labels in dt-bindings.

Ling Xu (3):
  arm64: dts: qcom: sa8775p: add GPDSP fastrpc-compute-cb nodes
  misc: fastrpc: add support for gpdsp remoteproc
  dt-bindings: misc: qcom,fastrpc: Add GPDSPs label

 .../bindings/misc/qcom,fastrpc.yaml           |  2 +
 arch/arm64/boot/dts/qcom/sa8775p.dtsi         | 58 +++++++++++++++++++
 drivers/misc/fastrpc.c                        | 10 +++-
 3 files changed, 68 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] arm64: dts: qcom: sa8775p: add GPDSP fastrpc-compute-cb nodes

[Ling Xu]

Add GPDSP0 and GPDSP1 fastrpc compute-cb nodes for sa8775p SoC.

Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p.dtsi | 58 +++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 581dac8556ec..28025c76c96a 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -4850,6 +4850,35 @@ IPCC_MPROC_SIGNAL_GLINK_QMP
 
 				label = "gpdsp0";
 				qcom,remote-pid = <17>;
+
+				fastrpc {
+					compatible = "qcom,fastrpc";
+					qcom,glink-channels = "fastrpcglink-apps-dsp";
+					label = "gdsp0";
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					compute-cb@1 {
+						compatible = "qcom,fastrpc-compute-cb";
+						reg = <1>;
+						iommus = <&apps_smmu 0x38a1 0x0>;
+						dma-coherent;
+					};
+
+					compute-cb@2 {
+						compatible = "qcom,fastrpc-compute-cb";
+						reg = <2>;
+						iommus = <&apps_smmu 0x38a2 0x0>;
+						dma-coherent;
+					};
+
+					compute-cb@3 {
+						compatible = "qcom,fastrpc-compute-cb";
+						reg = <3>;
+						iommus = <&apps_smmu 0x38a3 0x0>;
+						dma-coherent;
+					};
+				};
 			};
 		};
 
@@ -4893,6 +4922,35 @@ IPCC_MPROC_SIGNAL_GLINK_QMP
 
 				label = "gpdsp1";
 				qcom,remote-pid = <18>;
+
+				fastrpc {
+					compatible = "qcom,fastrpc";
+					qcom,glink-channels = "fastrpcglink-apps-dsp";
+					label = "gdsp1";
+					#address-cells = <1>;
+					#size-cells = <0>;
+
+					compute-cb@1 {
+						compatible = "qcom,fastrpc-compute-cb";
+						reg = <1>;
+						iommus = <&apps_smmu 0x38c1 0x0>;
+						dma-coherent;
+					};
+
+					compute-cb@2 {
+						compatible = "qcom,fastrpc-compute-cb";
+						reg = <2>;
+						iommus = <&apps_smmu 0x38c2 0x0>;
+						dma-coherent;
+					};
+
+					compute-cb@3 {
+						compatible = "qcom,fastrpc-compute-cb";
+						reg = <3>;
+						iommus = <&apps_smmu 0x38c3 0x0>;
+						dma-coherent;
+					};
+				};
 			};
 		};
 
-- 
2.34.1

Re: [PATCH v2 1/3] arm64: dts: qcom: sa8775p: add GPDSP fastrpc-compute-cb nodes

[Dmitry Baryshkov]

On Thu, Mar 20, 2025 at 02:44:44PM +0530, Ling Xu wrote:
> Add GPDSP0 and GPDSP1 fastrpc compute-cb nodes for sa8775p SoC.
> 
> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p.dtsi | 58 +++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

-- 
With best wishes
Dmitry

[PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Ling Xu]

The fastrpc driver has support for 5 types of remoteprocs. There are
some products which support GPDSP remoteprocs. Add changes to support
GPDSP remoteprocs.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
---
 drivers/misc/fastrpc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 7b7a22c91fe4..80aa554b3042 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -28,7 +28,9 @@
 #define SDSP_DOMAIN_ID (2)
 #define CDSP_DOMAIN_ID (3)
 #define CDSP1_DOMAIN_ID (4)
-#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
+#define GDSP0_DOMAIN_ID (5)
+#define GDSP1_DOMAIN_ID (6)
+#define FASTRPC_DEV_MAX		7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
 #define FASTRPC_MAX_SESSIONS	14
 #define FASTRPC_MAX_VMIDS	16
 #define FASTRPC_ALIGN		128
@@ -107,7 +109,9 @@
 #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
 
 static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
-						"sdsp", "cdsp", "cdsp1" };
+						"sdsp", "cdsp",
+						"cdsp1", "gdsp0",
+						"gdsp1" };
 struct fastrpc_phy_page {
 	u64 addr;		/* physical address */
 	u64 size;		/* size of contiguous region */
@@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 		break;
 	case CDSP_DOMAIN_ID:
 	case CDSP1_DOMAIN_ID:
+	case GDSP0_DOMAIN_ID:
+	case GDSP1_DOMAIN_ID:
 		data->unsigned_support = true;
 		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
 		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
-- 
2.34.1

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Ekansh Gupta]

On 3/20/2025 2:44 PM, Ling Xu wrote:
> The fastrpc driver has support for 5 types of remoteprocs. There are
> some products which support GPDSP remoteprocs. Add changes to support
> GPDSP remoteprocs.
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7b7a22c91fe4..80aa554b3042 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -28,7 +28,9 @@
>  #define SDSP_DOMAIN_ID (2)
>  #define CDSP_DOMAIN_ID (3)
>  #define CDSP1_DOMAIN_ID (4)
> -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> +#define GDSP0_DOMAIN_ID (5)
> +#define GDSP1_DOMAIN_ID (6)
> +#define FASTRPC_DEV_MAX		7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>  #define FASTRPC_MAX_SESSIONS	14
>  #define FASTRPC_MAX_VMIDS	16
>  #define FASTRPC_ALIGN		128
> @@ -107,7 +109,9 @@
>  #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>  
>  static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> -						"sdsp", "cdsp", "cdsp1" };
> +						"sdsp", "cdsp",
> +						"cdsp1", "gdsp0",
> +						"gdsp1" };
>  struct fastrpc_phy_page {
>  	u64 addr;		/* physical address */
>  	u64 size;		/* size of contiguous region */
> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  		break;
>  	case CDSP_DOMAIN_ID:
>  	case CDSP1_DOMAIN_ID:
> +	case GDSP0_DOMAIN_ID:
> +	case GDSP1_DOMAIN_ID:

Reviewed-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

Thanks,
Ekansh

>  		data->unsigned_support = true;
>  		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
>  		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Konrad Dybcio]

On 3/20/25 10:14 AM, Ling Xu wrote:
> The fastrpc driver has support for 5 types of remoteprocs. There are
> some products which support GPDSP remoteprocs. Add changes to support
> GPDSP remoteprocs.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> ---
>  drivers/misc/fastrpc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7b7a22c91fe4..80aa554b3042 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -28,7 +28,9 @@
>  #define SDSP_DOMAIN_ID (2)
>  #define CDSP_DOMAIN_ID (3)
>  #define CDSP1_DOMAIN_ID (4)
> -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> +#define GDSP0_DOMAIN_ID (5)
> +#define GDSP1_DOMAIN_ID (6)
> +#define FASTRPC_DEV_MAX		7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>  #define FASTRPC_MAX_SESSIONS	14
>  #define FASTRPC_MAX_VMIDS	16
>  #define FASTRPC_ALIGN		128
> @@ -107,7 +109,9 @@
>  #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>  
>  static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> -						"sdsp", "cdsp", "cdsp1" };
> +						"sdsp", "cdsp",
> +						"cdsp1", "gdsp0",
> +						"gdsp1" };
>  struct fastrpc_phy_page {
>  	u64 addr;		/* physical address */
>  	u64 size;		/* size of contiguous region */
> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  		break;
>  	case CDSP_DOMAIN_ID:
>  	case CDSP1_DOMAIN_ID:
> +	case GDSP0_DOMAIN_ID:
> +	case GDSP1_DOMAIN_ID:
>  		data->unsigned_support = true;

There's a comment above this hunk that is no longer valid:

'/* Unsigned PD offloading is only supported on CDSP and CDSP1 */'

I would say it can be removed altogether

I would also support renaming "unsigned_support" which is very generic to
something like allow_unsigned_pds

Konrad

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Srinivas Kandagatla]

On 20/03/2025 09:14, Ling Xu wrote:
> The fastrpc driver has support for 5 types of remoteprocs. There are
> some products which support GPDSP remoteprocs. Add changes to support
> GPDSP remoteprocs.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> ---
>   drivers/misc/fastrpc.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7b7a22c91fe4..80aa554b3042 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -28,7 +28,9 @@
>   #define SDSP_DOMAIN_ID (2)
>   #define CDSP_DOMAIN_ID (3)
>   #define CDSP1_DOMAIN_ID (4)
> -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> +#define GDSP0_DOMAIN_ID (5)
> +#define GDSP1_DOMAIN_ID (6)

We have already made the driver look silly here, Lets not add domain ids 
for each instance, which is not a scalable.

Domain ids are strictly for a domain not each instance.


> +#define FASTRPC_DEV_MAX		7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>   #define FASTRPC_MAX_SESSIONS	14
>   #define FASTRPC_MAX_VMIDS	16
>   #define FASTRPC_ALIGN		128
> @@ -107,7 +109,9 @@
>   #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>   
>   static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> -						"sdsp", "cdsp", "cdsp1" };
> +						"sdsp", "cdsp",
> +						"cdsp1", "gdsp0",
> +						"gdsp1" };
>   struct fastrpc_phy_page {
>   	u64 addr;		/* physical address */
>   	u64 size;		/* size of contiguous region */
> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>   		break;
>   	case CDSP_DOMAIN_ID:
>   	case CDSP1_DOMAIN_ID:
> +	case GDSP0_DOMAIN_ID:
> +	case GDSP1_DOMAIN_ID:
>   		data->unsigned_support = true;
>   		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
>   		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);


Can you try this patch: only compile tested.

---------------------------------->cut<---------------------------------------
 From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date: Thu, 20 Mar 2025 17:07:05 +0000
Subject: [PATCH] misc: fastrpc: cleanup the domain names

Currently the domain ids are added for each instance of domain, this is
totally not scalable approch.

Clean this mess and create domain ids for only domains not its
instances.
This patch also moves the domain ids to uapi header as this is required
for FASTRPC_IOCTL_GET_DSP_INFO ioctl.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
  drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
  include/uapi/misc/fastrpc.h |  7 ++++++
  2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 7b7a22c91fe4..b3932897a437 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -23,12 +23,6 @@
  #include <uapi/misc/fastrpc.h>
  #include <linux/of_reserved_mem.h>

-#define ADSP_DOMAIN_ID (0)
-#define MDSP_DOMAIN_ID (1)
-#define SDSP_DOMAIN_ID (2)
-#define CDSP_DOMAIN_ID (3)
-#define CDSP1_DOMAIN_ID (4)
-#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
  #define FASTRPC_MAX_SESSIONS	14
  #define FASTRPC_MAX_VMIDS	16
  #define FASTRPC_ALIGN		128
@@ -106,8 +100,6 @@

  #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, 
miscdev)

-static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
-						"sdsp", "cdsp", "cdsp1" };
  struct fastrpc_phy_page {
  	u64 addr;		/* physical address */
  	u64 size;		/* size of contiguous region */
@@ -1769,7 +1761,7 @@ static int fastrpc_get_dsp_info(struct 
fastrpc_user *fl, char __user *argp)
  		return  -EFAULT;

  	cap.capability = 0;
-	if (cap.domain >= FASTRPC_DEV_MAX) {
+	if (cap.domain >= FASTRPC_DOMAIN_MAX) {
  		dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
  			cap.domain, err);
  		return -ECHRNG;
@@ -2255,6 +2247,24 @@ static int fastrpc_device_register(struct device 
*dev, struct fastrpc_channel_ct
  	return err;
  }

+static int fastrpc_get_domain_id(const char *domain)
+{
+	if (strncmp(domain, "adsp", 4) == 0) {
+		return ADSP_DOMAIN_ID;
+	} else	if (strncmp(domain, "cdsp", 4) == 0) {
+		return CDSP_DOMAIN_ID;
+	} else if (strncmp(domain, "mdsp", 4) ==0) {
+		return MDSP_DOMAIN_ID;
+	} else if (strncmp(domain, "sdsp", 4) ==0) {
+		return SDSP_DOMAIN_ID;
+	} else if (strncmp(domain, "gdsp", 4) ==0) {
+		return GDSP_DOMAIN_ID;
+	}
+
+	return -EINVAL;
+
+}
+
  static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
  {
  	struct device *rdev = &rpdev->dev;
@@ -2272,15 +2282,10 @@ static int fastrpc_rpmsg_probe(struct 
rpmsg_device *rpdev)
  		return err;
  	}

-	for (i = 0; i < FASTRPC_DEV_MAX; i++) {
-		if (!strcmp(domains[i], domain)) {
-			domain_id = i;
-			break;
-		}
-	}
+	domain_id = fastrpc_get_domain_id(domain);

  	if (domain_id < 0) {
-		dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
+		dev_info(rdev, "FastRPC Domain %s not supported\n", domain);
  		return -EINVAL;
  	}

@@ -2332,19 +2337,19 @@ static int fastrpc_rpmsg_probe(struct 
rpmsg_device *rpdev)
  	case SDSP_DOMAIN_ID:
  		/* Unsigned PD offloading is only supported on CDSP and CDSP1 */
  		data->unsigned_support = false;
-		err = fastrpc_device_register(rdev, data, secure_dsp, 
domains[domain_id]);
+		err = fastrpc_device_register(rdev, data, secure_dsp, domain);
  		if (err)
  			goto fdev_error;
  		break;
  	case CDSP_DOMAIN_ID:
-	case CDSP1_DOMAIN_ID:
+	case GDSP_DOMAIN_ID:
  		data->unsigned_support = true;
  		/* Create both device nodes so that we can allow both Signed and 
Unsigned PD */
-		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
+		err = fastrpc_device_register(rdev, data, true, domain);
  		if (err)
  			goto fdev_error;

-		err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
+		err = fastrpc_device_register(rdev, data, false, domain);
  		if (err)
  			goto populate_error;
  		break;
diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
index f33d914d8f46..89516abd258f 100644
--- a/include/uapi/misc/fastrpc.h
+++ b/include/uapi/misc/fastrpc.h
@@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
  	__s32 reserved[5];
  };

+#define ADSP_DOMAIN_ID (0)
+#define MDSP_DOMAIN_ID (1)
+#define SDSP_DOMAIN_ID (2)
+#define CDSP_DOMAIN_ID (3)
+#define GDSP_DOMAIN_ID (4)
+
+#define FASTRPC_DOMAIN_MAX	4
  struct fastrpc_ioctl_capability {
  	__u32 domain;
  	__u32 attribute_id;
-- 
2.25.1


---------------------------------->cut<---------------------------------------

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Dmitry Baryshkov]

On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 20/03/2025 09:14, Ling Xu wrote:
> > The fastrpc driver has support for 5 types of remoteprocs. There are
> > some products which support GPDSP remoteprocs. Add changes to support
> > GPDSP remoteprocs.
> > 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> > ---
> >   drivers/misc/fastrpc.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > index 7b7a22c91fe4..80aa554b3042 100644
> > --- a/drivers/misc/fastrpc.c
> > +++ b/drivers/misc/fastrpc.c
> > @@ -28,7 +28,9 @@
> >   #define SDSP_DOMAIN_ID (2)
> >   #define CDSP_DOMAIN_ID (3)
> >   #define CDSP1_DOMAIN_ID (4)
> > -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> > +#define GDSP0_DOMAIN_ID (5)
> > +#define GDSP1_DOMAIN_ID (6)
> 
> We have already made the driver look silly here, Lets not add domain ids for
> each instance, which is not a scalable.
> 
> Domain ids are strictly for a domain not each instance.

Then CDSP1 should also be gone, correct?

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Srinivas Kandagatla]

On 20/03/2025 18:43, Dmitry Baryshkov wrote:
> On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
>>
>>
>> On 20/03/2025 09:14, Ling Xu wrote:
>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>> some products which support GPDSP remoteprocs. Add changes to support
>>> GPDSP remoteprocs.
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>> ---
>>>    drivers/misc/fastrpc.c | 10 ++++++++--
>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 7b7a22c91fe4..80aa554b3042 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -28,7 +28,9 @@
>>>    #define SDSP_DOMAIN_ID (2)
>>>    #define CDSP_DOMAIN_ID (3)
>>>    #define CDSP1_DOMAIN_ID (4)
>>> -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>> +#define GDSP0_DOMAIN_ID (5)
>>> +#define GDSP1_DOMAIN_ID (6)
>>
>> We have already made the driver look silly here, Lets not add domain ids for
>> each instance, which is not a scalable.
>>
>> Domain ids are strictly for a domain not each instance.
> 
> Then CDSP1 should also be gone, correct?
Its already gone as part of the patch that I shared in this discussion.

I will send a proper patch to list once Ling/Ekansh has agree with it.

--srini
> 

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Dmitry Baryshkov]

On Fri, Mar 21, 2025 at 12:23:15PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 20/03/2025 18:43, Dmitry Baryshkov wrote:
> > On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
> > > 
> > > 
> > > On 20/03/2025 09:14, Ling Xu wrote:
> > > > The fastrpc driver has support for 5 types of remoteprocs. There are
> > > > some products which support GPDSP remoteprocs. Add changes to support
> > > > GPDSP remoteprocs.
> > > > 
> > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > > Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> > > > ---
> > > >    drivers/misc/fastrpc.c | 10 ++++++++--
> > > >    1 file changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > > index 7b7a22c91fe4..80aa554b3042 100644
> > > > --- a/drivers/misc/fastrpc.c
> > > > +++ b/drivers/misc/fastrpc.c
> > > > @@ -28,7 +28,9 @@
> > > >    #define SDSP_DOMAIN_ID (2)
> > > >    #define CDSP_DOMAIN_ID (3)
> > > >    #define CDSP1_DOMAIN_ID (4)
> > > > -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> > > > +#define GDSP0_DOMAIN_ID (5)
> > > > +#define GDSP1_DOMAIN_ID (6)
> > > 
> > > We have already made the driver look silly here, Lets not add domain ids for
> > > each instance, which is not a scalable.
> > > 
> > > Domain ids are strictly for a domain not each instance.
> > 
> > Then CDSP1 should also be gone, correct?
> Its already gone as part of the patch that I shared in this discussion.
> 
> I will send a proper patch to list once Ling/Ekansh has agree with it.

I see. For some reason mobile gmail decided that your patch is a quoted
message and ignored it.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Ekansh Gupta]

On 3/21/2025 5:53 PM, Srinivas Kandagatla wrote:
>
>
> On 20/03/2025 18:43, Dmitry Baryshkov wrote:
>> On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>> GPDSP remoteprocs.
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>>> ---
>>>>    drivers/misc/fastrpc.c | 10 ++++++++--
>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -28,7 +28,9 @@
>>>>    #define SDSP_DOMAIN_ID (2)
>>>>    #define CDSP_DOMAIN_ID (3)
>>>>    #define CDSP1_DOMAIN_ID (4)
>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>> +#define GDSP0_DOMAIN_ID (5)
>>>> +#define GDSP1_DOMAIN_ID (6)
>>>
>>> We have already made the driver look silly here, Lets not add domain ids for
>>> each instance, which is not a scalable.
>>>
>>> Domain ids are strictly for a domain not each instance.
>>
>> Then CDSP1 should also be gone, correct?
> Its already gone as part of the patch that I shared in this discussion.
>
> I will send a proper patch to list once Ling/Ekansh has agree with it.
>
Thanks, Srini, for sharing this clean-up patch. It looks proper to
me, but I was thinking if we could remove the domain_id dependency
from the fastrpc driver. The addition of any new DSP will frequently
require changes in the driver. Currently, its usage is for creating
different types of device nodes and transferring memory ownership to
SLPI when a memory region is added.

The actual intention behind different types of device nodes can be
defined as follows:

fastrpc-xdsp-secure: Used for signed (privileged) PD offload and for daemons.
fastrpc-xdsp: Should be used only for unsigned (less privileged) PD offload.

The reason for this constraint is to prevent any untrusted process
from communicating with any privileged PD on DSP, which poses a security risk.
The access to different device nodes can be provided/restricted based on UID/GID
(still need to check more on this; on Android-like systems, this is controlled by
SELinux).

There is already a qcom,non-secure-domain device tree property[1] which doesn't
have a proper definition as of today. The actual way to differentiate between
secure and non-secure DSP should be based on its ability to support unsigned PD.

One way to remove the domain_id dependency that I can think of is to use this
property to create different types of device nodes. Essentially, if unsigned PD
is supported (e.g., CDSP, GPDSP), we add this property to the DT node and create
both types of device nodes based on this. Otherwise, only the secure device node
is created.

This raises the question of backward compatibility, but I see that on most older
platform DTs, this property is already added, so both device nodes will be created
there, and applications will work as expected. If any old DT DSP node lacks this
property, we can add it there as well.

Going forward, the qcom-non-secure-property should be added only if unsigned PD
is supported. This way, we can clean up the driver completely to remove the
domain_id dependency.

If this sounds good, I can work on this design and send out a patch.

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml#n44

--Ekansh

> --srini
>>
>

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Dmitry Baryshkov]

On 02/04/2025 11:38, Ekansh Gupta wrote:
> 
> 
> On 3/21/2025 5:53 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 20/03/2025 18:43, Dmitry Baryshkov wrote:
>>> On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
>>>>
>>>>
>>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>>> GPDSP remoteprocs.
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>>>> ---
>>>>>     drivers/misc/fastrpc.c | 10 ++++++++--
>>>>>     1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>>> --- a/drivers/misc/fastrpc.c
>>>>> +++ b/drivers/misc/fastrpc.c
>>>>> @@ -28,7 +28,9 @@
>>>>>     #define SDSP_DOMAIN_ID (2)
>>>>>     #define CDSP_DOMAIN_ID (3)
>>>>>     #define CDSP1_DOMAIN_ID (4)
>>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>>> +#define GDSP0_DOMAIN_ID (5)
>>>>> +#define GDSP1_DOMAIN_ID (6)
>>>>
>>>> We have already made the driver look silly here, Lets not add domain ids for
>>>> each instance, which is not a scalable.
>>>>
>>>> Domain ids are strictly for a domain not each instance.
>>>
>>> Then CDSP1 should also be gone, correct?
>> Its already gone as part of the patch that I shared in this discussion.
>>
>> I will send a proper patch to list once Ling/Ekansh has agree with it.
>>
> Thanks, Srini, for sharing this clean-up patch. It looks proper to
> me, but I was thinking if we could remove the domain_id dependency
> from the fastrpc driver. The addition of any new DSP will frequently
> require changes in the driver. Currently, its usage is for creating
> different types of device nodes and transferring memory ownership to
> SLPI when a memory region is added.
> 
> The actual intention behind different types of device nodes can be
> defined as follows:
> 
> fastrpc-xdsp-secure: Used for signed (privileged) PD offload and for daemons.
> fastrpc-xdsp: Should be used only for unsigned (less privileged) PD offload.
> 
> The reason for this constraint is to prevent any untrusted process
> from communicating with any privileged PD on DSP, which poses a security risk.
> The access to different device nodes can be provided/restricted based on UID/GID
> (still need to check more on this; on Android-like systems, this is controlled by
> SELinux).
> 
> There is already a qcom,non-secure-domain device tree property[1] which doesn't
> have a proper definition as of today. The actual way to differentiate between
> secure and non-secure DSP should be based on its ability to support unsigned PD.
> 
> One way to remove the domain_id dependency that I can think of is to use this
> property to create different types of device nodes. Essentially, if unsigned PD
> is supported (e.g., CDSP, GPDSP), we add this property to the DT node and create
> both types of device nodes based on this. Otherwise, only the secure device node
> is created.

This sounds like breaking backwards compatibility on the userspace side. 
You can not do that.

> 
> This raises the question of backward compatibility, but I see that on most older
> platform DTs, this property is already added, so both device nodes will be created
> there, and applications will work as expected. If any old DT DSP node lacks this
> property, we can add it there as well.
> 
> Going forward, the qcom-non-secure-property should be added only if unsigned PD
> is supported. This way, we can clean up the driver completely to remove the
> domain_id dependency.
> 
> If this sounds good, I can work on this design and send out a patch.
> 
> [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml#n44
> 
> --Ekansh
> 
>> --srini
>>>
>>
> 


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Ekansh Gupta]

On 4/2/2025 2:12 PM, Dmitry Baryshkov wrote:
> On 02/04/2025 11:38, Ekansh Gupta wrote:
>>
>>
>> On 3/21/2025 5:53 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 20/03/2025 18:43, Dmitry Baryshkov wrote:
>>>> On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
>>>>>
>>>>>
>>>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>>>> GPDSP remoteprocs.
>>>>>>
>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>>>>> ---
>>>>>>     drivers/misc/fastrpc.c | 10 ++++++++--
>>>>>>     1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>>>> --- a/drivers/misc/fastrpc.c
>>>>>> +++ b/drivers/misc/fastrpc.c
>>>>>> @@ -28,7 +28,9 @@
>>>>>>     #define SDSP_DOMAIN_ID (2)
>>>>>>     #define CDSP_DOMAIN_ID (3)
>>>>>>     #define CDSP1_DOMAIN_ID (4)
>>>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>>>> +#define GDSP0_DOMAIN_ID (5)
>>>>>> +#define GDSP1_DOMAIN_ID (6)
>>>>>
>>>>> We have already made the driver look silly here, Lets not add domain ids for
>>>>> each instance, which is not a scalable.
>>>>>
>>>>> Domain ids are strictly for a domain not each instance.
>>>>
>>>> Then CDSP1 should also be gone, correct?
>>> Its already gone as part of the patch that I shared in this discussion.
>>>
>>> I will send a proper patch to list once Ling/Ekansh has agree with it.
>>>
>> Thanks, Srini, for sharing this clean-up patch. It looks proper to
>> me, but I was thinking if we could remove the domain_id dependency
>> from the fastrpc driver. The addition of any new DSP will frequently
>> require changes in the driver. Currently, its usage is for creating
>> different types of device nodes and transferring memory ownership to
>> SLPI when a memory region is added.
>>
>> The actual intention behind different types of device nodes can be
>> defined as follows:
>>
>> fastrpc-xdsp-secure: Used for signed (privileged) PD offload and for daemons.
>> fastrpc-xdsp: Should be used only for unsigned (less privileged) PD offload.
>>
>> The reason for this constraint is to prevent any untrusted process
>> from communicating with any privileged PD on DSP, which poses a security risk.
>> The access to different device nodes can be provided/restricted based on UID/GID
>> (still need to check more on this; on Android-like systems, this is controlled by
>> SELinux).
>>
>> There is already a qcom,non-secure-domain device tree property[1] which doesn't
>> have a proper definition as of today. The actual way to differentiate between
>> secure and non-secure DSP should be based on its ability to support unsigned PD.
>>
>> One way to remove the domain_id dependency that I can think of is to use this
>> property to create different types of device nodes. Essentially, if unsigned PD
>> is supported (e.g., CDSP, GPDSP), we add this property to the DT node and create
>> both types of device nodes based on this. Otherwise, only the secure device node
>> is created.
>
> This sounds like breaking backwards compatibility on the userspace side. You can not do that.
Okay, I thought if the property is added for all older platforms, that will ensure backward
compatibility is maintained for old built applications.

From userspace, the expected device open sequence is to try with the secure device node and
fallback to the default/non-secure node if the secure node is not available/accessible.
I understand the ABI cannot be broken, and this expectation should be added for new
applications/platforms.

This is still a security issue that needs to be fixed in some way. I'll try to find out if any other
approach can address this.

That being said, I'm fine with Srini's change for domain name clean-up.

I would request Ling to test the patch.

--Ekansh

>
>>
>> This raises the question of backward compatibility, but I see that on most older
>> platform DTs, this property is already added, so both device nodes will be created
>> there, and applications will work as expected. If any old DT DSP node lacks this
>> property, we can add it there as well.
>>
>> Going forward, the qcom-non-secure-property should be added only if unsigned PD
>> is supported. This way, we can clean up the driver completely to remove the
>> domain_id dependency.
>>
>> If this sounds good, I can work on this design and send out a patch.
>>
>> [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml#n44
>>
>> --Ekansh
>>
>>> --srini
>>>>
>>>
>>
>
>

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Srinivas Kandagatla]

On 03/04/2025 05:44, Ekansh Gupta wrote:
> 
> On 4/2/2025 2:12 PM, Dmitry Baryshkov wrote:
>> On 02/04/2025 11:38, Ekansh Gupta wrote:
>>>
>>> On 3/21/2025 5:53 PM, Srinivas Kandagatla wrote:
>>>>
>>>> On 20/03/2025 18:43, Dmitry Baryshkov wrote:
>>>>> On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
>>>>>>
>>>>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>>>>> GPDSP remoteprocs.
>>>>>>>
>>>>>>> Reviewed-by: Dmitry Baryshkov<dmitry.baryshkov@oss.qualcomm.com>
>>>>>>> Signed-off-by: Ling Xu<quic_lxu5@quicinc.com>
>>>>>>> ---
>>>>>>>      drivers/misc/fastrpc.c | 10 ++++++++--
>>>>>>>      1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>>>>> --- a/drivers/misc/fastrpc.c
>>>>>>> +++ b/drivers/misc/fastrpc.c
>>>>>>> @@ -28,7 +28,9 @@
>>>>>>>      #define SDSP_DOMAIN_ID (2)
>>>>>>>      #define CDSP_DOMAIN_ID (3)
>>>>>>>      #define CDSP1_DOMAIN_ID (4)
>>>>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>>>>> +#define GDSP0_DOMAIN_ID (5)
>>>>>>> +#define GDSP1_DOMAIN_ID (6)
>>>>>> We have already made the driver look silly here, Lets not add domain ids for
>>>>>> each instance, which is not a scalable.
>>>>>>
>>>>>> Domain ids are strictly for a domain not each instance.
>>>>> Then CDSP1 should also be gone, correct?
>>>> Its already gone as part of the patch that I shared in this discussion.
>>>>
>>>> I will send a proper patch to list once Ling/Ekansh has agree with it.
>>>>
>>> Thanks, Srini, for sharing this clean-up patch. It looks proper to
>>> me, but I was thinking if we could remove the domain_id dependency
>>> from the fastrpc driver. The addition of any new DSP will frequently
>>> require changes in the driver. Currently, its usage is for creating
>>> different types of device nodes and transferring memory ownership to
>>> SLPI when a memory region is added.
>>>
>>> The actual intention behind different types of device nodes can be
>>> defined as follows:
>>>
>>> fastrpc-xdsp-secure: Used for signed (privileged) PD offload and for daemons.
>>> fastrpc-xdsp: Should be used only for unsigned (less privileged) PD offload.
>>>
>>> The reason for this constraint is to prevent any untrusted process
>>> from communicating with any privileged PD on DSP, which poses a security risk.
>>> The access to different device nodes can be provided/restricted based on UID/GID
>>> (still need to check more on this; on Android-like systems, this is controlled by
>>> SELinux).
>>>
>>> There is already a qcom,non-secure-domain device tree property[1] which doesn't
>>> have a proper definition as of today. The actual way to differentiate between
>>> secure and non-secure DSP should be based on its ability to support unsigned PD.
>>>
>>> One way to remove the domain_id dependency that I can think of is to use this
>>> property to create different types of device nodes. Essentially, if unsigned PD
>>> is supported (e.g., CDSP, GPDSP), we add this property to the DT node and create
>>> both types of device nodes based on this. Otherwise, only the secure device node
>>> is created.
>> This sounds like breaking backwards compatibility on the userspace side. You can not do that.
> Okay, I thought if the property is added for all older platforms, that will ensure backward
> compatibility is maintained for old built applications.
> 
>  From userspace, the expected device open sequence is to try with the secure device node and
> fallback to the default/non-secure node if the secure node is not available/accessible.
> I understand the ABI cannot be broken, and this expectation should be added for new
> applications/platforms.
> 
> This is still a security issue that needs to be fixed in some way. I'll try to find out if any other
> approach can address this.
In the past I have suggested you to update the dt-bindings so that any 
new platforms that get added will not use the qcom,non-secure-domain 
flag. The usage of this flag is still confusing for any new users, as 
per the dt bindings its open to be used.

As we can not break the backwards compatibility, why not just restrict 
that to those platforms for now, and enforce new platforms to use not 
use it for for domains like adsp.

--srini

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Dmitry Baryshkov]

On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 20/03/2025 09:14, Ling Xu wrote:
> > The fastrpc driver has support for 5 types of remoteprocs. There are
> > some products which support GPDSP remoteprocs. Add changes to support
> > GPDSP remoteprocs.
> > 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> > ---
> >   drivers/misc/fastrpc.c | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > index 7b7a22c91fe4..80aa554b3042 100644
> > --- a/drivers/misc/fastrpc.c
> > +++ b/drivers/misc/fastrpc.c
> > @@ -28,7 +28,9 @@
> >   #define SDSP_DOMAIN_ID (2)
> >   #define CDSP_DOMAIN_ID (3)
> >   #define CDSP1_DOMAIN_ID (4)
> > -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> > +#define GDSP0_DOMAIN_ID (5)
> > +#define GDSP1_DOMAIN_ID (6)
> 
> We have already made the driver look silly here, Lets not add domain ids for
> each instance, which is not a scalable.
> 
> Domain ids are strictly for a domain not each instance.
> 
> 
> > +#define FASTRPC_DEV_MAX		7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
> >   #define FASTRPC_MAX_SESSIONS	14
> >   #define FASTRPC_MAX_VMIDS	16
> >   #define FASTRPC_ALIGN		128
> > @@ -107,7 +109,9 @@
> >   #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
> >   static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> > -						"sdsp", "cdsp", "cdsp1" };
> > +						"sdsp", "cdsp",
> > +						"cdsp1", "gdsp0",
> > +						"gdsp1" };
> >   struct fastrpc_phy_page {
> >   	u64 addr;		/* physical address */
> >   	u64 size;		/* size of contiguous region */
> > @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> >   		break;
> >   	case CDSP_DOMAIN_ID:
> >   	case CDSP1_DOMAIN_ID:
> > +	case GDSP0_DOMAIN_ID:
> > +	case GDSP1_DOMAIN_ID:
> >   		data->unsigned_support = true;
> >   		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
> >   		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> 
> 
> Can you try this patch: only compile tested.
> 
> ---------------------------------->cut<---------------------------------------
> From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Date: Thu, 20 Mar 2025 17:07:05 +0000
> Subject: [PATCH] misc: fastrpc: cleanup the domain names
> 
> Currently the domain ids are added for each instance of domain, this is
> totally not scalable approch.
> 
> Clean this mess and create domain ids for only domains not its
> instances.
> This patch also moves the domain ids to uapi header as this is required
> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
>  include/uapi/misc/fastrpc.h |  7 ++++++
>  2 files changed, 32 insertions(+), 20 deletions(-)
> 


> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> index f33d914d8f46..89516abd258f 100644
> --- a/include/uapi/misc/fastrpc.h
> +++ b/include/uapi/misc/fastrpc.h
> @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
>  	__s32 reserved[5];
>  };
> 
> +#define ADSP_DOMAIN_ID (0)
> +#define MDSP_DOMAIN_ID (1)
> +#define SDSP_DOMAIN_ID (2)
> +#define CDSP_DOMAIN_ID (3)
> +#define GDSP_DOMAIN_ID (4)

Why are you adding these to uAPI? How are they going to be used by the
userspace?

> +
> +#define FASTRPC_DOMAIN_MAX	4
>  struct fastrpc_ioctl_capability {
>  	__u32 domain;
>  	__u32 attribute_id;
> -- 
> 2.25.1
> 
> 
> ---------------------------------->cut<---------------------------------------

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Srinivas Kandagatla]

On 21/03/2025 14:07, Dmitry Baryshkov wrote:
> On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
>>
>>
>> On 20/03/2025 09:14, Ling Xu wrote:
>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>> some products which support GPDSP remoteprocs. Add changes to support
>>> GPDSP remoteprocs.
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>> ---
>>>    drivers/misc/fastrpc.c | 10 ++++++++--
>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 7b7a22c91fe4..80aa554b3042 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -28,7 +28,9 @@
>>>    #define SDSP_DOMAIN_ID (2)
>>>    #define CDSP_DOMAIN_ID (3)
>>>    #define CDSP1_DOMAIN_ID (4)
>>> -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>> +#define GDSP0_DOMAIN_ID (5)
>>> +#define GDSP1_DOMAIN_ID (6)
>>
>> We have already made the driver look silly here, Lets not add domain ids for
>> each instance, which is not a scalable.
>>
>> Domain ids are strictly for a domain not each instance.
>>
>>
>>> +#define FASTRPC_DEV_MAX		7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>>>    #define FASTRPC_MAX_SESSIONS	14
>>>    #define FASTRPC_MAX_VMIDS	16
>>>    #define FASTRPC_ALIGN		128
>>> @@ -107,7 +109,9 @@
>>>    #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>    static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>> -						"sdsp", "cdsp", "cdsp1" };
>>> +						"sdsp", "cdsp",
>>> +						"cdsp1", "gdsp0",
>>> +						"gdsp1" };
>>>    struct fastrpc_phy_page {
>>>    	u64 addr;		/* physical address */
>>>    	u64 size;		/* size of contiguous region */
>>> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>    		break;
>>>    	case CDSP_DOMAIN_ID:
>>>    	case CDSP1_DOMAIN_ID:
>>> +	case GDSP0_DOMAIN_ID:
>>> +	case GDSP1_DOMAIN_ID:
>>>    		data->unsigned_support = true;
>>>    		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>>    		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>>
>>
>> Can you try this patch: only compile tested.
>>
>> ---------------------------------->cut<---------------------------------------
>>  From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Date: Thu, 20 Mar 2025 17:07:05 +0000
>> Subject: [PATCH] misc: fastrpc: cleanup the domain names
>>
>> Currently the domain ids are added for each instance of domain, this is
>> totally not scalable approch.
>>
>> Clean this mess and create domain ids for only domains not its
>> instances.
>> This patch also moves the domain ids to uapi header as this is required
>> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
>>   include/uapi/misc/fastrpc.h |  7 ++++++
>>   2 files changed, 32 insertions(+), 20 deletions(-)
>>
> 
> 
>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>> index f33d914d8f46..89516abd258f 100644
>> --- a/include/uapi/misc/fastrpc.h
>> +++ b/include/uapi/misc/fastrpc.h
>> @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
>>   	__s32 reserved[5];
>>   };
>>
>> +#define ADSP_DOMAIN_ID (0)
>> +#define MDSP_DOMAIN_ID (1)
>> +#define SDSP_DOMAIN_ID (2)
>> +#define CDSP_DOMAIN_ID (3)
>> +#define GDSP_DOMAIN_ID (4)
> 
> Why are you adding these to uAPI? How are they going to be used by the
> userspace?
> 
>> +
>> +#define FASTRPC_DOMAIN_MAX	4
>>   struct fastrpc_ioctl_capability {
>>   	__u32 domain;

here, in domain value of fastrpc_ioctl_capability.



>>   	__u32 attribute_id;
>> -- 
>> 2.25.1
>>
>>
>> ---------------------------------->cut<---------------------------------------
> 

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Dmitry Baryshkov]

On Mon, Mar 24, 2025 at 01:29:30PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 21/03/2025 14:07, Dmitry Baryshkov wrote:
> > On Thu, Mar 20, 2025 at 05:11:20PM +0000, Srinivas Kandagatla wrote:
> > > 
> > > 
> > > On 20/03/2025 09:14, Ling Xu wrote:
> > > > The fastrpc driver has support for 5 types of remoteprocs. There are
> > > > some products which support GPDSP remoteprocs. Add changes to support
> > > > GPDSP remoteprocs.
> > > > 
> > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > > Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> > > > ---
> > > >    drivers/misc/fastrpc.c | 10 ++++++++--
> > > >    1 file changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> > > > index 7b7a22c91fe4..80aa554b3042 100644
> > > > --- a/drivers/misc/fastrpc.c
> > > > +++ b/drivers/misc/fastrpc.c
> > > > @@ -28,7 +28,9 @@
> > > >    #define SDSP_DOMAIN_ID (2)
> > > >    #define CDSP_DOMAIN_ID (3)
> > > >    #define CDSP1_DOMAIN_ID (4)
> > > > -#define FASTRPC_DEV_MAX		5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> > > > +#define GDSP0_DOMAIN_ID (5)
> > > > +#define GDSP1_DOMAIN_ID (6)
> > > 
> > > We have already made the driver look silly here, Lets not add domain ids for
> > > each instance, which is not a scalable.
> > > 
> > > Domain ids are strictly for a domain not each instance.
> > > 
> > > 
> > > > +#define FASTRPC_DEV_MAX		7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
> > > >    #define FASTRPC_MAX_SESSIONS	14
> > > >    #define FASTRPC_MAX_VMIDS	16
> > > >    #define FASTRPC_ALIGN		128
> > > > @@ -107,7 +109,9 @@
> > > >    #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
> > > >    static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> > > > -						"sdsp", "cdsp", "cdsp1" };
> > > > +						"sdsp", "cdsp",
> > > > +						"cdsp1", "gdsp0",
> > > > +						"gdsp1" };
> > > >    struct fastrpc_phy_page {
> > > >    	u64 addr;		/* physical address */
> > > >    	u64 size;		/* size of contiguous region */
> > > > @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> > > >    		break;
> > > >    	case CDSP_DOMAIN_ID:
> > > >    	case CDSP1_DOMAIN_ID:
> > > > +	case GDSP0_DOMAIN_ID:
> > > > +	case GDSP1_DOMAIN_ID:
> > > >    		data->unsigned_support = true;
> > > >    		/* Create both device nodes so that we can allow both Signed and Unsigned PD */
> > > >    		err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> > > 
> > > 
> > > Can you try this patch: only compile tested.
> > > 
> > > ---------------------------------->cut<---------------------------------------
> > >  From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
> > > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > Date: Thu, 20 Mar 2025 17:07:05 +0000
> > > Subject: [PATCH] misc: fastrpc: cleanup the domain names
> > > 
> > > Currently the domain ids are added for each instance of domain, this is
> > > totally not scalable approch.
> > > 
> > > Clean this mess and create domain ids for only domains not its
> > > instances.
> > > This patch also moves the domain ids to uapi header as this is required
> > > for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
> > > 
> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > ---
> > >   drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
> > >   include/uapi/misc/fastrpc.h |  7 ++++++
> > >   2 files changed, 32 insertions(+), 20 deletions(-)
> > > 
> > 
> > 
> > > diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> > > index f33d914d8f46..89516abd258f 100644
> > > --- a/include/uapi/misc/fastrpc.h
> > > +++ b/include/uapi/misc/fastrpc.h
> > > @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
> > >   	__s32 reserved[5];
> > >   };
> > > 
> > > +#define ADSP_DOMAIN_ID (0)
> > > +#define MDSP_DOMAIN_ID (1)
> > > +#define SDSP_DOMAIN_ID (2)
> > > +#define CDSP_DOMAIN_ID (3)
> > > +#define GDSP_DOMAIN_ID (4)
> > 
> > Why are you adding these to uAPI? How are they going to be used by the
> > userspace?
> > 
> > > +
> > > +#define FASTRPC_DOMAIN_MAX	4
> > >   struct fastrpc_ioctl_capability {
> > >   	__u32 domain;
> 
> here, in domain value of fastrpc_ioctl_capability.

I see. I'd say, moving those IDs to a header is a separate change. Also
please document that this patch changes the user-visible value for CDSP1
DSP.

> 
> 
> 
> > >   	__u32 attribute_id;
> > > -- 
> > > 2.25.1
> > > 
> > > 
> > > ---------------------------------->cut<---------------------------------------
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Ling Xu]

在 3/21/2025 1:11 AM, Srinivas Kandagatla 写道:
> 
> 
> On 20/03/2025 09:14, Ling Xu wrote:
>> The fastrpc driver has support for 5 types of remoteprocs. There are
>> some products which support GPDSP remoteprocs. Add changes to support
>> GPDSP remoteprocs.
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>> ---
>>   drivers/misc/fastrpc.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 7b7a22c91fe4..80aa554b3042 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -28,7 +28,9 @@
>>   #define SDSP_DOMAIN_ID (2)
>>   #define CDSP_DOMAIN_ID (3)
>>   #define CDSP1_DOMAIN_ID (4)
>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>> +#define GDSP0_DOMAIN_ID (5)
>> +#define GDSP1_DOMAIN_ID (6)
> 
> We have already made the driver look silly here, Lets not add domain ids for each instance, which is not a scalable.
> 
> Domain ids are strictly for a domain not each instance.
> 
> 
>> +#define FASTRPC_DEV_MAX        7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>>   #define FASTRPC_MAX_SESSIONS    14
>>   #define FASTRPC_MAX_VMIDS    16
>>   #define FASTRPC_ALIGN        128
>> @@ -107,7 +109,9 @@
>>   #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>     static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>> -                        "sdsp", "cdsp", "cdsp1" };
>> +                        "sdsp", "cdsp",
>> +                        "cdsp1", "gdsp0",
>> +                        "gdsp1" };
>>   struct fastrpc_phy_page {
>>       u64 addr;        /* physical address */
>>       u64 size;        /* size of contiguous region */
>> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>           break;
>>       case CDSP_DOMAIN_ID:
>>       case CDSP1_DOMAIN_ID:
>> +    case GDSP0_DOMAIN_ID:
>> +    case GDSP1_DOMAIN_ID:
>>           data->unsigned_support = true;
>>           /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>           err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> 
> 
> Can you try this patch: only compile tested.
> 
> ---------------------------------->cut<---------------------------------------
> From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Date: Thu, 20 Mar 2025 17:07:05 +0000
> Subject: [PATCH] misc: fastrpc: cleanup the domain names
> 
> Currently the domain ids are added for each instance of domain, this is
> totally not scalable approch.
> 
> Clean this mess and create domain ids for only domains not its
> instances.
> This patch also moves the domain ids to uapi header as this is required
> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
>  include/uapi/misc/fastrpc.h |  7 ++++++
>  2 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 7b7a22c91fe4..b3932897a437 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -23,12 +23,6 @@
>  #include <uapi/misc/fastrpc.h>
>  #include <linux/of_reserved_mem.h>
> 
> -#define ADSP_DOMAIN_ID (0)
> -#define MDSP_DOMAIN_ID (1)
> -#define SDSP_DOMAIN_ID (2)
> -#define CDSP_DOMAIN_ID (3)
> -#define CDSP1_DOMAIN_ID (4)
> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>  #define FASTRPC_MAX_SESSIONS    14
>  #define FASTRPC_MAX_VMIDS    16
>  #define FASTRPC_ALIGN        128
> @@ -106,8 +100,6 @@
> 
>  #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
> 
> -static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> -                        "sdsp", "cdsp", "cdsp1" };
>  struct fastrpc_phy_page {
>      u64 addr;        /* physical address */
>      u64 size;        /* size of contiguous region */
> @@ -1769,7 +1761,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>          return  -EFAULT;
> 
>      cap.capability = 0;
> -    if (cap.domain >= FASTRPC_DEV_MAX) {
> +    if (cap.domain >= FASTRPC_DOMAIN_MAX) {
>          dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
>              cap.domain, err);
>          return -ECHRNG;

I tested this patch and saw one issue.
Here FASTRPC_DOMAIN_MAX is set to 4, but in userspace, cdsp1 is 4, gdsp0 is 5 and gdsp1 is 6.
For example, if we run a demo on gdsp0, cap.domain copied from userspace will be 5 which could lead to wrong message.

--Ling Xu

> @@ -2255,6 +2247,24 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>      return err;
>  }
> 
> +static int fastrpc_get_domain_id(const char *domain)
> +{
> +    if (strncmp(domain, "adsp", 4) == 0) {
> +        return ADSP_DOMAIN_ID;
> +    } else    if (strncmp(domain, "cdsp", 4) == 0) {
> +        return CDSP_DOMAIN_ID;
> +    } else if (strncmp(domain, "mdsp", 4) ==0) {
> +        return MDSP_DOMAIN_ID;
> +    } else if (strncmp(domain, "sdsp", 4) ==0) {
> +        return SDSP_DOMAIN_ID;
> +    } else if (strncmp(domain, "gdsp", 4) ==0) {
> +        return GDSP_DOMAIN_ID;
> +    }
> +
> +    return -EINVAL;
> +
> +}
> +
>  static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  {
>      struct device *rdev = &rpdev->dev;
> @@ -2272,15 +2282,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>          return err;
>      }
> 
> -    for (i = 0; i < FASTRPC_DEV_MAX; i++) {
> -        if (!strcmp(domains[i], domain)) {
> -            domain_id = i;
> -            break;
> -        }
> -    }
> +    domain_id = fastrpc_get_domain_id(domain);
> 
>      if (domain_id < 0) {
> -        dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
> +        dev_info(rdev, "FastRPC Domain %s not supported\n", domain);
>          return -EINVAL;
>      }
> 
> @@ -2332,19 +2337,19 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>      case SDSP_DOMAIN_ID:
>          /* Unsigned PD offloading is only supported on CDSP and CDSP1 */
>          data->unsigned_support = false;
> -        err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
> +        err = fastrpc_device_register(rdev, data, secure_dsp, domain);
>          if (err)
>              goto fdev_error;
>          break;
>      case CDSP_DOMAIN_ID:
> -    case CDSP1_DOMAIN_ID:
> +    case GDSP_DOMAIN_ID:
>          data->unsigned_support = true;
>          /* Create both device nodes so that we can allow both Signed and Unsigned PD */
> -        err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> +        err = fastrpc_device_register(rdev, data, true, domain);
>          if (err)
>              goto fdev_error;
> 
> -        err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
> +        err = fastrpc_device_register(rdev, data, false, domain);
>          if (err)
>              goto populate_error;
>          break;
> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> index f33d914d8f46..89516abd258f 100644
> --- a/include/uapi/misc/fastrpc.h
> +++ b/include/uapi/misc/fastrpc.h
> @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
>      __s32 reserved[5];
>  };
> 
> +#define ADSP_DOMAIN_ID (0)
> +#define MDSP_DOMAIN_ID (1)
> +#define SDSP_DOMAIN_ID (2)
> +#define CDSP_DOMAIN_ID (3)
> +#define GDSP_DOMAIN_ID (4)
> +
> +#define FASTRPC_DOMAIN_MAX    4
>  struct fastrpc_ioctl_capability {
>      __u32 domain;
>      __u32 attribute_id;

-- 
Thx and BRs,
Ling Xu

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Srinivas Kandagatla]

On 07/04/2025 10:13, Ling Xu wrote:
> 在 3/21/2025 1:11 AM, Srinivas Kandagatla 写道:
>>
>>
>> On 20/03/2025 09:14, Ling Xu wrote:
>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>> some products which support GPDSP remoteprocs. Add changes to support
>>> GPDSP remoteprocs.
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>> ---
>>>    drivers/misc/fastrpc.c | 10 ++++++++--
>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 7b7a22c91fe4..80aa554b3042 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -28,7 +28,9 @@
>>>    #define SDSP_DOMAIN_ID (2)
>>>    #define CDSP_DOMAIN_ID (3)
>>>    #define CDSP1_DOMAIN_ID (4)
>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>> +#define GDSP0_DOMAIN_ID (5)
>>> +#define GDSP1_DOMAIN_ID (6)
>>
>> We have already made the driver look silly here, Lets not add domain ids for each instance, which is not a scalable.
>>
>> Domain ids are strictly for a domain not each instance.
>>
>>
>>> +#define FASTRPC_DEV_MAX        7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>>>    #define FASTRPC_MAX_SESSIONS    14
>>>    #define FASTRPC_MAX_VMIDS    16
>>>    #define FASTRPC_ALIGN        128
>>> @@ -107,7 +109,9 @@
>>>    #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>      static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>> -                        "sdsp", "cdsp", "cdsp1" };
>>> +                        "sdsp", "cdsp",
>>> +                        "cdsp1", "gdsp0",
>>> +                        "gdsp1" };
>>>    struct fastrpc_phy_page {
>>>        u64 addr;        /* physical address */
>>>        u64 size;        /* size of contiguous region */
>>> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>            break;
>>>        case CDSP_DOMAIN_ID:
>>>        case CDSP1_DOMAIN_ID:
>>> +    case GDSP0_DOMAIN_ID:
>>> +    case GDSP1_DOMAIN_ID:
>>>            data->unsigned_support = true;
>>>            /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>>            err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>>
>>
>> Can you try this patch: only compile tested.
>>
>> ---------------------------------->cut<---------------------------------------
>>  From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Date: Thu, 20 Mar 2025 17:07:05 +0000
>> Subject: [PATCH] misc: fastrpc: cleanup the domain names
>>
>> Currently the domain ids are added for each instance of domain, this is
>> totally not scalable approch.
>>
>> Clean this mess and create domain ids for only domains not its
>> instances.
>> This patch also moves the domain ids to uapi header as this is required
>> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
>>   include/uapi/misc/fastrpc.h |  7 ++++++
>>   2 files changed, 32 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 7b7a22c91fe4..b3932897a437 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -23,12 +23,6 @@
>>   #include <uapi/misc/fastrpc.h>
>>   #include <linux/of_reserved_mem.h>
>>
>> -#define ADSP_DOMAIN_ID (0)
>> -#define MDSP_DOMAIN_ID (1)
>> -#define SDSP_DOMAIN_ID (2)
>> -#define CDSP_DOMAIN_ID (3)
>> -#define CDSP1_DOMAIN_ID (4)
>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>   #define FASTRPC_MAX_SESSIONS    14
>>   #define FASTRPC_MAX_VMIDS    16
>>   #define FASTRPC_ALIGN        128
>> @@ -106,8 +100,6 @@
>>
>>   #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>
>> -static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>> -                        "sdsp", "cdsp", "cdsp1" };
>>   struct fastrpc_phy_page {
>>       u64 addr;        /* physical address */
>>       u64 size;        /* size of contiguous region */
>> @@ -1769,7 +1761,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>>           return  -EFAULT;
>>
>>       cap.capability = 0;
>> -    if (cap.domain >= FASTRPC_DEV_MAX) {
>> +    if (cap.domain >= FASTRPC_DOMAIN_MAX) {
>>           dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
>>               cap.domain, err);
>>           return -ECHRNG;
> 
> I tested this patch and saw one issue.
> Here FASTRPC_DOMAIN_MAX is set to 4, but in userspace, cdsp1 is 4, gdsp0 is 5 and gdsp1 is 6.


Why is the userspace using something that is not uAPI?

Why does it matter if its gdsp0 or gdsp1 for the userspace?
It should only matter if its gdsp domain or not.



--srini


> For example, if we run a demo on gdsp0, cap.domain copied from userspace will be 5 which could lead to wrong message.
> 
> --Ling Xu
> 
>> @@ -2255,6 +2247,24 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>>       return err;
>>   }
>>
>> +static int fastrpc_get_domain_id(const char *domain)
>> +{
>> +    if (strncmp(domain, "adsp", 4) == 0) {
>> +        return ADSP_DOMAIN_ID;
>> +    } else    if (strncmp(domain, "cdsp", 4) == 0) {
>> +        return CDSP_DOMAIN_ID;
>> +    } else if (strncmp(domain, "mdsp", 4) ==0) {
>> +        return MDSP_DOMAIN_ID;
>> +    } else if (strncmp(domain, "sdsp", 4) ==0) {
>> +        return SDSP_DOMAIN_ID;
>> +    } else if (strncmp(domain, "gdsp", 4) ==0) {
>> +        return GDSP_DOMAIN_ID;
>> +    }
>> +
>> +    return -EINVAL;
>> +
>> +}
>> +
>>   static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>   {
>>       struct device *rdev = &rpdev->dev;
>> @@ -2272,15 +2282,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>           return err;
>>       }
>>
>> -    for (i = 0; i < FASTRPC_DEV_MAX; i++) {
>> -        if (!strcmp(domains[i], domain)) {
>> -            domain_id = i;
>> -            break;
>> -        }
>> -    }
>> +    domain_id = fastrpc_get_domain_id(domain);
>>
>>       if (domain_id < 0) {
>> -        dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
>> +        dev_info(rdev, "FastRPC Domain %s not supported\n", domain);
>>           return -EINVAL;
>>       }
>>
>> @@ -2332,19 +2337,19 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>       case SDSP_DOMAIN_ID:
>>           /* Unsigned PD offloading is only supported on CDSP and CDSP1 */
>>           data->unsigned_support = false;
>> -        err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
>> +        err = fastrpc_device_register(rdev, data, secure_dsp, domain);
>>           if (err)
>>               goto fdev_error;
>>           break;
>>       case CDSP_DOMAIN_ID:
>> -    case CDSP1_DOMAIN_ID:
>> +    case GDSP_DOMAIN_ID:
>>           data->unsigned_support = true;
>>           /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>> -        err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>> +        err = fastrpc_device_register(rdev, data, true, domain);
>>           if (err)
>>               goto fdev_error;
>>
>> -        err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>> +        err = fastrpc_device_register(rdev, data, false, domain);
>>           if (err)
>>               goto populate_error;
>>           break;
>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>> index f33d914d8f46..89516abd258f 100644
>> --- a/include/uapi/misc/fastrpc.h
>> +++ b/include/uapi/misc/fastrpc.h
>> @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
>>       __s32 reserved[5];
>>   };
>>
>> +#define ADSP_DOMAIN_ID (0)
>> +#define MDSP_DOMAIN_ID (1)
>> +#define SDSP_DOMAIN_ID (2)
>> +#define CDSP_DOMAIN_ID (3)
>> +#define GDSP_DOMAIN_ID (4)
>> +
>> +#define FASTRPC_DOMAIN_MAX    4
>>   struct fastrpc_ioctl_capability {
>>       __u32 domain;
>>       __u32 attribute_id;
> 

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Ling Xu]

在 4/8/2025 4:14 PM, Srinivas Kandagatla 写道:
> 
> 
> On 07/04/2025 10:13, Ling Xu wrote:
>> 在 3/21/2025 1:11 AM, Srinivas Kandagatla 写道:
>>>
>>>
>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>> GPDSP remoteprocs.
>>>>
>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>>> ---
>>>>    drivers/misc/fastrpc.c | 10 ++++++++--
>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -28,7 +28,9 @@
>>>>    #define SDSP_DOMAIN_ID (2)
>>>>    #define CDSP_DOMAIN_ID (3)
>>>>    #define CDSP1_DOMAIN_ID (4)
>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>> +#define GDSP0_DOMAIN_ID (5)
>>>> +#define GDSP1_DOMAIN_ID (6)
>>>
>>> We have already made the driver look silly here, Lets not add domain ids for each instance, which is not a scalable.
>>>
>>> Domain ids are strictly for a domain not each instance.
>>>
>>>
>>>> +#define FASTRPC_DEV_MAX        7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>>>>    #define FASTRPC_MAX_SESSIONS    14
>>>>    #define FASTRPC_MAX_VMIDS    16
>>>>    #define FASTRPC_ALIGN        128
>>>> @@ -107,7 +109,9 @@
>>>>    #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>>      static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>>> -                        "sdsp", "cdsp", "cdsp1" };
>>>> +                        "sdsp", "cdsp",
>>>> +                        "cdsp1", "gdsp0",
>>>> +                        "gdsp1" };
>>>>    struct fastrpc_phy_page {
>>>>        u64 addr;        /* physical address */
>>>>        u64 size;        /* size of contiguous region */
>>>> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>            break;
>>>>        case CDSP_DOMAIN_ID:
>>>>        case CDSP1_DOMAIN_ID:
>>>> +    case GDSP0_DOMAIN_ID:
>>>> +    case GDSP1_DOMAIN_ID:
>>>>            data->unsigned_support = true;
>>>>            /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>>>            err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>>>
>>>
>>> Can you try this patch: only compile tested.
>>>
>>> ---------------------------------->cut<---------------------------------------
>>>  From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> Date: Thu, 20 Mar 2025 17:07:05 +0000
>>> Subject: [PATCH] misc: fastrpc: cleanup the domain names
>>>
>>> Currently the domain ids are added for each instance of domain, this is
>>> totally not scalable approch.
>>>
>>> Clean this mess and create domain ids for only domains not its
>>> instances.
>>> This patch also moves the domain ids to uapi header as this is required
>>> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> ---
>>>   drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
>>>   include/uapi/misc/fastrpc.h |  7 ++++++
>>>   2 files changed, 32 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 7b7a22c91fe4..b3932897a437 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -23,12 +23,6 @@
>>>   #include <uapi/misc/fastrpc.h>
>>>   #include <linux/of_reserved_mem.h>
>>>
>>> -#define ADSP_DOMAIN_ID (0)
>>> -#define MDSP_DOMAIN_ID (1)
>>> -#define SDSP_DOMAIN_ID (2)
>>> -#define CDSP_DOMAIN_ID (3)
>>> -#define CDSP1_DOMAIN_ID (4)
>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>   #define FASTRPC_MAX_SESSIONS    14
>>>   #define FASTRPC_MAX_VMIDS    16
>>>   #define FASTRPC_ALIGN        128
>>> @@ -106,8 +100,6 @@
>>>
>>>   #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>
>>> -static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>> -                        "sdsp", "cdsp", "cdsp1" };
>>>   struct fastrpc_phy_page {
>>>       u64 addr;        /* physical address */
>>>       u64 size;        /* size of contiguous region */
>>> @@ -1769,7 +1761,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>>>           return  -EFAULT;
>>>
>>>       cap.capability = 0;
>>> -    if (cap.domain >= FASTRPC_DEV_MAX) {
>>> +    if (cap.domain >= FASTRPC_DOMAIN_MAX) {
>>>           dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
>>>               cap.domain, err);
>>>           return -ECHRNG;
>>
>> I tested this patch and saw one issue.
>> Here FASTRPC_DOMAIN_MAX is set to 4, but in userspace, cdsp1 is 4, gdsp0 is 5 and gdsp1 is 6.
> 
> 
> Why is the userspace using something that is not uAPI?
> 
> Why does it matter if its gdsp0 or gdsp1 for the userspace?
> It should only matter if its gdsp domain or not.
> 

Give an example here:
In test example, user can use below API to query the notification capability of the specific domain_id,
(actually this will not have any functional issue, but just return an error and lead wrong message):
request_status_notifications_enable(domain_id, (void*)STATUS_CONTEXT, pd_status_notifier_callback)

this will call ioctl_getdspinfo in fastrpc_ioctl.c:
https://github.com/quic-lxu5/fastrpc/blob/8feccfd2eb46272ad1fabed195bfddb7fd680cbd/src/fastrpc_ioctl.c#L201

code snip:
	FARF(ALWAYS, "ioctl_getdspinfo in ioctl.c domain:%d", domain);
	ioErr = ioctl(dev, FASTRPC_IOCTL_GET_DSP_INFO, &cap);
	FARF(ALWAYS, "done ioctl_getdspinfo in ioctl.c ioErr:%x", ioErr);

and finally call fastrpc_get_dsp_info in fastrpc.c.

if I use the patch you shared, it will report below error:

UMD log:
2025-01-08T18:45:03.168718+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:201: ioctl_getdspinfo in ioctl.c domain:5
2025-01-08T18:45:03.169307+00:00 qcs9100-ride-sx calculator: log_config.c:396: file_watcher_thread starting for domain 5
2025-01-08T18:45:03.180355+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:203: done ioctl_getdspinfo in ioctl.c ioErr:ffffffff

putty log:
[ 1332.308444] qcom,fastrpc 20c00000.remoteproc:glink-edge.fastrpcglink-apps-dsp.-1.-1: Error: Invalid domain id:5, err:0

Because on the user side, gdsp0 and gdsp1 will be distinguished to 5 and 6.
so do you mean you want me to modify UMD code to transfer both gdsp0 and gdsp1 to gdsp just in ioctl_getdspinfo?
> 
> --srini
> 
> 
>> For example, if we run a demo on gdsp0, cap.domain copied from userspace will be 5 which could lead to wrong message.
>>
>> --Ling Xu
>>
>>> @@ -2255,6 +2247,24 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>>>       return err;
>>>   }
>>>
>>> +static int fastrpc_get_domain_id(const char *domain)
>>> +{
>>> +    if (strncmp(domain, "adsp", 4) == 0) {
>>> +        return ADSP_DOMAIN_ID;
>>> +    } else    if (strncmp(domain, "cdsp", 4) == 0) {
>>> +        return CDSP_DOMAIN_ID;
>>> +    } else if (strncmp(domain, "mdsp", 4) ==0) {
>>> +        return MDSP_DOMAIN_ID;
>>> +    } else if (strncmp(domain, "sdsp", 4) ==0) {
>>> +        return SDSP_DOMAIN_ID;
>>> +    } else if (strncmp(domain, "gdsp", 4) ==0) {
>>> +        return GDSP_DOMAIN_ID;
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +
>>> +}
>>> +
>>>   static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>   {
>>>       struct device *rdev = &rpdev->dev;
>>> @@ -2272,15 +2282,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>           return err;
>>>       }
>>>
>>> -    for (i = 0; i < FASTRPC_DEV_MAX; i++) {
>>> -        if (!strcmp(domains[i], domain)) {
>>> -            domain_id = i;
>>> -            break;
>>> -        }
>>> -    }
>>> +    domain_id = fastrpc_get_domain_id(domain);
>>>
>>>       if (domain_id < 0) {
>>> -        dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
>>> +        dev_info(rdev, "FastRPC Domain %s not supported\n", domain);
>>>           return -EINVAL;
>>>       }
>>>
>>> @@ -2332,19 +2337,19 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>       case SDSP_DOMAIN_ID:
>>>           /* Unsigned PD offloading is only supported on CDSP and CDSP1 */
>>>           data->unsigned_support = false;
>>> -        err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
>>> +        err = fastrpc_device_register(rdev, data, secure_dsp, domain);
>>>           if (err)
>>>               goto fdev_error;
>>>           break;
>>>       case CDSP_DOMAIN_ID:
>>> -    case CDSP1_DOMAIN_ID:
>>> +    case GDSP_DOMAIN_ID:
>>>           data->unsigned_support = true;
>>>           /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>> -        err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>>> +        err = fastrpc_device_register(rdev, data, true, domain);
>>>           if (err)
>>>               goto fdev_error;
>>>
>>> -        err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>>> +        err = fastrpc_device_register(rdev, data, false, domain);
>>>           if (err)
>>>               goto populate_error;
>>>           break;
>>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>>> index f33d914d8f46..89516abd258f 100644
>>> --- a/include/uapi/misc/fastrpc.h
>>> +++ b/include/uapi/misc/fastrpc.h
>>> @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
>>>       __s32 reserved[5];
>>>   };
>>>
>>> +#define ADSP_DOMAIN_ID (0)
>>> +#define MDSP_DOMAIN_ID (1)
>>> +#define SDSP_DOMAIN_ID (2)
>>> +#define CDSP_DOMAIN_ID (3)
>>> +#define GDSP_DOMAIN_ID (4)
>>> +
>>> +#define FASTRPC_DOMAIN_MAX    4
>>>   struct fastrpc_ioctl_capability {
>>>       __u32 domain;
>>>       __u32 attribute_id;
>>

-- 
Thx and BRs,
Ling Xu

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Dmitry Baryshkov]

On 16/06/2025 14:28, Ling Xu wrote:
> 在 4/8/2025 4:14 PM, Srinivas Kandagatla 写道:
>>
>>
>> On 07/04/2025 10:13, Ling Xu wrote:
>>> 在 3/21/2025 1:11 AM, Srinivas Kandagatla 写道:
>>>>
>>>>
>>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>>> GPDSP remoteprocs.
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>>>> ---
>>>>>     drivers/misc/fastrpc.c | 10 ++++++++--
>>>>>     1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>>> --- a/drivers/misc/fastrpc.c
>>>>> +++ b/drivers/misc/fastrpc.c
>>>>> @@ -28,7 +28,9 @@
>>>>>     #define SDSP_DOMAIN_ID (2)
>>>>>     #define CDSP_DOMAIN_ID (3)
>>>>>     #define CDSP1_DOMAIN_ID (4)
>>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>>> +#define GDSP0_DOMAIN_ID (5)
>>>>> +#define GDSP1_DOMAIN_ID (6)
>>>>
>>>> We have already made the driver look silly here, Lets not add domain ids for each instance, which is not a scalable.
>>>>
>>>> Domain ids are strictly for a domain not each instance.
>>>>
>>>>
>>>>> +#define FASTRPC_DEV_MAX        7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>>>>>     #define FASTRPC_MAX_SESSIONS    14
>>>>>     #define FASTRPC_MAX_VMIDS    16
>>>>>     #define FASTRPC_ALIGN        128
>>>>> @@ -107,7 +109,9 @@
>>>>>     #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>>>       static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>>>> -                        "sdsp", "cdsp", "cdsp1" };
>>>>> +                        "sdsp", "cdsp",
>>>>> +                        "cdsp1", "gdsp0",
>>>>> +                        "gdsp1" };
>>>>>     struct fastrpc_phy_page {
>>>>>         u64 addr;        /* physical address */
>>>>>         u64 size;        /* size of contiguous region */
>>>>> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>>             break;
>>>>>         case CDSP_DOMAIN_ID:
>>>>>         case CDSP1_DOMAIN_ID:
>>>>> +    case GDSP0_DOMAIN_ID:
>>>>> +    case GDSP1_DOMAIN_ID:
>>>>>             data->unsigned_support = true;
>>>>>             /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>>>>             err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>>>>
>>>>
>>>> Can you try this patch: only compile tested.
>>>>
>>>> ---------------------------------->cut<---------------------------------------
>>>>   From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
>>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>> Date: Thu, 20 Mar 2025 17:07:05 +0000
>>>> Subject: [PATCH] misc: fastrpc: cleanup the domain names
>>>>
>>>> Currently the domain ids are added for each instance of domain, this is
>>>> totally not scalable approch.
>>>>
>>>> Clean this mess and create domain ids for only domains not its
>>>> instances.
>>>> This patch also moves the domain ids to uapi header as this is required
>>>> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
>>>>
>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>> ---
>>>>    drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
>>>>    include/uapi/misc/fastrpc.h |  7 ++++++
>>>>    2 files changed, 32 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 7b7a22c91fe4..b3932897a437 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -23,12 +23,6 @@
>>>>    #include <uapi/misc/fastrpc.h>
>>>>    #include <linux/of_reserved_mem.h>
>>>>
>>>> -#define ADSP_DOMAIN_ID (0)
>>>> -#define MDSP_DOMAIN_ID (1)
>>>> -#define SDSP_DOMAIN_ID (2)
>>>> -#define CDSP_DOMAIN_ID (3)
>>>> -#define CDSP1_DOMAIN_ID (4)
>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>>    #define FASTRPC_MAX_SESSIONS    14
>>>>    #define FASTRPC_MAX_VMIDS    16
>>>>    #define FASTRPC_ALIGN        128
>>>> @@ -106,8 +100,6 @@
>>>>
>>>>    #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>>
>>>> -static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>>> -                        "sdsp", "cdsp", "cdsp1" };
>>>>    struct fastrpc_phy_page {
>>>>        u64 addr;        /* physical address */
>>>>        u64 size;        /* size of contiguous region */
>>>> @@ -1769,7 +1761,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>>>>            return  -EFAULT;
>>>>
>>>>        cap.capability = 0;
>>>> -    if (cap.domain >= FASTRPC_DEV_MAX) {
>>>> +    if (cap.domain >= FASTRPC_DOMAIN_MAX) {
>>>>            dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
>>>>                cap.domain, err);
>>>>            return -ECHRNG;
>>>
>>> I tested this patch and saw one issue.
>>> Here FASTRPC_DOMAIN_MAX is set to 4, but in userspace, cdsp1 is 4, gdsp0 is 5 and gdsp1 is 6.
>>
>>
>> Why is the userspace using something that is not uAPI?
>>
>> Why does it matter if its gdsp0 or gdsp1 for the userspace?
>> It should only matter if its gdsp domain or not.
>>
> 
> Give an example here:
> In test example, user can use below API to query the notification capability of the specific domain_id,
> (actually this will not have any functional issue, but just return an error and lead wrong message):
> request_status_notifications_enable(domain_id, (void*)STATUS_CONTEXT, pd_status_notifier_callback)
> 
> this will call ioctl_getdspinfo in fastrpc_ioctl.c:
> https://github.com/quic-lxu5/fastrpc/blob/8feccfd2eb46272ad1fabed195bfddb7fd680cbd/src/fastrpc_ioctl.c#L201
> 
> code snip:
> 	FARF(ALWAYS, "ioctl_getdspinfo in ioctl.c domain:%d", domain);
> 	ioErr = ioctl(dev, FASTRPC_IOCTL_GET_DSP_INFO, &cap);
> 	FARF(ALWAYS, "done ioctl_getdspinfo in ioctl.c ioErr:%x", ioErr);
> 
> and finally call fastrpc_get_dsp_info in fastrpc.c.
> 
> if I use the patch you shared, it will report below error:
> 
> UMD log:
> 2025-01-08T18:45:03.168718+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:201: ioctl_getdspinfo in ioctl.c domain:5
> 2025-01-08T18:45:03.169307+00:00 qcs9100-ride-sx calculator: log_config.c:396: file_watcher_thread starting for domain 5
> 2025-01-08T18:45:03.180355+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:203: done ioctl_getdspinfo in ioctl.c ioErr:ffffffff
> 
> putty log:
> [ 1332.308444] qcom,fastrpc 20c00000.remoteproc:glink-edge.fastrpcglink-apps-dsp.-1.-1: Error: Invalid domain id:5, err:0
> 
> Because on the user side, gdsp0 and gdsp1 will be distinguished to 5 and 6.
> so do you mean you want me to modify UMD code to transfer both gdsp0 and gdsp1 to gdsp just in ioctl_getdspinfo?

No, we need to modify the kernel code to ignore cap.domain here. The 
user has already open the particular FastRPC device. All queries should 
be target that device and that domain. As such, cap.domain doesn't make 
sense and should be ignored by the kernel.

>>
>> --srini
>>
>>
>>> For example, if we run a demo on gdsp0, cap.domain copied from userspace will be 5 which could lead to wrong message.
>>>
>>> --Ling Xu
>>>
>>>> @@ -2255,6 +2247,24 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>>>>        return err;
>>>>    }
>>>>
>>>> +static int fastrpc_get_domain_id(const char *domain)
>>>> +{
>>>> +    if (strncmp(domain, "adsp", 4) == 0) {
>>>> +        return ADSP_DOMAIN_ID;
>>>> +    } else    if (strncmp(domain, "cdsp", 4) == 0) {
>>>> +        return CDSP_DOMAIN_ID;
>>>> +    } else if (strncmp(domain, "mdsp", 4) ==0) {
>>>> +        return MDSP_DOMAIN_ID;
>>>> +    } else if (strncmp(domain, "sdsp", 4) ==0) {
>>>> +        return SDSP_DOMAIN_ID;
>>>> +    } else if (strncmp(domain, "gdsp", 4) ==0) {
>>>> +        return GDSP_DOMAIN_ID;
>>>> +    }
>>>> +
>>>> +    return -EINVAL;
>>>> +
>>>> +}
>>>> +
>>>>    static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>    {
>>>>        struct device *rdev = &rpdev->dev;
>>>> @@ -2272,15 +2282,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>            return err;
>>>>        }
>>>>
>>>> -    for (i = 0; i < FASTRPC_DEV_MAX; i++) {
>>>> -        if (!strcmp(domains[i], domain)) {
>>>> -            domain_id = i;
>>>> -            break;
>>>> -        }
>>>> -    }
>>>> +    domain_id = fastrpc_get_domain_id(domain);
>>>>
>>>>        if (domain_id < 0) {
>>>> -        dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
>>>> +        dev_info(rdev, "FastRPC Domain %s not supported\n", domain);
>>>>            return -EINVAL;
>>>>        }
>>>>
>>>> @@ -2332,19 +2337,19 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>        case SDSP_DOMAIN_ID:
>>>>            /* Unsigned PD offloading is only supported on CDSP and CDSP1 */
>>>>            data->unsigned_support = false;
>>>> -        err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
>>>> +        err = fastrpc_device_register(rdev, data, secure_dsp, domain);
>>>>            if (err)
>>>>                goto fdev_error;
>>>>            break;
>>>>        case CDSP_DOMAIN_ID:
>>>> -    case CDSP1_DOMAIN_ID:
>>>> +    case GDSP_DOMAIN_ID:
>>>>            data->unsigned_support = true;
>>>>            /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>>> -        err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>>>> +        err = fastrpc_device_register(rdev, data, true, domain);
>>>>            if (err)
>>>>                goto fdev_error;
>>>>
>>>> -        err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>>>> +        err = fastrpc_device_register(rdev, data, false, domain);
>>>>            if (err)
>>>>                goto populate_error;
>>>>            break;
>>>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>>>> index f33d914d8f46..89516abd258f 100644
>>>> --- a/include/uapi/misc/fastrpc.h
>>>> +++ b/include/uapi/misc/fastrpc.h
>>>> @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
>>>>        __s32 reserved[5];
>>>>    };
>>>>
>>>> +#define ADSP_DOMAIN_ID (0)
>>>> +#define MDSP_DOMAIN_ID (1)
>>>> +#define SDSP_DOMAIN_ID (2)
>>>> +#define CDSP_DOMAIN_ID (3)
>>>> +#define GDSP_DOMAIN_ID (4)
>>>> +
>>>> +#define FASTRPC_DOMAIN_MAX    4
>>>>    struct fastrpc_ioctl_capability {
>>>>        __u32 domain;
>>>>        __u32 attribute_id;
>>>
> 


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Ling Xu]

在 6/16/2025 7:28 PM, Ling Xu 写道:
> 在 4/8/2025 4:14 PM, Srinivas Kandagatla 写道:
>>
>>
>> On 07/04/2025 10:13, Ling Xu wrote:
>>> 在 3/21/2025 1:11 AM, Srinivas Kandagatla 写道:
>>>>
>>>>
>>>> On 20/03/2025 09:14, Ling Xu wrote:
>>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
>>>>> some products which support GPDSP remoteprocs. Add changes to support
>>>>> GPDSP remoteprocs.
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
>>>>> ---
>>>>>    drivers/misc/fastrpc.c | 10 ++++++++--
>>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>>> index 7b7a22c91fe4..80aa554b3042 100644
>>>>> --- a/drivers/misc/fastrpc.c
>>>>> +++ b/drivers/misc/fastrpc.c
>>>>> @@ -28,7 +28,9 @@
>>>>>    #define SDSP_DOMAIN_ID (2)
>>>>>    #define CDSP_DOMAIN_ID (3)
>>>>>    #define CDSP1_DOMAIN_ID (4)
>>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>>> +#define GDSP0_DOMAIN_ID (5)
>>>>> +#define GDSP1_DOMAIN_ID (6)
>>>>
>>>> We have already made the driver look silly here, Lets not add domain ids for each instance, which is not a scalable.
>>>>
>>>> Domain ids are strictly for a domain not each instance.
>>>>
>>>>
>>>>> +#define FASTRPC_DEV_MAX        7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
>>>>>    #define FASTRPC_MAX_SESSIONS    14
>>>>>    #define FASTRPC_MAX_VMIDS    16
>>>>>    #define FASTRPC_ALIGN        128
>>>>> @@ -107,7 +109,9 @@
>>>>>    #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>>>      static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>>>> -                        "sdsp", "cdsp", "cdsp1" };
>>>>> +                        "sdsp", "cdsp",
>>>>> +                        "cdsp1", "gdsp0",
>>>>> +                        "gdsp1" };
>>>>>    struct fastrpc_phy_page {
>>>>>        u64 addr;        /* physical address */
>>>>>        u64 size;        /* size of contiguous region */
>>>>> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>>            break;
>>>>>        case CDSP_DOMAIN_ID:
>>>>>        case CDSP1_DOMAIN_ID:
>>>>> +    case GDSP0_DOMAIN_ID:
>>>>> +    case GDSP1_DOMAIN_ID:
>>>>>            data->unsigned_support = true;
>>>>>            /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>>>>            err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>>>>
>>>>
>>>> Can you try this patch: only compile tested.
>>>>
>>>> ---------------------------------->cut<---------------------------------------
>>>>  From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
>>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>> Date: Thu, 20 Mar 2025 17:07:05 +0000
>>>> Subject: [PATCH] misc: fastrpc: cleanup the domain names
>>>>
>>>> Currently the domain ids are added for each instance of domain, this is
>>>> totally not scalable approch.
>>>>
>>>> Clean this mess and create domain ids for only domains not its
>>>> instances.
>>>> This patch also moves the domain ids to uapi header as this is required
>>>> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
>>>>
>>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>>> ---
>>>>   drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
>>>>   include/uapi/misc/fastrpc.h |  7 ++++++
>>>>   2 files changed, 32 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>>> index 7b7a22c91fe4..b3932897a437 100644
>>>> --- a/drivers/misc/fastrpc.c
>>>> +++ b/drivers/misc/fastrpc.c
>>>> @@ -23,12 +23,6 @@
>>>>   #include <uapi/misc/fastrpc.h>
>>>>   #include <linux/of_reserved_mem.h>
>>>>
>>>> -#define ADSP_DOMAIN_ID (0)
>>>> -#define MDSP_DOMAIN_ID (1)
>>>> -#define SDSP_DOMAIN_ID (2)
>>>> -#define CDSP_DOMAIN_ID (3)
>>>> -#define CDSP1_DOMAIN_ID (4)
>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
>>>>   #define FASTRPC_MAX_SESSIONS    14
>>>>   #define FASTRPC_MAX_VMIDS    16
>>>>   #define FASTRPC_ALIGN        128
>>>> @@ -106,8 +100,6 @@
>>>>
>>>>   #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>>>
>>>> -static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>>> -                        "sdsp", "cdsp", "cdsp1" };
>>>>   struct fastrpc_phy_page {
>>>>       u64 addr;        /* physical address */
>>>>       u64 size;        /* size of contiguous region */
>>>> @@ -1769,7 +1761,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>>>>           return  -EFAULT;
>>>>
>>>>       cap.capability = 0;
>>>> -    if (cap.domain >= FASTRPC_DEV_MAX) {
>>>> +    if (cap.domain >= FASTRPC_DOMAIN_MAX) {
>>>>           dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
>>>>               cap.domain, err);
>>>>           return -ECHRNG;
>>>
>>> I tested this patch and saw one issue.
>>> Here FASTRPC_DOMAIN_MAX is set to 4, but in userspace, cdsp1 is 4, gdsp0 is 5 and gdsp1 is 6.
>>
>>
>> Why is the userspace using something that is not uAPI?
>>
>> Why does it matter if its gdsp0 or gdsp1 for the userspace?
>> It should only matter if its gdsp domain or not.
>>
> 
> Give an example here:
> In test example, user can use below API to query the notification capability of the specific domain_id,
> (actually this will not have any functional issue, but just return an error and lead wrong message):
> request_status_notifications_enable(domain_id, (void*)STATUS_CONTEXT, pd_status_notifier_callback)
> 
> this will call ioctl_getdspinfo in fastrpc_ioctl.c:
> https://github.com/quic-lxu5/fastrpc/blob/8feccfd2eb46272ad1fabed195bfddb7fd680cbd/src/fastrpc_ioctl.c#L201
> 
> code snip:
> 	FARF(ALWAYS, "ioctl_getdspinfo in ioctl.c domain:%d", domain);
> 	ioErr = ioctl(dev, FASTRPC_IOCTL_GET_DSP_INFO, &cap);
> 	FARF(ALWAYS, "done ioctl_getdspinfo in ioctl.c ioErr:%x", ioErr);
> 
> and finally call fastrpc_get_dsp_info in fastrpc.c.
> 
> if I use the patch you shared, it will report below error:
> 
> UMD log:
> 2025-01-08T18:45:03.168718+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:201: ioctl_getdspinfo in ioctl.c domain:5
> 2025-01-08T18:45:03.169307+00:00 qcs9100-ride-sx calculator: log_config.c:396: file_watcher_thread starting for domain 5
> 2025-01-08T18:45:03.180355+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:203: done ioctl_getdspinfo in ioctl.c ioErr:ffffffff
> 
> putty log:
> [ 1332.308444] qcom,fastrpc 20c00000.remoteproc:glink-edge.fastrpcglink-apps-dsp.-1.-1: Error: Invalid domain id:5, err:0
> 
> Because on the user side, gdsp0 and gdsp1 will be distinguished to 5 and 6.
> so do you mean you want me to modify UMD code to transfer both gdsp0 and gdsp1 to gdsp just in ioctl_getdspinfo?
>>

There is no error message after removing these lines based on srini's patch, is this modification appropriate? If so, I will submit a new patch.
@@ -1771,17 +1763,6 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
                return  -EFAULT;

        cap.capability = 0;
-       if (cap.domain >= FASTRPC_DEV_MAX) {
-               dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
-                       cap.domain, err);
-               return -ECHRNG;
-       }
-
-       /* Fastrpc Capablities does not support modem domain */
-       if (cap.domain == MDSP_DOMAIN_ID) {
-               dev_err(&fl->cctx->rpdev->dev, "Error: modem not supported %d\n", err);
-               return -ECHRNG;
-       }

        if (cap.attribute_id >= FASTRPC_MAX_DSP_ATTRIBUTES) {
                dev_err(&fl->cctx->rpdev->dev, "Error: invalid attribute: %d, err: %d\n",

>> --srini
>>
>>
>>> For example, if we run a demo on gdsp0, cap.domain copied from userspace will be 5 which could lead to wrong message.
>>>
>>> --Ling Xu
>>>
>>>> @@ -2255,6 +2247,24 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct
>>>>       return err;
>>>>   }
>>>>
>>>> +static int fastrpc_get_domain_id(const char *domain)
>>>> +{
>>>> +    if (strncmp(domain, "adsp", 4) == 0) {
>>>> +        return ADSP_DOMAIN_ID;
>>>> +    } else    if (strncmp(domain, "cdsp", 4) == 0) {
>>>> +        return CDSP_DOMAIN_ID;
>>>> +    } else if (strncmp(domain, "mdsp", 4) ==0) {
>>>> +        return MDSP_DOMAIN_ID;
>>>> +    } else if (strncmp(domain, "sdsp", 4) ==0) {
>>>> +        return SDSP_DOMAIN_ID;
>>>> +    } else if (strncmp(domain, "gdsp", 4) ==0) {
>>>> +        return GDSP_DOMAIN_ID;
>>>> +    }
>>>> +
>>>> +    return -EINVAL;
>>>> +
>>>> +}
>>>> +
>>>>   static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>   {
>>>>       struct device *rdev = &rpdev->dev;
>>>> @@ -2272,15 +2282,10 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>           return err;
>>>>       }
>>>>
>>>> -    for (i = 0; i < FASTRPC_DEV_MAX; i++) {
>>>> -        if (!strcmp(domains[i], domain)) {
>>>> -            domain_id = i;
>>>> -            break;
>>>> -        }
>>>> -    }
>>>> +    domain_id = fastrpc_get_domain_id(domain);
>>>>
>>>>       if (domain_id < 0) {
>>>> -        dev_info(rdev, "FastRPC Invalid Domain ID %d\n", domain_id);
>>>> +        dev_info(rdev, "FastRPC Domain %s not supported\n", domain);
>>>>           return -EINVAL;
>>>>       }
>>>>
>>>> @@ -2332,19 +2337,19 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>>>>       case SDSP_DOMAIN_ID:
>>>>           /* Unsigned PD offloading is only supported on CDSP and CDSP1 */
>>>>           data->unsigned_support = false;
>>>> -        err = fastrpc_device_register(rdev, data, secure_dsp, domains[domain_id]);
>>>> +        err = fastrpc_device_register(rdev, data, secure_dsp, domain);
>>>>           if (err)
>>>>               goto fdev_error;
>>>>           break;
>>>>       case CDSP_DOMAIN_ID:
>>>> -    case CDSP1_DOMAIN_ID:
>>>> +    case GDSP_DOMAIN_ID:
>>>>           data->unsigned_support = true;
>>>>           /* Create both device nodes so that we can allow both Signed and Unsigned PD */
>>>> -        err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
>>>> +        err = fastrpc_device_register(rdev, data, true, domain);
>>>>           if (err)
>>>>               goto fdev_error;
>>>>
>>>> -        err = fastrpc_device_register(rdev, data, false, domains[domain_id]);
>>>> +        err = fastrpc_device_register(rdev, data, false, domain);
>>>>           if (err)
>>>>               goto populate_error;
>>>>           break;
>>>> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
>>>> index f33d914d8f46..89516abd258f 100644
>>>> --- a/include/uapi/misc/fastrpc.h
>>>> +++ b/include/uapi/misc/fastrpc.h
>>>> @@ -133,6 +133,13 @@ struct fastrpc_mem_unmap {
>>>>       __s32 reserved[5];
>>>>   };
>>>>
>>>> +#define ADSP_DOMAIN_ID (0)
>>>> +#define MDSP_DOMAIN_ID (1)
>>>> +#define SDSP_DOMAIN_ID (2)
>>>> +#define CDSP_DOMAIN_ID (3)
>>>> +#define GDSP_DOMAIN_ID (4)
>>>> +
>>>> +#define FASTRPC_DOMAIN_MAX    4
>>>>   struct fastrpc_ioctl_capability {
>>>>       __u32 domain;
>>>>       __u32 attribute_id;
>>>
> 

-- 
Thx and BRs,
Ling Xu

Re: [PATCH v2 2/3] misc: fastrpc: add support for gpdsp remoteproc

[Dmitry Baryshkov]

On Wed, Jun 18, 2025 at 01:15:16PM +0800, Ling Xu wrote:
> 在 6/16/2025 7:28 PM, Ling Xu 写道:
> > 在 4/8/2025 4:14 PM, Srinivas Kandagatla 写道:
> >>
> >>
> >> On 07/04/2025 10:13, Ling Xu wrote:
> >>> 在 3/21/2025 1:11 AM, Srinivas Kandagatla 写道:
> >>>>
> >>>>
> >>>> On 20/03/2025 09:14, Ling Xu wrote:
> >>>>> The fastrpc driver has support for 5 types of remoteprocs. There are
> >>>>> some products which support GPDSP remoteprocs. Add changes to support
> >>>>> GPDSP remoteprocs.
> >>>>>
> >>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> >>>>> Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
> >>>>> ---
> >>>>>    drivers/misc/fastrpc.c | 10 ++++++++--
> >>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >>>>> index 7b7a22c91fe4..80aa554b3042 100644
> >>>>> --- a/drivers/misc/fastrpc.c
> >>>>> +++ b/drivers/misc/fastrpc.c
> >>>>> @@ -28,7 +28,9 @@
> >>>>>    #define SDSP_DOMAIN_ID (2)
> >>>>>    #define CDSP_DOMAIN_ID (3)
> >>>>>    #define CDSP1_DOMAIN_ID (4)
> >>>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> >>>>> +#define GDSP0_DOMAIN_ID (5)
> >>>>> +#define GDSP1_DOMAIN_ID (6)
> >>>>
> >>>> We have already made the driver look silly here, Lets not add domain ids for each instance, which is not a scalable.
> >>>>
> >>>> Domain ids are strictly for a domain not each instance.
> >>>>
> >>>>
> >>>>> +#define FASTRPC_DEV_MAX        7 /* adsp, mdsp, slpi, cdsp, cdsp1, gdsp0, gdsp1 */
> >>>>>    #define FASTRPC_MAX_SESSIONS    14
> >>>>>    #define FASTRPC_MAX_VMIDS    16
> >>>>>    #define FASTRPC_ALIGN        128
> >>>>> @@ -107,7 +109,9 @@
> >>>>>    #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
> >>>>>      static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> >>>>> -                        "sdsp", "cdsp", "cdsp1" };
> >>>>> +                        "sdsp", "cdsp",
> >>>>> +                        "cdsp1", "gdsp0",
> >>>>> +                        "gdsp1" };
> >>>>>    struct fastrpc_phy_page {
> >>>>>        u64 addr;        /* physical address */
> >>>>>        u64 size;        /* size of contiguous region */
> >>>>> @@ -2338,6 +2342,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
> >>>>>            break;
> >>>>>        case CDSP_DOMAIN_ID:
> >>>>>        case CDSP1_DOMAIN_ID:
> >>>>> +    case GDSP0_DOMAIN_ID:
> >>>>> +    case GDSP1_DOMAIN_ID:
> >>>>>            data->unsigned_support = true;
> >>>>>            /* Create both device nodes so that we can allow both Signed and Unsigned PD */
> >>>>>            err = fastrpc_device_register(rdev, data, true, domains[domain_id]);
> >>>>
> >>>>
> >>>> Can you try this patch: only compile tested.
> >>>>
> >>>> ---------------------------------->cut<---------------------------------------
> >>>>  From 3f8607557162e16673b26fa253d11cafdc4444cf Mon Sep 17 00:00:00 2001
> >>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> >>>> Date: Thu, 20 Mar 2025 17:07:05 +0000
> >>>> Subject: [PATCH] misc: fastrpc: cleanup the domain names
> >>>>
> >>>> Currently the domain ids are added for each instance of domain, this is
> >>>> totally not scalable approch.
> >>>>
> >>>> Clean this mess and create domain ids for only domains not its
> >>>> instances.
> >>>> This patch also moves the domain ids to uapi header as this is required
> >>>> for FASTRPC_IOCTL_GET_DSP_INFO ioctl.
> >>>>
> >>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> >>>> ---
> >>>>   drivers/misc/fastrpc.c      | 45 ++++++++++++++++++++-----------------
> >>>>   include/uapi/misc/fastrpc.h |  7 ++++++
> >>>>   2 files changed, 32 insertions(+), 20 deletions(-)
> >>>>
> >>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> >>>> index 7b7a22c91fe4..b3932897a437 100644
> >>>> --- a/drivers/misc/fastrpc.c
> >>>> +++ b/drivers/misc/fastrpc.c
> >>>> @@ -23,12 +23,6 @@
> >>>>   #include <uapi/misc/fastrpc.h>
> >>>>   #include <linux/of_reserved_mem.h>
> >>>>
> >>>> -#define ADSP_DOMAIN_ID (0)
> >>>> -#define MDSP_DOMAIN_ID (1)
> >>>> -#define SDSP_DOMAIN_ID (2)
> >>>> -#define CDSP_DOMAIN_ID (3)
> >>>> -#define CDSP1_DOMAIN_ID (4)
> >>>> -#define FASTRPC_DEV_MAX        5 /* adsp, mdsp, slpi, cdsp, cdsp1 */
> >>>>   #define FASTRPC_MAX_SESSIONS    14
> >>>>   #define FASTRPC_MAX_VMIDS    16
> >>>>   #define FASTRPC_ALIGN        128
> >>>> @@ -106,8 +100,6 @@
> >>>>
> >>>>   #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
> >>>>
> >>>> -static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
> >>>> -                        "sdsp", "cdsp", "cdsp1" };
> >>>>   struct fastrpc_phy_page {
> >>>>       u64 addr;        /* physical address */
> >>>>       u64 size;        /* size of contiguous region */
> >>>> @@ -1769,7 +1761,7 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
> >>>>           return  -EFAULT;
> >>>>
> >>>>       cap.capability = 0;
> >>>> -    if (cap.domain >= FASTRPC_DEV_MAX) {
> >>>> +    if (cap.domain >= FASTRPC_DOMAIN_MAX) {
> >>>>           dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
> >>>>               cap.domain, err);
> >>>>           return -ECHRNG;
> >>>
> >>> I tested this patch and saw one issue.
> >>> Here FASTRPC_DOMAIN_MAX is set to 4, but in userspace, cdsp1 is 4, gdsp0 is 5 and gdsp1 is 6.
> >>
> >>
> >> Why is the userspace using something that is not uAPI?
> >>
> >> Why does it matter if its gdsp0 or gdsp1 for the userspace?
> >> It should only matter if its gdsp domain or not.
> >>
> > 
> > Give an example here:
> > In test example, user can use below API to query the notification capability of the specific domain_id,
> > (actually this will not have any functional issue, but just return an error and lead wrong message):
> > request_status_notifications_enable(domain_id, (void*)STATUS_CONTEXT, pd_status_notifier_callback)
> > 
> > this will call ioctl_getdspinfo in fastrpc_ioctl.c:
> > https://github.com/quic-lxu5/fastrpc/blob/8feccfd2eb46272ad1fabed195bfddb7fd680cbd/src/fastrpc_ioctl.c#L201
> > 
> > code snip:
> > 	FARF(ALWAYS, "ioctl_getdspinfo in ioctl.c domain:%d", domain);
> > 	ioErr = ioctl(dev, FASTRPC_IOCTL_GET_DSP_INFO, &cap);
> > 	FARF(ALWAYS, "done ioctl_getdspinfo in ioctl.c ioErr:%x", ioErr);
> > 
> > and finally call fastrpc_get_dsp_info in fastrpc.c.
> > 
> > if I use the patch you shared, it will report below error:
> > 
> > UMD log:
> > 2025-01-08T18:45:03.168718+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:201: ioctl_getdspinfo in ioctl.c domain:5
> > 2025-01-08T18:45:03.169307+00:00 qcs9100-ride-sx calculator: log_config.c:396: file_watcher_thread starting for domain 5
> > 2025-01-08T18:45:03.180355+00:00 qcs9100-ride-sx calculator: fastrpc_ioctl.c:203: done ioctl_getdspinfo in ioctl.c ioErr:ffffffff
> > 
> > putty log:
> > [ 1332.308444] qcom,fastrpc 20c00000.remoteproc:glink-edge.fastrpcglink-apps-dsp.-1.-1: Error: Invalid domain id:5, err:0
> > 
> > Because on the user side, gdsp0 and gdsp1 will be distinguished to 5 and 6.
> > so do you mean you want me to modify UMD code to transfer both gdsp0 and gdsp1 to gdsp just in ioctl_getdspinfo?
> >>
> 
> There is no error message after removing these lines based on srini's patch, is this modification appropriate? If so, I will submit a new patch.
> @@ -1771,17 +1763,6 @@ static int fastrpc_get_dsp_info(struct fastrpc_user *fl, char __user *argp)
>                 return  -EFAULT;
> 
>         cap.capability = 0;
> -       if (cap.domain >= FASTRPC_DEV_MAX) {
> -               dev_err(&fl->cctx->rpdev->dev, "Error: Invalid domain id:%d, err:%d\n",
> -                       cap.domain, err);
> -               return -ECHRNG;
> -       }
> -
> -       /* Fastrpc Capablities does not support modem domain */
> -       if (cap.domain == MDSP_DOMAIN_ID) {
> -               dev_err(&fl->cctx->rpdev->dev, "Error: modem not supported %d\n", err);
> -               return -ECHRNG;
> -       }
> 
>         if (cap.attribute_id >= FASTRPC_MAX_DSP_ATTRIBUTES) {
>                 dev_err(&fl->cctx->rpdev->dev, "Error: invalid attribute: %d, err: %d\n",


You also need to modify fastrpc_ioctl_capability definition to drop the
domain field and also modify fastrpc_get_info_from_kernel().


-- 
With best wishes
Dmitry

[PATCH v2 3/3] dt-bindings: misc: qcom,fastrpc: Add GPDSPs label

[Ling Xu]

Add "gdsp0" and "gdsp1" as the new supported labels for GPDSPs fastrpc
domains.

Signed-off-by: Ling Xu <quic_lxu5@quicinc.com>
---
 Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
index 0840a3d92513..3f6199fc9ae6 100644
--- a/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
+++ b/Documentation/devicetree/bindings/misc/qcom,fastrpc.yaml
@@ -27,6 +27,8 @@ properties:
       - sdsp
       - cdsp
       - cdsp1
+      - gdsp0
+      - gdsp1
 
   memory-region:
     maxItems: 1
-- 
2.34.1

Re: [PATCH v2 3/3] dt-bindings: misc: qcom,fastrpc: Add GPDSPs label

[Krzysztof Kozlowski]

On Thu, Mar 20, 2025 at 02:44:46PM +0530, Ling Xu wrote:
> Add "gdsp0" and "gdsp1" as the new supported labels for GPDSPs fastrpc
> domains.

Why? What problem is this solving? What is GPDSP and GDSP? Why they
differ? So many questions, so little explained in commit msg.

Also, bindings are before users.

Best regards,
Krzysztof