[PATCH v3 0/5] drm/i915: Atomic sprites v3

[PATCH v3 0/5] drm/i915: Atomic sprites v3

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

OK, so Daniel convinced me that the check+wait thing I used earlier
wasn't really safe. I then decided that the only to make it safe it
open coding the wait_for_event() mysefl, and after doing that I
realized that we don't even need the silly vbl_received thingy, and
just a wait queue + DSL check is enough. Let's see if Daniel can
tear this one to shreds too :)

Oh and I also did some more digging to the scanline counter + vblank
interrupt mess, and decided that all my earlier hacks were crap. The
solution was so simple once I started to look at the pixel counter
values on gen4. I got inspired to have another look at this mess
after Imre did some clock based vblank interrupt timing measurements
on his VLV. Art confirmed my findings, so it should all be good now,
no ugly hacks included.

There is one tiny race window on gen2. That's caused by the fact that
we only have the frame start interrupt there, and scanline counter
increments slightly after the interrupt is triggered. So if we would
extremely unlucky we might wake up, check the DSL, and determine we
need to sleep more. But at this time we don't use the atomic update
mechanism on gen2, and I think the windows is so small that we should
be able to ignore it. Also the 1ms timeout would anyway prevent us from
sleeping another full frame. So if it ever becomes a problem, we can
try to think of something to overcome it, but at this point it doesn't
seem worth the effort.

Ville Syrjälä (5):
  drm/i915: Fix scanout position for real
  drm/i915: Add intel_get_crtc_scanline()
  drm/i915: Make sprite updates atomic
  drm/i915: Perform primary enable/disable atomically with sprite
    updates
  drm/i915: Add pipe update trace points

 drivers/gpu/drm/i915/i915_irq.c      | 137 ++++++++++++------------
 drivers/gpu/drm/i915/i915_trace.h    |  77 ++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 drivers/gpu/drm/i915/intel_sprite.c  | 195 ++++++++++++++++++++++++++++-------
 5 files changed, 306 insertions(+), 108 deletions(-)

-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[ville.syrjala]

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

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

[akash goel]

[-- Attachment #1.1: Type: text/plain, Size: 11398 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 20369 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

[PATCH v3 2/5] drm/i915: Add intel_get_crtc_scanline()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a new function intel_get_crtc_scanline() that returns the current
scanline counter for the crtc.

v2: Rebase after vblank timestamp changes.
    Use intel_ prefix instead of i915_ as is more customary for
    display related functions.
    Include DRM_SCANOUTPOS_INVBL in the return value even w/o
    adjustments, for a bit of extra consistency.
v3: Change the implementation to be based on DSL on all gens,
    since that's enough for the needs of atomic updates, and
    it will avoid complicating the scanout position calculations
    for the vblank timestamps

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  | 51 +++++++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d547994..ffeda27 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -707,6 +707,29 @@ 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__))
 
+static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe = crtc->pipe;
+	int position;
+
+	if (IS_GEN2(dev))
+		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
+	else
+		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
+
+	/*
+	 * 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.
+	 */
+	return (position + 1) % crtc->config.adjusted_mode.crtc_vtotal;
+}
+
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 				    unsigned int flags, int *vpos, int *hpos,
 				    ktime_t *stime, ktime_t *etime)
@@ -758,20 +781,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		/* No obvious pixelcount register. Only query vertical
 		 * scanout position from Display scan line register.
 		 */
-		if (IS_GEN2(dev))
-			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
-		else
-			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
-
-		/*
-		 * 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;
+		position = __intel_get_crtc_scanline(intel_crtc);
 	} else {
 		/* Have access to pixelcount since start of frame.
 		 * We can split this into vertical and horizontal
@@ -832,6 +842,19 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	return ret;
 }
 
+int intel_get_crtc_scanline(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	unsigned long irqflags;
+	int position;
+
+	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	position = __intel_get_crtc_scanline(crtc);
+	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+
+	return position;
+}
+
 static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 			      int *max_error,
 			      struct timeval *vblank_time,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bff5d0a..a471d53 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -610,6 +610,7 @@ void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void hsw_pc8_disable_interrupts(struct drm_device *dev);
 void hsw_pc8_restore_interrupts(struct drm_device *dev);
+int intel_get_crtc_scanline(struct intel_crtc *crtc);
 
 
 /* intel_crt.c */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 2/5] drm/i915: Add intel_get_crtc_scanline()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a new function intel_get_crtc_scanline() that returns the current
scanline counter for the crtc.

v2: Rebase after vblank timestamp changes.
    Use intel_ prefix instead of i915_ as is more customary for
    display related functions.
    Include DRM_SCANOUTPOS_INVBL in the return value even w/o
    adjustments, for a bit of extra consistency.
v3: Change the implementation to be based on DSL on all gens,
    since that's enough for the needs of atomic updates, and
    it will avoid complicating the scanout position calculations
    for the vblank timestamps
v4: Don't break scanline wraparound for interlaced modes

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c  | 56 ++++++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index d547994..0fbd0a3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -707,6 +707,34 @@ 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__))
 
+static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	int vtotal = mode->crtc_vtotal;
+	int position;
+
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vtotal /= 2;
+
+	if (IS_GEN2(dev))
+		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
+	else
+		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
+
+	/*
+	 * 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.
+	 */
+	return (position + 1) % vtotal;
+}
+
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 				    unsigned int flags, int *vpos, int *hpos,
 				    ktime_t *stime, ktime_t *etime)
@@ -758,20 +786,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		/* No obvious pixelcount register. Only query vertical
 		 * scanout position from Display scan line register.
 		 */
-		if (IS_GEN2(dev))
-			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
-		else
-			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
-
-		/*
-		 * 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;
+		position = __intel_get_crtc_scanline(intel_crtc);
 	} else {
 		/* Have access to pixelcount since start of frame.
 		 * We can split this into vertical and horizontal
@@ -832,6 +847,19 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	return ret;
 }
 
+int intel_get_crtc_scanline(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	unsigned long irqflags;
+	int position;
+
+	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+	position = __intel_get_crtc_scanline(crtc);
+	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+
+	return position;
+}
+
 static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 			      int *max_error,
 			      struct timeval *vblank_time,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bff5d0a..a471d53 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -610,6 +610,7 @@ void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
 void hsw_pc8_disable_interrupts(struct drm_device *dev);
 void hsw_pc8_restore_interrupts(struct drm_device *dev);
+int intel_get_crtc_scanline(struct intel_crtc *crtc);
 
 
 /* intel_crt.c */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 3/5] drm/i915: Make sprite updates atomic

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the
vblank interrupt handler, which is a bit dangerous since we set up
interrupts before the crtcs. However in this case since it's the vblank
interrupt, we don't actually unmask it until some piece of code
requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      | 27 ++++++++--
 drivers/gpu/drm/i915/intel_display.c |  2 +
 drivers/gpu/drm/i915/intel_drv.h     |  2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 95 ++++++++++++++++++++++++++++++++++++
 4 files changed, 122 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ffeda27..578747a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1530,6 +1530,13 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+
+	wake_up(&crtc->vbl_wait);
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1580,8 +1587,10 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 	spin_unlock(&dev_priv->irq_lock);
 
 	for_each_pipe(pipe) {
-		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
+		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
 			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
+		}
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1807,8 +1816,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 		DRM_ERROR("Poison interrupt\n");
 
 	for_each_pipe(pipe) {
-		if (de_iir & DE_PIPE_VBLANK(pipe))
+		if (de_iir & DE_PIPE_VBLANK(pipe)) {
 			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
+		}
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1857,8 +1868,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 		intel_opregion_asle_intr(dev);
 
 	for_each_pipe(i) {
-		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
+		if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
 			drm_handle_vblank(dev, i);
+			intel_pipe_handle_vblank(dev, i);
+		}
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
@@ -2000,8 +2013,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 			continue;
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
-		if (pipe_iir & GEN8_PIPE_VBLANK)
+		if (pipe_iir & GEN8_PIPE_VBLANK) {
 			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
+		}
 
 		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3274,6 +3289,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	if (!drm_handle_vblank(dev, pipe))
 		return false;
 
+	intel_pipe_handle_vblank(dev, pipe);
+
 	if ((iir & flip_pending) == 0)
 		return false;
 
@@ -3457,6 +3474,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	if (!drm_handle_vblank(dev, pipe))
 		return false;
 
+	intel_pipe_handle_vblank(dev, pipe);
+
 	if ((iir & flip_pending) == 0)
 		return false;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac05da4..d35952b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10298,6 +10298,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a471d53..1e6dae4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -376,6 +376,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..f455a2e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,71 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static void intel_pipe_update_start(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	/* FIXME needs to be calibrated sensibly */
+	unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
+	unsigned int max = mode->crtc_vblank_start - 1;
+	long timeout = msecs_to_jiffies_timeout(1);
+	unsigned int scanline;
+	DEFINE_WAIT(wait);
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+		preempt_check_resched();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+}
+
+static void intel_pipe_update_end(struct intel_crtc *crtc)
+{
+	local_irq_enable();
+	preempt_check_resched();
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,6 +113,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
@@ -131,6 +197,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +212,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_pipe_update_end(intel_crtc);
 }
 
 static void
@@ -152,15 +222,20 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
 	/* Activate double buffered register update */
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	intel_pipe_update_end(intel_crtc);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -226,6 +301,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
@@ -299,6 +375,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +396,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	intel_pipe_update_end(intel_crtc);
 }
 
 static void
