diff mbox series

[v2] misc: fastrpc: Add support for multiple PD from one process

Message ID 20240720034611.2219308-1-quic_ekangupt@quicinc.com (mailing list archive)
State Superseded
Headers show
Series [v2] misc: fastrpc: Add support for multiple PD from one process | expand

Commit Message

Ekansh Gupta July 20, 2024, 3:46 a.m. UTC 
Memory intensive applications(which requires more tha 4GB) that wants
to offload tasks to DSP might have to split the tasks to multiple
user PD to make the resources available.

For every call to DSP, fastrpc driver passes the process tgid which
works as an identifier for the DSP to enqueue the tasks to specific PD.
With current design, if any process opens device node more than once
and makes PD initmrequest, same tgid will be passed to DSP which will
be considered a bad request and this will result in failure as the same
identifier cannot be used for multiple DSP PD.

Allocate and pass an effective pgid to DSP which would be allocated
during device open and will have a lifetime till the device is closed.
This will allow the same process to open the device more than once and
spawn multiple dynamic PD for ease of processing.

Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
---
Changes in v2:
  - Reformatted commit text.
  - Moved from ida to idr.
  - Changed dsp_pgid data type.
  - Resolved memory leak.

 drivers/misc/fastrpc.c | 49 +++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 12 deletions(-)

Comments

Dmitry Baryshkov July 22, 2024, 8:18 a.m. UTC | #1 
On Sat, Jul 20, 2024 at 09:16:11AM GMT, Ekansh Gupta wrote:
> Memory intensive applications(which requires more tha 4GB) that wants
> to offload tasks to DSP might have to split the tasks to multiple
> user PD to make the resources available.
> 
> For every call to DSP, fastrpc driver passes the process tgid which
> works as an identifier for the DSP to enqueue the tasks to specific PD.
> With current design, if any process opens device node more than once
> and makes PD initmrequest, same tgid will be passed to DSP which will
> be considered a bad request and this will result in failure as the same
> identifier cannot be used for multiple DSP PD.
> 
> Allocate and pass an effective pgid to DSP which would be allocated

effective pgid makes me think about the setegid() system call. Can we
just name them "client ID" (granted that session is already reserved)?
Or is it really session ID? Can we use the index of the session instead
and skip the whole IDR allocation?

> during device open and will have a lifetime till the device is closed.
> This will allow the same process to open the device more than once and
> spawn multiple dynamic PD for ease of processing.
> 
> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
> ---
> Changes in v2:
>   - Reformatted commit text.
>   - Moved from ida to idr.
>   - Changed dsp_pgid data type.
>   - Resolved memory leak.
> 
>  drivers/misc/fastrpc.c | 49 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index a7a2bcedb37e..b4a5af2d2dfa 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -105,6 +105,10 @@
>  
>  #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>  
> +#define MAX_DSP_PD	64	/* Maximum 64 PDs are allowed on DSP */

Why?

> +#define MIN_FRPC_PGID	1000

Is it some random number or some pre-defined constant? Can we use 0
instead?

