On Wednesday, July 16, 2008 11:51 pm Michel Dänzer wrote:
> On Wed, 2008-07-16 at 16:01 -0700, Jesse Barnes wrote:
> > Here's a patch that implements what we've been talking about:
> > - uses the atomic counter while interrupts are on, which means the
> > get_vblank_counter callback is only used when going from off->on to
> > account for missed events
> > - won't disable interrupts until the modeset ioctl is called, to
> > preserve old behavior with old clients
>
> Sounds good, thanks for tackling this.
No problem, thanks again for the thorough review.
> > +/*
> > + * At load time, disabling the vblank interrupt won't be allowed since
> > + * old clients may not call the modeset ioctl and therefore misbehave.
> > + * Once the modeset ioctl *has* been called though, we can safely
> > + * disable them when unused.
> > + */
> > +static int vblank_disable_allowed;
>
> This should probably be tracked per device instead of globally.
Definitely. Not sure what I was thinking there...
> > @@ -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. 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). Now that we just use the counter when
interrupts are enabled, we can remove some code.
If interrupts are enabled across the modeset, we don't have to do anything
since the counter will stay accurate.
If interrupts are disabled, we just have to add either the difference between
the pre & post mode set values if the hw counter wasn't reset, or simply the
new value if the hw counter did reset.
Seems to work here across mode switches in my testing, so I think I got it
right...
> > @@ -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?
Thanks,
Jesse
diff --git a/linux-core/drmP.h b/linux-core/drmP.h
index 331f3ac..45a599b 100644
--- a/linux-core/drmP.h
+++ b/linux-core/drmP.h
@@ -832,6 +832,14 @@ struct drm_device {
/** \name VBLANK IRQ support */
/[EMAIL PROTECTED] */
+ /*
+ * At load time, disabling the vblank interrupt won't be allowed since
+ * old clients may not call the modeset ioctl and therefore misbehave.
+ * Once the modeset ioctl *has* been called though, we can safely
+ * disable them when unused.
+ */
+ int vblank_disable_allowed;
+
wait_queue_head_t *vbl_queue; /**< VBLANK wait queue */
atomic_t *_vblank_count; /**< number of VBLANK interrupts (driver must alloc the right number of counters) */
spinlock_t vbl_lock;
diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
index abedbe7..75e53da 100644
--- a/linux-core/drm_irq.c
+++ b/linux-core/drm_irq.c
@@ -77,10 +77,16 @@ static void vblank_disable_fn(unsigned long arg)
unsigned long irqflags;
int i;
+ if (!dev->vblank_disable_allowed)
+ return;
+
for (i = 0; i < dev->num_crtcs; i++) {
spin_lock_irqsave(&dev->vbl_lock, irqflags);
if (atomic_read(&dev->vblank_refcount[i]) == 0 &&
dev->vblank_enabled[i]) {
+ DRM_DEBUG("disabling vblank on crtc %d\n", i);
+ dev->last_vblank[i] =
+ dev->driver->get_vblank_counter(dev, i);
dev->driver->disable_vblank(dev, i);
dev->vblank_enabled[i] = 0;
}
@@ -175,6 +181,8 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
atomic_set(&dev->vblank_refcount[i], 0);
}
+ dev->vblank_disable_allowed = 0;
+
return 0;
err:
@@ -344,10 +352,15 @@ EXPORT_SYMBOL(drm_vblank_count);
* (specified by @crtc). Deal with wraparound, if it occurred, and
* update the last read value so we can deal with wraparound on the next
* call if necessary.
+ *
+ * Only necessary when going from off->on, to account for frames we
+ * didn't get an interrupt for.
+ *
+ * Note: caller must hold dev->vbl_lock since this reads & writes
+ * device vblank fields.
*/
void drm_update_vblank_count(struct drm_device *dev, int crtc)
{
- unsigned long irqflags;
u32 cur_vblank, diff;
if (dev->vblank_suspend[crtc])
@@ -361,7 +374,6 @@ void drm_update_vblank_count(struct drm_device *dev, int crtc)
* a long time.
*/
cur_vblank = dev->driver->get_vblank_counter(dev, crtc);
- spin_lock_irqsave(&dev->vbl_lock, irqflags);
if (cur_vblank < dev->last_vblank[crtc]) {
if (cur_vblank == dev->last_vblank[crtc] - 1) {
diff = 0;
@@ -377,11 +389,12 @@ void drm_update_vblank_count(struct drm_device *dev, int crtc)
diff = cur_vblank - dev->last_vblank[crtc];
}
dev->last_vblank[crtc] = cur_vblank;
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+
+ DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
+ crtc, diff);
atomic_add(diff, &dev->_vblank_count[crtc]);
}
-EXPORT_SYMBOL(drm_update_vblank_count);
/**
* drm_vblank_get - get a reference count on vblank events
@@ -389,9 +402,7 @@ EXPORT_SYMBOL(drm_update_vblank_count);
* @crtc: which CRTC to own
*
* Acquire a reference count on vblank events to avoid having them disabled
- * while in use. Note callers will probably want to update the master counter
- * using drm_update_vblank_count() above before calling this routine so that
- * wakeups occur on the right vblank event.
+ * while in use.
*
* RETURNS
* Zero on success, nonzero on failure.
@@ -408,8 +419,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);
+ }
}
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
@@ -439,11 +452,18 @@ EXPORT_SYMBOL(drm_vblank_put);
*
* Applications should call the %_DRM_PRE_MODESET and %_DRM_POST_MODESET
* ioctls around modesetting so that any lost vblank events are accounted for.
+ *
+ * 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.
*/
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;
crtc = modeset->crtc;
@@ -454,27 +474,28 @@ int drm_modeset_ctl(struct drm_device *dev, void *data,
switch (modeset->cmd) {
case _DRM_PRE_MODESET:
- dev->vblank_premodeset[crtc] =
- dev->driver->get_vblank_counter(dev, crtc);
- dev->vblank_suspend[crtc] = 1;
+ 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_suspend[crtc] = 1;
+ }
+ spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
break;
case _DRM_POST_MODESET:
- if (dev->vblank_suspend[crtc]) {
- u32 new = dev->driver->get_vblank_counter(dev, crtc);
-
- /* Compensate for spurious wraparound */
- if (new < dev->vblank_premodeset[crtc]) {
- atomic_sub(dev->max_vblank_count + new -
- dev->vblank_premodeset[crtc],
- &dev->_vblank_count[crtc]);
- DRM_DEBUG("vblank_premodeset[%d]=0x%x, new=0x%x"
- " => _vblank_count[%d]-=0x%x\n", crtc,
- dev->vblank_premodeset[crtc], new,
- crtc, dev->max_vblank_count + new -
- dev->vblank_premodeset[crtc]);
- }
+ 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]);
}
dev->vblank_suspend[crtc] = 0;
+ spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
break;
default:
ret = -EINVAL;
@@ -528,7 +549,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) {
@@ -680,7 +700,7 @@ static void drm_vbl_send_signals(struct drm_device * dev, int crtc)
*/
void drm_handle_vblank(struct drm_device *dev, int crtc)
{
- drm_update_vblank_count(dev, crtc);
+ atomic_inc(&dev->_vblank_count[crtc]);
DRM_WAKEUP(&dev->vbl_queue[crtc]);
drm_vbl_send_signals(dev, crtc);
}
diff --git a/shared-core/i915_irq.c b/shared-core/i915_irq.c
index d7fa47d..ecb2d7f 100644
--- a/shared-core/i915_irq.c
+++ b/shared-core/i915_irq.c
@@ -361,28 +361,7 @@ static void i915_vblank_tasklet(struct drm_device *dev)
drm_free(swap_hit, sizeof(*swap_hit), DRM_MEM_DRIVER);
}
}
-#if 0
-static int i915_in_vblank(struct drm_device *dev, int pipe)
-{
- drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
- unsigned long pipedsl, vblank, vtotal;
- unsigned long vbl_start, vbl_end, cur_line;
-
- pipedsl = pipe ? PIPEBDSL : PIPEADSL;
- vblank = pipe ? VBLANK_B : VBLANK_A;
- vtotal = pipe ? VTOTAL_B : VTOTAL_A;
-
- vbl_start = I915_READ(vblank) & VBLANK_START_MASK;
- vbl_end = (I915_READ(vblank) >> VBLANK_END_SHIFT) & VBLANK_END_MASK;
-
- cur_line = I915_READ(pipedsl);
-
- if (cur_line >= vbl_start)
- return 1;
- return 0;
-}
-#endif
u32 i915_get_vblank_counter(struct drm_device *dev, int plane)
{
drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -416,22 +395,6 @@ u32 i915_get_vblank_counter(struct drm_device *dev, int plane)
count = (high1 << 8) | low;
- /*
- * If we're in the middle of the vblank period, the
- * above regs won't have been updated yet, so return
- * an incremented count to stay accurate
- */
-#if 0
- if (i915_in_vblank(dev, pipe))
- count++;
-#endif
- /* count may be reset by other driver(e.g. 2D driver),
- we have no way to know if it is wrapped or resetted
- when count is zero. do a rough guess.
- */
- if (count == 0 && dev->last_vblank[pipe] < dev->max_vblank_count/2)
- dev->last_vblank[pipe] = 0;
-
return count;
}
@@ -444,18 +407,8 @@ irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS)
int vblank = 0;
iir = I915_READ(IIR);
-#if 0
- DRM_DEBUG("flag=%08x\n", iir);
-#endif
- if (iir == 0) {
- DRM_DEBUG ("iir 0x%08x im 0x%08x ie 0x%08x pipea 0x%08x pipeb 0x%08x\n",
- iir,
- I915_READ(IMR),
- I915_READ(IER),
- I915_READ(PIPEASTAT),
- I915_READ(PIPEBSTAT));
+ if (iir == 0)
return IRQ_NONE;
- }
/*
* Clear the PIPE(A|B)STAT regs before the IIR otherwise
@@ -735,20 +688,12 @@ int i915_vblank_pipe_set(struct drm_device *dev, void *data,
struct drm_file *file_priv)
{
drm_i915_private_t *dev_priv = dev->dev_private;
- drm_i915_vblank_pipe_t *pipe = data;
if (!dev_priv) {
DRM_ERROR("called with no initialization\n");
return -EINVAL;
}
- if (pipe->pipe & ~(DRM_I915_VBLANK_PIPE_A|DRM_I915_VBLANK_PIPE_B)) {
- DRM_ERROR("called with invalid pipe 0x%x\n", pipe->pipe);
- return -EINVAL;
- }
-
- dev_priv->vblank_pipe = pipe->pipe;
-
return 0;
}
@@ -757,20 +702,13 @@ int i915_vblank_pipe_get(struct drm_device *dev, void *data,
{
drm_i915_private_t *dev_priv = dev->dev_private;
drm_i915_vblank_pipe_t *pipe = data;
- u16 flag;
if (!dev_priv) {
DRM_ERROR("called with no initialization\n");
return -EINVAL;
}
- flag = I915_READ(IER);
-
- pipe->pipe = 0;
- if (flag & I915_DISPLAY_PIPE_A_EVENT_INTERRUPT)
- pipe->pipe |= DRM_I915_VBLANK_PIPE_A;
- if (flag & I915_DISPLAY_PIPE_B_EVENT_INTERRUPT)
- pipe->pipe |= DRM_I915_VBLANK_PIPE_B;
+ pipe->pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
return 0;
}
@@ -831,7 +769,6 @@ int i915_vblank_swap(struct drm_device *dev, void *data,
DRM_SPINUNLOCK_IRQRESTORE(&dev->drw_lock, irqflags);
- drm_update_vblank_count(dev, pipe);
curseq = drm_vblank_count(dev, pipe);
if (seqtype == _DRM_VBLANK_RELATIVE)
@@ -957,6 +894,7 @@ int i915_driver_irq_postinstall(struct drm_device * dev)
if (ret)
return ret;
+ dev_priv->vblank_pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
i915_enable_interrupt(dev);
@@ -978,6 +916,8 @@ void i915_driver_irq_uninstall(struct drm_device * dev)
if (!dev_priv)
return;
+ dev_priv->vblank_pipe = 0;
+
dev_priv->irq_enabled = 0;
I915_WRITE(HWSTAM, 0xffffffff);
I915_WRITE(IMR, 0xffffffff);
-------------------------------------------------------------------------
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
