From: Jesse B. <jb...@vi...> - 2008-07-18 17:24:46
|
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 |