On 18-08-2026 09:49, Dmitry Baryshkov wrote: > On Mon, Aug 17, 2026 at 10:17:47AM +0530, Ekansh Gupta wrote: >> Implement the FastRPC remote procedure call path, allowing user-space to >> invoke methods on the DSP via DRM_IOCTL_QDA_REMOTE_INVOKE. >> >> qda_fastrpc.c / qda_fastrpc.h >> Implements the FastRPC protocol layer: argument marshalling >> (qda_fastrpc_invoke_pack), response unmarshalling >> (qda_fastrpc_invoke_unpack), and invocation context lifecycle >> management. Each invocation allocates a qda_fastrpc_invoke_ctx which >> tracks buffer descriptors, GEM objects, and the completion used to >> synchronise with the DSP response. >> >> Buffer arguments are identified by GEM handles. Userspace imports any >> DMA-BUF fd to a GEM handle with DRM_IOCTL_PRIME_FD_TO_HANDLE before >> invoking; the driver never accepts DMA-BUF fds directly. Each argument >> is described by its GEM handle, the user virtual address of the data >> and its length, from which the driver derives the offset within the >> buffer and the page-aligned range to describe to the DSP. Packing >> several arguments into one buffer, and any overlap handling, is left >> to user space. >> >> qda_rpmsg.c >> Implements qda_rpmsg_send_msg() which sends the wire-format >> fastrpc_msg (embedded as the first member of qda_msg) directly via >> rpmsg_send(), and qda_rpmsg_wait_for_rsp() which blocks on the context >> completion. The RPMsg callback dispatches responses to waiting >> contexts via the ctx_xa XArray. >> >> qda_ioctl.c >> qda_ioctl_invoke() drives the full invocation lifecycle: it builds the >> invocation context from the user-supplied arguments, packs the >> arguments into the message buffer, sends the message to the DSP, waits >> for the response, unpacks the output arguments back to user space and >> releases the context. >> >> include/uapi/drm/qda_accel.h >> Adds DRM_IOCTL_QDA_REMOTE_INVOKE with struct drm_qda_invoke_args and >> the per-argument descriptor struct drm_qda_fastrpc_invoke_args. >> >> Assisted-by: Claude:claude-sonnet-5 >> Signed-off-by: Ekansh Gupta <[email protected]> >> --- >> Changes in v2: >> - Drop the DMA-BUF fd argument path. Buffer arguments are now identified >> by GEM handles only; user space is responsible for importing fds to >> GEM handles before invoking (Dmitry Baryshkov) >> - Leave argument packing and overlap handling to user space rather than >> supporting several buffer-passing formats in the driver >> (Dmitry Baryshkov) >> --- >> drivers/accel/qda/Makefile | 1 + >> drivers/accel/qda/qda_drv.c | 8 + >> drivers/accel/qda/qda_drv.h | 8 + >> drivers/accel/qda/qda_fastrpc.c | 434 >> ++++++++++++++++++++++++++++++++++++++++ >> drivers/accel/qda/qda_fastrpc.h | 242 ++++++++++++++++++++++ >> drivers/accel/qda/qda_ioctl.c | 83 ++++++++ >> drivers/accel/qda/qda_ioctl.h | 1 + >> drivers/accel/qda/qda_rpmsg.c | 91 ++++++++- >> drivers/accel/qda/qda_rpmsg.h | 26 +++ >> include/uapi/drm/qda_accel.h | 42 ++++ >> 10 files changed, 934 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/accel/qda/Makefile b/drivers/accel/qda/Makefile >> index fb092e56d7f3..2d10420cd1ec 100644 >> --- a/drivers/accel/qda/Makefile >> +++ b/drivers/accel/qda/Makefile >> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_ACCEL_QDA) := qda.o >> qda-y := \ >> qda_cb.o \ >> qda_drv.o \ >> + qda_fastrpc.o \ >> qda_gem.o \ >> qda_ioctl.o \ >> qda_memory_dma.o \ >> diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c >> index a68a07d0ae56..f925bbcfa6e9 100644 >> --- a/drivers/accel/qda/qda_drv.c >> +++ b/drivers/accel/qda/qda_drv.c >> @@ -25,6 +25,8 @@ static int qda_open(struct drm_device *dev, struct >> drm_file *file) >> >> qda_file_priv->pid = current->pid; >> qda_file_priv->qda_dev = qda_dev_from_drm(dev); >> + qda_file_priv->remote_session_id = >> + >> atomic_inc_return(&qda_file_priv->qda_dev->remote_session_id_counter); > > What happens on the wraparound? What if some of those sessions are still > alive? collision would require 2^31 opens without close, is this reachable? Should I add a comment here? Or some how address this?> >> file->driver_priv = qda_file_priv; >> >> return 0; > > [...] > >> + >> +/* >> + * The driver only accepts GEM handles from user space; userspace imports >> + * DMA-BUF fds with DRM_IOCTL_PRIME_FD_TO_HANDLE before issuing any ioctl >> that >> + * carries a buffer reference. > > What else can it accept? will remove the obvious statement here> >> + * >> + * The caller owns the returned reference and must drop it with >> + * drm_gem_object_put(). >> + */ >> +static int get_gem_obj_from_handle(struct qda_fastrpc_invoke_ctx *ctx, u32 >> handle, >> + struct drm_gem_object **gem_obj) >> +{ >> + *gem_obj = drm_gem_object_lookup(ctx->file_priv, handle); >> + if (!*gem_obj) >> + return -ENOENT; >> + >> + return 0; >> +} >> + >> +static void setup_pages_from_gem_obj(struct qda_gem_obj *qda_gem_obj, >> + struct fastrpc_phy_page *pages) >> +{ >> + pages->addr = qda_gem_obj->dma_addr; >> + pages->size = qda_gem_obj->size; >> +} >> + >> +static u64 calculate_vma_offset(u64 user_ptr) >> +{ >> + struct vm_area_struct *vma; >> + u64 vma_offset = 0; >> + >> + mmap_read_lock(current->mm); >> + vma = find_vma(current->mm, user_ptr); >> + if (vma) >> + vma_offset = (user_ptr & PAGE_MASK) - vma->vm_start; >> + mmap_read_unlock(current->mm); >> + >> + return vma_offset; >> +} >> + >> +static u64 calculate_page_aligned_size(u64 ptr, u64 len) >> +{ >> + u64 pg_start = (ptr & PAGE_MASK) >> PAGE_SHIFT; >> + u64 pg_end = ((ptr + len - 1) & PAGE_MASK) >> PAGE_SHIFT; >> + >> + return (pg_end - pg_start + 1) * PAGE_SIZE; >> +} >> + >> +static struct fastrpc_invoke_buf *fastrpc_invoke_buf_start(union >> fastrpc_remote_arg *pra, int len) >> +{ >> + return (struct fastrpc_invoke_buf *)(&pra[len]); > > Ugh. Is it not start + len * sizeof(union fastrpc_remote_arg)? Can't you > just write it at the caller site? ack> >> +} >> + >> +static struct fastrpc_phy_page *fastrpc_phy_page_start(struct >> fastrpc_invoke_buf *buf, int len) >> +{ >> + return (struct fastrpc_phy_page *)(&buf[len]); >> +} >> + >> +static size_t fastrpc_get_meta_size(struct qda_fastrpc_invoke_ctx *ctx) >> +{ >> + return (sizeof(struct fastrpc_remote_buf) + > > Just one buffer? I'll add a comment explaining this formulation like it was in fastrpc[1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/misc/fastrpc.c#n982> >> + sizeof(struct fastrpc_invoke_buf) + >> + sizeof(struct fastrpc_phy_page)) * ctx->nscalars + >> + sizeof(u64) * FASTRPC_MAX_HANDLELIST + >> + sizeof(u32) * FASTRPC_MAX_CRCLIST; >> +} >> + >> +/** >> + * qda_fastrpc_cleanup_handlelist() - Release DMA handles the DSP no longer >> needs >> + * @ctx: Invocation context whose response buffer contains the handle list >> + * >> + * The DSP signals handle release by writing GEM handles into the handle >> list >> + * region of the response buffer. Walk the list and close each handle, which >> + * drops the GEM reference and tears down the IOMMU mapping. This must be >> called >> + * after dma_rmb() has made the DSP's writes visible. >> + */ >> +void qda_fastrpc_cleanup_handlelist(struct qda_fastrpc_invoke_ctx *ctx) >> +{ >> + u64 *handlelist; >> + int i; >> + >> + handlelist = (u64 *)(ctx->pages + ctx->nscalars); >> + for (i = 0; i < FASTRPC_MAX_HANDLELIST; i++) { >> + if (!handlelist[i]) >> + break; >> + drm_gem_handle_delete(ctx->file_priv, (u32)handlelist[i]); >> + } >> +} >> + >> +/** >> + * qda_fastrpc_context_free() - Release an invocation context >> + * @ref: Reference counter embedded in the context >> + * >> + * Called when the last reference is dropped, which may be either the caller >> + * that issued the invocation or the response callback. The context is >> removed >> + * from the lookup table here so that a late response can never find a >> context >> + * that is already being destroyed. >> + * >> + * ctx->req, ctx->rsp, ctx->inbuf and ctx->input_pages point into >> kernel-owned >> + * GEM mappings and are released along with ctx->gem_objs[]. >> + */ >> +void qda_fastrpc_context_free(struct kref *ref) >> +{ >> + struct qda_fastrpc_invoke_ctx *ctx; >> + int i; >> + >> + ctx = container_of(ref, struct qda_fastrpc_invoke_ctx, refcount); >> + >> + xa_erase(&ctx->qdev->ctx_xa, ctx->ctxid >> 4); >> + >> + if (ctx->gem_objs) { >> + for (i = 0; i < ctx->nscalars; ++i) { >> + if (ctx->gem_objs[i]) >> + drm_gem_object_put(ctx->gem_objs[i]); >> + } >> + kfree(ctx->gem_objs); >> + } >> + >> + if (ctx->msg_gem_obj) >> + drm_gem_object_put(&ctx->msg_gem_obj->base); >> + >> + kfree(ctx->args); >> + kfree(ctx); >> +} >> + >> +/** >> + * qda_fastrpc_flush_pending() - Fail every outstanding invocation >> + * @qdev: Device whose contexts should be flushed >> + * >> + * Called when the remote processor goes away. The DSP will never answer the >> + * in-flight invocations, so this stands in for the response callback: it >> wakes >> + * each waiter with an error and drops the reference the send path took for >> that >> + * callback. The waiter then tears the context down, releasing the GEM >> objects >> + * it pinned. >> + * >> + * Must run after drm_dev_unplug() has drained qda_rpmsg_cb(), so the real >> + * callback can no longer run and this is the only path completing a >> context. >> + */ >> +void qda_fastrpc_flush_pending(struct qda_dev *qdev) >> +{ >> + struct qda_fastrpc_invoke_ctx *ctx; >> + unsigned long idx; >> + >> + xa_for_each(&qdev->ctx_xa, idx, ctx) { >> + ctx->retval = -EPIPE; >> + complete(&ctx->work); >> + kref_put(&ctx->refcount, qda_fastrpc_context_free); >> + } >> +} >> + >> +/** >> + * qda_fastrpc_context_alloc() - Allocate a FastRPC invocation context >> + * @qdev: Device whose lookup table the context is registered in >> + * >> + * Return: Pointer to the new context, or ERR_PTR on failure >> + */ >> +struct qda_fastrpc_invoke_ctx *qda_fastrpc_context_alloc(struct qda_dev >> *qdev) >> +{ >> + struct qda_fastrpc_invoke_ctx *ctx; >> + int ret; >> + u32 id; >> + >> + ctx = kzalloc_obj(*ctx); >> + if (!ctx) >> + return ERR_PTR(-ENOMEM); >> + >> + INIT_LIST_HEAD(&ctx->node); >> + init_completion(&ctx->work); >> + kref_init(&ctx->refcount); >> + ctx->qdev = qdev; >> + ctx->retval = -1; >> + ctx->pid = current->pid; >> + >> + ret = xa_alloc(&qdev->ctx_xa, &id, ctx, xa_limit_32b, GFP_KERNEL); >> + if (ret) { >> + kfree(ctx); >> + return ERR_PTR(ret); >> + } >> + ctx->ctxid = id << 4; >> + >> + return ctx; >> +} >> + >> +/* >> + * Resolve the physical address of an in/out buffer argument. The buffer is >> + * backed either by a kernel-owned GEM object already recorded in >> + * ctx->gem_objs[i], or by a user-supplied GEM handle in >> ctx->args[i].handle. >> + */ >> +static int process_msg_buffer(struct qda_fastrpc_invoke_ctx *ctx, int i, >> + union fastrpc_remote_arg *rpra, >> + struct fastrpc_phy_page *pages) >> +{ >> + u64 len = ctx->args[i].length; >> + struct qda_gem_obj *qda_gem_obj; >> + >> + rpra[i].buf.pv = ctx->args[i].ptr; >> + >> + if (ctx->gem_objs[i]) { >> + u64 unaligned_addr, offset; >> + >> + qda_gem_obj = to_qda_gem_obj(ctx->gem_objs[i]); >> + offset = ctx->args[i].ptr - (u64)(uintptr_t)qda_gem_obj->virt; >> + >> + unaligned_addr = qda_gem_obj->dma_addr + offset; >> + pages[i].addr = unaligned_addr & PAGE_MASK; >> + pages[i].size = calculate_page_aligned_size(unaligned_addr, >> len); >> + } else if (ctx->args[i].handle) { >> + struct drm_gem_object *gem_obj; >> + int err; >> + >> + err = get_gem_obj_from_handle(ctx, ctx->args[i].handle, >> &gem_obj); >> + if (err) >> + return err; >> + >> + ctx->gem_objs[i] = gem_obj; >> + qda_gem_obj = to_qda_gem_obj(gem_obj); >> + >> + pages[i].addr = qda_gem_obj->dma_addr + >> + calculate_vma_offset(ctx->args[i].ptr); >> + pages[i].size = calculate_page_aligned_size(ctx->args[i].ptr, >> len); >> + } else { >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * Resolve a DMA-handle argument. The kernel needs the physical page >> + * descriptor, while the DSP identifies the buffer by its GEM handle. >> + */ >> +static int process_dma_handle(struct qda_fastrpc_invoke_ctx *ctx, int i, >> + union fastrpc_remote_arg *rpra, >> + struct fastrpc_phy_page *pages) >> +{ >> + struct drm_gem_object *gem_obj; >> + int err; >> + >> + if (!ctx->args[i].handle) { >> + rpra[i].buf.pv = ctx->args[i].ptr; >> + rpra[i].buf.len = ctx->args[i].length; >> + return 0; >> + } >> + >> + err = get_gem_obj_from_handle(ctx, ctx->args[i].handle, &gem_obj); >> + if (err) >> + return err; >> + >> + ctx->gem_objs[i] = gem_obj; >> + setup_pages_from_gem_obj(to_qda_gem_obj(gem_obj), &pages[i]); >> + >> + /* The DSP-facing offset and length fields are 32-bit by wire format */ >> + rpra[i].dma.handle = ctx->args[i].handle; >> + rpra[i].dma.len = (u32)ctx->args[i].length; >> + rpra[i].dma.offset = (u32)ctx->args[i].ptr; >> + >> + return 0; >> +} >> + >> +/** >> + * qda_fastrpc_get_header_size() - Compute the FastRPC message header size >> + * @ctx: FastRPC invocation context >> + * >> + * Also caches the intermediate metadata and packet sizes in @ctx for the >> + * subsequent pack step. >> + * >> + * Return: Page-aligned size of the message payload buffer in bytes >> + */ >> +size_t qda_fastrpc_get_header_size(struct qda_fastrpc_invoke_ctx *ctx) >> +{ >> + ctx->inbufs = REMOTE_SCALARS_INBUFS(ctx->sc); >> + ctx->metalen = fastrpc_get_meta_size(ctx); >> + ctx->pkt_size = ALIGN(ctx->metalen, FASTRPC_ALIGN); >> + ctx->aligned_pkt_size = PAGE_ALIGN(ctx->pkt_size); >> + >> + return ctx->aligned_pkt_size; >> +} >> + >> +static int qda_fastrpc_get_args(struct qda_fastrpc_invoke_ctx *ctx) >> +{ >> + struct fastrpc_invoke_buf *list; >> + struct fastrpc_phy_page *pages; >> + union fastrpc_remote_arg *rpra; >> + int i, err; >> + >> + ctx->msg->buf = ctx->msg_gem_obj->virt; >> + ctx->msg->phys = ctx->msg_gem_obj->dma_addr; >> + memset(ctx->msg->buf, 0, ctx->aligned_pkt_size); >> + >> + rpra = ctx->msg->buf; >> + list = fastrpc_invoke_buf_start(rpra, ctx->nscalars); >> + pages = fastrpc_phy_page_start(list, ctx->nscalars); >> + ctx->rpra = rpra; >> + ctx->list = list; >> + ctx->pages = pages; >> + >> + for (i = 0; i < ctx->nbufs; ++i) { >> + rpra[i].buf.pv = 0; >> + rpra[i].buf.len = ctx->args[i].length; >> + list[i].num = ctx->args[i].length ? 1 : 0; >> + list[i].pgidx = i; >> + >> + if (!ctx->args[i].length) >> + continue; >> + >> + err = process_msg_buffer(ctx, i, rpra, pages); >> + if (err) >> + return err; >> + } >> + >> + for (i = ctx->nbufs; i < ctx->nscalars; ++i) { >> + list[i].num = ctx->args[i].length ? 1 : 0; >> + list[i].pgidx = i; >> + >> + err = process_dma_handle(ctx, i, rpra, pages); >> + if (err) >> + return err; >> + } >> + >> + return 0; >> +} >> + >> +/** >> + * qda_fastrpc_invoke_pack() - Pack an invocation context into a QDA message >> + * @ctx: FastRPC invocation context >> + * @msg: QDA message to fill in >> + * >> + * Return: 0 on success, negative error code on failure >> + */ >> +int qda_fastrpc_invoke_pack(struct qda_fastrpc_invoke_ctx *ctx, struct >> qda_msg *msg) >> +{ >> + int err; >> + >> + ctx->msg = msg; >> + >> + err = qda_fastrpc_get_args(ctx); >> + if (err) >> + return err; >> + >> + /* Ensure the payload is visible to the DSP before the doorbell */ >> + dma_wmb(); >> + >> + if (ctx->handle == FASTRPC_INIT_HANDLE) >> + msg->fastrpc.remote_session_id = 0; >> + else >> + msg->fastrpc.remote_session_id = ctx->remote_session_id; >> + >> + msg->fastrpc.tid = ctx->pid; >> + msg->fastrpc.ctx = ctx->ctxid | ctx->pd; >> + msg->fastrpc.handle = ctx->handle; >> + msg->fastrpc.sc = ctx->sc; >> + msg->fastrpc.addr = msg->phys; >> + msg->fastrpc.size = roundup(ctx->pkt_size, PAGE_SIZE); >> + msg->fastrpc_ctx = ctx; >> + msg->file_priv = ctx->file_priv; >> + >> + return 0; >> +} >> + >> +/* >> + * INVOKE_DYNAMIC: the argument descriptors are supplied by user space, >> which >> + * is also responsible for having imported every buffer to a GEM handle. >> + */ >> +static int qda_fastrpc_prepare_args_invoke(struct qda_fastrpc_invoke_ctx >> *ctx, void *argp) >> +{ >> + struct drm_qda_invoke_args *invoke_args = argp; >> + struct drm_qda_fastrpc_invoke_args *args; >> + u32 nscalars; >> + >> + ctx->handle = invoke_args->handle; >> + ctx->sc = invoke_args->sc; >> + >> + nscalars = REMOTE_SCALARS_LENGTH(ctx->sc); >> + if (!nscalars) >> + return 0; >> + >> + args = kzalloc_objs(*args, nscalars); >> + if (!args) >> + return -ENOMEM; >> + >> + if (copy_from_user(args, u64_to_user_ptr(invoke_args->args), >> + nscalars * sizeof(*args))) { >> + kfree(args); >> + return -EFAULT; >> + } >> + >> + ctx->args = args; >> + >> + return 0; >> +} >> + >> +/** >> + * qda_fastrpc_prepare_args() - Prepare arguments for a FastRPC invocation >> + * @ctx: FastRPC invocation context >> + * @argp: Kernel-side ioctl argument buffer owned by the DRM core >> + * >> + * Return: 0 on success, negative error code on failure >> + */ >> +int qda_fastrpc_prepare_args(struct qda_fastrpc_invoke_ctx *ctx, void *argp) >> +{ >> + int err; >> + >> + switch (ctx->type) { >> + case FASTRPC_RMID_INVOKE_DYNAMIC: >> + err = qda_fastrpc_prepare_args_invoke(ctx, argp); >> + break; >> + default: >> + return -EINVAL; >> + } >> + if (err) >> + return err; >> + >> + ctx->nscalars = REMOTE_SCALARS_LENGTH(ctx->sc); >> + ctx->nbufs = REMOTE_SCALARS_INBUFS(ctx->sc) + >> REMOTE_SCALARS_OUTBUFS(ctx->sc); >> + >> + if (ctx->nscalars && !ctx->gem_objs) { >> + ctx->gem_objs = kzalloc_objs(*ctx->gem_objs, ctx->nscalars); >> + if (!ctx->gem_objs) >> + return -ENOMEM; >> + } >> + >> + return 0; >> +} >> diff --git a/drivers/accel/qda/qda_fastrpc.h >> b/drivers/accel/qda/qda_fastrpc.h >> new file mode 100644 >> index 000000000000..a25818923a6e >> --- /dev/null >> +++ b/drivers/accel/qda/qda_fastrpc.h >> @@ -0,0 +1,242 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >> + */ >> + >> +#ifndef __QDA_FASTRPC_H__ >> +#define __QDA_FASTRPC_H__ >> + >> +#include <linux/completion.h> >> +#include <linux/kref.h> >> +#include <linux/list.h> >> +#include <linux/types.h> >> +#include <drm/drm_drv.h> >> +#include <drm/drm_file.h> >> +#include <drm/qda_accel.h> >> + >> +struct qda_dev; >> +struct qda_gem_obj; >> +struct qda_msg; >> + >> +/* >> + * FastRPC scalar extraction macros >> + * >> + * These macros extract different fields from the scalar value that >> describes >> + * the arguments passed in a FastRPC invocation. >> + */ >> +#define REMOTE_SCALARS_INBUFS(sc) (((sc) >> 16) & 0x0ff) >> +#define REMOTE_SCALARS_OUTBUFS(sc) (((sc) >> 8) & 0x0ff) >> +#define REMOTE_SCALARS_INHANDLES(sc) (((sc) >> 4) & 0x0f) >> +#define REMOTE_SCALARS_OUTHANDLES(sc) ((sc) & 0x0f) >> +#define REMOTE_SCALARS_LENGTH(sc) (REMOTE_SCALARS_INBUFS(sc) + \ >> + REMOTE_SCALARS_OUTBUFS(sc) + \ >> + REMOTE_SCALARS_INHANDLES(sc) + \ >> + REMOTE_SCALARS_OUTHANDLES(sc)) >> + >> +/* FastRPC configuration constants */ >> +#define FASTRPC_ALIGN 128 /* Alignment >> requirement */ >> +#define FASTRPC_MAX_HANDLELIST 16 /* Maximum handles the >> DSP can release */ >> +#define FASTRPC_MAX_CRCLIST 64 /* Maximum CRC list entries */ >> + >> +/* >> + * FastRPC scalar construction macros >> + * >> + * These macros build the scalar value that describes the arguments >> + * for a FastRPC invocation. >> + */ >> +#define FASTRPC_BUILD_SCALARS(attr, method, in, out, oin, oout) >> \ >> + (((attr & 0x07) << 29) | \ >> + ((method & 0x1f) << 24) | \ >> + ((in & 0xff) << 16) | \ >> + ((out & 0xff) << 8) | \ >> + ((oin & 0x0f) << 4) | \ >> + (oout & 0x0f)) >> + >> +#define FASTRPC_SCALARS(method, in, out) \ >> + FASTRPC_BUILD_SCALARS(0, method, in, out, 0, 0) >> + >> +/** >> + * struct fastrpc_remote_dmahandle - Remote DMA handle descriptor >> + */ >> +struct fastrpc_remote_dmahandle { >> + /** @handle: Handle of the buffer */ >> + s32 handle; >> + /** @offset: Byte offset within the buffer object */ >> + u32 offset; >> + /** @len: Length of the region in bytes */ >> + u32 len; >> +}; >> + >> +/** >> + * struct fastrpc_remote_buf - Remote buffer descriptor >> + */ >> +struct fastrpc_remote_buf { >> + /** @pv: Buffer pointer (user virtual address) */ >> + u64 pv; > > pv would mean physical something. Also, where do you validate the user's > address? Where do we check that it's ok to access the buffer, that it's > not a garbage? this is inherited from fastrpc(same is also used on DSP firmware) but I can rename it to something more meaningful like addr/va. This `pv` is used for offset calculation which eventually gives address of the page carrying the buffer. I can add checks when this calculation is done to ensure the pv is in proper range of the buffer passed by user.> >> + /** @len: Length of the buffer in bytes */ >> + u64 len; >> +}; >> + >> +/** >> + * union fastrpc_remote_arg - Remote argument (buffer or DMA handle) >> + */ >> +union fastrpc_remote_arg { >> + /** @buf: Inline buffer descriptor */ >> + struct fastrpc_remote_buf buf; >> + /** @dma: DMA-BUF handle descriptor */ >> + struct fastrpc_remote_dmahandle dma; >> +}; >> + >> +/** >> + * struct fastrpc_phy_page - Physical page descriptor >> + */ >> +struct fastrpc_phy_page { >> + /** @addr: Physical (IOMMU) address of the page */ >> + u64 addr; >> + /** @size: Size of the contiguous region in bytes */ >> + u64 size; >> +}; >> + >> +/** >> + * struct fastrpc_invoke_buf - Invoke buffer descriptor >> + */ >> +struct fastrpc_invoke_buf { >> + /** @num: Number of contiguous physical regions */ >> + u32 num; >> + /** @pgidx: Index into the physical page array */ >> + u32 pgidx; >> +}; >> + >> +/** >> + * struct fastrpc_msg - FastRPC wire message for remote invocations >> + * >> + * Sent to the remote processor via RPMsg. This is the exact layout >> + * the DSP expects; do not reorder or add fields without DSP firmware >> + * coordination. > > __packed? I'll check more on this.> >> + */ >> +struct fastrpc_msg { >> + /** @remote_session_id: Session identifier on the remote processor */ >> + int remote_session_id; > > Is it int or u32? This is again from fastrpc where it's a mix of int and s32. I'll keep it the to actual s32 here in QDA.> >> + /** @tid: Thread ID of the invoking thread */ >> + int tid; >> + /** @ctx: Context identifier for matching request/response */ >> + u64 ctx; >> + /** @handle: Handle of the remote method to invoke */ >> + u32 handle; >> + /** @sc: Scalars value encoding in/out buffer counts */ >> + u32 sc; >> + /** @addr: Physical address of the message payload buffer */ >> + u64 addr; >> + /** @size: Size of the message payload in bytes */ >> + u64 size; >> +}; >> + >> +/** >> + * struct qda_fastrpc_invoke_ctx - Remote procedure call invocation context >> + * >> + * Maintains all state for a single remote procedure call, including buffer >> + * management, synchronisation, and result handling. >> + */ >> +struct qda_fastrpc_invoke_ctx { >> + /** @node: List node for linking contexts in a queue */ >> + struct list_head node; >> + /** @qdev: Device owning the XArray this context is registered in */ >> + struct qda_dev *qdev; >> + /** @ctxid: Unique context identifier (XArray key shifted left by 4) */ > > Why is it shifted by 4? Why not by 3? I'll add the PD encoding comment here.> >> + u64 ctxid; >> + /** @inbufs: Number of input buffers */ >> + int inbufs; >> + /** @nscalars: Total number of scalar arguments */ >> + int nscalars; >> + /** @nbufs: Total number of buffer arguments (inbufs + outbufs) */ >> + int nbufs; >> + /** @pid: Process ID of the calling process */ >> + int pid; >> + /** @retval: Status code reported by the DSP for this invocation */ >> + int retval; >> + /** @remote_session_id: Session identifier on the remote processor */ >> + int remote_session_id; > > is pid linked to remote_session_id? not really, remote_session_id is more of session specific, so in case of multi-session(same PID having multiple sessions) should have different remote_session_id.> >> + /** @pd: Protection domain identifier encoded into the context ID */ > > If it's already encoded, why do you need it here?c it's stores so the packing step can use it, I'll document more on this. Or if I can have both msg->fastrpc.ctx and ctxid same, I'll check that also> >> + int pd; >> + /** @type: Invocation type (e.g. FASTRPC_RMID_INVOKE_DYNAMIC) */ >> + u32 type; > > What other types can exist? I'll add all the types in document or just add FASTRPC_RMID_* and point to qda_fastrpc.h for more details.> >> + /** @sc: Scalars value encoding in/out buffer counts */ >> + u32 sc; > > Remove it and calculate from in/outbufs above? The source of sc is the auto-generated stub file which passes this to remote_handle_invoke() based on the method requested by the user. This as is as it is passed to DSP where this is used in auto-generated skel. I think the other way around(in/outbufs from sc) would make more sense.> >> + /** @handle: Handle of the remote method being invoked */ >> + u32 handle; >> + /** @metalen: Length of the FastRPC metadata header in bytes */ >> + size_t metalen; >> + /** @pkt_size: Total payload size in bytes */ >> + u64 pkt_size; >> + /** @aligned_pkt_size: Page-aligned payload size for GEM allocation */ > > ??? I'll check and remove this.> >> + u64 aligned_pkt_size; >> + /** @list: Array of invoke buffer descriptors */ >> + struct fastrpc_invoke_buf *list; >> + /** @pages: Array of physical page descriptors for all arguments */ >> + struct fastrpc_phy_page *pages; >> + /** @input_pages: Array of physical page descriptors for input buffers >> */ >> + struct fastrpc_phy_page *input_pages; >> + /** @work: Completion used to synchronise with the DSP response */ >> + struct completion work; >> + /** @msg: Pointer to the QDA message structure for this invocation */ >> + struct qda_msg *msg; >> + /** @rpra: Array of remote procedure arguments */ >> + union fastrpc_remote_arg *rpra; >> + /** @gem_objs: Array of GEM objects imported for argument buffers */ >> + struct drm_gem_object **gem_objs; >> + /** @args: Invoke argument descriptors */ >> + struct drm_qda_fastrpc_invoke_args *args; >> + /** @refcount: Reference counter for context lifetime management */ > > What for? just to ensure that the context is not freed in case the wait_for_completion() is interrupted.> >> + struct kref refcount; >> + /** @msg_gem_obj: GEM object backing the message payload buffer */ >> + struct qda_gem_obj *msg_gem_obj; >> + /** @file_priv: DRM file private data */ >> + struct drm_file *file_priv; >> + /** >> + * @req: Request buffer for the internal init/map/unmap calls. Points >> + * into a kernel-owned GEM mapping tracked by @gem_objs, so it must >> + * never be freed directly. >> + */ >> + void *req; > > Why do you need separate pointers here? Can't you be getting them from > msm_gem_obj? These are typed pointers into different offsets of the kernel GEM mapping. I could compute them each time from `msg_gem_obj->virt + offset`, but caching them avoids repetitive casting. Let me see if removing this looks cleaner.> >> + /** @rsp: Response buffer, same lifetime rules as @req */ >> + void *rsp; >> + /** @inbuf: Process-create input buffer, same lifetime rules as @req */ >> + void *inbuf; >> +}; >> + >> +/** >> + * struct qda_msg - FastRPC message with kernel-internal bookkeeping >> + */ >> +struct qda_msg { >> + /** >> + * @fastrpc: Wire-format message sent to the DSP via RPMsg. >> + * Must be the first member. >> + */ >> + struct fastrpc_msg fastrpc; >> + /** @buf: Kernel virtual address of the payload buffer */ >> + void *buf; >> + /** @phys: Physical/DMA address of the payload buffer */ >> + u64 phys; >> + /** @ret: Return value from the remote processor */ >> + int ret; >> + /** @fastrpc_ctx: Back-pointer to the owning invocation context */ >> + struct qda_fastrpc_invoke_ctx *fastrpc_ctx; >> + /** @file_priv: DRM file private data for GEM object lookup */ >> + struct drm_file *file_priv; >> +}; >> + >> +/* Remote Method ID table - identifies initialization and control >> operations */ >> +#define FASTRPC_RMID_INVOKE_DYNAMIC 0xFFFFFFFFU /* Dynamic method >> invocation */ >> + >> +/* Common handle for initialization operations */ >> +#define FASTRPC_INIT_HANDLE 0x1 >> + >> +void qda_fastrpc_context_free(struct kref *ref); >> +void qda_fastrpc_cleanup_handlelist(struct qda_fastrpc_invoke_ctx *ctx); >> +void qda_fastrpc_flush_pending(struct qda_dev *qdev); >> +struct qda_fastrpc_invoke_ctx *qda_fastrpc_context_alloc(struct qda_dev >> *qdev); >> +int qda_fastrpc_prepare_args(struct qda_fastrpc_invoke_ctx *ctx, void >> *argp); >> +size_t qda_fastrpc_get_header_size(struct qda_fastrpc_invoke_ctx *ctx); >> +int qda_fastrpc_invoke_pack(struct qda_fastrpc_invoke_ctx *ctx, struct >> qda_msg *msg); >> + >> +#endif /* __QDA_FASTRPC_H__ */ >> diff --git a/drivers/accel/qda/qda_ioctl.c b/drivers/accel/qda/qda_ioctl.c >> index f65325c80a12..b7ee4899ba74 100644 >> --- a/drivers/accel/qda/qda_ioctl.c >> +++ b/drivers/accel/qda/qda_ioctl.c >> @@ -1,10 +1,13 @@ >> // SPDX-License-Identifier: GPL-2.0-only >> // Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >> #include <drm/drm_ioctl.h> >> +#include <drm/drm_print.h> >> #include <drm/qda_accel.h> >> #include "qda_drv.h" >> +#include "qda_fastrpc.h" >> #include "qda_gem.h" >> #include "qda_ioctl.h" >> +#include "qda_rpmsg.h" >> >> /** >> * qda_ioctl_query() - Query DSP device information >> @@ -83,3 +86,83 @@ int qda_ioctl_gem_mmap_offset(struct drm_device *dev, >> void *data, struct drm_fil >> >> return drm_gem_dumb_map_offset(file_priv, dev, args->handle, >> &args->offset); >> } >> + >> +static int qda_fastrpc_invoke(u32 type, struct drm_device *dev, void *data, >> + struct drm_file *file_priv) >> +{ >> + struct qda_file_priv *qda_file_priv = file_priv->driver_priv; >> + struct qda_dev *qdev = qda_file_priv->qda_dev; >> + struct qda_fastrpc_invoke_ctx *ctx; >> + struct drm_gem_object *gem_obj; >> + struct qda_msg msg; >> + size_t hdr_size; >> + int err; >> + >> + ctx = qda_fastrpc_context_alloc(qdev); >> + if (IS_ERR(ctx)) >> + return PTR_ERR(ctx); >> + >> + ctx->type = type; >> + ctx->file_priv = file_priv; >> + ctx->remote_session_id = qda_file_priv->remote_session_id; >> + >> + err = qda_fastrpc_prepare_args(ctx, data); >> + if (err) >> + goto err_context_free; >> + >> + hdr_size = qda_fastrpc_get_header_size(ctx); >> + >> + gem_obj = qda_gem_create_object(dev, qdev->iommu_mgr, hdr_size, >> file_priv); >> + if (IS_ERR(gem_obj)) { >> + err = PTR_ERR(gem_obj); >> + goto err_context_free; >> + } >> + >> + ctx->msg_gem_obj = to_qda_gem_obj(gem_obj); >> + >> + err = qda_fastrpc_invoke_pack(ctx, &msg); >> + if (err) >> + goto err_context_free; >> + >> + err = qda_rpmsg_send_msg(qdev, &msg); >> + if (err) >> + goto err_context_free; >> + >> + err = qda_rpmsg_wait_for_rsp(ctx); >> + if (err) >> + goto err_context_free; >> + >> + /* Ensure the results written by the DSP are visible before reading */ >> + dma_rmb(); >> + >> + err = ctx->retval; >> + if (err) { >> + drm_dbg_driver(dev, "DSP returned status 0x%x for type %u\n", >> + ctx->retval, type); >> + goto err_context_free; >> + } >> + >> + qda_fastrpc_cleanup_handlelist(ctx); >> + >> + kref_put(&ctx->refcount, qda_fastrpc_context_free); >> + >> + return 0; >> + >> +err_context_free: >> + kref_put(&ctx->refcount, qda_fastrpc_context_free); >> + >> + return err; >> +} >> + >> +/** >> + * qda_ioctl_invoke() - Perform a dynamic FastRPC method invocation >> + * @dev: DRM device structure >> + * @data: User-space data (struct qda_invoke_args) >> + * @file_priv: DRM file private data >> + * >> + * Return: 0 on success, negative error code on failure >> + */ >> +int qda_ioctl_invoke(struct drm_device *dev, void *data, struct drm_file >> *file_priv) >> +{ >> + return qda_fastrpc_invoke(FASTRPC_RMID_INVOKE_DYNAMIC, dev, data, >> file_priv); >> +} >> diff --git a/drivers/accel/qda/qda_ioctl.h b/drivers/accel/qda/qda_ioctl.h >> index d1cbbfb6d965..3bb9cfd98370 100644 >> --- a/drivers/accel/qda/qda_ioctl.h >> +++ b/drivers/accel/qda/qda_ioctl.h >> @@ -11,5 +11,6 @@ >> int qda_ioctl_query(struct drm_device *dev, void *data, struct drm_file >> *file_priv); >> int qda_ioctl_gem_create(struct drm_device *dev, void *data, struct >> drm_file *file_priv); >> int qda_ioctl_gem_mmap_offset(struct drm_device *dev, void *data, struct >> drm_file *file_priv); >> +int qda_ioctl_invoke(struct drm_device *dev, void *data, struct drm_file >> *file_priv); >> >> #endif /* __QDA_IOCTL_H__ */ >> diff --git a/drivers/accel/qda/qda_rpmsg.c b/drivers/accel/qda/qda_rpmsg.c >> index 64bf503106d9..7a5e0d3e20ca 100644 >> --- a/drivers/accel/qda/qda_rpmsg.c >> +++ b/drivers/accel/qda/qda_rpmsg.c >> @@ -1,12 +1,17 @@ >> // SPDX-License-Identifier: GPL-2.0-only >> // Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >> +#include <linux/completion.h> >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/rpmsg.h> >> +#include <linux/sched.h> >> +#include <linux/wait.h> >> #include <drm/drm_print.h> >> >> #include "qda_cb.h" >> #include "qda_drv.h" >> +#include "qda_fastrpc.h" >> +#include "qda_rpmsg.h" >> >> static struct qda_dev *qda_rpmsg_alloc_and_init_qdev(struct rpmsg_device >> *rpdev) >> { >> @@ -23,11 +28,92 @@ static struct qda_dev >> *qda_rpmsg_alloc_and_init_qdev(struct rpmsg_device *rpdev) >> return qdev; >> } >> >> +/** >> + * qda_rpmsg_send_msg() - Send a packed invocation to the remote processor >> + * @qdev: QDA device structure >> + * @msg: Packed message to send >> + * >> + * Takes a reference on the invocation context on behalf of the response >> + * callback, which keeps the context alive until the DSP has replied. >> + * >> + * Return: 0 on success, negative error code on failure >> + */ >> +int qda_rpmsg_send_msg(struct qda_dev *qdev, struct qda_msg *msg) >> +{ >> + struct qda_fastrpc_invoke_ctx *ctx = msg->fastrpc_ctx; >> + int ret, idx; >> + >> + if (!drm_dev_enter(&qdev->drm_dev, &idx)) >> + return -ENODEV; >> + >> + if (!qdev->rpdev) { >> + ret = -ENODEV; >> + goto out_exit; >> + } >> + >> + kref_get(&ctx->refcount); >> + >> + ret = rpmsg_send(qdev->rpdev->ept, &msg->fastrpc, sizeof(msg->fastrpc)); >> + if (ret) { >> + drm_err_ratelimited(&qdev->drm_dev, "Failed to send rpmsg: >> %d\n", ret); >> + kref_put(&ctx->refcount, qda_fastrpc_context_free); >> + } >> + >> +out_exit: >> + drm_dev_exit(idx); >> + >> + return ret; >> +} >> + >> +/** >> + * qda_rpmsg_wait_for_rsp() - Wait for the DSP to answer an invocation >> + * @ctx: FastRPC invocation context >> + * >> + * Return: 0 once the response arrived, or -ERESTARTSYS if a signal >> + * interrupted the wait >> + */ >> +int qda_rpmsg_wait_for_rsp(struct qda_fastrpc_invoke_ctx *ctx) >> +{ >> + return wait_for_completion_interruptible(&ctx->work); >> +} >> + >> static int qda_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len, >> void *priv, u32 src) >> { >> - /* Placeholder: responses will be dispatched here */ >> - return 0; >> + struct qda_dev *qdev = dev_get_drvdata(&rpdev->dev); >> + struct qda_invoke_rsp *resp_msg = data; >> + struct qda_fastrpc_invoke_ctx *ctx; >> + int ret = 0, idx; >> + >> + if (!qdev) >> + return -ENODEV; >> + >> + if (!drm_dev_enter(&qdev->drm_dev, &idx)) >> + return -ENODEV; >> + >> + if (len < sizeof(*resp_msg)) { >> + ret = -EINVAL; >> + goto out_exit; >> + } >> + >> + ctx = xa_load(&qdev->ctx_xa, resp_msg->ctx >> 4); >> + if (!ctx) { >> + drm_dbg_driver(&qdev->drm_dev, "No context matches response >> 0x%llx\n", >> + resp_msg->ctx); >> + ret = -ENOENT; >> + goto out_exit; >> + } >> + >> + ctx->retval = resp_msg->retval; >> + complete(&ctx->work); >> + >> + /* Release the reference taken by qda_rpmsg_send_msg() */ >> + kref_put(&ctx->refcount, qda_fastrpc_context_free); >> + >> +out_exit: >> + drm_dev_exit(idx); >> + >> + return ret; >> } >> >> static void qda_rpmsg_remove(struct rpmsg_device *rpdev) >> @@ -41,6 +127,7 @@ static void qda_rpmsg_remove(struct rpmsg_device *rpdev) >> */ >> drm_dev_unplug(&qdev->drm_dev); >> qdev->rpdev = NULL; >> + qda_fastrpc_flush_pending(qdev); >> qda_cb_unpopulate(qdev); >> qda_deinit_device(qdev); >> } >> diff --git a/drivers/accel/qda/qda_rpmsg.h b/drivers/accel/qda/qda_rpmsg.h >> new file mode 100644 >> index 000000000000..a70f4a80808d >> --- /dev/null >> +++ b/drivers/accel/qda/qda_rpmsg.h >> @@ -0,0 +1,26 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. >> + */ >> + >> +#ifndef __QDA_RPMSG_H__ >> +#define __QDA_RPMSG_H__ >> + >> +#include "qda_drv.h" >> +#include "qda_fastrpc.h" >> + >> +/** >> + * struct qda_invoke_rsp - Response structure for FastRPC invocations >> + */ >> +struct qda_invoke_rsp { >> + /** @ctx: Invoke caller context for matching request/response */ >> + u64 ctx; >> + /** @retval: Return value from the remote invocation */ >> + int retval; >> +}; >> + >> +/* RPMsg transport layer functions */ >> +int qda_rpmsg_send_msg(struct qda_dev *qdev, struct qda_msg *msg); >> +int qda_rpmsg_wait_for_rsp(struct qda_fastrpc_invoke_ctx *ctx); >> + >> +#endif /* __QDA_RPMSG_H__ */ >> diff --git a/include/uapi/drm/qda_accel.h b/include/uapi/drm/qda_accel.h >> index 1d699f00e7be..5cf0fec1d91c 100644 >> --- a/include/uapi/drm/qda_accel.h >> +++ b/include/uapi/drm/qda_accel.h >> @@ -21,6 +21,8 @@ extern "C" { >> #define DRM_QDA_QUERY 0x00 >> #define DRM_QDA_GEM_CREATE 0x01 >> #define DRM_QDA_GEM_MMAP_OFFSET 0x02 >> +/* Command numbers 0x03-0x06 reserved for INIT_ATTACH, INIT_CREATE, MAP, >> MUNMAP */ > > WHY? I'll move away from this reserved idea in the next version and update it in order of IOCTLs that are getting enabled.> >> +#define DRM_QDA_REMOTE_INVOKE 0x07 >> >> /* >> * QDA IOCTL definitions >> @@ -35,6 +37,8 @@ extern "C" { >> struct drm_qda_gem_create) >> #define DRM_IOCTL_QDA_GEM_MMAP_OFFSET DRM_IOWR(DRM_COMMAND_BASE + >> DRM_QDA_GEM_MMAP_OFFSET, \ >> struct drm_qda_gem_mmap_offset) >> +#define DRM_IOCTL_QDA_REMOTE_INVOKE DRM_IOWR(DRM_COMMAND_BASE + >> DRM_QDA_REMOTE_INVOKE, \ >> + struct drm_qda_invoke_args) >> >> /* Query type definitions for drm_qda_query */ >> #define QDA_QUERY_DSP_NAME 1 >> @@ -84,6 +88,44 @@ struct drm_qda_gem_mmap_offset { >> __u32 pad; >> }; >> >> +/** >> + * struct drm_qda_fastrpc_invoke_args - FastRPC invocation argument >> descriptor >> + * @ptr: Pointer to argument data (user virtual address) > > Why do you need it? Is handle + length not enough? If you want, specify > offset inside the GEM BO. The user VA (`ptr`) is needed because userspace mmaps the GEM BO and then passes pointers into it to the DSP. The kernel derives the offset within the BO from the VMA (via `calculate_vma_offset()`). I can check if just passing handle + offset + length works here but that might require some userspace change. > >> + * @length: Length of the argument data in bytes >> + * @handle: GEM handle for buffer arguments; 0 for scalar arguments >> + * @attr: Argument attributes and flags >> + * >> + * This structure describes a single argument passed to a FastRPC >> invocation. >> + * Arguments can be either scalar values or buffer references (via GEM >> handle). >> + * Userspace must import any DMA-BUF fd to a GEM handle before populating >> + * this field; the driver never accepts DMA-BUF fds directly. >> + */ >> +struct drm_qda_fastrpc_invoke_args { >> + __u64 ptr; >> + __u64 length; >> + __u32 handle; >> + __u32 attr; >> +}; >> + >> +/** >> + * struct drm_qda_invoke_args - Dynamic FastRPC invocation parameters >> + * @handle: Remote handle to invoke on the DSP >> + * @sc: FastRPC scalars value encoding the number of in/out buffers >> + * @args: User-space pointer to array of drm_qda_fastrpc_invoke_args >> descriptors; >> + * the handle field in each entry must be a GEM handle (or 0 for >> + * inline scalar buffers). Userspace must import DMA-BUF fds to GEM > > What is inline scalar buffer? I think, we agreed that everything is a > GEM BO. I missed updating this comment, The inline buffer logic is now removed. I'll fix this.> >> + * handles before passing them here. > > I'd say this is useless. It adds no information. If the driver only > accepts GEM handles, why do you need to speak about DMA BUFs here? ack, will update the details properly.> >> + * >> + * This structure is used with DRM_IOCTL_QDA_REMOTE_INVOKE to perform a >> + * dynamic remote procedure call on the DSP. The args pointer must reference >> + * an array of REMOTE_SCALARS_LENGTH(sc) drm_qda_fastrpc_invoke_args >> entries. > > And how does the user know, what is REMOTE_SCALARS_LENGTH? Define it > here, please (and start with QDA). ack.> >> + */ >> +struct drm_qda_invoke_args { >> + __u32 handle; >> + __u32 sc; > > Can we place sc with __u8 inargs, __u8 outargs? Then there is no need to > play with the bytes and talk about REMOTE_SCALARS_LENGTH. Explained the source of `sc` above, I can correct the comments here.> >> + __u64 args; > > __u64 inargs, __u64 outargs? > >> +}; >> + >> #if defined(__cplusplus) >> } >> #endif >> >> -- >> 2.34.1 >> >
