dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Mario Kleiner <mario.kleiner.de@gmail.com>
To: dri-devel@lists.freedesktop.org, airlied@gmail.com
Cc: intel-gfx@lists.freedesktop.org
Subject: [PATCH 4/4] drm/intel: Push get_scanout_position() timestamping into kms driver.
Date: Sat, 26 Oct 2013 10:27:42 +0200	[thread overview]
Message-ID: <1382776062-3770-5-git-send-email-mario.kleiner.de@gmail.com> (raw)
In-Reply-To: <1382776062-3770-1-git-send-email-mario.kleiner.de@gmail.com>

Move the ktime_get() clock readouts and potential preempt_disable()
calls from drm core into kms driver to make it compatible with the
api changes in the drm core.

The intel-kms driver needs to take the uncore.lock inside
i915_get_crtc_scanoutpos() and intel_pipe_in_vblank().
This is incompatible with the preempt_disable() on a
PREEMPT_RT patched kernel, as regular spin locks must not
be taken within a preempt_disable'd section. Lock contention
on the uncore.lock also introduced too much uncertainty in vblank
timestamps.

Push the ktime_get() timestamping for scanoutpos queries and
potential preempt_disable_rt() into i915_get_crtc_scanoutpos(),
so these problems can be avoided:

1. First lock the uncore.lock (might sleep on a PREEMPT_RT kernel).
2. preempt_disable_rt() (will be added by the rt-linux folks).
3. ktime_get() a timestamp before scanout pos query.
4. Do all mmio reads as fast as possible without grabbing any new locks!
5. ktime_get() a post-query timestamp.
6. preempt_enable_rt()
7. Unlock the uncore.lock.

This reduces timestamp uncertainty on a low-end HP Atom Mini netbook
with Intel GMA-950 nicely:

Before: 3-8 usecs with spikes > 20 usecs, triggering query retries.
After : Typically 1 usec (98% of all samples), occassionally 2 usecs
        (2% of all samples), with maximum of 3 usecs (a handful).

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   53 +++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 156a1a4..a3e41d3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -599,35 +599,40 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
 	return I915_READ(reg);
 }
 
-static bool intel_pipe_in_vblank(struct drm_device *dev, enum pipe pipe)
+/* raw reads, only for fast reads of display block, no need for forcewake etc. */
+#define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__))
+#define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__))
+
+static bool intel_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t status;
+	int reg;
 
 	if (IS_VALLEYVIEW(dev)) {
 		status = pipe == PIPE_A ?
 			I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT :
 			I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
 
-		return I915_READ(VLV_ISR) & status;
+		reg = VLV_ISR;
 	} else if (IS_GEN2(dev)) {
 		status = pipe == PIPE_A ?
 			I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT :
 			I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
 
-		return I915_READ16(ISR) & status;
+		reg = ISR;
 	} else if (INTEL_INFO(dev)->gen < 5) {
 		status = pipe == PIPE_A ?
 			I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT :
 			I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
 
-		return I915_READ(ISR) & status;
+		reg = ISR;
 	} else if (INTEL_INFO(dev)->gen < 7) {
 		status = pipe == PIPE_A ?
 			DE_PIPEA_VBLANK :
 			DE_PIPEB_VBLANK;
 
-		return I915_READ(DEISR) & status;
+		reg = DEISR;
 	} else {
 		switch (pipe) {
 		default:
@@ -642,12 +647,17 @@ static bool intel_pipe_in_vblank(struct drm_device *dev, enum pipe pipe)
 			break;
 		}
 
-		return I915_READ(DEISR) & status;
+		reg = DEISR;
 	}
+
+	if (IS_GEN2(dev))
+		return __raw_i915_read16(dev_priv, reg) & status;
+	else
+		return __raw_i915_read32(dev_priv, reg) & status;
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
-			     int *vpos, int *hpos)
+			     int *vpos, int *hpos, ktime_t *stime, ktime_t *etime)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
@@ -657,6 +667,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	int vbl_start, vbl_end, htotal, vtotal;
 	bool in_vbl = true;
 	int ret = 0;
+	unsigned long irqflags;
 
 	if (!intel_crtc->active) {
 		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
@@ -671,14 +682,26 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 
 	ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE;
 
+	/* Lock uncore.lock, as we will do multiple timing critical raw
+	 * register reads, potentially with preemption disabled, so the
+	 * following code must not block on uncore.lock.
+	 */
+	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	
+	/* preempt_disable_rt() should go right here in PREEMPT_RT patchset. */
+
+	/* Get optional system timestamp before query. */
+	if (stime)
+		*stime = ktime_get();
+
 	if (IS_GEN2(dev) || IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
 		/* No obvious pixelcount register. Only query vertical
 		 * scanout position from Display scan line register.
 		 */
 		if (IS_GEN2(dev))
-			position = I915_READ(PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
+			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
 		else
-			position = I915_READ(PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
+			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
 
 		/*
 		 * The scanline counter increments at the leading edge
@@ -687,7 +710,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		 * to get a more accurate picture whether we're in vblank
 		 * or not.
 		 */
-		in_vbl = intel_pipe_in_vblank(dev, pipe);
+		in_vbl = intel_pipe_in_vblank_locked(dev, pipe);
 		if ((in_vbl && position == vbl_start - 1) ||
 		    (!in_vbl && position == vbl_end - 1))
 			position = (position + 1) % vtotal;
@@ -696,7 +719,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		 * We can split this into vertical and horizontal
 		 * scanout position.
 		 */
-		position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT;
+		position = (__raw_i915_read32(dev_priv, PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT;
 
 		/* convert to pixel counts */
 		vbl_start *= htotal;
@@ -704,6 +727,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		vtotal *= htotal;
 	}
 
+	/* Get optional system timestamp after query. */
+	if (etime)
+		*etime = ktime_get();
+
+	/* preempt_enable_rt() should go right here in PREEMPT_RT patchset. */
+
+	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+
 	in_vbl = position >= vbl_start && position < vbl_end;
 
 	/*
-- 
1.7.10.4

  parent reply	other threads:[~2013-10-26  8:27 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-26  8:27 Vblank timestamping improvements/fixes for Linux drm Mario Kleiner
2013-10-26  8:27 ` [PATCH 1/4] drm: Remove preempt_disable() from vblank timestamping code Mario Kleiner
2013-10-26  8:27 ` [PATCH 2/4] drm: Push latency sensitive bits of vblank scanoutpos timestamping into kms drivers Mario Kleiner
2013-10-26  8:27 ` [PATCH 3/4] drm/radeon: Push get_scanout_position() timestamping into kms driver Mario Kleiner
2013-10-26  8:27 ` Mario Kleiner [this message]
2013-10-28 11:08 ` Vblank timestamping improvements/fixes for Linux drm Ville Syrjälä
2013-10-28 15:54 ` Alex Deucher
  -- strict thread matches above, loose matches on Subject: below --
2013-10-30  4:13 Vblank timestamping improvements/fixes for Linux drm. [v2] Mario Kleiner
2013-10-30  4:13 ` [PATCH 4/4] drm/intel: Push get_scanout_position() timestamping into kms driver Mario Kleiner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1382776062-3770-5-git-send-email-mario.kleiner.de@gmail.com \
    --to=mario.kleiner.de@gmail.com \
    --cc=airlied@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).