We should never nest these since in theory kms drivers should know when a pipe is on/off and call the corresponding enable/disable functions for the vblank helper code only once. But for historical reasons (the shared-with-ums version of this code in modeset_pre/post needed to be able to cope with silly userspace that lost track of things) we still have this bit of "robustness" around.
Enforce this with a WARN_ON, preparing to eventually rip out this special handling.
Cc: Yogesh Mohan Marimuthu yogesh.mohan.marimuthu@intel.com Cc: Gaurav K Singh gaurav.k.singh@intel.com Cc: Michel Dänzer michel.daenzer@amd.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_irq.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index f9cc68fbd2a3..3819465abe22 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1219,6 +1219,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) vblank_disable_and_save(dev, crtc); wake_up(&vblank->queue);
+ WARN_ON(vblank->inmodeset); + /* * Prevent subsequent drm_vblank_get() from re-enabling * the vblank interrupt by bumping the refcount. @@ -1318,6 +1320,8 @@ void drm_vblank_on(struct drm_device *dev, int crtc) return;
spin_lock_irqsave(&dev->vbl_lock, irqflags); + WARN_ON(!vblank->inmodeset); + /* Drop our private "prevent drm_vblank_get" refcount */ if (vblank->inmodeset) { atomic_dec(&vblank->refcount);
On Mon, Jun 22, 2015 at 8:02 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
We should never nest these since in theory kms drivers should know when a pipe is on/off and call the corresponding enable/disable functions for the vblank helper code only once. But for historical reasons (the shared-with-ums version of this code in modeset_pre/post needed to be able to cope with silly userspace that lost track of things) we still have this bit of "robustness" around.
Enforce this with a WARN_ON, preparing to eventually rip out this special handling.
This doesn't really provide any context in the WARN_ON itself. It will just result in a splat that looks like a kernel oops, and end users and distribution maintainers will be left scratching their heads.
Could this be done with a printk warning instead, or could you at least provide a pr_warn statement to help people understand why their machine has an oops splat?
josh
Cc: Yogesh Mohan Marimuthu yogesh.mohan.marimuthu@intel.com Cc: Gaurav K Singh gaurav.k.singh@intel.com Cc: Michel Dänzer michel.daenzer@amd.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_irq.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index f9cc68fbd2a3..3819465abe22 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1219,6 +1219,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) vblank_disable_and_save(dev, crtc); wake_up(&vblank->queue);
WARN_ON(vblank->inmodeset);/* * Prevent subsequent drm_vblank_get() from re-enabling * the vblank interrupt by bumping the refcount.@@ -1318,6 +1320,8 @@ void drm_vblank_on(struct drm_device *dev, int crtc) return;
spin_lock_irqsave(&dev->vbl_lock, irqflags);
WARN_ON(!vblank->inmodeset);/* Drop our private "prevent drm_vblank_get" refcount */ if (vblank->inmodeset) { atomic_dec(&vblank->refcount);-- 2.1.4
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 22 Jun 2015, Josh Boyer jwboyer@fedoraproject.org wrote:
On Mon, Jun 22, 2015 at 8:02 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
We should never nest these since in theory kms drivers should know when a pipe is on/off and call the corresponding enable/disable functions for the vblank helper code only once. But for historical reasons (the shared-with-ums version of this code in modeset_pre/post needed to be able to cope with silly userspace that lost track of things) we still have this bit of "robustness" around.
Enforce this with a WARN_ON, preparing to eventually rip out this special handling.
This doesn't really provide any context in the WARN_ON itself. It will just result in a splat that looks like a kernel oops, and end users and distribution maintainers will be left scratching their heads.
Could this be done with a printk warning instead, or could you at least provide a pr_warn statement to help people understand why their machine has an oops splat?
FWIW i915_drv.h has
#define WARN_ON(x) WARN((x), "WARN_ON(" #x ")")
which makes it a little better...
BR, Jani.
josh
Cc: Yogesh Mohan Marimuthu yogesh.mohan.marimuthu@intel.com Cc: Gaurav K Singh gaurav.k.singh@intel.com Cc: Michel Dänzer michel.daenzer@amd.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_irq.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index f9cc68fbd2a3..3819465abe22 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1219,6 +1219,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) vblank_disable_and_save(dev, crtc); wake_up(&vblank->queue);
WARN_ON(vblank->inmodeset);/* * Prevent subsequent drm_vblank_get() from re-enabling * the vblank interrupt by bumping the refcount.@@ -1318,6 +1320,8 @@ void drm_vblank_on(struct drm_device *dev, int crtc) return;
spin_lock_irqsave(&dev->vbl_lock, irqflags);
WARN_ON(!vblank->inmodeset);/* Drop our private "prevent drm_vblank_get" refcount */ if (vblank->inmodeset) { atomic_dec(&vblank->refcount);-- 2.1.4
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 22, 2015 at 9:27 AM, Jani Nikula jani.nikula@linux.intel.com wrote:
On Mon, 22 Jun 2015, Josh Boyer jwboyer@fedoraproject.org wrote:
On Mon, Jun 22, 2015 at 8:02 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
We should never nest these since in theory kms drivers should know when a pipe is on/off and call the corresponding enable/disable functions for the vblank helper code only once. But for historical reasons (the shared-with-ums version of this code in modeset_pre/post needed to be able to cope with silly userspace that lost track of things) we still have this bit of "robustness" around.
Enforce this with a WARN_ON, preparing to eventually rip out this special handling.
This doesn't really provide any context in the WARN_ON itself. It will just result in a splat that looks like a kernel oops, and end users and distribution maintainers will be left scratching their heads.
Could this be done with a printk warning instead, or could you at least provide a pr_warn statement to help people understand why their machine has an oops splat?
FWIW i915_drv.h has
#define WARN_ON(x) WARN((x), "WARN_ON(" #x ")")
which makes it a little better...
Only a little though, and only in i915. This is in the generic DRM code, isn't it? I mean, a DRM developer is certainly helped by that. But an end user seeing this won't know it is because of nested calls. A simple:
dev_warn("Driver has nested vblank calls. This works but should be fixed soon.")
or something similar would at least help people understand what is happening and that their machine isn't actually broken yet.
josh
Cc: Yogesh Mohan Marimuthu yogesh.mohan.marimuthu@intel.com Cc: Gaurav K Singh gaurav.k.singh@intel.com Cc: Michel Dänzer michel.daenzer@amd.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_irq.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index f9cc68fbd2a3..3819465abe22 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1219,6 +1219,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) vblank_disable_and_save(dev, crtc); wake_up(&vblank->queue);
WARN_ON(vblank->inmodeset);/* * Prevent subsequent drm_vblank_get() from re-enabling * the vblank interrupt by bumping the refcount.@@ -1318,6 +1320,8 @@ void drm_vblank_on(struct drm_device *dev, int crtc) return;
spin_lock_irqsave(&dev->vbl_lock, irqflags);
WARN_ON(!vblank->inmodeset);/* Drop our private "prevent drm_vblank_get" refcount */ if (vblank->inmodeset) { atomic_dec(&vblank->refcount);-- 2.1.4
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
On Mon, 22 Jun 2015, Josh Boyer jwboyer@fedoraproject.org wrote:
On Mon, Jun 22, 2015 at 9:27 AM, Jani Nikula jani.nikula@linux.intel.com wrote:
On Mon, 22 Jun 2015, Josh Boyer jwboyer@fedoraproject.org wrote:
On Mon, Jun 22, 2015 at 8:02 AM, Daniel Vetter daniel.vetter@ffwll.ch wrote:
We should never nest these since in theory kms drivers should know when a pipe is on/off and call the corresponding enable/disable functions for the vblank helper code only once. But for historical reasons (the shared-with-ums version of this code in modeset_pre/post needed to be able to cope with silly userspace that lost track of things) we still have this bit of "robustness" around.
Enforce this with a WARN_ON, preparing to eventually rip out this special handling.
This doesn't really provide any context in the WARN_ON itself. It will just result in a splat that looks like a kernel oops, and end users and distribution maintainers will be left scratching their heads.
Could this be done with a printk warning instead, or could you at least provide a pr_warn statement to help people understand why their machine has an oops splat?
FWIW i915_drv.h has
#define WARN_ON(x) WARN((x), "WARN_ON(" #x ")")
which makes it a little better...
Only a little though, and only in i915. This is in the generic DRM code, isn't it?
You're absolutely right, sorry. Agreed with your complaint.
BR, Jani.
On 22 June 2015 at 14:02, Daniel Vetter daniel.vetter@ffwll.ch wrote:
We should never nest these since in theory kms drivers should know when a pipe is on/off and call the corresponding enable/disable functions for the vblank helper code only once. But for historical reasons (the shared-with-ums version of this code in modeset_pre/post needed to be able to cope with silly userspace that lost track of things) we still have this bit of "robustness" around.
Enforce this with a WARN_ON, preparing to eventually rip out this special handling.
Hello,
just wanted to mention that this warning triggered on tegra124 and that I find it a bit disruptive:
WARNING: CPU: 0 PID: 52 at drivers/gpu/drm/drm_irq.c:1323 drm_vblank_on+0x10c/0x130() Modules linked in: CPU: 0 PID: 52 Comm: kworker/0:1 Not tainted 4.1.0-next-20150625ccu-00024-gd8cbda3 #2295 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) Workqueue: events output_poll_execute [<c021b12c>] (unwind_backtrace) from [<c0215388>] (show_stack+0x20/0x24) [<c0215388>] (show_stack) from [<c0a9637c>] (dump_stack+0x8c/0x9c) [<c0a9637c>] (dump_stack) from [<c0256b58>] (warn_slowpath_common+0x94/0xc4) [<c0256b58>] (warn_slowpath_common) from [<c0256c44>] (warn_slowpath_null+0x2c/0x34) [<c0256c44>] (warn_slowpath_null) from [<c0686d38>] (drm_vblank_on+0x10c/0x130) [<c0686d38>] (drm_vblank_on) from [<c0686d84>] (drm_crtc_vblank_on+0x28/0x2c) [<c0686d84>] (drm_crtc_vblank_on) from [<c06cd518>] (tegra_crtc_commit+0x18/0x1c) [<c06cd518>] (tegra_crtc_commit) from [<c067b014>] (drm_atomic_helper_commit_modeset_enables+0xa0/0x198) [<c067b014>] (drm_atomic_helper_commit_modeset_enables) from [<c06cb068>] (tegra_atomic_complete.isra.1+0x38/0x5c) [<c06cb068>] (tegra_atomic_complete.isra.1) from [<c06cb0f4>] (tegra_atomic_commit+0x68/0x98) [<c06cb0f4>] (tegra_atomic_commit) from [<c06a1690>] (drm_atomic_commit+0x54/0x74) [<c06a1690>] (drm_atomic_commit) from [<c067b6ec>] (drm_atomic_helper_set_config+0x1b4/0x414) [<c067b6ec>] (drm_atomic_helper_set_config) from [<c068f43c>] (drm_mode_set_config_internal+0x68/0xe4) [<c068f43c>] (drm_mode_set_config_internal) from [<c067d34c>] (restore_fbdev_mode+0xe8/0x108) [<c067d34c>] (restore_fbdev_mode) from [<c067f390>] (drm_fb_helper_restore_fbdev_mode_unlocked+0x2c/0x68) [<c067f390>] (drm_fb_helper_restore_fbdev_mode_unlocked) from [<c067f3f4>] (drm_fb_helper_set_par+0x28/0x44) [<c067f3f4>] (drm_fb_helper_set_par) from [<c067f328>] (drm_fb_helper_hotplug_event+0xcc/0x108) [<c067f328>] (drm_fb_helper_hotplug_event) from [<c06cce98>] (tegra_fb_output_poll_changed+0x28/0x2c) [<c06cce98>] (tegra_fb_output_poll_changed) from [<c0674228>] (drm_kms_helper_hotplug_event+0x34/0x38) [<c0674228>] (drm_kms_helper_hotplug_event) from [<c06743dc>] (output_poll_execute+0x158/0x198) [<c06743dc>] (output_poll_execute) from [<c026d830>] (process_one_work+0x154/0x48c) [<c026d830>] (process_one_work) from [<c026dbbc>] (worker_thread+0x54/0x568) [<c026dbbc>] (worker_thread) from [<c02739ec>] (kthread+0xec/0x104) [<c02739ec>] (kthread) from [<c0211228>] (ret_from_fork+0x14/0x2c)
Regards,
Tomeu
Cc: Yogesh Mohan Marimuthu yogesh.mohan.marimuthu@intel.com Cc: Gaurav K Singh gaurav.k.singh@intel.com Cc: Michel Dänzer michel.daenzer@amd.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_irq.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index f9cc68fbd2a3..3819465abe22 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1219,6 +1219,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) vblank_disable_and_save(dev, crtc); wake_up(&vblank->queue);
WARN_ON(vblank->inmodeset);/* * Prevent subsequent drm_vblank_get() from re-enabling * the vblank interrupt by bumping the refcount.@@ -1318,6 +1320,8 @@ void drm_vblank_on(struct drm_device *dev, int crtc) return;
spin_lock_irqsave(&dev->vbl_lock, irqflags);
WARN_ON(!vblank->inmodeset);/* Drop our private "prevent drm_vblank_get" refcount */ if (vblank->inmodeset) { atomic_dec(&vblank->refcount);-- 2.1.4
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jani Nikula -
Josh Boyer -
Tomeu Vizoso