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.
Jesse
diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
index 75e53da..9f09221 100644
--- a/linux-core/drm_irq.c
+++ b/linux-core/drm_irq.c
@@ -343,29 +343,10 @@ u32 drm_vblank_count(struct drm_device *dev, int crtc)
}
EXPORT_SYMBOL(drm_vblank_count);
-/**
- * drm_update_vblank_count - update the master vblank counter
- * @dev: DRM device
- * @crtc: counter to update
- *
- * Call back into the driver to update the appropriate vblank counter
- * (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)
+void do_update_vblank_count(struct drm_device *dev, int crtc)
{
u32 cur_vblank, diff;
- if (dev->vblank_suspend[crtc])
- return;
-
/*
* Interrupts were disabled prior to this call, so deal with counter
* wrap if needed.
@@ -392,11 +373,39 @@ void drm_update_vblank_count(struct drm_device *dev, int crtc)
DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
crtc, diff);
-
atomic_add(diff, &dev->_vblank_count[crtc]);
}
/**
+ * drm_update_vblank_count - update the master vblank counter
+ * @dev: DRM device
+ * @crtc: counter to update
+ *
+ * Call back into the driver to update the appropriate vblank counter
+ * (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;
+
+ if (dev->vblank_suspend[crtc] || dev->vblank_enabled[crtc])
+ return;
+
+ spin_lock_irqsave(&dev->vbl_lock, irqflags);
+ do_update_vblank_count(dev, crtc);
+ spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+}
+EXPORT_SYMBOL(drm_update_vblank_count);
+
+/**
* drm_vblank_get - get a reference count on vblank events
* @dev: DRM device
* @crtc: which CRTC to own
@@ -420,8 +429,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
if (ret)
atomic_dec(&dev->vblank_refcount[crtc]);
else {
+ do_update_vblank_count(dev, crtc);
dev->vblank_enabled[crtc] = 1;
- drm_update_vblank_count(dev, crtc);
}
}
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
@@ -462,8 +471,6 @@ 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;
@@ -474,28 +481,26 @@ int drm_modeset_ctl(struct drm_device *dev, void *data,
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_suspend[crtc] = 1;
- }
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+ dev->vblank_premodeset[crtc] =
+ dev->driver->get_vblank_counter(dev, crtc);
+ dev->vblank_suspend[crtc] = 1;
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]) {
+ 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]);
+ }
}
- dev->vblank_suspend[crtc] = 0;
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
break;
default:
ret = -EINVAL;
@@ -549,6 +554,7 @@ 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) {
diff --git a/shared-core/i915_irq.c b/shared-core/i915_irq.c
index ecb2d7f..713759e 100644
--- a/shared-core/i915_irq.c
+++ b/shared-core/i915_irq.c
@@ -769,6 +769,7 @@ 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)
-------------------------------------------------------------------------
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
