On 04/05/2015 05:40 PM, Chris Wilson wrote:
Bypass all the spinlocks and return the last timestamp and counter from the last vblank if the driver delcares that it is accurate (and stable across on/off), and the vblank is currently enabled.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel@ffwll.ch Cc: Michel Dänzer michel@daenzer.net Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Dave Airlie airlied@redhat.com, Cc: Mario Kleiner mario.kleiner.de@gmail.com
drivers/gpu/drm/drm_irq.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index ba80b51b4b00..be9c210bb22e 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1538,6 +1538,17 @@ err_put: return ret; }
+static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait) +{
- if (vblwait->request.sequence)
return false;- return _DRM_VBLANK_RELATIVE ==
(vblwait->request.type & (_DRM_VBLANK_TYPES_MASK |_DRM_VBLANK_EVENT |_DRM_VBLANK_NEXTONMISS));+}
- /*
- Wait for VBLANK.
@@ -1587,6 +1598,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
vblank = &dev->vblank[crtc];
- /* If the counter is currently enabled and accurate, short-circuit queries
* to return the cached timestamp of the last vblank.*/
Maybe somehow stress in the comment that this location in drm_wait_vblank is really the only place where it is ok'ish to call drm_vblank_count_and_time() without wrapping it into a drm_vblank_get/put(), so nobody thinks this approach is ok anywhere else.
- if (dev->vblank_disable_immediate &&
drm_wait_vblank_is_query(vblwait) &&vblank->enabled) {
You should also check for (drm_vblank_offdelay != 0) whenever checking for dev->vblank_disable_immediate. This is so one can override all the vblank_disable_immediate related logic via the drm vblankoffdelay module parameter, both for debugging and as a safety switch for desparate users in case some driver+gpu combo screws up wrt. immediate disable and that makes it into distro kernels.
The other thing i'm not sure is if it wouldn't be a good idea to have some kind of write memory barrier in vblank_disable_and_save() after setting vblank->enabled = false; and some read memory barrier here before your check for vblank->enabled? I don't have a feeling for how much time can pass between one core executing the disable and the other core receiving the news that vblank->enabled is no longer true if those bits run on different cores?
I've run your patches through my standard tests on x86_64 and they don't seem to introduce errors or more skipped frames. Normally it would be so wrong to do this without drm_vblank_get/put(), but i think here potential errors introduced wouldn't be worse than what a userspace client would see due to preemption or other execution delays at the wrong moment, so it's probably ok. But i don't know if lack of memory barriers etc. could introduce large delays and trouble on other architectures?
struct timeval now;vblwait->reply.sequence =drm_vblank_count_and_time(dev, crtc, &now);vblwait->reply.tval_sec = now.tv_sec;vblwait->reply.tval_usec = now.tv_usec;
Have some DRM_DEBUG here, so one can follow the client doing the instant query through this path.
return 0;- }
- ret = drm_vblank_get(dev, crtc); if (ret) { DRM_DEBUG("failed to acquire vblank counter, %d\n", ret);
With the above addressed i'd give you a Reviewed-and-tested-by, but it would be good if somebody else could look over it as well.
-mario