msm: vidc: Allocate memory dynamically for debugfs info_reads

Use dynamically allocated memory for constructing strings in
core_info_read & inst_info_read. This ensures that there is no
contention for a shared memory & hence avoids the requirement of
a lock. Allocate on demand, as the calls implement a debugfs
facility and hence rarely invoked. Statically allocated memory
otherwise remain idle.

Change-Id: I3ae04e0a51801a2fc901591e41e28ff6b7d198b4
Signed-off-by: Abdulla Anam <abdullahanam@codeaurora.org>

drivers/media/platform/msm/vidc/msm_v4l2_vidc.c

@@ -766,7 +766,6 @@ static int __init msm_vidc_init(void)
 	if (rc) {
 		dprintk(VIDC_ERR,
 			"Failed to register platform driver\n");
-		msm_vidc_debugfs_deinit_drv();
 		debugfs_remove_recursive(vidc_driver->debugfs_root);
 		kfree(vidc_driver);
 		vidc_driver = NULL;
@@ -778,7 +777,6 @@ static int __init msm_vidc_init(void)
 static void __exit msm_vidc_exit(void)
 {
 	platform_driver_unregister(&msm_vidc_driver);
-	msm_vidc_debugfs_deinit_drv();
 	debugfs_remove_recursive(vidc_driver->debugfs_root);
 	mutex_destroy(&vidc_driver->lock);
 	kfree(vidc_driver);

drivers/media/platform/msm/vidc/msm_vidc.c

@@ -1347,8 +1347,6 @@ static void cleanup_instance(struct msm_vidc_inst *inst)
 				"Failed to release output buffers\n");
 		}
 
-		debugfs_remove_recursive(inst->debugfs_root);
-
 		mutex_lock(&inst->pending_getpropq.lock);
 		if (!list_empty(&inst->pending_getpropq.list)) {
 			dprintk(VIDC_ERR,
@@ -1390,6 +1388,8 @@ int msm_vidc_destroy(struct msm_vidc_inst *inst)
 	mutex_destroy(&inst->bufq[OUTPUT_PORT].lock);
 	mutex_destroy(&inst->lock);
 
+	msm_vidc_debugfs_deinit_inst(inst);
+
 	pr_info(VIDC_DBG_TAG "Closed video instance: %pK\n",
 			VIDC_MSG_PRIO2STRING(VIDC_INFO), inst);
 	kfree(inst);

drivers/media/platform/msm/vidc/msm_vidc_debug.c

@@ -38,44 +38,31 @@ bool msm_vidc_debug_timeout = false;
 
 #define MAX_DBG_BUF_SIZE 4096
 
-struct debug_buffer {
-	struct mutex lock;
-	char ptr[MAX_DBG_BUF_SIZE];
-	char *curr;
-	u32 filled_size;
-};
-
-static struct debug_buffer dbg_buf;
-
-#define INIT_DBG_BUF(__buf) ({ \
-	__buf.curr = __buf.ptr;\
-	__buf.filled_size = 0; \
-})
-
 #define DYNAMIC_BUF_OWNER(__binfo) ({ \
 	atomic_read(&__binfo->ref_count) == 2 ? "video driver" : "firmware";\
 })
 
+struct core_inst_pair {
+	struct msm_vidc_core *core;
+	struct msm_vidc_inst *inst;
+};
+
 static int core_info_open(struct inode *inode, struct file *file)
 {
 	file->private_data = inode->i_private;
 	return 0;
 }
 
-static u32 write_str(struct debug_buffer *buffer, const char *fmt, ...)
+static u32 write_str(char *buffer,
+		size_t size, const char *fmt, ...)
 {
 	va_list args;
-	u32 size;
-
-	char *curr = buffer->curr;
-	char *end = buffer->ptr + MAX_DBG_BUF_SIZE;
+	u32 len;
 
 	va_start(args, fmt);
-	size = vscnprintf(curr, end - curr, fmt, args);
+	len = vscnprintf(buffer, size, fmt, args);
 	va_end(args);
-	buffer->curr += size;
-	buffer->filled_size += size;
-	return size;
+	return len;
 }
 
 static ssize_t core_info_read(struct file *file, char __user *buf,
@@ -84,6 +71,7 @@ static ssize_t core_info_read(struct file *file, char __user *buf,
 	struct msm_vidc_core *core = file->private_data;
 	struct hfi_device *hdev;
 	struct hal_fw_info fw_info = { {0} };
+	char *dbuf, *cur, *end;
 	int i = 0, rc = 0;
 	ssize_t len = 0;
 
@@ -91,36 +79,46 @@ static ssize_t core_info_read(struct file *file, char __user *buf,
 		dprintk(VIDC_ERR, "Invalid params, core: %pK\n", core);
 		return 0;
 	}
+
+	dbuf = kzalloc(MAX_DBG_BUF_SIZE, GFP_KERNEL);
+	if (!dbuf) {
+		dprintk(VIDC_ERR, "%s: Allocation failed!\n", __func__);
+		return -ENOMEM;
+	}
+	cur = dbuf;
+	end = cur + MAX_DBG_BUF_SIZE;
 	hdev = core->device;
 
-	mutex_lock(&dbg_buf.lock);
-	INIT_DBG_BUF(dbg_buf);
-	write_str(&dbg_buf, "===============================\n");
-	write_str(&dbg_buf, "CORE %d: %pK\n", core->id, core);
-	write_str(&dbg_buf, "===============================\n");
-	write_str(&dbg_buf, "Core state: %d\n", core->state);
+	cur += write_str(cur, end - cur, "===============================\n");
+	cur += write_str(cur, end - cur, "CORE %d: %pK\n", core->id, core);
+	cur += write_str(cur, end - cur, "===============================\n");
+	cur += write_str(cur, end - cur, "Core state: %d\n", core->state);
 	rc = call_hfi_op(hdev, get_fw_info, hdev->hfi_device_data, &fw_info);
 	if (rc) {
 		dprintk(VIDC_WARN, "Failed to read FW info\n");
 		goto err_fw_info;
 	}
 
-	write_str(&dbg_buf, "FW version : %s\n", &fw_info.version);
-	write_str(&dbg_buf, "base addr: 0x%x\n", fw_info.base_addr);
-	write_str(&dbg_buf, "register_base: 0x%x\n", fw_info.register_base);
-	write_str(&dbg_buf, "register_size: %u\n", fw_info.register_size);
-	write_str(&dbg_buf, "irq: %u\n", fw_info.irq);
+	cur += write_str(cur, end - cur,
+		"FW version : %s\n", &fw_info.version);
+	cur += write_str(cur, end - cur,
+		"base addr: 0x%x\n", fw_info.base_addr);
+	cur += write_str(cur, end - cur,
+		"register_base: 0x%x\n", fw_info.register_base);
+	cur += write_str(cur, end - cur,
+		"register_size: %u\n", fw_info.register_size);
+	cur += write_str(cur, end - cur, "irq: %u\n", fw_info.irq);
 
 err_fw_info:
 	for (i = SYS_MSG_START; i < SYS_MSG_END; i++) {
-		write_str(&dbg_buf, "completions[%d]: %s\n", i,
+		cur += write_str(cur, end - cur, "completions[%d]: %s\n", i,
 			completion_done(&core->completions[SYS_MSG_INDEX(i)]) ?
 			"pending" : "done");
 	}
 	len = simple_read_from_buffer(buf, count, ppos,
-			dbg_buf.ptr, dbg_buf.filled_size);
+			dbuf, cur - dbuf);
 
-	mutex_unlock(&dbg_buf.lock);
+	kfree(dbuf);
 	return len;
 }
 
@@ -177,7 +175,6 @@ struct dentry *msm_vidc_debugfs_init_drv(void)
 	bool ok = false;
 	struct dentry *dir = NULL;
 
-	mutex_init(&dbg_buf.lock);
 	dir = debugfs_create_dir("msm_vidc", NULL);
 	if (IS_ERR_OR_NULL(dir)) {
 		dir = NULL;
@@ -263,12 +260,15 @@ failed_create_dir:
 
 static int inst_info_open(struct inode *inode, struct file *file)
 {
+	dprintk(VIDC_INFO, "Open inode ptr: %pK\n", inode->i_private);
 	file->private_data = inode->i_private;
 	return 0;
 }
 
-static int publish_unreleased_reference(struct msm_vidc_inst *inst)
+static int publish_unreleased_reference(struct msm_vidc_inst *inst,
+		char **dbuf, char *end)
 {
+	char *cur = *dbuf;
 	struct buffer_info *temp = NULL;
 
 	if (!inst) {
@@ -277,128 +277,226 @@ static int publish_unreleased_reference(struct msm_vidc_inst *inst)
 	}
 
 	if (inst->buffer_mode_set[CAPTURE_PORT] == HAL_BUFFER_MODE_DYNAMIC) {
-		write_str(&dbg_buf, "Pending buffer references:\n");
+		cur += write_str(cur, end - cur, "Pending buffer references\n");
 
 		mutex_lock(&inst->registeredbufs.lock);
 		list_for_each_entry(temp, &inst->registeredbufs.list, list) {
 			if (temp->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
 			!temp->inactive && atomic_read(&temp->ref_count)) {
-				write_str(&dbg_buf,
-				"\tpending buffer: %#lx fd[0] = %d ref_count = %d held by: %s\n",
-				temp->device_addr[0],
-				temp->fd[0],
-				atomic_read(&temp->ref_count),
-				DYNAMIC_BUF_OWNER(temp));
+				cur += write_str(cur, end - cur,
+					"\tpending buffer: %#lx fd[0] = %d ref_count = %d held by: %s\n",
+					temp->device_addr[0],
+					temp->fd[0],
+					atomic_read(&temp->ref_count),
+					DYNAMIC_BUF_OWNER(temp));
 			}
 		}
 		mutex_unlock(&inst->registeredbufs.lock);
 	}
+
+	*dbuf = cur;
 	return 0;
 }
 
+static void put_inst_helper(struct kref *kref)
+{
+	struct msm_vidc_inst *inst = container_of(kref,
+			struct msm_vidc_inst, kref);
+
+	msm_vidc_destroy(inst);
+}
+
 static ssize_t inst_info_read(struct file *file, char __user *buf,
 		size_t count, loff_t *ppos)
 {
-	struct msm_vidc_inst *inst = file->private_data;
+	struct core_inst_pair *idata = file->private_data;
+	struct msm_vidc_core *core;
+	struct msm_vidc_inst *inst, *temp = NULL;
+	char *dbuf, *cur, *end;
 	int i, j;
 	ssize_t len = 0;
 
-	if (!inst) {
-		dprintk(VIDC_ERR, "Invalid params, inst %pK\n", inst);
+	if (!idata || !idata->core || !idata->inst) {
+		dprintk(VIDC_ERR, "%s: Invalid params\n", __func__);
 		return 0;
 	}
 
-	mutex_lock(&dbg_buf.lock);
-	INIT_DBG_BUF(dbg_buf);
-	write_str(&dbg_buf, "===============================\n");
-	write_str(&dbg_buf, "INSTANCE: %pK (%s)\n", inst,
+	core = idata->core;
+	inst = idata->inst;
+
+	mutex_lock(&core->lock);
+	list_for_each_entry(temp, &core->instances, list) {
+		if (temp == inst)
+			break;
+	}
+	inst = ((temp == inst) && kref_get_unless_zero(&inst->kref)) ?
+		inst : NULL;
+	mutex_unlock(&core->lock);
+
+	if (!inst) {
+		dprintk(VIDC_ERR, "%s: Instance has become obsolete", __func__);
+		return 0;
+	}
+
+	dbuf = kzalloc(MAX_DBG_BUF_SIZE, GFP_KERNEL);
+	if (!dbuf) {
+		dprintk(VIDC_ERR, "%s: Allocation failed!\n", __func__);
+		len = -ENOMEM;
+		goto failed_alloc;
+	}
+	cur = dbuf;
+	end = cur + MAX_DBG_BUF_SIZE;
+
+	cur += write_str(cur, end - cur, "==============================\n");
+	cur += write_str(cur, end - cur, "INSTANCE: %pK (%s)\n", inst,
 		inst->session_type == MSM_VIDC_ENCODER ? "Encoder" : "Decoder");
-	write_str(&dbg_buf, "===============================\n");
-	write_str(&dbg_buf, "core: %pK\n", inst->core);
-	write_str(&dbg_buf, "height: %d\n", inst->prop.height[CAPTURE_PORT]);
-	write_str(&dbg_buf, "width: %d\n", inst->prop.width[CAPTURE_PORT]);
-	write_str(&dbg_buf, "fps: %d\n", inst->prop.fps);
-	write_str(&dbg_buf, "state: %d\n", inst->state);
-	write_str(&dbg_buf, "secure: %d\n", !!(inst->flags & VIDC_SECURE));
-	write_str(&dbg_buf, "-----------Formats-------------\n");
+	cur += write_str(cur, end - cur, "==============================\n");
+	cur += write_str(cur, end - cur, "core: %pK\n", inst->core);
+	cur += write_str(cur, end - cur, "height: %d\n",
+		inst->prop.height[CAPTURE_PORT]);
+	cur += write_str(cur, end - cur, "width: %d\n",
+		inst->prop.width[CAPTURE_PORT]);
+	cur += write_str(cur, end - cur, "fps: %d\n", inst->prop.fps);
+	cur += write_str(cur, end - cur, "state: %d\n", inst->state);
+	cur += write_str(cur, end - cur, "secure: %d\n",
+		!!(inst->flags & VIDC_SECURE));
+	cur += write_str(cur, end - cur, "-----------Formats-------------\n");
 	for (i = 0; i < MAX_PORT_NUM; i++) {
-		write_str(&dbg_buf, "capability: %s\n", i == OUTPUT_PORT ?
+		cur += write_str(cur, end - cur, "capability: %s\n",
+			i == OUTPUT_PORT ? "Output" : "Capture");
+		cur += write_str(cur, end - cur, "name : %s\n",
+			inst->fmts[i].name);
+		cur += write_str(cur, end - cur, "planes : %d\n",
+			inst->prop.num_planes[i]);
+		cur += write_str(cur, end - cur,
+			"type: %s\n", inst->fmts[i].type == OUTPUT_PORT ?
 			"Output" : "Capture");
-		write_str(&dbg_buf, "name : %s\n", inst->fmts[i].name);
-		write_str(&dbg_buf, "planes : %d\n", inst->prop.num_planes[i]);
-		write_str(
-		&dbg_buf, "type: %s\n", inst->fmts[i].type == OUTPUT_PORT ?
-		"Output" : "Capture");
 		switch (inst->buffer_mode_set[i]) {
 		case HAL_BUFFER_MODE_STATIC:
-			write_str(&dbg_buf, "buffer mode : %s\n", "static");
+			cur += write_str(cur, end - cur,
+				"buffer mode : %s\n", "static");
 			break;
 		case HAL_BUFFER_MODE_RING:
-			write_str(&dbg_buf, "buffer mode : %s\n", "ring");
+			cur += write_str(cur, end - cur,
+				"buffer mode : %s\n", "ring");
 			break;
 		case HAL_BUFFER_MODE_DYNAMIC:
-			write_str(&dbg_buf, "buffer mode : %s\n", "dynamic");
+			cur += write_str(cur, end - cur,
+				"buffer mode : %s\n", "dynamic");
 			break;
 		default:
-			write_str(&dbg_buf, "buffer mode : unsupported\n");
+			cur += write_str(cur, end - cur,
+				"buffer mode : unsupported\n");
 		}
 
-		write_str(&dbg_buf, "count: %u\n",
+		cur += write_str(cur, end - cur, "count: %u\n",
 				inst->bufq[i].vb2_bufq.num_buffers);
 
 		for (j = 0; j < inst->prop.num_planes[i]; j++)
-			write_str(&dbg_buf, "size for plane %d: %u\n", j,
+			cur += write_str(cur, end - cur,
+			"size for plane %d: %u\n", j,
 			inst->bufq[i].vb2_bufq.plane_sizes[j]);
 
 		if (i < MAX_PORT_NUM - 1)
-			write_str(&dbg_buf, "\n");
+			cur += write_str(cur, end - cur, "\n");
 	}
-	write_str(&dbg_buf, "-------------------------------\n");
+	cur += write_str(cur, end - cur, "-------------------------------\n");
 	for (i = SESSION_MSG_START; i < SESSION_MSG_END; i++) {
-		write_str(&dbg_buf, "completions[%d]: %s\n", i,
+		cur += write_str(cur, end - cur, "completions[%d]: %s\n", i,
 		completion_done(&inst->completions[SESSION_MSG_INDEX(i)]) ?
 		"pending" : "done");
 	}
-	write_str(&dbg_buf, "ETB Count: %d\n", inst->count.etb);
-	write_str(&dbg_buf, "EBD Count: %d\n", inst->count.ebd);
-	write_str(&dbg_buf, "FTB Count: %d\n", inst->count.ftb);
-	write_str(&dbg_buf, "FBD Count: %d\n", inst->count.fbd);
-
-	publish_unreleased_reference(inst);
+	cur += write_str(cur, end - cur, "ETB Count: %d\n", inst->count.etb);
+	cur += write_str(cur, end - cur, "EBD Count: %d\n", inst->count.ebd);
+	cur += write_str(cur, end - cur, "FTB Count: %d\n", inst->count.ftb);
+	cur += write_str(cur, end - cur, "FBD Count: %d\n", inst->count.fbd);
 
+	publish_unreleased_reference(inst, &cur, end);
 	len = simple_read_from_buffer(buf, count, ppos,
-		dbg_buf.ptr, dbg_buf.filled_size);
-	mutex_unlock(&dbg_buf.lock);
+		dbuf, cur - dbuf);
+
+	kfree(dbuf);
+failed_alloc:
+	kref_put(&inst->kref, put_inst_helper);
 	return len;
 }
 
+static int inst_info_release(struct inode *inode, struct file *file)
+{
+	dprintk(VIDC_INFO, "Release inode ptr: %pK\n", inode->i_private);
+	file->private_data = NULL;
+	return 0;
+}
+
 static const struct file_operations inst_info_fops = {
 	.open = inst_info_open,
 	.read = inst_info_read,
+	.release = inst_info_release,
 };
 
 struct dentry *msm_vidc_debugfs_init_inst(struct msm_vidc_inst *inst,
 		struct dentry *parent)
 {
-	struct dentry *dir = NULL;
+	struct dentry *dir = NULL, *info = NULL;
 	char debugfs_name[MAX_DEBUGFS_NAME];
+	struct core_inst_pair *idata = NULL;
+
 	if (!inst) {
 		dprintk(VIDC_ERR, "Invalid params, inst: %pK\n", inst);
-		goto failed_create_dir;
+		goto exit;
 	}
 	snprintf(debugfs_name, MAX_DEBUGFS_NAME, "inst_%p", inst);
+
+	idata = kzalloc(sizeof(struct core_inst_pair), GFP_KERNEL);
+	if (!idata) {
+		dprintk(VIDC_ERR, "%s: Allocation failed!\n", __func__);
+		goto exit;
+	}
+
+	idata->core = inst->core;
+	idata->inst = inst;
+
 	dir = debugfs_create_dir(debugfs_name, parent);
 	if (!dir) {
 		dprintk(VIDC_ERR, "Failed to create debugfs for msm_vidc\n");
 		goto failed_create_dir;
 	}
-	if (!debugfs_create_file("info", S_IRUGO, dir, inst, &inst_info_fops)) {
+
+	info = debugfs_create_file("info", S_IRUGO, dir,
+			idata, &inst_info_fops);
+	if (!info) {
 		dprintk(VIDC_ERR, "debugfs_create_file: fail\n");
-		goto failed_create_dir;
+		goto failed_create_file;
 	}
+
+	dir->d_inode->i_private = info->d_inode->i_private;
 	inst->debug.pdata[FRAME_PROCESSING].sampling = true;
-failed_create_dir:
 	return dir;
+
+failed_create_file:
+	debugfs_remove_recursive(dir);
+	dir = NULL;
+failed_create_dir:
+	kfree(idata);
+exit:
+	return dir;
+}
+
+void msm_vidc_debugfs_deinit_inst(struct msm_vidc_inst *inst)
+{
+	struct dentry *dentry = NULL;
+
+	if (!inst || !inst->debugfs_root)
+		return;
+
+	dentry = inst->debugfs_root;
+	if (dentry->d_inode) {
+		dprintk(VIDC_INFO, "Destroy %pK\n", dentry->d_inode->i_private);
+		kfree(dentry->d_inode->i_private);
+		dentry->d_inode->i_private = NULL;
+	}
+	debugfs_remove_recursive(dentry);
+	inst->debugfs_root = NULL;
 }
 
 void msm_vidc_debugfs_update(struct msm_vidc_inst *inst,
@@ -450,8 +548,3 @@ void msm_vidc_debugfs_update(struct msm_vidc_inst *inst,
 	}
 }
 
-void msm_vidc_debugfs_deinit_drv(void)
-{
-	mutex_destroy(&dbg_buf.lock);
-}
-

drivers/media/platform/msm/vidc/msm_vidc_debug.h

@@ -124,9 +124,9 @@ struct dentry *msm_vidc_debugfs_init_core(struct msm_vidc_core *core,
 		struct dentry *parent);
 struct dentry *msm_vidc_debugfs_init_inst(struct msm_vidc_inst *inst,
 		struct dentry *parent);
+void msm_vidc_debugfs_deinit_inst(struct msm_vidc_inst *inst);
 void msm_vidc_debugfs_update(struct msm_vidc_inst *inst,
 		enum msm_vidc_debugfs_event e);
-void msm_vidc_debugfs_deinit_drv(void);
 
 static inline void tic(struct msm_vidc_inst *i, enum profiling_points p,
 				 char *b)