> +#define MAX_FRPC_PGID	(MIN_FRPC_PGID + MAX_DSP_PD)
> +
>  static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>  						"sdsp", "cdsp"};
>  struct fastrpc_phy_page {
Ekansh Gupta July 26, 2024, 6:36 a.m. UTC | #2 
On 7/22/2024 1:48 PM, Dmitry Baryshkov wrote:
> On Sat, Jul 20, 2024 at 09:16:11AM GMT, Ekansh Gupta wrote:
>> Memory intensive applications(which requires more tha 4GB) that wants
>> to offload tasks to DSP might have to split the tasks to multiple
>> user PD to make the resources available.
>>
>> For every call to DSP, fastrpc driver passes the process tgid which
>> works as an identifier for the DSP to enqueue the tasks to specific PD.
>> With current design, if any process opens device node more than once
>> and makes PD initmrequest, same tgid will be passed to DSP which will
>> be considered a bad request and this will result in failure as the same
>> identifier cannot be used for multiple DSP PD.
>>
>> Allocate and pass an effective pgid to DSP which would be allocated
> effective pgid makes me think about the setegid() system call. Can we
> just name them "client ID" (granted that session is already reserved)?
> Or is it really session ID? Can we use the index of the session instead
> and skip the whole IDR allocation?
Thanks for the suggestion, I'm trying out experiments with the index of session as
client ID to avoid idr operations. This sounds much better. I will update the patch
soon.

--Ekansh
>> during device open and will have a lifetime till the device is closed.
>> This will allow the same process to open the device more than once and
>> spawn multiple dynamic PD for ease of processing.
>>
>> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com>
>> ---
>> Changes in v2:
>>   - Reformatted commit text.
>>   - Moved from ida to idr.
>>   - Changed dsp_pgid data type.
>>   - Resolved memory leak.
>>
>>  drivers/misc/fastrpc.c | 49 +++++++++++++++++++++++++++++++-----------
>>  1 file changed, 37 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index a7a2bcedb37e..b4a5af2d2dfa 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -105,6 +105,10 @@
>>  
>>  #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
>>  
>> +#define MAX_DSP_PD	64	/* Maximum 64 PDs are allowed on DSP */
> Why?
>
>> +#define MIN_FRPC_PGID	1000
> Is it some random number or some pre-defined constant? Can we use 0
> instead?
>
>> +#define MAX_FRPC_PGID	(MIN_FRPC_PGID + MAX_DSP_PD)
>> +
>>  static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
>>  						"sdsp", "cdsp"};
>>  struct fastrpc_phy_page {
>
diff mbox series

Patch

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index a7a2bcedb37e..b4a5af2d2dfa 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -105,6 +105,10 @@ 
 
 #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev)
 
+#define MAX_DSP_PD	64	/* Maximum 64 PDs are allowed on DSP */
+#define MIN_FRPC_PGID	1000
+#define MAX_FRPC_PGID	(MIN_FRPC_PGID + MAX_DSP_PD)
+
 static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp",
 						"sdsp", "cdsp"};
 struct fastrpc_phy_page {
@@ -268,6 +272,7 @@  struct fastrpc_channel_ctx {
 	struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS];
 	spinlock_t lock;
 	struct idr ctx_idr;
+	struct idr dsp_pgid_idr;
 	struct list_head users;
 	struct kref refcount;
 	/* Flag if dsp attributes are cached */
@@ -298,7 +303,7 @@  struct fastrpc_user {
 	struct fastrpc_session_ctx *sctx;
 	struct fastrpc_buf *init_mem;
 
-	int tgid;
+	u16 dsp_pgid;
 	int pd;
 	bool is_secure_dev;
 	/* Lock for lists */
@@ -462,6 +467,7 @@  static void fastrpc_channel_ctx_free(struct kref *ref)
 	struct fastrpc_channel_ctx *cctx;
 
 	cctx = container_of(ref, struct fastrpc_channel_ctx, refcount);
+	idr_destroy(&cctx->dsp_pgid_idr);
 
 	kfree(cctx);
 }
@@ -613,7 +619,7 @@  static struct fastrpc_invoke_ctx *fastrpc_context_alloc(
 	ctx->sc = sc;
 	ctx->retval = -1;
 	ctx->pid = current->pid;
-	ctx->tgid = user->tgid;
+	ctx->tgid = user->dsp_pgid;
 	ctx->cctx = cctx;
 	init_completion(&ctx->work);
 	INIT_WORK(&ctx->put_work, fastrpc_context_put_wq);
@@ -1111,7 +1117,7 @@  static int fastrpc_invoke_send(struct fastrpc_session_ctx *sctx,
 	int ret;
 
 	cctx = fl->cctx;
-	msg->pid = fl->tgid;
+	msg->pid = fl->dsp_pgid;
 	msg->tid = current->pid;
 
 	if (kernel)
@@ -1294,7 +1300,7 @@  static int fastrpc_init_create_static_process(struct fastrpc_user *fl,
 		}
 	}
 
-	inbuf.pgid = fl->tgid;
+	inbuf.pgid = fl->dsp_pgid;
 	inbuf.namelen = init.namelen;
 	inbuf.pageslen = 0;
 	fl->pd = USER_PD;
@@ -1396,7 +1402,7 @@  static int fastrpc_init_create_process(struct fastrpc_user *fl,
 		goto err;
 	}
 
-	inbuf.pgid = fl->tgid;
+	inbuf.pgid = fl->dsp_pgid;
 	inbuf.namelen = strlen(current->comm) + 1;
 	inbuf.filelen = init.filelen;
 	inbuf.pageslen = 1;
@@ -1505,7 +1511,7 @@  static int fastrpc_release_current_dsp_process(struct fastrpc_user *fl)
 	int tgid = 0;
 	u32 sc;
 
-	tgid = fl->tgid;
+	tgid = fl->dsp_pgid;
 	args[0].ptr = (u64)(uintptr_t) &tgid;
 	args[0].length = sizeof(tgid);
 	args[0].fd = -1;
@@ -1528,6 +1534,7 @@  static int fastrpc_device_release(struct inode *inode, struct file *file)
 
 	spin_lock_irqsave(&cctx->lock, flags);
 	list_del(&fl->user);
+	idr_remove(&cctx->dsp_pgid_idr, fl->dsp_pgid);
 	spin_unlock_irqrestore(&cctx->lock, flags);
 
 	if (fl->init_mem)
@@ -1562,6 +1569,7 @@  static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	struct fastrpc_device *fdevice;
 	struct fastrpc_user *fl = NULL;
 	unsigned long flags;
+	int ret;
 
 	fdevice = miscdev_to_fdevice(filp->private_data);
 	cctx = fdevice->cctx;
@@ -1580,13 +1588,29 @@  static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	INIT_LIST_HEAD(&fl->maps);
 	INIT_LIST_HEAD(&fl->mmaps);
 	INIT_LIST_HEAD(&fl->user);
-	fl->tgid = current->tgid;
 	fl->cctx = cctx;
 	fl->is_secure_dev = fdevice->secure;
 
+	spin_lock_irqsave(&cctx->lock, flags);
+	/* allocate unique id between MIN_FRPC_PGID and MAX_FRPC_PGID */
+	ret = idr_alloc_cyclic(&cctx->dsp_pgid_idr, NULL, MIN_FRPC_PGID,
+					MAX_FRPC_PGID, GFP_ATOMIC);
+	if (ret < 0) {
+		dev_dbg(&cctx->rpdev->dev, "too many fastrpc clients, max %u allowed\n", MAX_DSP_PD);
+		spin_unlock_irqrestore(&cctx->lock, flags);
+		mutex_destroy(&fl->mutex);
+		kfree(fl);
+		return ret;
+	}
+	fl->dsp_pgid = ret;
+	spin_unlock_irqrestore(&cctx->lock, flags);
+
 	fl->sctx = fastrpc_session_alloc(cctx);
 	if (!fl->sctx) {
 		dev_err(&cctx->rpdev->dev, "No session available\n");
+		spin_lock_irqsave(&cctx->lock, flags);
+		idr_remove(&cctx->dsp_pgid_idr, fl->dsp_pgid);
+		spin_unlock_irqrestore(&cctx->lock, flags);
 		mutex_destroy(&fl->mutex);
 		kfree(fl);
 
@@ -1648,7 +1672,7 @@  static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char __user *argp)
 static int fastrpc_init_attach(struct fastrpc_user *fl, int pd)
 {
 	struct fastrpc_invoke_args args[1];
-	int tgid = fl->tgid;
+	int tgid = fl->dsp_pgid;
 	u32 sc;
 
 	args[0].ptr = (u64)(uintptr_t) &tgid;
@@ -1804,7 +1828,7 @@  static int fastrpc_req_munmap_impl(struct fastrpc_user *fl, struct fastrpc_buf *
 	int err;
 	u32 sc;
 
-	req_msg.pgid = fl->tgid;
+	req_msg.pgid = fl->dsp_pgid;
 	req_msg.size = buf->size;
 	req_msg.vaddr = buf->raddr;
 
@@ -1890,7 +1914,7 @@  static int fastrpc_req_mmap(struct fastrpc_user *fl, char __user *argp)
 		return err;
 	}
 
-	req_msg.pgid = fl->tgid;
+	req_msg.pgid = fl->dsp_pgid;
 	req_msg.flags = req.flags;
 	req_msg.vaddr = req.vaddrin;
 	req_msg.num = sizeof(pages);
@@ -1980,7 +2004,7 @@  static int fastrpc_req_mem_unmap_impl(struct fastrpc_user *fl, struct fastrpc_me
 		return -EINVAL;
 	}
 
-	req_msg.pgid = fl->tgid;
+	req_msg.pgid = fl->dsp_pgid;
 	req_msg.len = map->len;
 	req_msg.vaddrin = map->raddr;
 	req_msg.fd = map->fd;
@@ -2033,7 +2057,7 @@  static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
 		return err;
 	}
 
-	req_msg.pgid = fl->tgid;
+	req_msg.pgid = fl->dsp_pgid;
 	req_msg.fd = req.fd;
 	req_msg.offset = req.offset;
 	req_msg.vaddrin = req.vaddrin;
@@ -2358,6 +2382,7 @@  static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
 	INIT_LIST_HEAD(&data->invoke_interrupted_mmaps);
 	spin_lock_init(&data->lock);
 	idr_init(&data->ctx_idr);
+	idr_init(&data->dsp_pgid_idr);
 	data->domain_id = domain_id;
 	data->rpdev = rpdev;