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...
Jesse
diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
index 75e53da..4cbd070 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]) {
@@ -472,15 +472,34 @@ int drm_modeset_ctl(struct drm_device *dev, void *data,
goto out;
}
+ /*
+ * Cases to handle here:
+ * 1) interrupts are off across both calls
+ * be sure to add any lost frames
+ * 2) interrupts are on across both calls
+ * nothing to do, count will be updated by interupt handlers
+ * 3) interrupts go from on->off between calls
+ * add the post mode set frame count
+ * note that if interrupts go off before the counter resets
+ * we'll lose some frames.
+ * and if interrupts go off after it resets we'll end up
+ * double counting some events since the interrupt handlers ill
+ * have added some of the new frame count value too
+ * 4) interrupts going from off->on between calls
+ * this really shouldn't happen much since clients generally don't
+ * start while modesetting is happening
+ * we need to account for lost frames in this case too, since
+ * there may be quite a few of them
+ */
+
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);
+ dev->vblank_premodeset[crtc] =
+ dev->driver->get_vblank_counter(dev, crtc);
+ if (!dev->vblank_enabled[crtc])
dev->vblank_suspend[crtc] = 1;
- }
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
break;
case _DRM_POST_MODESET:
@@ -549,6 +568,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 +580,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 +594,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 +619,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 +654,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 +670,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
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
