Re: [PATCH v2 07/15] accel/qda: Add memory manager for CB devices

[Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>]

On 17-08-2026 12:22, Dmitry Baryshkov wrote:
> On Mon, Aug 17, 2026 at 10:17:42AM +0530, Ekansh Gupta wrote:
>> Introduce the QDA memory manager (qda_memory_manager) to track the
>> IOMMU devices that back each compute context bank (CB).
>>
>> Each CB device registered on the qda-compute-cb bus is wrapped in a
>> qda_iommu_device descriptor recording the device pointer and its stream
>> ID, and stored in the memory manager's registry. Later patches use this
>> registry to resolve the IOMMU device a session should allocate from.
>>
>> The registry is a plain array sized to the number of
>> "qcom,fastrpc-compute-cb" nodes present in the device tree: the RPMsg
>> probe counts those nodes and passes the count to qda_init_device(),
>> which allocates the array in qda_memory_manager_init(). The memory
>> manager is created before CB devices are populated and destroyed after
>> they are torn down, so no dangling descriptors remain.
>>
>> qda_cb_setup_device() is called immediately after a CB device is
>> registered on the bus: it allocates the descriptor, registers it with
>> the memory manager, and stores it as the CB device's driver data so
>> that qda_destroy_cb_device() can unregister and free it during teardown.
>>
>> Assisted-by: Claude:claude-sonnet-5
>> Signed-off-by: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>
>> ---
>> Changes in v2:
>> - Replace the XArray with a plain array sized to the device tree's CB
>>   node count instead of a fixed 16-entry table (Dmitry Baryshkov)
>> - Fold the init_memory_manager()/cleanup_memory_manager() wrappers into
>>   qda_init_device()/qda_deinit_device() (Dmitry Baryshkov)
>> - Drop the pr_debug() calls (Dmitry Baryshkov)
>> - Use goto labels to unwind probe failures instead of open-coding the
>>   cleanup at each error site (Dmitry Baryshkov)
>> ---
>>  drivers/accel/qda/Makefile             |  1 +
>>  drivers/accel/qda/qda_cb.c             | 39 ++++++++++++++
>>  drivers/accel/qda/qda_drv.c            | 26 +++++++++
>>  drivers/accel/qda/qda_drv.h            |  5 ++
>>  drivers/accel/qda/qda_memory_manager.c | 98 ++++++++++++++++++++++++++++++++++
>>  drivers/accel/qda/qda_memory_manager.h | 55 +++++++++++++++++++
>>  drivers/accel/qda/qda_rpmsg.c          | 23 +++++++-
>>  7 files changed, 246 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/accel/qda/Makefile b/drivers/accel/qda/Makefile
>> index 143c9e4e789e..701fad5ffb50 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_memory_manager.o \
>>  	qda_rpmsg.o
>>  
>>  obj-$(CONFIG_DRM_ACCEL_QDA_COMPUTE_BUS) += qda_compute_bus.o
>> diff --git a/drivers/accel/qda/qda_cb.c b/drivers/accel/qda/qda_cb.c
>> index 9c5ef32f351e..cd32d6df6318 100644
>> --- a/drivers/accel/qda/qda_cb.c
>> +++ b/drivers/accel/qda/qda_cb.c
>> @@ -8,13 +8,40 @@
>>  #include <linux/slab.h>
>>  #include <drm/drm_print.h>
>>  #include "qda_drv.h"
>> +#include "qda_memory_manager.h"
>>  #include "qda_cb.h"
>>  
>> +static int qda_cb_setup_device(struct qda_dev *qdev, struct device *cb_dev, u32 sid)
>> +{
>> +	struct qda_iommu_device *iommu_dev;
>> +	int ret;
>> +
>> +	iommu_dev = kzalloc_obj(*iommu_dev);
>> +	if (!iommu_dev)
>> +		return -ENOMEM;
>> +
>> +	iommu_dev->dev = cb_dev;
>> +	iommu_dev->qdev = qdev;
>> +	iommu_dev->sid = sid;
>> +
>> +	ret = qda_memory_manager_register_device(qdev->iommu_mgr, iommu_dev);
>> +	if (ret) {
>> +		drm_err(&qdev->drm_dev, "Failed to register IOMMU device: %d\n", ret);
>> +		kfree(iommu_dev);
>> +		return ret;
>> +	}
>> +
>> +	dev_set_drvdata(cb_dev, iommu_dev);
>> +
>> +	return 0;
>> +}
>> +
>>  int qda_create_cb_device(struct qda_dev *qdev, struct device_node *cb_node)
>>  {
>>  	struct qda_cb_dev *entry;
>>  	struct device *cb_dev;
>>  	u32 sid = 0;
>> +	int ret;
>>  
>>  	if (of_property_read_u32(cb_node, "reg", &sid))
>>  		drm_info(&qdev->drm_dev, "No stream ID in DT node %pOFn, using 0\n", cb_node);
>> @@ -27,6 +54,12 @@ int qda_create_cb_device(struct qda_dev *qdev, struct device_node *cb_node)
>>  		return PTR_ERR(cb_dev);
>>  	}
>>  
>> +	ret = qda_cb_setup_device(qdev, cb_dev, sid);
>> +	if (ret) {
>> +		device_unregister(cb_dev);
>> +		return ret;
>> +	}
>> +
>>  	entry = kzalloc_obj(*entry);
>>  	if (!entry) {
>>  		qda_destroy_cb_device(cb_dev);
>> @@ -74,8 +107,14 @@ int qda_cb_populate(struct qda_dev *qdev, struct device_node *parent_node)
>>  
>>  void qda_destroy_cb_device(struct device *cb_dev)
>>  {
>> +	struct qda_iommu_device *iommu_dev;
>>  	struct iommu_group *group;
>>  
>> +	iommu_dev = dev_get_drvdata(cb_dev);
>> +	if (iommu_dev && iommu_dev->qdev->iommu_mgr)
> 
> Can either of them be NULL?
it's only called from qda_cb_unpopulate() so it can't be NULL. I'll
remove all the unnecessary non-NULL check like it was done for GEM.>
>> +		qda_memory_manager_unregister_device(iommu_dev->qdev->iommu_mgr,
>> +						     iommu_dev);
>> +
>>  	group = iommu_group_get(cb_dev);
>>  	if (group) {
>>  		iommu_group_remove_device(cb_dev);
>> diff --git a/drivers/accel/qda/qda_drv.c b/drivers/accel/qda/qda_drv.c
>> index 09b6d5ac390b..fdc909facd95 100644
>> --- a/drivers/accel/qda/qda_drv.c
>> +++ b/drivers/accel/qda/qda_drv.c
>> @@ -56,6 +56,32 @@ struct qda_dev *qda_alloc_device(struct device *dev)
>>  	return qdev;
>>  }
>>  
>> +void qda_deinit_device(struct qda_dev *qdev)
>> +{
>> +	if (qdev->iommu_mgr) {
> 
> Why?
> 
>> +		qda_memory_manager_exit(qdev->iommu_mgr);
>> +		kfree(qdev->iommu_mgr);
>> +		qdev->iommu_mgr = NULL;
> 
> What for? If it's a teardown path, qdev will be destroyed soon.
ack, will remove it.>
>> +	}
>> +}
>> +
>> +int qda_init_device(struct qda_dev *qdev, int num_cbs)
>> +{
>> +	int ret;
>> +
>> +	qdev->iommu_mgr = kzalloc_obj(*qdev->iommu_mgr);
> 
> devm_kzalloc_obj()?
I'll check on how this impacts the exit path. Will implement with this
if it make the logic cleaner, thanks for the suggestion.>
>> +	if (!qdev->iommu_mgr)
>> +		return -ENOMEM;
>> +	ret = qda_memory_manager_init(qdev->iommu_mgr, num_cbs);
>> +	if (ret) {
>> +		drm_err(&qdev->drm_dev, "Failed to initialize memory manager: %d\n", ret);
>> +		kfree(qdev->iommu_mgr);
>> +		qdev->iommu_mgr = NULL;
> 
> And what is going to read this NULL?
won't be used, will remove this.>
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  int qda_register_device(struct qda_dev *qdev)
>>  {
>>  	int ret;
>> diff --git a/drivers/accel/qda/qda_drv.h b/drivers/accel/qda/qda_drv.h
>> index 3bd41b9593fa..690a833d732b 100644
>> --- a/drivers/accel/qda/qda_drv.h
>> +++ b/drivers/accel/qda/qda_drv.h
>> @@ -13,6 +13,7 @@
>>  #include <drm/drm_device.h>
>>  #include <drm/drm_drv.h>
>>  #include <drm/drm_file.h>
>> +#include "qda_memory_manager.h"
>>  
>>  /* Driver identification */
>>  #define QDA_DRIVER_NAME "qda"
>> @@ -40,6 +41,8 @@ struct qda_dev {
>>  	struct device *dev;
>>  	/** @cb_devs: Compute context-bank (CB) child devices */
>>  	struct list_head cb_devs;
>> +	/** @iommu_mgr: IOMMU/memory manager instance */
>> +	struct qda_memory_manager *iommu_mgr;
>>  	/** @dsp_name: Name of the DSP domain (e.g. "cdsp", "adsp") */
>>  	const char *dsp_name;
>>  };
>> @@ -59,6 +62,8 @@ static inline struct qda_dev *qda_dev_from_drm(struct drm_device *dev)
>>  struct qda_dev *qda_alloc_device(struct device *dev);
>>  
>>  /* Core device lifecycle */
>> +int qda_init_device(struct qda_dev *qdev, int num_cbs);
>> +void qda_deinit_device(struct qda_dev *qdev);
>>  int qda_register_device(struct qda_dev *qdev);
>>  
>>  #endif /* __QDA_DRV_H__ */
>> diff --git a/drivers/accel/qda/qda_memory_manager.c b/drivers/accel/qda/qda_memory_manager.c
>> new file mode 100644
>> index 000000000000..b1a80ee77c35
>> --- /dev/null
>> +++ b/drivers/accel/qda/qda_memory_manager.c
>> @@ -0,0 +1,98 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> +
>> +#include <linux/slab.h>
>> +#include "qda_drv.h"
>> +#include "qda_memory_manager.h"
>> +
>> +static void cleanup_all_memory_devices(struct qda_memory_manager *mem_mgr)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < mem_mgr->num_devices; i++) {
>> +		struct qda_iommu_device *iommu_dev = mem_mgr->devices[i];
>> +
>> +		if (!iommu_dev)
>> +			continue;
>> +
>> +		mem_mgr->devices[i] = NULL;
>> +		kfree(iommu_dev);
>> +	}
>> +	mem_mgr->num_devices = 0;
> 
> What for?
not needed. I'll remove this.>
>> +}
>> +
>> +/**
>> + * qda_memory_manager_register_device() - Register an IOMMU device
>> + * @mem_mgr: Pointer to memory manager
>> + * @iommu_dev: Pointer to IOMMU device to register
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +int qda_memory_manager_register_device(struct qda_memory_manager *mem_mgr,
>> +				       struct qda_iommu_device *iommu_dev)
>> +{
>> +	if (mem_mgr->num_devices >= mem_mgr->max_devices)
>> +		return -ENOSPC;
>> +
>> +	iommu_dev->id = mem_mgr->num_devices;
>> +	mem_mgr->devices[mem_mgr->num_devices++] = iommu_dev;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * qda_memory_manager_unregister_device() - Unregister an IOMMU device
>> + * @mem_mgr: Pointer to memory manager
>> + * @iommu_dev: Pointer to IOMMU device to unregister
>> + *
>> + * Removes the device from the registry and frees it.
>> + */
>> +void qda_memory_manager_unregister_device(struct qda_memory_manager *mem_mgr,
>> +					  struct qda_iommu_device *iommu_dev)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < mem_mgr->num_devices; i++) {
>> +		if (mem_mgr->devices[i] == iommu_dev) {
>> +			mem_mgr->devices[i] = NULL;
>> +			break;
>> +		}
>> +	}
>> +
>> +	kfree(iommu_dev);
>> +}
>> +
>> +/**
>> + * qda_memory_manager_init() - Initialize the memory manager
>> + * @mem_mgr: Pointer to memory manager structure to initialize
>> + * @max_devices: Number of IOMMU context bank devices to make room for
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +int qda_memory_manager_init(struct qda_memory_manager *mem_mgr, int max_devices)
>> +{
>> +	if (max_devices <= 0)
>> +		return -EINVAL;
>> +
>> +	mem_mgr->devices = kcalloc(max_devices, sizeof(*mem_mgr->devices), GFP_KERNEL);
>> +	if (!mem_mgr->devices)
>> +		return -ENOMEM;
>> +
>> +	mem_mgr->num_devices = 0;
>> +	mem_mgr->max_devices = max_devices;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * qda_memory_manager_exit() - Clean up the memory manager
>> + * @mem_mgr: Pointer to memory manager structure to clean up
>> + */
>> +void qda_memory_manager_exit(struct qda_memory_manager *mem_mgr)
>> +{
>> +	cleanup_all_memory_devices(mem_mgr);
>> +
>> +	kfree(mem_mgr->devices);
>> +	mem_mgr->devices = NULL;
>> +	mem_mgr->max_devices = 0;
>> +}
>> diff --git a/drivers/accel/qda/qda_memory_manager.h b/drivers/accel/qda/qda_memory_manager.h
>> new file mode 100644
>> index 000000000000..7e38c8a18284
>> --- /dev/null
>> +++ b/drivers/accel/qda/qda_memory_manager.h
>> @@ -0,0 +1,55 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#ifndef __QDA_MEMORY_MANAGER_H__
>> +#define __QDA_MEMORY_MANAGER_H__
>> +
>> +#include <linux/device.h>
>> +
>> +/* Forward declarations */
>> +struct qda_dev;
>> +
>> +/**
>> + * struct qda_iommu_device - IOMMU device instance for memory management
>> + *
>> + * Represents a single IOMMU-enabled device managed by the memory manager.
>> + * Each device can be assigned to a specific process session.
>> + */
>> +struct qda_iommu_device {
>> +	/** @dev: Pointer to the underlying device */
>> +	struct device *dev;
>> +	/** @qdev: Back-pointer to the parent QDA device */
>> +	struct qda_dev *qdev;
>> +	/** @id: Unique identifier assigned by the memory manager */
>> +	u32 id;
> 
> What kind of identifier? What is going to use it? I don't see any
> readers in this patch, please don't add useless data.
I'll move this to the patch where it's actually used.>
>> +	/** @sid: Stream ID for IOMMU transactions */
>> +	u32 sid;
> 
> Do you need to store it?
will move it to later patch where it's actually used.>
>> +};
>> +
>> +/**
>> + * struct qda_memory_manager - Central memory management coordinator
>> + *
>> + * Coordinates memory management across multiple IOMMU devices. Maintains
>> + * a registry of devices in an array sized to the number of context banks
>> + * described in the device tree.
>> + */
>> +struct qda_memory_manager {
>> +	/** @devices: Array storing registered IOMMU devices */
>> +	struct qda_iommu_device **devices;
>> +	/** @num_devices: Number of registered IOMMU devices */
>> +	int num_devices;
> 
> What for? Is devices array to be looped up to num_devices or
> max_devices?
num_devices is to trace device registry with manager, it grows on
register operation. I'll improve the comment.
> 
>> +	/** @max_devices: Capacity of the @devices array */
>> +	int max_devices;
>> +};
>> +
>> +int qda_memory_manager_init(struct qda_memory_manager *mem_mgr, int max_devices);
>> +void qda_memory_manager_exit(struct qda_memory_manager *mem_mgr);
>> +
>> +int qda_memory_manager_register_device(struct qda_memory_manager *mem_mgr,
>> +				       struct qda_iommu_device *iommu_dev);
>> +void qda_memory_manager_unregister_device(struct qda_memory_manager *mem_mgr,
>> +					  struct qda_iommu_device *iommu_dev);
>> +
>> +#endif /* __QDA_MEMORY_MANAGER_H__ */
>> diff --git a/drivers/accel/qda/qda_rpmsg.c b/drivers/accel/qda/qda_rpmsg.c
>> index 723b3e61bf0a..64bf503106d9 100644
>> --- a/drivers/accel/qda/qda_rpmsg.c
>> +++ b/drivers/accel/qda/qda_rpmsg.c
>> @@ -42,11 +42,25 @@ static void qda_rpmsg_remove(struct rpmsg_device *rpdev)
>>  	drm_dev_unplug(&qdev->drm_dev);
>>  	qdev->rpdev = NULL;
>>  	qda_cb_unpopulate(qdev);
>> +	qda_deinit_device(qdev);
>> +}
>> +
>> +static int qda_count_cb_nodes(struct device_node *parent_node)
>> +{
>> +	struct device_node *child;
>> +	int count = 0;
>> +
>> +	for_each_child_of_node(parent_node, child)
>> +		if (of_device_is_compatible(child, "qcom,fastrpc-compute-cb"))
>> +			count++;
>> +
>> +	return count;
>>  }
>>  
>>  static int qda_rpmsg_probe(struct rpmsg_device *rpdev)
>>  {
>>  	struct qda_dev *qdev;
>> +	int num_cbs;
>>  	int ret;
>>  
>>  	qdev = qda_rpmsg_alloc_and_init_qdev(rpdev);
>> @@ -59,10 +73,15 @@ static int qda_rpmsg_probe(struct rpmsg_device *rpdev)
>>  		return ret;
>>  	}
>>  
>> -	ret = qda_cb_populate(qdev, rpdev->dev.of_node);
>> +	num_cbs = qda_count_cb_nodes(rpdev->dev.of_node);
>> +	ret = qda_init_device(qdev, num_cbs);
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = qda_cb_populate(qdev, rpdev->dev.of_node);
>> +	if (ret)
>> +		goto err_deinit;
>> +
>>  	ret = qda_register_device(qdev);
>>  	if (ret)
>>  		goto err_unpopulate;
>> @@ -71,6 +90,8 @@ static int qda_rpmsg_probe(struct rpmsg_device *rpdev)
>>  
>>  err_unpopulate:
>>  	qda_cb_unpopulate(qdev);
>> +err_deinit:
>> +	qda_deinit_device(qdev);
>>  	return ret;
>>  }
>>  
>>
>> -- 
>> 2.34.1
>>
>