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

[Jani Nikula <jani.nikula@linux.intel.com>]

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