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.
Jesse
diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
index 75e53da..4576983 100644
--- a/linux-core/drm_irq.c
+++ b/linux-core/drm_irq.c
@@ -363,7 +363,7 @@ void drm_update_vblank_count(struct drm_device *dev, int crtc)
{
u32 cur_vblank, diff;
- if (dev->vblank_suspend[crtc])
+ if (dev->vblank_suspend[crtc] || dev->vblank_enabled[crtc])
return;
/*
@@ -395,6 +395,7 @@ void drm_update_vblank_count(struct drm_device *dev, int crtc)
atomic_add(diff, &dev->_vblank_count[crtc]);
}
+EXPORT_SYMBOL(drm_update_vblank_count);
/**
* drm_vblank_get - get a reference count on vblank events
@@ -420,8 +421,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
if (ret)
atomic_dec(&dev->vblank_refcount[crtc]);
else {
- dev->vblank_enabled[crtc] = 1;
drm_update_vblank_count(dev, crtc);
+ dev->vblank_enabled[crtc] = 1;
}
}
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
@@ -549,6 +550,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
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel
