On Friday, July 18, 2008 1:12 am Michel Dänzer wrote:
> On Thu, 2008-07-17 at 09:32 -0700, Jesse Barnes wrote:
> > On Wednesday, July 16, 2008 11:51 pm Michel Dänzer wrote:
> > > On Wed, 2008-07-16 at 16:01 -0700, Jesse Barnes wrote:
> > > > @@ -408,8 +426,10 @@ int drm_vblank_get(struct drm_device *dev, int
> > > > crtc) ret = dev->driver->enable_vblank(dev, crtc); if (ret)
> > > > atomic_dec(&dev->vblank_refcount[crtc]);
> > > > - else
> > > > + else {
> > > > dev->vblank_enabled[crtc] = 1;
> > > > + drm_update_vblank_count(dev, crtc);
> > > > + }
> > >
> > > I think this is missing a corresponding
> > >
> > > dev->last_vblank[crtc] = dev->driver->get_vblank_counter(dev, crtc);
> > >
> > > when the interrupt gets disabled, to make sure this does the right
> > > thing.
> >
> > Oh right, I forgot about the wraparound logic.
>
> It's not about that; drm_update_vblank_count() just needs to increase
> dev->_vblank_count[crtc] by the number of frames since the interrupt was
> disabled, not since the last time it was enabled.
But we also use the last_count to handle wraparound if it occurred. Either
way it needs to be update at disable time, you're right.
> > I also got to thinking about the modeset ioctl. With a few changes I
> > think we can avoid the huge jumps we used to see (both forward &
> > backward).
>
> I thought we'd been over that before on IRC... Please revert the
> drm_modeset_ctl() changes, what was there before should DTRT under any
> circumstances and certainly does IME. If you've found specific cases
> where it doesn't, please provide more information about those. (Thought
> experiments are fine :)
Ok, we've been over this on IRC, but I'll document it here for our audience
(who I'm sure are on the edge of their seats by now).
The initial API relied on drm_update_vblank_count() to be called before
getting the actual count, that way we'd account for any wraparound that might
have occurred in the hw register, and we'd also update the counter with any
events that happened while interrupts were off.
But this introduced problems. Around mode sets, the counter would always be
reset, so the update_vblank_count() wraparound logic would be triggered,
increasing the counter by huge amounts on every mode set. So if a client was
doing a wait on count + 1 but we incremented our internal counter by count +
wraparound (0xffffff in the intel case) it would just hang waiting forever in
the case of AIGLX (this would happen if a modeset occurred while an app was
running for example).
So you fixed up the compensation logic in pre/post modeset, which fixed this
case, so that userspace would never see big jumps.
In this last push, I changed things over to use the internal counter directly
while interrupts were on, using update_vblank_count to add in lost events
when interrupts went from off->on. So while interrupts are enabled, there
are no issues. And if we go from off->on we add the frame count to the
counter and check for wraparound, so things generally work.
*However*, what I neglected to do was fixup drm_wait_vblank() and the rest of
the update_vblank_count callers to use drm_vblank_get(), which will both
enable interrupts (if they're off) and account for any lost events if we
happen to be going from an off->on state. So if wait_vblank() is called and
interrupts are currently off, we'll get a stale value from
drm_vblank_count(), then call get() which will update it, and then interrupts
will be enabled and things should work. This might mean tearing on the first
frame of a given app. Likewise for scheduled swaps, they may occur too soon
if interrupts are off when they're first scheduled.
The new modeset code is also susceptible to problems if interrupts go from
on->off between pre & post mode set, since in that case we'll use the last
known pre-modeset count to calculate the difference to add to the counter.
>
> > > > @@ -528,7 +550,6 @@ int drm_wait_vblank(struct drm_device *dev, void
> > > > *data, if (crtc >= dev->num_crtcs)
> > > > return -EINVAL;
> > > >
> > > > - drm_update_vblank_count(dev, crtc);
> > > > seq = drm_vblank_count(dev, crtc);
> > > >
> > > > switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
> > >
> > > I don't think this can be removed altogether, or seq will be stale if
> > > the interrupt is disabled when drm_wait_vblank() is called. So I guess
> > > call drm_update_vblank_count() when the interrupt is disabled, or just
> > > bail inside drm_update_vblank_count() when it is enabled.
> >
> > Yeah, I think just a get_vblank_counter() in vblank_disable_fn() is
> > sufficient, since we'll hit the wraparound logic again when we re-enable.
> > Look ok?
>
> I'm afraid not. Again, the wraparound logic is irrelevant for this, it's
> in drm_update_vblank_count() after all...
>
> What's wrong with
>
> if (!dev->vblank_enabled[crtc])
> drm_update_vblank_count(dev, crtc);
> seq = drm_vblank_count(dev, crtc);
> ...
That's one way of addressing the interrupts-are-off counter update, but really
I had intended to get rid of drm_update_vblank_count() as part of the API
since it led to the spurious wraparound problem in the first place, and
isolate all the update logic to drm_vblank_get(). My thinking was that
get/put around any API usage would be simpler, but maybe keeping the
update_vblank_count() call in place is better, I dunno.
Jesse
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
