[PATCH v4 0/9] drm/i915: Atomic sprites v4

[PATCH v4 0/9] drm/i915: Atomic sprites v4

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Another posting of the atomic sprite series. Most things are reviewed, but there
are a few patches in there for the scanline fixup that don't have r-bs.

The HSW+ situation is also still unclear. The scanline counter behaviour depends
on either the actual DDI port used (A vs. rest) or the encoder type (HDMI vs. DP).
If someone wants to help me find out which, grab [1] and run the resulting
intel_poller tool to figure out at which scanline the vblank irq triggers and/or
live surface address gets updated.

Examples from running the tool on my IVB:

# intel_poller -t iir -p 0 -b 0
...
dsl / pipe A / DEIIR[0] (pch):   1078 -   1079

This tells me that the pipe A vblank bit in DEIIR is set when the scanline
counter increments from 1078 to 1079. The display mode was 1920x1080, so this
tells us we need a +1 adjustment for the scaline counter.

# intel_poller -t surflive -p 0
...
dsl / pipe A / Surflive:   1078 -   1079

which tells me that SURFLIVE updates at the same time, as was expected.

You can also run these tests:
# ./intel_poller -t flip -p 0 -l 1078
# ./intel_poller -t pan -p 0 -l 1078

These write the DSPSURF/OFFSET register when the scanline counter reaches the target
value, and overwrite it again with the original value on the next scanline. If the
target scanline is correctly set the picture remains shifted for the duration
of the test, and if you specify any another scanline the picture should remain in
its original position the whole time.

So if someone can run these on HSW and BDW systems with both DP and HDMI outputs
on various ports we can try to figure out what the actual rule is.

Ville Syrjälä (9):
  drm/i915: Fix scanout position for real
  drm/i915: Add intel_get_crtc_scanline()
  drm/i915: Make sprite updates atomic
  drm/i915: Perform primary enable/disable atomically with sprite
    updates
  drm/i915: Add pipe update trace points
  drm/i915: Add a small adjustment to the pixel counter on interlaced
    modes
  drm/i915: Improve gen3/4 frame counter
  drm/i915: Draw a picture about video timings
  drm/i915: Fix gen2 and hsw scanline counter

 drivers/gpu/drm/i915/i915_irq.c      | 231 +++++++++++++++++++++--------------
 drivers/gpu/drm/i915/i915_trace.h    |  75 ++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  50 +++++++-
 drivers/gpu/drm/i915/intel_drv.h     |   5 +
 drivers/gpu/drm/i915/intel_sprite.c  | 231 +++++++++++++++++++++++++++++------
 5 files changed, 463 insertions(+), 129 deletions(-)

-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/9] drm/i915: Fix scanout position for real

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Seems I've been a bit dense with regards to the start of vblank
vs. the scanline counter / pixel counter.

After staring at the pixel counter on gen4 I came to the conclusion
that the start of vblank interrupt and scanline counter increment
happen at the same time. The scanline counter increment is documented
to occur at start of hsync, which means that the start of vblank
interrupt must also trigger there. Looking at the pixel counter value
when the scanline wraps from vtotal-1 to 0 confirms that, as the pixel
counter at that point reads hsync_start. This also clarifies why we see
need the +1 adjustment to the scaline counter. The counter actually
starts counting from vtotal-1 on the first active line.

I also confirmed that the frame start interrupt happens ~1 line after
the start of vblank, but the frame start occurs at hblank_start instead.
We only use the frame start interrupt on gen2 where the start of vblank
interrupt isn't available. The only important thing to note here is that
frame start occurs after vblank start, so we don't have to play any
additional tricks to fix up the scanline counter.

The other thing to note is the fact that the pixel counter on gen3-4
starts counting from the start of horizontal active on the first active
line. That means that when we get the start of vblank interrupt, the
pixel counter reads (htotal*(vblank_start-1)+hsync_start). Since we
consider vblank to start at (htotal*vblank_start) we need to add a
constant (htotal-hsync_start) offset to the pixel counter, or else we
risk misdetecting whether we're in vblank or not.

I talked a bit with Art Runyan about these topics, and he confirmed my
findings. And that the same rules should hold for platforms which don't
have the pixel counter. That's good since without the pixel counter it's
rather difficult to verify the timings to this accuracy.

So the conclusion is that we can throw away all the ISR tricks I added,
and just increment the scanline counter by one always.

Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 102 +++++++++-------------------------------
 1 file changed, 23 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2446e61..a72e635a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -751,26 +751,6 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int 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__))
 
-static bool ilk_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 (INTEL_INFO(dev)->gen >= 8) {
-		status = GEN8_PIPE_VBLANK;
-		reg = GEN8_DE_PIPE_ISR(pipe);
-	} else if (INTEL_INFO(dev)->gen >= 7) {
-		status = DE_PIPE_VBLANK_IVB(pipe);
-		reg = DEISR;
-	} else {
-		status = DE_PIPE_VBLANK(pipe);
-		reg = DEISR;
-	}
-
-	return __raw_i915_read32(dev_priv, reg) & status;
-}
-
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 				    unsigned int flags, int *vpos, int *hpos,
 				    ktime_t *stime, ktime_t *etime)
@@ -780,7 +760,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode;
 	int position;
-	int vbl_start, vbl_end, htotal, vtotal;
+	int vbl_start, vbl_end, hsync_start, htotal, vtotal;
 	bool in_vbl = true;
 	int ret = 0;
 	unsigned long irqflags;
@@ -792,6 +772,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	}
 
 	htotal = mode->crtc_htotal;
+	hsync_start = mode->crtc_hsync_start;
 	vtotal = mode->crtc_vtotal;
 	vbl_start = mode->crtc_vblank_start;
 	vbl_end = mode->crtc_vblank_end;
@@ -810,7 +791,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	 * 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. */
@@ -826,63 +807,15 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		else
 			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
 
-		if (HAS_DDI(dev)) {
-			/*
-			 * On HSW HDMI outputs there seems to be a 2 line
-			 * difference, whereas eDP has the normal 1 line
-			 * difference that earlier platforms have. External
-			 * DP is unknown. For now just check for the 2 line
-			 * difference case on all output types on HSW+.
-			 *
-			 * This might misinterpret the scanline counter being
-			 * one line too far along on eDP, but that's less
-			 * dangerous than the alternative since that would lead
-			 * the vblank timestamp code astray when it sees a
-			 * scanline count before vblank_start during a vblank
-			 * interrupt.
-			 */
-			in_vbl = ilk_pipe_in_vblank_locked(dev, pipe);
-			if ((in_vbl && (position == vbl_start - 2 ||
-					position == vbl_start - 1)) ||
-			    (!in_vbl && (position == vbl_end - 2 ||
-					 position == vbl_end - 1)))
-				position = (position + 2) % vtotal;
-		} else if (HAS_PCH_SPLIT(dev)) {
-			/*
-			 * The scanline counter increments at the leading edge
-			 * of hsync, ie. it completely misses the active portion
-			 * of the line. Fix up the counter at both edges of vblank
-			 * to get a more accurate picture whether we're in vblank
-			 * or not.
-			 */
-			in_vbl = ilk_pipe_in_vblank_locked(dev, pipe);
-			if ((in_vbl && position == vbl_start - 1) ||
-			    (!in_vbl && position == vbl_end - 1))
-				position = (position + 1) % vtotal;
-		} else {
-			/*
-			 * ISR vblank status bits don't work the way we'd want
-			 * them to work on non-PCH platforms (for
-			 * ilk_pipe_in_vblank_locked()), and there doesn't
-			 * appear any other way to determine if we're currently
-			 * in vblank.
-			 *
-			 * Instead let's assume that we're already in vblank if
-			 * we got called from the vblank interrupt and the
-			 * scanline counter value indicates that we're on the
-			 * line just prior to vblank start. This should result
-			 * in the correct answer, unless the vblank interrupt
-			 * delivery really got delayed for almost exactly one
-			 * full frame/field.
-			 */
-			if (flags & DRM_CALLED_FROM_VBLIRQ &&
-			    position == vbl_start - 1) {
-				position = (position + 1) % vtotal;
-
-				/* Signal this correction as "applied". */
-				ret |= 0x8;
-			}
-		}
+		/*
+		 * Scanline counter increments at leading edge of hsync, and
+		 * it starts counting from vtotal-1 on the first active line.
+		 * That means the scanline counter value is always one less
+		 * than what we would expect. Ie. just after start of vblank,
+		 * which also occurs at start of hsync (on the last active line),
+		 * the scanline counter will read vblank_start-1.
+		 */
+		position = (position + 1) % vtotal;
 	} else {
 		/* Have access to pixelcount since start of frame.
 		 * We can split this into vertical and horizontal
@@ -894,6 +827,17 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		vbl_start *= htotal;
 		vbl_end *= htotal;
 		vtotal *= htotal;
+
+		/*
+		 * Start of vblank interrupt is triggered at start of hsync,
+		 * just prior to the first active line of vblank. However we
+		 * consider lines to start at the leading edge of horizontal
+		 * active. So, should we get here before we've crossed into
+		 * the horizontal active of the first line in vblank, we would
+		 * not set the DRM_SCANOUTPOS_INVBL flag. In order to fix that,
+		 * always add htotal-hsync_start to the current pixel position.
+		 */
+		position = (position + htotal - hsync_start) % vtotal;
 	}
 
 	/* Get optional system timestamp after query. */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 2/9] drm/i915: Add intel_get_crtc_scanline()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a new function intel_get_crtc_scanline() that returns the current
scanline counter for the crtc.

v2: Rebase after vblank timestamp changes.
    Use intel_ prefix instead of i915_ as is more customary for
    display related functions.
    Include DRM_SCANOUTPOS_INVBL in the return value even w/o
    adjustments, for a bit of extra consistency.
v3: Change the implementation to be based on DSL on all gens,
    since that's enough for the needs of atomic updates, and
    it will avoid complicating the scanout position calculations
    for the vblank timestamps
v4: Don't break scanline wraparound for interlaced modes

Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  | 56 ++++++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a72e635a..ed30a5e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -751,6 +751,34 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int 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__))
 
+static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	int vtotal = mode->crtc_vtotal;
+	int position;
+
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vtotal /= 2;
+
+	if (IS_GEN2(dev))
+		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
+	else
+		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
+
+	/*
+	 * Scanline counter increments at leading edge of hsync, and
+	 * it starts counting from vtotal-1 on the first active line.
+	 * That means the scanline counter value is always one less
+	 * than what we would expect. Ie. just after start of vblank,
+	 * which also occurs at start of hsync (on the last active line),
+	 * the scanline counter will read vblank_start-1.
+	 */
+	return (position + 1) % vtotal;
+}
+
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 				    unsigned int flags, int *vpos, int *hpos,
 				    ktime_t *stime, ktime_t *etime)
@@ -802,20 +830,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		/* No obvious pixelcount register. Only query vertical
 		 * scanout position from Display scan line register.
 		 */
-		if (IS_GEN2(dev))
-			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
-		else
-			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
-
-		/*
-		 * Scanline counter increments at leading edge of hsync, and
-		 * it starts counting from vtotal-1 on the first active line.
-		 * That means the scanline counter value is always one less
-		 * than what we would expect. Ie. just after start of vblank,
-		 * which also occurs at start of hsync (on the last active line),
-		 * the scanline counter will read vblank_start-1.
-		 */
-		position = (position + 1) % vtotal;
+		position = __intel_get_crtc_scanline(intel_crtc);
 	} else {
 		/* Have access to pixelcount since start of frame.
 		 * We can split this into vertical and horizontal
@@ -876,6 +891,19 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	return ret;
 }
 
+int intel_get_crtc_scanline(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	unsigned long irqflags;
+	int position;
+
+	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	position = __intel_get_crtc_scanline(crtc);
+	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+
+	return position;
+}
+
 static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 			      int *max_error,
 			      struct timeval *vblank_time,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bc77705..96ae78d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -653,6 +653,7 @@ void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
 void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
+int intel_get_crtc_scanline(struct intel_crtc *crtc);
 
 
 /* intel_crt.c */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 3/9] drm/i915: Make sprite updates atomic

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the
vblank interrupt handler, which is a bit dangerous since we set up
interrupts before the crtcs. However in this case since it's the vblank
interrupt, we don't actually unmask it until some piece of code
requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
    Check for min/max scanline <= 0 (Chris)
    Don't call intel_pipe_update_end() if start failed totally (Chris)
    Check that the vblank counters match on both sides of the critical
    section (Chris)
v6: Fix atomic update for interlaced modes
v7: Reorder code for better readability (Chris)
v8: Drop preempt_check_resched(). It's not available to modules
    anymore and isn't even needed unless we ourselves cause
    a wakeup needing reschedule while interrupts are off

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 131 +++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ed30a5e..7e0d577 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1556,6 +1556,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *crtc;
+
+	if (!drm_handle_vblank(dev, pipe))
+		return false;
+
+	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	wake_up(&crtc->vbl_wait);
+
+	return true;
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1607,7 +1620,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1850,7 +1863,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & DE_PIPE_VBLANK(pipe))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1900,7 +1913,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
@@ -2043,7 +2056,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
 		if (pipe_iir & GEN8_PIPE_VBLANK)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3391,7 +3404,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
@@ -3576,7 +3589,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca33ef5..993597b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10566,6 +10566,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 96ae78d..d8b540b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -401,6 +401,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..a3ed840 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,89 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	long timeout = msecs_to_jiffies_timeout(1);
+	int scanline, min, max, vblank_start;
+	DEFINE_WAIT(wait);
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	vblank_start = mode->crtc_vblank_start;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vblank_start = DIV_ROUND_UP(vblank_start, 2);
+
+	/* FIXME needs to be calibrated sensibly */
+	min = vblank_start - usecs_to_scanlines(mode, 100);
+	max = vblank_start - 1;
+
+	if (min <= 0 || max <= 0)
+		return false;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return false;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+
+	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	return true;
+}
+
+static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	local_irq_enable();
+
+	if (start_vbl_count != end_vbl_count)
+		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
+			  pipe_name(pipe), start_vbl_count, end_vbl_count);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,11 +131,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -131,6 +217,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +232,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -152,8 +243,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
@@ -161,6 +257,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -226,10 +325,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -299,6 +401,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +422,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -326,7 +433,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
@@ -336,6 +448,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,10 +525,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -478,6 +596,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +612,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -500,7 +623,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
@@ -509,6 +637,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: FW: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Arun Murthy]

> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.


Here only one sprite update followed by the primary enable/disable can be
achieved atomically. But I feel update of all planes are to be considered, i.e
update of planes per pipe basis to achieve atomicity.

>
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.

This can be achieved easily by checking the previous vblank time in
drm_get_last_vblanktimestamp(), using this the next vblank time cab be
predicted. Using these with the hardcoded value 100usec, a check can be
made to continue or to wait for a vblank. For waiting for a vblank instead
of adding new function just use the available intel_wait_for_vblank().

last_vblank = drm_get_last_vblanktimestamp();
curr_time = do_getttimeofday();
if (curr_time.tv_usec - (last_vblank +  VBLANK_TIME_INTERVAL) > 100)
    /* acquire lock and proceed*/
else
    /* wait for one vblank, acquire lock and proceed */

Thanks and Regards,
Arun R Murthy
-------------------

Re: FW: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Mon, May 19, 2014 at 04:08:09PM +0530, Arun Murthy wrote:
> > Add a mechanism by which we can evade the leading edge of vblank. This
> > guarantees that no two sprite register writes will straddle on either
> > side of the vblank start, and that means all the writes will be latched
> > together in one atomic operation.
> 
> 
> Here only one sprite update followed by the primary enable/disable can be
> achieved atomically. But I feel update of all planes are to be considered, i.e
> update of planes per pipe basis to achieve atomicity.

That's the final goal. This patch introduces the mechanism by which
we can eventually implement the full atomic update.

-- 
Ville Syrjälä
Intel OTC

Re: FW: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Arun Murthy]

On Mon, May 19, 2014 at 4:19 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, May 19, 2014 at 04:08:09PM +0530, Arun Murthy wrote:
>> > Add a mechanism by which we can evade the leading edge of vblank. This
>> > guarantees that no two sprite register writes will straddle on either
>> > side of the vblank start, and that means all the writes will be latched
>> > together in one atomic operation.
>>
>>
>> Here only one sprite update followed by the primary enable/disable can be
>> achieved atomically. But I feel update of all planes are to be considered, i.e
>> update of planes per pipe basis to achieve atomicity.
>
> That's the final goal. This patch introduces the mechanism by which
> we can eventually implement the full atomic update.
>
Instead of tweaking with the existing functions pointers or IOCTL's can adding a
new function pointer in drm_crtc_funcs with element set_display.
This function in turn can set plane, do a page flip and also set few parameters
per pipe basis. Thereby achieving atomicity.
Can this approach work better?

Thanks and Regards,
Arun R Murthy
------------------
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Murthy, Arun R]

> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.

Here only one sprite update followed by the primary enable/disable can be
achieved atomically. But I feel update of all planes are to be 
considered, i.e
update of planes per pipe basis to achieve atomicity.
>
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.

This can be achieved easily by checking the previous vblank time in
drm_get_last_vblanktimestamp(), using this the next vblank time cab be
predicted. Using these with the hardcoded value 100usec, a check can be
made to continue or to wait for a vblank. For waiting for a vblank instead
of adding new function just use the available intel_wait_for_vblank().

last_vblank = drm_get_last_vblanktimestamp();
curr_time = do_getttimeofday();
if (curr_time.tv_usec - (last_vblank +  VBLANK_TIME_INTERVAL) > 100)
     /* acquire lock and proceed*/
else
     /* wait for one vblank, acquire lock and proceed */

Thanks and Regards,
Arun R Murthy
--------------------

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[G, Pallavi]

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of ville.syrjala@linux.intel.com
Sent: Tuesday, April 29, 2014 4:06 PM
To: intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH v8 3/9] drm/i915: Make sprite updates atomic

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This guarantees that no two sprite register writes will straddle on either side of the vblank start, and that means all the writes will be latched together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too close to the start of vblank (too close has been hardcoded to 100usec for now), we will wait for the vblank start to pass. In order to eliminate random delayes from the rest of the system, we operate with interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the vblank interrupt handler, which is a bit dangerous since we set up interrupts before the crtcs. However in this case since it's the vblank interrupt, we don't actually unmask it until some piece of code requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
    Check for min/max scanline <= 0 (Chris)
    Don't call intel_pipe_update_end() if start failed totally (Chris)
    Check that the vblank counters match on both sides of the critical
    section (Chris)
v6: Fix atomic update for interlaced modes
v7: Reorder code for better readability (Chris)
v8: Drop preempt_check_resched(). It's not available to modules
    anymore and isn't even needed unless we ourselves cause
    a wakeup needing reschedule while interrupts are off

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 131 +++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index ed30a5e..7e0d577 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1556,6 +1556,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe 
+pipe) {
+	struct intel_crtc *crtc;
+
+	if (!drm_handle_vblank(dev, pipe))
+		return false;
+
+	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	wake_up(&crtc->vbl_wait);
+
+	return true;
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)  {
 	struct drm_i915_private *dev_priv = dev->dev_private; @@ -1607,7 +1620,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1850,7 +1863,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & DE_PIPE_VBLANK(pipe))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false)) @@ -1900,7 +1913,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) { @@ -2043,7 +2056,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
 		if (pipe_iir & GEN8_PIPE_VBLANK)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_iir & GEN8_PIPE_PRIMARY_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3391,7 +3404,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
@@ -3576,7 +3589,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca33ef5..993597b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10566,6 +10566,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 96ae78d..d8b540b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -401,6 +401,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..a3ed840 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,89 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int usecs_to_scanlines(const struct drm_display_mode *mode, int 
+usecs) {
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * 
+mode->crtc_htotal); }
+
+static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t 
+*start_vbl_count) {
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	long timeout = msecs_to_jiffies_timeout(1);
+	int scanline, min, max, vblank_start;
+	DEFINE_WAIT(wait);
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	vblank_start = mode->crtc_vblank_start;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vblank_start = DIV_ROUND_UP(vblank_start, 2);
+
+	/* FIXME needs to be calibrated sensibly */
+	min = vblank_start - usecs_to_scanlines(mode, 100);
+	max = vblank_start - 1;
+
+	if (min <= 0 || max <= 0)
+		return false;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return false;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+
+	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	return true;
+}

The irq enable/disable appear to be a costly operation. Am suspecting whether we will get 60fps performance from the sprite setplane update.
Becoz we had faced similar kind of performance issues in VLV when sprite planes are enabled we got only 30fps of performance due to wait_for_vblank call before the old fb unpin.
In case of VLV we have one primary plane and two sprite planes, assume we have some scenario like all the three planes are enabled in android and all the three flip calls (primary_page_flip and two setPlane) will try get/put the vblank  and in between we have irq enable/disable. I hope all these things will be solved once your atomic framework in place but now we are trying to call the pageflip ioctls sequentially and with some WA able to achieve some level of atomicity in android.

Also one more thing in Android is that the HAL is keep on requesting vblank event continuously and report back the vblank timestamp to the window manager and window manager will try to sync all the fb update with respect to the hw vblank time. If we disable/enable the irq in between this will create big kiosk in that path also.

+static void intel_pipe_update_end(struct intel_crtc *crtc, u32 
+start_vbl_count) {
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	local_irq_enable();
+
+	if (start_vbl_count != end_vbl_count)
+		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
+			  pipe_name(pipe), start_vbl_count, end_vbl_count); }
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,11 +131,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -131,6 +217,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +232,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -152,8 +243,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
@@ -161,6 +257,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);  }
 
@@ -226,10 +325,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -299,6 +401,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +422,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -326,7 +433,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */ @@ -336,6 +448,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,10 +525,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -478,6 +596,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +612,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -500,7 +623,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
@@ -509,6 +637,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
--
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Daniel Vetter]

On Tue, Apr 29, 2014 at 01:35:46PM +0300, ville.syrjala@linux.intel.com wrote:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 96ae78d..d8b540b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -401,6 +401,8 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		struct intel_pipe_wm active;
>  	} wm;
> +
> +	wait_queue_head_t vbl_wait;
>  };

A bit late, but found something: Why can't we use dev->vblank[crtc].queue
here? We reuse all the vblank infrastructure already ...

If you want pretty just add a tiny helper function

wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc)
{
	return &dev->vblank[drm_crtc_index(crtc)].queue;
}

somewhere.

I feel guilty here since I've killed Akash's patch over adding a new
vblank wait queue and let yours slip through ;-) Can you please take care
of this?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v8 3/9] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Thu, May 22, 2014 at 03:03:56PM +0200, Daniel Vetter wrote:
> On Tue, Apr 29, 2014 at 01:35:46PM +0300, ville.syrjala@linux.intel.com wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 96ae78d..d8b540b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -401,6 +401,8 @@ struct intel_crtc {
> >  		/* watermarks currently being used  */
> >  		struct intel_pipe_wm active;
> >  	} wm;
> > +
> > +	wait_queue_head_t vbl_wait;
> >  };
> 
> A bit late, but found something: Why can't we use dev->vblank[crtc].queue
> here? We reuse all the vblank infrastructure already ...
> 
> If you want pretty just add a tiny helper function
> 
> wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc)
> {
> 	return &dev->vblank[drm_crtc_index(crtc)].queue;
> }
> 
> somewhere.
> 
> I feel guilty here since I've killed Akash's patch over adding a new
> vblank wait queue and let yours slip through ;-) Can you please take care
> of this?

Can do.

-- 
Ville Syrjälä
Intel OTC

[PATCH v2 4/9] drm/i915: Perform primary enable/disable atomically with sprite updates

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move the primary plane enable/disable to occur atomically with the
sprite update that caused the primary plane visibility to change.

FBC and IPS enable/disable is left to happen well before or after
the primary plane change.

v2: Pass intel_crtc instead of drm_crtc (Daniel)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 94 ++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a3ed840..6192e58 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -120,6 +120,17 @@ static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 			  pipe_name(pipe), start_vbl_count, end_vbl_count);
 }
 
+static void intel_update_primary_plane(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	int reg = DSPCNTR(crtc->plane);
+
+	if (crtc->primary_enabled)
+		I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
+	else
+		I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -219,6 +230,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -231,7 +244,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPCNTR(pipe, plane), sprctl);
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
@@ -251,11 +265,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
 	/* Activate double buffered register update */
 	I915_WRITE(SPSURF(pipe, plane), 0);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
@@ -403,6 +420,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -421,7 +440,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRCTL(pipe), sprctl);
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
@@ -440,13 +460,16 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
 	if (intel_plane->can_scale)
 		I915_WRITE(SPRSCALE(pipe), 0);
 	/* Activate double buffered register update */
 	I915_WRITE(SPRSURF(pipe), 0);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
@@ -598,6 +621,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -611,7 +636,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSCNTR(pipe), dvscntr);
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
@@ -630,12 +656,15 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
 	I915_WRITE(DVSSCALE(pipe), 0);
 	/* Flush double buffered register updates */
 	I915_WRITE(DVSSURF(pipe), 0);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count);
@@ -650,20 +679,10 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 }
 
 static void
-intel_enable_primary(struct drm_crtc *crtc)
+intel_post_enable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = true;
-
-	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is
@@ -682,17 +701,11 @@ intel_enable_primary(struct drm_crtc *crtc)
 }
 
 static void
-intel_disable_primary(struct drm_crtc *crtc)
+intel_pre_disable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (!intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = false;
 
 	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == intel_crtc->plane)
@@ -706,9 +719,6 @@ intel_disable_primary(struct drm_crtc *crtc)
 	 * versa.
 	 */
 	hsw_disable_ips(intel_crtc);
-
-	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 }
 
 static int
@@ -802,7 +812,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int ret;
-	bool disable_primary = false;
+	bool primary_enabled;
 	bool visible;
 	int hscale, vscale;
 	int max_scale, min_scale;
@@ -973,8 +983,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
-	WARN_ON(disable_primary && !visible && intel_crtc->active);
+	primary_enabled = !drm_rect_equals(&dst, &clip) || colorkey_enabled(intel_plane);
+	WARN_ON(!primary_enabled && !visible && intel_crtc->active);
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -1001,12 +1011,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		/*
-		 * Be sure to re-enable the primary before the sprite is no longer
-		 * covering it fully.
-		 */
-		if (!disable_primary)
-			intel_enable_primary(crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = primary_enabled;
+
+		if (primary_was_enabled && !primary_enabled)
+			intel_pre_disable_primary(crtc);
 
 		if (visible)
 			intel_plane->update_plane(plane, crtc, fb, obj,
@@ -1015,8 +1025,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		else
 			intel_plane->disable_plane(plane, crtc);
 
-		if (disable_primary)
-			intel_disable_primary(crtc);
+		if (!primary_was_enabled && primary_enabled)
+			intel_post_enable_primary(crtc);
 	}
 
 	/* Unpin old obj after new one is active to avoid ugliness */
@@ -1054,8 +1064,14 @@ intel_disable_plane(struct drm_plane *plane)
 	intel_crtc = to_intel_crtc(plane->crtc);
 
 	if (intel_crtc->active) {
-		intel_enable_primary(plane->crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = true;
+
 		intel_plane->disable_plane(plane, plane->crtc);
+
+		if (!primary_was_enabled && intel_crtc->primary_enabled)
+			intel_post_enable_primary(plane->crtc);
 	}
 
 	if (intel_plane->obj) {
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/9] drm/i915: Perform primary enable/disable atomically with sprite updates

[G, Pallavi]

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of ville.syrjala@linux.intel.com
Sent: Tuesday, April 29, 2014 4:06 PM
To: intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH v2 4/9] drm/i915: Perform primary enable/disable atomically with sprite updates

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move the primary plane enable/disable to occur atomically with the sprite update that caused the primary plane visibility to change.

FBC and IPS enable/disable is left to happen well before or after the primary plane change.

v2: Pass intel_crtc instead of drm_crtc (Daniel)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 94 ++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a3ed840..6192e58 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -120,6 +120,17 @@ static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 			  pipe_name(pipe), start_vbl_count, end_vbl_count);  }
 
+static void intel_update_primary_plane(struct intel_crtc *crtc) {
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	int reg = DSPCNTR(crtc->plane);
+
+	if (crtc->primary_enabled)
+		I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
+	else
+		I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE); }
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -219,6 +230,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -231,7 +244,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPCNTR(pipe, plane), sprctl);
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count); @@ -251,11 +265,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
 	/* Activate double buffered register update */
 	I915_WRITE(SPSURF(pipe, plane), 0);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count); @@ -403,6 +420,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -421,7 +440,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRCTL(pipe), sprctl);
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count); @@ -440,13 +460,16 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
 	if (intel_plane->can_scale)
 		I915_WRITE(SPRSCALE(pipe), 0);
 	/* Activate double buffered register update */
 	I915_WRITE(SPRSURF(pipe), 0);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count); @@ -598,6 +621,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -611,7 +636,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSCNTR(pipe), dvscntr);
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count); @@ -630,12 +656,15 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
 	I915_WRITE(DVSSCALE(pipe), 0);
 	/* Flush double buffered register updates */
 	I915_WRITE(DVSSURF(pipe), 0);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	if (atomic_update)
 		intel_pipe_update_end(intel_crtc, start_vbl_count); @@ -650,20 +679,10 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)  }
 
 static void
-intel_enable_primary(struct drm_crtc *crtc)
+intel_post_enable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = true;
-
-	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is @@ -682,17 +701,11 @@ intel_enable_primary(struct drm_crtc *crtc)  }
 
 static void
-intel_disable_primary(struct drm_crtc *crtc)
+intel_pre_disable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (!intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = false;
 
 	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == intel_crtc->plane) @@ -706,9 +719,6 @@ intel_disable_primary(struct drm_crtc *crtc)
 	 * versa.
 	 */
 	hsw_disable_ips(intel_crtc);
-
-	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 }
 
 static int
@@ -802,7 +812,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int ret;
-	bool disable_primary = false;
+	bool primary_enabled;
 	bool visible;
 	int hscale, vscale;
 	int max_scale, min_scale;
@@ -973,8 +983,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
-	WARN_ON(disable_primary && !visible && intel_crtc->active);
+	primary_enabled = !drm_rect_equals(&dst, &clip) || colorkey_enabled(intel_plane);
+	WARN_ON(!primary_enabled && !visible && intel_crtc->active);
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -1001,12 +1011,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		/*
-		 * Be sure to re-enable the primary before the sprite is no longer
-		 * covering it fully.
-		 */
-		if (!disable_primary)
-			intel_enable_primary(crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = primary_enabled;
+
+		if (primary_was_enabled && !primary_enabled)
+			intel_pre_disable_primary(crtc);
 
 		if (visible)
 			intel_plane->update_plane(plane, crtc, fb, obj, @@ -1015,8 +1025,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		else
 			intel_plane->disable_plane(plane, crtc);
 
-		if (disable_primary)
-			intel_disable_primary(crtc);
+		if (!primary_was_enabled && primary_enabled)
+			intel_post_enable_primary(crtc);
 	}
 
 	/* Unpin old obj after new one is active to avoid ugliness */ @@ -1054,8 +1064,14 @@ intel_disable_plane(struct drm_plane *plane)
 	intel_crtc = to_intel_crtc(plane->crtc);
 
 	if (intel_crtc->active) {
-		intel_enable_primary(plane->crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = true;
+
 		intel_plane->disable_plane(plane, plane->crtc);
+
+		if (!primary_was_enabled && intel_crtc->primary_enabled)
+			intel_post_enable_primary(plane->crtc);
 	}
 
 	if (intel_plane->obj) {
--
1.8.3.2

Hi Ville,

We are not considering the transparency and the variable Z-order stuffs.
In case of the VLV the Z-order is not fixed we can program any plane on any order PASASBCA or SAPASBCA and even if the sprite covers the entire primary plane, part of the content on the sprite plane can be transparent/opaque. 

This won't work in case of Android scenario where we faced the similar problem and disabled this primary enable/disable as a temp WA.

Apart from this rest of the code looks fine.

Thanks,
Pallavi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 5/9] drm/i915: Add pipe update trace points

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add trace points for observing the atomic pipe update mechanism.

v2: Rebased due to earlier changes
v3: Pass intel_crtc instead of drm_crtc (Daniel)
v4: Pass frame counter from the caller to evaded/end since
    the caller now always has that ready

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_trace.h   | 75 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c |  6 +++
 2 files changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 23c26f1..b29d7b1 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -7,6 +7,7 @@
 
 #include <drm/drmP.h>
 #include "i915_drv.h"
+#include "intel_drv.h"
 #include "intel_ringbuffer.h"
 
 #undef TRACE_SYSTEM
@@ -14,6 +15,80 @@
 #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM)
 #define TRACE_INCLUDE_FILE i915_trace
 
+/* pipe updates */
+
+TRACE_EVENT(i915_pipe_update_start,
+	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max),
+	    TP_ARGS(crtc, min, max),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
+										       crtc->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_vblank_evaded,
+	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max, u32 frame),
+	    TP_ARGS(crtc, min, max, frame),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = frame;
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_end,
+	    TP_PROTO(struct intel_crtc *crtc, u32 frame),
+	    TP_ARGS(crtc, frame),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = frame;
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		      __entry->scanline)
+);
+
 /* object tracking */
 
 TRACE_EVENT(i915_gem_object_create,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 6192e58..213cd58 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -73,6 +73,8 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
 
 	local_irq_disable();
 
+	trace_i915_pipe_update_start(crtc, min, max);
+
 	for (;;) {
 		/*
 		 * prepare_to_wait() has a memory barrier, which guarantees
@@ -104,6 +106,8 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
 
 	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
 
+	trace_i915_pipe_update_vblank_evaded(crtc, min, max, *start_vbl_count);
+
 	return true;
 }
 
@@ -113,6 +117,8 @@ static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 	enum pipe pipe = crtc->pipe;
 	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
 
+	trace_i915_pipe_update_end(crtc, end_vbl_count);
+
 	local_irq_enable();
 
 	if (start_vbl_count != end_vbl_count)
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 5/9] drm/i915: Add pipe update trace points

[Daniel Vetter]

On Tue, Apr 29, 2014 at 01:35:48PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add trace points for observing the atomic pipe update mechanism.
> 
> v2: Rebased due to earlier changes
> v3: Pass intel_crtc instead of drm_crtc (Daniel)
> v4: Pass frame counter from the caller to evaded/end since
>     the caller now always has that ready
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
> Reviewed-by: Akash Goel <akash.goels@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Merged up to this one here. Can you please update the review-AR for Akash
and Sourab for the remaining patches?

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/i915_trace.h   | 75 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_sprite.c |  6 +++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 23c26f1..b29d7b1 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -7,6 +7,7 @@
>  
>  #include <drm/drmP.h>
>  #include "i915_drv.h"
> +#include "intel_drv.h"
>  #include "intel_ringbuffer.h"
>  
>  #undef TRACE_SYSTEM
> @@ -14,6 +15,80 @@
>  #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM)
>  #define TRACE_INCLUDE_FILE i915_trace
>  
> +/* pipe updates */
> +
> +TRACE_EVENT(i915_pipe_update_start,
> +	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max),
> +	    TP_ARGS(crtc, min, max),
> +
> +	    TP_STRUCT__entry(
> +			     __field(enum pipe, pipe)
> +			     __field(u32, frame)
> +			     __field(u32, scanline)
> +			     __field(u32, min)
> +			     __field(u32, max)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->pipe = crtc->pipe;
> +			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
> +										       crtc->pipe);
> +			   __entry->scanline = intel_get_crtc_scanline(crtc);
> +			   __entry->min = min;
> +			   __entry->max = max;
> +			   ),
> +
> +	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
> +		      pipe_name(__entry->pipe), __entry->frame,
> +		       __entry->scanline, __entry->min, __entry->max)
> +);
> +
> +TRACE_EVENT(i915_pipe_update_vblank_evaded,
> +	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max, u32 frame),
> +	    TP_ARGS(crtc, min, max, frame),
> +
> +	    TP_STRUCT__entry(
> +			     __field(enum pipe, pipe)
> +			     __field(u32, frame)
> +			     __field(u32, scanline)
> +			     __field(u32, min)
> +			     __field(u32, max)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->pipe = crtc->pipe;
> +			   __entry->frame = frame;
> +			   __entry->scanline = intel_get_crtc_scanline(crtc);
> +			   __entry->min = min;
> +			   __entry->max = max;
> +			   ),
> +
> +	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
> +		      pipe_name(__entry->pipe), __entry->frame,
> +		       __entry->scanline, __entry->min, __entry->max)
> +);
> +
> +TRACE_EVENT(i915_pipe_update_end,
> +	    TP_PROTO(struct intel_crtc *crtc, u32 frame),
> +	    TP_ARGS(crtc, frame),
> +
> +	    TP_STRUCT__entry(
> +			     __field(enum pipe, pipe)
> +			     __field(u32, frame)
> +			     __field(u32, scanline)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->pipe = crtc->pipe;
> +			   __entry->frame = frame;
> +			   __entry->scanline = intel_get_crtc_scanline(crtc);
> +			   ),
> +
> +	    TP_printk("pipe %c, frame=%u, scanline=%u",
> +		      pipe_name(__entry->pipe), __entry->frame,
> +		      __entry->scanline)
> +);
> +
>  /* object tracking */
>  
>  TRACE_EVENT(i915_gem_object_create,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 6192e58..213cd58 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -73,6 +73,8 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
>  
>  	local_irq_disable();
>  
> +	trace_i915_pipe_update_start(crtc, min, max);
> +
>  	for (;;) {
>  		/*
>  		 * prepare_to_wait() has a memory barrier, which guarantees
> @@ -104,6 +106,8 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
>  
>  	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
>  
> +	trace_i915_pipe_update_vblank_evaded(crtc, min, max, *start_vbl_count);
> +
>  	return true;
>  }
>  
> @@ -113,6 +117,8 @@ static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
>  	enum pipe pipe = crtc->pipe;
>  	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
>  
> +	trace_i915_pipe_update_end(crtc, end_vbl_count);
> +
>  	local_irq_enable();
>  
>  	if (start_vbl_count != end_vbl_count)
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v4 5/9] drm/i915: Add pipe update trace points

[Ville Syrjälä]

On Tue, Apr 29, 2014 at 01:17:12PM +0200, Daniel Vetter wrote:
> On Tue, Apr 29, 2014 at 01:35:48PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add trace points for observing the atomic pipe update mechanism.
> > 
> > v2: Rebased due to earlier changes
> > v3: Pass intel_crtc instead of drm_crtc (Daniel)
> > v4: Pass frame counter from the caller to evaded/end since
> >     the caller now always has that ready
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Reviewed-by: Sourab Gupta <sourabgupta@gmail.com>
> > Reviewed-by: Akash Goel <akash.goels@gmail.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Merged up to this one here. Can you please update the review-AR for Akash
> and Sourab for the remaining patches?

Board updated.

Akash and Sourab, please look through the remaining patches (6-9). I
know it's a bit vague until we get the HSW+ situation resolved, but
even the current patch should be a bit more correct than the code we
have at the moment.

-- 
Ville Syrjälä
Intel OTC

[PATCH 6/9] drm/i915: Add a small adjustment to the pixel counter on interlaced modes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

In interlaced modes, the pixel counter counts all pixels,
so one field will have htotal more pixels. In order to avoid
the reported position from jumping backwards when the pixel
counter is beyond the length of the shorter field, just
clamp the position the length of the shorter field. This
matches how the scanline counter based position works since
the scanline counter doesn't count the two half lines.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7e0d577..64cd888 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -844,6 +844,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		vtotal *= htotal;
 
 		/*
+		 * In interlaced modes, the pixel counter counts all pixels,
+		 * so one field will have htotal more pixels. In order to avoid
+		 * the reported position from jumping backwards when the pixel
+		 * counter is beyond the length of the shorter field, just
+		 * clamp the position the length of the shorter field. This
+		 * matches how the scanline counter based position works since
+		 * the scanline counter doesn't count the two half lines.
+		 */
+		if (position >= vtotal)
+			position = vtotal - 1;
+
+		/*
 		 * Start of vblank interrupt is triggered at start of hsync,
 		 * just prior to the first active line of vblank. However we
 		 * consider lines to start at the leading edge of horizontal
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915: Add a small adjustment to the pixel counter on interlaced modes

[akash goel]

[-- Attachment #1.1: Type: text/plain, Size: 2507 bytes --]

Reviewed the patch & it looks good.
Just to confirm, this patch tries to address the case of a tiny window of
transition, i.e. from the 1st field (last half line) to 2nd field (first
half line).

Reviewed-by: "Akash Goel <akash.goels@gmail.com>"


On Tue, Apr 29, 2014 at 4:05 PM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> In interlaced modes, the pixel counter counts all pixels,
> so one field will have htotal more pixels. In order to avoid
> the reported position from jumping backwards when the pixel
> counter is beyond the length of the shorter field, just
> clamp the position the length of the shorter field. This
> matches how the scanline counter based position works since
> the scanline counter doesn't count the two half lines.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index 7e0d577..64cd888 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -844,6 +844,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device
> *dev, int pipe,
>                 vtotal *= htotal;
>
>                 /*
> +                * In interlaced modes, the pixel counter counts all
> pixels,
> +                * so one field will have htotal more pixels. In order to
> avoid
> +                * the reported position from jumping backwards when the
> pixel
> +                * counter is beyond the length of the shorter field, just
> +                * clamp the position the length of the shorter field. This
> +                * matches how the scanline counter based position works
> since
> +                * the scanline counter doesn't count the two half lines.
> +                */
> +               if (position >= vtotal)
> +                       position = vtotal - 1;
> +
> +               /*
>                  * Start of vblank interrupt is triggered at start of
> hsync,
>                  * just prior to the first active line of vblank. However
> we
>                  * consider lines to start at the leading edge of
> horizontal
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 3585 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915: Add a small adjustment to the pixel counter on interlaced modes

[sourab gupta]

[-- Attachment #1.1: Type: text/plain, Size: 2960 bytes --]

On Thu, May 15, 2014 at 8:23 PM, akash goel <akash.goels@gmail.com> wrote:

> Reviewed the patch & it looks good.
> Just to confirm, this patch tries to address the case of a tiny window of
> transition, i.e. from the 1st field (last half line) to 2nd field (first
> half line).
>
> Reviewed-by: "Akash Goel <akash.goels@gmail.com>"
>
>
> Reviewed the patch and looks fine. You can add my r-b tag:

Reviewed-by: "Sourab Gupta <sourabgupta@gmail.com>"

On Tue, Apr 29, 2014 at 4:05 PM, <ville.syrjala@linux.intel.com> wrote:
>
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> In interlaced modes, the pixel counter counts all pixels,
>> so one field will have htotal more pixels. In order to avoid
>> the reported position from jumping backwards when the pixel
>> counter is beyond the length of the shorter field, just
>> clamp the position the length of the shorter field. This
>> matches how the scanline counter based position works since
>> the scanline counter doesn't count the two half lines.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 7e0d577..64cd888 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -844,6 +844,18 @@ static int i915_get_crtc_scanoutpos(struct
>> drm_device *dev, int pipe,
>>                 vtotal *= htotal;
>>
>>                 /*
>> +                * In interlaced modes, the pixel counter counts all
>> pixels,
>> +                * so one field will have htotal more pixels. In order to
>> avoid
>> +                * the reported position from jumping backwards when the
>> pixel
>> +                * counter is beyond the length of the shorter field, just
>> +                * clamp the position the length of the shorter field.
>> This
>> +                * matches how the scanline counter based position works
>> since
>> +                * the scanline counter doesn't count the two half lines.
>> +                */
>> +               if (position >= vtotal)
>> +                       position = vtotal - 1;
>> +
>> +               /*
>>                  * Start of vblank interrupt is triggered at start of
>> hsync,
>>                  * just prior to the first active line of vblank. However
>> we
>>                  * consider lines to start at the leading edge of
>> horizontal
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

[-- Attachment #1.2: Type: text/html, Size: 4799 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915: Add a small adjustment to the pixel counter on interlaced modes

[Ville Syrjälä]

On Thu, May 15, 2014 at 08:23:47PM +0530, akash goel wrote:
> Reviewed the patch & it looks good.
> Just to confirm, this patch tries to address the case of a tiny window of
> transition, i.e. from the 1st field (last half line) to 2nd field (first
> half line).

The hardware counts things so that one field ends up being one line
taller than the other, ie. both half lines get accounted for the same
field (as far as the pixel counter is concerned at least).

So the total number of pixels in the fields is like this:
field A = htotal * floor(vtotal/2)
field B = htotal *  ceil(vtotal/2)

But when we compute the scanout position we assume that the total
number of pixels is always 'htotal * floor(vtotal/2)'. So if we start
with a number that is greater than that, the value wraps back to zero
already before we reach the real 0. And then when we do reach the
real zero, the scanout position would appear to jump backwards. So this
patch eliminates that problem by making it appear that the scanout
position stops moving when we're on that last line of the taller field.
Occasionally non-moving (but still non-decreasing) scanout position
seems like a lesser evil than one that jumps back and forth at times.

And as stated, this more or less matches the scanline counter based
scanout position since the scanline counter doesn't include the half
lines.

> 
> Reviewed-by: "Akash Goel <akash.goels@gmail.com>"
> 
> 
> On Tue, Apr 29, 2014 at 4:05 PM, <ville.syrjala@linux.intel.com> wrote:
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > In interlaced modes, the pixel counter counts all pixels,
> > so one field will have htotal more pixels. In order to avoid
> > the reported position from jumping backwards when the pixel
> > counter is beyond the length of the shorter field, just
> > clamp the position the length of the shorter field. This
> > matches how the scanline counter based position works since
> > the scanline counter doesn't count the two half lines.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 7e0d577..64cd888 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -844,6 +844,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device
> > *dev, int pipe,
> >                 vtotal *= htotal;
> >
> >                 /*
> > +                * In interlaced modes, the pixel counter counts all
> > pixels,
> > +                * so one field will have htotal more pixels. In order to
> > avoid
> > +                * the reported position from jumping backwards when the
> > pixel
> > +                * counter is beyond the length of the shorter field, just
> > +                * clamp the position the length of the shorter field. This
> > +                * matches how the scanline counter based position works
> > since
> > +                * the scanline counter doesn't count the two half lines.
> > +                */
> > +               if (position >= vtotal)
> > +                       position = vtotal - 1;
> > +
> > +               /*
> >                  * Start of vblank interrupt is triggered at start of
> > hsync,
> >                  * just prior to the first active line of vblank. However
> > we
> >                  * consider lines to start at the leading edge of
> > horizontal
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

-- 
Ville Syrjälä
Intel OTC

[PATCH 7/9] drm/i915: Improve gen3/4 frame counter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently the logic to fix up the frame counter on gen3/4 assumes that
start of vblank occurs at vblank_start*htotal pixels, when in fact
it occurs htotal-hsync_start pixels earlier. Apply the appropriate
adjustment to make the frame counter more accurate.

Also fix the vblank start position for interlaced display modes.

Reviewed-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 64cd888..742f276 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -683,7 +683,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long high_frame;
 	unsigned long low_frame;
-	u32 high1, high2, low, pixel, vbl_start;
+	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
 
 	if (!i915_pipe_enabled(dev, pipe)) {
 		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
@@ -697,17 +697,28 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 		const struct drm_display_mode *mode =
 			&intel_crtc->config.adjusted_mode;
 
-		vbl_start = mode->crtc_vblank_start * mode->crtc_htotal;
+		htotal = mode->crtc_htotal;
+		hsync_start = mode->crtc_hsync_start;
+		vbl_start = mode->crtc_vblank_start;
+		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+			vbl_start = DIV_ROUND_UP(vbl_start, 2);
 	} else {
 		enum transcoder cpu_transcoder = (enum transcoder) pipe;
-		u32 htotal;
 
 		htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff) + 1;
+		hsync_start = (I915_READ(HSYNC(cpu_transcoder))  & 0x1fff) + 1;
 		vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff) + 1;
-
-		vbl_start *= htotal;
+		if ((I915_READ(PIPECONF(cpu_transcoder)) &
+		     PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
+			vbl_start = DIV_ROUND_UP(vbl_start, 2);
 	}
 
+	/* Convert to pixel count */
+	vbl_start *= htotal;
+
+	/* Start of vblank event occurs at start of hsync */
+	vbl_start -= htotal - hsync_start;
+
 	high_frame = PIPEFRAME(pipe);
 	low_frame = PIPEFRAMEPIXEL(pipe);
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915: Improve gen3/4 frame counter

[akash goel]

[-- Attachment #1.1: Type: text/plain, Size: 3208 bytes --]

Reviewed the patch & it looks fine.

Reviewed-by: "Akash Goel <akash.goels@gmail.com>"
On Tue, Apr 29, 2014 at 4:05 PM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently the logic to fix up the frame counter on gen3/4 assumes that
> start of vblank occurs at vblank_start*htotal pixels, when in fact
> it occurs htotal-hsync_start pixels earlier. Apply the appropriate
> adjustment to make the frame counter more accurate.
>
> Also fix the vblank start position for interlaced display modes.
>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index 64cd888..742f276 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -683,7 +683,7 @@ static u32 i915_get_vblank_counter(struct drm_device
> *dev, int pipe)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         unsigned long high_frame;
>         unsigned long low_frame;
> -       u32 high1, high2, low, pixel, vbl_start;
> +       u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
>
>         if (!i915_pipe_enabled(dev, pipe)) {
>                 DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> @@ -697,17 +697,28 @@ static u32 i915_get_vblank_counter(struct drm_device
> *dev, int pipe)
>                 const struct drm_display_mode *mode =
>                         &intel_crtc->config.adjusted_mode;
>
> -               vbl_start = mode->crtc_vblank_start * mode->crtc_htotal;
> +               htotal = mode->crtc_htotal;
> +               hsync_start = mode->crtc_hsync_start;
> +               vbl_start = mode->crtc_vblank_start;
> +               if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +                       vbl_start = DIV_ROUND_UP(vbl_start, 2);
>         } else {
>                 enum transcoder cpu_transcoder = (enum transcoder) pipe;
> -               u32 htotal;
>
>                 htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) &
> 0x1fff) + 1;
> +               hsync_start = (I915_READ(HSYNC(cpu_transcoder))  & 0x1fff)
> + 1;
>                 vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff) +
> 1;
> -
> -               vbl_start *= htotal;
> +               if ((I915_READ(PIPECONF(cpu_transcoder)) &
> +                    PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
> +                       vbl_start = DIV_ROUND_UP(vbl_start, 2);
>         }
>
> +       /* Convert to pixel count */
> +       vbl_start *= htotal;
> +
> +       /* Start of vblank event occurs at start of hsync */
> +       vbl_start -= htotal - hsync_start;
> +
>         high_frame = PIPEFRAME(pipe);
>         low_frame = PIPEFRAMEPIXEL(pipe);
>
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 4879 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915: Improve gen3/4 frame counter

[sourab gupta]

[-- Attachment #1.1: Type: text/plain, Size: 3654 bytes --]

Reviewed the patch. Looks good.

Reviewed-by: "Sourab Gupta <sourabgupta@gmail.com>"


On Thu, May 15, 2014 at 8:16 PM, akash goel <akash.goels@gmail.com> wrote:

>
>
> Reviewed the patch & it looks fine.
>
> Reviewed-by: "Akash Goel <akash.goels@gmail.com>"
> On Tue, Apr 29, 2014 at 4:05 PM, <ville.syrjala@linux.intel.com> wrote:
>
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Currently the logic to fix up the frame counter on gen3/4 assumes that
>> start of vblank occurs at vblank_start*htotal pixels, when in fact
>> it occurs htotal-hsync_start pixels earlier. Apply the appropriate
>> adjustment to make the frame counter more accurate.
>>
>> Also fix the vblank start position for interlaced display modes.
>>
>> Reviewed-by: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 64cd888..742f276 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -683,7 +683,7 @@ static u32 i915_get_vblank_counter(struct drm_device
>> *dev, int pipe)
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         unsigned long high_frame;
>>         unsigned long low_frame;
>> -       u32 high1, high2, low, pixel, vbl_start;
>> +       u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
>>
>>         if (!i915_pipe_enabled(dev, pipe)) {
>>                 DRM_DEBUG_DRIVER("trying to get vblank count for disabled
>> "
>> @@ -697,17 +697,28 @@ static u32 i915_get_vblank_counter(struct
>> drm_device *dev, int pipe)
>>                 const struct drm_display_mode *mode =
>>                         &intel_crtc->config.adjusted_mode;
>>
>> -               vbl_start = mode->crtc_vblank_start * mode->crtc_htotal;
>> +               htotal = mode->crtc_htotal;
>> +               hsync_start = mode->crtc_hsync_start;
>> +               vbl_start = mode->crtc_vblank_start;
>> +               if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> +                       vbl_start = DIV_ROUND_UP(vbl_start, 2);
>>         } else {
>>                 enum transcoder cpu_transcoder = (enum transcoder) pipe;
>> -               u32 htotal;
>>
>>                 htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) &
>> 0x1fff) + 1;
>> +               hsync_start = (I915_READ(HSYNC(cpu_transcoder))  &
>> 0x1fff) + 1;
>>                 vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff)
>> + 1;
>> -
>> -               vbl_start *= htotal;
>> +               if ((I915_READ(PIPECONF(cpu_transcoder)) &
>> +                    PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
>> +                       vbl_start = DIV_ROUND_UP(vbl_start, 2);
>>         }
>>
>> +       /* Convert to pixel count */
>> +       vbl_start *= htotal;
>> +
>> +       /* Start of vblank event occurs at start of hsync */
>> +       vbl_start -= htotal - hsync_start;
>> +
>>         high_frame = PIPEFRAME(pipe);
>>         low_frame = PIPEFRAMEPIXEL(pipe);
>>
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

[-- Attachment #1.2: Type: text/html, Size: 5795 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 8/9] drm/i915: Draw a picture about video timings

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The docs are a bit lacking when it comes to describing when certain
timing related events occur in the hardware. Draw a picture which
tries to capture the most important ones.

v2: Clarify a few details (Imre)

Acked-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 742f276..bc4585b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -669,6 +669,55 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
 	}
 }
 
+/*
+ * This timing diagram depicts the video signal in and
+ * around the vertical blanking period.
+ *
+ * Assumptions about the fictitious mode used in this example:
+ *  vblank_start >= 3
+ *  vsync_start = vblank_start + 1
+ *  vsync_end = vblank_start + 2
+ *  vtotal = vblank_start + 3
+ *
+ *           start of vblank:
+ *           latch double buffered registers
+ *           increment frame counter (ctg+)
+ *           generate start of vblank interrupt (gen4+)
+ *           |
+ *           |          frame start:
+ *           |          generate frame start interrupt (aka. vblank interrupt) (gmch)
+ *           |          may be shifted forward 1-3 extra lines via PIPECONF
+ *           |          |
+ *           |          |  start of vsync:
+ *           |          |  generate vsync interrupt
+ *           |          |  |
+ * ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___
+ *       .   \hs/   .      \hs/          \hs/          \hs/   .      \hs/
+ * ----va---> <-----------------vb--------------------> <--------va---------
+ *       |          |       <----vs----->                     |
+ * -vbs-----> <---vbs+1---> <---vbs+2---> <-----0-----> <-----1-----> <---2- (scanline counter gen2)
+ * -vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1---> <---vbs+2---> <---0- (scanline counter gen3+)
+ *       |          |                                         |
+ *       last visible pixel                                   first visible pixel
+ *                  |                                         increment frame counter (gen3/4)
+ *                  pixel counter = vblank_start * htotal     pixel counter = 0 (gen3/4)
+ *
+ * x  = horizontal active
+ * _  = horizontal blanking
+ * hs = horizontal sync
+ * va = vertical active
+ * vb = vertical blanking
+ * vs = vertical sync
+ * vbs = vblank_start (number)
+ *
+ * Summary:
+ * - most events happen at the start of horizontal sync
+ * - frame start happens at the start of horizontal blank, 1-4 lines
+ *   (depending on PIPECONF settings) after the start of vblank
+ * - gen3/4 pixel and frame counter are synchronized with the start
+ *   of horizontal active on the first line of vertical active
+ */
+
 static u32 i8xx_get_vblank_counter(struct drm_device *dev, int pipe)
 {
 	/* Gen2 doesn't have a hardware frame counter */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 8/9] drm/i915: Draw a picture about video timings

[akash goel]

[-- Attachment #1.1: Type: text/plain, Size: 3750 bytes --]

Nice illustration of the various events in a Video signal.

Reviewed-by: "Akash Goel <akash.goels@gmail.com>"


On Tue, Apr 29, 2014 at 4:05 PM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The docs are a bit lacking when it comes to describing when certain
> timing related events occur in the hardware. Draw a picture which
> tries to capture the most important ones.
>
> v2: Clarify a few details (Imre)
>
> Acked-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 49
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index 742f276..bc4585b 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -669,6 +669,55 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
>         }
>  }
>
> +/*
> + * This timing diagram depicts the video signal in and
> + * around the vertical blanking period.
> + *
> + * Assumptions about the fictitious mode used in this example:
> + *  vblank_start >= 3
> + *  vsync_start = vblank_start + 1
> + *  vsync_end = vblank_start + 2
> + *  vtotal = vblank_start + 3
> + *
> + *           start of vblank:
> + *           latch double buffered registers
> + *           increment frame counter (ctg+)
> + *           generate start of vblank interrupt (gen4+)
> + *           |
> + *           |          frame start:
> + *           |          generate frame start interrupt (aka. vblank
> interrupt) (gmch)
> + *           |          may be shifted forward 1-3 extra lines via
> PIPECONF
> + *           |          |
> + *           |          |  start of vsync:
> + *           |          |  generate vsync interrupt
> + *           |          |  |
> + * ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___
>  ___
> + *       .   \hs/   .      \hs/          \hs/          \hs/   .      \hs/
> + * ----va---> <-----------------vb-------------------->
> <--------va---------
> + *       |          |       <----vs----->                     |
> + * -vbs-----> <---vbs+1---> <---vbs+2---> <-----0-----> <-----1----->
> <---2- (scanline counter gen2)
> + * -vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1---> <---vbs+2--->
> <---0- (scanline counter gen3+)
> + *       |          |                                         |
> + *       last visible pixel                                   first
> visible pixel
> + *                  |                                         increment
> frame counter (gen3/4)
> + *                  pixel counter = vblank_start * htotal     pixel
> counter = 0 (gen3/4)
> + *
> + * x  = horizontal active
> + * _  = horizontal blanking
> + * hs = horizontal sync
> + * va = vertical active
> + * vb = vertical blanking
> + * vs = vertical sync
> + * vbs = vblank_start (number)
> + *
> + * Summary:
> + * - most events happen at the start of horizontal sync
> + * - frame start happens at the start of horizontal blank, 1-4 lines
> + *   (depending on PIPECONF settings) after the start of vblank
> + * - gen3/4 pixel and frame counter are synchronized with the start
> + *   of horizontal active on the first line of vertical active
> + */
> +
>  static u32 i8xx_get_vblank_counter(struct drm_device *dev, int pipe)
>  {
>         /* Gen2 doesn't have a hardware frame counter */
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 5035 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 8/9] drm/i915: Draw a picture about video timings

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The docs are a bit lacking when it comes to describing when certain
timing related events occur in the hardware. Draw a picture which
tries to capture the most important ones.

v2: Clarify a few details (Imre)
v3: Add HSW+ HDMI scanline counter numbers

Acked-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Akash Goel <akash.goels@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 50 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bdb0b67..bb9b061 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -669,6 +669,56 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
 	}
 }
 
