Re: [PATCH v2 12/15] accel/qda: Add FastRPC invocation support

[Dmitry Baryshkov]

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 <ekansh.gupta@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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