| Message ID | 20240308204741.3631919-1-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: zynqmp_dpsub: Always register bridge | expand |
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
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
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
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>
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> >
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);
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(-)