| Message ID | 1303884659-739-1-git-send-email-christopher.halse.rogers@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote: > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > This is the least-bad behaviour. It means that we signal the > vblank event before it actually happens, but since we're disabling > vblanks there's no guarantee that it will *ever* happen otherwise. This may indeed be the best we can do for events that are pending when the CRTC is disabled[0], but I can't see anything that would prevent new events from getting scheduled (or synchronous vblank waits from timing out) while the CRTC is disabled? [0] Though it might unnecessarily send events prematurely when the CRTC is just disabled temporarily, e.g. as part of a modeset. Also, this patch won't seem to help at all for other drivers which don't call drm_vblank_off() directly when disabling a CRTC. Maybe it would be possible to move those calls to core code, and/or only force sending out events when the CRTC isn't just getting disabled temporarily.
On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote: > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote: > > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > > > This is the least-bad behaviour. It means that we signal the > > vblank event before it actually happens, but since we're disabling > > vblanks there's no guarantee that it will *ever* happen otherwise. > > This may indeed be the best we can do for events that are pending when > the CRTC is disabled[0], but I can't see anything that would prevent new > events from getting scheduled (or synchronous vblank waits from timing > out) while the CRTC is disabled? > > [0] Though it might unnecessarily send events prematurely when the CRTC > is just disabled temporarily, e.g. as part of a modeset. > > > Also, this patch won't seem to help at all for other drivers which don't > call drm_vblank_off() directly when disabling a CRTC. This is true. On the other hand, the other drivers don't wedge the vblank code into a state where vblanks cannot be re-enabled. So it's only a problem when disabling one of 2+ monitors on those drivers, whereas it's easily triggerable on single monitor systems on intel. > > Maybe it would be possible to move those calls to core code, and/or only > force sending out events when the CRTC isn't just getting disabled > temporarily. > As in: have the core modesetting code call drm_vblank_off before making the driver-specific calls when disabling a crtc? I'll look into it - that would appear to be a more general solution. It would be nice if the OML_sync_control had been written with a less than laser-like focus on the single monitor case, too. :/
On Mit, 2011-04-27 at 18:58 +1000, Christopher James Halse Rogers wrote: > On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote: > > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote: > > > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > > > > > This is the least-bad behaviour. It means that we signal the > > > vblank event before it actually happens, but since we're disabling > > > vblanks there's no guarantee that it will *ever* happen otherwise. > > > > This may indeed be the best we can do for events that are pending when > > the CRTC is disabled[0], but I can't see anything that would prevent new > > events from getting scheduled (or synchronous vblank waits from timing > > out) while the CRTC is disabled? > > > > [0] Though it might unnecessarily send events prematurely when the CRTC > > is just disabled temporarily, e.g. as part of a modeset. > > > > > > Also, this patch won't seem to help at all for other drivers which don't > > call drm_vblank_off() directly when disabling a CRTC. > > This is true. On the other hand, the other drivers don't wedge the > vblank code into a state where vblanks cannot be re-enabled. So it's > only a problem when disabling one of 2+ monitors on those drivers, And with DPMS? > whereas it's easily triggerable on single monitor systems on intel. Anyway, this patch is probably good at least as a preliminary fix for the inconsistent vblank state with the intel driver. > > Maybe it would be possible to move those calls to core code, and/or only > > force sending out events when the CRTC isn't just getting disabled > > temporarily. > > > > As in: have the core modesetting code call drm_vblank_off before making > the driver-specific calls when disabling a crtc? I'll look into it - > that would appear to be a more general solution. Yeah, something like that, thanks.
On Wed, 2011-04-27 at 11:08 +0200, Michel Dänzer wrote: > On Mit, 2011-04-27 at 18:58 +1000, Christopher James Halse Rogers > wrote: > > On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote: > > > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote: > > > > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > > > > > > > This is the least-bad behaviour. It means that we signal the > > > > vblank event before it actually happens, but since we're disabling > > > > vblanks there's no guarantee that it will *ever* happen otherwise. > > > > > > This may indeed be the best we can do for events that are pending when > > > the CRTC is disabled[0], but I can't see anything that would prevent new > > > events from getting scheduled (or synchronous vblank waits from timing > > > out) while the CRTC is disabled? > > > > > > [0] Though it might unnecessarily send events prematurely when the CRTC > > > is just disabled temporarily, e.g. as part of a modeset. > > > > > > > > > Also, this patch won't seem to help at all for other drivers which don't > > > call drm_vblank_off() directly when disabling a CRTC. > > > > This is true. On the other hand, the other drivers don't wedge the > > vblank code into a state where vblanks cannot be re-enabled. So it's > > only a problem when disabling one of 2+ monitors on those drivers, > > And with DPMS? > Possibly. Since vblanks aren't wedged off in this case it's more likely that the user turning the monitors back on will result in a vblank irq, which will kick everything back into correct operation. I've not managed to trigger this on my radeon system in the same way as my intel systems, but I haven't stressed it as hard either.
On Wed, 2011-04-27 at 11:08 +0200, Michel Dänzer wrote: > On Mit, 2011-04-27 at 18:58 +1000, Christopher James Halse Rogers > wrote: > > On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote: > > > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote: > > > > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > > > > > > > This is the least-bad behaviour. It means that we signal the > > > > vblank event before it actually happens, but since we're disabling > > > > vblanks there's no guarantee that it will *ever* happen otherwise. > > > > > > This may indeed be the best we can do for events that are pending when > > > the CRTC is disabled[0], but I can't see anything that would prevent new > > > events from getting scheduled (or synchronous vblank waits from timing > > > out) while the CRTC is disabled? > > > > > > [0] Though it might unnecessarily send events prematurely when the CRTC > > > is just disabled temporarily, e.g. as part of a modeset. > > > > > > > > > Also, this patch won't seem to help at all for other drivers which don't > > > call drm_vblank_off() directly when disabling a CRTC. > > > > This is true. On the other hand, the other drivers don't wedge the > > vblank code into a state where vblanks cannot be re-enabled. So it's > > only a problem when disabling one of 2+ monitors on those drivers, > > And with DPMS? > > > whereas it's easily triggerable on single monitor systems on intel. > > Anyway, this patch is probably good at least as a preliminary fix for > the inconsistent vblank state with the intel driver. > > > > > Maybe it would be possible to move those calls to core code, and/or only > > > force sending out events when the CRTC isn't just getting disabled > > > temporarily. > > > > > > > As in: have the core modesetting code call drm_vblank_off before making > > the driver-specific calls when disabling a crtc? I'll look into it - > > that would appear to be a more general solution. > > Yeah, something like that, thanks. > Ok. This appears to be surprisingly difficult. Particularly, the vblank code indexes crtcs based on the driver-private numbering, and there doesn't appear to be a generic interface to grab this number; Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses something different. I'll see what I can come up with, but I don't think I'm sufficiently familiar with the kms code to quickly come up with a nice API.
On Wed, 27 Apr 2011 16:10:57 +1000 christopher.halse.rogers@canonical.com wrote: > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > This is the least-bad behaviour. It means that we signal the > vblank event before it actually happens, but since we're disabling > vblanks there's no guarantee that it will *ever* happen otherwise. > > This prevents GL applications which use WaitMSC from hanging > indefinitely. > > Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > --- > drivers/gpu/drm/drm_irq.c | 23 +++++++++++++++++++++++ > 1 files changed, 23 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 741457b..a1f12cb 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -932,11 +932,34 @@ EXPORT_SYMBOL(drm_vblank_put); > > void drm_vblank_off(struct drm_device *dev, int crtc) > { > + struct drm_pending_vblank_event *e, *t; > + struct timeval now; > unsigned long irqflags; > + unsigned int seq; > > spin_lock_irqsave(&dev->vbl_lock, irqflags); > vblank_disable_and_save(dev, crtc); > DRM_WAKEUP(&dev->vbl_queue[crtc]); > + > + /* Send any queued vblank events, lest the natives grow disquiet */ > + seq = drm_vblank_count_and_time(dev, crtc, &now); > + list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) { > + if (e->pipe != crtc) > + continue; > + DRM_DEBUG("Sending premature vblank event on disable: \ > + wanted %d, current %d\n", > + e->event.sequence, seq); > + > + e->event.sequence = seq; > + e->event.tv_sec = now.tv_sec; > + e->event.tv_usec = now.tv_usec; > + drm_vblank_put(dev, e->pipe); > + list_move_tail(&e->base.link, &e->base.file_priv->event_list); > + wake_up_interruptible(&e->base.file_priv->event_wait); > + trace_drm_vblank_event_delivered(e->base.pid, e->pipe, > + e->event.sequence); > + } > + > spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > } > EXPORT_SYMBOL(drm_vblank_off); Yeah, this matches what we do for the blocking waits, and it's probably a good idea. Userspace can decide what to do with apps running against a disabled CRTC. Would be nice to share the code with drm_handle_vblank_events though somehow. Looks like e->event.sequence = seq; e->event.tv_sec = now.tv_sec; e->event.tv_usec = now.tv_usec; drm_vblank_put(dev, e->pipe); list_move_tail(&e->base.link, &e->base.file_priv->event_list); wake_up_interruptible(&e->base.file_priv->event_wait); trace_drm_vblank_event_delivered(e->base.pid, e->pipe, e->event.sequence); could be pulled out into a separate function for both to use.
On Wed, 27 Apr 2011 10:32:36 +0200 Michel Dänzer <michel@daenzer.net> wrote: > On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote: > > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > > > This is the least-bad behaviour. It means that we signal the > > vblank event before it actually happens, but since we're disabling > > vblanks there's no guarantee that it will *ever* happen otherwise. > > This may indeed be the best we can do for events that are pending when > the CRTC is disabled[0], but I can't see anything that would prevent new > events from getting scheduled (or synchronous vblank waits from timing > out) while the CRTC is disabled? > > [0] Though it might unnecessarily send events prematurely when the CRTC > is just disabled temporarily, e.g. as part of a modeset. We should return -EINVAL in that case from drm_wait_vblank due to drm_vblank_get failing (i.e. the driver enable_vblank hook should fail if the corresponding crtc is off). At least that's how it's supposed to work.
On Thu, 28 Apr 2011 18:09:58 +1000 Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> wrote: > Ok. This appears to be surprisingly difficult. Particularly, the > vblank code indexes crtcs based on the driver-private numbering, and > there doesn't appear to be a generic interface to grab this number; > Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses > something different. > > I'll see what I can come up with, but I don't think I'm sufficiently > familiar with the kms code to quickly come up with a nice API. Yeah, unfortunately the vblank code pre-dates the KMS code by quite a bit, so the IDs don't match. But with the events emitted as in your previous patch, this should be a harder case to hit, but it sounds like we need to make sure the flip waits are dealt with too to avoid this from triggering at all on Intel. Or did you see other cases where the refcount was nonzero at this point? Thanks,
On Thu, 28 Apr 2011 13:46:30 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Thu, 28 Apr 2011 18:09:58 +1000 > Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > wrote: > > Ok. This appears to be surprisingly difficult. Particularly, the > > vblank code indexes crtcs based on the driver-private numbering, and > > there doesn't appear to be a generic interface to grab this number; > > Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses > > something different. > > > > I'll see what I can come up with, but I don't think I'm sufficiently > > familiar with the kms code to quickly come up with a nice API. > > Yeah, unfortunately the vblank code pre-dates the KMS code by quite a > bit, so the IDs don't match. > > But with the events emitted as in your previous patch, this should be a > harder case to hit, but it sounds like we need to make sure the flip > waits are dealt with too to avoid this from triggering at all on > Intel. Or did you see other cases where the refcount was nonzero at > this point? Actually it looks like we wait for any outstanding flips before disabling so that seems correct (though I didn't check the locking, it's possible we could race a completion and a new flip if the flip ioctl and the dpms code don't exclude each other).
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 741457b..a1f12cb 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -932,11 +932,34 @@ EXPORT_SYMBOL(drm_vblank_put); void drm_vblank_off(struct drm_device *dev, int crtc) { + struct drm_pending_vblank_event *e, *t; + struct timeval now; unsigned long irqflags; + unsigned int seq; spin_lock_irqsave(&dev->vbl_lock, irqflags); vblank_disable_and_save(dev, crtc); DRM_WAKEUP(&dev->vbl_queue[crtc]); + + /* Send any queued vblank events, lest the natives grow disquiet */ + seq = drm_vblank_count_and_time(dev, crtc, &now); + list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) { + if (e->pipe != crtc) + continue; + DRM_DEBUG("Sending premature vblank event on disable: \ + wanted %d, current %d\n", + e->event.sequence, seq); + + e->event.sequence = seq; + e->event.tv_sec = now.tv_sec; + e->event.tv_usec = now.tv_usec; + drm_vblank_put(dev, e->pipe); + list_move_tail(&e->base.link, &e->base.file_priv->event_list); + wake_up_interruptible(&e->base.file_priv->event_wait); + trace_drm_vblank_event_delivered(e->base.pid, e->pipe, + e->event.sequence); + } + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } EXPORT_SYMBOL(drm_vblank_off);