Venus video encode/decode hardware driver consists of three modules.
The parent module venus-core, and two sub modules venus-enc and venus-dec.
The venus-core module allocates a common structure that is used by the
enc/dec modules, loads the firmware, and performs some common hardware
initialization. Since the three modules are loaded one after the other,
and their probe functions can run in parallel it is possible that
the venc_probe and vdec_probe functions can finish before the core
venus_probe function, which then can fail when, for example it
fails to load the firmware. In this case the subsequent call to venc_open
causes an Oops as it tries to dereference already uninitialized structures
through dev->parent and the system crashes in __pm_runtime_resume() as in
the trace below:
[ 26.064835][ T485] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[ 26.270914][ T485] Hardware name: Thundercomm Dragonboard 845c (DT)
[ 26.285019][ T485] pc : __pm_runtime_resume+0x34/0x178
[ 26.286374][ T213] lt9611 10-003b: hdmi cable connected
[ 26.290285][ T485] lr : venc_open+0xc0/0x278 [venus_enc]
[ 26.290326][ T485] Call trace:
[ 26.290328][ T485] __pm_runtime_resume+0x34/0x178
[ 26.290330][ T485] venc_open+0xc0/0x278 [venus_enc]
[ 26.290335][ T485] v4l2_open+0x184/0x294
[ 26.290340][ T485] chrdev_open+0x468/0x5c8
[ 26.290344][ T485] do_dentry_open+0x260/0x54c
[ 26.290349][ T485] path_openat+0xbe8/0xd5c
[ 26.290352][ T485] do_filp_open+0xb8/0x168
[ 26.290354][ T485] do_sys_openat2+0xa4/0x1e8
[ 26.290357][ T485] __arm64_compat_sys_openat+0x70/0x9c
[ 26.290359][ T485] invoke_syscall+0x60/0x170
[ 26.290363][ T485] el0_svc_common+0xb8/0xf8
[ 26.290365][ T485] do_el0_svc_compat+0x20/0x30
[ 26.290367][ T485] el0_svc_compat+0x24/0x84
[ 26.290372][ T485] el0t_32_sync_handler+0x7c/0xbc
[ 26.290374][ T485] el0t_32_sync+0x1b8/0x1bc
[ 26.290381][ T485] ---[ end trace 04ca7c088b4c1a9c ]---
[ 26.290383][ T485] Kernel panic - not syncing: Oops: Fatal exception
This can be fixed by synchronizing the three probe functions and
only allowing the venc_probe() and vdec_probe() to pass when venus_probe()
returns success.
Signed-off-by: Tadeusz Struk <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 6 ++++++
drivers/media/platform/qcom/venus/core.h | 2 ++
drivers/media/platform/qcom/venus/vdec.c | 18 +++++++++++++++---
drivers/media/platform/qcom/venus/venc.c | 18 +++++++++++++++---
4 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 91b15842c555..18f3e3a9823f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -317,6 +317,7 @@ static int venus_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&core->instances);
mutex_init(&core->lock);
+ mutex_init(&core->sync_lock);
INIT_DELAYED_WORK(&core->work, venus_sys_error_handler);
ret = devm_request_threaded_irq(dev, core->irq, hfi_isr, hfi_isr_thread,
@@ -331,6 +332,8 @@ static int venus_probe(struct platform_device *pdev)
venus_assign_register_offsets(core);
+ mutex_lock(&core->sync_lock);
+
ret = v4l2_device_register(dev, &core->v4l2_dev);
if (ret)
goto err_core_deinit;
@@ -377,6 +380,7 @@ static int venus_probe(struct platform_device *pdev)
goto err_dev_unregister;
}
+ mutex_unlock(&core->sync_lock);
venus_dbgfs_init(core);
return 0;
@@ -392,6 +396,7 @@ static int venus_probe(struct platform_device *pdev)
hfi_destroy(core);
err_core_deinit:
hfi_core_deinit(core, false);
+ mutex_unlock(&core->sync_lock);
err_core_put:
if (core->pm_ops->core_put)
core->pm_ops->core_put(core);
@@ -428,6 +433,7 @@ static int venus_remove(struct platform_device *pdev)
mutex_destroy(&core->pm_lock);
mutex_destroy(&core->lock);
+ mutex_destroy(&core->sync_lock);
venus_dbgfs_deinit(core);
return ret;
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 5ec851115eca..3f80dc26febb 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -119,6 +119,7 @@ struct venus_format {
* @use_tz: a flag that suggests presence of trustzone
* @fw: structure of firmware parameters
* @lock: a lock for this strucure
+ * @sync_lock a lock for probe sync between venus_core and venus_enc/dec
* @instances: a list_head of all instances
* @insts_count: num of instances
* @state: the state of the venus core
@@ -176,6 +177,7 @@ struct venus_core {
size_t mem_size;
} fw;
struct mutex lock;
+ struct mutex sync_lock;
struct list_head instances;
atomic_t insts_count;
unsigned int state;
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 198e47eb63f4..9dbda3d7a2d2 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1659,17 +1659,26 @@ static int vdec_probe(struct platform_device *pdev)
if (!core)
return -EPROBE_DEFER;
+ mutex_lock(&core->sync_lock);
+
+ if (core->state != CORE_INIT) {
+ ret = -ENODEV;
+ goto err_core_unlock;
+ }
+
platform_set_drvdata(pdev, core);
if (core->pm_ops->vdec_get) {
ret = core->pm_ops->vdec_get(dev);
if (ret)
- return ret;
+ goto err_core_unlock;
}
vdev = video_device_alloc();
- if (!vdev)
- return -ENOMEM;
+ if (!vdev) {
+ ret = -ENOMEM;
+ goto err_core_unlock;
+ }
strscpy(vdev->name, "qcom-venus-decoder", sizeof(vdev->name));
vdev->release = video_device_release;
@@ -1690,11 +1699,14 @@ static int vdec_probe(struct platform_device *pdev)
pm_runtime_set_autosuspend_delay(dev, 2000);
pm_runtime_use_autosuspend(dev);
pm_runtime_enable(dev);
+ mutex_unlock(&core->sync_lock);
return 0;
err_vdev_release:
video_device_release(vdev);
+err_core_unlock:
+ mutex_unlock(&core->sync_lock);
return ret;
}
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index bc1c42dd53c0..e7439236385a 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1338,17 +1338,26 @@ static int venc_probe(struct platform_device *pdev)
if (!core)
return -EPROBE_DEFER;
+ mutex_lock(&core->sync_lock);
+
+ if (core->state != CORE_INIT) {
+ ret = -ENODEV;
+ goto err_core_unlock;
+ }
+
platform_set_drvdata(pdev, core);
if (core->pm_ops->venc_get) {
ret = core->pm_ops->venc_get(dev);
if (ret)
- return ret;
+ goto err_core_unlock;
}
vdev = video_device_alloc();
- if (!vdev)
- return -ENOMEM;
+ if (!vdev) {
+ ret = -ENOMEM;
+ goto err_core_unlock;
+ }
strscpy(vdev->name, "qcom-venus-encoder", sizeof(vdev->name));
vdev->release = video_device_release;
@@ -1367,11 +1376,14 @@ static int venc_probe(struct platform_device *pdev)
video_set_drvdata(vdev, core);
pm_runtime_enable(dev);
+ mutex_unlock(&core->sync_lock);
return 0;
err_vdev_release:
video_device_release(vdev);
+err_core_unlock:
+ mutex_unlock(&core->sync_lock);
return ret;
}
--
2.31.1
On 25/10/2021 15:43, Tadeusz Struk wrote:
> Venus video encode/decode hardware driver consists of three modules.
> The parent module venus-core, and two sub modules venus-enc and venus-dec.
> The venus-core module allocates a common structure that is used by the
> enc/dec modules, loads the firmware, and performs some common hardware
> initialization. Since the three modules are loaded one after the other,
> and their probe functions can run in parallel it is possible that
> the venc_probe and vdec_probe functions can finish before the core
> venus_probe function, which then can fail when, for example it
> fails to load the firmware. In this case the subsequent call to venc_open
> causes an Oops as it tries to dereference already uninitialized structures
> through dev->parent and the system crashes in __pm_runtime_resume() as in
> the trace below:
>
> [ 26.064835][ T485] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> [ 26.270914][ T485] Hardware name: Thundercomm Dragonboard 845c (DT)
> [ 26.285019][ T485] pc : __pm_runtime_resume+0x34/0x178
> [ 26.286374][ T213] lt9611 10-003b: hdmi cable connected
> [ 26.290285][ T485] lr : venc_open+0xc0/0x278 [venus_enc]
> [ 26.290326][ T485] Call trace:
> [ 26.290328][ T485] __pm_runtime_resume+0x34/0x178
> [ 26.290330][ T485] venc_open+0xc0/0x278 [venus_enc]
> [ 26.290335][ T485] v4l2_open+0x184/0x294
> [ 26.290340][ T485] chrdev_open+0x468/0x5c8
> [ 26.290344][ T485] do_dentry_open+0x260/0x54c
> [ 26.290349][ T485] path_openat+0xbe8/0xd5c
> [ 26.290352][ T485] do_filp_open+0xb8/0x168
> [ 26.290354][ T485] do_sys_openat2+0xa4/0x1e8
> [ 26.290357][ T485] __arm64_compat_sys_openat+0x70/0x9c
> [ 26.290359][ T485] invoke_syscall+0x60/0x170
> [ 26.290363][ T485] el0_svc_common+0xb8/0xf8
> [ 26.290365][ T485] do_el0_svc_compat+0x20/0x30
> [ 26.290367][ T485] el0_svc_compat+0x24/0x84
> [ 26.290372][ T485] el0t_32_sync_handler+0x7c/0xbc
> [ 26.290374][ T485] el0t_32_sync+0x1b8/0x1bc
> [ 26.290381][ T485] ---[ end trace 04ca7c088b4c1a9c ]---
> [ 26.290383][ T485] Kernel panic - not syncing: Oops: Fatal exception
>
> This can be fixed by synchronizing the three probe functions and
> only allowing the venc_probe() and vdec_probe() to pass when venus_probe()
> returns success.
>
> Signed-off-by: Tadeusz Struk <[email protected]>
> ---
> + struct mutex sync_lock;
Why have an additional mutex, will the existing core::lock not do ?
> struct list_head instances;
> atomic_t insts_count;
> unsigned int state;
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 198e47eb63f4..9dbda3d7a2d2 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -1659,17 +1659,26 @@ static int vdec_probe(struct platform_device *pdev)
> if (!core)
> return -EPROBE_DEFER;
>
> + mutex_lock(&core->sync_lock);
> +
> + if (core->state != CORE_INIT) {
> + ret = -ENODEV;
> + goto err_core_unlock;
> + }
> +
> platform_set_drvdata(pdev, core);
>
> if (core->pm_ops->vdec_get) {
> ret = core->pm_ops->vdec_get(dev);
> if (ret)
> - return ret;
> + goto err_core_unlock;
> }
>
> vdev = video_device_alloc();
> - if (!vdev)
> - return -ENOMEM;
> + if (!vdev) {
> + ret = -ENOMEM;
> + goto err_core_unlock;
> + }
>
> strscpy(vdev->name, "qcom-venus-decoder", sizeof(vdev->name));
> vdev->release = video_device_release;
> @@ -1690,11 +1699,14 @@ static int vdec_probe(struct platform_device *pdev)
> pm_runtime_set_autosuspend_delay(dev, 2000);
> pm_runtime_use_autosuspend(dev);
> pm_runtime_enable(dev);
> + mutex_unlock(&core->sync_lock);
>
> return 0;
>
> err_vdev_release:
> video_device_release(vdev);
> +err_core_unlock:
> + mutex_unlock(&core->sync_lock);
> return ret;
> }
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index bc1c42dd53c0..e7439236385a 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -1338,17 +1338,26 @@ static int venc_probe(struct platform_device *pdev)
> if (!core)
> return -EPROBE_DEFER;
>
> + mutex_lock(&core->sync_lock);
> +
> + if (core->state != CORE_INIT) {
> + ret = -ENODEV;
> + goto err_core_unlock;
> + }
> +
shouldn't this be an -EPROBE_DEFER i.e. CORE_INIT hasn't completed/run
yet so defer until it does.
This fragment here looks racy to me without a DEFER above ?
drivers/media/platform/qcom/venus/core.c::venus_probe()
ret = v4l2_device_register(dev, &core->v4l2_dev);
if (ret)
goto err_core_deinit;
platform_set_drvdata(pdev, core);
pm_runtime_enable(dev);
ret = pm_runtime_get_sync(dev);
if (ret < 0)
goto err_runtime_disable;
ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
if (ret)
goto err_runtime_disable;
ret = venus_firmware_init(core);
if (ret)
goto err_runtime_disable;
ret = venus_boot(core);
if (ret)
goto err_runtime_disable;
ret = hfi_core_resume(core, true);
if (ret)
goto err_venus_shutdown;
ret = hfi_core_init(core);
if (ret)
goto err_venus_shutdown;
---
bod
Hi Bryan,
On 10/25/21 08:24, Bryan O'Donoghue wrote:
>
>> + struct mutex sync_lock;
>
> Why have an additional mutex, will the existing core::lock not do ?
I wanted to reuse it, but the core::lock in used internally in hfi and
it will deadlock there if we use that one here.
> shouldn't this be an -EPROBE_DEFER i.e. CORE_INIT hasn't completed/run yet so defer until
> it does.
>
> This fragment here looks racy to me without a DEFER above ?
>
> drivers/media/platform/qcom/venus/core.c::venus_probe()
No, we want a hard stop here. At this point the venus_core probe()
has finished and it failed. Returning -EPROBE_DEFER here will just
cause it to loop infinitely.
--
Thanks,
Tadeusz
On 25/10/2021 16:30, Tadeusz Struk wrote:
> No, we want a hard stop here. At this point the venus_core probe()
> has finished and it failed. Returning -EPROBE_DEFER here will just
> cause it to loop infinitely.
I don't think there's any guarantee at all, that core probe() has
completed at that point.
of_platform_populate() doesn't guarantee ordering of the probe()
completing before or after the probe() of the platform drivers that are
associated with the devices in of_platform_populate().
When you think it about it can't do that and you wouldn't want it to do
that since a device might have a legitimate reason to EPROBE_DEFER
As an example core could call of_platform_populate() and then as a
ridiculous example go to sleep for five seconds - in which case it is
perfectly possible the encoder and decoder probe() functions will bug
out illegitimately waiting because of core->state != CORE_INIT
This is a problem we have and still haven't solved in
drivers/usb/dwc3/dwc3-qcom.c::probe() and child devices
drivers/usb/dwc3/dwc3-qcom.c
Here:
https://patches.linaro.org/cover/470387/
There's no serialisation guarantee between parent and child on
of_platform_populate() - at least none I'm aware of.
---
bod
On 25/10/2021 17:03, Bryan O'Donoghue wrote:
> child devices drivers/usb/dwc3/dwc3-qcom.c
doh - learn to type
*child devices drivers/usb/dwc3/core.c
On 10/25/21 09:03, Bryan O'Donoghue wrote:
> I don't think there's any guarantee at all, that core probe() has completed at
> that point.
I think there is, thanks to the new sync_mutex. The enc/dec probe will keep
returning -EPROBE_DEFER; until the core driver calls
platform_set_drvdata(pdev, core); in line 338, but before it does that it
takes the syn_lock. Then both enc/dec drivers will block on the same sync_lock
until either the core has finished initialization fully and unlocks it in line
378 just before returning 0, or it fails in between and unlocks it on the err
path. Only then the other two can proceed and check if the core probe failed,
in which case the condition core->state != CORE_INIT will be true.
>
> of_platform_populate() doesn't guarantee ordering of the probe() completing
> before or after the probe() of the platform drivers that are associated with the
> devices in of_platform_populate().
agree, but I don't depend on of_platform_populate(). The ordering between the
three probe functions is enforced by the new sync mutex.
>
> When you think it about it can't do that and you wouldn't want it to do that
> since a device might have a legitimate reason to EPROBE_DEFER
>
> As an example core could call of_platform_populate() and then as a ridiculous
> example go to sleep for five seconds - in which case it is perfectly possible
and this is exactly what happens when the core probe() loads the firmware from
disk.
> the encoder and decoder probe() functions will bug out illegitimately waiting
> because of core->state != CORE_INIT
not really, because it will block on the mutex and only check the condition
after the sync_lock is unlocked.
--
Thanks,
Tadeusz
On 25/10/2021 17:57, Tadeusz Struk wrote:
> agree, but I don't depend on of_platform_populate(). The ordering
> between the
> three probe functions is enforced by the new sync mutex.
That explanation works for me.
Reviewed-by: Bryan O'Donoghue <[email protected]>
On 10/25/21 10:17, Bryan O'Donoghue wrote:
>> agree, but I don't depend on of_platform_populate(). The ordering between the
>> three probe functions is enforced by the new sync mutex.
>
> That explanation works for me.
>
> Reviewed-by: Bryan O'Donoghue <[email protected]>
Thanks Brian, I appreciate that.
On Mon, Oct 25, 2021 at 7:44 AM Tadeusz Struk <[email protected]> wrote:
>
> Venus video encode/decode hardware driver consists of three modules.
> The parent module venus-core, and two sub modules venus-enc and venus-dec.
> The venus-core module allocates a common structure that is used by the
> enc/dec modules, loads the firmware, and performs some common hardware
> initialization. Since the three modules are loaded one after the other,
> and their probe functions can run in parallel it is possible that
> the venc_probe and vdec_probe functions can finish before the core
> venus_probe function, which then can fail when, for example it
> fails to load the firmware. In this case the subsequent call to venc_open
> causes an Oops as it tries to dereference already uninitialized structures
> through dev->parent and the system crashes in __pm_runtime_resume() as in
> the trace below:
>
> [ 26.064835][ T485] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> [ 26.270914][ T485] Hardware name: Thundercomm Dragonboard 845c (DT)
> [ 26.285019][ T485] pc : __pm_runtime_resume+0x34/0x178
> [ 26.286374][ T213] lt9611 10-003b: hdmi cable connected
> [ 26.290285][ T485] lr : venc_open+0xc0/0x278 [venus_enc]
> [ 26.290326][ T485] Call trace:
> [ 26.290328][ T485] __pm_runtime_resume+0x34/0x178
> [ 26.290330][ T485] venc_open+0xc0/0x278 [venus_enc]
> [ 26.290335][ T485] v4l2_open+0x184/0x294
> [ 26.290340][ T485] chrdev_open+0x468/0x5c8
> [ 26.290344][ T485] do_dentry_open+0x260/0x54c
> [ 26.290349][ T485] path_openat+0xbe8/0xd5c
> [ 26.290352][ T485] do_filp_open+0xb8/0x168
> [ 26.290354][ T485] do_sys_openat2+0xa4/0x1e8
> [ 26.290357][ T485] __arm64_compat_sys_openat+0x70/0x9c
> [ 26.290359][ T485] invoke_syscall+0x60/0x170
> [ 26.290363][ T485] el0_svc_common+0xb8/0xf8
> [ 26.290365][ T485] do_el0_svc_compat+0x20/0x30
> [ 26.290367][ T485] el0_svc_compat+0x24/0x84
> [ 26.290372][ T485] el0t_32_sync_handler+0x7c/0xbc
> [ 26.290374][ T485] el0t_32_sync+0x1b8/0x1bc
> [ 26.290381][ T485] ---[ end trace 04ca7c088b4c1a9c ]---
> [ 26.290383][ T485] Kernel panic - not syncing: Oops: Fatal exception
>
> This can be fixed by synchronizing the three probe functions and
> only allowing the venc_probe() and vdec_probe() to pass when venus_probe()
> returns success.
>
> Signed-off-by: Tadeusz Struk <[email protected]>
Hey Tadeusz
Thanks so much for sending this out, I definitely would like to see
these crashes sorted!
Unfortunately this patch causes some odd behavior when I use it with a
modular config. The display does not start up and trying to reboot
the board ends up with it hanging instead of rebooting.
And booting with this patch in my non-modular config, it just seems to
get stuck during bootup (I suspect waiting on firmware that's not yet
mounted?).
I've got to run right now, but I'll try to help debug this down further.
thanks
-john
Hi John,
On 10/27/21 17:01, John Stultz wrote:
> Thanks so much for sending this out, I definitely would like to see
> these crashes sorted!
>
> Unfortunately this patch causes some odd behavior when I use it with a
> modular config. The display does not start up and trying to reboot
> the board ends up with it hanging instead of rebooting.
>
> And booting with this patch in my non-modular config, it just seems to
> get stuck during bootup (I suspect waiting on firmware that's not yet
> mounted?).
>
Thanks for trying the patch. With this patch I was able to boot android13
running 5.15.0-rc4-mainlineon on my Dragonboard 845c with the default
config common/build.config.db845c. Without it it was crashing.
It doesn't solve the firmware loading problem, it just makes it fail
gracefully for the boot to continue. If you share your config I can try
it and see what's wrong.
--
Thanks,
Tadeusz
On Wed, Oct 27, 2021 at 5:19 PM Tadeusz Struk <[email protected]> wrote:
>
> Hi John,
> On 10/27/21 17:01, John Stultz wrote:
> > Thanks so much for sending this out, I definitely would like to see
> > these crashes sorted!
> >
> > Unfortunately this patch causes some odd behavior when I use it with a
> > modular config. The display does not start up and trying to reboot
> > the board ends up with it hanging instead of rebooting.
> >
> > And booting with this patch in my non-modular config, it just seems to
> > get stuck during bootup (I suspect waiting on firmware that's not yet
> > mounted?).
> >
>
> Thanks for trying the patch. With this patch I was able to boot android13
> running 5.15.0-rc4-mainlineon on my Dragonboard 845c with the default
> config common/build.config.db845c. Without it it was crashing.
Hrm.. For my module enabled build I'm using the current
android-mainline as well w/ AOSP.
Still seeing some odd behavior, but I'm trying to isolate what change
in your patch is causing it (as it's not obvious).
thanks
-john