On Friday, July 18, 2008 10:36 am Jesse Barnes wrote:
> On Friday, July 18, 2008 10:24 am Jesse Barnes wrote:
> > On Friday, July 18, 2008 10:15 am Jesse Barnes wrote:
> > > On Friday, July 18, 2008 10:09 am Michel Dänzer wrote:
> > > > On Fri, 2008-07-18 at 09:41 -0700, Jesse Barnes wrote:
> > > > > 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:
> > > > > > > > > @@ -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.
> > > >
> > > > While it may be possible to fix the problems of your change in the
> > > > long run, I think it's a pretty bad idea to make such fundamental
> > > > changes to a basically mature implementation this shortly before
> > > > submitting the functionality to the kernel. Maybe we can try your
> > > > approach again once it's made it into a kernel release.
> > >
> > > FWIW here's the patch for the get/put change. I'm still working on the
> > > other one you suggested, but it's not trivial either...
> >
> > And here's the other one.
>
> Oops, missed a few things in the last one.
I failed to get the old update_vblank approach working, messing with the
counter is different under the new interrupt counter scheme. But I did try
out the interrupt enable scheme we talked about and it seems solid.
The case that you needed the old logic for was this:
1) modeset happens so hw reg gets reset
2) client starts up and does a vblank wait
3) update_count wraps the value since cur < old
4) client waits on new count
so unless the modeset ioctl did the compensation you added, every mode set
followed by a vblank wait would end up with a huge jump like you observed.
However with what's checked in now, the following will happen:
1) modeset occurs, hw reg gets reset
2) client starts up and does a vblank wait
3) client gets old vblank counter value from when interrupts were last on
4) vblank wait does drm_vblank_get around wait
5) client probably wakes up since drm_vblank_get accounts for lost events
But this patch fixes the behavior at step (3), and requires drm_vblank_get
rather than drm_update_vblank_count.
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. And of course I'm interested to hear about any bugs you
discover in testing.
Thanks,
Jesse
diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
index 75e53da..80ab7f6 100644
--- a/linux-core/drm_irq.c
+++ b/linux-core/drm_irq.c
@@ -412,7 +412,7 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
unsigned long irqflags;
int ret = 0;
- spin_lock_irqsave(&dev->vbl_lock, irqflags);
+ spin_lock_irqsave(&dev->vbl_lock, irqflags);
/* Going from 0->1 means we have to enable interrupts again */
if (atomic_add_return(1, &dev->vblank_refcount[crtc]) == 1 &&
!dev->vblank_enabled[crtc]) {
@@ -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;
+
+ /*
+ * To avoid all the problems that might happen if interrupts
+ * were enabled/disabled around or between these calls, we just
+ * have the kernel take a reference on the CRTC (just once though
+ * to avoid corrupting the count if multiple, mismatch calls occur),
+ * so that interrupts remain enabled in the interim.
+ */
switch (modeset->cmd) {
case _DRM_PRE_MODESET:
- spin_lock_irqsave(&dev->vbl_lock, irqflags);
- dev->vblank_disable_allowed = 1;
- if (!dev->vblank_enabled[crtc]) {
- dev->vblank_premodeset[crtc] =
- dev->driver->get_vblank_counter(dev, crtc);
+ if (!dev->vblank_suspend[crtc]) {
+ spin_lock_irqsave(&dev->vbl_lock, irqflags);
+ dev->vblank_disable_allowed = 1;
dev->vblank_suspend[crtc] = 1;
+ spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+ drm_vblank_get(dev, crtc);
}
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
break;
case _DRM_POST_MODESET:
- spin_lock_irqsave(&dev->vbl_lock, irqflags);
- dev->vblank_disable_allowed = 1;
- new = dev->driver->get_vblank_counter(dev, crtc);
- if (dev->vblank_suspend[crtc] && !dev->vblank_enabled[crtc]) {
- if (new > dev->vblank_premodeset[crtc])
- diff = dev->vblank_premodeset[crtc] - new;
- else
- diff = new;
- atomic_add(diff, &dev->_vblank_count[crtc]);
+ if (dev->vblank_suspend[crtc]) {
+ spin_lock_irqsave(&dev->vbl_lock, irqflags);
+ dev->vblank_disable_allowed = 1;
+ dev->vblank_suspend[crtc] = 0;
+ spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+ drm_vblank_put(dev, crtc);
}
- dev->vblank_suspend[crtc] = 0;
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
break;
default:
ret = -EINVAL;
@@ -549,6 +554,9 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
if (crtc >= dev->num_crtcs)
return -EINVAL;
+ ret = drm_vblank_get(dev, crtc);
+ if (ret)
+ return ret;
seq = drm_vblank_count(dev, crtc);
switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
@@ -558,7 +566,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
case _DRM_VBLANK_ABSOLUTE:
break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ goto done;
}
if ((flags & _DRM_VBLANK_NEXTONMISS) &&
@@ -571,8 +580,10 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
struct list_head *vbl_sigs = &dev->vbl_sigs[crtc];
struct drm_vbl_sig *vbl_sig;
- if (dev->vblank_suspend[crtc])
- return -EBUSY;
+ if (dev->vblank_suspend[crtc]) {
+ ret = -EBUSY;
+ goto done;
+ }
spin_lock_irqsave(&dev->vbl_lock, irqflags);
@@ -594,15 +605,18 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
if (atomic_read(&dev->vbl_signal_pending) >= 100) {
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
- return -EBUSY;
+ ret = -EBUSY;
+ goto done;
}
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
vbl_sig = drm_calloc(1, sizeof(struct drm_vbl_sig),
DRM_MEM_DRIVER);
- if (!vbl_sig)
- return -ENOMEM;
+ if (!vbl_sig) {
+ ret = -ENOMEM;
+ goto done;
+ }
ret = drm_vblank_get(dev, crtc);
if (ret) {
@@ -626,13 +640,9 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
vblwait->reply.sequence = seq;
} else {
if (!dev->vblank_suspend[crtc]) {
- ret = drm_vblank_get(dev, crtc);
- if (ret)
- return ret;
DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ,
((drm_vblank_count(dev, crtc)
- vblwait->request.sequence) <= (1 << 23)));
- drm_vblank_put(dev, crtc);
}
if (ret != -EINTR) {
@@ -646,7 +656,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
}
}
- done:
+done:
+ drm_vblank_put(dev, crtc);
return ret;
}
diff --git a/shared-core/i915_irq.c b/shared-core/i915_irq.c
index ecb2d7f..b95d0f1 100644
--- a/shared-core/i915_irq.c
+++ b/shared-core/i915_irq.c
@@ -769,6 +769,13 @@ int i915_vblank_swap(struct drm_device *dev, void *data,
DRM_SPINUNLOCK_IRQRESTORE(&dev->drw_lock, irqflags);
+ /*
+ * We take the ref here and put it when the swap actually completes
+ * in the tasklet.
+ */
+ ret = drm_vblank_get(dev, pipe);
+ if (ret)
+ return ret;
curseq = drm_vblank_count(dev, pipe);
if (seqtype == _DRM_VBLANK_RELATIVE)
@@ -779,6 +786,7 @@ int i915_vblank_swap(struct drm_device *dev, void *data,
swap->sequence = curseq + 1;
} else {
DRM_DEBUG("Missed target sequence\n");
+ drm_vblank_put(dev, pipe);
return -EINVAL;
}
}
@@ -800,6 +808,7 @@ int i915_vblank_swap(struct drm_device *dev, void *data,
irqflags);
DRM_DEBUG("Invalid drawable ID %d\n",
swap->drawable);
+ drm_vblank_put(dev, pipe);
return -EINVAL;
}
@@ -807,6 +816,7 @@ int i915_vblank_swap(struct drm_device *dev, void *data,
DRM_SPINUNLOCK_IRQRESTORE(&dev->drw_lock, irqflags);
+ drm_vblank_put(dev, pipe);
return 0;
}
}
@@ -830,6 +840,7 @@ int i915_vblank_swap(struct drm_device *dev, void *data,
if (dev_priv->swaps_pending >= 100) {
DRM_DEBUG("Too many swaps queued\n");
+ drm_vblank_put(dev, pipe);
return -EBUSY;
}
@@ -837,17 +848,12 @@ int i915_vblank_swap(struct drm_device *dev, void *data,
if (!vbl_swap) {
DRM_ERROR("Failed to allocate memory to queue swap\n");
+ drm_vblank_put(dev, pipe);
return -ENOMEM;
}
DRM_DEBUG("\n");
- ret = drm_vblank_get(dev, pipe);
- if (ret) {
- drm_free(vbl_swap, sizeof(*vbl_swap), DRM_MEM_DRIVER);
- return ret;
- }
-
vbl_swap->drw_id = swap->drawable;
vbl_swap->plane = plane;
vbl_swap->sequence = swap->sequence;
-------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel
