| Message ID | 1434974573-19528-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);
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(+)