<div class="MsoNormal" style="margin:0in 0in 0pt">Hi,</div><div class="MsoNormal" style="margin:0in 0in 0pt"> </div> <div class="MsoNormal" style="margin:0in 0in 0pt">We have reviewed the patch & overall it seems to be fine.</div><div class="MsoNormal" style="margin:0in 0in 0pt"> Although this patch has really simplified the scan-out calculation, but we do have few doubts/opens. </div> There is a particular change :- position = (position + htotal - hsync_start) % vtotal; <div class="MsoNormal" style="margin:0in 0in 0pt">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 ?</div><div class="MsoNormal" style="margin:0in 0in 0pt"> </div> <div class="MsoNormal" style="margin:0in 0in 0pt">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. </div> <div class="MsoNormal" style="margin:0in 0in 0pt">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. </div><div class="MsoNormal" style="margin:0in 0in 0pt"> </div> <div class="MsoNormal" style="margin:0in 0in 0pt">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. </div> <div class="MsoNormal" style="margin:0in 0in 0pt"> </div> Sorry but as of now, we mainly have understanding & visibility from GEN7 Hw onwards, so we could review the patches from >= GEN7 perspective only. <div> </div><div>Best Regards</div><div>Akash</div><div> </div><div> </div><div class="gmail_quote">On Thu, Feb 13, 2014 at 9:12 PM, <<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">From: Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a>> 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ä <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a>> --- 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 <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </blockquote></div>