@@ -326,8 +406,11 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
 	if (intel_plane->can_scale)
@@ -336,6 +419,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	intel_pipe_update_end(intel_crtc);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,6 +495,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
@@ -478,6 +564,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +580,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	intel_pipe_update_end(intel_crtc);
 }
 
 static void
@@ -500,8 +590,11 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 
+	intel_pipe_update_start(intel_crtc);
+
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
 	I915_WRITE(DVSSCALE(pipe), 0);
@@ -509,6 +602,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	intel_pipe_update_end(intel_crtc);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 3/5] drm/i915: Make sprite updates atomic

[Chris Wilson]

On Thu, Feb 13, 2014 at 05:42:52PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.
> 
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.
> 
> Note that we now go digging through pipe_to_crtc_mapping[] in the
> vblank interrupt handler, which is a bit dangerous since we set up
> interrupts before the crtcs. However in this case since it's the vblank
> interrupt, we don't actually unmask it until some piece of code
> requests it.
> 
> v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
>     Hook up the vblank irq stuff on BDW as well
> v3: Pass intel_crtc instead of drm_crtc (Daniel)
>     Warn if crtc.mutex isn't locked (Daniel)
>     Add an explicit compiler barrier and document the barriers (Daniel)
>     Note the irq vs. modeset setup madness in the commit message (Daniel)
> v4: Use prepare_to_wait() & co. directly and eliminate vbl_received

intel_pipe_update_start / intel_pipe_update_end are unbalanced (end()
does too much in the cases where start() failed.)

intel_pipe_update_start should check for min <= 0 (i.e.
usecs_to_scanline() returns a value greater than vblank_start).

intel_pipe_update_end() could do a sanity check that it is in the same
frame as start() and so give us warning when the code is broken.

Looks like drm_handle_vblank() could be moved to
intel_pipe_handle_vblank() for a small refactoring win.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v4 3/5] drm/i915: Make sprite updates atomic

[Ville Syrjälä]

On Thu, Feb 13, 2014 at 04:01:52PM +0000, Chris Wilson wrote:
> On Thu, Feb 13, 2014 at 05:42:52PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a mechanism by which we can evade the leading edge of vblank. This
> > guarantees that no two sprite register writes will straddle on either
> > side of the vblank start, and that means all the writes will be latched
> > together in one atomic operation.
> > 
> > We do the vblank evade by checking the scanline counter, and if it's too
> > close to the start of vblank (too close has been hardcoded to 100usec
> > for now), we will wait for the vblank start to pass. In order to
> > eliminate random delayes from the rest of the system, we operate with
> > interrupts disabled, except when waiting for the vblank obviously.
> > 
> > Note that we now go digging through pipe_to_crtc_mapping[] in the
> > vblank interrupt handler, which is a bit dangerous since we set up
> > interrupts before the crtcs. However in this case since it's the vblank
> > interrupt, we don't actually unmask it until some piece of code
> > requests it.
> > 
> > v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
> >     Hook up the vblank irq stuff on BDW as well
> > v3: Pass intel_crtc instead of drm_crtc (Daniel)
> >     Warn if crtc.mutex isn't locked (Daniel)
> >     Add an explicit compiler barrier and document the barriers (Daniel)
> >     Note the irq vs. modeset setup madness in the commit message (Daniel)
> > v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
> 
> intel_pipe_update_start / intel_pipe_update_end are unbalanced (end()
> does too much in the cases where start() failed.)

Ah the drm_vblank_get() fail. Should never happen. But I guess I could
make intel_pipe_update_start() return something to tell the caller
whether it needs to call intel_pipe_update_end().

> 
> intel_pipe_update_start should check for min <= 0 (i.e.
> usecs_to_scanline() returns a value greater than vblank_start).

Mode w/ vertical active portion of < 100 usec. Sounds rather unlikely,
but I suppose I could add a check for that too while I'm at it.

> 
> intel_pipe_update_end() could do a sanity check that it is in the same
> frame as start() and so give us warning when the code is broken.

I think I had something like that a long time, but it vanished along the
way. I'll add it. Getting a clear indication that things went south is
better than just some silent glitch on the screen. On gen2 we don't have
a frame counter but since it always returns 0, I don't even need to
add a special case. We just can't detect the failure on gen2.

> 
> Looks like drm_handle_vblank() could be moved to
> intel_pipe_handle_vblank() for a small refactoring win.

Hmm, yeah. I just need to make intel_pipe_handle_vblank() propagate
the return value from drm_handle_vblank(). Seems cleaner, consider
it done.

-- 
Ville Syrjälä
Intel OTC

[PATCH v5 3/5] drm/i915: Make sprite updates atomic

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the
vblank interrupt handler, which is a bit dangerous since we set up
interrupts before the crtcs. However in this case since it's the vblank
interrupt, we don't actually unmask it until some piece of code
requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
    Check for min/max scanline <= 0 (Chris)
    Don't call intel_pipe_update_end() if start failed totally (Chris)
    Check that the vblank counters match on both sides of the critical
    section (Chris)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 128 +++++++++++++++++++++++++++++++++++
 4 files changed, 151 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ffeda27..ca18c26 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1530,6 +1530,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *crtc;
+
+	if (!drm_handle_vblank(dev, pipe))
+		return false;
+
+	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	wake_up(&crtc->vbl_wait);
+
+	return true;
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1581,7 +1594,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1808,7 +1821,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & DE_PIPE_VBLANK(pipe))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1858,7 +1871,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(i) {
 		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
-			drm_handle_vblank(dev, i);
+			intel_pipe_handle_vblank(dev, i);
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
@@ -2001,7 +2014,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
 		if (pipe_iir & GEN8_PIPE_VBLANK)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3271,7 +3284,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
@@ -3454,7 +3467,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac05da4..d35952b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10298,6 +10298,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a471d53..1e6dae4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -376,6 +376,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..cf2a1ad 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,86 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	/* FIXME needs to be calibrated sensibly */
+	int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
+	int max = mode->crtc_vblank_start - 1;
+	long timeout = msecs_to_jiffies_timeout(1);
+	int scanline;
+	DEFINE_WAIT(wait);
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	if (min <= 0 || max <= 0)
+		return false;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return false;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+		preempt_check_resched();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+
+	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	return true;
+}
+
+static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	local_irq_enable();
+	preempt_check_resched();
+
+	if (start_vbl_count != end_vbl_count)
+		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
+			  pipe_name(pipe), start_vbl_count, end_vbl_count);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,11 +128,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -131,6 +214,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +229,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -152,8 +240,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
@@ -161,6 +254,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -226,10 +322,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -299,6 +398,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +419,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -326,7 +430,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
@@ -336,6 +445,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,10 +522,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -478,6 +593,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +609,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -500,7 +620,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
@@ -509,6 +634,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v6 3/5] drm/i915: Make sprite updates atomic

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the
vblank interrupt handler, which is a bit dangerous since we set up
interrupts before the crtcs. However in this case since it's the vblank
interrupt, we don't actually unmask it until some piece of code
requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
    Check for min/max scanline <= 0 (Chris)
    Don't call intel_pipe_update_end() if start failed totally (Chris)
    Check that the vblank counters match on both sides of the critical
    section (Chris)
v6: Fix atomic update for interlaced modes

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 133 +++++++++++++++++++++++++++++++++++
 4 files changed, 156 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0fbd0a3..1b73cf2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1535,6 +1535,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *crtc;
+
+	if (!drm_handle_vblank(dev, pipe))
+		return false;
+
+	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	wake_up(&crtc->vbl_wait);
+
+	return true;
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1586,7 +1599,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1813,7 +1826,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & DE_PIPE_VBLANK(pipe))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1863,7 +1876,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(i) {
 		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
-			drm_handle_vblank(dev, i);
+			intel_pipe_handle_vblank(dev, i);
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
@@ -2006,7 +2019,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
 		if (pipe_iir & GEN8_PIPE_VBLANK)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3276,7 +3289,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
@@ -3459,7 +3472,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac05da4..d35952b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10298,6 +10298,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a471d53..1e6dae4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -376,6 +376,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..0f0a91a 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,91 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	int vblank_start = mode->crtc_vblank_start;
+	enum pipe pipe = crtc->pipe;
+	long timeout = msecs_to_jiffies_timeout(1);
+	int scanline, min, max;
+	DEFINE_WAIT(wait);
+
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vblank_start = DIV_ROUND_UP(vblank_start, 2);
+
+	/* FIXME needs to be calibrated sensibly */
+	min = vblank_start - usecs_to_scanlines(mode, 100);
+	max = vblank_start - 1;
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	if (min <= 0 || max <= 0)
+		return false;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return false;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+		preempt_check_resched();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+
+	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	return true;
+}
+
+static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	local_irq_enable();
+	preempt_check_resched();
+
+	if (start_vbl_count != end_vbl_count)
+		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
+			  pipe_name(pipe), start_vbl_count, end_vbl_count);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,11 +133,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -131,6 +219,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +234,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -152,8 +245,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
@@ -161,6 +259,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -226,10 +327,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -299,6 +403,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +424,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -326,7 +435,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
@@ -336,6 +450,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,10 +527,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -478,6 +598,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +614,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -500,7 +625,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
@@ -509,6 +639,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v7 3/5] drm/i915: Make sprite updates atomic

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the
vblank interrupt handler, which is a bit dangerous since we set up
interrupts before the crtcs. However in this case since it's the vblank
interrupt, we don't actually unmask it until some piece of code
requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
    Check for min/max scanline <= 0 (Chris)
    Don't call intel_pipe_update_end() if start failed totally (Chris)
    Check that the vblank counters match on both sides of the critical
    section (Chris)
