[PATCH] drm/vblank: WARN_ON nested use of drm_vblank_on/off

[PATCH] drm/vblank: WARN_ON nested use of drm_vblank_on/off

[Daniel Vetter]

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);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/vblank: WARN_ON nested use of drm_vblank_on/off

[Josh Boyer]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/vblank: WARN_ON nested use of drm_vblank_on/off

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/vblank: WARN_ON nested use of drm_vblank_on/off

[Josh Boyer]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/vblank: WARN_ON nested use of drm_vblank_on/off

[Jani Nikula]

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.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vblank: WARN_ON nested use of drm_vblank_on/off

[Tomeu Vizoso]

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 mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel