[Intel-gfx] [PATCH 1/5] drm/i915: Fix scanout position for real
akash goel
akash.goels at gmail.com
Fri Mar 14 07:16:35 CET 2014
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 ?
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.
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 at linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala at 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.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140314/e2fe90bf/attachment.html>
More information about the Intel-gfx
mailing list