+/*
+ * This timing diagram depicts the video signal in and
+ * around the vertical blanking period.
+ *
+ * Assumptions about the fictitious mode used in this example:
+ *  vblank_start >= 3
+ *  vsync_start = vblank_start + 1
+ *  vsync_end = vblank_start + 2
+ *  vtotal = vblank_start + 3
+ *
+ *           start of vblank:
+ *           latch double buffered registers
+ *           increment frame counter (ctg+)
+ *           generate start of vblank interrupt (gen4+)
+ *           |
+ *           |          frame start:
+ *           |          generate frame start interrupt (aka. vblank interrupt) (gmch)
+ *           |          may be shifted forward 1-3 extra lines via PIPECONF
+ *           |          |
+ *           |          |  start of vsync:
+ *           |          |  generate vsync interrupt
+ *           |          |  |
+ * ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx
+ *       .   \hs/   .      \hs/          \hs/          \hs/   .      \hs/
+ * ----va---> <-----------------vb--------------------> <--------va-------------
+ *       |          |       <----vs----->                     |
+ * -vbs-----> <---vbs+1---> <---vbs+2---> <-----0-----> <-----1-----> <-----2--- (scanline counter gen2)
+ * -vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1---> <---vbs+2---> <-----0--- (scanline counter gen3+)
+ * -vbs-2---> <---vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1---> <---vbs+2- (scanline counter hsw+ hdmi)
+ *       |          |                                         |
+ *       last visible pixel                                   first visible pixel
+ *                  |                                         increment frame counter (gen3/4)
+ *                  pixel counter = vblank_start * htotal     pixel counter = 0 (gen3/4)
+ *
+ * x  = horizontal active
+ * _  = horizontal blanking
+ * hs = horizontal sync
+ * va = vertical active
+ * vb = vertical blanking
+ * vs = vertical sync
+ * vbs = vblank_start (number)
+ *
+ * Summary:
+ * - most events happen at the start of horizontal sync
+ * - frame start happens at the start of horizontal blank, 1-4 lines
+ *   (depending on PIPECONF settings) after the start of vblank
+ * - gen3/4 pixel and frame counter are synchronized with the start
+ *   of horizontal active on the first line of vertical active
+ */
+
 static u32 i8xx_get_vblank_counter(struct drm_device *dev, int pipe)
 {
 	/* Gen2 doesn't have a hardware frame counter */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 8/9] drm/i915: Draw a picture about video timings

[sourab gupta]

[-- Attachment #1.1: Type: text/plain, Size: 4040 bytes --]

On Thu, May 15, 2014 at 10:50 PM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The docs are a bit lacking when it comes to describing when certain
> timing related events occur in the hardware. Draw a picture which
> tries to capture the most important ones.
>
> v2: Clarify a few details (Imre)
> v3: Add HSW+ HDMI scanline counter numbers
>
> Acked-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Akash Goel <akash.goels@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 50
> +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index bdb0b67..bb9b061 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -669,6 +669,56 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
>         }
>  }
>
> +/*
> + * This timing diagram depicts the video signal in and
> + * around the vertical blanking period.
> + *
> + * Assumptions about the fictitious mode used in this example:
> + *  vblank_start >= 3
> + *  vsync_start = vblank_start + 1
> + *  vsync_end = vblank_start + 2
> + *  vtotal = vblank_start + 3
> + *
> + *           start of vblank:
> + *           latch double buffered registers
> + *           increment frame counter (ctg+)
> + *           generate start of vblank interrupt (gen4+)
> + *           |
> + *           |          frame start:
> + *           |          generate frame start interrupt (aka. vblank
> interrupt) (gmch)
> + *           |          may be shifted forward 1-3 extra lines via
> PIPECONF
> + *           |          |
> + *           |          |  start of vsync:
> + *           |          |  generate vsync interrupt
> + *           |          |  |
> + * ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___
>  ___xxxx
> + *       .   \hs/   .      \hs/          \hs/          \hs/   .      \hs/
> + * ----va---> <-----------------vb-------------------->
> <--------va-------------
> + *       |          |       <----vs----->                     |
> + * -vbs-----> <---vbs+1---> <---vbs+2---> <-----0-----> <-----1----->
> <-----2--- (scanline counter gen2)
> + * -vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1---> <---vbs+2--->
> <-----0--- (scanline counter gen3+)
> + * -vbs-2---> <---vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1--->
> <---vbs+2- (scanline counter hsw+ hdmi)
> + *       |          |                                         |
> + *       last visible pixel                                   first
> visible pixel
> + *                  |                                         increment
> frame counter (gen3/4)
> + *                  pixel counter = vblank_start * htotal     pixel
> counter = 0 (gen3/4)
> + *
> + * x  = horizontal active
> + * _  = horizontal blanking
> + * hs = horizontal sync
> + * va = vertical active
> + * vb = vertical blanking
> + * vs = vertical sync
> + * vbs = vblank_start (number)
> + *
> + * Summary:
> + * - most events happen at the start of horizontal sync
> + * - frame start happens at the start of horizontal blank, 1-4 lines
> + *   (depending on PIPECONF settings) after the start of vblank
> + * - gen3/4 pixel and frame counter are synchronized with the start
> + *   of horizontal active on the first line of vertical active
> + */
> +
>  static u32 i8xx_get_vblank_counter(struct drm_device *dev, int pipe)
>  {
>         /* Gen2 doesn't have a hardware frame counter */
> --
> 1.8.3.2
>
>
I'm not really sure how the scanline counter moves in the HSW and HDMI case.
Other than that, you can add my r-b tag:
Reviewed-by: "Sourab Gupta <sourabgupta@gmail.com>"

_______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 5622 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 9/9] drm/i915: Fix gen2 and hsw scanline counter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On gen2 the scanline counter behaves a bit differently from the
later generations. Instead of adding one to the raw scanline
counter value, we must subtract one.

