diff mbox

drm: Don't allow interruptions when opening debugfs/crc

Message ID 20170407111712.13962-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson April 7, 2017, 11:17 a.m. UTC 
The code does not like to be interrupted when waiting for the first
vblank after opening a debugfs/crc channel, so don't.

[66285.716870] WARNING: CPU: 1 PID: 16615 at drivers/gpu/drm/drm_debugfs_crc.c:185 crtc_crc_open+0x1d0/0x1f0 [drm]
[66285.716877] Modules linked in: i915 intel_powerclamp crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel cryptd intel_gtt i2c_algo_bit lpc_ich mfd_core drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops prime_numbers drm video button autofs4 sd_mod ahci libahci libata i2c_i801 scsi_mod i2c_designware_platform i2c_designware_core i2c_core
[66285.716929] CPU: 1 PID: 16615 Comm: kms_frontbuffer Not tainted 4.11.0-rc5+ #7
[66285.716935] Hardware name: GIGABYTE GB-BXBT-1900/MZBAYAB-00, BIOS F8 03/02/2016
[66285.716941] Call Trace:
[66285.716955]  dump_stack+0x4d/0x6f
[66285.716966]  __warn+0xc1/0xe0
[66285.716975]  warn_slowpath_null+0x18/0x20
[66285.717004]  crtc_crc_open+0x1d0/0x1f0 [drm]
[66285.717014]  ? wake_atomic_t_function+0x50/0x50
[66285.717024]  full_proxy_open+0xf0/0x1b0
[66285.717032]  ? full_proxy_release+0x80/0x80
[66285.717042]  do_dentry_open.isra.17+0x14b/0x2d0
[66285.717051]  vfs_open+0x42/0x60
[66285.717064]  path_openat+0x5e7/0x13d0
[66285.717074]  ? refcount_dec_and_test+0x11/0x20
[66285.717081]  ? down_read+0xd/0x30
[66285.717087]  do_filp_open+0x85/0xf0
[66285.717093]  ? __vfs_write+0x23/0x120
[66285.717100]  ? __alloc_fd+0x3a/0x170
[66285.717107]  do_sys_open+0x11e/0x1f0
[66285.717113]  ? do_sys_open+0x11e/0x1f0
[66285.717119]  SyS_openat+0xf/0x20
[66285.717125]  entry_SYSCALL_64_fastpath+0x17/0x98
[66285.717131] RIP: 0033:0x7f5f2235146a
[66285.717135] RSP: 002b:00007ffd892e6bc0 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
[66285.717142] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f5f2235146a
[66285.717147] RDX: 0000000000000000 RSI: 00007ffd892e6c40 RDI: 0000000000000006
[66285.717151] RBP: 00007ffd892e6b20 R08: 0000000000000000 R09: 000000000000000f
[66285.717156] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
[66285.717161] R13: 00007ffd892e6b10 R14: 0000000000000004 R15: 00000000007e61f4

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100610
Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Tomeu Vizoso April 7, 2017, 11:55 a.m. UTC | #1 
On 04/07/2017 01:17 PM, Chris Wilson wrote:
> The code does not like to be interrupted when waiting for the first
> vblank after opening a debugfs/crc channel, so don't.
> 
> [66285.716870] WARNING: CPU: 1 PID: 16615 at drivers/gpu/drm/drm_debugfs_crc.c:185 crtc_crc_open+0x1d0/0x1f0 [drm]
> [66285.716877] Modules linked in: i915 intel_powerclamp crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel cryptd intel_gtt i2c_algo_bit lpc_ich mfd_core drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops prime_numbers drm video button autofs4 sd_mod ahci libahci libata i2c_i801 scsi_mod i2c_designware_platform i2c_designware_core i2c_core
> [66285.716929] CPU: 1 PID: 16615 Comm: kms_frontbuffer Not tainted 4.11.0-rc5+ #7
> [66285.716935] Hardware name: GIGABYTE GB-BXBT-1900/MZBAYAB-00, BIOS F8 03/02/2016
> [66285.716941] Call Trace:
> [66285.716955]  dump_stack+0x4d/0x6f
> [66285.716966]  __warn+0xc1/0xe0
> [66285.716975]  warn_slowpath_null+0x18/0x20
> [66285.717004]  crtc_crc_open+0x1d0/0x1f0 [drm]
> [66285.717014]  ? wake_atomic_t_function+0x50/0x50
> [66285.717024]  full_proxy_open+0xf0/0x1b0
> [66285.717032]  ? full_proxy_release+0x80/0x80
> [66285.717042]  do_dentry_open.isra.17+0x14b/0x2d0
> [66285.717051]  vfs_open+0x42/0x60
> [66285.717064]  path_openat+0x5e7/0x13d0
> [66285.717074]  ? refcount_dec_and_test+0x11/0x20
> [66285.717081]  ? down_read+0xd/0x30
> [66285.717087]  do_filp_open+0x85/0xf0
> [66285.717093]  ? __vfs_write+0x23/0x120
> [66285.717100]  ? __alloc_fd+0x3a/0x170
> [66285.717107]  do_sys_open+0x11e/0x1f0
> [66285.717113]  ? do_sys_open+0x11e/0x1f0
> [66285.717119]  SyS_openat+0xf/0x20
> [66285.717125]  entry_SYSCALL_64_fastpath+0x17/0x98
> [66285.717131] RIP: 0033:0x7f5f2235146a
> [66285.717135] RSP: 002b:00007ffd892e6bc0 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
> [66285.717142] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f5f2235146a
> [66285.717147] RDX: 0000000000000000 RSI: 00007ffd892e6c40 RDI: 0000000000000006
> [66285.717151] RBP: 00007ffd892e6b20 R08: 0000000000000000 R09: 000000000000000f
> [66285.717156] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
> [66285.717161] R13: 00007ffd892e6b10 R14: 0000000000000004 R15: 00000000007e61f4
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100610
> Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Sounds good to me, there isn't any good reason for the wait to be
interruptible.

