From: Jesse B. <jb...@vi...> - 2008-07-19 17:04:18
|
On Saturday, July 19, 2008 5:01 am Michel Dänzer wrote: > On Fri, 2008-07-18 at 14:41 -0700, Jesse Barnes wrote: > > I failed to get the old update_vblank approach working, messing with the > > counter is different under the new interrupt counter scheme. > > Okay, I don't really understand why it's so hard, but maybe I'm missing > something... I don't think it's hard, just impossible unless I re-add the (my) crappy code that made us need the compensation in the first place (i.e. the spurious wraparounds caused by updating the vblank count after a mode set). Now that we're using the counter for everything, we may as well keep the code understandable too, imo. > > If you feel strongly that we should stick with drm_update_vblank_count > > even though it's changed a lot (therefore invaliding earlier tests), I > > can change over to using that. > > Let's give your approach a spin. Note that due to the not fully > deterministic nature of the issues caused by the hardware frame counter > getting reset, it'll take at least a couple of days for me to become > confident it's solid. > > > And of course I'm interested to hear about any bugs you discover in > > testing. > > I noticed one on a quick look over: > > @@ -455,47 +455,52 @@ EXPORT_SYMBOL(drm_vblank_put); > > * > > * Generally the counter will reset across mode sets. If interrupts are > > * enabled around this call, we don't have to do anything since the > > counter - * will have already been incremented. If interrupts are off > > though, we need - * to make sure we account for the lost events at > > %_DRM_POST_MODESET time. + * will have already been incremented. > > */ > > int drm_modeset_ctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv) > > { > > struct drm_modeset_ctl *modeset = data; > > unsigned long irqflags; > > - u32 new, diff; > > int crtc, ret = 0; > > > > + /* If drm_vblank_init() hasn't been called yet, just no-op */ > > + if (!dev->num_crtcs) > > + goto out; > > + > > crtc = modeset->crtc; > > if (crtc >= dev->num_crtcs) { > > ret = -EINVAL; > > goto out; > > } > > > > + return 0; > > ^^^^^^^^^^^^^^^^^^^ Oh, heh that was some debugging I left in. > This means the interrupt never gets disabled, doesn't it? > > On another minor note, the > > dev->vblank_disable_allowed = 1; > > assignment in the _DRM_PRE_MODESET case is superfluous, as the interrupt > won't actually get disabled until the _DRM_POST_MODESET case is hit as > well. Yeah that's true, I can move it to just post modeset to avoid confusion. Jesse |