| Message ID | 20240606165939.12950-6-quic_ekangupt@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add missing features to FastRPC driver | expand |
On Thu, Jun 06, 2024 at 10:29:25PM +0530, Ekansh Gupta wrote: > Static PDs are created on DSPs to support specific use cases like Audio > and Sensors. The static PDs uses any CPU requirements like file > operations or memory need with the help of a daemon running on the CPU. What do you mean by 'CPU requirements' here? > Audio and sensors daemons attaches to audio PD and sensors PD on DSP. attach > Audio PD expects some CMA memory for dynamic loading purpose which is > allocated and sent to DSP in fastrpc_init_create_static_process call. A pointer to audio daemon would be helpful. We don't have such a thing in the normal Linux operation cycle. > For sensor daemon, the expectation is just to attach to sensors PD and > take up any requests made by the PD(like file operations etc.). > > Static PDs run on the audio and sensor supporting subsystem which can > be ADSP or SDSP. They are expected to support PD restart. There are some > CPU resources like buffers etc. for static PDs which are expected to be > cleaned up by fastrpc driver during PDR scenario. For this, there is a Why are they cleaned by fastrpc driver and not by the userspace daemon itself? Userspace can also subscribe to PDR notifications and handle restarts on its own. > requirement of PD service locator to get the event notifications for > static PD services. Also when events are received, the driver needs to > handle based on PD states. To handle what? And why? > > PDR handling is required for static PD only. There are no static PD > supported on MDSP or CDSP hence no PDR handling is required. PDR is also > not required for root_pd as if root_pd is shutting down, that basically > suggests that the remoteproc itself is shutting down which is handled > with rpmsg functionalities(probe and remove). > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > drivers/misc/Kconfig | 2 + > drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 195 insertions(+), 12 deletions(-) > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index faf983680040..e2d83cd085b5 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -280,8 +280,10 @@ config QCOM_FASTRPC > tristate "Qualcomm FastRPC" > depends on ARCH_QCOM || COMPILE_TEST > depends on RPMSG > + depends on NET > select DMA_SHARED_BUFFER > select QCOM_SCM > + select QCOM_PDR_HELPERS > help > Provides a communication mechanism that allows for clients to > make remote method invocations across processor boundary to > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index abdd35b7c3ad..13e368279765 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -22,6 +22,7 @@ > #include <linux/firmware/qcom/qcom_scm.h> > #include <uapi/misc/fastrpc.h> > #include <linux/of_reserved_mem.h> > +#include <linux/soc/qcom/pdr.h> > > #define ADSP_DOMAIN_ID (0) > #define MDSP_DOMAIN_ID (1) > @@ -29,6 +30,7 @@ > #define CDSP_DOMAIN_ID (3) > #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/ > #define FASTRPC_MAX_SESSIONS 14 > +#define FASTRPC_MAX_SPD 4 > #define FASTRPC_MAX_VMIDS 16 > #define FASTRPC_ALIGN 128 > #define FASTRPC_MAX_FDLIST 16 > @@ -105,6 +107,18 @@ > > #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) > > +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp" > +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio" > +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd" > + > +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp" > +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg" > +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd" > + > +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi" > +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME > +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd" > + > static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp", > "sdsp", "cdsp"}; > struct fastrpc_phy_page { > @@ -259,6 +273,15 @@ struct fastrpc_session_ctx { > bool valid; > }; > > +struct fastrpc_static_pd { > + char *servloc_name; > + char *spdname; > + void *pdrhandle; > + struct fastrpc_channel_ctx *cctx; > + struct fastrpc_user *fl; > + bool ispdup; is_pd_up > +}; > + > struct fastrpc_channel_ctx { > int domain_id; > int sesscount; > @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx { > struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS]; > struct rpmsg_device *rpdev; > struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS]; > + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD]; > spinlock_t lock; > struct idr ctx_idr; > struct list_head users; > @@ -297,10 +321,12 @@ struct fastrpc_user { > struct fastrpc_channel_ctx *cctx; > struct fastrpc_session_ctx *sctx; > struct fastrpc_buf *init_mem; > + struct fastrpc_static_pd *spd; > > int tgid; > int pd; > bool is_secure_dev; > + char *servloc_name; > /* Lock for lists */ > spinlock_t lock; > /* lock for allocations */ > @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques > return false; > } > > +static struct fastrpc_static_pd *fastrpc_get_spd_session( > + struct fastrpc_user *fl) Single line, please > +{ > + int i; > + struct fastrpc_static_pd *spd = NULL; > + struct fastrpc_channel_ctx *cctx = fl->cctx; > + > + for (i = 0; i < FASTRPC_MAX_SPD ; i++) { > + if (!cctx->spd[i].servloc_name) > + continue; > + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) { > + spd = &cctx->spd[i]; > + spd->fl = fl; > + break; > + } Should there be a protection for spd->fl being overwritten / overtaken? How should it be handled by the driver? > + } > + > + return spd; > +} > + > static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > char __user *argp) > { > struct fastrpc_init_create_static init; > struct fastrpc_invoke_args *args; > struct fastrpc_phy_page pages[1]; > + struct fastrpc_static_pd *spd = NULL; > char *name; > int err; > struct { > @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > goto err_name; > } > > + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME; > + > + spd = fastrpc_get_spd_session(fl); > + if (!spd) { > + err = -EUSERS; "Too many users" ? > + goto err_name; > + } > + > + if (!spd->ispdup) { This is racy. What if the domain restarts right after this line? > + err = -ENOTCONN; > + goto err_name; > + } > + fl->spd = spd; > if (!fl->cctx->remote_heap) { > err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, > &fl->cctx->remote_heap); > @@ -1645,6 +1705,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]; > + struct fastrpc_static_pd *spd = NULL; > int tgid = fl->tgid; > u32 sc; > > @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd) > sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0); > fl->pd = pd; > > + if (pd == SENSORS_PD) { > + if (fl->cctx->domain_id == ADSP_DOMAIN_ID) > + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME; > + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID) > + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME; > + > + spd = fastrpc_get_spd_session(fl); > + if (!spd) > + return -EUSERS; > + > + if (!spd->ispdup) > + return -ENOTCONN; > + > + fl->spd = spd; > + } > + > return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, > sc, &args[0]); > } > @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, > return err; > } > > +static void fastrpc_notify_users(struct fastrpc_user *user) > +{ > + struct fastrpc_invoke_ctx *ctx; > + > + spin_lock(&user->lock); > + list_for_each_entry(ctx, &user->pending, node) { > + ctx->retval = -EPIPE; > + complete(&ctx->work); > + } > + spin_unlock(&user->lock); > +} > + > +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx, > + char *servloc_name) > +{ > + struct fastrpc_user *fl; > + unsigned long flags; > + > + spin_lock_irqsave(&cctx->lock, flags); > + list_for_each_entry(fl, &cctx->users, user) { > + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name)) > + fastrpc_notify_users(fl); > + } > + spin_unlock_irqrestore(&cctx->lock, flags); > +} > + > +static void fastrpc_pdr_cb(int state, char *service_path, void *priv) > +{ > + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv; > + struct fastrpc_channel_ctx *cctx; > + > + if (!spd) > + return; > + > + cctx = spd->cctx; > + switch (state) { > + case SERVREG_SERVICE_STATE_DOWN: > + dev_info(&cctx->rpdev->dev, > + "%s: %s (%s) is down for PDR on %s\n", > + __func__, spd->spdname, > + spd->servloc_name, > + domains[cctx->domain_id]); Too noisy. Is it really dev_info()? Or dev_warn? There should be no need for __func__. > + spd->ispdup = false; > + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name); > + break; > + case SERVREG_SERVICE_STATE_UP: > + dev_info(&cctx->rpdev->dev, > + "%s: %s (%s) is up for PDR on %s\n", > + __func__, spd->spdname, > + spd->servloc_name, > + domains[cctx->domain_id]); Same comment here. > + spd->ispdup = true; > + break; > + default: > + break; > + } > +} > + > static const struct file_operations fastrpc_fops = { > .open = fastrpc_device_open, > .release = fastrpc_device_release, > @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct > return err; > } > > +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name, > + char *service_name, char *service_path, int domain, int spd_session) > +{ > + int err = 0; > + struct pdr_handle *handle = NULL; > + struct pdr_service *service = NULL; > + > + /* Register the service locator's callback function */ > + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]); > + if (IS_ERR(handle)) { > + err = PTR_ERR(handle); > + goto bail; > + } > + cctx->spd[spd_session].pdrhandle = handle; > + cctx->spd[spd_session].servloc_name = client_name; > + cctx->spd[spd_session].spdname = service_path; > + cctx->spd[spd_session].cctx = cctx; > + service = pdr_add_lookup(handle, service_name, service_path); > + if (IS_ERR(service)) { > + err = PTR_ERR(service); > + goto bail; > + } > + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n", > + __func__, service_name, client_name, service_path); Definitely too noisy. Drop __func__ everywhere. Use dev_dbg instead. > + > +bail: > + if (err) { > + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n", > + __func__, service_name, client_name, service_path, err); dev_warn, drop brackets, align properly. Did you run this through checkpatch.pl --strict? > + } > + return err; > +} > + > static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) > { > struct device *rdev = &rpdev->dev; > @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) > goto fdev_error; > } > > + if (domain_id == ADSP_DOMAIN_ID) { > + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME, > + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0); > + if (err) > + goto populate_error; > + > + err = fastrpc_setup_service_locator(data, > + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME, > + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1); > + if (err) > + goto populate_error; > + } else if (domain_id == SDSP_DOMAIN_ID) { > + err = fastrpc_setup_service_locator(data, > + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME, > + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0); > + if (err) > + goto populate_error; > + } > + > kref_init(&data->refcount); > > dev_set_drvdata(&rpdev->dev, data); > @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) > return err; > } > > -static void fastrpc_notify_users(struct fastrpc_user *user) > -{ > - struct fastrpc_invoke_ctx *ctx; > - > - spin_lock(&user->lock); > - list_for_each_entry(ctx, &user->pending, node) { > - ctx->retval = -EPIPE; > - complete(&ctx->work); > - } > - spin_unlock(&user->lock); > -} > - > static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) > { > struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev); > struct fastrpc_buf *buf, *b; > struct fastrpc_user *user; > unsigned long flags; > + int i; > > /* No invocations past this point */ > spin_lock_irqsave(&cctx->lock, flags); > @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) > if (cctx->remote_heap) > fastrpc_buf_free(cctx->remote_heap); > > + for (i = 0; i < FASTRPC_MAX_SPD; i++) { > + if (cctx->spd[i].pdrhandle) > + pdr_handle_release(cctx->spd[i].pdrhandle); > + } > + > of_platform_depopulate(&rpdev->dev); > > fastrpc_channel_ctx_put(cctx); > -- > 2.43.0 >
On 06/06/2024 18:59, Ekansh Gupta wrote: > Static PDs are created on DSPs to support specific use cases like Audio > and Sensors. The static PDs uses any CPU requirements like file > operations or memory need with the help of a daemon running on the CPU. > Audio and sensors daemons attaches to audio PD and sensors PD on DSP. > Audio PD expects some CMA memory for dynamic loading purpose which is > allocated and sent to DSP in fastrpc_init_create_static_process call. > For sensor daemon, the expectation is just to attach to sensors PD and > take up any requests made by the PD(like file operations etc.). > > Static PDs run on the audio and sensor supporting subsystem which can > be ADSP or SDSP. They are expected to support PD restart. There are some > CPU resources like buffers etc. for static PDs which are expected to be > cleaned up by fastrpc driver during PDR scenario. For this, there is a > requirement of PD service locator to get the event notifications for > static PD services. Also when events are received, the driver needs to > handle based on PD states. > > PDR handling is required for static PD only. There are no static PD > supported on MDSP or CDSP hence no PDR handling is required. PDR is also > not required for root_pd as if root_pd is shutting down, that basically > suggests that the remoteproc itself is shutting down which is handled > with rpmsg functionalities(probe and remove). > > Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> > --- > drivers/misc/Kconfig | 2 + > drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++--- I think this justifies introducing a new C file. The functionality added here should be quite easily abstracted behind a sensible interface. > 2 files changed, 195 insertions(+), 12 deletions(-) > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index faf983680040..e2d83cd085b5 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -280,8 +280,10 @@ config QCOM_FASTRPC > tristate "Qualcomm FastRPC" > depends on ARCH_QCOM || COMPILE_TEST > depends on RPMSG > + depends on NET > select DMA_SHARED_BUFFER > select QCOM_SCM > + select QCOM_PDR_HELPERS > help > Provides a communication mechanism that allows for clients to > make remote method invocations across processor boundary to > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index abdd35b7c3ad..13e368279765 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c > @@ -22,6 +22,7 @@ > #include <linux/firmware/qcom/qcom_scm.h> > #include <uapi/misc/fastrpc.h> > #include <linux/of_reserved_mem.h> > +#include <linux/soc/qcom/pdr.h> > > #define ADSP_DOMAIN_ID (0) > #define MDSP_DOMAIN_ID (1) > @@ -29,6 +30,7 @@ > #define CDSP_DOMAIN_ID (3) > #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/ > #define FASTRPC_MAX_SESSIONS 14 > +#define FASTRPC_MAX_SPD 4 Why 4? This patch only makes use of two. > #define FASTRPC_MAX_VMIDS 16 > #define FASTRPC_ALIGN 128 > #define FASTRPC_MAX_FDLIST 16 > @@ -105,6 +107,18 @@ > > #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) > > +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp" > +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio" > +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd" > + > +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp" > +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg" > +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd" > + > +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi" > +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME > +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd" This data should be defined in some const static data struct, not as a bunch of macros, then you can actually describe the relationship between a domain_id and the fact that the ADSP registers two PDR lookups. See my comments in fastrpc_setup_service_locator() and where it's called in probe. struct fastrpc_pdr_domain { const char *servloc_client_name; const char *service_name; const char *pd_name; }; static const struct fastrpc_pdr_domain adsp_pdr_services[] = { { .servloc_client_name = "audio_pdr_adsp"; .service_name = "avs/audio"; .pd_name = "msm/adsp/audio_pd"; }, { .servloc_client_name = "sensors_pdr_adsp"; ... }; > + > static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp", > "sdsp", "cdsp"}; > struct fastrpc_phy_page { > @@ -259,6 +273,15 @@ struct fastrpc_session_ctx { > bool valid; > }; > > +struct fastrpc_static_pd { > + char *servloc_name; > + char *spdname; > + void *pdrhandle; > + struct fastrpc_channel_ctx *cctx; > + struct fastrpc_user *fl; > + bool ispdup; > +}; > + > struct fastrpc_channel_ctx { > int domain_id; > int sesscount; > @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx { > struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS]; > struct rpmsg_device *rpdev; > struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS]; > + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD]; > spinlock_t lock; > struct idr ctx_idr; > struct list_head users; > @@ -297,10 +321,12 @@ struct fastrpc_user { > struct fastrpc_channel_ctx *cctx; > struct fastrpc_session_ctx *sctx; > struct fastrpc_buf *init_mem; > + struct fastrpc_static_pd *spd; > > int tgid; > int pd; > bool is_secure_dev; > + char *servloc_name; This is duplicated from spd->servloc_name > /* Lock for lists */ > spinlock_t lock; > /* lock for allocations */ > @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques > return false; > } > > +static struct fastrpc_static_pd *fastrpc_get_spd_session( > + struct fastrpc_user *fl) Every caller of this function has the same handling (checking !spd and then !spd->ispdup and returning different errors in each case). Please lift this common error handling into this function. While at it, I'd propose dropping the opaque *fl pointer and instead passing in the data which is actually used. The servloc_name could be looked up from the fastrpc_pdr_domain data. static int fastrpc_pdr_is_up(int pd, int domain_id, struct fastrpc_static_pd *spds, struct fastrpc_static_pd *spd) > +{ > + int i; > + struct fastrpc_static_pd *spd = NULL; > + struct fastrpc_channel_ctx *cctx = fl->cctx; > + > + for (i = 0; i < FASTRPC_MAX_SPD ; i++) { > + if (!cctx->spd[i].servloc_name) > + continue; > + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) { > + spd = &cctx->spd[i]; > + spd->fl = fl; > + break; > + } > + } > + > + return spd; > +} > + > static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > char __user *argp) > { > struct fastrpc_init_create_static init; > struct fastrpc_invoke_args *args; > struct fastrpc_phy_page pages[1]; > + struct fastrpc_static_pd *spd = NULL; > char *name; > int err; > struct { > @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, > goto err_name; > } > > + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME; > + > + spd = fastrpc_get_spd_session(fl); > + if (!spd) { > + err = -EUSERS; > + goto err_name; > + } > + > + if (!spd->ispdup) { > + err = -ENOTCONN; > + goto err_name; > + } > + fl->spd = spd; > if (!fl->cctx->remote_heap) { > err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, > &fl->cctx->remote_heap); > @@ -1645,6 +1705,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]; > + struct fastrpc_static_pd *spd = NULL; > int tgid = fl->tgid; > u32 sc; > > @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd) > sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0); > fl->pd = pd; > > + if (pd == SENSORS_PD) { Why is this only relevant for the sensors pd? > + if (fl->cctx->domain_id == ADSP_DOMAIN_ID) > + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME; > + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID) > + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME; What if domain_id isn't either of these? Should this be checked? If not, why? This whole block could be replaced with a call to the fastrpc_pdr_is_up() function I proposed. > + > + spd = fastrpc_get_spd_session(fl); > + if (!spd) > + return -EUSERS; > + > + if (!spd->ispdup) > + return -ENOTCONN; > + > + fl->spd = spd; > + } > + > return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, > sc, &args[0]); > } > @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, > return err; > } > > +static void fastrpc_notify_users(struct fastrpc_user *user) > +{ > + struct fastrpc_invoke_ctx *ctx; > + > + spin_lock(&user->lock); > + list_for_each_entry(ctx, &user->pending, node) { > + ctx->retval = -EPIPE; > + complete(&ctx->work); > + } > + spin_unlock(&user->lock); > +} > + > +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx, > + char *servloc_name) > +{ > + struct fastrpc_user *fl; > + unsigned long flags; > + > + spin_lock_irqsave(&cctx->lock, flags); > + list_for_each_entry(fl, &cctx->users, user) { > + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name)) > + fastrpc_notify_users(fl); > + } > + spin_unlock_irqrestore(&cctx->lock, flags); > +} > + > +static void fastrpc_pdr_cb(int state, char *service_path, void *priv) > +{ > + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv; > + struct fastrpc_channel_ctx *cctx; > + > + if (!spd) > + return; > + > + cctx = spd->cctx; > + switch (state) { > + case SERVREG_SERVICE_STATE_DOWN: > + dev_info(&cctx->rpdev->dev, > + "%s: %s (%s) is down for PDR on %s\n", > + __func__, spd->spdname, > + spd->servloc_name, > + domains[cctx->domain_id]); dev_dbg > + spd->ispdup = false; > + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name); > + break; > + case SERVREG_SERVICE_STATE_UP: > + dev_info(&cctx->rpdev->dev, > + "%s: %s (%s) is up for PDR on %s\n", > + __func__, spd->spdname, > + spd->servloc_name, > + domains[cctx->domain_id]); > + spd->ispdup = true; > + break; > + default: > + break; > + } > +} > + > static const struct file_operations fastrpc_fops = { > .open = fastrpc_device_open, > .release = fastrpc_device_release, > @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct > return err; > } > > +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name, > + char *service_name, char *service_path, int domain, int spd_session) This function should instead take a struct fastrpc_pdr_domain *services array. > +{ > + int err = 0; > + struct pdr_handle *handle = NULL; > + struct pdr_service *service = NULL; > + > + /* Register the service locator's callback function */ > + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]); > + if (IS_ERR(handle)) { > + err = PTR_ERR(handle); > + goto bail; > + } > + cctx->spd[spd_session].pdrhandle = handle; > + cctx->spd[spd_session].servloc_name = client_name; > + cctx->spd[spd_session].spdname = service_path; > + cctx->spd[spd_session].cctx = cctx; > + service = pdr_add_lookup(handle, service_name, service_path); > + if (IS_ERR(service)) { > + err = PTR_ERR(service); > + goto bail; > + } > + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n", dev_dbg > + __func__, service_name, client_name, service_path); > + > +bail: > + if (err) { > + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n", dev_err > + __func__, service_name, client_name, service_path, err); > + } > + return err; > +} > + > static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) > { > struct device *rdev = &rpdev->dev; > @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) > goto fdev_error; > } > > + if (domain_id == ADSP_DOMAIN_ID) { > + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME, > + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0); > + if (err) > + goto populate_error; > + > + err = fastrpc_setup_service_locator(data, > + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME, > + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1); I assume this is basically a nop on platforms where this service doesn't exist? A comment mentioning this would be nice. > + if (err) > + goto populate_error; > + } else if (domain_id == SDSP_DOMAIN_ID) { > + err = fastrpc_setup_service_locator(data, > + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME, > + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0); > + if (err) > + goto populate_error; > + } This block should be moved into the domain_id switch/case which is directly above it. > + > kref_init(&data->refcount); > > dev_set_drvdata(&rpdev->dev, data); > @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) > return err; > } > > -static void fastrpc_notify_users(struct fastrpc_user *user) > -{ > - struct fastrpc_invoke_ctx *ctx; > - > - spin_lock(&user->lock); > - list_for_each_entry(ctx, &user->pending, node) { > - ctx->retval = -EPIPE; > - complete(&ctx->work); > - } > - spin_unlock(&user->lock); > -} > - > static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) > { > struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev); > struct fastrpc_buf *buf, *b; > struct fastrpc_user *user; > unsigned long flags; > + int i; > > /* No invocations past this point */ > spin_lock_irqsave(&cctx->lock, flags); > @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) > if (cctx->remote_heap) > fastrpc_buf_free(cctx->remote_heap); > > + for (i = 0; i < FASTRPC_MAX_SPD; i++) { > + if (cctx->spd[i].pdrhandle) > + pdr_handle_release(cctx->spd[i].pdrhandle); > + } > + > of_platform_depopulate(&rpdev->dev); > > fastrpc_channel_ctx_put(cctx);
On 6/7/2024 4:55 PM, Dmitry Baryshkov wrote: > On Thu, Jun 06, 2024 at 10:29:25PM +0530, Ekansh Gupta wrote: >> Static PDs are created on DSPs to support specific use cases like Audio >> and Sensors. The static PDs uses any CPU requirements like file >> operations or memory need with the help of a daemon running on the CPU. > What do you mean by 'CPU requirements' here? Something like file system request or any memory requirement. For these types of requirements PD will rely on CPU process. Planning to drop this patch from the series and move the pd notification specific implementations to a different .c file as per Caleb's suggestion. I'll address all other comments in the new patch series. > >> Audio and sensors daemons attaches to audio PD and sensors PD on DSP. > attach > >> Audio PD expects some CMA memory for dynamic loading purpose which is >> allocated and sent to DSP in fastrpc_init_create_static_process call. > A pointer to audio daemon would be helpful. We don't have such a thing > in the normal Linux operation cycle. > >> For sensor daemon, the expectation is just to attach to sensors PD and >> take up any requests made by the PD(like file operations etc.). >> >> Static PDs run on the audio and sensor supporting subsystem which can >> be ADSP or SDSP. They are expected to support PD restart. There are some >> CPU resources like buffers etc. for static PDs which are expected to be >> cleaned up by fastrpc driver during PDR scenario. For this, there is a > Why are they cleaned by fastrpc driver and not by the userspace daemon > itself? Userspace can also subscribe to PDR notifications and handle > restarts on its own. > >> requirement of PD service locator to get the event notifications for >> static PD services. Also when events are received, the driver needs to >> handle based on PD states. > To handle what? And why? > >> PDR handling is required for static PD only. There are no static PD >> supported on MDSP or CDSP hence no PDR handling is required. PDR is also >> not required for root_pd as if root_pd is shutting down, that basically >> suggests that the remoteproc itself is shutting down which is handled >> with rpmsg functionalities(probe and remove). >> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> >> --- >> drivers/misc/Kconfig | 2 + >> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++--- >> 2 files changed, 195 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig >> index faf983680040..e2d83cd085b5 100644 >> --- a/drivers/misc/Kconfig >> +++ b/drivers/misc/Kconfig >> @@ -280,8 +280,10 @@ config QCOM_FASTRPC >> tristate "Qualcomm FastRPC" >> depends on ARCH_QCOM || COMPILE_TEST >> depends on RPMSG >> + depends on NET >> select DMA_SHARED_BUFFER >> select QCOM_SCM >> + select QCOM_PDR_HELPERS >> help >> Provides a communication mechanism that allows for clients to >> make remote method invocations across processor boundary to >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index abdd35b7c3ad..13e368279765 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -22,6 +22,7 @@ >> #include <linux/firmware/qcom/qcom_scm.h> >> #include <uapi/misc/fastrpc.h> >> #include <linux/of_reserved_mem.h> >> +#include <linux/soc/qcom/pdr.h> >> >> #define ADSP_DOMAIN_ID (0) >> #define MDSP_DOMAIN_ID (1) >> @@ -29,6 +30,7 @@ >> #define CDSP_DOMAIN_ID (3) >> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/ >> #define FASTRPC_MAX_SESSIONS 14 >> +#define FASTRPC_MAX_SPD 4 >> #define FASTRPC_MAX_VMIDS 16 >> #define FASTRPC_ALIGN 128 >> #define FASTRPC_MAX_FDLIST 16 >> @@ -105,6 +107,18 @@ >> >> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) >> >> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp" >> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio" >> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd" >> + >> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp" >> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg" >> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd" >> + >> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi" >> +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME >> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd" >> + >> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp", >> "sdsp", "cdsp"}; >> struct fastrpc_phy_page { >> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx { >> bool valid; >> }; >> >> +struct fastrpc_static_pd { >> + char *servloc_name; >> + char *spdname; >> + void *pdrhandle; >> + struct fastrpc_channel_ctx *cctx; >> + struct fastrpc_user *fl; >> + bool ispdup; > is_pd_up > >> +}; >> + >> struct fastrpc_channel_ctx { >> int domain_id; >> int sesscount; >> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx { >> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS]; >> struct rpmsg_device *rpdev; >> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS]; >> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD]; >> spinlock_t lock; >> struct idr ctx_idr; >> struct list_head users; >> @@ -297,10 +321,12 @@ struct fastrpc_user { >> struct fastrpc_channel_ctx *cctx; >> struct fastrpc_session_ctx *sctx; >> struct fastrpc_buf *init_mem; >> + struct fastrpc_static_pd *spd; >> >> int tgid; >> int pd; >> bool is_secure_dev; >> + char *servloc_name; >> /* Lock for lists */ >> spinlock_t lock; >> /* lock for allocations */ >> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques >> return false; >> } >> >> +static struct fastrpc_static_pd *fastrpc_get_spd_session( >> + struct fastrpc_user *fl) > Single line, please > >> +{ >> + int i; >> + struct fastrpc_static_pd *spd = NULL; >> + struct fastrpc_channel_ctx *cctx = fl->cctx; >> + >> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) { >> + if (!cctx->spd[i].servloc_name) >> + continue; >> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) { >> + spd = &cctx->spd[i]; >> + spd->fl = fl; >> + break; >> + } > Should there be a protection for spd->fl being overwritten / overtaken? > How should it be handled by the driver? > >> + } >> + >> + return spd; >> +} >> + >> static int fastrpc_init_create_static_process(struct fastrpc_user *fl, >> char __user *argp) >> { >> struct fastrpc_init_create_static init; >> struct fastrpc_invoke_args *args; >> struct fastrpc_phy_page pages[1]; >> + struct fastrpc_static_pd *spd = NULL; >> char *name; >> int err; >> struct { >> @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, >> goto err_name; >> } >> >> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME; >> + >> + spd = fastrpc_get_spd_session(fl); >> + if (!spd) { >> + err = -EUSERS; > "Too many users" ? > >> + goto err_name; >> + } >> + >> + if (!spd->ispdup) { > This is racy. What if the domain restarts right after this line? > >> + err = -ENOTCONN; >> + goto err_name; >> + } >> + fl->spd = spd; >> if (!fl->cctx->remote_heap) { >> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, >> &fl->cctx->remote_heap); >> @@ -1645,6 +1705,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]; >> + struct fastrpc_static_pd *spd = NULL; >> int tgid = fl->tgid; >> u32 sc; >> >> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd) >> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0); >> fl->pd = pd; >> >> + if (pd == SENSORS_PD) { >> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID) >> + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME; >> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID) >> + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME; >> + >> + spd = fastrpc_get_spd_session(fl); >> + if (!spd) >> + return -EUSERS; >> + >> + if (!spd->ispdup) >> + return -ENOTCONN; >> + >> + fl->spd = spd; >> + } >> + >> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, >> sc, &args[0]); >> } >> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, >> return err; >> } >> >> +static void fastrpc_notify_users(struct fastrpc_user *user) >> +{ >> + struct fastrpc_invoke_ctx *ctx; >> + >> + spin_lock(&user->lock); >> + list_for_each_entry(ctx, &user->pending, node) { >> + ctx->retval = -EPIPE; >> + complete(&ctx->work); >> + } >> + spin_unlock(&user->lock); >> +} >> + >> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx, >> + char *servloc_name) >> +{ >> + struct fastrpc_user *fl; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&cctx->lock, flags); >> + list_for_each_entry(fl, &cctx->users, user) { >> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name)) >> + fastrpc_notify_users(fl); >> + } >> + spin_unlock_irqrestore(&cctx->lock, flags); >> +} >> + >> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv) >> +{ >> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv; >> + struct fastrpc_channel_ctx *cctx; >> + >> + if (!spd) >> + return; >> + >> + cctx = spd->cctx; >> + switch (state) { >> + case SERVREG_SERVICE_STATE_DOWN: >> + dev_info(&cctx->rpdev->dev, >> + "%s: %s (%s) is down for PDR on %s\n", >> + __func__, spd->spdname, >> + spd->servloc_name, >> + domains[cctx->domain_id]); > Too noisy. Is it really dev_info()? Or dev_warn? There should be no need > for __func__. > >> + spd->ispdup = false; >> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name); >> + break; >> + case SERVREG_SERVICE_STATE_UP: >> + dev_info(&cctx->rpdev->dev, >> + "%s: %s (%s) is up for PDR on %s\n", >> + __func__, spd->spdname, >> + spd->servloc_name, >> + domains[cctx->domain_id]); > Same comment here. > >> + spd->ispdup = true; >> + break; >> + default: >> + break; >> + } >> +} >> + >> static const struct file_operations fastrpc_fops = { >> .open = fastrpc_device_open, >> .release = fastrpc_device_release, >> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct >> return err; >> } >> >> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name, >> + char *service_name, char *service_path, int domain, int spd_session) >> +{ >> + int err = 0; >> + struct pdr_handle *handle = NULL; >> + struct pdr_service *service = NULL; >> + >> + /* Register the service locator's callback function */ >> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]); >> + if (IS_ERR(handle)) { >> + err = PTR_ERR(handle); >> + goto bail; >> + } >> + cctx->spd[spd_session].pdrhandle = handle; >> + cctx->spd[spd_session].servloc_name = client_name; >> + cctx->spd[spd_session].spdname = service_path; >> + cctx->spd[spd_session].cctx = cctx; >> + service = pdr_add_lookup(handle, service_name, service_path); >> + if (IS_ERR(service)) { >> + err = PTR_ERR(service); >> + goto bail; >> + } >> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n", >> + __func__, service_name, client_name, service_path); > Definitely too noisy. Drop __func__ everywhere. Use dev_dbg instead. > >> + >> +bail: >> + if (err) { >> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n", >> + __func__, service_name, client_name, service_path, err); > dev_warn, drop brackets, align properly. Did you run this through checkpatch.pl --strict? > >> + } >> + return err; >> +} >> + >> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) >> { >> struct device *rdev = &rpdev->dev; >> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) >> goto fdev_error; >> } >> >> + if (domain_id == ADSP_DOMAIN_ID) { >> + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME, >> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0); >> + if (err) >> + goto populate_error; >> + >> + err = fastrpc_setup_service_locator(data, >> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME, >> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1); >> + if (err) >> + goto populate_error; >> + } else if (domain_id == SDSP_DOMAIN_ID) { >> + err = fastrpc_setup_service_locator(data, >> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME, >> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0); >> + if (err) >> + goto populate_error; >> + } >> + >> kref_init(&data->refcount); >> >> dev_set_drvdata(&rpdev->dev, data); >> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) >> return err; >> } >> >> -static void fastrpc_notify_users(struct fastrpc_user *user) >> -{ >> - struct fastrpc_invoke_ctx *ctx; >> - >> - spin_lock(&user->lock); >> - list_for_each_entry(ctx, &user->pending, node) { >> - ctx->retval = -EPIPE; >> - complete(&ctx->work); >> - } >> - spin_unlock(&user->lock); >> -} >> - >> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) >> { >> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev); >> struct fastrpc_buf *buf, *b; >> struct fastrpc_user *user; >> unsigned long flags; >> + int i; >> >> /* No invocations past this point */ >> spin_lock_irqsave(&cctx->lock, flags); >> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) >> if (cctx->remote_heap) >> fastrpc_buf_free(cctx->remote_heap); >> >> + for (i = 0; i < FASTRPC_MAX_SPD; i++) { >> + if (cctx->spd[i].pdrhandle) >> + pdr_handle_release(cctx->spd[i].pdrhandle); >> + } >> + >> of_platform_depopulate(&rpdev->dev); >> >> fastrpc_channel_ctx_put(cctx); >> -- >> 2.43.0 >>
On 6/7/2024 7:15 PM, Caleb Connolly wrote: > > > On 06/06/2024 18:59, Ekansh Gupta wrote: >> Static PDs are created on DSPs to support specific use cases like Audio >> and Sensors. The static PDs uses any CPU requirements like file >> operations or memory need with the help of a daemon running on the CPU. >> Audio and sensors daemons attaches to audio PD and sensors PD on DSP. >> Audio PD expects some CMA memory for dynamic loading purpose which is >> allocated and sent to DSP in fastrpc_init_create_static_process call. >> For sensor daemon, the expectation is just to attach to sensors PD and >> take up any requests made by the PD(like file operations etc.). >> >> Static PDs run on the audio and sensor supporting subsystem which can >> be ADSP or SDSP. They are expected to support PD restart. There are some >> CPU resources like buffers etc. for static PDs which are expected to be >> cleaned up by fastrpc driver during PDR scenario. For this, there is a >> requirement of PD service locator to get the event notifications for >> static PD services. Also when events are received, the driver needs to >> handle based on PD states. >> >> PDR handling is required for static PD only. There are no static PD >> supported on MDSP or CDSP hence no PDR handling is required. PDR is also >> not required for root_pd as if root_pd is shutting down, that basically >> suggests that the remoteproc itself is shutting down which is handled >> with rpmsg functionalities(probe and remove). >> >> Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> >> --- >> drivers/misc/Kconfig | 2 + >> drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++--- > > I think this justifies introducing a new C file. The functionality added here should be quite easily abstracted behind a sensible interface. Thanks for the suggestion, Caleb. I'm dropping this patch from the series for now. I'll move PD notification specific implementations to a new C file and send the patches separately. I'll also address all your comments there. --ekansh >> 2 files changed, 195 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig >> index faf983680040..e2d83cd085b5 100644 >> --- a/drivers/misc/Kconfig >> +++ b/drivers/misc/Kconfig >> @@ -280,8 +280,10 @@ config QCOM_FASTRPC >> tristate "Qualcomm FastRPC" >> depends on ARCH_QCOM || COMPILE_TEST >> depends on RPMSG >> + depends on NET >> select DMA_SHARED_BUFFER >> select QCOM_SCM >> + select QCOM_PDR_HELPERS >> help >> Provides a communication mechanism that allows for clients to >> make remote method invocations across processor boundary to >> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c >> index abdd35b7c3ad..13e368279765 100644 >> --- a/drivers/misc/fastrpc.c >> +++ b/drivers/misc/fastrpc.c >> @@ -22,6 +22,7 @@ >> #include <linux/firmware/qcom/qcom_scm.h> >> #include <uapi/misc/fastrpc.h> >> #include <linux/of_reserved_mem.h> >> +#include <linux/soc/qcom/pdr.h> >> #define ADSP_DOMAIN_ID (0) >> #define MDSP_DOMAIN_ID (1) >> @@ -29,6 +30,7 @@ >> #define CDSP_DOMAIN_ID (3) >> #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/ >> #define FASTRPC_MAX_SESSIONS 14 >> +#define FASTRPC_MAX_SPD 4 > > Why 4? This patch only makes use of two. >> #define FASTRPC_MAX_VMIDS 16 >> #define FASTRPC_ALIGN 128 >> #define FASTRPC_MAX_FDLIST 16 >> @@ -105,6 +107,18 @@ >> #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) >> +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp" >> +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio" >> +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd" >> + >> +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp" >> +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg" >> +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd" >> + >> +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi" >> +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME >> +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd" > > This data should be defined in some const static data struct, not as a bunch of macros, then you can actually describe the relationship between a domain_id and the fact that the ADSP registers two PDR lookups. See my comments in fastrpc_setup_service_locator() and where it's called in probe. > > struct fastrpc_pdr_domain { > const char *servloc_client_name; > const char *service_name; > const char *pd_name; > }; > > static const struct fastrpc_pdr_domain adsp_pdr_services[] = { > { > .servloc_client_name = "audio_pdr_adsp"; > .service_name = "avs/audio"; > .pd_name = "msm/adsp/audio_pd"; > }, > { > .servloc_client_name = "sensors_pdr_adsp"; > ... > }; >> + >> static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp", >> "sdsp", "cdsp"}; >> struct fastrpc_phy_page { >> @@ -259,6 +273,15 @@ struct fastrpc_session_ctx { >> bool valid; >> }; >> +struct fastrpc_static_pd { >> + char *servloc_name; >> + char *spdname; >> + void *pdrhandle; >> + struct fastrpc_channel_ctx *cctx; >> + struct fastrpc_user *fl; >> + bool ispdup; >> +}; >> + >> struct fastrpc_channel_ctx { >> int domain_id; >> int sesscount; >> @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx { >> struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS]; >> struct rpmsg_device *rpdev; >> struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS]; >> + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD]; >> spinlock_t lock; >> struct idr ctx_idr; >> struct list_head users; >> @@ -297,10 +321,12 @@ struct fastrpc_user { >> struct fastrpc_channel_ctx *cctx; >> struct fastrpc_session_ctx *sctx; >> struct fastrpc_buf *init_mem; >> + struct fastrpc_static_pd *spd; >> int tgid; >> int pd; >> bool is_secure_dev; >> + char *servloc_name; > > This is duplicated from spd->servloc_name >> /* Lock for lists */ >> spinlock_t lock; >> /* lock for allocations */ >> @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques >> return false; >> } >> +static struct fastrpc_static_pd *fastrpc_get_spd_session( > + struct fastrpc_user *fl) > > Every caller of this function has the same handling (checking !spd and then !spd->ispdup and returning different errors in each case). Please lift this common error handling into this function. While at it, I'd propose dropping the opaque *fl pointer and instead passing in the data which is actually used. The servloc_name could be looked up from the fastrpc_pdr_domain data. > > static int fastrpc_pdr_is_up(int pd, int domain_id, struct > fastrpc_static_pd *spds, > struct fastrpc_static_pd *spd) >> +{ >> + int i; >> + struct fastrpc_static_pd *spd = NULL; >> + struct fastrpc_channel_ctx *cctx = fl->cctx; >> + >> + for (i = 0; i < FASTRPC_MAX_SPD ; i++) { >> + if (!cctx->spd[i].servloc_name) >> + continue; >> + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) { >> + spd = &cctx->spd[i]; >> + spd->fl = fl; >> + break; >> + } >> + } >> + >> + return spd; >> +} >> + >> static int fastrpc_init_create_static_process(struct fastrpc_user *fl, >> char __user *argp) >> { >> struct fastrpc_init_create_static init; >> struct fastrpc_invoke_args *args; >> struct fastrpc_phy_page pages[1]; >> + struct fastrpc_static_pd *spd = NULL; >> char *name; >> int err; >> struct { >> @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, >> goto err_name; >> } >> + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME; >> + >> + spd = fastrpc_get_spd_session(fl); >> + if (!spd) { >> + err = -EUSERS; >> + goto err_name; >> + } >> + >> + if (!spd->ispdup) { >> + err = -ENOTCONN; >> + goto err_name; >> + } >> + fl->spd = spd; >> if (!fl->cctx->remote_heap) { >> err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, >> &fl->cctx->remote_heap); >> @@ -1645,6 +1705,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]; >> + struct fastrpc_static_pd *spd = NULL; >> int tgid = fl->tgid; >> u32 sc; >> @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd) >> sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0); >> fl->pd = pd; >> + if (pd == SENSORS_PD) { > > Why is this only relevant for the sensors pd? >> + if (fl->cctx->domain_id == ADSP_DOMAIN_ID) >> + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME; >> + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID) >> + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME; > What if domain_id isn't either of these? Should this be checked? If not, why? > > This whole block could be replaced with a call to the fastrpc_pdr_is_up() function I proposed. >> + >> + spd = fastrpc_get_spd_session(fl); >> + if (!spd) >> + return -EUSERS; >> + >> + if (!spd->ispdup) >> + return -ENOTCONN; >> + >> + fl->spd = spd; >> + } >> + >> return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, >> sc, &args[0]); >> } >> @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, >> return err; >> } >> +static void fastrpc_notify_users(struct fastrpc_user *user) >> +{ >> + struct fastrpc_invoke_ctx *ctx; >> + >> + spin_lock(&user->lock); >> + list_for_each_entry(ctx, &user->pending, node) { >> + ctx->retval = -EPIPE; >> + complete(&ctx->work); >> + } >> + spin_unlock(&user->lock); >> +} >> + >> +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx, >> + char *servloc_name) >> +{ >> + struct fastrpc_user *fl; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&cctx->lock, flags); >> + list_for_each_entry(fl, &cctx->users, user) { >> + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name)) >> + fastrpc_notify_users(fl); >> + } >> + spin_unlock_irqrestore(&cctx->lock, flags); >> +} >> + >> +static void fastrpc_pdr_cb(int state, char *service_path, void *priv) >> +{ >> + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv; >> + struct fastrpc_channel_ctx *cctx; >> + >> + if (!spd) >> + return; >> + >> + cctx = spd->cctx; >> + switch (state) { >> + case SERVREG_SERVICE_STATE_DOWN: >> + dev_info(&cctx->rpdev->dev, >> + "%s: %s (%s) is down for PDR on %s\n", >> + __func__, spd->spdname, >> + spd->servloc_name, >> + domains[cctx->domain_id]); > > dev_dbg >> + spd->ispdup = false; >> + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name); >> + break; >> + case SERVREG_SERVICE_STATE_UP: >> + dev_info(&cctx->rpdev->dev, >> + "%s: %s (%s) is up for PDR on %s\n", >> + __func__, spd->spdname, >> + spd->servloc_name, >> + domains[cctx->domain_id]); >> + spd->ispdup = true; >> + break; >> + default: >> + break; >> + } >> +} >> + >> static const struct file_operations fastrpc_fops = { >> .open = fastrpc_device_open, >> .release = fastrpc_device_release, >> @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct >> return err; >> } >> +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name, >> + char *service_name, char *service_path, int domain, int spd_session) > > This function should instead take a struct fastrpc_pdr_domain *services array. >> +{ >> + int err = 0; >> + struct pdr_handle *handle = NULL; >> + struct pdr_service *service = NULL; >> + >> + /* Register the service locator's callback function */ >> + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]); >> + if (IS_ERR(handle)) { >> + err = PTR_ERR(handle); >> + goto bail; >> + } >> + cctx->spd[spd_session].pdrhandle = handle; >> + cctx->spd[spd_session].servloc_name = client_name; >> + cctx->spd[spd_session].spdname = service_path; >> + cctx->spd[spd_session].cctx = cctx; >> + service = pdr_add_lookup(handle, service_name, service_path); >> + if (IS_ERR(service)) { >> + err = PTR_ERR(service); >> + goto bail; >> + } >> + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n", > dev_dbg >> + __func__, service_name, client_name, service_path); >> + >> +bail: >> + if (err) { >> + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n", > dev_err >> + __func__, service_name, client_name, service_path, err); >> + } >> + return err; >> +} >> + >> static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) >> { >> struct device *rdev = &rpdev->dev; >> @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) >> goto fdev_error; >> } >> + if (domain_id == ADSP_DOMAIN_ID) { >> + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME, >> + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0); >> + if (err) >> + goto populate_error; >> + >> + err = fastrpc_setup_service_locator(data, >> + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME, >> + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1); > > I assume this is basically a nop on platforms where this service doesn't exist? A comment mentioning this would be nice. >> + if (err) >> + goto populate_error; >> + } else if (domain_id == SDSP_DOMAIN_ID) { >> + err = fastrpc_setup_service_locator(data, >> + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME, >> + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0); >> + if (err) >> + goto populate_error; >> + } > > This block should be moved into the domain_id switch/case which is directly above it. > >> + >> kref_init(&data->refcount); >> dev_set_drvdata(&rpdev->dev, data); >> @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) >> return err; >> } >> -static void fastrpc_notify_users(struct fastrpc_user *user) >> -{ >> - struct fastrpc_invoke_ctx *ctx; >> - >> - spin_lock(&user->lock); >> - list_for_each_entry(ctx, &user->pending, node) { >> - ctx->retval = -EPIPE; >> - complete(&ctx->work); >> - } >> - spin_unlock(&user->lock); >> -} >> - >> static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) >> { >> struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev); >> struct fastrpc_buf *buf, *b; >> struct fastrpc_user *user; >> unsigned long flags; >> + int i; >> /* No invocations past this point */ >> spin_lock_irqsave(&cctx->lock, flags); >> @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) >> if (cctx->remote_heap) >> fastrpc_buf_free(cctx->remote_heap); >> + for (i = 0; i < FASTRPC_MAX_SPD; i++) { >> + if (cctx->spd[i].pdrhandle) >> + pdr_handle_release(cctx->spd[i].pdrhandle); >> + } >> + >> of_platform_depopulate(&rpdev->dev); >> fastrpc_channel_ctx_put(cctx); >
On Mon, Jun 10, 2024 at 02:35:48PM +0530, Ekansh Gupta wrote: > > > On 6/7/2024 4:55 PM, Dmitry Baryshkov wrote: > > On Thu, Jun 06, 2024 at 10:29:25PM +0530, Ekansh Gupta wrote: > >> Static PDs are created on DSPs to support specific use cases like Audio > >> and Sensors. The static PDs uses any CPU requirements like file > >> operations or memory need with the help of a daemon running on the CPU. > > What do you mean by 'CPU requirements' here? > Something like file system request or any memory requirement. For these types of requirements > PD will rely on CPU process. So, this is not 'CPU requirements'. It should be something like 'requests to the HLOS'. CPU requirements would usually mean something requiring CPU cycles or min/max freq, etc. > > Planning to drop this patch from the series and move the pd notification specific implementations > to a different .c file as per Caleb's suggestion. > > I'll address all other comments in the new patch series. Ack
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index faf983680040..e2d83cd085b5 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -280,8 +280,10 @@ config QCOM_FASTRPC tristate "Qualcomm FastRPC" depends on ARCH_QCOM || COMPILE_TEST depends on RPMSG + depends on NET select DMA_SHARED_BUFFER select QCOM_SCM + select QCOM_PDR_HELPERS help Provides a communication mechanism that allows for clients to make remote method invocations across processor boundary to diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index abdd35b7c3ad..13e368279765 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -22,6 +22,7 @@ #include <linux/firmware/qcom/qcom_scm.h> #include <uapi/misc/fastrpc.h> #include <linux/of_reserved_mem.h> +#include <linux/soc/qcom/pdr.h> #define ADSP_DOMAIN_ID (0) #define MDSP_DOMAIN_ID (1) @@ -29,6 +30,7 @@ #define CDSP_DOMAIN_ID (3) #define FASTRPC_DEV_MAX 4 /* adsp, mdsp, slpi, cdsp*/ #define FASTRPC_MAX_SESSIONS 14 +#define FASTRPC_MAX_SPD 4 #define FASTRPC_MAX_VMIDS 16 #define FASTRPC_ALIGN 128 #define FASTRPC_MAX_FDLIST 16 @@ -105,6 +107,18 @@ #define miscdev_to_fdevice(d) container_of(d, struct fastrpc_device, miscdev) +#define AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME "audio_pdr_adsp" +#define AUDIO_PDR_ADSP_SERVICE_NAME "avs/audio" +#define ADSP_AUDIOPD_NAME "msm/adsp/audio_pd" + +#define SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_adsp" +#define SENSORS_PDR_ADSP_SERVICE_NAME "tms/servreg" +#define ADSP_SENSORPD_NAME "msm/adsp/sensor_pd" + +#define SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME "sensors_pdr_slpi" +#define SENSORS_PDR_SLPI_SERVICE_NAME SENSORS_PDR_ADSP_SERVICE_NAME +#define SLPI_SENSORPD_NAME "msm/slpi/sensor_pd" + static const char *domains[FASTRPC_DEV_MAX] = { "adsp", "mdsp", "sdsp", "cdsp"}; struct fastrpc_phy_page { @@ -259,6 +273,15 @@ struct fastrpc_session_ctx { bool valid; }; +struct fastrpc_static_pd { + char *servloc_name; + char *spdname; + void *pdrhandle; + struct fastrpc_channel_ctx *cctx; + struct fastrpc_user *fl; + bool ispdup; +}; + struct fastrpc_channel_ctx { int domain_id; int sesscount; @@ -266,6 +289,7 @@ struct fastrpc_channel_ctx { struct qcom_scm_vmperm vmperms[FASTRPC_MAX_VMIDS]; struct rpmsg_device *rpdev; struct fastrpc_session_ctx session[FASTRPC_MAX_SESSIONS]; + struct fastrpc_static_pd spd[FASTRPC_MAX_SPD]; spinlock_t lock; struct idr ctx_idr; struct list_head users; @@ -297,10 +321,12 @@ struct fastrpc_user { struct fastrpc_channel_ctx *cctx; struct fastrpc_session_ctx *sctx; struct fastrpc_buf *init_mem; + struct fastrpc_static_pd *spd; int tgid; int pd; bool is_secure_dev; + char *servloc_name; /* Lock for lists */ spinlock_t lock; /* lock for allocations */ @@ -1230,12 +1256,33 @@ static bool is_session_rejected(struct fastrpc_user *fl, bool unsigned_pd_reques return false; } +static struct fastrpc_static_pd *fastrpc_get_spd_session( + struct fastrpc_user *fl) +{ + int i; + struct fastrpc_static_pd *spd = NULL; + struct fastrpc_channel_ctx *cctx = fl->cctx; + + for (i = 0; i < FASTRPC_MAX_SPD ; i++) { + if (!cctx->spd[i].servloc_name) + continue; + if (!strcmp(fl->servloc_name, cctx->spd[i].servloc_name)) { + spd = &cctx->spd[i]; + spd->fl = fl; + break; + } + } + + return spd; +} + static int fastrpc_init_create_static_process(struct fastrpc_user *fl, char __user *argp) { struct fastrpc_init_create_static init; struct fastrpc_invoke_args *args; struct fastrpc_phy_page pages[1]; + struct fastrpc_static_pd *spd = NULL; char *name; int err; struct { @@ -1270,6 +1317,19 @@ static int fastrpc_init_create_static_process(struct fastrpc_user *fl, goto err_name; } + fl->servloc_name = AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME; + + spd = fastrpc_get_spd_session(fl); + if (!spd) { + err = -EUSERS; + goto err_name; + } + + if (!spd->ispdup) { + err = -ENOTCONN; + goto err_name; + } + fl->spd = spd; if (!fl->cctx->remote_heap) { err = fastrpc_remote_heap_alloc(fl, fl->sctx->dev, init.memlen, &fl->cctx->remote_heap); @@ -1645,6 +1705,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]; + struct fastrpc_static_pd *spd = NULL; int tgid = fl->tgid; u32 sc; @@ -1654,6 +1715,22 @@ static int fastrpc_init_attach(struct fastrpc_user *fl, int pd) sc = FASTRPC_SCALARS(FASTRPC_RMID_INIT_ATTACH, 1, 0); fl->pd = pd; + if (pd == SENSORS_PD) { + if (fl->cctx->domain_id == ADSP_DOMAIN_ID) + fl->servloc_name = SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME; + else if (fl->cctx->domain_id == SDSP_DOMAIN_ID) + fl->servloc_name = SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME; + + spd = fastrpc_get_spd_session(fl); + if (!spd) + return -EUSERS; + + if (!spd->ispdup) + return -ENOTCONN; + + fl->spd = spd; + } + return fastrpc_internal_invoke(fl, true, FASTRPC_INIT_HANDLE, sc, &args[0]); } @@ -2129,6 +2206,64 @@ static long fastrpc_device_ioctl(struct file *file, unsigned int cmd, return err; } +static void fastrpc_notify_users(struct fastrpc_user *user) +{ + struct fastrpc_invoke_ctx *ctx; + + spin_lock(&user->lock); + list_for_each_entry(ctx, &user->pending, node) { + ctx->retval = -EPIPE; + complete(&ctx->work); + } + spin_unlock(&user->lock); +} + +static void fastrpc_notify_pdr_drivers(struct fastrpc_channel_ctx *cctx, + char *servloc_name) +{ + struct fastrpc_user *fl; + unsigned long flags; + + spin_lock_irqsave(&cctx->lock, flags); + list_for_each_entry(fl, &cctx->users, user) { + if (fl->servloc_name && !strcmp(servloc_name, fl->servloc_name)) + fastrpc_notify_users(fl); + } + spin_unlock_irqrestore(&cctx->lock, flags); +} + +static void fastrpc_pdr_cb(int state, char *service_path, void *priv) +{ + struct fastrpc_static_pd *spd = (struct fastrpc_static_pd *)priv; + struct fastrpc_channel_ctx *cctx; + + if (!spd) + return; + + cctx = spd->cctx; + switch (state) { + case SERVREG_SERVICE_STATE_DOWN: + dev_info(&cctx->rpdev->dev, + "%s: %s (%s) is down for PDR on %s\n", + __func__, spd->spdname, + spd->servloc_name, + domains[cctx->domain_id]); + spd->ispdup = false; + fastrpc_notify_pdr_drivers(cctx, spd->servloc_name); + break; + case SERVREG_SERVICE_STATE_UP: + dev_info(&cctx->rpdev->dev, + "%s: %s (%s) is up for PDR on %s\n", + __func__, spd->spdname, + spd->servloc_name, + domains[cctx->domain_id]); + spd->ispdup = true; + break; + default: + break; + } +} + static const struct file_operations fastrpc_fops = { .open = fastrpc_device_open, .release = fastrpc_device_release, @@ -2248,6 +2383,39 @@ static int fastrpc_device_register(struct device *dev, struct fastrpc_channel_ct return err; } +static int fastrpc_setup_service_locator(struct fastrpc_channel_ctx *cctx, char *client_name, + char *service_name, char *service_path, int domain, int spd_session) +{ + int err = 0; + struct pdr_handle *handle = NULL; + struct pdr_service *service = NULL; + + /* Register the service locator's callback function */ + handle = pdr_handle_alloc(fastrpc_pdr_cb, &cctx->spd[spd_session]); + if (IS_ERR(handle)) { + err = PTR_ERR(handle); + goto bail; + } + cctx->spd[spd_session].pdrhandle = handle; + cctx->spd[spd_session].servloc_name = client_name; + cctx->spd[spd_session].spdname = service_path; + cctx->spd[spd_session].cctx = cctx; + service = pdr_add_lookup(handle, service_name, service_path); + if (IS_ERR(service)) { + err = PTR_ERR(service); + goto bail; + } + pr_info("fastrpc: %s: pdr_add_lookup enabled for %s (%s, %s)\n", + __func__, service_name, client_name, service_path); + +bail: + if (err) { + pr_warn("fastrpc: %s: failed for %s (%s, %s)with err %d\n", + __func__, service_name, client_name, service_path, err); + } + return err; +} + static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) { struct device *rdev = &rpdev->dev; @@ -2326,6 +2494,25 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) goto fdev_error; } + if (domain_id == ADSP_DOMAIN_ID) { + err = fastrpc_setup_service_locator(data, AUDIO_PDR_SERVICE_LOCATION_CLIENT_NAME, + AUDIO_PDR_ADSP_SERVICE_NAME, ADSP_AUDIOPD_NAME, domain_id, 0); + if (err) + goto populate_error; + + err = fastrpc_setup_service_locator(data, + SENSORS_PDR_ADSP_SERVICE_LOCATION_CLIENT_NAME, + SENSORS_PDR_ADSP_SERVICE_NAME, ADSP_SENSORPD_NAME, domain_id, 1); + if (err) + goto populate_error; + } else if (domain_id == SDSP_DOMAIN_ID) { + err = fastrpc_setup_service_locator(data, + SENSORS_PDR_SLPI_SERVICE_LOCATION_CLIENT_NAME, + SENSORS_PDR_SLPI_SERVICE_NAME, SLPI_SENSORPD_NAME, domain_id, 0); + if (err) + goto populate_error; + } + kref_init(&data->refcount); dev_set_drvdata(&rpdev->dev, data); @@ -2355,24 +2542,13 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) return err; } -static void fastrpc_notify_users(struct fastrpc_user *user) -{ - struct fastrpc_invoke_ctx *ctx; - - spin_lock(&user->lock); - list_for_each_entry(ctx, &user->pending, node) { - ctx->retval = -EPIPE; - complete(&ctx->work); - } - spin_unlock(&user->lock); -} - static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) { struct fastrpc_channel_ctx *cctx = dev_get_drvdata(&rpdev->dev); struct fastrpc_buf *buf, *b; struct fastrpc_user *user; unsigned long flags; + int i; /* No invocations past this point */ spin_lock_irqsave(&cctx->lock, flags); @@ -2393,6 +2569,11 @@ static void fastrpc_rpmsg_remove(struct rpmsg_device *rpdev) if (cctx->remote_heap) fastrpc_buf_free(cctx->remote_heap); + for (i = 0; i < FASTRPC_MAX_SPD; i++) { + if (cctx->spd[i].pdrhandle) + pdr_handle_release(cctx->spd[i].pdrhandle); + } + of_platform_depopulate(&rpdev->dev); fastrpc_channel_ctx_put(cctx);
Static PDs are created on DSPs to support specific use cases like Audio and Sensors. The static PDs uses any CPU requirements like file operations or memory need with the help of a daemon running on the CPU. Audio and sensors daemons attaches to audio PD and sensors PD on DSP. Audio PD expects some CMA memory for dynamic loading purpose which is allocated and sent to DSP in fastrpc_init_create_static_process call. For sensor daemon, the expectation is just to attach to sensors PD and take up any requests made by the PD(like file operations etc.). Static PDs run on the audio and sensor supporting subsystem which can be ADSP or SDSP. They are expected to support PD restart. There are some CPU resources like buffers etc. for static PDs which are expected to be cleaned up by fastrpc driver during PDR scenario. For this, there is a requirement of PD service locator to get the event notifications for static PD services. Also when events are received, the driver needs to handle based on PD states. PDR handling is required for static PD only. There are no static PD supported on MDSP or CDSP hence no PDR handling is required. PDR is also not required for root_pd as if root_pd is shutting down, that basically suggests that the remoteproc itself is shutting down which is handled with rpmsg functionalities(probe and remove). Signed-off-by: Ekansh Gupta <quic_ekangupt@quicinc.com> --- drivers/misc/Kconfig | 2 + drivers/misc/fastrpc.c | 205 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 195 insertions(+), 12 deletions(-)