v6: Fix atomic update for interlaced modes
v7: Reorder code for better readability (Chris)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 133 +++++++++++++++++++++++++++++++++++
 4 files changed, 156 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0fbd0a3..1b73cf2 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1535,6 +1535,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *crtc;
+
+	if (!drm_handle_vblank(dev, pipe))
+		return false;
+
+	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	wake_up(&crtc->vbl_wait);
+
+	return true;
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1586,7 +1599,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1813,7 +1826,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & DE_PIPE_VBLANK(pipe))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1863,7 +1876,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(i) {
 		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
-			drm_handle_vblank(dev, i);
+			intel_pipe_handle_vblank(dev, i);
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
@@ -2006,7 +2019,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
 		if (pipe_iir & GEN8_PIPE_VBLANK)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3276,7 +3289,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
@@ -3459,7 +3472,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac05da4..d35952b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10298,6 +10298,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a471d53..1e6dae4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -376,6 +376,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..94c4e5e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,91 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	long timeout = msecs_to_jiffies_timeout(1);
+	int scanline, min, max, vblank_start;
+	DEFINE_WAIT(wait);
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	vblank_start = mode->crtc_vblank_start;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vblank_start = DIV_ROUND_UP(vblank_start, 2);
+
+	/* FIXME needs to be calibrated sensibly */
+	min = vblank_start - usecs_to_scanlines(mode, 100);
+	max = vblank_start - 1;
+
+	if (min <= 0 || max <= 0)
+		return false;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return false;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+		preempt_check_resched();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+
+	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	return true;
+}
+
+static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	local_irq_enable();
+	preempt_check_resched();
+
+	if (start_vbl_count != end_vbl_count)
+		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
+			  pipe_name(pipe), start_vbl_count, end_vbl_count);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,11 +133,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -131,6 +219,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +234,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -152,8 +245,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
@@ -161,6 +259,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -226,10 +327,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -299,6 +403,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +424,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -326,7 +435,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
@@ -336,6 +450,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,10 +527,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -478,6 +598,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +614,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -500,7 +625,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
@@ -509,6 +639,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v8 3/5] drm/i915: Make sprite updates atomic

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a mechanism by which we can evade the leading edge of vblank. This
guarantees that no two sprite register writes will straddle on either
side of the vblank start, and that means all the writes will be latched
together in one atomic operation.

We do the vblank evade by checking the scanline counter, and if it's too
close to the start of vblank (too close has been hardcoded to 100usec
for now), we will wait for the vblank start to pass. In order to
eliminate random delayes from the rest of the system, we operate with
interrupts disabled, except when waiting for the vblank obviously.

Note that we now go digging through pipe_to_crtc_mapping[] in the
vblank interrupt handler, which is a bit dangerous since we set up
interrupts before the crtcs. However in this case since it's the vblank
interrupt, we don't actually unmask it until some piece of code
requests it.

v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
    Hook up the vblank irq stuff on BDW as well
v3: Pass intel_crtc instead of drm_crtc (Daniel)
    Warn if crtc.mutex isn't locked (Daniel)
    Add an explicit compiler barrier and document the barriers (Daniel)
    Note the irq vs. modeset setup madness in the commit message (Daniel)
v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
    Check for min/max scanline <= 0 (Chris)
    Don't call intel_pipe_update_end() if start failed totally (Chris)
    Check that the vblank counters match on both sides of the critical
    section (Chris)
v6: Fix atomic update for interlaced modes
v7: Reorder code for better readability (Chris)
v8: Drop preempt_check_resched(). It's not available to modules
    anymore and isn't even needed unless we ourselves cause
    a wakeup needing reschedule while interrupts are off

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
 drivers/gpu/drm/i915/intel_display.c |   2 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_sprite.c  | 131 +++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6ec3f8a..d35120e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1536,6 +1536,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
 	}
 }
 
+static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
+{
+	struct intel_crtc *crtc;
+
+	if (!drm_handle_vblank(dev, pipe))
+		return false;
+
+	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+	wake_up(&crtc->vbl_wait);
+
+	return true;
+}
+
 static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1587,7 +1600,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
 
 	for_each_pipe(pipe) {
 		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
 			intel_prepare_page_flip(dev, pipe);
@@ -1814,7 +1827,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & DE_PIPE_VBLANK(pipe))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
 			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
@@ -1864,7 +1877,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
 
 	for_each_pipe(pipe) {
 		if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		/* plane/pipes map 1:1 on ilk+ */
 		if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
@@ -2007,7 +2020,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
 
 		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
 		if (pipe_iir & GEN8_PIPE_VBLANK)
-			drm_handle_vblank(dev, pipe);
+			intel_pipe_handle_vblank(dev, pipe);
 
 		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
 			intel_prepare_page_flip(dev, pipe);
@@ -3284,7 +3297,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
@@ -3469,7 +3482,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
 
-	if (!drm_handle_vblank(dev, pipe))
+	if (!intel_pipe_handle_vblank(dev, pipe))
 		return false;
 
 	if ((iir & flip_pending) == 0)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e8bfd8..3fc739c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10299,6 +10299,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	init_waitqueue_head(&intel_crtc->vbl_wait);
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9790477..8c9892d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -384,6 +384,8 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	wait_queue_head_t vbl_wait;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..a3ed840 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,89 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
+{
+	/* paranoia */
+	if (!mode->crtc_htotal)
+		return 1;
+
+	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
+}
+
+static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
+	enum pipe pipe = crtc->pipe;
+	long timeout = msecs_to_jiffies_timeout(1);
+	int scanline, min, max, vblank_start;
+	DEFINE_WAIT(wait);
+
+	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
+
+	vblank_start = mode->crtc_vblank_start;
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+		vblank_start = DIV_ROUND_UP(vblank_start, 2);
+
+	/* FIXME needs to be calibrated sensibly */
+	min = vblank_start - usecs_to_scanlines(mode, 100);
+	max = vblank_start - 1;
+
+	if (min <= 0 || max <= 0)
+		return false;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe)))
+		return false;
+
+	local_irq_disable();
+
+	for (;;) {
+		/*
+		 * prepare_to_wait() has a memory barrier, which guarantees
+		 * other CPUs can see the task state update by the time we
+		 * read the scanline.
+		 */
+		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
+
+		scanline = intel_get_crtc_scanline(crtc);
+		if (scanline < min || scanline > max)
+			break;
+
+		if (timeout <= 0) {
+			DRM_ERROR("Potential atomic update failure on pipe %c\n",
+				  pipe_name(crtc->pipe));
+			break;
+		}
+
+		local_irq_enable();
+
+		timeout = schedule_timeout(timeout);
+
+		local_irq_disable();
+	}
+
+	finish_wait(&crtc->vbl_wait, &wait);
+
+	drm_vblank_put(dev, pipe);
+
+	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	return true;
+}
+
+static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
+{
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
+
+	local_irq_enable();
+
+	if (start_vbl_count != end_vbl_count)
+		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
+			  pipe_name(pipe), start_vbl_count, end_vbl_count);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -48,11 +131,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
 	u32 sprctl;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
@@ -131,6 +217,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 							fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -144,6 +232,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -152,8 +243,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
@@ -161,6 +257,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	I915_WRITE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
@@ -226,10 +325,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	u32 sprctl, sprscale = 0;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	sprctl = I915_READ(SPRCTL(pipe));
 
@@ -299,6 +401,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -318,6 +422,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -326,7 +433,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
@@ -336,6 +448,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(SPRSURF(pipe), 0);
 	POSTING_READ(SPRSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
@@ -410,10 +525,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
 	unsigned long dvssurf_offset, linear_offset;
 	u32 dvscntr, dvsscale;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
+	u32 start_vbl_count;
+	bool atomic_update;
 
 	dvscntr = I915_READ(DVSCNTR(pipe));
 
@@ -478,6 +596,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;
 
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -492,6 +612,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
 	POSTING_READ(DVSSURF(pipe));
+
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void
@@ -500,7 +623,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_plane->pipe;
+	u32 start_vbl_count;
+	bool atomic_update;
+
+	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
@@ -509,6 +637,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	I915_WRITE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
 
+	if (atomic_update)
+		intel_pipe_update_end(intel_crtc, start_vbl_count);
+
 	/*
 	 * Avoid underruns when disabling the sprite.
 	 * FIXME remove once watermark updates are done properly.
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v8 3/5] drm/i915: Make sprite updates atomic

[Jesse Barnes]

On Fri,  7 Mar 2014 15:42:43 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.
> 
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.
> 
> Note that we now go digging through pipe_to_crtc_mapping[] in the
> vblank interrupt handler, which is a bit dangerous since we set up
> interrupts before the crtcs. However in this case since it's the vblank
> interrupt, we don't actually unmask it until some piece of code
> requests it.
> 
> v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
>     Hook up the vblank irq stuff on BDW as well
> v3: Pass intel_crtc instead of drm_crtc (Daniel)
>     Warn if crtc.mutex isn't locked (Daniel)
>     Add an explicit compiler barrier and document the barriers (Daniel)
>     Note the irq vs. modeset setup madness in the commit message (Daniel)
> v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
> v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
>     Check for min/max scanline <= 0 (Chris)
>     Don't call intel_pipe_update_end() if start failed totally (Chris)
>     Check that the vblank counters match on both sides of the critical
>     section (Chris)
> v6: Fix atomic update for interlaced modes
> v7: Reorder code for better readability (Chris)
> v8: Drop preempt_check_resched(). It's not available to modules
>     anymore and isn't even needed unless we ourselves cause
>     a wakeup needing reschedule while interrupts are off
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
>  drivers/gpu/drm/i915/intel_display.c |   2 +
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 131 +++++++++++++++++++++++++++++++++++
>  4 files changed, 154 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6ec3f8a..d35120e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1536,6 +1536,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	}
>  }
>  
> +static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> +{
> +	struct intel_crtc *crtc;
> +
> +	if (!drm_handle_vblank(dev, pipe))
> +		return false;
> +
> +	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +	wake_up(&crtc->vbl_wait);
> +
> +	return true;
> +}
> +
>  static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1587,7 +1600,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  
>  	for_each_pipe(pipe) {
>  		if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> -			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
>  
>  		if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
>  			intel_prepare_page_flip(dev, pipe);
> @@ -1814,7 +1827,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  
>  	for_each_pipe(pipe) {
>  		if (de_iir & DE_PIPE_VBLANK(pipe))
> -			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
>  
>  		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
>  			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> @@ -1864,7 +1877,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  
>  	for_each_pipe(pipe) {
>  		if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
> -			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
>  
>  		/* plane/pipes map 1:1 on ilk+ */
>  		if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
> @@ -2007,7 +2020,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  
>  		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
>  		if (pipe_iir & GEN8_PIPE_VBLANK)
> -			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
>  
>  		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
>  			intel_prepare_page_flip(dev, pipe);
> @@ -3284,7 +3297,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
>  
> -	if (!drm_handle_vblank(dev, pipe))
> +	if (!intel_pipe_handle_vblank(dev, pipe))
>  		return false;
>  
>  	if ((iir & flip_pending) == 0)
> @@ -3469,7 +3482,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
>  
> -	if (!drm_handle_vblank(dev, pipe))
> +	if (!intel_pipe_handle_vblank(dev, pipe))
>  		return false;
>  
>  	if ((iir & flip_pending) == 0)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7e8bfd8..3fc739c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10299,6 +10299,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  		intel_crtc->plane = !pipe;
>  	}
>  
> +	init_waitqueue_head(&intel_crtc->vbl_wait);
> +
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9790477..8c9892d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -384,6 +384,8 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		struct intel_pipe_wm active;
>  	} wm;
> +
> +	wait_queue_head_t vbl_wait;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 336ae6c..a3ed840 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -37,6 +37,89 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
> +{
> +	/* paranoia */
> +	if (!mode->crtc_htotal)
> +		return 1;
> +
> +	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> +}
> +
> +static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> +	enum pipe pipe = crtc->pipe;
> +	long timeout = msecs_to_jiffies_timeout(1);
> +	int scanline, min, max, vblank_start;
> +	DEFINE_WAIT(wait);
> +
> +	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
> +
> +	vblank_start = mode->crtc_vblank_start;
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		vblank_start = DIV_ROUND_UP(vblank_start, 2);
> +
> +	/* FIXME needs to be calibrated sensibly */
> +	min = vblank_start - usecs_to_scanlines(mode, 100);
> +	max = vblank_start - 1;
> +
> +	if (min <= 0 || max <= 0)
> +		return false;
> +
> +	if (WARN_ON(drm_vblank_get(dev, pipe)))
> +		return false;
> +
> +	local_irq_disable();
> +
> +	for (;;) {
> +		/*
> +		 * prepare_to_wait() has a memory barrier, which guarantees
> +		 * other CPUs can see the task state update by the time we
> +		 * read the scanline.
> +		 */
> +		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
> +
> +		scanline = intel_get_crtc_scanline(crtc);
> +		if (scanline < min || scanline > max)
> +			break;
> +
> +		if (timeout <= 0) {
> +			DRM_ERROR("Potential atomic update failure on pipe %c\n",
> +				  pipe_name(crtc->pipe));
> +			break;
> +		}
> +
> +		local_irq_enable();
> +
> +		timeout = schedule_timeout(timeout);
> +
> +		local_irq_disable();
> +	}
> +
> +	finish_wait(&crtc->vbl_wait, &wait);
> +
> +	drm_vblank_put(dev, pipe);
> +
> +	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
> +
> +	return true;
> +}
> +
> +static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	enum pipe pipe = crtc->pipe;
> +	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
> +
> +	local_irq_enable();
> +
> +	if (start_vbl_count != end_vbl_count)
> +		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
> +			  pipe_name(pipe), start_vbl_count, end_vbl_count);
> +}
> +
>  static void
>  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  		 struct drm_framebuffer *fb,
> @@ -48,11 +131,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
>  	u32 sprctl;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	u32 start_vbl_count;
> +	bool atomic_update;
>  
>  	sprctl = I915_READ(SPCNTR(pipe, plane));
>  
> @@ -131,6 +217,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  							fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> +
>  	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
>  	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>  
> @@ -144,6 +232,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
>  		   sprsurf_offset);
>  	POSTING_READ(SPSURF(pipe, plane));
> +
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
>  }
>  
>  static void
> @@ -152,8 +243,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
> +	u32 start_vbl_count;
> +	bool atomic_update;
> +
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>  
>  	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
>  		   ~SP_ENABLE);
> @@ -161,6 +257,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	I915_WRITE(SPSURF(pipe, plane), 0);
>  	POSTING_READ(SPSURF(pipe, plane));
>  
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
> +
>  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
>  }
>  
> @@ -226,10 +325,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	u32 sprctl, sprscale = 0;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	u32 start_vbl_count;
> +	bool atomic_update;
>  
>  	sprctl = I915_READ(SPRCTL(pipe));
>  
> @@ -299,6 +401,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> +
>  	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -318,6 +422,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_WRITE(SPRSURF(pipe),
>  		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>  	POSTING_READ(SPRSURF(pipe));
> +
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
>  }
>  
>  static void
> @@ -326,7 +433,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
> +	u32 start_vbl_count;
> +	bool atomic_update;
> +
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>  
>  	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
>  	/* Can't leave the scaler enabled... */
> @@ -336,6 +448,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	I915_WRITE(SPRSURF(pipe), 0);
>  	POSTING_READ(SPRSURF(pipe));
>  
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
> +
>  	/*
>  	 * Avoid underruns when disabling the sprite.
>  	 * FIXME remove once watermark updates are done properly.
> @@ -410,10 +525,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	unsigned long dvssurf_offset, linear_offset;
>  	u32 dvscntr, dvsscale;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	u32 start_vbl_count;
> +	bool atomic_update;
>  
>  	dvscntr = I915_READ(DVSCNTR(pipe));
>  
> @@ -478,6 +596,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= dvssurf_offset;
>  
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> +
>  	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -492,6 +612,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_WRITE(DVSSURF(pipe),
>  		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
>  	POSTING_READ(DVSSURF(pipe));
> +
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
>  }
>  
>  static void
> @@ -500,7 +623,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
> +	u32 start_vbl_count;
> +	bool atomic_update;
> +
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>  
>  	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
>  	/* Disable the scaler */
> @@ -509,6 +637,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	I915_WRITE(DVSSURF(pipe), 0);
>  	POSTING_READ(DVSSURF(pipe));
>  
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
> +
>  	/*
>  	 * Avoid underruns when disabling the sprite.
>  	 * FIXME remove once watermark updates are done properly.

Yeah looks like this will work ok.

I don't understand the prepare_to_wait() comment, since we're both
holding the crtc mutex and prepare_to_wait() will take the crtc
vbl_wait queue lock, but since things look safe as-is I guess it's not
a big deal.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/5] drm/i915: Perform primary enable/disable atomically with sprite updates

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move the primary plane enable/disable to occur atomically with the
sprite update that caused the primary plane visibility to change.

FBC and IPS enable/disable is left to happen well before or after
the primary plane change.

v2: Pass intel_crtc instead of drm_crtc (Daniel)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 94 ++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index f455a2e..193aef1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -102,6 +102,17 @@ static void intel_pipe_update_end(struct intel_crtc *crtc)
 	preempt_check_resched();
 }
 
+static void intel_update_primary_plane(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	int reg = DSPCNTR(crtc->plane);
+
+	if (crtc->primary_enabled)
+		I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
+	else
+		I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -199,6 +210,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
 	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
 
@@ -211,7 +224,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	I915_WRITE(SPCNTR(pipe, plane), sprctl);
 	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
 		   sprsurf_offset);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 }
@@ -228,11 +242,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
 		   ~SP_ENABLE);
 	/* Activate double buffered register update */
 	I915_WRITE(SPSURF(pipe, plane), 0);
