| Message ID | 6d463d21f0dd55c3d84db0458c7a5c4e0d7c5bc1.1619621413.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Address some issues with PM runtime at media subsystem | expand |
On Wed, 28 Apr 2021 16:51:22 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > The venus_sys_error_handler() assumes that pm_runtime was > able to resume, as it does things like: > > while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc)) > msleep(10); > > Well, if, for whatever reason, this won't happen, the routine > won't do what's expected. So, check for the returned error > condition, warning if it returns an error. > > Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions") > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > drivers/media/platform/qcom/venus/core.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index 54bac7ec14c5..c80c27c87ccc 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -84,7 +84,11 @@ static void venus_sys_error_handler(struct work_struct *work) > container_of(work, struct venus_core, work.work); > int ret = 0; > > - pm_runtime_get_sync(core->dev); > + ret = pm_runtime_get_sync(core->dev); > + if (WARN_ON(ret < 0)) { > + pm_runtime_put_noidle(core->dev); > + return; > + } > > hfi_core_deinit(core, true); > > @@ -106,9 +110,13 @@ static void venus_sys_error_handler(struct work_struct *work) > > hfi_reinit(core); > > - pm_runtime_get_sync(core->dev); > + ret = pm_runtime_get_sync(core->dev); > + if (WARN_ON(ret < 0)) { > + pm_runtime_put_noidle(core->dev); mutex_unlock(&core->lock); (the unlock is currently just below the enable_irq() in 5.12) > + return; > + } > > - ret |= venus_boot(core); > + ret = venus_boot(core); > ret |= hfi_core_resume(core, true); > > enable_irq(core->irq);
Em Fri, 30 Apr 2021 16:21:10 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu: > On Wed, 28 Apr 2021 16:51:22 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > The venus_sys_error_handler() assumes that pm_runtime was > > able to resume, as it does things like: > > > > while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc)) > > msleep(10); > > > > Well, if, for whatever reason, this won't happen, the routine > > won't do what's expected. So, check for the returned error > > condition, warning if it returns an error. > > > > Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions") > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > --- > > drivers/media/platform/qcom/venus/core.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > > index 54bac7ec14c5..c80c27c87ccc 100644 > > --- a/drivers/media/platform/qcom/venus/core.c > > +++ b/drivers/media/platform/qcom/venus/core.c > > @@ -84,7 +84,11 @@ static void venus_sys_error_handler(struct work_struct *work) > > container_of(work, struct venus_core, work.work); > > int ret = 0; > > > > - pm_runtime_get_sync(core->dev); > > + ret = pm_runtime_get_sync(core->dev); > > + if (WARN_ON(ret < 0)) { > > + pm_runtime_put_noidle(core->dev); > > + return; > > + } > > > > hfi_core_deinit(core, true); > > > > @@ -106,9 +110,13 @@ static void venus_sys_error_handler(struct work_struct *work) > > > > hfi_reinit(core); > > > > - pm_runtime_get_sync(core->dev); > > + ret = pm_runtime_get_sync(core->dev); > > + if (WARN_ON(ret < 0)) { > > + pm_runtime_put_noidle(core->dev); Thanks for review! > mutex_unlock(&core->lock); > (the unlock is currently just below the enable_irq() in 5.12) Basically, this function assumes that the core->lock is not locked and that IRQs are disabled, as can be seen at the function which starts such work: static void venus_event_notify(struct venus_core *core, u32 event) { struct venus_inst *inst; switch (event) { case EVT_SYS_WATCHDOG_TIMEOUT: case EVT_SYS_ERROR: break; default: return; } mutex_lock(&core->lock); core->sys_error = true; list_for_each_entry(inst, &core->instances, list) inst->ops->event_notify(inst, EVT_SESSION_ERROR, NULL); mutex_unlock(&core->lock); disable_irq_nosync(core->irq); schedule_delayed_work(&core->work, msecs_to_jiffies(10)); } The code inside it actually locks/unlocks two times the core->lock. See, this is the original code: static void venus_sys_error_handler(struct work_struct *work) { <not locked> pm_runtime_get_sync(core->dev); hfi_core_deinit(core, true); dev_warn(core->dev, "system error has occurred, starting recovery!\n"); mutex_lock(&core->lock); while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc)) msleep(10); ... enable_irq(core->irq); mutex_unlock(&core->lock); ... if (ret) { disable_irq_nosync(core->irq); dev_warn(core->dev, "recovery failed (%d)\n", ret); schedule_delayed_work(&core->work, msecs_to_jiffies(10)); return; } mutex_lock(&core->lock); core->sys_error = false; mutex_unlock(&core->lock); } It should be noticed that, once started, this delayed work re-starts itself, with IRQs disabled, trying to reboot the Venus IP hardware, until it stops failing [1]. [1] IMHO, it seems a very bad idea to keep running the work forever, flooding syslog with error messages on every 10ms or so. That's said, my patch doesn't seem to fix all potential issues that could happen there. I'll propose a separate fix, outside this patch series, as the issues here are not only due to RPM, but the main issue is that both while loops inside this code can run forever with the core->lock hold. Thanks, Mauro
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c index 54bac7ec14c5..c80c27c87ccc 100644 --- a/drivers/media/platform/qcom/venus/core.c +++ b/drivers/media/platform/qcom/venus/core.c @@ -84,7 +84,11 @@ static void venus_sys_error_handler(struct work_struct *work) container_of(work, struct venus_core, work.work); int ret = 0; - pm_runtime_get_sync(core->dev); + ret = pm_runtime_get_sync(core->dev); + if (WARN_ON(ret < 0)) { + pm_runtime_put_noidle(core->dev); + return; + } hfi_core_deinit(core, true); @@ -106,9 +110,13 @@ static void venus_sys_error_handler(struct work_struct *work) hfi_reinit(core); - pm_runtime_get_sync(core->dev); + ret = pm_runtime_get_sync(core->dev); + if (WARN_ON(ret < 0)) { + pm_runtime_put_noidle(core->dev); + return; + } - ret |= venus_boot(core); + ret = venus_boot(core); ret |= hfi_core_resume(core, true); enable_irq(core->irq);
The venus_sys_error_handler() assumes that pm_runtime was able to resume, as it does things like: while (pm_runtime_active(core->dev_dec) || pm_runtime_active(core->dev_enc)) msleep(10); Well, if, for whatever reason, this won't happen, the routine won't do what's expected. So, check for the returned error condition, warning if it returns an error. Fixes: af2c3834c8ca ("[media] media: venus: adding core part and helper functions") Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/platform/qcom/venus/core.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)