by Srinivas Kandagatla

[permalink] [raw]

Subject: Re: [PATCH v1 04/16] misc: fastrpc: Add fastrpc multimode invoke request support

On 02/02/2024 06:40, Ekansh Gupta wrote:
> Multimode invocation request is intended to support multiple
> different type of requests. This will include enhanced invoke
> request to support CRC check and performance counter enablement.
> This will also support few driver level user controllable
> mechanisms like usage of shared context banks, wakelock support,
> etc. This IOCTL is also added with the aim to support few
> new fastrpc features like DSP PD notification framework,
> DSP Signalling mechanism etc.
>
> Signed-off-by: Ekansh Gupta <[email protected]>
> ---
> drivers/misc/fastrpc.c | 176 ++++++++++++++++++++++++++----------
> include/uapi/misc/fastrpc.h | 26 ++++++
> 2 files changed, 154 insertions(+), 48 deletions(-)
>
..

> diff --git a/include/uapi/misc/fastrpc.h b/include/uapi/misc/fastrpc.h
> index f33d914d8f46..45c15be1de58 100644
> --- a/include/uapi/misc/fastrpc.h
> +++ b/include/uapi/misc/fastrpc.h
> @@ -16,6 +16,7 @@
> #define FASTRPC_IOCTL_INIT_CREATE_STATIC _IOWR('R', 9, struct fastrpc_init_create_static)
> #define FASTRPC_IOCTL_MEM_MAP _IOWR('R', 10, struct fastrpc_mem_map)
> #define FASTRPC_IOCTL_MEM_UNMAP _IOWR('R', 11, struct fastrpc_mem_unmap)
> +#define FASTRPC_IOCTL_MULTIMODE_INVOKE _IOWR('R', 12, struct fastrpc_ioctl_multimode_invoke)
> #define FASTRPC_IOCTL_GET_DSP_INFO _IOWR('R', 13, struct fastrpc_ioctl_capability)
>
> /**
> @@ -80,6 +81,31 @@ struct fastrpc_invoke {
> __u64 args;
> };
>
> +struct fastrpc_enhanced_invoke {
> + struct fastrpc_invoke inv;
> + __u64 crc;
> + __u64 perf_kernel;
> + __u64 perf_dsp;
> + __u32 reserved[8]; /* keeping reserved bits for new requirements */
> +};
> +
> +struct fastrpc_ioctl_multimode_invoke {
This struct needs some documentation.

> + __u32 req;
we use req here and then in next few lines we define the same as
fastrpc_multimode_invoke_type. I would recommend to make this type
instead of req.

> + __u32 rsvd; /* padding field */
reserved?

<---
> + __u64 invparam;
> + __u64 size;
-->
Isn't size obvious when we know request type?

This is also opening up a path for userspace to pass some random structures.

It makes more sense to have a union of all the request structures.

Why not add all the enhanced invoke uapi structures as part of this patch?

> + __u32 reserved[8]; /* keeping reserved bits for new requirements */
> +};
> +
> +enum fastrpc_multimode_invoke_type {
> + FASTRPC_INVOKE = 1,
> + FASTRPC_INVOKE_ENHANCED = 2,
> + FASTRPC_INVOKE_CONTROL = 3,
> + FASTRPC_INVOKE_DSPSIGNAL = 4,
> + FASTRPC_INVOKE_NOTIF = 5,
> + FASTRPC_INVOKE_MULTISESSION = 6,

All of these needs a proper documentation. Its impossible to understand
what they actually mean.

This applies to all the enums that are added as part of other patches to
the uapi headers.

thanks,
Srini

> +};
> +
> struct fastrpc_init_create {
> __u32 filelen; /* elf file length */
> __s32 filefd; /* fd for the file */

2024-02-22 07:04:06

by Wu, Wentong

[permalink] [raw]

Subject: RE: [PATCH v1 00/16] Add missing features to FastRPC driver

> From: Ekansh Gupta <[email protected]>
> This patch series adds the listed features that have been missing in
> upstream fastRPC driver.

Thanks for the patch, and is there any latency data on driver side?

BR,
Wentong
>
> - Redesign and improve remote heap management.
> - Add static PD restart support for audio and sensors PD using
> PDR framework.
> - Add changes to support multimode invocation ioctl request. This
> ioctl call facilitates multiple types of requests from user including
> CRC check, performance counters, shared context bank usage, etc.
> This series also carries patch to save and restore interrupted
> context.
> - Add early wakeup support to allow DSP user to send early response
> to CPU and improve fastrpc performance.
> - Add polling mode support with which driver polls on memory to avoid
> CPU from going to low power modes.
> - Add notifications frameworks to provide users with the DSP PD status
> notifications.
> - Add a control mechanism to allow users to clean up DSP user PD
> - Add wakelock management support
> - Add DSP signalling support
> - Add check for untrusted applications and allow trusted processed to
> offload to system unsigned PD.
>
> Ekansh Gupta (16):
> misc: fastrpc: Redesign remote heap management
> misc: fastrpc: Add support for unsigned PD
> misc: fastrpc: Add static PD restart support
> misc: fastrpc: Add fastrpc multimode invoke request support
> misc: fastrpc: Add CRC support for remote buffers
> misc: fastrpc: Capture kernel and DSP performance counters
> misc: fastrpc: Add support to save and restore interrupted
> misc: fastrpc: Add support to allocate shared context bank
> misc: fastrpc: Add early wakeup support for fastRPC driver
> misc: fastrpc: Add polling mode support for fastRPC driver
> misc: fastrpc: Add DSP PD notification support
> misc: fastrpc: Add support for users to clean up DSP user PD
> misc: fastrpc: Add wakelock management support
> misc: fastrpc: Add DSP signal support
> misc: fastrpc: Restrict untrusted apk to spawn privileged PD
> misc: fastrpc: Add system unsigned PD support
>
> drivers/misc/fastrpc.c | 1949 +++++++++++++++++++++++++++++++----
> include/uapi/misc/fastrpc.h | 112 ++
> 2 files changed, 1844 insertions(+), 217 deletions(-)
>
> --
> 2.17.0
>