-	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 
@@ -377,6 +394,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -395,7 +414,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRCTL(pipe), sprctl);
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 }
@@ -411,13 +431,16 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
 	/* Can't leave the scaler enabled... */
 	if (intel_plane->can_scale)
 		I915_WRITE(SPRSCALE(pipe), 0);
 	/* Activate double buffered register update */
 	I915_WRITE(SPRSURF(pipe), 0);
-	POSTING_READ(SPRSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 
@@ -566,6 +589,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
 
@@ -579,7 +604,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(DVSCNTR(pipe), dvscntr);
 	I915_WRITE(DVSSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 }
@@ -595,12 +621,15 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	intel_pipe_update_start(intel_crtc);
 
+	intel_update_primary_plane(intel_crtc);
+
 	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
 	/* Disable the scaler */
 	I915_WRITE(DVSSCALE(pipe), 0);
 	/* Flush double buffered register updates */
 	I915_WRITE(DVSSURF(pipe), 0);
-	POSTING_READ(DVSSURF(pipe));
+
+	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	intel_pipe_update_end(intel_crtc);
 
@@ -614,20 +643,10 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 }
 
 static void
-intel_enable_primary(struct drm_crtc *crtc)
+intel_post_enable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = true;
-
-	I915_WRITE(reg, I915_READ(reg) | DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 
 	/*
 	 * FIXME IPS should be fine as long as one plane is
@@ -646,17 +665,11 @@ intel_enable_primary(struct drm_crtc *crtc)
 }
 
 static void
-intel_disable_primary(struct drm_crtc *crtc)
+intel_pre_disable_primary(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int reg = DSPCNTR(intel_crtc->plane);
-
-	if (!intel_crtc->primary_enabled)
-		return;
-
-	intel_crtc->primary_enabled = false;
 
 	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == intel_crtc->plane)
@@ -670,9 +683,6 @@ intel_disable_primary(struct drm_crtc *crtc)
 	 * versa.
 	 */
 	hsw_disable_ips(intel_crtc);
-
-	I915_WRITE(reg, I915_READ(reg) & ~DISPLAY_PLANE_ENABLE);
-	intel_flush_primary_plane(dev_priv, intel_crtc->plane);
 }
 
 static int
@@ -766,7 +776,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_i915_gem_object *old_obj = intel_plane->obj;
 	int ret;
-	bool disable_primary = false;
+	bool primary_enabled;
 	bool visible;
 	int hscale, vscale;
 	int max_scale, min_scale;
@@ -937,8 +947,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * If the sprite is completely covering the primary plane,
 	 * we can disable the primary and save power.
 	 */
-	disable_primary = drm_rect_equals(&dst, &clip) && !colorkey_enabled(intel_plane);
-	WARN_ON(disable_primary && !visible && intel_crtc->active);
+	primary_enabled = !drm_rect_equals(&dst, &clip) || colorkey_enabled(intel_plane);
+	WARN_ON(!primary_enabled && !visible && intel_crtc->active);
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -965,12 +975,12 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
-		/*
-		 * Be sure to re-enable the primary before the sprite is no longer
-		 * covering it fully.
-		 */
-		if (!disable_primary)
-			intel_enable_primary(crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = primary_enabled;
+
+		if (primary_was_enabled && !primary_enabled)
+			intel_pre_disable_primary(crtc);
 
 		if (visible)
 			intel_plane->update_plane(plane, crtc, fb, obj,
@@ -979,8 +989,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		else
 			intel_plane->disable_plane(plane, crtc);
 
-		if (disable_primary)
-			intel_disable_primary(crtc);
+		if (!primary_was_enabled && primary_enabled)
+			intel_post_enable_primary(crtc);
 	}
 
 	/* Unpin old obj after new one is active to avoid ugliness */
@@ -1018,8 +1028,14 @@ intel_disable_plane(struct drm_plane *plane)
 	intel_crtc = to_intel_crtc(plane->crtc);
 
 	if (intel_crtc->active) {
-		intel_enable_primary(plane->crtc);
+		bool primary_was_enabled = intel_crtc->primary_enabled;
+
+		intel_crtc->primary_enabled = true;
+
 		intel_plane->disable_plane(plane, plane->crtc);
+
+		if (!primary_was_enabled && intel_crtc->primary_enabled)
+			intel_post_enable_primary(plane->crtc);
 	}
 
 	if (intel_plane->obj) {
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 5/5] drm/i915: Add pipe update trace points

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add trace points for observing the atomic pipe update mechanism.

v2: Rebased due to earlier changes
v3: Pass intel_crtc instead of drm_crtc (Daniel)

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_trace.h   | 77 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c |  6 +++
 2 files changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 6e580c9..57b4960 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -7,6 +7,7 @@
 
 #include <drm/drmP.h>
 #include "i915_drv.h"
+#include "intel_drv.h"
 #include "intel_ringbuffer.h"
 
 #undef TRACE_SYSTEM
@@ -14,6 +15,82 @@
 #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM)
 #define TRACE_INCLUDE_FILE i915_trace
 
+/* pipe updates */
+
+TRACE_EVENT(i915_pipe_update_start,
+	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max),
+	    TP_ARGS(crtc, min, max),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
+										       crtc->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_vblank_evaded,
+	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max),
+	    TP_ARGS(crtc, min, max),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
+										       crtc->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_end,
+	    TP_PROTO(struct intel_crtc *crtc),
+	    TP_ARGS(crtc),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
+										       crtc->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		      __entry->scanline)
+);
+
 /* object tracking */
 
 TRACE_EVENT(i915_gem_object_create,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 193aef1..57842a1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -65,6 +65,8 @@ static void intel_pipe_update_start(struct intel_crtc *crtc)
 
 	local_irq_disable();
 
+	trace_i915_pipe_update_start(crtc, min, max);
+
 	for (;;) {
 		/*
 		 * prepare_to_wait() has a memory barrier, which guarantees
@@ -94,10 +96,14 @@ static void intel_pipe_update_start(struct intel_crtc *crtc)
 	finish_wait(&crtc->vbl_wait, &wait);
 
 	drm_vblank_put(dev, pipe);
+
+	trace_i915_pipe_update_vblank_evaded(crtc, min, max);
 }
 
 static void intel_pipe_update_end(struct intel_crtc *crtc)
 {
+	trace_i915_pipe_update_end(crtc);
+
 	local_irq_enable();
 	preempt_check_resched();
 }
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 5/5] drm/i915: Add pipe update trace points

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add trace points for observing the atomic pipe update mechanism.

v2: Rebased due to earlier changes
v3: Pass intel_crtc instead of drm_crtc (Daniel)
v4: Pass frame counter from the caller to evaded/end since
    the caller now always has that ready

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_trace.h   | 75 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c |  6 +++
 2 files changed, 81 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 6e580c9..2afe8bd 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -7,6 +7,7 @@
 
 #include <drm/drmP.h>
 #include "i915_drv.h"
+#include "intel_drv.h"
 #include "intel_ringbuffer.h"
 
 #undef TRACE_SYSTEM
@@ -14,6 +15,80 @@
 #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM)
 #define TRACE_INCLUDE_FILE i915_trace
 
+/* pipe updates */
+
+TRACE_EVENT(i915_pipe_update_start,
+	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max),
+	    TP_ARGS(crtc, min, max),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
+										       crtc->pipe);
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_vblank_evaded,
+	    TP_PROTO(struct intel_crtc *crtc, u32 min, u32 max, u32 frame),
+	    TP_ARGS(crtc, min, max, frame),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     __field(u32, min)
+			     __field(u32, max)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = frame;
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   __entry->min = min;
+			   __entry->max = max;
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u, min=%u, max=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		       __entry->scanline, __entry->min, __entry->max)
+);
+
+TRACE_EVENT(i915_pipe_update_end,
+	    TP_PROTO(struct intel_crtc *crtc, u32 frame),
+	    TP_ARGS(crtc, frame),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(u32, frame)
+			     __field(u32, scanline)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = crtc->pipe;
+			   __entry->frame = frame;
+			   __entry->scanline = intel_get_crtc_scanline(crtc);
+			   ),
+
+	    TP_printk("pipe %c, frame=%u, scanline=%u",
+		      pipe_name(__entry->pipe), __entry->frame,
+		      __entry->scanline)
+);
+
 /* object tracking */
 
 TRACE_EVENT(i915_gem_object_create,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 808e905..9e1c818 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -68,6 +68,8 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
 
 	local_irq_disable();
 
+	trace_i915_pipe_update_start(crtc, min, max);
+
 	for (;;) {
 		/*
 		 * prepare_to_wait() has a memory barrier, which guarantees
@@ -100,6 +102,8 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl
 
 	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
 
+	trace_i915_pipe_update_vblank_evaded(crtc, min, max, *start_vbl_count);
+
 	return true;
 }
 
@@ -109,6 +113,8 @@ static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 	enum pipe pipe = crtc->pipe;
 	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
 
+	trace_i915_pipe_update_end(crtc, end_vbl_count);
+
 	local_irq_enable();
 	preempt_check_resched();
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/5] drm/i915: Add a small adjustment to the pixel counter on interlaced modes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

In interlaced modes, the pixel counter counts all pixels,
so one field will have htotal more pixels. In order to avoid
the reported position from jumping backwards when the pixel
counter is beyond the length of the shorter field, just
clamp the position the length of the shorter field. This
matches how the scanline counter based position works since
the scanline counter doesn't count the two half lines.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1b73cf2..9f1c449 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -800,6 +800,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		vtotal *= htotal;
 
 		/*
+		 * In interlaced modes, the pixel counter counts all pixels,
+		 * so one field will have htotal more pixels. In order to avoid
+		 * the reported position from jumping backwards when the pixel
+		 * counter is beyond the length of the shorter field, just
+		 * clamp the position the length of the shorter field. This
+		 * matches how the scanline counter based position works since
+		 * the scanline counter doesn't count the two half lines.
+		 */
+		if (position >= vtotal)
+			position = vtotal - 1;
+
+		/*
 		 * 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
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/5] drm/i915: Improve gen3/4 frame counter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently the logic to fix up the frame counter on gen3/4 assumes that
start of vblank occurs at vblank_start*htotal pixels, when in fact
it occurs htotal-hsync_start pixels earlier. Apply the appropriate
adjustment to make the frame counter more accurate.

Also fix the vblank start position for interlaced display modes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9f1c449..fc49fb6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -639,7 +639,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
 	unsigned long high_frame;
 	unsigned long low_frame;
-	u32 high1, high2, low, pixel, vbl_start;
+	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
 
 	if (!i915_pipe_enabled(dev, pipe)) {
 		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
@@ -653,17 +653,28 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 		const struct drm_display_mode *mode =
 			&intel_crtc->config.adjusted_mode;
 
-		vbl_start = mode->crtc_vblank_start * mode->crtc_htotal;
+		htotal = mode->crtc_htotal;
+		hsync_start = mode->crtc_hsync_start;
+		vbl_start = mode->crtc_vblank_start;
+		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+			vbl_start = DIV_ROUND_UP(vbl_start, 2);
 	} else {
 		enum transcoder cpu_transcoder = (enum transcoder) pipe;
-		u32 htotal;
 
 		htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff) + 1;
+		hsync_start = (I915_READ(HSYNC(cpu_transcoder))  & 0x1fff) + 1;
 		vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff) + 1;
-
-		vbl_start *= htotal;
+		if ((I915_READ(PIPECONF(cpu_transcoder)) &
+		     PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
+			vbl_start = DIV_ROUND_UP(vbl_start, 2);
 	}
 
+	/* Convert to pixel count */
+	vbl_start *= htotal;
+
+	/* Start of vblank event occurs at start of hsync */
+	vbl_start -= htotal - hsync_start;
+
 	high_frame = PIPEFRAME(pipe);
 	low_frame = PIPEFRAMEPIXEL(pipe);
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/5] drm/i915: Fix gen2 scanline counter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On gen2 the scanline counter behaves a bit differently from the
later generations. Instead of adding one to the raw scanline
counter value, we must subtract one.

I've not yet verified which way gen3 works. My suspicion is that
it behaves like gen4 since for the most part the other timing
related registers are similar to gen4. The one similarity to gen2
that gen3 shares is the lack of the "start of vblank" interrupt.
The event is there internally even on gen2 (that's where the
registers get latched) but the PIPESTAT status bit and the actual
interrupt are not available to the driver.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fc49fb6..bec9af8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -724,26 +724,36 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
 	enum pipe pipe = crtc->pipe;
-	int vtotal = mode->crtc_vtotal;
-	int position;
+	int position, vtotal;
 
+	vtotal = mode->crtc_vtotal;
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 		vtotal /= 2;
 
+	/*
+	 * The scanline counter increments at the leading edge of hsync.
+	 *
+	 * On most platforms 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.
+	 *
+	 * Gen2 is the exception as the scanline counter starts counting
+	 * from 1 instead of vtotal-1, so we have to subtract one (or
+	 * rather add vtotal-1 to keep the value positive), instead of
+	 * adding one.
+	 *
+	 * FIXME which way does gen3 work? Gen4 is for sure in the +1 camp.
+	 */
 	if (IS_GEN2(dev))
-		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
+		position = (__raw_i915_read32(dev_priv, PIPEDSL(pipe)) &
+			    DSL_LINEMASK_GEN2) + vtotal - 1;
 	else
-		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
+		position = (__raw_i915_read32(dev_priv, PIPEDSL(pipe)) &
+			    DSL_LINEMASK_GEN3) + 1;
 
-	/*
-	 * 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.
-	 */
-	return (position + 1) % vtotal;
+	return position % vtotal;
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 8/5] drm/i915: Fix gen2 scanline counter

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On gen2 the scanline counter behaves a bit differently from the
later generations. Instead of adding one to the raw scanline
counter value, we must subtract one.

