Hi,
We have reviewed the patch & overall it seems to be fine.
Although this patch has really simplified the scan-out calculation, but we do have few doubts/opens.
There is a particular change :-
position = (position + htotal - hsync_start) % vtotal;
This adjustment could although lead
to more accurate estimation of 'Vblank' interval, but will compromise with the
accuracy of 'hpos'.
Should we be avoiding the latter & preserve the original value of ‘hpos’ returned by pixel counter ?
Should we be avoiding the latter & preserve the original value of ‘hpos’ returned by pixel counter ?
Also we couldn’t fully understand
the comments related to Scanline counter increment in the commit message &
under the ‘if (HAS_PCH_SPLIT(dev))‘ condition in the code.
What we understood is that the scanline counter will increment on a Hsync pulse & when the first active line is being scanned, the counter will return '0' . When the 2nd second active line is being scanned, counter will return '1'. Assuming if there are 10 active lines, so when the last active line is being scanned, the scan line counter will return 9, but on the Hsync pulse of the last active line (which will mark the start of vblank interval also),
counter will increment to 10. And the counter should wraparound to 0, when last line ('vtotal') has been scanned completely.
counter will increment to 10. And the counter should wraparound to 0, when last line ('vtotal') has been scanned completely.
Please could you let us know that if there are 10 active lines, the value of 'vbl_start' will be 10 or 11 ? Actually we are bit confused that whether zero based indexing is being used here or not. Ideally the 'hpos' & 'vpos' shall be calculated with zero based indexing.
Sorry but as of now, we mainly have understanding & visibility from GEN7 Hw onwards, so we could review the patches from >= GEN7 perspective only.
Best Regards
Akash
On Thu, Feb 13, 2014 at 9:12 PM, <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
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.
Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_irq.c | 89 +++++++++++------------------------------
1 file changed, 23 insertions(+), 66 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f68aee3..d547994 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -706,34 +706,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__))
-#define __raw_i915_read16(dev_priv__, reg__) readw((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;
-
- if (INTEL_INFO(dev)->gen < 7) {
- status = pipe == PIPE_A ?
- DE_PIPEA_VBLANK :
- DE_PIPEB_VBLANK;
- } else {
- switch (pipe) {
- default:
- case PIPE_A:
- status = DE_PIPEA_VBLANK_IVB;
- break;
- case PIPE_B:
- status = DE_PIPEB_VBLANK_IVB;
- break;
- case PIPE_C:
- status = DE_PIPEC_VBLANK_IVB;
- break;
- }
- }
-
- return __raw_i915_read32(dev_priv, DEISR) & status;
-}
static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
unsigned int flags, int *vpos, int *hpos,
@@ -744,7 +716,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;
@@ -756,6 +728,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;
@@ -774,7 +747,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. */
@@ -790,42 +763,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_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
@@ -837,6 +783,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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
