* [PATCH] media: venus: core, venc, vdec: Fix probe dependency error
@ 2021-02-04 12:56 Bryan O'Donoghue
2021-02-05 8:31 ` Stanimir Varbanov
0 siblings, 1 reply; 6+ messages in thread
From: Bryan O'Donoghue @ 2021-02-04 12:56 UTC (permalink / raw)
To: stanimir.varbanov, linux-media, linux-arm-msm
Cc: bryan.odonoghue, dmitry.baryshkov
Commit aaaa93eda64b ("media] media: venus: venc: add video encoder files")
is the last in a series of three commits to add core.c vdec.c and venc.c
adding core, encoder and decoder.
The encoder and decoder check for core drvdata as set and return -EPROBE_DEFER
if it has not been set, however both the encoder and decoder rely on
core.v4l2_dev as valid.
core.v4l2_dev will not be valid until v4l2_device_register() has completed
in core.c's probe().
Normally this is never seen however, Dmitry reported the following
backtrace when compiling drivers and firmware directly into a kernel image.
[ 5.259968] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[ 5.269850] sd 0:0:0:3: [sdd] Optimal transfer size 524288 bytes
[ 5.275505] Workqueue: events deferred_probe_work_func
[ 5.275513] pstate: 60400005 (nZCv daif +PAN -UAO -TCO BTYPE=--)
[ 5.441211] usb 2-1: new SuperSpeedPlus Gen 2 USB device number 2 using xhci-hcd
[ 5.442486] pc : refcount_warn_saturate+0x140/0x148
[ 5.493756] hub 2-1:1.0: USB hub found
[ 5.496266] lr : refcount_warn_saturate+0x140/0x148
[ 5.500982] hub 2-1:1.0: 4 ports detected
[ 5.503440] sp : ffff80001067b730
[ 5.503442] x29: ffff80001067b730
[ 5.592660] usb 1-1: new high-speed USB device number 2 using xhci-hcd
[ 5.598478] x28: ffff6c6bc1c379b8
[ 5.598480] x27: ffffa5c673852960 x26: ffffa5c673852000
[ 5.598484] x25: ffff6c6bc1c37800 x24: 0000000000000001
[ 5.810652] x23: 0000000000000000 x22: ffffa5c673bc7118
[ 5.813777] hub 1-1:1.0: USB hub found
[ 5.816108] x21: ffffa5c674440000 x20: 0000000000000001
[ 5.820846] hub 1-1:1.0: 4 ports detected
[ 5.825415] x19: ffffa5c6744f4000 x18: ffffffffffffffff
[ 5.825418] x17: 0000000000000000 x16: 0000000000000000
[ 5.825421] x15: 00000a4810c193ba x14: 0000000000000000
[ 5.825424] x13: 00000000000002b8 x12: 000000000000f20a
[ 5.825427] x11: 000000000000f20a x10: 0000000000000038
[ 5.845447] usb 2-1.1: new SuperSpeed Gen 1 USB device number 3 using xhci-hcd
[ 5.845904]
[ 5.845905] x9 : 0000000000000000 x8 : ffff6c6d36fae780
[ 5.871208] x7 : ffff6c6d36faf240 x6 : 0000000000000000
[ 5.876664] x5 : 0000000000000004 x4 : 0000000000000085
[ 5.882121] x3 : 0000000000000119 x2 : ffffa5c6741ef478
[ 5.887578] x1 : 3acbb3926faf5f00 x0 : 0000000000000000
[ 5.893036] Call trace:
[ 5.895551] refcount_warn_saturate+0x140/0x148
[ 5.900202] __video_register_device+0x64c/0xd10
[ 5.904944] venc_probe+0xc4/0x148
[ 5.908444] platform_probe+0x68/0xe0
[ 5.912210] really_probe+0x118/0x3e0
[ 5.915977] driver_probe_device+0x5c/0xc0
[ 5.920187] __device_attach_driver+0x98/0xb8
[ 5.924661] bus_for_each_drv+0x68/0xd0
[ 5.928604] __device_attach+0xec/0x148
[ 5.932547] device_initial_probe+0x14/0x20
[ 5.936845] bus_probe_device+0x9c/0xa8
[ 5.940788] device_add+0x3e8/0x7c8
[ 5.944376] of_device_add+0x4c/0x60
[ 5.948056] of_platform_device_create_pdata+0xbc/0x140
[ 5.953425] of_platform_bus_create+0x17c/0x3c0
[ 5.958078] of_platform_populate+0x80/0x110
[ 5.962463] venus_probe+0x2ec/0x4d8
[ 5.966143] platform_probe+0x68/0xe0
[ 5.969907] really_probe+0x118/0x3e0
[ 5.973674] driver_probe_device+0x5c/0xc0
[ 5.977882] __device_attach_driver+0x98/0xb8
[ 5.982356] bus_for_each_drv+0x68/0xd0
[ 5.986298] __device_attach+0xec/0x148
[ 5.990242] device_initial_probe+0x14/0x20
[ 5.994539] bus_probe_device+0x9c/0xa8
[ 5.998481] deferred_probe_work_func+0x74/0xb0
[ 6.003132] process_one_work+0x1e8/0x360
[ 6.007254] worker_thread+0x208/0x478
[ 6.011106] kthread+0x150/0x158
[ 6.014431] ret_from_fork+0x10/0x30
[ 6.018111] ---[ end trace f074246b1ecdb466 ]---
This patch fixes by
- Making core.v4l2_dev into core->v4l_dev
- Only setting core->v4l2_dev when v4l2_device_register() completes
- Deferring encoder/decoder probe until core->v4l2_dev is set
Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Fixes: aaaa93eda64b ("media] media: venus: venc: add video encoder files")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/media/platform/qcom/venus/core.c | 14 +++++++++++---
drivers/media/platform/qcom/venus/core.h | 2 +-
drivers/media/platform/qcom/venus/vdec.c | 4 ++--
drivers/media/platform/qcom/venus/venc.c | 4 ++--
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 07faed26c477..b4f1ae6ca146 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -231,6 +231,7 @@ static void venus_assign_register_offsets(struct venus_core *core)
static int venus_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct v4l2_device *v4l2_dev;
struct venus_core *core;
struct resource *r;
int ret;
@@ -239,6 +240,10 @@ static int venus_probe(struct platform_device *pdev)
if (!core)
return -ENOMEM;
+ v4l2_dev = devm_kzalloc(dev, sizeof(*v4l2_dev), GFP_KERNEL);
+ if (!v4l2_dev)
+ return -ENOMEM;
+
core->dev = dev;
platform_set_drvdata(pdev, core);
@@ -331,10 +336,12 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
goto err_venus_shutdown;
- ret = v4l2_device_register(dev, &core->v4l2_dev);
+ ret = v4l2_device_register(dev, v4l2_dev);
if (ret)
goto err_core_deinit;
+ core->v4l2_dev = v4l2_dev;
+
ret = pm_runtime_put_sync(dev);
if (ret) {
pm_runtime_get_noresume(dev);
@@ -346,7 +353,7 @@ static int venus_probe(struct platform_device *pdev)
return 0;
err_dev_unregister:
- v4l2_device_unregister(&core->v4l2_dev);
+ v4l2_device_unregister(core->v4l2_dev);
err_core_deinit:
hfi_core_deinit(core, false);
err_venus_shutdown:
@@ -391,7 +398,8 @@ static int venus_remove(struct platform_device *pdev)
icc_put(core->video_path);
icc_put(core->cpucfg_path);
- v4l2_device_unregister(&core->v4l2_dev);
+ v4l2_device_unregister(core->v4l2_dev);
+
mutex_destroy(&core->pm_lock);
mutex_destroy(&core->lock);
venus_dbgfs_deinit(core);
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 8328f7210d6c..baf4d01c18c9 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -150,7 +150,7 @@ struct venus_core {
struct reset_control *resets[VIDC_RESETS_NUM_MAX];
struct video_device *vdev_dec;
struct video_device *vdev_enc;
- struct v4l2_device v4l2_dev;
+ struct v4l2_device *v4l2_dev;
const struct venus_resources *res;
struct device *dev;
struct device *dev_dec;
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 9fbff40c4568..e5c81b59965b 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1636,7 +1636,7 @@ static int vdec_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
core = dev_get_drvdata(dev->parent);
- if (!core)
+ if (!core || !core->v4l2_dev)
return -EPROBE_DEFER;
platform_set_drvdata(pdev, core);
@@ -1656,7 +1656,7 @@ static int vdec_probe(struct platform_device *pdev)
vdev->fops = &vdec_fops;
vdev->ioctl_ops = &vdec_ioctl_ops;
vdev->vfl_dir = VFL_DIR_M2M;
- vdev->v4l2_dev = &core->v4l2_dev;
+ vdev->v4l2_dev = core->v4l2_dev;
vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 4b5ab0c790c9..ff2bf11bd89e 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1242,7 +1242,7 @@ static int venc_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
core = dev_get_drvdata(dev->parent);
- if (!core)
+ if (!core || !core->v4l2_dev)
return -EPROBE_DEFER;
platform_set_drvdata(pdev, core);
@@ -1262,7 +1262,7 @@ static int venc_probe(struct platform_device *pdev)
vdev->fops = &venc_fops;
vdev->ioctl_ops = &venc_ioctl_ops;
vdev->vfl_dir = VFL_DIR_M2M;
- vdev->v4l2_dev = &core->v4l2_dev;
+ vdev->v4l2_dev = core->v4l2_dev;
vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] media: venus: core, venc, vdec: Fix probe dependency error
2021-02-04 12:56 [PATCH] media: venus: core, venc, vdec: Fix probe dependency error Bryan O'Donoghue
@ 2021-02-05 8:31 ` Stanimir Varbanov
2021-02-05 11:24 ` Bryan O'Donoghue
0 siblings, 1 reply; 6+ messages in thread
From: Stanimir Varbanov @ 2021-02-05 8:31 UTC (permalink / raw)
To: Bryan O'Donoghue, linux-media, linux-arm-msm; +Cc: dmitry.baryshkov
Hi Bryan,
Thanks for the fix!
On 2/4/21 2:56 PM, Bryan O'Donoghue wrote:
> Commit aaaa93eda64b ("media] media: venus: venc: add video encoder files")
> is the last in a series of three commits to add core.c vdec.c and venc.c
> adding core, encoder and decoder.
>
> The encoder and decoder check for core drvdata as set and return -EPROBE_DEFER
> if it has not been set, however both the encoder and decoder rely on
> core.v4l2_dev as valid.
>
> core.v4l2_dev will not be valid until v4l2_device_register() has completed
> in core.c's probe().
>
> Normally this is never seen however, Dmitry reported the following
> backtrace when compiling drivers and firmware directly into a kernel image.
>
> [ 5.259968] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
> [ 5.269850] sd 0:0:0:3: [sdd] Optimal transfer size 524288 bytes
> [ 5.275505] Workqueue: events deferred_probe_work_func
> [ 5.275513] pstate: 60400005 (nZCv daif +PAN -UAO -TCO BTYPE=--)
> [ 5.441211] usb 2-1: new SuperSpeedPlus Gen 2 USB device number 2 using xhci-hcd
> [ 5.442486] pc : refcount_warn_saturate+0x140/0x148
> [ 5.493756] hub 2-1:1.0: USB hub found
> [ 5.496266] lr : refcount_warn_saturate+0x140/0x148
> [ 5.500982] hub 2-1:1.0: 4 ports detected
> [ 5.503440] sp : ffff80001067b730
> [ 5.503442] x29: ffff80001067b730
> [ 5.592660] usb 1-1: new high-speed USB device number 2 using xhci-hcd
> [ 5.598478] x28: ffff6c6bc1c379b8
> [ 5.598480] x27: ffffa5c673852960 x26: ffffa5c673852000
> [ 5.598484] x25: ffff6c6bc1c37800 x24: 0000000000000001
> [ 5.810652] x23: 0000000000000000 x22: ffffa5c673bc7118
> [ 5.813777] hub 1-1:1.0: USB hub found
> [ 5.816108] x21: ffffa5c674440000 x20: 0000000000000001
> [ 5.820846] hub 1-1:1.0: 4 ports detected
> [ 5.825415] x19: ffffa5c6744f4000 x18: ffffffffffffffff
> [ 5.825418] x17: 0000000000000000 x16: 0000000000000000
> [ 5.825421] x15: 00000a4810c193ba x14: 0000000000000000
> [ 5.825424] x13: 00000000000002b8 x12: 000000000000f20a
> [ 5.825427] x11: 000000000000f20a x10: 0000000000000038
> [ 5.845447] usb 2-1.1: new SuperSpeed Gen 1 USB device number 3 using xhci-hcd
> [ 5.845904]
> [ 5.845905] x9 : 0000000000000000 x8 : ffff6c6d36fae780
> [ 5.871208] x7 : ffff6c6d36faf240 x6 : 0000000000000000
> [ 5.876664] x5 : 0000000000000004 x4 : 0000000000000085
> [ 5.882121] x3 : 0000000000000119 x2 : ffffa5c6741ef478
> [ 5.887578] x1 : 3acbb3926faf5f00 x0 : 0000000000000000
> [ 5.893036] Call trace:
> [ 5.895551] refcount_warn_saturate+0x140/0x148
> [ 5.900202] __video_register_device+0x64c/0xd10
> [ 5.904944] venc_probe+0xc4/0x148
> [ 5.908444] platform_probe+0x68/0xe0
> [ 5.912210] really_probe+0x118/0x3e0
> [ 5.915977] driver_probe_device+0x5c/0xc0
> [ 5.920187] __device_attach_driver+0x98/0xb8
> [ 5.924661] bus_for_each_drv+0x68/0xd0
> [ 5.928604] __device_attach+0xec/0x148
> [ 5.932547] device_initial_probe+0x14/0x20
> [ 5.936845] bus_probe_device+0x9c/0xa8
> [ 5.940788] device_add+0x3e8/0x7c8
> [ 5.944376] of_device_add+0x4c/0x60
> [ 5.948056] of_platform_device_create_pdata+0xbc/0x140
> [ 5.953425] of_platform_bus_create+0x17c/0x3c0
> [ 5.958078] of_platform_populate+0x80/0x110
> [ 5.962463] venus_probe+0x2ec/0x4d8
> [ 5.966143] platform_probe+0x68/0xe0
> [ 5.969907] really_probe+0x118/0x3e0
> [ 5.973674] driver_probe_device+0x5c/0xc0
> [ 5.977882] __device_attach_driver+0x98/0xb8
> [ 5.982356] bus_for_each_drv+0x68/0xd0
> [ 5.986298] __device_attach+0xec/0x148
> [ 5.990242] device_initial_probe+0x14/0x20
> [ 5.994539] bus_probe_device+0x9c/0xa8
> [ 5.998481] deferred_probe_work_func+0x74/0xb0
> [ 6.003132] process_one_work+0x1e8/0x360
> [ 6.007254] worker_thread+0x208/0x478
> [ 6.011106] kthread+0x150/0x158
> [ 6.014431] ret_from_fork+0x10/0x30
> [ 6.018111] ---[ end trace f074246b1ecdb466 ]---
>
> This patch fixes by
>
> - Making core.v4l2_dev into core->v4l_dev
> - Only setting core->v4l2_dev when v4l2_device_register() completes
> - Deferring encoder/decoder probe until core->v4l2_dev is set
Why not just move platform_set_drvdata(pdev, core) at the end of
venus_probe() after we registered v4l2_dev? I think this way we will
avoid this v4l2_dev gymnastics.
>
> Reported-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Fixes: aaaa93eda64b ("media] media: venus: venc: add video encoder files")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
> drivers/media/platform/qcom/venus/core.c | 14 +++++++++++---
> drivers/media/platform/qcom/venus/core.h | 2 +-
> drivers/media/platform/qcom/venus/vdec.c | 4 ++--
> drivers/media/platform/qcom/venus/venc.c | 4 ++--
> 4 files changed, 16 insertions(+), 8 deletions(-)
>
--
regards,
Stan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: venus: core, venc, vdec: Fix probe dependency error
2021-02-05 8:31 ` Stanimir Varbanov
@ 2021-02-05 11:24 ` Bryan O'Donoghue
2021-02-05 11:37 ` Stanimir Varbanov
0 siblings, 1 reply; 6+ messages in thread
From: Bryan O'Donoghue @ 2021-02-05 11:24 UTC (permalink / raw)
To: Stanimir Varbanov, linux-media, linux-arm-msm; +Cc: dmitry.baryshkov
On 05/02/2021 08:31, Stanimir Varbanov wrote:
> Why not just move platform_set_drvdata(pdev, core) at the end of
> venus_probe() after we registered v4l2_dev? I think this way we will
> avoid this v4l2_dev gymnastics.
Because pm_ops->core_functionname() relies on getdrvdata.
I changed that in a version of this fix which I didn't publish but also
found that there were other dependencies in core::probe venc::probe and
vdec::probe on drvdata.
I can publish a fix for drvdata and you can take your preference
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: venus: core, venc, vdec: Fix probe dependency error
2021-02-05 11:24 ` Bryan O'Donoghue
@ 2021-02-05 11:37 ` Stanimir Varbanov
2021-02-05 11:52 ` Stanimir Varbanov
0 siblings, 1 reply; 6+ messages in thread
From: Stanimir Varbanov @ 2021-02-05 11:37 UTC (permalink / raw)
To: Bryan O'Donoghue, linux-media, linux-arm-msm; +Cc: dmitry.baryshkov
On 2/5/21 1:24 PM, Bryan O'Donoghue wrote:
> On 05/02/2021 08:31, Stanimir Varbanov wrote:
>> Why not just move platform_set_drvdata(pdev, core) at the end of
>> venus_probe() after we registered v4l2_dev? I think this way we will
>> avoid this v4l2_dev gymnastics.
>
> Because pm_ops->core_functionname() relies on getdrvdata.
>
> I changed that in a version of this fix which I didn't publish but also
> found that there were other dependencies in core::probe venc::probe and
> vdec::probe on drvdata.
>
> I can publish a fix for drvdata and you can take your preference
I'd prefer this solution. Do you see a problem if we change
core_get|put|power functional pointers from pm_ops to receive a
venus_core pointer instead of core->dev?
--
regards,
Stan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: venus: core, venc, vdec: Fix probe dependency error
2021-02-05 11:37 ` Stanimir Varbanov
@ 2021-02-05 11:52 ` Stanimir Varbanov
2021-02-05 12:16 ` Bryan O'Donoghue
0 siblings, 1 reply; 6+ messages in thread
From: Stanimir Varbanov @ 2021-02-05 11:52 UTC (permalink / raw)
To: Bryan O'Donoghue, linux-media, linux-arm-msm; +Cc: dmitry.baryshkov
On 2/5/21 1:37 PM, Stanimir Varbanov wrote:
>
>
> On 2/5/21 1:24 PM, Bryan O'Donoghue wrote:
>> On 05/02/2021 08:31, Stanimir Varbanov wrote:
>>> Why not just move platform_set_drvdata(pdev, core) at the end of
>>> venus_probe() after we registered v4l2_dev? I think this way we will
>>> avoid this v4l2_dev gymnastics.
>>
>> Because pm_ops->core_functionname() relies on getdrvdata.
>>
>> I changed that in a version of this fix which I didn't publish but also
>> found that there were other dependencies in core::probe venc::probe and
>> vdec::probe on drvdata.
>>
>> I can publish a fix for drvdata and you can take your preference
>
> I'd prefer this solution. Do you see a problem if we change
> core_get|put|power functional pointers from pm_ops to receive a
> venus_core pointer instead of core->dev?
>
>
Wait, venus_runtime_suspend|resume also relies on that
dev_get_drvdata(). Can we call v4l2_device_register() earlier in
venus_probe?
--
regards,
Stan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: venus: core, venc, vdec: Fix probe dependency error
2021-02-05 11:52 ` Stanimir Varbanov
@ 2021-02-05 12:16 ` Bryan O'Donoghue
0 siblings, 0 replies; 6+ messages in thread
From: Bryan O'Donoghue @ 2021-02-05 12:16 UTC (permalink / raw)
To: Stanimir Varbanov, linux-media, linux-arm-msm; +Cc: dmitry.baryshkov
On 05/02/2021 11:52, Stanimir Varbanov wrote:
> Wait, venus_runtime_suspend|resume also relies on that
> dev_get_drvdata(). Can we call v4l2_device_register() earlier in
> venus_probe?
I can give it a go
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-05 12:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-04 12:56 [PATCH] media: venus: core, venc, vdec: Fix probe dependency error Bryan O'Donoghue
2021-02-05 8:31 ` Stanimir Varbanov
2021-02-05 11:24 ` Bryan O'Donoghue
2021-02-05 11:37 ` Stanimir Varbanov
2021-02-05 11:52 ` Stanimir Varbanov
2021-02-05 12:16 ` Bryan O'Donoghue
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.