On (24/12/19 14:03), Bryan O'Donoghue wrote:
On 19/12/2024 05:37, Sergey Senozhatsky wrote:
This partially reverts commit that made hfi_session_destroy() the first step of vdec/venc close(). The reason being is a regression report when, supposedly, encode/decoder is closed with still active streaming (no ->stop_streaming() call before close()) and pending pkts, so isr_thread cannot find instance and fails to process those pending pkts. This was the idea behind the original patch - make it impossible to use instance under destruction, because this is racy, but apparently there are uses cases that depend on that unsafe pattern. Return to the old (unsafe) behaviour for the time being (until a better fix is found).
[..]
Two questions:
1: Will this revert re-instantiate the original bug @
I'm afraid pretty much so, yes. isr_thread runs concurrently with close(), the instance is still in the streaming mode and there are pending pkts. As far as I understand it, stop_streaming() is called from
close() v4l2_m2m_ctx_release() vb2_queue_release() // ->cap_q_ctx.q ->out_q_ctx.q vb2_core_queue_release() __vb2_cleanup_fileio() vb2_core_streamoff()
At least this is how I understand the test that is failing.
I don't have a fix (nor even an idea) at this point.
2: Why not balanced out the ordering of calls by moving hfi_session_create() in vdec_open_common() ? to match the ordering in venus_close_common();
Something like this?
---
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c index 98c22b9f9372..9f82882b77bc 100644 --- a/drivers/media/platform/qcom/venus/vdec.c +++ b/drivers/media/platform/qcom/venus/vdec.c @@ -1697,10 +1697,6 @@ static int vdec_open(struct file *file) if (ret) goto err_free;
- ret = hfi_session_create(inst, &vdec_hfi_ops); - if (ret) - goto err_ctrl_deinit; - vdec_inst_init(inst);
ida_init(&inst->dpb_ids); @@ -1712,15 +1708,19 @@ static int vdec_open(struct file *file) inst->m2m_dev = v4l2_m2m_init(&vdec_m2m_ops); if (IS_ERR(inst->m2m_dev)) { ret = PTR_ERR(inst->m2m_dev); - goto err_session_destroy; + goto err_ctrl_deinit; }
inst->m2m_ctx = v4l2_m2m_ctx_init(inst->m2m_dev, inst, m2m_queue_init); if (IS_ERR(inst->m2m_ctx)) { ret = PTR_ERR(inst->m2m_ctx); - goto err_m2m_release; + goto err_m2m_dev_release; }
+ ret = hfi_session_create(inst, &vdec_hfi_ops); + if (ret) + goto err_m2m_ctx_release; + v4l2_fh_init(&inst->fh, core->vdev_dec);
inst->fh.ctrl_handler = &inst->ctrl_handler; @@ -1730,10 +1730,10 @@ static int vdec_open(struct file *file)
return 0;
-err_m2m_release: +err_m2m_ctx_release: + v4l2_m2m_ctx_release(inst->m2m_ctx); +err_m2m_dev_release: v4l2_m2m_release(inst->m2m_dev); -err_session_destroy: - hfi_session_destroy(inst); err_ctrl_deinit: v4l2_ctrl_handler_free(&inst->ctrl_handler); err_free: diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c index c1c543535aaf..c7f8e37dba9b 100644 --- a/drivers/media/platform/qcom/venus/venc.c +++ b/drivers/media/platform/qcom/venus/venc.c @@ -1492,10 +1492,6 @@ static int venc_open(struct file *file) if (ret) goto err_free;
- ret = hfi_session_create(inst, &venc_hfi_ops); - if (ret) - goto err_ctrl_deinit; - venc_inst_init(inst);
/* @@ -1505,15 +1501,19 @@ static int venc_open(struct file *file) inst->m2m_dev = v4l2_m2m_init(&venc_m2m_ops); if (IS_ERR(inst->m2m_dev)) { ret = PTR_ERR(inst->m2m_dev); - goto err_session_destroy; + goto err_ctrl_deinit; }
inst->m2m_ctx = v4l2_m2m_ctx_init(inst->m2m_dev, inst, m2m_queue_init); if (IS_ERR(inst->m2m_ctx)) { ret = PTR_ERR(inst->m2m_ctx); - goto err_m2m_release; + goto err_m2m_dev_release; }
+ ret = hfi_session_create(inst, &venc_hfi_ops); + if (ret) + goto err_m2m_ctx_release; + v4l2_fh_init(&inst->fh, core->vdev_enc);
inst->fh.ctrl_handler = &inst->ctrl_handler; @@ -1523,10 +1523,10 @@ static int venc_open(struct file *file)
return 0;
-err_m2m_release: +err_m2m_ctx_release: + v4l2_m2m_ctx_release(inst->m2m_ctx); +err_m2m_dev_release: v4l2_m2m_release(inst->m2m_dev); -err_session_destroy: - hfi_session_destroy(inst); err_ctrl_deinit: v4l2_ctrl_handler_free(&inst->ctrl_handler); err_free: