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@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
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
