[PATCH 0/5] Venus fatal error handling

[PATCH 0/5] Venus fatal error handling

[Stanimir Varbanov]

According to the stateful decoder spec fatal errors could be
recovered either by close and open it again the file descriptor
or by stop the streaming and starting it again. This patch series
is attempting to solve the second fatal recovery case.

regards,
Stan

Stanimir Varbanov (5):
  venus: venc: Use pmruntime autosuspend
  venus: Make sys_error flag an atomic bitops
  venus: hfi: Check for sys error on session hfi functions
  venus: helpers: Add helper to mark fatal vb2 error
  venus: Handle fatal errors during encoding and decoding

 drivers/media/platform/qcom/venus/core.c    |  13 ++-
 drivers/media/platform/qcom/venus/core.h    |   6 +-
 drivers/media/platform/qcom/venus/helpers.c |  16 ++-
 drivers/media/platform/qcom/venus/helpers.h |   1 +
 drivers/media/platform/qcom/venus/hfi.c     |  48 +++++++-
 drivers/media/platform/qcom/venus/vdec.c    |  18 ++-
 drivers/media/platform/qcom/venus/venc.c    | 117 ++++++++++++++++++--
 7 files changed, 202 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH 1/5] venus: venc: Use pmruntime autosuspend

[Stanimir Varbanov]

Migrate encoder to use pm-runtime autosuspend APIs.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/venc.c | 104 +++++++++++++++++++++--
 1 file changed, 96 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 4a7291f934b6..a7a858f03ba3 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -536,6 +536,64 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
 	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
 };
 
