On Thu, Aug 20, 2026 at 02:05:01PM +0530, Ekansh Gupta wrote:
> 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?>
Just address it in the driver, disallowing such collisions. If there is
a way for the counter to wrap, it will wrap once.
> >> file->driver_priv = qda_file_priv;
> >>
> >> return 0;
> >
> > [...]
> >> +
> >> +/**
> >> + * 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.>
Don't inherit bad names. Don't use 'user virtual address' unless you
really mean __user. In the latter case the address must go through
access_ok() checks.
> >> + /** @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.>
Thanks. Also please stop using fastrpc_ prefix in the QDA driver.
> >> + /** @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.>
So, why?
> >> + 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.>
Then drop the pid, please. You already have a session here.
> >> + /** @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.>
Do we really need to support those in the first iteration? I think you
should have heart it several times: get the minimal sensible driver in.
You can add all the features afterwards.
> >> + /** @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.>
ok
> >> + /** @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.>
If it's not _interruptible, it can't be interrupted. Or there should be
a better description of the reasons.
> >> + 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.>
Calculate them when necessary. If you use them in a few places, they
don't need to be cached.
> >> + /** @rsp: Response buffer, same lifetime rules as @req */
> >> + void *rsp;
> >> + /** @inbuf: Process-create input buffer, same lifetime rules as @req */
> >> + void *inbuf;
> >> +};
> >> +
> >> @@ -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.
You are going to have a different userspace. It must pass only
handle+offset+length triplets. All the rest must go to the compatibility
layers.
--
With best wishes
Dmitry