Thanks,

Tomeu

> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 1722d8f21449..aa13e734c9e5 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -177,13 +177,9 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>  	 * guess when this particular piece of HW will be ready to start
>  	 * generating CRCs.
>  	 */
> -	ret = wait_event_interruptible_lock_irq(crc->wq,
> -						crtc_crc_data_count(crc),
> -						crc->lock);
> +	wait_event_lock_irq(crc->wq, crtc_crc_data_count(crc), crc->lock);
>  	spin_unlock_irq(&crc->lock);
>  
> -	WARN_ON(ret);
> -
>  	return 0;
>  
>  err_disable:
>
Ville Syrjala April 7, 2017, 1:08 p.m. UTC | #2 
On Fri, Apr 07, 2017 at 12:17:12PM +0100, Chris Wilson wrote:
> The code does not like to be interrupted when waiting for the first
> vblank after opening a debugfs/crc channel, so don't.
> 
> [66285.716870] WARNING: CPU: 1 PID: 16615 at drivers/gpu/drm/drm_debugfs_crc.c:185 crtc_crc_open+0x1d0/0x1f0 [drm]
> [66285.716877] Modules linked in: i915 intel_powerclamp crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel cryptd intel_gtt i2c_algo_bit lpc_ich mfd_core drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops prime_numbers drm video button autofs4 sd_mod ahci libahci libata i2c_i801 scsi_mod i2c_designware_platform i2c_designware_core i2c_core
> [66285.716929] CPU: 1 PID: 16615 Comm: kms_frontbuffer Not tainted 4.11.0-rc5+ #7
> [66285.716935] Hardware name: GIGABYTE GB-BXBT-1900/MZBAYAB-00, BIOS F8 03/02/2016
> [66285.716941] Call Trace:
> [66285.716955]  dump_stack+0x4d/0x6f
> [66285.716966]  __warn+0xc1/0xe0
> [66285.716975]  warn_slowpath_null+0x18/0x20
> [66285.717004]  crtc_crc_open+0x1d0/0x1f0 [drm]
> [66285.717014]  ? wake_atomic_t_function+0x50/0x50
> [66285.717024]  full_proxy_open+0xf0/0x1b0
> [66285.717032]  ? full_proxy_release+0x80/0x80
> [66285.717042]  do_dentry_open.isra.17+0x14b/0x2d0
> [66285.717051]  vfs_open+0x42/0x60
> [66285.717064]  path_openat+0x5e7/0x13d0
> [66285.717074]  ? refcount_dec_and_test+0x11/0x20
> [66285.717081]  ? down_read+0xd/0x30
> [66285.717087]  do_filp_open+0x85/0xf0
> [66285.717093]  ? __vfs_write+0x23/0x120
> [66285.717100]  ? __alloc_fd+0x3a/0x170
> [66285.717107]  do_sys_open+0x11e/0x1f0
> [66285.717113]  ? do_sys_open+0x11e/0x1f0
> [66285.717119]  SyS_openat+0xf/0x20
> [66285.717125]  entry_SYSCALL_64_fastpath+0x17/0x98
> [66285.717131] RIP: 0033:0x7f5f2235146a
> [66285.717135] RSP: 002b:00007ffd892e6bc0 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
> [66285.717142] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f5f2235146a
> [66285.717147] RDX: 0000000000000000 RSI: 00007ffd892e6c40 RDI: 0000000000000006
> [66285.717151] RBP: 00007ffd892e6b20 R08: 0000000000000000 R09: 000000000000000f
> [66285.717156] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
> [66285.717161] R13: 00007ffd892e6b10 R14: 0000000000000004 R15: 00000000007e61f4
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100610
> Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 1722d8f21449..aa13e734c9e5 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -177,13 +177,9 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>  	 * guess when this particular piece of HW will be ready to start
>  	 * generating CRCs.
>  	 */
> -	ret = wait_event_interruptible_lock_irq(crc->wq,
> -						crtc_crc_data_count(crc),
> -						crc->lock);
> +	wait_event_lock_irq(crc->wq, crtc_crc_data_count(crc), crc->lock);

