On Mon, Sep 5, 2022 at 3:37 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> gma_crtc_page_flip() was holding the event_lock spinlock while calling
> crtc_funcs->mode_set_base() which takes ww_mutex.
>
> The only reason to hold event_lock is to clear gma_crtc->page_flip_event
> on mode_set_base() errors.
>
> Instead unlock it after setting gma_crtc->page_flip_event and on
> errors re-take the lock and clear gma_crtc->page_flip_event it
> it is still set.
Hi Hans, thanks for having a look at gma500.
See comments below.
>
> This fixes the following WARN/stacktrace:
>
> [ 512.122953] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:870
> [ 512.123004] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1253, name: gnome-shell
> [ 512.123031] preempt_count: 1, expected: 0
> [ 512.123048] RCU nest depth: 0, expected: 0
> [ 512.123066] INFO: lockdep is turned off.
> [ 512.123080] irq event stamp: 0
> [ 512.123094] hardirqs last enabled at (0): [<0000000000000000>] 0x0
> [ 512.123134] hardirqs last disabled at (0): [<ffffffff8d0ec28c>] copy_process+0x9fc/0x1de0
> [ 512.123176] softirqs last enabled at (0): [<ffffffff8d0ec28c>] copy_process+0x9fc/0x1de0
> [ 512.123207] softirqs last disabled at (0): [<0000000000000000>] 0x0
> [ 512.123233] Preemption disabled at:
> [ 512.123241] [<0000000000000000>] 0x0
> [ 512.123275] CPU: 3 PID: 1253 Comm: gnome-shell Tainted: G W 5.19.0+ #1
> [ 512.123304] Hardware name: Packard Bell dot s/SJE01_CT, BIOS V1.10 07/23/2013
> [ 512.123323] Call Trace:
> [ 512.123346] <TASK>
> [ 512.123370] dump_stack_lvl+0x5b/0x77
> [ 512.123412] __might_resched.cold+0xff/0x13a
> [ 512.123458] ww_mutex_lock+0x1e/0xa0
> [ 512.123495] psb_gem_pin+0x2c/0x150 [gma500_gfx]
> [ 512.123601] gma_pipe_set_base+0x76/0x240 [gma500_gfx]
> [ 512.123708] gma_crtc_page_flip+0x95/0x130 [gma500_gfx]
> [ 512.123808] drm_mode_page_flip_ioctl+0x57d/0x5d0
> [ 512.123897] ? drm_mode_cursor2_ioctl+0x10/0x10
> [ 512.123936] drm_ioctl_kernel+0xa1/0x150
> [ 512.123984] drm_ioctl+0x21f/0x420
> [ 512.124025] ? drm_mode_cursor2_ioctl+0x10/0x10
> [ 512.124070] ? rcu_read_lock_bh_held+0xb/0x60
> [ 512.124104] ? lock_release+0x1ef/0x2d0
> [ 512.124161] __x64_sys_ioctl+0x8d/0xd0
> [ 512.124203] do_syscall_64+0x58/0x80
> [ 512.124239] ? do_syscall_64+0x67/0x80
> [ 512.124267] ? trace_hardirqs_on_prepare+0x55/0xe0
> [ 512.124300] ? do_syscall_64+0x67/0x80
> [ 512.124340] ? rcu_read_lock_sched_held+0x10/0x80
> [ 512.124377] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [ 512.124411] RIP: 0033:0x7fcc4a70740f
> [ 512.124442] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 18 48 8b 44 24 18 64 48 2b 04 25 28 00 00
> [ 512.124470] RSP: 002b:00007ffda73f5390 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [ 512.124503] RAX: ffffffffffffffda RBX: 000055cc9e474500 RCX: 00007fcc4a70740f
> [ 512.124524] RDX: 00007ffda73f5420 RSI: 00000000c01864b0 RDI: 0000000000000009
> [ 512.124544] RBP: 00007ffda73f5420 R08: 000055cc9c0b0cb0 R09: 0000000000000034
> [ 512.124564] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000c01864b0
> [ 512.124584] R13: 0000000000000009 R14: 000055cc9df484d0 R15: 000055cc9af5d0c0
> [ 512.124647] </TASK>
>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/gpu/drm/gma500/gma_display.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index bd40c040a2c9..cf038e322164 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -532,15 +532,19 @@ int gma_crtc_page_flip(struct drm_crtc *crtc,
> WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>
> gma_crtc->page_flip_event = event;
> + spin_unlock_irqrestore(&dev->event_lock, flags);
>
> /* Call this locked if we want an event at vblank interrupt. */
If we don't hold the event_lock around mode_set_base() we could
potentially get a vblank before we do the modeset. That would send the
event prematurely. I think this is what the comment tries to tell us.
-Patrik
> ret = crtc_funcs->mode_set_base(crtc, crtc->x, crtc->y, old_fb);
> if (ret) {
> - gma_crtc->page_flip_event = NULL;
> - drm_crtc_vblank_put(crtc);
> + spin_lock_irqsave(&dev->event_lock, flags);
> + if (gma_crtc->page_flip_event) {
> + gma_crtc->page_flip_event = NULL;
> + drm_crtc_vblank_put(crtc);
> + }
> + spin_unlock_irqrestore(&dev->event_lock, flags);
> }
>
> - spin_unlock_irqrestore(&dev->event_lock, flags);
> } else {
> ret = crtc_funcs->mode_set_base(crtc, crtc->x, crtc->y, old_fb);
> }
> --
> 2.36.1
>