On HSW the scanline counter apparently requires a +2 adjustment
on HDMI outputs. eDP on port A however wants the +1 adjustment.
So far no one has tested DP/eDP on port != A, and also BDW
behaviour is still a mystery.

As the fixup we must apply to the hardware scanline counter
depends on several factors, compute the desired offset at modeset
time and tuck it away for when it's needed.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 14 ++++-------
 drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 3 files changed, 54 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bc4585b..8e1b104 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -817,9 +817,9 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
 	enum pipe pipe = crtc->pipe;
-	int vtotal = mode->crtc_vtotal;
-	int position;
+	int position, vtotal;
 
+	vtotal = mode->crtc_vtotal;
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 		vtotal /= 2;
 
@@ -829,14 +829,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
 
 	/*
-	 * Scanline counter increments at leading edge of hsync, and
-	 * it starts counting from vtotal-1 on the first active line.
-	 * That means the scanline counter value is always one less
-	 * than what we would expect. Ie. just after start of vblank,
-	 * which also occurs at start of hsync (on the last active line),
-	 * the scanline counter will read vblank_start-1.
+	 * See update_scanline_offset() for the details on the
+	 * scanline_offset adjustment.
 	 */
-	return (position + 1) % vtotal;
+	return (position + crtc->scanline_offset) % vtotal;
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 993597b..93ef06b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9886,6 +9886,47 @@ void ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config
 	     pipe_config->adjusted_mode.crtc_clock, dotclock);
 }
 
+static void update_scanline_offset(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+
+	/*
+	 * The scanline counter increments at the leading edge of hsync.
+	 *
+	 * On most platforms it starts counting from vtotal-1 on the
+	 * first active line. That means the scanline counter value is
+	 * always one less than what we would expect. Ie. just after
+	 * start of vblank, which also occurs at start of hsync (on the
+	 * last active line), the scanline counter will read vblank_start-1.
+	 *
+	 * On gen2 the scanline counter starts counting from 1 instead
+	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
+	 * to keep the value positive), instead of adding one.
+	 *
+	 * On HSW the behaviour of the scanline counter depends either on
+	 * the output type, or the DDI port used (so far it's unclear which).
+	 * In certain cases we need to add 2 instead of 1 to the reported
+	 * scanline counter value.
+	 * FIXME: +2 seems to be correct for HDMI, but eDP on port A wants +1
+	 * instead. Not sure how DP on other ports behaves.
+	 * FIXME: How does BDW behave?
+	 */
+	if (IS_GEN2(dev)) {
+		const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+		int vtotal;
+
+		vtotal = mode->crtc_vtotal;
+		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+			vtotal /= 2;
+
+		crtc->scanline_offset = vtotal - 1;
+	} else if (HAS_DDI(dev) &&
+		   intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI)) {
+		crtc->scanline_offset = 2;
+	} else
+		crtc->scanline_offset = 1;
+}
+
 static int __intel_set_mode(struct drm_crtc *crtc,
 			    struct drm_display_mode *mode,
 			    int x, int y, struct drm_framebuffer *fb)
@@ -9984,8 +10025,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
-	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
+	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
+		update_scanline_offset(intel_crtc);
+
 		dev_priv->display.crtc_enable(&intel_crtc->base);
+	}
 
 	/* FIXME: add subpixel order */
 done:
@@ -11511,6 +11555,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 */
 		crtc->cpu_fifo_underrun_disabled = true;
 		crtc->pch_fifo_underrun_disabled = true;
+
+		update_scanline_offset(crtc);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d8b540b..5420368 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -403,6 +403,8 @@ struct intel_crtc {
 	} wm;
 
 	wait_queue_head_t vbl_wait;
+
+	int scanline_offset;
 };
 
 struct intel_plane_wm_parameters {
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 9/9] drm/i915: Fix gen2 and hsw+ scanline counter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On gen2 the scanline counter behaves a bit differently from the
later generations. Instead of adding one to the raw scanline
counter value, we must subtract one.

On HSW/BDW the scanline counter requires a +2 adjustment on HDMI
outputs. DP outputs on the on the other require the typical +1
adjustment.

As the fixup we must apply to the hardware scanline counter
depends on several factors, compute the desired offset at modeset
time and tuck it away for when it's needed.

v2: Clarify HSW+ situation

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 14 ++++-------
 drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 3 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bb9b061..80003b5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -818,9 +818,9 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
 	enum pipe pipe = crtc->pipe;
-	int vtotal = mode->crtc_vtotal;
-	int position;
+	int position, vtotal;
 
+	vtotal = mode->crtc_vtotal;
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 		vtotal /= 2;
 
@@ -830,14 +830,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
 
 	/*
-	 * Scanline counter increments at leading edge of hsync, and
-	 * it starts counting from vtotal-1 on the first active line.
-	 * That means the scanline counter value is always one less
-	 * than what we would expect. Ie. just after start of vblank,
-	 * which also occurs at start of hsync (on the last active line),
-	 * the scanline counter will read vblank_start-1.
+	 * See update_scanline_offset() for the details on the
+	 * scanline_offset adjustment.
 	 */
-	return (position + 1) % vtotal;
+	return (position + crtc->scanline_offset) % vtotal;
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0f8f9bc..f7222d7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10164,6 +10164,44 @@ void ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config
 	     pipe_config->adjusted_mode.crtc_clock, dotclock);
 }
 
+static void update_scanline_offset(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+
+	/*
+	 * The scanline counter increments at the leading edge of hsync.
+	 *
+	 * On most platforms it starts counting from vtotal-1 on the
+	 * first active line. That means the scanline counter value is
+	 * always one less than what we would expect. Ie. just after
+	 * start of vblank, which also occurs at start of hsync (on the
+	 * last active line), the scanline counter will read vblank_start-1.
+	 *
+	 * On gen2 the scanline counter starts counting from 1 instead
+	 * of vtotal-1, so we have to subtract one (or rather add vtotal-1
+	 * to keep the value positive), instead of adding one.
+	 *
+	 * On HSW+ the behaviour of the scanline counter depends on the output
+	 * type. For DP ports it behaves like most other platforms, but on HDMI
+	 * there's an extra 1 line difference. So we need to add two instead of
+	 * one to the value.
+	 */
+	if (IS_GEN2(dev)) {
+		const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+		int vtotal;
+
+		vtotal = mode->crtc_vtotal;
+		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+			vtotal /= 2;
+
+		crtc->scanline_offset = vtotal - 1;
+	} else if (HAS_DDI(dev) &&
+		   intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI)) {
+		crtc->scanline_offset = 2;
+	} else
+		crtc->scanline_offset = 1;
+}
+
 static int __intel_set_mode(struct drm_crtc *crtc,
 			    struct drm_display_mode *mode,
 			    int x, int y, struct drm_framebuffer *fb)
@@ -10262,8 +10300,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
-	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
+	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
+		update_scanline_offset(intel_crtc);
+
 		dev_priv->display.crtc_enable(&intel_crtc->base);
+	}
 
 	/* FIXME: add subpixel order */
 done:
@@ -11789,6 +11830,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 */
 		crtc->cpu_fifo_underrun_disabled = true;
 		crtc->pch_fifo_underrun_disabled = true;
+
+		update_scanline_offset(crtc);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 32a74e1..dd562b9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -403,6 +403,8 @@ struct intel_crtc {
 	} wm;
 
 	wait_queue_head_t vbl_wait;
+
+	int scanline_offset;
 };
 
 struct intel_plane_wm_parameters {
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 9/9] drm/i915: Fix gen2 and hsw+ scanline counter

[akash goel]

[-- Attachment #1.1: Type: text/plain, Size: 6625 bytes --]

Sorry not aware of this specific difference in the starting value of
scanline counter for HSW+ (& gen 2), but implementation wise, patch looks
fine.

Reviewed-by: "Akash Goel <akash.goels@gmail.com>"


On Thu, May 15, 2014 at 10:53 PM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On gen2 the scanline counter behaves a bit differently from the
> later generations. Instead of adding one to the raw scanline
> counter value, we must subtract one.
>
> On HSW/BDW the scanline counter requires a +2 adjustment on HDMI
> outputs. DP outputs on the on the other require the typical +1
> adjustment.
>
> As the fixup we must apply to the hardware scanline counter
> depends on several factors, compute the desired offset at modeset
> time and tuck it away for when it's needed.
>
> v2: Clarify HSW+ situation
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 14 ++++-------
>  drivers/gpu/drm/i915/intel_display.c | 45
> +++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  3 files changed, 51 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index bb9b061..80003b5 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -818,9 +818,9 @@ static int __intel_get_crtc_scanline(struct intel_crtc
> *crtc)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
>         enum pipe pipe = crtc->pipe;
> -       int vtotal = mode->crtc_vtotal;
> -       int position;
> +       int position, vtotal;
>
> +       vtotal = mode->crtc_vtotal;
>         if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>                 vtotal /= 2;
>
> @@ -830,14 +830,10 @@ static int __intel_get_crtc_scanline(struct
> intel_crtc *crtc)
>                 position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) &
> DSL_LINEMASK_GEN3;
>
>         /*
> -        * Scanline counter increments at leading edge of hsync, and
> -        * it starts counting from vtotal-1 on the first active line.
> -        * That means the scanline counter value is always one less
> -        * than what we would expect. Ie. just after start of vblank,
> -        * which also occurs at start of hsync (on the last active line),
> -        * the scanline counter will read vblank_start-1.
> +        * See update_scanline_offset() for the details on the
> +        * scanline_offset adjustment.
>          */
> -       return (position + 1) % vtotal;
> +       return (position + crtc->scanline_offset) % vtotal;
>  }
>
>  static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 0f8f9bc..f7222d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10164,6 +10164,44 @@ void ironlake_check_encoder_dotclock(const struct
> intel_crtc_config *pipe_config
>              pipe_config->adjusted_mode.crtc_clock, dotclock);
>  }
>
> +static void update_scanline_offset(struct intel_crtc *crtc)
> +{
> +       struct drm_device *dev = crtc->base.dev;
> +
> +       /*
> +        * The scanline counter increments at the leading edge of hsync.
> +        *
> +        * On most platforms it starts counting from vtotal-1 on the
> +        * first active line. That means the scanline counter value is
> +        * always one less than what we would expect. Ie. just after
> +        * start of vblank, which also occurs at start of hsync (on the
> +        * last active line), the scanline counter will read
> vblank_start-1.
> +        *
> +        * On gen2 the scanline counter starts counting from 1 instead
> +        * of vtotal-1, so we have to subtract one (or rather add vtotal-1
> +        * to keep the value positive), instead of adding one.
> +        *
> +        * On HSW+ the behaviour of the scanline counter depends on the
> output
> +        * type. For DP ports it behaves like most other platforms, but on
> HDMI
> +        * there's an extra 1 line difference. So we need to add two
> instead of
> +        * one to the value.
> +        */
> +       if (IS_GEN2(dev)) {
> +               const struct drm_display_mode *mode =
> &crtc->config.adjusted_mode;
> +               int vtotal;
> +
> +               vtotal = mode->crtc_vtotal;
> +               if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +                       vtotal /= 2;
> +
> +               crtc->scanline_offset = vtotal - 1;
> +       } else if (HAS_DDI(dev) &&
> +                  intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI)) {
> +               crtc->scanline_offset = 2;
> +       } else
> +               crtc->scanline_offset = 1;
> +}
> +
>  static int __intel_set_mode(struct drm_crtc *crtc,
>                             struct drm_display_mode *mode,
>                             int x, int y, struct drm_framebuffer *fb)
> @@ -10262,8 +10300,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>         }
>
>         /* Now enable the clocks, plane, pipe, and connectors that we set
> up. */
> -       for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
> +       for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
> +               update_scanline_offset(intel_crtc);
> +
>                 dev_priv->display.crtc_enable(&intel_crtc->base);
> +       }
>
>         /* FIXME: add subpixel order */
>  done:
> @@ -11789,6 +11830,8 @@ static void intel_sanitize_crtc(struct intel_crtc
> *crtc)
>                  */
>                 crtc->cpu_fifo_underrun_disabled = true;
>                 crtc->pch_fifo_underrun_disabled = true;
> +
> +               update_scanline_offset(crtc);
>         }
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 32a74e1..dd562b9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -403,6 +403,8 @@ struct intel_crtc {
>         } wm;
>
>         wait_queue_head_t vbl_wait;
> +
> +       int scanline_offset;
>  };
>
>  struct intel_plane_wm_parameters {
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 8156 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 9/9] drm/i915: Fix gen2 and hsw+ scanline counter

[sourab gupta]

[-- Attachment #1.1: Type: text/plain, Size: 7252 bytes --]

On Fri, May 16, 2014 at 11:03 AM, akash goel <akash.goels@gmail.com> wrote:

>
> Sorry not aware of this specific difference in the starting value of
> scanline counter for HSW+ (& gen 2), but implementation wise, patch looks
> fine.
>
> Reviewed-by: "Akash Goel <akash.goels@gmail.com>"
>
>
Don't have enough info about the initial scanline counter values for
HSW+ and gen2. Otherwise, you can add my r-b tag

Reviewed-by: "Sourab Gupta <sourabgupta@gmail.com>"


>
On Thu, May 15, 2014 at 10:53 PM, <ville.syrjala@linux.intel.com> wrote:
>
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> On gen2 the scanline counter behaves a bit differently from the
>> later generations. Instead of adding one to the raw scanline
>> counter value, we must subtract one.
>>
>> On HSW/BDW the scanline counter requires a +2 adjustment on HDMI
>> outputs. DP outputs on the on the other require the typical +1
>> adjustment.
>>
>> As the fixup we must apply to the hardware scanline counter
>> depends on several factors, compute the desired offset at modeset
>> time and tuck it away for when it's needed.
>>
>> v2: Clarify HSW+ situation
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c      | 14 ++++-------
>>  drivers/gpu/drm/i915/intel_display.c | 45
>> +++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>>  3 files changed, 51 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index bb9b061..80003b5 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -818,9 +818,9 @@ static int __intel_get_crtc_scanline(struct
>> intel_crtc *crtc)
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
>>         enum pipe pipe = crtc->pipe;
>> -       int vtotal = mode->crtc_vtotal;
>> -       int position;
>> +       int position, vtotal;
>>
>> +       vtotal = mode->crtc_vtotal;
>>         if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>>                 vtotal /= 2;
>>
>> @@ -830,14 +830,10 @@ static int __intel_get_crtc_scanline(struct
>> intel_crtc *crtc)
>>                 position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) &
>> DSL_LINEMASK_GEN3;
>>
>>         /*
>> -        * Scanline counter increments at leading edge of hsync, and
>> -        * it starts counting from vtotal-1 on the first active line.
>> -        * That means the scanline counter value is always one less
>> -        * than what we would expect. Ie. just after start of vblank,
>> -        * which also occurs at start of hsync (on the last active line),
>> -        * the scanline counter will read vblank_start-1.
>> +        * See update_scanline_offset() for the details on the
>> +        * scanline_offset adjustment.
>>          */
>> -       return (position + 1) % vtotal;
>> +       return (position + crtc->scanline_offset) % vtotal;
>>  }
>>
>>  static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 0f8f9bc..f7222d7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -10164,6 +10164,44 @@ void ironlake_check_encoder_dotclock(const
>> struct intel_crtc_config *pipe_config
>>              pipe_config->adjusted_mode.crtc_clock, dotclock);
>>  }
>>
>> +static void update_scanline_offset(struct intel_crtc *crtc)
>> +{
>> +       struct drm_device *dev = crtc->base.dev;
>> +
>> +       /*
>> +        * The scanline counter increments at the leading edge of hsync.
>> +        *
>> +        * On most platforms it starts counting from vtotal-1 on the
>> +        * first active line. That means the scanline counter value is
>> +        * always one less than what we would expect. Ie. just after
>> +        * start of vblank, which also occurs at start of hsync (on the
>> +        * last active line), the scanline counter will read
>> vblank_start-1.
>> +        *
>> +        * On gen2 the scanline counter starts counting from 1 instead
>> +        * of vtotal-1, so we have to subtract one (or rather add vtotal-1
>> +        * to keep the value positive), instead of adding one.
>> +        *
>> +        * On HSW+ the behaviour of the scanline counter depends on the
>> output
>> +        * type. For DP ports it behaves like most other platforms, but
>> on HDMI
>> +        * there's an extra 1 line difference. So we need to add two
>> instead of
>> +        * one to the value.
>> +        */
>> +       if (IS_GEN2(dev)) {
>> +               const struct drm_display_mode *mode =
>> &crtc->config.adjusted_mode;
>> +               int vtotal;
>> +
>> +               vtotal = mode->crtc_vtotal;
>> +               if (mode->flags & DRM_MODE_FLAG_INTERLACE)
>> +                       vtotal /= 2;
>> +
>> +               crtc->scanline_offset = vtotal - 1;
>> +       } else if (HAS_DDI(dev) &&
>> +                  intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI)) {
>> +               crtc->scanline_offset = 2;
>> +       } else
>> +               crtc->scanline_offset = 1;
>> +}
>> +
>>  static int __intel_set_mode(struct drm_crtc *crtc,
>>                             struct drm_display_mode *mode,
>>                             int x, int y, struct drm_framebuffer *fb)
>> @@ -10262,8 +10300,11 @@ static int __intel_set_mode(struct drm_crtc
>> *crtc,
>>         }
>>
>>         /* Now enable the clocks, plane, pipe, and connectors that we set
>> up. */
>> -       for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
>> +       for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) {
>> +               update_scanline_offset(intel_crtc);
>> +
>>                 dev_priv->display.crtc_enable(&intel_crtc->base);
>> +       }
>>
>>         /* FIXME: add subpixel order */
>>  done:
>> @@ -11789,6 +11830,8 @@ static void intel_sanitize_crtc(struct intel_crtc
>> *crtc)
>>                  */
>>                 crtc->cpu_fifo_underrun_disabled = true;
>>                 crtc->pch_fifo_underrun_disabled = true;
>> +
>> +               update_scanline_offset(crtc);
>>         }
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 32a74e1..dd562b9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -403,6 +403,8 @@ struct intel_crtc {
>>         } wm;
>>
>>         wait_queue_head_t vbl_wait;
>> +
>> +       int scanline_offset;
>>  };
>>
>>  struct intel_plane_wm_parameters {
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

[-- Attachment #1.2: Type: text/html, Size: 9699 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 9/9] drm/i915: Fix gen2 and hsw+ scanline counter

[Ville Syrjälä]

On Fri, May 16, 2014 at 11:54:09AM +0530, sourab gupta wrote:
> On Fri, May 16, 2014 at 11:03 AM, akash goel <akash.goels@gmail.com> wrote:
> 
> >
> > Sorry not aware of this specific difference in the starting value of
> > scanline counter for HSW+ (& gen 2), but implementation wise, patch looks
> > fine.
> >
> > Reviewed-by: "Akash Goel <akash.goels@gmail.com>"
> >
> >
> Don't have enough info about the initial scanline counter values for
> HSW+ and gen2. Otherwise, you can add my r-b tag
> 
> Reviewed-by: "Sourab Gupta <sourabgupta@gmail.com>"

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78997

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v4 0/9] drm/i915: Atomic sprites v4

[Ville Syrjälä]

On Tue, Apr 29, 2014 at 01:35:43PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Another posting of the atomic sprite series. Most things are reviewed, but there
> are a few patches in there for the scanline fixup that don't have r-bs.
> 
> The HSW+ situation is also still unclear. The scanline counter behaviour depends
> on either the actual DDI port used (A vs. rest) or the encoder type (HDMI vs. DP).
> If someone wants to help me find out which, grab [1] and run the resulting
> intel_poller tool to figure out at which scanline the vblank irq triggers and/or
> live surface address gets updated.

And naturally I forgot to specify [1]:
git://gitorious.org/vsyrjala/intel-gpu-tools.git intel_poller

> 
> Examples from running the tool on my IVB:
> 
> # intel_poller -t iir -p 0 -b 0
> ...
> dsl / pipe A / DEIIR[0] (pch):   1078 -   1079
> 
> This tells me that the pipe A vblank bit in DEIIR is set when the scanline
> counter increments from 1078 to 1079. The display mode was 1920x1080, so this
> tells us we need a +1 adjustment for the scaline counter.
> 
> # intel_poller -t surflive -p 0
> ...
> dsl / pipe A / Surflive:   1078 -   1079
> 
> which tells me that SURFLIVE updates at the same time, as was expected.
> 
> You can also run these tests:
> # ./intel_poller -t flip -p 0 -l 1078
> # ./intel_poller -t pan -p 0 -l 1078
> 
> These write the DSPSURF/OFFSET register when the scanline counter reaches the target
> value, and overwrite it again with the original value on the next scanline. If the
> target scanline is correctly set the picture remains shifted for the duration
> of the test, and if you specify any another scanline the picture should remain in
> its original position the whole time.
> 
> So if someone can run these on HSW and BDW systems with both DP and HDMI outputs
> on various ports we can try to figure out what the actual rule is.
> 
> Ville Syrjälä (9):
>   drm/i915: Fix scanout position for real
>   drm/i915: Add intel_get_crtc_scanline()
>   drm/i915: Make sprite updates atomic
>   drm/i915: Perform primary enable/disable atomically with sprite
>     updates
>   drm/i915: Add pipe update trace points
>   drm/i915: Add a small adjustment to the pixel counter on interlaced
>     modes
>   drm/i915: Improve gen3/4 frame counter
>   drm/i915: Draw a picture about video timings
>   drm/i915: Fix gen2 and hsw scanline counter
> 
>  drivers/gpu/drm/i915/i915_irq.c      | 231 +++++++++++++++++++++--------------
>  drivers/gpu/drm/i915/i915_trace.h    |  75 ++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  50 +++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |   5 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 231 +++++++++++++++++++++++++++++------
>  5 files changed, 463 insertions(+), 129 deletions(-)
> 
> -- 
> 1.8.3.2

-- 
Ville Syrjälä
Intel OTC