v2: Remove the gen3 FIXME. Gen3 behaves like gen4

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index fc49fb6..40adce0 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -724,26 +724,34 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
 	enum pipe pipe = crtc->pipe;
-	int vtotal = mode->crtc_vtotal;
-	int position;
+	int position, vtotal;
 
+	vtotal = mode->crtc_vtotal;
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 		vtotal /= 2;
 
+	/*
+	 * The scanline counter increments at the leading edge of hsync.
+	 *
+	 * On most platforms 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.
+	 *
+	 * Gen2 is the exception as the scanline counter starts counting
+	 * from 1 instead of vtotal-1, so we have to subtract one (or
+	 * rather add vtotal-1 to keep the value positive), instead of
+	 * adding one.
+	 */
 	if (IS_GEN2(dev))
-		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
+		position = (__raw_i915_read32(dev_priv, PIPEDSL(pipe)) &
+			    DSL_LINEMASK_GEN2) + vtotal - 1;
 	else
-		position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
+		position = (__raw_i915_read32(dev_priv, PIPEDSL(pipe)) &
+			    DSL_LINEMASK_GEN3) + 1;
 
-	/*
-	 * 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.
-	 */
-	return (position + 1) % vtotal;
+	return position % vtotal;
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 9/5] drm/i915: Draw a picture about video timings

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The docs are a bit lacking when it comes to describing when certain
timing related events occur in the hardware. Draw a picture which
tries to capture the most important ones.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 48 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bec9af8..6fa8693 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -625,6 +625,54 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
 	}
 }
 
+/*
+ * This timing diagram depicts the video signal in and
+ * around the vertical blanking period.
+ *
+ * Assumptions about the fictitious mode used in this example:
+ *  vblank_start >= 3
+ *  vsync_start = vblank_start + 1
+ *  vsync_end = vblank_start + 2
+ *  vtotal = vblank_start + 3
+ *
+ *           start of vblank:
+ *           latch double buffered registers
+ *           increment frame counter (ctg+)
+ *           generate start of vblank interrupt (gen4+)
+ *           |
+ *           |          frame start:
+ *           |          generate frame start interrupt (aka. vblank interrupt) (gmch)
+ *           |          may be shifted forward 1-3 extra lines via PIPECONF
+ *           |          |
+ *           |          |  start of vsync:
+ *           |          |  generate vsync interrupt
+ *           |          |  |
+ * ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___
+ *       .   \hs/   .      \hs/          \hs/          \hs/   .      \hs/
+ * ----va---> <-----------------vb--------------------> <--------va---------
+ *       |          |       <----vs----->                     |
+ * -vbs-----> <---vbs+1---> <---vbs+2---> <-----0-----> <-----1-----> <---2- (scanline gen2)
+ * -vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1---> <---vbs+2---> <---0- (scanline gen3+)
+ *       |          |                                         |
+ *       last visible pixel                                   first visible pixel
+ *                  |                                         increment frame counter (gen3/4)
+ *                  pixel counter = vblank_start * htotal     pixel counter = 0 (gen3/4)
+ *
+ * x  = horizontal active
+ * _  = horizontal blanking
+ * hs = horizontal sync
+ * va = vertical active
+ * vb = vertical blanking
+ * vs = vertical sync
+ * vbs = vblank_start (number)
+ *
+ * Summary:
+ * - most events happen at the start of horizontal sync
+ * - frame start happens at the start of horizontal blank
+ * - gen3/4 pixel and frame counter are synchronized with the start
+ *   of horizontal active on the first line of vertical active
+ */
+
 static u32 i8xx_get_vblank_counter(struct drm_device *dev, int pipe)
 {
 	/* Gen2 doesn't have a hardware frame counter */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 9/5] drm/i915: Draw a picture about video timings

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The docs are a bit lacking when it comes to describing when certain
timing related events occur in the hardware. Draw a picture which
tries to capture the most important ones.

v2: Clarify a few details (Imre)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 40adce0..ed0df3e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -625,6 +625,55 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
 	}
 }
 
+/*
+ * This timing diagram depicts the video signal in and
+ * around the vertical blanking period.
+ *
+ * Assumptions about the fictitious mode used in this example:
+ *  vblank_start >= 3
+ *  vsync_start = vblank_start + 1
+ *  vsync_end = vblank_start + 2
+ *  vtotal = vblank_start + 3
+ *
+ *           start of vblank:
+ *           latch double buffered registers
+ *           increment frame counter (ctg+)
+ *           generate start of vblank interrupt (gen4+)
+ *           |
+ *           |          frame start:
+ *           |          generate frame start interrupt (aka. vblank interrupt) (gmch)
+ *           |          may be shifted forward 1-3 extra lines via PIPECONF
+ *           |          |
+ *           |          |  start of vsync:
+ *           |          |  generate vsync interrupt
+ *           |          |  |
+ * ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___
+ *       .   \hs/   .      \hs/          \hs/          \hs/   .      \hs/
+ * ----va---> <-----------------vb--------------------> <--------va---------
+ *       |          |       <----vs----->                     |
+ * -vbs-----> <---vbs+1---> <---vbs+2---> <-----0-----> <-----1-----> <---2- (scanline counter gen2)
+ * -vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1---> <---vbs+2---> <---0- (scanline counter gen3+)
+ *       |          |                                         |
+ *       last visible pixel                                   first visible pixel
+ *                  |                                         increment frame counter (gen3/4)
+ *                  pixel counter = vblank_start * htotal     pixel counter = 0 (gen3/4)
+ *
+ * x  = horizontal active
+ * _  = horizontal blanking
+ * hs = horizontal sync
+ * va = vertical active
+ * vb = vertical blanking
+ * vs = vertical sync
+ * vbs = vblank_start (number)
+ *
+ * Summary:
+ * - most events happen at the start of horizontal sync
+ * - frame start happens at the start of horizontal blank, 1-4 lines
+ *   (depending on PIPECONF settings) after the start of vblank
+ * - gen3/4 pixel and frame counter are synchronized with the start
+ *   of horizontal active on the first line of vertical active
+ */
+
 static u32 i8xx_get_vblank_counter(struct drm_device *dev, int pipe)
 {
 	/* Gen2 doesn't have a hardware frame counter */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 9/5] drm/i915: Draw a picture about video timings

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 3462 bytes --]

On Thu, 2014-02-20 at 13:14 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The docs are a bit lacking when it comes to describing when certain
> timing related events occur in the hardware. Draw a picture which
> tries to capture the most important ones.
> 
> v2: Clarify a few details (Imre)
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 49 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 40adce0..ed0df3e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -625,6 +625,55 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
>  	}
>  }
>  
> +/*
> + * This timing diagram depicts the video signal in and
> + * around the vertical blanking period.
> + *
> + * Assumptions about the fictitious mode used in this example:
> + *  vblank_start >= 3
> + *  vsync_start = vblank_start + 1
> + *  vsync_end = vblank_start + 2
> + *  vtotal = vblank_start + 3
> + *
> + *           start of vblank:
> + *           latch double buffered registers
> + *           increment frame counter (ctg+)
> + *           generate start of vblank interrupt (gen4+)
> + *           |
> + *           |          frame start:
> + *           |          generate frame start interrupt (aka. vblank interrupt) (gmch)
> + *           |          may be shifted forward 1-3 extra lines via PIPECONF
> + *           |          |
> + *           |          |  start of vsync:
> + *           |          |  generate vsync interrupt
> + *           |          |  |
> + * ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___xxxx___    ___
> + *       .   \hs/   .      \hs/          \hs/          \hs/   .      \hs/
> + * ----va---> <-----------------vb--------------------> <--------va---------
> + *       |          |       <----vs----->                     |
> + * -vbs-----> <---vbs+1---> <---vbs+2---> <-----0-----> <-----1-----> <---2- (scanline counter gen2)
> + * -vbs-2---> <---vbs-1---> <---vbs-----> <---vbs+1---> <---vbs+2---> <---0- (scanline counter gen3+)
> + *       |          |                                         |
> + *       last visible pixel                                   first visible pixel
> + *                  |                                         increment frame counter (gen3/4)
> + *                  pixel counter = vblank_start * htotal     pixel counter = 0 (gen3/4)
> + *
> + * x  = horizontal active
> + * _  = horizontal blanking
> + * hs = horizontal sync
> + * va = vertical active
> + * vb = vertical blanking
> + * vs = vertical sync
> + * vbs = vblank_start (number)
> + *
> + * Summary:
> + * - most events happen at the start of horizontal sync
> + * - frame start happens at the start of horizontal blank, 1-4 lines
> + *   (depending on PIPECONF settings) after the start of vblank
> + * - gen3/4 pixel and frame counter are synchronized with the start
> + *   of horizontal active on the first line of vertical active

A nice description that I haven't found in any docs, so:
Acked-by: Imre Deak <imre.deak@intel.com>

> + */
> +
>  static u32 i8xx_get_vblank_counter(struct drm_device *dev, int pipe)
>  {
>  	/* Gen2 doesn't have a hardware frame counter */


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/5] drm/i915: Improve gen3/4 frame counter

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2798 bytes --]

On Tue, 2014-02-18 at 14:04 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently the logic to fix up the frame counter on gen3/4 assumes that
> start of vblank occurs at vblank_start*htotal pixels, when in fact
> it occurs htotal-hsync_start pixels earlier. Apply the appropriate
> adjustment to make the frame counter more accurate.
> 
> Also fix the vblank start position for interlaced display modes.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9f1c449..fc49fb6 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -639,7 +639,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	unsigned long high_frame;
>  	unsigned long low_frame;
> -	u32 high1, high2, low, pixel, vbl_start;
> +	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
>  
>  	if (!i915_pipe_enabled(dev, pipe)) {
>  		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> @@ -653,17 +653,28 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
>  		const struct drm_display_mode *mode =
>  			&intel_crtc->config.adjusted_mode;
>  
> -		vbl_start = mode->crtc_vblank_start * mode->crtc_htotal;
> +		htotal = mode->crtc_htotal;
> +		hsync_start = mode->crtc_hsync_start;
> +		vbl_start = mode->crtc_vblank_start;
> +		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +			vbl_start = DIV_ROUND_UP(vbl_start, 2);

The adjustment for interlace mode is already done in drm_mode_setcrtc,
so I think we don't need it here. Otherwise this patch makes sense to me
based on the signal chart you drew on this, so:

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  	} else {
>  		enum transcoder cpu_transcoder = (enum transcoder) pipe;
> -		u32 htotal;
>  
>  		htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff) + 1;
> +		hsync_start = (I915_READ(HSYNC(cpu_transcoder))  & 0x1fff) + 1;
>  		vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff) + 1;
> -
> -		vbl_start *= htotal;
> +		if ((I915_READ(PIPECONF(cpu_transcoder)) &
> +		     PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
> +			vbl_start = DIV_ROUND_UP(vbl_start, 2);
>  	}
>  
> +	/* Convert to pixel count */
> +	vbl_start *= htotal;
> +
> +	/* Start of vblank event occurs at start of hsync */
> +	vbl_start -= htotal - hsync_start;
> +
>  	high_frame = PIPEFRAME(pipe);
>  	low_frame = PIPEFRAMEPIXEL(pipe);
>  


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/5] drm/i915: Improve gen3/4 frame counter

[Ville Syrjälä]

On Tue, Feb 18, 2014 at 04:16:00PM +0200, Imre Deak wrote:
> On Tue, 2014-02-18 at 14:04 +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently the logic to fix up the frame counter on gen3/4 assumes that
> > start of vblank occurs at vblank_start*htotal pixels, when in fact
> > it occurs htotal-hsync_start pixels earlier. Apply the appropriate
> > adjustment to make the frame counter more accurate.
> > 
> > Also fix the vblank start position for interlaced display modes.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 9f1c449..fc49fb6 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -639,7 +639,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
> >  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> >  	unsigned long high_frame;
> >  	unsigned long low_frame;
> > -	u32 high1, high2, low, pixel, vbl_start;
> > +	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> >  
> >  	if (!i915_pipe_enabled(dev, pipe)) {
> >  		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> > @@ -653,17 +653,28 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
> >  		const struct drm_display_mode *mode =
> >  			&intel_crtc->config.adjusted_mode;
> >  
> > -		vbl_start = mode->crtc_vblank_start * mode->crtc_htotal;
> > +		htotal = mode->crtc_htotal;
> > +		hsync_start = mode->crtc_hsync_start;
> > +		vbl_start = mode->crtc_vblank_start;
> > +		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > +			vbl_start = DIV_ROUND_UP(vbl_start, 2);
> 
> The adjustment for interlace mode is already done in drm_mode_setcrtc,
> so I think we don't need it here.

We throw away the values filled in by drm_mode_setcrtc(). Which is a
good thing since it rounds the result the wrong way (for our hardware
at least). The values we see here are filled in by
intel_modeset_pipe_config() which doesn't pass the
CRTC_INTERLACE_HALVE_V flag to drm_mode_set_crtcinfo().

> Otherwise this patch makes sense to me
> based on the signal chart you drew on this, so:
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> >  	} else {
> >  		enum transcoder cpu_transcoder = (enum transcoder) pipe;
> > -		u32 htotal;
> >  
> >  		htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff) + 1;
> > +		hsync_start = (I915_READ(HSYNC(cpu_transcoder))  & 0x1fff) + 1;
> >  		vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff) + 1;
> > -
> > -		vbl_start *= htotal;
> > +		if ((I915_READ(PIPECONF(cpu_transcoder)) &
> > +		     PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
> > +			vbl_start = DIV_ROUND_UP(vbl_start, 2);
> >  	}
> >  
> > +	/* Convert to pixel count */
> > +	vbl_start *= htotal;
> > +
> > +	/* Start of vblank event occurs at start of hsync */
> > +	vbl_start -= htotal - hsync_start;
> > +
> >  	high_frame = PIPEFRAME(pipe);
> >  	low_frame = PIPEFRAMEPIXEL(pipe);
> >  
> 



-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 7/5] drm/i915: Improve gen3/4 frame counter

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 3591 bytes --]

