Re: [PATCH] drm & i915 vblank fixes

[Robert N. <rn...@2h...>]

On Sat, 2008-07-19 at 14:01 +0200, 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...
> 
> 
> > 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;
> ^^^^^^^^^^^^^^^^^^^
> 
> This means the interrupt never gets disabled, doesn't it?

Yes, this was not correct, it prevents any of the pre/post modeset code
from running.  I've fixed this on the bsd side.  I'll try and get the
linux side fixed up later and send an updated patch.
 
> 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.

I've changed this to vblank_disable_allowed = 0 as an additional
safeguard against disabling interupts across modeset.  It shouldn't be
needed though, as the vblank_get will bump the refcount and prevent the
callout from disabling interupts.

One other issue that I spotted while testing is that we initialize
vblank_disable_allowed to 0.  The only place that it ever becomes true
is in post modeset.  So, for me... vblanks were never getting disabled.
For now, I'm initializing it to 1 and it will only toggle over modeset.

I'm still (or again) sometimes getting the error about get_vblank_count
being called on disabled pipe that I need to try and chase down.

I also realized that I was inadvertently setting the timeout too long in
the bsd code, which I've fixed...

Otherwise, it seems to be working well.

robert.