What if the crc never gets produced? The first read will block if
the user asked it to block, so I don't really see the point of this
wait at all.

Seems to me that the crc thing needs to be pimped work better in cases
where the crtc is disabled. Currently it's kinda trying to hide a bit
too much to be useful for observing the hardware behaviour during crtc
enable/disable. I have definitely used it in the past to observe that,
but I'm not sure the current thing really works for that anymore.

>  	spin_unlock_irq(&crc->lock);
>  
> -	WARN_ON(ret);
> -
>  	return 0;
>  
>  err_disable:
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter April 7, 2017, 4:42 p.m. UTC | #3 
On Fri, Apr 07, 2017 at 01:55:00PM +0200, Tomeu Vizoso wrote:
> On 04/07/2017 01:17 PM, Chris Wilson wrote:
> > The code does not like to be interrupted when waiting for the first
> > vblank after opening a debugfs/crc channel, so don't.
> > 
> > [66285.716870] WARNING: CPU: 1 PID: 16615 at drivers/gpu/drm/drm_debugfs_crc.c:185 crtc_crc_open+0x1d0/0x1f0 [drm]
> > [66285.716877] Modules linked in: i915 intel_powerclamp crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel cryptd intel_gtt i2c_algo_bit lpc_ich mfd_core drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops prime_numbers drm video button autofs4 sd_mod ahci libahci libata i2c_i801 scsi_mod i2c_designware_platform i2c_designware_core i2c_core
> > [66285.716929] CPU: 1 PID: 16615 Comm: kms_frontbuffer Not tainted 4.11.0-rc5+ #7
> > [66285.716935] Hardware name: GIGABYTE GB-BXBT-1900/MZBAYAB-00, BIOS F8 03/02/2016
> > [66285.716941] Call Trace:
> > [66285.716955]  dump_stack+0x4d/0x6f
> > [66285.716966]  __warn+0xc1/0xe0
> > [66285.716975]  warn_slowpath_null+0x18/0x20
> > [66285.717004]  crtc_crc_open+0x1d0/0x1f0 [drm]
> > [66285.717014]  ? wake_atomic_t_function+0x50/0x50
> > [66285.717024]  full_proxy_open+0xf0/0x1b0
> > [66285.717032]  ? full_proxy_release+0x80/0x80
> > [66285.717042]  do_dentry_open.isra.17+0x14b/0x2d0
> > [66285.717051]  vfs_open+0x42/0x60
> > [66285.717064]  path_openat+0x5e7/0x13d0
> > [66285.717074]  ? refcount_dec_and_test+0x11/0x20
> > [66285.717081]  ? down_read+0xd/0x30
> > [66285.717087]  do_filp_open+0x85/0xf0
> > [66285.717093]  ? __vfs_write+0x23/0x120
> > [66285.717100]  ? __alloc_fd+0x3a/0x170
> > [66285.717107]  do_sys_open+0x11e/0x1f0
> > [66285.717113]  ? do_sys_open+0x11e/0x1f0
> > [66285.717119]  SyS_openat+0xf/0x20
> > [66285.717125]  entry_SYSCALL_64_fastpath+0x17/0x98
> > [66285.717131] RIP: 0033:0x7f5f2235146a
> > [66285.717135] RSP: 002b:00007ffd892e6bc0 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
> > [66285.717142] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f5f2235146a
> > [66285.717147] RDX: 0000000000000000 RSI: 00007ffd892e6c40 RDI: 0000000000000006
> > [66285.717151] RBP: 00007ffd892e6b20 R08: 0000000000000000 R09: 000000000000000f
> > [66285.717156] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
> > [66285.717161] R13: 00007ffd892e6b10 R14: 0000000000000004 R15: 00000000007e61f4
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100610
> > Fixes: e8fa5671183c ("drm: crc: Wait for a frame before returning from open()")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> 
> Sounds good to me, there isn't any good reason for the wait to be
> interruptible.

