Message-ID: <20150222114903.GP24485@phenom.ffwll.local>
Date:	Sun, 22 Feb 2015 12:49:03 +0100
From:	Daniel Vetter <daniel@...ll.ch>
To:	Aaron Plattner <aplattner@...dia.com>
Cc:	David Airlie <airlied@...ux.ie>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/udl: add enable/disable_vblank stubs

On Sun, Feb 15, 2015 at 04:52:39PM -0800, Aaron Plattner wrote:
> vblank_disable_and_save calls the driver's disable_vblank hook unconditionally,
> which crashes the udl driver since it doesn't implement it.  Fix this by adding
> stub implementations of these functions identical to the qxl ones.
> 
>   usb 5-1: USB disconnect, device number 2
>   BUG: unable to handle kernel NULL pointer dereference at           (null)
>   IP: [<          (null)>]           (null)
>   PGD 3bc23d067 PUD 3bc0fc067 PMD 0
>   Oops: 0010 [#1] PREEMPT SMP
>   Modules linked in: udlfb fb_sys_fops nvidia(PO) udl drm_kms_helper drm
>                      syscopyarea sysfillrect sysimgblt netconsole cfg80211
>                      joydev mousedev nls_iso8859_1 nls_cp437 vfat fat coretemp
>                      intel_rapl eeepc_wmi asus_wmi sparse_keymap led_class
>                      rfkill hwmon iosf_mbi x86_pkg_temp_thermal intel_powerclamp
>                      iTCO_wdt iTCO_vendor_support evdev kvm crct10dif_pclmul
>                      crc32_pclmul ghash_clmulni_intel aesni_intel mac_hid
>                      tpm_infineon aes_x86_64 lrw gf128mul e1000e glue_helper
>                      ablk_helper cryptd mxm_wmi tpm_tis processor psmouse ptp
>                      serio_raw tpm snd_hda_codec_hdmi i2c_i801 i2c_core lpc_ich
>                      pps_core pcspkr snd_hda_codec_realtek snd_hda_codec_generic
>                      mei_me mei snd_hda_intel snd_hda_controller snd_hda_codec
>                      snd_hwdep snd_pcm snd_timer snd ie31200_edac edac_core
>                      soundcore thermal shpchp battery video wmi button fan
>                      sch_fq_codel nfs lockd grace sunrpc fscache btrfs xor
>                      raid6_pq hid_generic usbhid hid sd_mod atkbd libps2 ahci
>                      libahci crc32c_intel xhci_pci libata ehci_pci xhci_hcd
>                      ehci_hcd scsi_mod usbcore usb_common i8042 serio [last
>                      unloaded: drm]
>   CPU: 2 PID: 2044 Comm: Xorg.bin Tainted: P           O   3.19.0-1-agp #1
>   Hardware name: System manufacturer System Product Name/P8Z77-V, BIOS 2104 08/13/2013
>   task: ffff8803f99d27c0 ti: ffff8803bc1c4000 task.ti: ffff8803bc1c4000
>   RIP: 0010:[<0000000000000000>]  [<          (null)>]           (null)
>   RSP: 0018:ffff8803bc1c7d00  EFLAGS: 00010046
>   RAX: ffffffffa06d0140 RBX: 0000000000000000 RCX: 0000000000000000
>   RDX: 000000000000cee6 RSI: 0000000000000000 RDI: ffff8804037c3000
>   RBP: ffff8803bc1c7d88 R08: 000000000047b69a R09: 0000000000000000
>   R10: ffffffffa06de187 R11: 0000000000003246 R12: ffff880403d74540
>   R13: 0000000000000003 R14: ffff8804037c3000 R15: ffff8803bc32eac8
>   FS:  00007f7f4753a900(0000) GS:ffff88041ec80000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 0000000000000000 CR3: 00000003bc240000 CR4: 00000000001407e0
>   Stack:
>    ffffffffa06e06da ffff8803bc1c7d38 0000000000000000 0000000000000082
>    ffff8804037c3188 ffff8803bc1c7d40 0000000000000282 ffff8803bc1c7d68
>    0000000000000106 000000000000cee6 00000000f69756fa ffff880403d74580
>   Call Trace:
>    [<ffffffffa06e06da>] ? vblank_disable_and_save+0x9a/0x200 [drm]
>    [<ffffffffa06e2205>] drm_vblank_cleanup+0x65/0xb0 [drm]
>    [<ffffffffa06cc338>] udl_driver_unload+0x18/0x50 [udl]
>    [<ffffffffa06e3f2d>] drm_dev_unregister+0x2d/0xb0 [drm]
>    [<ffffffffa06e41e7>] drm_put_dev+0x27/0x70 [drm]
>    [<ffffffffa06de237>] drm_release+0x347/0x520 [drm]
>    [<ffffffff811d357f>] __fput+0x9f/0x200
>    [<ffffffff811d372e>] ____fput+0xe/0x10
>    [<ffffffff8108fc87>] task_work_run+0xb7/0xf0
>    [<ffffffff81015d35>] do_notify_resume+0x95/0xa0
>    [<ffffffff8155f060>] int_signal+0x12/0x17
>   Code:  Bad RIP value.
>   RIP  [<          (null)>]           (null)
>    RSP <ffff8803bc1c7d00>
>   CR2: 0000000000000000
>   ---[ end trace e0b184ca053571af ]---
> 
> Reported-by: Thomas Meyer <thomas@...3r.de>
> Link: http://lists.freedesktop.org/archives/dri-devel/2014-December/073652.html
> Signed-off-by: Aaron Plattner <aplattner@...dia.com>
> ---
> Daniel, it looks like your change "[5/5] drm/irq: Don't call
>  ->get_vblank_counter directly from irq_uninstall/cleanup" masks the immediate
> problem, but it's not clear to me whether that's just because I didn't manage to
> trigger any of the new vblank stuff, or whether your change really fixes it.  It
> does seem like these vblank functions are intended to be called unconditionally.

On a quick look udl vblank handling seems to be 100% cargo-cult. It never
calls handle_vblank, which means there's never going to be a useful vblank
timestamp/counter value, but it also doesn't use the -1 trick for
drm_send_vblank_event used to signal that just grabbing a cpu ts is all
that's possible.

I think the right fix here is to ditch both the vblank_init and cleanup
and use -1 in drm_send_vblank_event.
-Daniel
> 
>  drivers/gpu/drm/udl/udl_drv.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index d5728ec85254..5bacb556b0f5 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -16,6 +16,20 @@ static int udl_driver_set_busid(struct drm_device *d, struct drm_master *m)
>  	return 0;
>  }
>  
> +static u32 udl_noop_get_vblank_counter(struct drm_device *dev, int crtc)
> +{
> +	return dev->vblank[crtc].count.counter;
> +}
> +
> +static int udl_noop_enable_vblank(struct drm_device *dev, int crtc)
> +{
> +	return 0;
> +}
> +
> +static void udl_noop_disable_vblank(struct drm_device *dev, int crtc)
> +{
> +}
> +
>  static const struct vm_operations_struct udl_gem_vm_ops = {
>  	.fault = udl_gem_fault,
>  	.open = drm_gem_vm_open,
> @@ -42,6 +56,10 @@ static struct drm_driver driver = {
>  	.unload = udl_driver_unload,
>  	.set_busid = udl_driver_set_busid,
>  
> +	.get_vblank_counter = udl_noop_get_vblank_counter,
> +	.enable_vblank = udl_noop_enable_vblank,
> +	.disable_vblank = udl_noop_disable_vblank,
> +
>  	/* gem hooks */
>  	.gem_free_object = udl_gem_free_object,
>  	.gem_vm_ops = &udl_gem_vm_ops,
> -- 
> 2.3.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists