diff mbox

drm/i915: use BLT ring for flips on IVB

Message ID 1307646877-1979-1-git-send-email-jbarnes@virtuousgeek.org
State New, archived
Headers show

Commit Message

Jesse Barnes June 9, 2011, 7:14 p.m. UTC
Found a couple more problems:
  1) MI_DISPLAY_FLIP should take a '1' in the last byte to indicate
     length on IVB
  2) apprently only the BLT ring version of the command actually causes
     interrupts to be generated

With this patch, modetest -v works on my test platform.  Clearly it's in
need of more splitting though.  The case statement should be split into
per-chipset flip command generation.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

Comments

Chris Wilson June 13, 2011, 10:22 p.m. UTC | #1
On Tue, 14 Jun 2011 00:14:11 -0700, Kenneth Graunke <kenneth@whitecape.org> wrote:
> On 06/09/2011 12:14 PM, Jesse Barnes wrote:
> > Found a couple more problems:
> >    1) MI_DISPLAY_FLIP should take a '1' in the last byte to indicate
> >       length on IVB
> >    2) apprently only the BLT ring version of the command actually causes
> >       interrupts to be generated
> >
> > With this patch, modetest -v works on my test platform.  Clearly it's in
> > need of more splitting though.  The case statement should be split into
> > per-chipset flip command generation.
> >
> > Signed-off-by: Jesse Barnes<jbarnes@virtuousgeek.org>
> 
> Reviewed-and-tested-by: Kenneth Graunke <kenneth@whitecape.org>

But it's not the neatest patch in the world ;-)

Using struct intel_ring_buffer *ring for all generation and switching
between blt/render on demand should be neater. Also we should be able
to do pageflips from either the render or blt ring on gen7, right? That
will be useful to avoid a synchronisation point.
-Chris
Jesse Barnes June 13, 2011, 11 p.m. UTC | #2
On Tue, 14 Jun 2011 00:29:08 -0700
Kenneth Graunke <kenneth@whitecape.org> wrote:

> On 06/13/2011 03:22 PM, Chris Wilson wrote:
> > On Tue, 14 Jun 2011 00:14:11 -0700, Kenneth Graunke<kenneth@whitecape.org>  wrote:
> >> On 06/09/2011 12:14 PM, Jesse Barnes wrote:
> >>> Found a couple more problems:
> >>>     1) MI_DISPLAY_FLIP should take a '1' in the last byte to indicate
> >>>        length on IVB
> >>>     2) apprently only the BLT ring version of the command actually causes
> >>>        interrupts to be generated
> >>>
> >>> With this patch, modetest -v works on my test platform.  Clearly it's in
> >>> need of more splitting though.  The case statement should be split into
> >>> per-chipset flip command generation.
> >>>
> >>> Signed-off-by: Jesse Barnes<jbarnes@virtuousgeek.org>
> >>
> >> Reviewed-and-tested-by: Kenneth Graunke<kenneth@whitecape.org>
> >
> > But it's not the neatest patch in the world ;-)
> 
> True :)
> 
> > Using struct intel_ring_buffer *ring for all generation and switching
> > between blt/render on demand should be neater. Also we should be able
> > to do pageflips from either the render or blt ring on gen7, right? That
> > will be useful to avoid a synchronisation point.
> > -Chris
> 
> I thought so, from reading the docs.  But it doesn't seem to work from 
> the render ring...

Yeah, it's just an ugly hack.  So far the hw guys are confused why the
render ring doesn't work and the blit ring does, all else being equal.
I'll clean up the patch and re-post, and follow up with them and see if
we can root cause this issue either way.

Thanks,
Kenneth Graunke June 14, 2011, 7:29 a.m. UTC | #3
On 06/13/2011 03:22 PM, Chris Wilson wrote:
> On Tue, 14 Jun 2011 00:14:11 -0700, Kenneth Graunke<kenneth@whitecape.org>  wrote:
>> On 06/09/2011 12:14 PM, Jesse Barnes wrote:
>>> Found a couple more problems:
>>>     1) MI_DISPLAY_FLIP should take a '1' in the last byte to indicate
>>>        length on IVB
>>>     2) apprently only the BLT ring version of the command actually causes
>>>        interrupts to be generated
>>>
>>> With this patch, modetest -v works on my test platform.  Clearly it's in
>>> need of more splitting though.  The case statement should be split into
>>> per-chipset flip command generation.
>>>
>>> Signed-off-by: Jesse Barnes<jbarnes@virtuousgeek.org>
>>
>> Reviewed-and-tested-by: Kenneth Graunke<kenneth@whitecape.org>
>
> But it's not the neatest patch in the world ;-)

True :)

> Using struct intel_ring_buffer *ring for all generation and switching
> between blt/render on demand should be neater. Also we should be able
> to do pageflips from either the render or blt ring on gen7, right? That
> will be useful to avoid a synchronisation point.
> -Chris

I thought so, from reading the docs.  But it doesn't seem to work from 
the render ring...

--Kenneth
Chris Wilson June 14, 2011, 7:40 a.m. UTC | #4
On Mon, 13 Jun 2011 16:00:53 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Yeah, it's just an ugly hack.

Let's also put a default: return -ENODEV in there for future sanity.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 81a9059..37d0087 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6343,9 +6343,11 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	/* Offset into the new buffer for cases of shared fbs between CRTCs */
 	offset = crtc->y * fb->pitch + crtc->x * fb->bits_per_pixel/8;
 
-	ret = BEGIN_LP_RING(4);
-	if (ret)
-		goto cleanup_objs;
+	if (!IS_IVYBRIDGE(dev)) {
+		ret = BEGIN_LP_RING(4);
+		if (ret)
+			goto cleanup_objs;
+	}
 
 	/* Block clients from rendering to the new back buffer until
 	 * the flip occurs and the object is no longer visible.
@@ -6390,7 +6392,6 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		break;
 
 	case 6:
-	case 7:
 		OUT_RING(MI_DISPLAY_FLIP |
 			 MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
 		OUT_RING(fb->pitch | obj->tiling_mode);
@@ -6400,8 +6401,18 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		pipesrc = I915_READ(PIPESRC(pipe)) & 0x0fff0fff;
 		OUT_RING(pf | pipesrc);
 		break;
+	case 7:
+		intel_ring_begin(&dev_priv->ring[BCS], 4);
+		intel_ring_emit(&dev_priv->ring[BCS], (MI_DISPLAY_FLIP_I915 | (intel_crtc->plane << 19)));
+		intel_ring_emit(&dev_priv->ring[BCS], (fb->pitch | obj->tiling_mode));
+		intel_ring_emit(&dev_priv->ring[BCS], (obj->gtt_offset));
+		intel_ring_emit(&dev_priv->ring[BCS], (MI_NOOP));
+		intel_ring_advance(&dev_priv->ring[BCS]);
+		break;
 	}
-	ADVANCE_LP_RING();
+
+	if (!IS_IVYBRIDGE(dev))
+		ADVANCE_LP_RING();
 
 	mutex_unlock(&dev->struct_mutex);