* [PATCH] drm: zynqmp_dpsub: Always register bridge
@ 2024-03-08 20:47 Sean Anderson
2024-03-22 6:01 ` Tomi Valkeinen
0 siblings, 1 reply; 6+ messages in thread
From: Sean Anderson @ 2024-03-08 20:47 UTC (permalink / raw
To: Laurent Pinchart, dri-devel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
Cc: linux-arm-kernel, Michal Simek, linux-kernel, Daniel Vetter,
David Airlie, Sean Anderson
We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
initialized. We do this before zynqmp_dpsub_drm_init since that calls
drm_bridge_attach. This fixes the following lockdep warning:
[ 19.217084] ------------[ cut here ]------------
[ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582 __mutex_lock+0x4bc/0x550
[ 19.241696] Modules linked in:
[ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
[ 19.252046] Hardware name: xlnx,zynqmp (DT)
[ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
[ 19.261795] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 19.269104] pc : __mutex_lock+0x4bc/0x550
[ 19.273364] lr : __mutex_lock+0x4bc/0x550
[ 19.277592] sp : ffffffc085c5bbe0
[ 19.281066] x29: ffffffc085c5bbe0 x28: 0000000000000000 x27: ffffff88009417f8
[ 19.288624] x26: ffffff8800941788 x25: ffffff8800020008 x24: ffffffc082aa3000
[ 19.296227] x23: ffffffc080d90e3c x22: 0000000000000002 x21: 0000000000000000
[ 19.303744] x20: 0000000000000000 x19: ffffff88002f5210 x18: 0000000000000000
[ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
[ 19.318922] x14: 0000000000000000 x13: 284e4f5f4e524157 x12: 0000000000000001
[ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
[ 19.334003] x8 : 0001ff88003f3888 x7 : 0000000000000000 x6 : 0000000000000000
[ 19.341537] x5 : 0000000000000000 x4 : 0000000000001668 x3 : 0000000000000000
[ 19.349054] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff88003f3880
[ 19.356581] Call trace:
[ 19.359160] __mutex_lock+0x4bc/0x550
[ 19.363032] mutex_lock_nested+0x24/0x30
[ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
[ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
[ 19.376364] process_one_work+0x3ac/0x988
[ 19.380660] worker_thread+0x398/0x694
[ 19.384736] kthread+0x1bc/0x1c0
[ 19.388241] ret_from_fork+0x10/0x20
[ 19.392031] irq event stamp: 183
[ 19.395450] hardirqs last enabled at (183): [<ffffffc0800b9278>] finish_task_switch.isra.0+0xa8/0x2d4
[ 19.405140] hardirqs last disabled at (182): [<ffffffc081ad3754>] __schedule+0x714/0xd04
[ 19.413612] softirqs last enabled at (114): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
[ 19.423128] softirqs last disabled at (110): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
[ 19.432614] ---[ end trace 0000000000000000 ]---
Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..639fff2c693f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
if (ret)
goto err_dp;
+ drm_bridge_add(dpsub->bridge);
if (dpsub->dma_enabled) {
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
+ drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
--
2.35.1.1320.gc452695387.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
2024-03-08 20:47 [PATCH] drm: zynqmp_dpsub: Always register bridge Sean Anderson
@ 2024-03-22 6:01 ` Tomi Valkeinen
2024-04-23 20:50 ` Sean Anderson
2024-04-26 9:30 ` Laurent Pinchart
0 siblings, 2 replies; 6+ messages in thread
From: Tomi Valkeinen @ 2024-03-22 6:01 UTC (permalink / raw
To: Sean Anderson
Cc: linux-arm-kernel, Michal Simek, linux-kernel, Daniel Vetter,
David Airlie, Laurent Pinchart, dri-devel, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann
Hi,
On 08/03/2024 22:47, Sean Anderson wrote:
> We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
> calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
> initialized. We do this before zynqmp_dpsub_drm_init since that calls
> drm_bridge_attach. This fixes the following lockdep warning:
>
> [ 19.217084] ------------[ cut here ]------------
> [ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> [ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582 __mutex_lock+0x4bc/0x550
> [ 19.241696] Modules linked in:
> [ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
> [ 19.252046] Hardware name: xlnx,zynqmp (DT)
> [ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
> [ 19.261795] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 19.269104] pc : __mutex_lock+0x4bc/0x550
> [ 19.273364] lr : __mutex_lock+0x4bc/0x550
> [ 19.277592] sp : ffffffc085c5bbe0
> [ 19.281066] x29: ffffffc085c5bbe0 x28: 0000000000000000 x27: ffffff88009417f8
> [ 19.288624] x26: ffffff8800941788 x25: ffffff8800020008 x24: ffffffc082aa3000
> [ 19.296227] x23: ffffffc080d90e3c x22: 0000000000000002 x21: 0000000000000000
> [ 19.303744] x20: 0000000000000000 x19: ffffff88002f5210 x18: 0000000000000000
> [ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
> [ 19.318922] x14: 0000000000000000 x13: 284e4f5f4e524157 x12: 0000000000000001
> [ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
> [ 19.334003] x8 : 0001ff88003f3888 x7 : 0000000000000000 x6 : 0000000000000000
> [ 19.341537] x5 : 0000000000000000 x4 : 0000000000001668 x3 : 0000000000000000
> [ 19.349054] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff88003f3880
> [ 19.356581] Call trace:
> [ 19.359160] __mutex_lock+0x4bc/0x550
> [ 19.363032] mutex_lock_nested+0x24/0x30
> [ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
> [ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
> [ 19.376364] process_one_work+0x3ac/0x988
> [ 19.380660] worker_thread+0x398/0x694
> [ 19.384736] kthread+0x1bc/0x1c0
> [ 19.388241] ret_from_fork+0x10/0x20
> [ 19.392031] irq event stamp: 183
> [ 19.395450] hardirqs last enabled at (183): [<ffffffc0800b9278>] finish_task_switch.isra.0+0xa8/0x2d4
> [ 19.405140] hardirqs last disabled at (182): [<ffffffc081ad3754>] __schedule+0x714/0xd04
> [ 19.413612] softirqs last enabled at (114): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
> [ 19.423128] softirqs last disabled at (110): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
> [ 19.432614] ---[ end trace 0000000000000000 ]---
>
> Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>
> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index 88eb33acd5f0..639fff2c693f 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
> if (ret)
> goto err_dp;
>
> + drm_bridge_add(dpsub->bridge);
> if (dpsub->dma_enabled) {
> ret = zynqmp_dpsub_drm_init(dpsub);
> if (ret)
> goto err_disp;
> - } else {
> - drm_bridge_add(dpsub->bridge);
> }
>
> dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
> @@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev)
>
> if (dpsub->drm)
> zynqmp_dpsub_drm_cleanup(dpsub);
> - else
> - drm_bridge_remove(dpsub->bridge);
>
> + drm_bridge_remove(dpsub->bridge);
> zynqmp_disp_remove(dpsub);
> zynqmp_dp_remove(dpsub);
>
I sent a similar patch:
https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03bac@ideasonboard.com/
I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where
the bridge is set up, so it felt like a logical place. You add it later,
just before the bridge is used the first time.
I like mine a bit more as it has all the bridge code in the same place,
but I also wonder if there might be some risks in adding the bridge
early (before zynqmp_disp_probe()), although I can't see any issue right
away...
In any case, as this works for me too:
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tomi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
2024-03-22 6:01 ` Tomi Valkeinen
@ 2024-04-23 20:50 ` Sean Anderson
2024-04-24 12:17 ` Tomi Valkeinen
2024-04-26 9:30 ` Laurent Pinchart
1 sibling, 1 reply; 6+ messages in thread
From: Sean Anderson @ 2024-04-23 20:50 UTC (permalink / raw
To: Tomi Valkeinen
Cc: linux-arm-kernel, Michal Simek, linux-kernel, Daniel Vetter,
David Airlie, Laurent Pinchart, dri-devel, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann
Hi,
On 3/22/24 02:01, Tomi Valkeinen wrote:
> Hi,
>
> On 08/03/2024 22:47, Sean Anderson wrote:
>> We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
>> calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
>> initialized. We do this before zynqmp_dpsub_drm_init since that calls
>> drm_bridge_attach. This fixes the following lockdep warning:
>>
>> [ 19.217084] ------------[ cut here ]------------
>> [ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>> [ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582 __mutex_lock+0x4bc/0x550
>> [ 19.241696] Modules linked in:
>> [ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
>> [ 19.252046] Hardware name: xlnx,zynqmp (DT)
>> [ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
>> [ 19.261795] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [ 19.269104] pc : __mutex_lock+0x4bc/0x550
>> [ 19.273364] lr : __mutex_lock+0x4bc/0x550
>> [ 19.277592] sp : ffffffc085c5bbe0
>> [ 19.281066] x29: ffffffc085c5bbe0 x28: 0000000000000000 x27: ffffff88009417f8
>> [ 19.288624] x26: ffffff8800941788 x25: ffffff8800020008 x24: ffffffc082aa3000
>> [ 19.296227] x23: ffffffc080d90e3c x22: 0000000000000002 x21: 0000000000000000
>> [ 19.303744] x20: 0000000000000000 x19: ffffff88002f5210 x18: 0000000000000000
>> [ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
>> [ 19.318922] x14: 0000000000000000 x13: 284e4f5f4e524157 x12: 0000000000000001
>> [ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
>> [ 19.334003] x8 : 0001ff88003f3888 x7 : 0000000000000000 x6 : 0000000000000000
>> [ 19.341537] x5 : 0000000000000000 x4 : 0000000000001668 x3 : 0000000000000000
>> [ 19.349054] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff88003f3880
>> [ 19.356581] Call trace:
>> [ 19.359160] __mutex_lock+0x4bc/0x550
>> [ 19.363032] mutex_lock_nested+0x24/0x30
>> [ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
>> [ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
>> [ 19.376364] process_one_work+0x3ac/0x988
>> [ 19.380660] worker_thread+0x398/0x694
>> [ 19.384736] kthread+0x1bc/0x1c0
>> [ 19.388241] ret_from_fork+0x10/0x20
>> [ 19.392031] irq event stamp: 183
>> [ 19.395450] hardirqs last enabled at (183): [<ffffffc0800b9278>] finish_task_switch.isra.0+0xa8/0x2d4
>> [ 19.405140] hardirqs last disabled at (182): [<ffffffc081ad3754>] __schedule+0x714/0xd04
>> [ 19.413612] softirqs last enabled at (114): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
>> [ 19.423128] softirqs last disabled at (110): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
>> [ 19.432614] ---[ end trace 0000000000000000 ]---
>>
>> Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> index 88eb33acd5f0..639fff2c693f 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>> @@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
>> if (ret)
>> goto err_dp;
>> + drm_bridge_add(dpsub->bridge);
>> if (dpsub->dma_enabled) {
>> ret = zynqmp_dpsub_drm_init(dpsub);
>> if (ret)
>> goto err_disp;
>> - } else {
>> - drm_bridge_add(dpsub->bridge);
>> }
>> dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
>> @@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev)
>> if (dpsub->drm)
>> zynqmp_dpsub_drm_cleanup(dpsub);
>> - else
>> - drm_bridge_remove(dpsub->bridge);
>> + drm_bridge_remove(dpsub->bridge);
>> zynqmp_disp_remove(dpsub);
>> zynqmp_dp_remove(dpsub);
>>
>
> I sent a similar patch:
>
> https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03bac@ideasonboard.com/
>
> I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where the bridge is set up, so it felt like a logical place. You add it later, just before the bridge is used the first time.
>
> I like mine a bit more as it has all the bridge code in the same place, but I also wonder if there might be some risks in adding the bridge early (before zynqmp_disp_probe()), although I can't see any issue right away...
>
> In any case, as this works for me too:
>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>
> Tomi
>
Can someone pick up this fix before the release? I am still running into this bug on linux-next/master.
--Sean
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
2024-04-23 20:50 ` Sean Anderson
@ 2024-04-24 12:17 ` Tomi Valkeinen
0 siblings, 0 replies; 6+ messages in thread
From: Tomi Valkeinen @ 2024-04-24 12:17 UTC (permalink / raw
To: Sean Anderson
Cc: linux-arm-kernel, Michal Simek, linux-kernel, Daniel Vetter,
David Airlie, Laurent Pinchart, dri-devel, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann
On 23/04/2024 23:50, Sean Anderson wrote:
> Hi,
>
> On 3/22/24 02:01, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 08/03/2024 22:47, Sean Anderson wrote:
>>> We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
>>> calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
>>> initialized. We do this before zynqmp_dpsub_drm_init since that calls
>>> drm_bridge_attach. This fixes the following lockdep warning:
>>>
>>> [ 19.217084] ------------[ cut here ]------------
>>> [ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>>> [ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582 __mutex_lock+0x4bc/0x550
>>> [ 19.241696] Modules linked in:
>>> [ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
>>> [ 19.252046] Hardware name: xlnx,zynqmp (DT)
>>> [ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
>>> [ 19.261795] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>> [ 19.269104] pc : __mutex_lock+0x4bc/0x550
>>> [ 19.273364] lr : __mutex_lock+0x4bc/0x550
>>> [ 19.277592] sp : ffffffc085c5bbe0
>>> [ 19.281066] x29: ffffffc085c5bbe0 x28: 0000000000000000 x27: ffffff88009417f8
>>> [ 19.288624] x26: ffffff8800941788 x25: ffffff8800020008 x24: ffffffc082aa3000
>>> [ 19.296227] x23: ffffffc080d90e3c x22: 0000000000000002 x21: 0000000000000000
>>> [ 19.303744] x20: 0000000000000000 x19: ffffff88002f5210 x18: 0000000000000000
>>> [ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
>>> [ 19.318922] x14: 0000000000000000 x13: 284e4f5f4e524157 x12: 0000000000000001
>>> [ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
>>> [ 19.334003] x8 : 0001ff88003f3888 x7 : 0000000000000000 x6 : 0000000000000000
>>> [ 19.341537] x5 : 0000000000000000 x4 : 0000000000001668 x3 : 0000000000000000
>>> [ 19.349054] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff88003f3880
>>> [ 19.356581] Call trace:
>>> [ 19.359160] __mutex_lock+0x4bc/0x550
>>> [ 19.363032] mutex_lock_nested+0x24/0x30
>>> [ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
>>> [ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
>>> [ 19.376364] process_one_work+0x3ac/0x988
>>> [ 19.380660] worker_thread+0x398/0x694
>>> [ 19.384736] kthread+0x1bc/0x1c0
>>> [ 19.388241] ret_from_fork+0x10/0x20
>>> [ 19.392031] irq event stamp: 183
>>> [ 19.395450] hardirqs last enabled at (183): [<ffffffc0800b9278>] finish_task_switch.isra.0+0xa8/0x2d4
>>> [ 19.405140] hardirqs last disabled at (182): [<ffffffc081ad3754>] __schedule+0x714/0xd04
>>> [ 19.413612] softirqs last enabled at (114): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
>>> [ 19.423128] softirqs last disabled at (110): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
>>> [ 19.432614] ---[ end trace 0000000000000000 ]---
>>>
>>> Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>> ---
>>>
>>> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++----
>>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>>> index 88eb33acd5f0..639fff2c693f 100644
>>> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>>> @@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
>>> if (ret)
>>> goto err_dp;
>>> + drm_bridge_add(dpsub->bridge);
>>> if (dpsub->dma_enabled) {
>>> ret = zynqmp_dpsub_drm_init(dpsub);
>>> if (ret)
>>> goto err_disp;
>>> - } else {
>>> - drm_bridge_add(dpsub->bridge);
>>> }
>>> dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
>>> @@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev)
>>> if (dpsub->drm)
>>> zynqmp_dpsub_drm_cleanup(dpsub);
>>> - else
>>> - drm_bridge_remove(dpsub->bridge);
>>> + drm_bridge_remove(dpsub->bridge);
>>> zynqmp_disp_remove(dpsub);
>>> zynqmp_dp_remove(dpsub);
>>>
>>
>> I sent a similar patch:
>>
>> https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03bac@ideasonboard.com/
>>
>> I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where the bridge is set up, so it felt like a logical place. You add it later, just before the bridge is used the first time.
>>
>> I like mine a bit more as it has all the bridge code in the same place, but I also wonder if there might be some risks in adding the bridge early (before zynqmp_disp_probe()), although I can't see any issue right away...
>>
>> In any case, as this works for me too:
>>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>
>> Tomi
>>
> Can someone pick up this fix before the release? I am still running into this bug on linux-next/master.
Can you check my version (link above)? After looking at both versions
today, I'd rather push mine: it adds and removes the bridge in the same
file where all the bridge code resides, and it'd be nice to manage the
bridge in that file as much as possible.
Tomi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
2024-03-22 6:01 ` Tomi Valkeinen
2024-04-23 20:50 ` Sean Anderson
@ 2024-04-26 9:30 ` Laurent Pinchart
2024-04-27 7:47 ` Tomi Valkeinen
1 sibling, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2024-04-26 9:30 UTC (permalink / raw
To: Tomi Valkeinen
Cc: Sean Anderson, linux-arm-kernel, Michal Simek, linux-kernel,
Daniel Vetter, David Airlie, dri-devel, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann
On Fri, Mar 22, 2024 at 08:01:44AM +0200, Tomi Valkeinen wrote:
> On 08/03/2024 22:47, Sean Anderson wrote:
> > We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
> > calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
> > initialized. We do this before zynqmp_dpsub_drm_init since that calls
> > drm_bridge_attach. This fixes the following lockdep warning:
> >
> > [ 19.217084] ------------[ cut here ]------------
> > [ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> > [ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582 __mutex_lock+0x4bc/0x550
> > [ 19.241696] Modules linked in:
> > [ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
> > [ 19.252046] Hardware name: xlnx,zynqmp (DT)
> > [ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
> > [ 19.261795] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [ 19.269104] pc : __mutex_lock+0x4bc/0x550
> > [ 19.273364] lr : __mutex_lock+0x4bc/0x550
> > [ 19.277592] sp : ffffffc085c5bbe0
> > [ 19.281066] x29: ffffffc085c5bbe0 x28: 0000000000000000 x27: ffffff88009417f8
> > [ 19.288624] x26: ffffff8800941788 x25: ffffff8800020008 x24: ffffffc082aa3000
> > [ 19.296227] x23: ffffffc080d90e3c x22: 0000000000000002 x21: 0000000000000000
> > [ 19.303744] x20: 0000000000000000 x19: ffffff88002f5210 x18: 0000000000000000
> > [ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
> > [ 19.318922] x14: 0000000000000000 x13: 284e4f5f4e524157 x12: 0000000000000001
> > [ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
> > [ 19.334003] x8 : 0001ff88003f3888 x7 : 0000000000000000 x6 : 0000000000000000
> > [ 19.341537] x5 : 0000000000000000 x4 : 0000000000001668 x3 : 0000000000000000
> > [ 19.349054] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff88003f3880
> > [ 19.356581] Call trace:
> > [ 19.359160] __mutex_lock+0x4bc/0x550
> > [ 19.363032] mutex_lock_nested+0x24/0x30
> > [ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
> > [ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
> > [ 19.376364] process_one_work+0x3ac/0x988
> > [ 19.380660] worker_thread+0x398/0x694
> > [ 19.384736] kthread+0x1bc/0x1c0
> > [ 19.388241] ret_from_fork+0x10/0x20
> > [ 19.392031] irq event stamp: 183
> > [ 19.395450] hardirqs last enabled at (183): [<ffffffc0800b9278>] finish_task_switch.isra.0+0xa8/0x2d4
> > [ 19.405140] hardirqs last disabled at (182): [<ffffffc081ad3754>] __schedule+0x714/0xd04
> > [ 19.413612] softirqs last enabled at (114): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
> > [ 19.423128] softirqs last disabled at (110): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
> > [ 19.432614] ---[ end trace 0000000000000000 ]---
> >
> > Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
> > Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> > ---
> >
> > drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> > index 88eb33acd5f0..639fff2c693f 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> > @@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_dp;
> >
> > + drm_bridge_add(dpsub->bridge);
A blank line here would be nice.
> > if (dpsub->dma_enabled) {
> > ret = zynqmp_dpsub_drm_init(dpsub);
> > if (ret)
> > goto err_disp;
> > - } else {
> > - drm_bridge_add(dpsub->bridge);
> > }
> >
> > dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
> > @@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev)
> >
> > if (dpsub->drm)
> > zynqmp_dpsub_drm_cleanup(dpsub);
> > - else
> > - drm_bridge_remove(dpsub->bridge);
> >
> > + drm_bridge_remove(dpsub->bridge);
> > zynqmp_disp_remove(dpsub);
> > zynqmp_dp_remove(dpsub);
> >
>
> I sent a similar patch:
>
> https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03bac@ideasonboard.com/
>
> I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where
> the bridge is set up, so it felt like a logical place. You add it later,
> just before the bridge is used the first time.
>
> I like mine a bit more as it has all the bridge code in the same place,
> but I also wonder if there might be some risks in adding the bridge
> early (before zynqmp_disp_probe()), although I can't see any issue right
> away...
Seems we have the same concerns :-) I've reviewed your patch and wrote
pretty much the same. I would be more comfortable with this version,
even if I like gathering all bridge code in the same location.
> In any case, as this works for me too:
>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
2024-04-26 9:30 ` Laurent Pinchart
@ 2024-04-27 7:47 ` Tomi Valkeinen
0 siblings, 0 replies; 6+ messages in thread
From: Tomi Valkeinen @ 2024-04-27 7:47 UTC (permalink / raw
To: Laurent Pinchart, Sean Anderson
Cc: linux-arm-kernel, Michal Simek, linux-kernel, Daniel Vetter,
David Airlie, dri-devel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann
On 26/04/2024 12:30, Laurent Pinchart wrote:
> On Fri, Mar 22, 2024 at 08:01:44AM +0200, Tomi Valkeinen wrote:
>> On 08/03/2024 22:47, Sean Anderson wrote:
>>> We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
>>> calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
>>> initialized. We do this before zynqmp_dpsub_drm_init since that calls
>>> drm_bridge_attach. This fixes the following lockdep warning:
>>>
>>> [ 19.217084] ------------[ cut here ]------------
>>> [ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>>> [ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582 __mutex_lock+0x4bc/0x550
>>> [ 19.241696] Modules linked in:
>>> [ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
>>> [ 19.252046] Hardware name: xlnx,zynqmp (DT)
>>> [ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
>>> [ 19.261795] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>> [ 19.269104] pc : __mutex_lock+0x4bc/0x550
>>> [ 19.273364] lr : __mutex_lock+0x4bc/0x550
>>> [ 19.277592] sp : ffffffc085c5bbe0
>>> [ 19.281066] x29: ffffffc085c5bbe0 x28: 0000000000000000 x27: ffffff88009417f8
>>> [ 19.288624] x26: ffffff8800941788 x25: ffffff8800020008 x24: ffffffc082aa3000
>>> [ 19.296227] x23: ffffffc080d90e3c x22: 0000000000000002 x21: 0000000000000000
>>> [ 19.303744] x20: 0000000000000000 x19: ffffff88002f5210 x18: 0000000000000000
>>> [ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
>>> [ 19.318922] x14: 0000000000000000 x13: 284e4f5f4e524157 x12: 0000000000000001
>>> [ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
>>> [ 19.334003] x8 : 0001ff88003f3888 x7 : 0000000000000000 x6 : 0000000000000000
>>> [ 19.341537] x5 : 0000000000000000 x4 : 0000000000001668 x3 : 0000000000000000
>>> [ 19.349054] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffffff88003f3880
>>> [ 19.356581] Call trace:
>>> [ 19.359160] __mutex_lock+0x4bc/0x550
>>> [ 19.363032] mutex_lock_nested+0x24/0x30
>>> [ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
>>> [ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
>>> [ 19.376364] process_one_work+0x3ac/0x988
>>> [ 19.380660] worker_thread+0x398/0x694
>>> [ 19.384736] kthread+0x1bc/0x1c0
>>> [ 19.388241] ret_from_fork+0x10/0x20
>>> [ 19.392031] irq event stamp: 183
>>> [ 19.395450] hardirqs last enabled at (183): [<ffffffc0800b9278>] finish_task_switch.isra.0+0xa8/0x2d4
>>> [ 19.405140] hardirqs last disabled at (182): [<ffffffc081ad3754>] __schedule+0x714/0xd04
>>> [ 19.413612] softirqs last enabled at (114): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
>>> [ 19.423128] softirqs last disabled at (110): [<ffffffc080133de8>] srcu_invoke_callbacks+0x158/0x23c
>>> [ 19.432614] ---[ end trace 0000000000000000 ]---
>>>
>>> Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>> ---
>>>
>>> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++----
>>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>>> index 88eb33acd5f0..639fff2c693f 100644
>>> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
>>> @@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
>>> if (ret)
>>> goto err_dp;
>>>
>>> + drm_bridge_add(dpsub->bridge);
>
> A blank line here would be nice.
>
>>> if (dpsub->dma_enabled) {
>>> ret = zynqmp_dpsub_drm_init(dpsub);
>>> if (ret)
>>> goto err_disp;
>>> - } else {
>>> - drm_bridge_add(dpsub->bridge);
>>> }
>>>
>>> dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
>>> @@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev)
>>>
>>> if (dpsub->drm)
>>> zynqmp_dpsub_drm_cleanup(dpsub);
>>> - else
>>> - drm_bridge_remove(dpsub->bridge);
>>>
>>> + drm_bridge_remove(dpsub->bridge);
>>> zynqmp_disp_remove(dpsub);
>>> zynqmp_dp_remove(dpsub);
>>>
>>
>> I sent a similar patch:
>>
>> https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03bac@ideasonboard.com/
>>
>> I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where
>> the bridge is set up, so it felt like a logical place. You add it later,
>> just before the bridge is used the first time.
>>
>> I like mine a bit more as it has all the bridge code in the same place,
>> but I also wonder if there might be some risks in adding the bridge
>> early (before zynqmp_disp_probe()), although I can't see any issue right
>> away...
>
> Seems we have the same concerns :-) I've reviewed your patch and wrote
> pretty much the same. I would be more comfortable with this version,
> even if I like gathering all bridge code in the same location.
I guess there's no reason to take the risk here, so I have pushed this
one to drm-misc-next.
Tomi
>> In any case, as this works for me too:
>>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-27 7:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-08 20:47 [PATCH] drm: zynqmp_dpsub: Always register bridge Sean Anderson
2024-03-22 6:01 ` Tomi Valkeinen
2024-04-23 20:50 ` Sean Anderson
2024-04-24 12:17 ` Tomi Valkeinen
2024-04-26 9:30 ` Laurent Pinchart
2024-04-27 7:47 ` Tomi Valkeinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).