| Message ID | 20241224072444.2044956-2-senozhatsky@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: venus: partial revert of 45b1a1b348ec1 | expand |
Hi Sergey, On 24/12/2024 08:24, 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). > > Fixes: 45b1a1b348ec1 ("media: venus: sync with threaded IRQ during inst destruction") > Cc: stable@vger.kernel.org > Reported-by: Nathan Hebert <nhebert@google.com> Do you have a link to Nathan's report so I can add a 'Closes' tag afterwards? Regards, Hans > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > drivers/media/platform/qcom/venus/core.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index 2d27c5167246..807487a1f536 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -506,18 +506,14 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev) > void venus_close_common(struct venus_inst *inst) > { > /* > - * First, remove the inst from the ->instances list, so that > - * to_instance() will return NULL. > - */ > - hfi_session_destroy(inst); > - /* > - * Second, make sure we don't have IRQ/IRQ-thread currently running > + * Make sure we don't have IRQ/IRQ-thread currently running > * or pending execution, which would race with the inst destruction. > */ > synchronize_irq(inst->core->irq); > > v4l2_m2m_ctx_release(inst->m2m_ctx); > v4l2_m2m_release(inst->m2m_dev); > + hfi_session_destroy(inst); > v4l2_fh_del(&inst->fh); > v4l2_fh_exit(&inst->fh); > v4l2_ctrl_handler_free(&inst->ctrl_handler);
Hi Hans, On (25/01/06 14:15), Hans Verkuil wrote: > Hi Sergey, > > On 24/12/2024 08:24, 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). > > > > Fixes: 45b1a1b348ec1 ("media: venus: sync with threaded IRQ during inst destruction") > > Cc: stable@vger.kernel.org > > Reported-by: Nathan Hebert <nhebert@google.com> > > Do you have a link to Nathan's report so I can add a 'Closes' tag > afterwards? No public link is available as the report was internal.
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c index 2d27c5167246..807487a1f536 100644 --- a/drivers/media/platform/qcom/venus/core.c +++ b/drivers/media/platform/qcom/venus/core.c @@ -506,18 +506,14 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev) void venus_close_common(struct venus_inst *inst) { /* - * First, remove the inst from the ->instances list, so that - * to_instance() will return NULL. - */ - hfi_session_destroy(inst); - /* - * Second, make sure we don't have IRQ/IRQ-thread currently running + * Make sure we don't have IRQ/IRQ-thread currently running * or pending execution, which would race with the inst destruction. */ synchronize_irq(inst->core->irq); v4l2_m2m_ctx_release(inst->m2m_ctx); v4l2_m2m_release(inst->m2m_dev); + hfi_session_destroy(inst); v4l2_fh_del(&inst->fh); v4l2_fh_exit(&inst->fh); v4l2_ctrl_handler_free(&inst->ctrl_handler);
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). Fixes: 45b1a1b348ec1 ("media: venus: sync with threaded IRQ during inst destruction") Cc: stable@vger.kernel.org Reported-by: Nathan Hebert <nhebert@google.com> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- drivers/media/platform/qcom/venus/core.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)