Applied to drm-misc-next, thanks.
-Daniel

> 
> Thanks,
> 
> Tomeu
> 
> > ---
> >  drivers/gpu/drm/drm_debugfs_crc.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> > index 1722d8f21449..aa13e734c9e5 100644
> > --- a/drivers/gpu/drm/drm_debugfs_crc.c
> > +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> > @@ -177,13 +177,9 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
> >  	 * guess when this particular piece of HW will be ready to start
> >  	 * generating CRCs.
> >  	 */
> > -	ret = wait_event_interruptible_lock_irq(crc->wq,
> > -						crtc_crc_data_count(crc),
> > -						crc->lock);
> > +	wait_event_lock_irq(crc->wq, crtc_crc_data_count(crc), crc->lock);
> >  	spin_unlock_irq(&crc->lock);
> >  
> > -	WARN_ON(ret);
> > -
> >  	return 0;
> >  
> >  err_disable:
> > 
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 1722d8f21449..aa13e734c9e5 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -177,13 +177,9 @@  static int crtc_crc_open(struct inode *inode, struct file *filep)
 	 * guess when this particular piece of HW will be ready to start
 	 * generating CRCs.
 	 */
-	ret = wait_event_interruptible_lock_irq(crc->wq,
-						crtc_crc_data_count(crc),
-						crc->lock);
+	wait_event_lock_irq(crc->wq, crtc_crc_data_count(crc), crc->lock);
 	spin_unlock_irq(&crc->lock);
 
-	WARN_ON(ret);
-
 	return 0;
 
 err_disable: