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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 14, 2014 at 11:46:35AM +0530, akash goel wrote:
>  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 ?

Everyone more or less assumes that the start of vblank is when stuff
happens, and that the start of vblank happens at the start of horizontal
active. So I think we should keep the in_vblank status and position
consistent with that notion and accept the small error we get as a
result.

> 
> 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.

In theory the scanline counter should be 0 on the first active line
(the counter is 0 based). But the hardware is weird and the scanline
counter will actually read either 1 or vtotal-1 on the first active line
(or even vtotal-2 in some cases, and so far no one seems to know why that
is). So the best I've been able to do figure out the rules via empirical
experiments.

> 
> 
> 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@linux.intel.com> wrote:
> 
> > 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.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

-- 
Ville Syrjälä
Intel OTC