[Intel-gfx] [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 at linux.intel.com> wrote:
> On Mon, 22 Jun 2015, Josh Boyer <jwboyer at fedoraproject.org> wrote:
>> On Mon, Jun 22, 2015 at 8:02 AM, Daniel Vetter <daniel.vetter at 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 at intel.com>
>>> Cc: Gaurav K Singh <gaurav.k.singh at intel.com>
>>> Cc: Michel Dänzer <michel.daenzer at amd.com>
>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter at 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 at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center