+static int venc_pm_get(struct venus_inst *inst)
+{
+	struct venus_core *core = inst->core;
+	struct device *dev = core->dev_enc;
+	int ret;
+
+	mutex_lock(&core->pm_lock);
+	ret = pm_runtime_get_sync(dev);
+	mutex_unlock(&core->pm_lock);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int venc_pm_put(struct venus_inst *inst, bool autosuspend)
+{
+	struct venus_core *core = inst->core;
+	struct device *dev = core->dev_enc;
+	int ret;
+
+	mutex_lock(&core->pm_lock);
+
+	if (autosuspend)
+		ret = pm_runtime_put_autosuspend(dev);
+	else
+		ret = pm_runtime_put_sync(dev);
+
+	mutex_unlock(&core->pm_lock);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int venc_pm_get_put(struct venus_inst *inst)
+{
+	struct venus_core *core = inst->core;
+	struct device *dev = core->dev_enc;
+	int ret = 0;
+
+	mutex_lock(&core->pm_lock);
+
+	if (pm_runtime_suspended(dev)) {
+		ret = pm_runtime_get_sync(dev);
+		if (ret < 0)
+			goto error;
+
+		ret = pm_runtime_put_autosuspend(dev);
+	}
+
+error:
+	mutex_unlock(&core->pm_lock);
+
+	return ret < 0 ? ret : 0;
+}
+
+static void venc_pm_touch(struct venus_inst *inst)
+{
+	pm_runtime_mark_last_busy(inst->core->dev_enc);
+}
+
 static int venc_set_properties(struct venus_inst *inst)
 {
 	struct venc_controls *ctr = &inst->controls.enc;
@@ -891,10 +949,18 @@ static int venc_queue_setup(struct vb2_queue *q,
 		return 0;
 	}
 
+	ret = venc_pm_get(inst);
+	if (ret)
+		return ret;
+
 	mutex_lock(&inst->lock);
 	ret = venc_init_session(inst);
 	mutex_unlock(&inst->lock);
 
+	if (ret)
+		goto put_power;
+
+	ret = venc_pm_put(inst, false);
 	if (ret)
 		return ret;
 
@@ -930,6 +996,9 @@ static int venc_queue_setup(struct vb2_queue *q,
 		break;
 	}
 
+	return ret;
+put_power:
+	venc_pm_put(inst, false);
 	return ret;
 }
 
@@ -946,6 +1015,8 @@ static void venc_release_session(struct venus_inst *inst)
 {
 	int ret;
 
+	venc_pm_get(inst);
+
 	mutex_lock(&inst->lock);
 
 	ret = hfi_session_deinit(inst);
@@ -957,6 +1028,8 @@ static void venc_release_session(struct venus_inst *inst)
 	venus_pm_load_scale(inst);
 	INIT_LIST_HEAD(&inst->registeredbufs);
 	venus_pm_release_core(inst);
+
+	venc_pm_put(inst, false);
 }
 
 static void venc_buf_cleanup(struct vb2_buffer *vb)
@@ -1026,7 +1099,15 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
 	inst->sequence_cap = 0;
 	inst->sequence_out = 0;
 
+	ret = venc_pm_get(inst);
+	if (ret)
+		goto error;
+
 	ret = venus_pm_acquire_core(inst);
+	if (ret)
+		goto put_power;
+
+	ret = venc_pm_put(inst, true);
 	if (ret)
 		goto error;
 
@@ -1051,6 +1132,8 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
 
 	return 0;
 
+put_power:
+	venc_pm_put(inst, false);
 error:
 	venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);
 	if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
@@ -1065,6 +1148,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer *vb)
 {
 	struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
 
+	venc_pm_get_put(inst);
+
 	mutex_lock(&inst->lock);
 	venus_helper_vb2_buf_queue(vb);
 	mutex_unlock(&inst->lock);
@@ -1088,6 +1173,8 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
 	struct vb2_buffer *vb;
 	unsigned int type;
 
+	venc_pm_touch(inst);
+
 	if (buf_type == HFI_BUFFER_INPUT)
 		type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
 	else
@@ -1117,6 +1204,8 @@ static void venc_event_notify(struct venus_inst *inst, u32 event,
 {
 	struct device *dev = inst->core->dev_enc;
 
+	venc_pm_touch(inst);
+
 	if (event == EVT_SESSION_ERROR) {
 		inst->session_error = true;
 		dev_err(dev, "enc: event session error %x\n", inst->error);
@@ -1205,13 +1294,9 @@ static int venc_open(struct file *file)
 
 	venus_helper_init_instance(inst);
 
-	ret = pm_runtime_get_sync(core->dev_enc);
-	if (ret < 0)
-		goto err_put_sync;
-
 	ret = venc_ctrl_init(inst);
 	if (ret)
-		goto err_put_sync;
+		goto err_free;
 
 	ret = hfi_session_create(inst, &venc_hfi_ops);
 	if (ret)
@@ -1250,8 +1335,7 @@ static int venc_open(struct file *file)
 	hfi_session_destroy(inst);
 err_ctrl_deinit:
 	venc_ctrl_deinit(inst);
-err_put_sync:
-	pm_runtime_put_sync(core->dev_enc);
+err_free:
 	kfree(inst);
 	return ret;
 }
@@ -1260,6 +1344,8 @@ static int venc_close(struct file *file)
 {
 	struct venus_inst *inst = to_inst(file);
 
+	venc_pm_get(inst);
+
 	v4l2_m2m_ctx_release(inst->m2m_ctx);
 	v4l2_m2m_release(inst->m2m_dev);
 	venc_ctrl_deinit(inst);
@@ -1268,7 +1354,7 @@ static int venc_close(struct file *file)
 	v4l2_fh_del(&inst->fh);
 	v4l2_fh_exit(&inst->fh);
 
-	pm_runtime_put_sync(inst->core->dev_enc);
+	venc_pm_put(inst, false);
 
 	kfree(inst);
 	return 0;
@@ -1325,6 +1411,8 @@ static int venc_probe(struct platform_device *pdev)
 	core->dev_enc = dev;
 
 	video_set_drvdata(vdev, core);
+	pm_runtime_set_autosuspend_delay(dev, 2000);
+	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
 	return 0;
-- 
2.25.1

[PATCH 2/5] venus: Make sys_error flag an atomic bitops

[Stanimir Varbanov]

Make the sys_error flag an atomic bitops in order to avoid
locking in sys_error readers.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c    | 4 ++--
 drivers/media/platform/qcom/venus/core.h    | 3 ++-
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 drivers/media/platform/qcom/venus/hfi.c     | 2 +-
 drivers/media/platform/qcom/venus/vdec.c    | 2 +-
 5 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 91b15842c555..cc6195f2409c 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -65,7 +65,7 @@ static void venus_event_notify(struct venus_core *core, u32 event)
 	}
 
 	mutex_lock(&core->lock);
-	core->sys_error = true;
+	set_bit(0, &core->sys_error);
 	list_for_each_entry(inst, &core->instances, list)
 		inst->ops->event_notify(inst, EVT_SESSION_ERROR, NULL);
 	mutex_unlock(&core->lock);
@@ -161,7 +161,7 @@ static void venus_sys_error_handler(struct work_struct *work)
 	dev_warn(core->dev, "system error has occurred (recovered)\n");
 
 	mutex_lock(&core->lock);
-	core->sys_error = false;
+	clear_bit(0, &core->sys_error);
 	mutex_unlock(&core->lock);
 }
 
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 745f226a523f..15713209cc48 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -7,6 +7,7 @@
 #ifndef __VENUS_CORE_H_
 #define __VENUS_CORE_H_
 
+#include <linux/bitops.h>
 #include <linux/list.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/v4l2-ctrls.h>
@@ -182,7 +183,7 @@ struct venus_core {
 	unsigned int state;
 	struct completion done;
 	unsigned int error;
-	bool sys_error;
+	unsigned long sys_error;
 	const struct hfi_core_ops *core_ops;
 	const struct venus_pm_ops *pm_ops;
 	struct mutex pm_lock;
diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index b813d6dba481..de87f4c81a1c 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1478,7 +1478,7 @@ void venus_helper_vb2_stop_streaming(struct vb2_queue *q)
 		ret |= venus_helper_intbufs_free(inst);
 		ret |= hfi_session_deinit(inst);
 
-		if (inst->session_error || core->sys_error)
+		if (inst->session_error || test_bit(0, &core->sys_error))
 			ret = -EIO;
 
 		if (ret)
diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
index 0f2482367e06..179b1f8b2650 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -214,7 +214,7 @@ int hfi_session_init(struct venus_inst *inst, u32 pixfmt)
 	 * session_init() can't pass successfully
 	 */
 	mutex_lock(&core->lock);
-	if (!core->ops || core->sys_error) {
+	if (!core->ops || test_bit(0, &inst->core->sys_error)) {
 		mutex_unlock(&core->lock);
 		return -EIO;
 	}
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index ddb7cd39424e..132a2921902a 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1211,7 +1211,7 @@ static void vdec_session_release(struct venus_inst *inst)
 	ret = hfi_session_deinit(inst);
 	abort = (ret && ret != -EINVAL) ? 1 : 0;
 
-	if (inst->session_error || core->sys_error)
+	if (inst->session_error || test_bit(0, &core->sys_error))
 		abort = 1;
 
 	if (abort)
-- 
2.25.1

[PATCH 3/5] venus: hfi: Check for sys error on session hfi functions

[Stanimir Varbanov]

Check sys error flag for all hfi_session_xxx functions and
exit with EIO in case of an error.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/hfi.c | 46 +++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/hfi.c b/drivers/media/platform/qcom/venus/hfi.c
index 179b1f8b2650..4e2151fb47f0 100644
--- a/drivers/media/platform/qcom/venus/hfi.c
+++ b/drivers/media/platform/qcom/venus/hfi.c
@@ -187,6 +187,11 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
 
 	mutex_lock(&core->lock);
 
+	if (test_bit(0, &inst->core->sys_error)) {
+		ret = -EIO;
+		goto unlock;
+	}
+
 	max = atomic_add_unless(&core->insts_count, 1,
 				core->max_sessions_supported);
 	if (!max) {
@@ -196,6 +201,7 @@ int hfi_session_create(struct venus_inst *inst, const struct hfi_inst_ops *ops)
 		ret = 0;
 	}
 
+unlock:
 	mutex_unlock(&core->lock);
 
 	return ret;
@@ -263,6 +269,9 @@ int hfi_session_deinit(struct venus_inst *inst)
 	if (inst->state < INST_INIT)
 		return -EINVAL;
 
+	if (test_bit(0, &inst->core->sys_error))
+		goto done;
+
 	reinit_completion(&inst->done);
 
 	ret = ops->session_end(inst);
@@ -273,6 +282,7 @@ int hfi_session_deinit(struct venus_inst *inst)
 	if (ret)
 		return ret;
 
+done:
 	inst->state = INST_UNINIT;
 
 	return 0;
@@ -284,6 +294,9 @@ int hfi_session_start(struct venus_inst *inst)
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	if (inst->state != INST_LOAD_RESOURCES)
 		return -EINVAL;
 
@@ -308,6 +321,9 @@ int hfi_session_stop(struct venus_inst *inst)
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	if (inst->state != INST_START)
 		return -EINVAL;
 
@@ -331,6 +347,9 @@ int hfi_session_continue(struct venus_inst *inst)
 {
 	struct venus_core *core = inst->core;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	if (core->res->hfi_version == HFI_VERSION_1XX)
 		return 0;
 
@@ -343,6 +362,9 @@ int hfi_session_abort(struct venus_inst *inst)
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	reinit_completion(&inst->done);
 
 	ret = ops->session_abort(inst);
@@ -362,6 +384,9 @@ int hfi_session_load_res(struct venus_inst *inst)
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	if (inst->state != INST_INIT)
 		return -EINVAL;
 
@@ -385,6 +410,9 @@ int hfi_session_unload_res(struct venus_inst *inst)
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	if (inst->state != INST_STOP)
 		return -EINVAL;
 
@@ -409,6 +437,9 @@ int hfi_session_flush(struct venus_inst *inst, u32 type, bool block)
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	reinit_completion(&inst->done);
 
 	ret = ops->session_flush(inst, type);
@@ -429,6 +460,9 @@ int hfi_session_set_buffers(struct venus_inst *inst, struct hfi_buffer_desc *bd)
 {
 	const struct hfi_ops *ops = inst->core->ops;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	return ops->session_set_buffers(inst, bd);
 }
 
@@ -438,6 +472,9 @@ int hfi_session_unset_buffers(struct venus_inst *inst,
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	reinit_completion(&inst->done);
 
 	ret = ops->session_unset_buffers(inst, bd);
@@ -460,6 +497,9 @@ int hfi_session_get_property(struct venus_inst *inst, u32 ptype,
 	const struct hfi_ops *ops = inst->core->ops;
 	int ret;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	if (inst->state < INST_INIT || inst->state >= INST_STOP)
 		return -EINVAL;
 
@@ -483,6 +523,9 @@ int hfi_session_set_property(struct venus_inst *inst, u32 ptype, void *pdata)
 {
 	const struct hfi_ops *ops = inst->core->ops;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	if (inst->state < INST_INIT || inst->state >= INST_STOP)
 		return -EINVAL;
 
@@ -494,6 +537,9 @@ int hfi_session_process_buf(struct venus_inst *inst, struct hfi_frame_data *fd)
 {
 	const struct hfi_ops *ops = inst->core->ops;
 
+	if (test_bit(0, &inst->core->sys_error))
+		return -EIO;
+
 	if (fd->buffer_type == HFI_BUFFER_INPUT)
 		return ops->session_etb(inst, fd);
 	else if (fd->buffer_type == HFI_BUFFER_OUTPUT ||
-- 
2.25.1

[PATCH 4/5] venus: helpers: Add helper to mark fatal vb2 error

[Stanimir Varbanov]

Add a helper to mark source and destination vb2 queues fatal
unrecoverable error.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/helpers.c | 12 ++++++++++++
 drivers/media/platform/qcom/venus/helpers.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index de87f4c81a1c..6c4b64e995a1 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1506,6 +1506,18 @@ void venus_helper_vb2_stop_streaming(struct vb2_queue *q)
 }
 EXPORT_SYMBOL_GPL(venus_helper_vb2_stop_streaming);
 
+void venus_helper_vb2_queue_error(struct venus_inst *inst)
+{
+	struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
+	struct vb2_queue *q;
+
+	q = v4l2_m2m_get_src_vq(m2m_ctx);
+	vb2_queue_error(q);
+	q = v4l2_m2m_get_dst_vq(m2m_ctx);
+	vb2_queue_error(q);
+}
+EXPORT_SYMBOL_GPL(venus_helper_vb2_queue_error);
+
 int venus_helper_process_initial_cap_bufs(struct venus_inst *inst)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = inst->m2m_ctx;
diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h
index e6269b4be3af..6a250c3d3cfe 100644
--- a/drivers/media/platform/qcom/venus/helpers.h
+++ b/drivers/media/platform/qcom/venus/helpers.h
@@ -21,6 +21,7 @@ int venus_helper_vb2_buf_prepare(struct vb2_buffer *vb);
 void venus_helper_vb2_buf_queue(struct vb2_buffer *vb);
 void venus_helper_vb2_stop_streaming(struct vb2_queue *q);
 int venus_helper_vb2_start_streaming(struct venus_inst *inst);
+void venus_helper_vb2_queue_error(struct venus_inst *inst);
 void venus_helper_m2m_device_run(void *priv);
 void venus_helper_m2m_job_abort(void *priv);
 int venus_helper_get_bufreq(struct venus_inst *inst, u32 type,
-- 
2.25.1

[PATCH 5/5] venus: Handle fatal errors during encoding and decoding

[Stanimir Varbanov]

According to stateful decoder docs a fatal failure of decoding
(and encoding) could be recover it by closing the corresponding
file handle and open new one or reinitialize decoding (and encoding)
by stop streaming on both queues. In order to satisfy this
requirement we add a mechanism ins sys_error_handler and
corresponding decoder and encoder drivers to wait for sys_error_done
waitqueue in reqbuf.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/media/platform/qcom/venus/core.c    |  9 ++++++---
 drivers/media/platform/qcom/venus/core.h    |  3 +++
 drivers/media/platform/qcom/venus/helpers.c |  2 ++
 drivers/media/platform/qcom/venus/vdec.c    | 16 ++++++++++++++++
 drivers/media/platform/qcom/venus/venc.c    | 13 +++++++++++++
 5 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index cc6195f2409c..b1fa9b349efb 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -95,9 +95,8 @@ static void venus_sys_error_handler(struct work_struct *work)
 		failed = true;
 	}
 
-	hfi_core_deinit(core, true);
-
-	mutex_lock(&core->lock);
+	core->ops->core_deinit(core);
+	core->state = CORE_UNINIT;
 
 	for (i = 0; i < max_attempts; i++) {
 		if (!pm_runtime_active(core->dev_dec) && !pm_runtime_active(core->dev_enc))
@@ -105,6 +104,8 @@ static void venus_sys_error_handler(struct work_struct *work)
 		msleep(10);
 	}
 
+	mutex_lock(&core->lock);
+
 	venus_shutdown(core);
 
 	venus_coredump(core);
@@ -162,6 +163,7 @@ static void venus_sys_error_handler(struct work_struct *work)
 
 	mutex_lock(&core->lock);
 	clear_bit(0, &core->sys_error);
+	wake_up_all(&core->sys_err_done);
 	mutex_unlock(&core->lock);
 }
 
@@ -318,6 +320,7 @@ static int venus_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&core->instances);
 	mutex_init(&core->lock);
 	INIT_DELAYED_WORK(&core->work, venus_sys_error_handler);
+	init_waitqueue_head(&core->sys_err_done);
 
 	ret = devm_request_threaded_irq(dev, core->irq, hfi_isr, hfi_isr_thread,
 					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 15713209cc48..8e2b4635a396 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -184,6 +184,7 @@ struct venus_core {
 	struct completion done;
 	unsigned int error;
 	unsigned long sys_error;
+	wait_queue_head_t sys_err_done;
 	const struct hfi_core_ops *core_ops;
 	const struct venus_pm_ops *pm_ops;
 	struct mutex pm_lock;
@@ -329,6 +330,7 @@ struct venus_ts_metadata {
  * @registeredbufs:	a list of registered capture bufferes
  * @delayed_process:	a list of delayed buffers
  * @delayed_process_work:	a work_struct for process delayed buffers
+ * @nonblock:		nonblocking flag
  * @ctrl_handler:	v4l control handler
  * @controls:	a union of decoder and encoder control parameters
  * @fh:	 a holder of v4l file handle structure
@@ -392,6 +394,7 @@ struct venus_inst {
 	struct list_head registeredbufs;
 	struct list_head delayed_process;
 	struct work_struct delayed_process_work;
+	bool nonblock;
 
 	struct v4l2_ctrl_handler ctrl_handler;
 	union {
diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 6c4b64e995a1..9b8ff76e3c43 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -1502,6 +1502,8 @@ void venus_helper_vb2_stop_streaming(struct vb2_queue *q)
 
 	venus_pm_release_core(inst);
 
+	inst->session_error = 0;
+
 	mutex_unlock(&inst->lock);
 }
 EXPORT_SYMBOL_GPL(venus_helper_vb2_stop_streaming);
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 132a2921902a..5e5584fc21e9 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -830,6 +830,7 @@ static int vdec_queue_setup(struct vb2_queue *q,
 			    unsigned int sizes[], struct device *alloc_devs[])
 {
 	struct venus_inst *inst = vb2_get_drv_priv(q);
+	struct venus_core *core = inst->core;
 	unsigned int in_num, out_num;
 	int ret = 0;
 
@@ -855,6 +856,16 @@ static int vdec_queue_setup(struct vb2_queue *q,
 		return 0;
 	}
 
+	if (test_bit(0, &core->sys_error)) {
+		if (inst->nonblock)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(core->sys_err_done,
+					       !test_bit(0, &core->sys_error));
+		if (ret)
+			return ret;
+	}
+
 	ret = vdec_pm_get(inst);
 	if (ret)
 		return ret;
@@ -1178,6 +1189,8 @@ static void vdec_stop_streaming(struct vb2_queue *q)
 
 	venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_ERROR);
 
+	inst->session_error = 0;
+
 	if (ret)
 		goto unlock;
 
@@ -1448,6 +1461,7 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event,
 	switch (event) {
 	case EVT_SESSION_ERROR:
 		inst->session_error = true;
+		venus_helper_vb2_queue_error(inst);
 		dev_err(dev, "dec: event session error %x\n", inst->error);
 		break;
 	case EVT_SYS_EVENT_CHANGE:
@@ -1568,6 +1582,8 @@ static int vdec_open(struct file *file)
 	inst->bit_depth = VIDC_BITDEPTH_8;
 	inst->pic_struct = HFI_INTERLACE_FRAME_PROGRESSIVE;
 	init_waitqueue_head(&inst->reconf_wait);
+	inst->nonblock = file->f_flags & O_NONBLOCK;
+
 	venus_helper_init_instance(inst);
 
 	ret = vdec_ctrl_init(inst);
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index a7a858f03ba3..001b9bba3909 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -926,6 +926,7 @@ static int venc_queue_setup(struct vb2_queue *q,
 			    unsigned int sizes[], struct device *alloc_devs[])
 {
 	struct venus_inst *inst = vb2_get_drv_priv(q);
+	struct venus_core *core = inst->core;
 	unsigned int num, min = 4;
 	int ret;
 
@@ -949,6 +950,16 @@ static int venc_queue_setup(struct vb2_queue *q,
 		return 0;
 	}
 
+	if (test_bit(0, &core->sys_error)) {
+		if (inst->nonblock)
+			return -EAGAIN;
+
+		ret = wait_event_interruptible(core->sys_err_done,
+					       !test_bit(0, &core->sys_error));
+		if (ret)
+			return ret;
+	}
+
 	ret = venc_pm_get(inst);
 	if (ret)
 		return ret;
@@ -1208,6 +1219,7 @@ static void venc_event_notify(struct venus_inst *inst, u32 event,
 
 	if (event == EVT_SESSION_ERROR) {
 		inst->session_error = true;
+		venus_helper_vb2_queue_error(inst);
 		dev_err(dev, "enc: event session error %x\n", inst->error);
 	}
 }
@@ -1291,6 +1303,7 @@ static int venc_open(struct file *file)
 	inst->session_type = VIDC_SESSION_TYPE_ENC;
 	inst->clk_data.core_id = VIDC_CORE_ID_DEFAULT;
 	inst->core_acquired = false;
+	inst->nonblock = file->f_flags & O_NONBLOCK;
 
 	venus_helper_init_instance(inst);
 
-- 
2.25.1

Re: [PATCH 1/5] venus: venc: Use pmruntime autosuspend

[Doug Anderson]

Hi,

On Tue, May 18, 2021 at 8:46 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Migrate encoder to use pm-runtime autosuspend APIs.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/venc.c | 104 +++++++++++++++++++++--
>  1 file changed, 96 insertions(+), 8 deletions(-)

Not a full review but I happened to skim by this patch and it caught
my attention...


> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 4a7291f934b6..a7a858f03ba3 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -536,6 +536,64 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
>         .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>  };
>
> +static int venc_pm_get(struct venus_inst *inst)
> +{
> +       struct venus_core *core = inst->core;
> +       struct device *dev = core->dev_enc;
> +       int ret;
> +
> +       mutex_lock(&core->pm_lock);
> +       ret = pm_runtime_get_sync(dev);
> +       mutex_unlock(&core->pm_lock);

Why do you need a mutex around this?

> +
> +       return ret < 0 ? ret : 0;

Odd but true: if pm_runtime_get_sync() returns an error you still need
to put. If your code below isn't going to do this then you should
handle it here?

> +}
> +
> +static int venc_pm_put(struct venus_inst *inst, bool autosuspend)
> +{
> +       struct venus_core *core = inst->core;
> +       struct device *dev = core->dev_enc;
> +       int ret;
> +
> +       mutex_lock(&core->pm_lock);
> +
> +       if (autosuspend)
> +               ret = pm_runtime_put_autosuspend(dev);
> +       else
> +               ret = pm_runtime_put_sync(dev);
> +
> +       mutex_unlock(&core->pm_lock);
> +
> +       return ret < 0 ? ret : 0;
> +}
> +
> +static int venc_pm_get_put(struct venus_inst *inst)
> +{
> +       struct venus_core *core = inst->core;
> +       struct device *dev = core->dev_enc;
> +       int ret = 0;
> +
> +       mutex_lock(&core->pm_lock);
> +
> +       if (pm_runtime_suspended(dev)) {
> +               ret = pm_runtime_get_sync(dev);
> +               if (ret < 0)
> +                       goto error;

If pm_runtime_get_sync() returns an error you still need to put.


> +
> +               ret = pm_runtime_put_autosuspend(dev);
> +       }
> +
> +error:
> +       mutex_unlock(&core->pm_lock);
> +
> +       return ret < 0 ? ret : 0;
> +}

What is the purpose of "get_put"? It feels like using it would be racy to me.


> +
> +static void venc_pm_touch(struct venus_inst *inst)
> +{
> +       pm_runtime_mark_last_busy(inst->core->dev_enc);
> +}
> +
>  static int venc_set_properties(struct venus_inst *inst)
>  {
>         struct venc_controls *ctr = &inst->controls.enc;
> @@ -891,10 +949,18 @@ static int venc_queue_setup(struct vb2_queue *q,
>                 return 0;
>         }
>
> +       ret = venc_pm_get(inst);
> +       if (ret)
> +               return ret;
> +
>         mutex_lock(&inst->lock);
>         ret = venc_init_session(inst);
>         mutex_unlock(&inst->lock);
>
> +       if (ret)
> +               goto put_power;
> +
> +       ret = venc_pm_put(inst, false);
>         if (ret)
>                 return ret;
>
> @@ -930,6 +996,9 @@ static int venc_queue_setup(struct vb2_queue *q,
>                 break;
>         }
>
> +       return ret;
> +put_power:
> +       venc_pm_put(inst, false);
>         return ret;
>  }
>
> @@ -946,6 +1015,8 @@ static void venc_release_session(struct venus_inst *inst)
>  {
>         int ret;
>
> +       venc_pm_get(inst);
> +
>         mutex_lock(&inst->lock);
>
>         ret = hfi_session_deinit(inst);
> @@ -957,6 +1028,8 @@ static void venc_release_session(struct venus_inst *inst)
>         venus_pm_load_scale(inst);
>         INIT_LIST_HEAD(&inst->registeredbufs);
>         venus_pm_release_core(inst);
> +
> +       venc_pm_put(inst, false);
>  }
>
>  static void venc_buf_cleanup(struct vb2_buffer *vb)
> @@ -1026,7 +1099,15 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
>         inst->sequence_cap = 0;
>         inst->sequence_out = 0;
>
> +       ret = venc_pm_get(inst);
> +       if (ret)
> +               goto error;
> +
>         ret = venus_pm_acquire_core(inst);
> +       if (ret)
> +               goto put_power;
> +
> +       ret = venc_pm_put(inst, true);
>         if (ret)
>                 goto error;
>
> @@ -1051,6 +1132,8 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
>
>         return 0;
>
> +put_power:
> +       venc_pm_put(inst, false);
>  error:
>         venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);
>         if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> @@ -1065,6 +1148,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer *vb)
>  {
>         struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
>
> +       venc_pm_get_put(inst);
> +

I don't know this code at all, but I don't understand the point of the
"get_put". Couldn't the task running this code get scheduled out for 2
seconds right after your call to venc_pm_get_put() and then it would
be just like you didn't call it?

...or maybe the device wasn't suspended but it was 10 us away from
being suspended so your "get_put" decided it didn't need to do
anything. Then you get scheduled out for 10 us and it powers off.

Maybe there's a good reason for get_put() to exist and a good reason
why it's race-free but it feels like the kind of thing that needs a
comment.


>         mutex_lock(&inst->lock);
>         venus_helper_vb2_buf_queue(vb);
>         mutex_unlock(&inst->lock);
> @@ -1088,6 +1173,8 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
>         struct vb2_buffer *vb;
>         unsigned int type;
>
> +       venc_pm_touch(inst);
> +
>         if (buf_type == HFI_BUFFER_INPUT)
>                 type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
>         else
> @@ -1117,6 +1204,8 @@ static void venc_event_notify(struct venus_inst *inst, u32 event,
>  {
>         struct device *dev = inst->core->dev_enc;
>
> +       venc_pm_touch(inst);
> +
>         if (event == EVT_SESSION_ERROR) {
>                 inst->session_error = true;
>                 dev_err(dev, "enc: event session error %x\n", inst->error);
> @@ -1205,13 +1294,9 @@ static int venc_open(struct file *file)
>
>         venus_helper_init_instance(inst);
>
> -       ret = pm_runtime_get_sync(core->dev_enc);
> -       if (ret < 0)
> -               goto err_put_sync;
> -
>         ret = venc_ctrl_init(inst);
>         if (ret)
> -               goto err_put_sync;
> +               goto err_free;
>
>         ret = hfi_session_create(inst, &venc_hfi_ops);
>         if (ret)
> @@ -1250,8 +1335,7 @@ static int venc_open(struct file *file)
>         hfi_session_destroy(inst);
>  err_ctrl_deinit:
>         venc_ctrl_deinit(inst);
> -err_put_sync:
> -       pm_runtime_put_sync(core->dev_enc);
> +err_free:
>         kfree(inst);
>         return ret;
>  }
> @@ -1260,6 +1344,8 @@ static int venc_close(struct file *file)
>  {
>         struct venus_inst *inst = to_inst(file);
>
> +       venc_pm_get(inst);
> +
>         v4l2_m2m_ctx_release(inst->m2m_ctx);
>         v4l2_m2m_release(inst->m2m_dev);
>         venc_ctrl_deinit(inst);
> @@ -1268,7 +1354,7 @@ static int venc_close(struct file *file)
>         v4l2_fh_del(&inst->fh);
>         v4l2_fh_exit(&inst->fh);
>
> -       pm_runtime_put_sync(inst->core->dev_enc);
> +       venc_pm_put(inst, false);
>
>         kfree(inst);
>         return 0;
> @@ -1325,6 +1411,8 @@ static int venc_probe(struct platform_device *pdev)
>         core->dev_enc = dev;
>
>         video_set_drvdata(vdev, core);
> +       pm_runtime_set_autosuspend_delay(dev, 2000);
> +       pm_runtime_use_autosuspend(dev);
>         pm_runtime_enable(dev);

You have the same bug that I just made in another module! :-P
Specifically, the pm_runtime docs say:

> Drivers in ->remove() callback should undo the runtime PM changes done
> in ->probe(). Usually this means calling pm_runtime_disable(),
> pm_runtime_dont_use_autosuspend() etc.

-Doug

Re: [PATCH 1/5] venus: venc: Use pmruntime autosuspend

[Stanimir Varbanov]

Hi Doug,

Thanks for the comments!

On 5/19/21 7:51 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, May 18, 2021 at 8:46 AM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Migrate encoder to use pm-runtime autosuspend APIs.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/venc.c | 104 +++++++++++++++++++++--
>>  1 file changed, 96 insertions(+), 8 deletions(-)
> 
> Not a full review but I happened to skim by this patch and it caught
> my attention...
> 

Thanks!

> 
>> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
>> index 4a7291f934b6..a7a858f03ba3 100644
>> --- a/drivers/media/platform/qcom/venus/venc.c
>> +++ b/drivers/media/platform/qcom/venus/venc.c
>> @@ -536,6 +536,64 @@ static const struct v4l2_ioctl_ops venc_ioctl_ops = {
>>         .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>>  };
>>
>> +static int venc_pm_get(struct venus_inst *inst)
>> +{
>> +       struct venus_core *core = inst->core;
>> +       struct device *dev = core->dev_enc;
>> +       int ret;
>> +
>> +       mutex_lock(&core->pm_lock);
>> +       ret = pm_runtime_get_sync(dev);
>> +       mutex_unlock(&core->pm_lock);
> 
> Why do you need a mutex around this?


> 
>> +
>> +       return ret < 0 ? ret : 0;
> 
> Odd but true: if pm_runtime_get_sync() returns an error you still need
> to put. If your code below isn't going to do this then you should
> handle it here?

I have v2 where I use pm_runtime_resume_and_get().

> 
>> +}
>> +
>> +static int venc_pm_put(struct venus_inst *inst, bool autosuspend)
>> +{
>> +       struct venus_core *core = inst->core;
>> +       struct device *dev = core->dev_enc;
>> +       int ret;
>> +
>> +       mutex_lock(&core->pm_lock);
>> +
>> +       if (autosuspend)
>> +               ret = pm_runtime_put_autosuspend(dev);
>> +       else
>> +               ret = pm_runtime_put_sync(dev);
>> +
>> +       mutex_unlock(&core->pm_lock);
>> +
>> +       return ret < 0 ? ret : 0;
>> +}
>> +
>> +static int venc_pm_get_put(struct venus_inst *inst)
>> +{
>> +       struct venus_core *core = inst->core;
>> +       struct device *dev = core->dev_enc;
>> +       int ret = 0;
>> +
>> +       mutex_lock(&core->pm_lock);
>> +
>> +       if (pm_runtime_suspended(dev)) {
>> +               ret = pm_runtime_get_sync(dev);
>> +               if (ret < 0)
>> +                       goto error;
> 
> If pm_runtime_get_sync() returns an error you still need to put.

In v2 I replaced with pm_runtime_resume_and_get().

> 
> 
>> +
>> +               ret = pm_runtime_put_autosuspend(dev);
>> +       }
>> +
>> +error:
>> +       mutex_unlock(&core->pm_lock);
>> +
>> +       return ret < 0 ? ret : 0;
>> +}
> 
> What is the purpose of "get_put"? It feels like using it would be racy to me.

See below ...

> 
> 
>> +
>> +static void venc_pm_touch(struct venus_inst *inst)
>> +{
>> +       pm_runtime_mark_last_busy(inst->core->dev_enc);
>> +}
>> +
>>  static int venc_set_properties(struct venus_inst *inst)
>>  {
>>         struct venc_controls *ctr = &inst->controls.enc;
>> @@ -891,10 +949,18 @@ static int venc_queue_setup(struct vb2_queue *q,
>>                 return 0;
>>         }
>>
>> +       ret = venc_pm_get(inst);
>> +       if (ret)
>> +               return ret;
>> +
>>         mutex_lock(&inst->lock);
>>         ret = venc_init_session(inst);
>>         mutex_unlock(&inst->lock);
>>
>> +       if (ret)
>> +               goto put_power;
>> +
>> +       ret = venc_pm_put(inst, false);
>>         if (ret)
>>                 return ret;
>>
>> @@ -930,6 +996,9 @@ static int venc_queue_setup(struct vb2_queue *q,
>>                 break;
>>         }
>>
>> +       return ret;
>> +put_power:
>> +       venc_pm_put(inst, false);
>>         return ret;
>>  }
>>
>> @@ -946,6 +1015,8 @@ static void venc_release_session(struct venus_inst *inst)
>>  {
>>         int ret;
>>
>> +       venc_pm_get(inst);
>> +
>>         mutex_lock(&inst->lock);
>>
>>         ret = hfi_session_deinit(inst);
>> @@ -957,6 +1028,8 @@ static void venc_release_session(struct venus_inst *inst)
>>         venus_pm_load_scale(inst);
>>         INIT_LIST_HEAD(&inst->registeredbufs);
>>         venus_pm_release_core(inst);
>> +
>> +       venc_pm_put(inst, false);
>>  }
>>
>>  static void venc_buf_cleanup(struct vb2_buffer *vb)
>> @@ -1026,7 +1099,15 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
>>         inst->sequence_cap = 0;
>>         inst->sequence_out = 0;
>>
>> +       ret = venc_pm_get(inst);
>> +       if (ret)
>> +               goto error;
>> +
>>         ret = venus_pm_acquire_core(inst);
>> +       if (ret)
>> +               goto put_power;
>> +
>> +       ret = venc_pm_put(inst, true);
>>         if (ret)
>>                 goto error;
>>
>> @@ -1051,6 +1132,8 @@ static int venc_start_streaming(struct vb2_queue *q, unsigned int count)
>>
>>         return 0;
>>
>> +put_power:
>> +       venc_pm_put(inst, false);
>>  error:
>>         venus_helper_buffers_done(inst, q->type, VB2_BUF_STATE_QUEUED);
>>         if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>> @@ -1065,6 +1148,8 @@ static void venc_vb2_buf_queue(struct vb2_buffer *vb)
>>  {
>>         struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
>>
>> +       venc_pm_get_put(inst);
>> +
> 
> I don't know this code at all, but I don't understand the point of the
> "get_put". Couldn't the task running this code get scheduled out for 2
> seconds right after your call to venc_pm_get_put() and then it would
> be just like you didn't call it?
> 
> ...or maybe the device wasn't suspended but it was 10 us away from
> being suspended so your "get_put" decided it didn't need to do
> anything. Then you get scheduled out for 10 us and it powers off.
> 
> Maybe there's a good reason for get_put() to exist and a good reason
> why it's race-free but it feels like the kind of thing that needs a
> comment.
> 
> 

This technique was used in decoder for some time now without any issues,
so I guess it is fine in respect to races.

The idea of venc|vdec_pm_get_put was to resume pmruntime in case that 2s
elapsed and the driver does not receive any buffer (through
vb2_buf_queue()) during that time period. In this case (and there is no
other activity) the power and clocks will be turned off and we have to
power them up on next vb2_buf_queue.

>>         mutex_lock(&inst->lock);
>>         venus_helper_vb2_buf_queue(vb);
>>         mutex_unlock(&inst->lock);
>> @@ -1088,6 +1173,8 @@ static void venc_buf_done(struct venus_inst *inst, unsigned int buf_type,
>>         struct vb2_buffer *vb;
>>         unsigned int type;
>>
>> +       venc_pm_touch(inst);
>> +
>>         if (buf_type == HFI_BUFFER_INPUT)
>>                 type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
>>         else
>> @@ -1117,6 +1204,8 @@ static void venc_event_notify(struct venus_inst *inst, u32 event,
>>  {
>>         struct device *dev = inst->core->dev_enc;
>>
>> +       venc_pm_touch(inst);
>> +
>>         if (event == EVT_SESSION_ERROR) {
>>                 inst->session_error = true;
>>                 dev_err(dev, "enc: event session error %x\n", inst->error);
>> @@ -1205,13 +1294,9 @@ static int venc_open(struct file *file)
>>
>>         venus_helper_init_instance(inst);
>>
>> -       ret = pm_runtime_get_sync(core->dev_enc);
>> -       if (ret < 0)
>> -               goto err_put_sync;
>> -
>>         ret = venc_ctrl_init(inst);
>>         if (ret)
>> -               goto err_put_sync;
>> +               goto err_free;
>>
>>         ret = hfi_session_create(inst, &venc_hfi_ops);
>>         if (ret)
>> @@ -1250,8 +1335,7 @@ static int venc_open(struct file *file)
>>         hfi_session_destroy(inst);
>>  err_ctrl_deinit:
>>         venc_ctrl_deinit(inst);
>> -err_put_sync:
>> -       pm_runtime_put_sync(core->dev_enc);
>> +err_free:
>>         kfree(inst);
>>         return ret;
>>  }
>> @@ -1260,6 +1344,8 @@ static int venc_close(struct file *file)
>>  {
>>         struct venus_inst *inst = to_inst(file);
>>
>> +       venc_pm_get(inst);
>> +
>>         v4l2_m2m_ctx_release(inst->m2m_ctx);
>>         v4l2_m2m_release(inst->m2m_dev);
>>         venc_ctrl_deinit(inst);
>> @@ -1268,7 +1354,7 @@ static int venc_close(struct file *file)
>>         v4l2_fh_del(&inst->fh);
>>         v4l2_fh_exit(&inst->fh);
>>
>> -       pm_runtime_put_sync(inst->core->dev_enc);
>> +       venc_pm_put(inst, false);
>>
>>         kfree(inst);
>>         return 0;
>> @@ -1325,6 +1411,8 @@ static int venc_probe(struct platform_device *pdev)
>>         core->dev_enc = dev;
>>
>>         video_set_drvdata(vdev, core);
>> +       pm_runtime_set_autosuspend_delay(dev, 2000);
>> +       pm_runtime_use_autosuspend(dev);
>>         pm_runtime_enable(dev);
> 
> You have the same bug that I just made in another module! :-P
> Specifically, the pm_runtime docs say:
> 
>> Drivers in ->remove() callback should undo the runtime PM changes done
>> in ->probe(). Usually this means calling pm_runtime_disable(),
>> pm_runtime_dont_use_autosuspend() etc.
> 

Sure I will correct this in v2. Thanks!

> -Doug
> 

-- 
regards,
Stan