Currently, there's nothing actually stopping a driver from only registering vblank support for some of it's CRTCs and not for others. As far as I can tell, this isn't really defined behavior on the C side of things - as the documentation explicitly mentions to not use drm_vblank_init() if you don't have vblank support - since DRM then steps in and adds its own vblank emulation implementation.
So, let's fix this edge case and check to make sure it's all or none.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: 3ed4351a83ca ("drm: Extract drm_vblank.[hc]") Cc: Stefan Agner stefan@agner.ch Cc: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@gmail.com Cc: Simona Vetter simona@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v4.13+ --- drivers/gpu/drm/drm_vblank.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 94e45ed6869d0..4d00937e8ca2e 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -525,9 +525,19 @@ static void drm_vblank_init_release(struct drm_device *dev, void *ptr) */ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs) { + struct drm_crtc *crtc; int ret; unsigned int i;
+ // Confirm that the required vblank functions have been filled out for all CRTCS + drm_for_each_crtc(crtc, dev) { + if (!crtc->funcs->enable_vblank || !crtc->funcs->disable_vblank) { + drm_err(dev, "CRTC vblank functions not initialized for %s, abort\n", + crtc->name); + return -EINVAL; + } + } + spin_lock_init(&dev->vbl_lock); spin_lock_init(&dev->vblank_time_lock);
base-commit: 22512c3ee0f47faab5def71c4453638923c62522
Hello,
kernel test robot noticed "BUG:kernel_NULL_pointer_dereference,address" on:
commit: 8e1a430cf308254a61a2317a0dfc4d8f4b3e13cb ("[PATCH] drm/vblank: Require a driver register vblank support for 0 or all CRTCs") url: https://github.com/intel-lab-lkp/linux/commits/Lyude-Paul/drm-vblank-Require... patch link: https://lore.kernel.org/all/20240927203946.695934-2-lyude@redhat.com/ patch subject: [PATCH] drm/vblank: Require a driver register vblank support for 0 or all CRTCs
in testcase: boot
compiler: gcc-12 test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 4G
(please refer to attached dmesg/kmsg for entire log/backtrace)
+---------------------------------------------+------------+------------+ | | 22512c3ee0 | 8e1a430cf3 | +---------------------------------------------+------------+------------+ | boot_successes | 12 | 0 | | boot_failures | 0 | 12 | | BUG:kernel_NULL_pointer_dereference,address | 0 | 12 | | Oops:Oops:#[##] | 0 | 12 | | EIP:drm_vblank_init | 0 | 12 | | Kernel_panic-not_syncing:Fatal_exception | 0 | 12 | +---------------------------------------------+------------+------------+
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot oliver.sang@intel.com | Closes: https://lore.kernel.org/oe-lkp/202410101418.5704b4a5-lkp@intel.com
[ 4.727010][ T1] BUG: kernel NULL pointer dereference, address: 00000188 [ 4.728324][ T1] #PF: supervisor read access in kernel mode [ 4.729456][ T1] #PF: error_code(0x0000) - not-present page [ 4.729853][ T1] *pdpt = 0000000000000000 *pde = f000ff53f000ff53 [ 4.729853][ T1] Oops: Oops: 0000 [#1] [ 4.729853][ T1] CPU: 0 UID: 0 PID: 1 Comm: swapper Tainted: G T 6.11.0-rc7-01372-g8e1a430cf308 #1 577dd3e1adc1bccd6f381550d3179686c5f157a0 [ 4.729853][ T1] Tainted: [T]=RANDSTRUCT [ 4.729853][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014 [ 4.729853][ T1] EIP: drm_vblank_init (drivers/gpu/drm/drm_vblank.c:534) [ 4.729853][ T1] Code: 89 c6 53 83 ec 08 89 55 ec 8b 90 64 05 00 00 39 d1 74 56 8d 42 f8 eb 12 90 8b 5a 04 85 db 74 17 8b 50 08 8d 42 f8 39 d1 74 3f <8b> 90 90 01 00 00 8b 7a 08 85 ff 75 e2 8b 40 10 85 f6 74 03 8b 76 All code ======== 0: 89 c6 mov %eax,%esi 2: 53 push %rbx 3: 83 ec 08 sub $0x8,%esp 6: 89 55 ec mov %edx,-0x14(%rbp) 9: 8b 90 64 05 00 00 mov 0x564(%rax),%edx f: 39 d1 cmp %edx,%ecx 11: 74 56 je 0x69 13: 8d 42 f8 lea -0x8(%rdx),%eax 16: eb 12 jmp 0x2a 18: 90 nop 19: 8b 5a 04 mov 0x4(%rdx),%ebx 1c: 85 db test %ebx,%ebx 1e: 74 17 je 0x37 20: 8b 50 08 mov 0x8(%rax),%edx 23: 8d 42 f8 lea -0x8(%rdx),%eax 26: 39 d1 cmp %edx,%ecx 28: 74 3f je 0x69 2a:* 8b 90 90 01 00 00 mov 0x190(%rax),%edx <-- trapping instruction 30: 8b 7a 08 mov 0x8(%rdx),%edi 33: 85 ff test %edi,%edi 35: 75 e2 jne 0x19 37: 8b 40 10 mov 0x10(%rax),%eax 3a: 85 f6 test %esi,%esi 3c: 74 03 je 0x41 3e: 8b .byte 0x8b 3f: 76 .byte 0x76
Code starting with the faulting instruction =========================================== 0: 8b 90 90 01 00 00 mov 0x190(%rax),%edx 6: 8b 7a 08 mov 0x8(%rdx),%edi 9: 85 ff test %edi,%edi b: 75 e2 jne 0xffffffffffffffef d: 8b 40 10 mov 0x10(%rax),%eax 10: 85 f6 test %esi,%esi 12: 74 03 je 0x17 14: 8b .byte 0x8b 15: 76 .byte 0x76 [ 4.729853][ T1] EAX: fffffff8 EBX: 86802000 ECX: 86802564 EDX: 00000000 [ 4.729853][ T1] ESI: 86802000 EDI: 86813400 EBP: 85e1fe90 ESP: 85e1fe7c [ 4.729853][ T1] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068 EFLAGS: 00010282 [ 4.729853][ T1] CR0: 80050033 CR2: 00000188 CR3: 05182000 CR4: 000406b0 [ 4.729853][ T1] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 [ 4.729853][ T1] DR6: fffe0ff0 DR7: 00000400 [ 4.729853][ T1] Call Trace: [ 4.729853][ T1] ? show_regs (arch/x86/kernel/dumpstack.c:478) [ 4.729853][ T1] ? __die (arch/x86/kernel/dumpstack.c:421 arch/x86/kernel/dumpstack.c:434) [ 4.729853][ T1] ? page_fault_oops (arch/x86/mm/fault.c:715) [ 4.729853][ T1] ? kernelmode_fixup_or_oops+0x54/0x68 [ 4.729853][ T1] ? __bad_area_nosemaphore+0x103/0x180 [ 4.729853][ T1] ? sched_clock_noinstr (arch/x86/kernel/tsc.c:267) [ 4.729853][ T1] ? bad_area_nosemaphore (arch/x86/mm/fault.c:835) [ 4.729853][ T1] ? do_user_addr_fault (arch/x86/mm/fault.c:1452) [ 4.729853][ T1] ? exc_page_fault (arch/x86/include/asm/irqflags.h:26 arch/x86/include/asm/irqflags.h:87 arch/x86/include/asm/irqflags.h:147 arch/x86/mm/fault.c:1489 arch/x86/mm/fault.c:1539) [ 4.729853][ T1] ? pvclock_clocksource_read_nowd (arch/x86/mm/fault.c:1494) [ 4.729853][ T1] ? handle_exception (arch/x86/entry/entry_32.S:1054) [ 4.729853][ T1] ? pvclock_clocksource_read_nowd (arch/x86/mm/fault.c:1494) [ 4.729853][ T1] ? drm_vblank_init (drivers/gpu/drm/drm_vblank.c:534) [ 4.729853][ T1] ? pvclock_clocksource_read_nowd (arch/x86/mm/fault.c:1494) [ 4.729853][ T1] ? drm_vblank_init (drivers/gpu/drm/drm_vblank.c:534) [ 4.729853][ T1] vkms_create (drivers/gpu/drm/vkms/vkms_drv.c:211) [ 4.729853][ T1] vkms_init (drivers/gpu/drm/vkms/vkms_drv.c:254) [ 4.729853][ T1] ? vgem_init (drivers/gpu/drm/vkms/vkms_drv.c:240) [ 4.729853][ T1] do_one_initcall (init/main.c:1267) [ 4.729853][ T1] do_initcalls (init/main.c:1328 init/main.c:1345) [ 4.729853][ T1] kernel_init_freeable (init/main.c:1580) [ 4.729853][ T1] ? rest_init (init/main.c:1459) [ 4.729853][ T1] kernel_init (init/main.c:1469) [ 4.729853][ T1] ret_from_fork (arch/x86/kernel/process.c:153) [ 4.729853][ T1] ? rest_init (init/main.c:1459) [ 4.729853][ T1] ret_from_fork_asm (arch/x86/entry/entry_32.S:737) [ 4.729853][ T1] entry_INT80_32 (arch/x86/entry/entry_32.S:944) [ 4.729853][ T1] Modules linked in: [ 4.729853][ T1] CR2: 0000000000000188 [ 4.729853][ T1] ---[ end trace 0000000000000000 ]--- [ 4.729853][ T1] EIP: drm_vblank_init (drivers/gpu/drm/drm_vblank.c:534) [ 4.729853][ T1] Code: 89 c6 53 83 ec 08 89 55 ec 8b 90 64 05 00 00 39 d1 74 56 8d 42 f8 eb 12 90 8b 5a 04 85 db 74 17 8b 50 08 8d 42 f8 39 d1 74 3f <8b> 90 90 01 00 00 8b 7a 08 85 ff 75 e2 8b 40 10 85 f6 74 03 8b 76 All code ======== 0: 89 c6 mov %eax,%esi 2: 53 push %rbx 3: 83 ec 08 sub $0x8,%esp 6: 89 55 ec mov %edx,-0x14(%rbp) 9: 8b 90 64 05 00 00 mov 0x564(%rax),%edx f: 39 d1 cmp %edx,%ecx 11: 74 56 je 0x69 13: 8d 42 f8 lea -0x8(%rdx),%eax 16: eb 12 jmp 0x2a 18: 90 nop 19: 8b 5a 04 mov 0x4(%rdx),%ebx 1c: 85 db test %ebx,%ebx 1e: 74 17 je 0x37 20: 8b 50 08 mov 0x8(%rax),%edx 23: 8d 42 f8 lea -0x8(%rdx),%eax 26: 39 d1 cmp %edx,%ecx 28: 74 3f je 0x69 2a:* 8b 90 90 01 00 00 mov 0x190(%rax),%edx <-- trapping instruction 30: 8b 7a 08 mov 0x8(%rdx),%edi 33: 85 ff test %edi,%edi 35: 75 e2 jne 0x19 37: 8b 40 10 mov 0x10(%rax),%eax 3a: 85 f6 test %esi,%esi 3c: 74 03 je 0x41 3e: 8b .byte 0x8b 3f: 76 .byte 0x76
Code starting with the faulting instruction =========================================== 0: 8b 90 90 01 00 00 mov 0x190(%rax),%edx 6: 8b 7a 08 mov 0x8(%rdx),%edi 9: 85 ff test %edi,%edi b: 75 e2 jne 0xffffffffffffffef d: 8b 40 10 mov 0x10(%rax),%eax 10: 85 f6 test %esi,%esi 12: 74 03 je 0x17 14: 8b .byte 0x8b 15: 76 .byte 0x76
The kernel config and materials to reproduce are available at: https://download.01.org/0day-ci/archive/20241010/202410101418.5704b4a5-lkp@i...
Le 27/09/24 - 16:39, Lyude Paul a écrit :
Currently, there's nothing actually stopping a driver from only registering vblank support for some of it's CRTCs and not for others. As far as I can tell, this isn't really defined behavior on the C side of things - as the documentation explicitly mentions to not use drm_vblank_init() if you don't have vblank support - since DRM then steps in and adds its own vblank emulation implementation.
So, let's fix this edge case and check to make sure it's all or none.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: 3ed4351a83ca ("drm: Extract drm_vblank.[hc]") Cc: Stefan Agner stefan@agner.ch Cc: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@gmail.com Cc: Simona Vetter simona@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v4.13+
drivers/gpu/drm/drm_vblank.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 94e45ed6869d0..4d00937e8ca2e 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -525,9 +525,19 @@ static void drm_vblank_init_release(struct drm_device *dev, void *ptr) */ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs) {
- struct drm_crtc *crtc; int ret; unsigned int i;
- // Confirm that the required vblank functions have been filled out for all CRTCS
- drm_for_each_crtc(crtc, dev) {
if (!crtc->funcs->enable_vblank || !crtc->funcs->disable_vblank) {drm_err(dev, "CRTC vblank functions not initialized for %s, abort\n",crtc->name);return -EINVAL;}- }
Hi,
I noticed that the kernel bot reported an issue with VKMS and this patch.
I did not take the time to reproduce the issue, but it may come from the fact that VKMS call drm_vblank_init before calling drmm_crtc_init_with_planes [1]. I don't see anything in the documentation that requires the CRTC to be initialized before the vblank, is it a change of the API or an issue in VKMS since 2018 [2]?
Anyway, if this is a requirement, can you explain it in the drm_vblank_init documentation?
Thanks, Louis Chauvet
[1]:https://elixir.bootlin.com/linux/v6.11.2/source/drivers/gpu/drm/vkms/vkms_dr... [2]:https://lore.kernel.org/all/5d9ca7b3884c1995bd4a983b1d2ff1b840eb7f1a.1531402...
spin_lock_init(&dev->vbl_lock); spin_lock_init(&dev->vblank_time_lock);
base-commit: 22512c3ee0f47faab5def71c4453638923c62522
2.46.1
linux-stable-mirror@lists.linaro.org
-
kernel test robot -
Louis Chauvet -
Lyude Paul