| Message ID | 1393009415-27651-6-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote: > > We can kill of the drm_vblank_{pre,post}_modeset() calls since those are > there simply to make drm_vblank_get() fail during a modeset. Actually, their original purpose was to keep the DRM vblank counter consistent across modesets, assuming the modeset resets the hardware vblank counter.
On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote: > On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote: > > > > We can kill of the drm_vblank_{pre,post}_modeset() calls since those are > > there simply to make drm_vblank_get() fail during a modeset. > > Actually, their original purpose was to keep the DRM vblank counter > consistent across modesets, assuming the modeset resets the hardware > vblank counter. I see. Well, actually I really don't. The code is too funky for me to tell what it actually ends up doing. The obvious way would be to resample the hardware counter at drm_vblank_post_modeset(), which the code certainly doesn't do. But maybe it did something sensible in the past.
On Mon, 2014-02-24 at 14:11 +0200, Ville Syrjälä wrote: > On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote: > > On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote: > > > > > > We can kill of the drm_vblank_{pre,post}_modeset() calls since those are > > > there simply to make drm_vblank_get() fail during a modeset. > > > > Actually, their original purpose was to keep the DRM vblank counter > > consistent across modesets, assuming the modeset resets the hardware > > vblank counter. > > I see. Well, actually I really don't. The code is too funky for me to > tell what it actually ends up doing. The obvious way would be to > resample the hardware counter at drm_vblank_post_modeset(), which the > code certainly doesn't do. But maybe it did something sensible in the > past. When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value. But the vblank code has changed a lot since then, not sure it still works like that.
On Tue, 25 Feb 2014 11:58:26 +0900 Michel Dänzer <michel@daenzer.net> wrote: > On Mon, 2014-02-24 at 14:11 +0200, Ville Syrjälä wrote: > > On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote: > > > On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote: > > > > > > > > We can kill of the drm_vblank_{pre,post}_modeset() calls since those are > > > > there simply to make drm_vblank_get() fail during a modeset. > > > > > > Actually, their original purpose was to keep the DRM vblank counter > > > consistent across modesets, assuming the modeset resets the hardware > > > vblank counter. > > > > I see. Well, actually I really don't. The code is too funky for me to > > tell what it actually ends up doing. The obvious way would be to > > resample the hardware counter at drm_vblank_post_modeset(), which the > > code certainly doesn't do. But maybe it did something sensible in the > > past. > > When the pre/post-modeset hooks were originally added, it worked like > this: the pre-modeset hook enabled the vblank interrupt, which updated > the DRM vblank counter from the driver/HW counter. The post-modeset hook > disabled the vblank interrupt again, which recorded the post-modeset > driver/HW counter value. > > But the vblank code has changed a lot since then, not sure it still > works like that. Yeah it's changed a bit. I think Rob added the latest bits there. They were originally in place to support both UMS and KMS drivers, which is as ugly as it sounds. As long as nothing breaks (vblank vs dpms, vblank vs modeset, vblank vs high precision timestamps, vblank vs refcount) I think your code is ok. Cc'ing Mario for another opinion too. This makes me nervous but it seems ok. I think you should update the docbook (with examples) as well so other driver writers will know how to use this stuff. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Fri, Feb 21, 2014 at 09:03:35PM +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Tell the drm core vblank code to reject drm_vblank_get()s only between > drm_vblank_off() and drm_vblank_on() calls, and sprinkle the appropriate > drm_vblank_on() calls to the .crtc_enable() hooks. At this time I kept > the off calls in their current position, and added the on calls to the > end of .crtc_enable(). Later on these will be moved inwards a bit to > allow vblank interrupts during plane enable/disable steps. > > We can kill of the drm_vblank_{pre,post}_modeset() calls since those are > there simply to make drm_vblank_get() fail during a modeset. The way > they do it is by grabbing a vblank reference, and after drm_vblank_off() > gets called this will results in drm_vblank_get() failing due to the > elevated refcount while vblank interrupts are disabled. Unfortunately > this means there's no point during modeset where the behaviour can be > restored back to the normal state until the vblank refcount drops to 0. > There's no gurantee of that happening even after the modeset has > completed, so simply dropping the drm_vblank_{pre,post}_modeset() calls > is the best option. The new reject mechanism will take care of things > in a much more consistent and race free manner. > > Testcase: igt/kms_flip/{dpms,modeset}-vs-vblank-race QA hit the new tests and filed a bug. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75593 <snip>
On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote: > On Mon, 2014-02-24 at 14:11 +0200, Ville Syrjälä wrote: > > On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote: > > > On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote: > > > > > > > > We can kill of the drm_vblank_{pre,post}_modeset() calls since those are > > > > there simply to make drm_vblank_get() fail during a modeset. > > > > > > Actually, their original purpose was to keep the DRM vblank counter > > > consistent across modesets, assuming the modeset resets the hardware > > > vblank counter. > > > > I see. Well, actually I really don't. The code is too funky for me to > > tell what it actually ends up doing. The obvious way would be to > > resample the hardware counter at drm_vblank_post_modeset(), which the > > code certainly doesn't do. But maybe it did something sensible in the > > past. > > When the pre/post-modeset hooks were originally added, it worked like > this: the pre-modeset hook enabled the vblank interrupt, which updated > the DRM vblank counter from the driver/HW counter. The post-modeset hook > disabled the vblank interrupt again, which recorded the post-modeset > driver/HW counter value. > > But the vblank code has changed a lot since then, not sure it still > works like that. It still works like that, but there's two fundamental issues with this trick: - There's a race where the vblank state is fubar right between the completion of the modeset and before the first vblank happened. - It doesn't work across suspend/resume since no one re-enables the vblank interrupt. Cheers, Daniel
On Fri, Feb 28, 2014 at 10:56:20AM +0200, Ville Syrjälä wrote: > On Fri, Feb 21, 2014 at 09:03:35PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Tell the drm core vblank code to reject drm_vblank_get()s only between > > drm_vblank_off() and drm_vblank_on() calls, and sprinkle the appropriate > > drm_vblank_on() calls to the .crtc_enable() hooks. At this time I kept > > the off calls in their current position, and added the on calls to the > > end of .crtc_enable(). Later on these will be moved inwards a bit to > > allow vblank interrupts during plane enable/disable steps. > > > > We can kill of the drm_vblank_{pre,post}_modeset() calls since those are > > there simply to make drm_vblank_get() fail during a modeset. The way > > they do it is by grabbing a vblank reference, and after drm_vblank_off() > > gets called this will results in drm_vblank_get() failing due to the > > elevated refcount while vblank interrupts are disabled. Unfortunately > > this means there's no point during modeset where the behaviour can be > > restored back to the normal state until the vblank refcount drops to 0. > > There's no gurantee of that happening even after the modeset has > > completed, so simply dropping the drm_vblank_{pre,post}_modeset() calls > > is the best option. The new reject mechanism will take care of things > > in a much more consistent and race free manner. > > > > Testcase: igt/kms_flip/{dpms,modeset}-vs-vblank-race > > QA hit the new tests and filed a bug. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75593 I have another test request since you've just fixed this bug: Across suspend/resume the vblank state restoring through the pre/post_modeset hacks isn't just racy but flat-out doesn't work. Keith Packard stumbled over this when working on his present extension. So I think since you've (likely) also fixed this, we also should have a testcase for it. -Daniel
Digging out an ooold post of Daniel's... On 04.03.2014 18:13, Daniel Vetter wrote: > On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote: >> >> When the pre/post-modeset hooks were originally added, it worked like >> this: the pre-modeset hook enabled the vblank interrupt, which updated >> the DRM vblank counter from the driver/HW counter. The post-modeset hook >> disabled the vblank interrupt again, which recorded the post-modeset >> driver/HW counter value. >> >> But the vblank code has changed a lot since then, not sure it still >> works like that. > > It still works like that, but there's two fundamental issues with this > trick: > - There's a race where the vblank state is fubar right between the > completion of the modeset and before the first vblank happened. Can you provide more details about that? You mentioned on IRC that sometimes 'bogus' DRM vblank counter values are returned to userspace. The most likely cause of that would be drm_vblank_pre_modeset() being called too late, i.e. after the hardware counter was reset. (Or if you're reducing / eliminating the vblank disable timer, possibly the vblank interrupt getting disabled too early, i.e. before the hardware counter was reset) Speaking of reducing or disabling the vblank disable timer, that should be possible with drm_vblank_pre/post_modeset() as well. > - It doesn't work across suspend/resume since no one re-enables the vblank > interrupt. That sounds like a driver bug to me. The driver should re-enable the hardware interrupt on resume if the vblank interrupt is currently enabled by the DRM core. This seems to work fine for me with radeon. So, I'm afraid I'm still not convinced that the new vblank_off/on() functions and the ever-increasing series of fixes for problems related to them are the right (let alone only) solution for your problems.
On Wed, May 28, 2014 at 06:12:54PM +0900, Michel Dänzer wrote: > > Digging out an ooold post of Daniel's... > > On 04.03.2014 18:13, Daniel Vetter wrote: > > On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote: > >> > >> When the pre/post-modeset hooks were originally added, it worked like > >> this: the pre-modeset hook enabled the vblank interrupt, which updated > >> the DRM vblank counter from the driver/HW counter. The post-modeset hook > >> disabled the vblank interrupt again, which recorded the post-modeset > >> driver/HW counter value. > >> > >> But the vblank code has changed a lot since then, not sure it still > >> works like that. > > > > It still works like that, but there's two fundamental issues with this > > trick: > > - There's a race where the vblank state is fubar right between the > > completion of the modeset and before the first vblank happened. > > Can you provide more details about that? You mentioned on IRC that > sometimes 'bogus' DRM vblank counter values are returned to userspace. > The most likely cause of that would be drm_vblank_pre_modeset() being > called too late, i.e. after the hardware counter was reset. (Or if > you're reducing / eliminating the vblank disable timer, possibly the > vblank interrupt getting disabled too early, i.e. before the hardware > counter was reset) The hardware counter reset is a problem: 1. vblank_disable_and_save() updates .last 2. modeset/dpms/suspend (hw counter is reset) 3. drm_vblank_get() -> cur_vblank-.last == garbage The lack of drm_vblank_on() is a problem: 1. drm_vblank_get() 2. drm_vblank_off() 3. modeset/dpms/suspend 4. drm_vblank_get() -> -EINVAL Another issue: 1. drm_vblank_get() 2. drm_vblank_put() 3. disable timer expires which updates .last ... 4. drm_vblank_off() updates .last again 5. modeset/dpms/suspend 6. drm_vblank_get() -> sequence number doesn't account for the time between 3. and 4. I suppose this isn't a big issue, but I don't like leaking implementation details (the timer delay) into the sequence number. Now this last one should actually work with the current drm_vblank_pre_modeset() since it does a drm_vblank_get() which will apply the cur_vblank-.last diff, but it also enables the vblank interrupt which is entirely pointless, and also wrong on Intel hardware (well, if we didn't have drm_vblank_off()). Our docs say that we shouldn't have the vblank interrupt enabled+unmasked while the pipe is off. Anyway it's not a very obvious way to do things. Ie. you're doing the drm_vblank_get() not because you actually want vblank interrupts, but because you want the side effects. I usually prefer straightforward code to magic. > Speaking of reducing or disabling the vblank disable timer, that should > be possible with drm_vblank_pre/post_modeset() as well. I get the impression that you're a bit hung up on the names :) We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me. > > - It doesn't work across suspend/resume since no one re-enables the vblank > > interrupt. > > That sounds like a driver bug to me. The driver should re-enable the > hardware interrupt on resume if the vblank interrupt is currently > enabled by the DRM core. The interrupt is not enabled due to drm_vblank_off(). There's nothing to undo that which is why I added drm_vblank_on().
On 28.05.2014 20:19, Ville Syrjälä wrote: > On Wed, May 28, 2014 at 06:12:54PM +0900, Michel Dänzer wrote: >> >> Digging out an ooold post of Daniel's... >> >> On 04.03.2014 18:13, Daniel Vetter wrote: >>> On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote: >>>> >>>> When the pre/post-modeset hooks were originally added, it worked like >>>> this: the pre-modeset hook enabled the vblank interrupt, which updated >>>> the DRM vblank counter from the driver/HW counter. The post-modeset hook >>>> disabled the vblank interrupt again, which recorded the post-modeset >>>> driver/HW counter value. >>>> >>>> But the vblank code has changed a lot since then, not sure it still >>>> works like that. >>> >>> It still works like that, but there's two fundamental issues with this >>> trick: >>> - There's a race where the vblank state is fubar right between the >>> completion of the modeset and before the first vblank happened. >> >> Can you provide more details about that? You mentioned on IRC that >> sometimes 'bogus' DRM vblank counter values are returned to userspace. >> The most likely cause of that would be drm_vblank_pre_modeset() being >> called too late, i.e. after the hardware counter was reset. (Or if >> you're reducing / eliminating the vblank disable timer, possibly the >> vblank interrupt getting disabled too early, i.e. before the hardware >> counter was reset) > > The hardware counter reset is a problem: > 1. vblank_disable_and_save() updates .last > 2. modeset/dpms/suspend (hw counter is reset) > 3. drm_vblank_get() -> cur_vblank-.last == garbage > > The lack of drm_vblank_on() is a problem: > 1. drm_vblank_get() > 2. drm_vblank_off() > 3. modeset/dpms/suspend > 4. drm_vblank_get() -> -EINVAL I'd summarize these as 'drm_vblank_off() considered harmful'. > Another issue: > 1. drm_vblank_get() > 2. drm_vblank_put() > 3. disable timer expires which updates .last > ... > 4. drm_vblank_off() updates .last again > 5. modeset/dpms/suspend > 6. drm_vblank_get() > -> sequence number doesn't account for the time > between 3. and 4. I suppose this isn't a big > issue, but I don't like leaking implementation > details (the timer delay) into the sequence > number. Yes, I guess drm_vblank_off() shouldn't call vblank_disable_and_save() if vblank is already disabled. > Now this last one should actually work with the current > drm_vblank_pre_modeset() since it does a drm_vblank_get() > which will apply the cur_vblank-.last diff, but it also > enables the vblank interrupt which is entirely pointless, > and also wrong on Intel hardware (well, if we didn't have > drm_vblank_off()). Our docs say that we shouldn't have > the vblank interrupt enabled+unmasked while the pipe is off. That's a driver implementation detail. The driver isn't required to keep the hardware interrupt enabled all the time between the enable_vblank() and disable_vblank() hook calls. The DRM core just wants drm_handle_vblank() to be called for any vertical blank periods between them. > Anyway it's not a very obvious way to do things. Ie. > you're doing the drm_vblank_get() not because you > actually want vblank interrupts, but because you want > the side effects. No, that's not the only reason. It's also so that drm_handle_vblank() is called for any vertical blank periods occurring while the hardware counter might reset, so the DRM vblank counter gets incremented independently from the hardware counter. >> Speaking of reducing or disabling the vblank disable timer, that should >> be possible with drm_vblank_pre/post_modeset() as well. > > I get the impression that you're a bit hung up on the names :) Not at all. In fact, the pre/post_modeset names are slightly misleading, as they're not only about modesetting but about preventing the DRM vblank counter from jumping due to hardware counter jumps. > We could rename the off/on to pre/post_modeset if you like, but then > someone gets to audit all the other drivers. That someone isn't > going to be me. I appreciate your caution wrt other drivers, but I'm worried that having a second mechanism for keeping the DRM vblank counter consistent might cause subtle problems for drivers using the existing mechanism anyway.
On Thu, May 29, 2014 at 6:11 AM, Michel Dänzer <michel@daenzer.net> wrote: > >> We could rename the off/on to pre/post_modeset if you like, but then >> someone gets to audit all the other drivers. That someone isn't >> going to be me. > > I appreciate your caution wrt other drivers, but I'm worried that having > a second mechanism for keeping the DRM vblank counter consistent might > cause subtle problems for drivers using the existing mechanism anyway. I think that risk is rather low. Only i915 has been stupid enough to use both drm_vblank_off + pre/post_modeset in the normal crtc enable/disable functions. Everyone else uses either or the other, except for tegra which uses pre/post in the ->prepare/commit hooks and off in the crtc->disable callback. So should still get consistent results (since after ->disable vblank consistency stops mattering). The reason we do all this is that we want to kill the delayed vblank put for i915, which is possible if you have a hw vblank counter. There's apparently more stuff wrong here still, so while we wrestle around probably best to leave other drivers out. I guess once this is all done I have to roll it out across all drivers so that we have consistent behaviour again ... -Daniel
On 29.05.2014 19:56, Daniel Vetter wrote: > On Thu, May 29, 2014 at 6:11 AM, Michel Dänzer <michel@daenzer.net> wrote: >> >>> We could rename the off/on to pre/post_modeset if you like, but then >>> someone gets to audit all the other drivers. That someone isn't >>> going to be me. >> >> I appreciate your caution wrt other drivers, but I'm worried that having >> a second mechanism for keeping the DRM vblank counter consistent might >> cause subtle problems for drivers using the existing mechanism anyway. > > I think that risk is rather low. Only i915 has been stupid enough to > use both drm_vblank_off + pre/post_modeset in the normal crtc > enable/disable functions. Everyone else uses either or the other, > except for tegra which uses pre/post in the ->prepare/commit hooks and > off in the crtc->disable callback. So should still get consistent > results (since after ->disable vblank consistency stops mattering). > > The reason we do all this is that we want to kill the delayed vblank > put for i915, which is possible if you have a hw vblank counter. That should also be possible with pre/post_modeset. (Not sure eliminating it completely is a good idea for the reasons you mentioned before, but at least decreasing it significantly should be no problem. I think even dropping the default a lot for all drivers should be rather low risk) > There's apparently more stuff wrong here still, so while we wrestle > around probably best to leave other drivers out. I guess once this is > all done I have to roll it out across all drivers so that we have > consistent behaviour again ... Since AFAICT radeon isn't affected by the problem drm_vblank_off() was supposed to solve originally, I'd prefer if it didn't start using that and potentially suffer from all the trouble it seems to cause.
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 7688abc..d5e27bb 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1293,6 +1293,12 @@ static int i915_load_modeset_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int ret; + /* + * Allow the use of vblank interrupts during modeset, + * and make the vblank code behaviour more consistent + */ + dev->vblank_always_enable_on_get = true; + ret = intel_parse_bios(dev); if (ret) DRM_INFO("failed to find VBIOS tables\n"); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bab0d08..2933540 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3607,6 +3607,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) * happening. */ intel_wait_for_vblank(dev, intel_crtc->pipe); + + drm_vblank_on(dev, pipe); } /* IPS only exists on ULT machines and is tied to pipe A. */ @@ -3632,6 +3634,8 @@ static void haswell_crtc_enable_planes(struct drm_crtc *crtc) mutex_lock(&dev->struct_mutex); intel_update_fbc(dev); mutex_unlock(&dev->struct_mutex); + + drm_vblank_on(dev, pipe); } static void haswell_crtc_disable_planes(struct drm_crtc *crtc) @@ -3643,7 +3647,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc) int plane = intel_crtc->plane; intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe, false); + drm_vblank_off(dev, pipe, true); /* FBC must be disabled before disabling the plane on HSW. */ if (dev_priv->fbc.plane == plane) @@ -3774,7 +3778,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder); intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe, false); + drm_vblank_off(dev, pipe, true); if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev); @@ -4160,6 +4164,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); + + drm_vblank_on(dev, pipe); } static void i9xx_crtc_enable(struct drm_crtc *crtc) @@ -4205,6 +4211,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); + + drm_vblank_on(dev, pipe); } static void i9xx_pfit_disable(struct intel_crtc *crtc) @@ -4239,7 +4247,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) /* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe, false); + drm_vblank_off(dev, pipe, true); if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev); @@ -7035,15 +7043,10 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, struct intel_encoder *encoder; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_display_mode *mode = &intel_crtc->config.requested_mode; - int pipe = intel_crtc->pipe; int ret; - drm_vblank_pre_modeset(dev, pipe); - ret = dev_priv->display.crtc_mode_set(crtc, x, y, fb); - drm_vblank_post_modeset(dev, pipe); - if (ret != 0) return ret; @@ -11380,6 +11383,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); intel_sanitize_crtc(crtc); intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]"); + if (crtc->active) + drm_vblank_on(dev, crtc->pipe); + else + drm_vblank_off(dev, crtc->pipe, true); } for (i = 0; i < dev_priv->num_shared_dpll; i++) {