On Tue, 2014-02-18 at 16:41 +0200, Ville Syrjälä wrote:
> On Tue, Feb 18, 2014 at 04:16:00PM +0200, Imre Deak wrote:
> > On Tue, 2014-02-18 at 14:04 +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Currently the logic to fix up the frame counter on gen3/4 assumes that
> > > start of vblank occurs at vblank_start*htotal pixels, when in fact
> > > it occurs htotal-hsync_start pixels earlier. Apply the appropriate
> > > adjustment to make the frame counter more accurate.
> > > 
> > > Also fix the vblank start position for interlaced display modes.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++-----
> > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 9f1c449..fc49fb6 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -639,7 +639,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
> > >  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> > >  	unsigned long high_frame;
> > >  	unsigned long low_frame;
> > > -	u32 high1, high2, low, pixel, vbl_start;
> > > +	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> > >  
> > >  	if (!i915_pipe_enabled(dev, pipe)) {
> > >  		DRM_DEBUG_DRIVER("trying to get vblank count for disabled "
> > > @@ -653,17 +653,28 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
> > >  		const struct drm_display_mode *mode =
> > >  			&intel_crtc->config.adjusted_mode;
> > >  
> > > -		vbl_start = mode->crtc_vblank_start * mode->crtc_htotal;
> > > +		htotal = mode->crtc_htotal;
> > > +		hsync_start = mode->crtc_hsync_start;
> > > +		vbl_start = mode->crtc_vblank_start;
> > > +		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > > +			vbl_start = DIV_ROUND_UP(vbl_start, 2);
> > 
> > The adjustment for interlace mode is already done in drm_mode_setcrtc,
> > so I think we don't need it here.
> 
> We throw away the values filled in by drm_mode_setcrtc(). Which is a
> good thing since it rounds the result the wrong way (for our hardware
> at least). The values we see here are filled in by
> intel_modeset_pipe_config() which doesn't pass the
> CRTC_INTERLACE_HALVE_V flag to drm_mode_set_crtcinfo().

Ah, true, I see now. This looks also ok then.

--Imre

> > Otherwise this patch makes sense to me
> > based on the signal chart you drew on this, so:
> > 
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > 
> > >  	} else {
> > >  		enum transcoder cpu_transcoder = (enum transcoder) pipe;
> > > -		u32 htotal;
> > >  
> > >  		htotal = ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff) + 1;
> > > +		hsync_start = (I915_READ(HSYNC(cpu_transcoder))  & 0x1fff) + 1;
> > >  		vbl_start = (I915_READ(VBLANK(cpu_transcoder)) & 0x1fff) + 1;
> > > -
> > > -		vbl_start *= htotal;
> > > +		if ((I915_READ(PIPECONF(cpu_transcoder)) &
> > > +		     PIPECONF_INTERLACE_MASK) != PIPECONF_PROGRESSIVE)
> > > +			vbl_start = DIV_ROUND_UP(vbl_start, 2);
> > >  	}
> > >  
> > > +	/* Convert to pixel count */
> > > +	vbl_start *= htotal;
> > > +
> > > +	/* Start of vblank event occurs at start of hsync */
> > > +	vbl_start -= htotal - hsync_start;
> > > +
> > >  	high_frame = PIPEFRAME(pipe);
> > >  	low_frame = PIPEFRAMEPIXEL(pipe);
> > >  
> > 
> 
> 
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/5] drm/i915: Atomic sprites v3

[sourab gupta]

[-- Attachment #1.1: Type: text/plain, Size: 2648 bytes --]

On Thu, Feb 13, 2014 at 9:12 PM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> OK, so Daniel convinced me that the check+wait thing I used earlier
> wasn't really safe. I then decided that the only to make it safe it
> open coding the wait_for_event() mysefl, and after doing that I
> realized that we don't even need the silly vbl_received thingy, and
> just a wait queue + DSL check is enough. Let's see if Daniel can
> tear this one to shreds too :)
>
> Oh and I also did some more digging to the scanline counter + vblank
> interrupt mess, and decided that all my earlier hacks were crap. The
> solution was so simple once I started to look at the pixel counter
> values on gen4. I got inspired to have another look at this mess
> after Imre did some clock based vblank interrupt timing measurements
> on his VLV. Art confirmed my findings, so it should all be good now,
> no ugly hacks included.
>
> There is one tiny race window on gen2. That's caused by the fact that
> we only have the frame start interrupt there, and scanline counter
> increments slightly after the interrupt is triggered. So if we would
> extremely unlucky we might wake up, check the DSL, and determine we
> need to sleep more. But at this time we don't use the atomic update
> mechanism on gen2, and I think the windows is so small that we should
> be able to ignore it. Also the 1ms timeout would anyway prevent us from
> sleeping another full frame. So if it ever becomes a problem, we can
> try to think of something to overcome it, but at this point it doesn't
> seem worth the effort.
>
> Ville Syrjälä (5):
>   drm/i915: Fix scanout position for real
>   drm/i915: Add intel_get_crtc_scanline()
>   drm/i915: Make sprite updates atomic
>   drm/i915: Perform primary enable/disable atomically with sprite
>     updates
>   drm/i915: Add pipe update trace points
>
>  drivers/gpu/drm/i915/i915_irq.c      | 137 ++++++++++++------------
>  drivers/gpu/drm/i915/i915_trace.h    |  77 ++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   2 +
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 195
> ++++++++++++++++++++++++++++-------
>  5 files changed, 306 insertions(+), 108 deletions(-)
>
> Went through the patches and they seem fine.
Reviewed by: "Sourab Gupta <sourabgupta@gmail.com>"
for all 5 patches in the series.

> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 3598 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/5] drm/i915: Atomic sprites v3

[akash goel]

[-- Attachment #1.1: Type: text/plain, Size: 2820 bytes --]

We went through all the 5 patches.

Already patches have undergone multiple revisions and a thorough review has
been done for them. So we couldn't find any anomalies as such & all

patches seems to be fine.


Reviewed-by: "Akash Goel <akash.goels@gmail.com>"  for all the patches in
the series.


On Thu, Feb 13, 2014 at 9:12 PM, <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> OK, so Daniel convinced me that the check+wait thing I used earlier
> wasn't really safe. I then decided that the only to make it safe it
> open coding the wait_for_event() mysefl, and after doing that I
> realized that we don't even need the silly vbl_received thingy, and
> just a wait queue + DSL check is enough. Let's see if Daniel can
> tear this one to shreds too :)
>
> Oh and I also did some more digging to the scanline counter + vblank
> interrupt mess, and decided that all my earlier hacks were crap. The
> solution was so simple once I started to look at the pixel counter
> values on gen4. I got inspired to have another look at this mess
> after Imre did some clock based vblank interrupt timing measurements
> on his VLV. Art confirmed my findings, so it should all be good now,
> no ugly hacks included.
>
> There is one tiny race window on gen2. That's caused by the fact that
> we only have the frame start interrupt there, and scanline counter
> increments slightly after the interrupt is triggered. So if we would
> extremely unlucky we might wake up, check the DSL, and determine we
> need to sleep more. But at this time we don't use the atomic update
> mechanism on gen2, and I think the windows is so small that we should
> be able to ignore it. Also the 1ms timeout would anyway prevent us from
> sleeping another full frame. So if it ever becomes a problem, we can
> try to think of something to overcome it, but at this point it doesn't
> seem worth the effort.
>
> Ville Syrjälä (5):
>   drm/i915: Fix scanout position for real
>   drm/i915: Add intel_get_crtc_scanline()
>   drm/i915: Make sprite updates atomic
>   drm/i915: Perform primary enable/disable atomically with sprite
>     updates
>   drm/i915: Add pipe update trace points
>
>  drivers/gpu/drm/i915/i915_irq.c      | 137 ++++++++++++------------
>  drivers/gpu/drm/i915/i915_trace.h    |  77 ++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   2 +
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 195
> ++++++++++++++++++++++++++++-------
>  5 files changed, 306 insertions(+), 108 deletions(-)
>
> --
> 1.8.3.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 4720 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx