[PATCH 0/3] drm/i915: Reduce fence usage for scanout

[PATCH 0/3] drm/i915: Reduce fence usage for scanout

[ville.syrjala]

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

After staring at the scanout vs. fences situation a bit, I decided
to reduce our scanout fence usage a bit.

Ville Syrjälä (3):
  drm/i915: Don't waste fences on sprite buffers
  drm/i915: Fix fence leaks if fbdev setup fails
  drm/i915: Don't waste fences for scanout when not needed

 drivers/gpu/drm/i915/intel_display.c | 117 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |  10 ++-
 drivers/gpu/drm/i915/intel_fbdev.c   |   6 +-
 drivers/gpu/drm/i915/intel_sprite.c  |   2 +-
 4 files changed, 91 insertions(+), 44 deletions(-)

-- 
1.8.3.2

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

[PATCH 1/3] drm/i915: Don't waste fences on sprite buffers

[ville.syrjala]

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

Sprites never need fences for scanout, so let's not waste them where not
needed.

Renamed stuff a bit so that we have a nice set of functions for pinning
scanout buffers. intel_pin_fb_obj() and intel_unpin_fb_obj() are now the
versions that don't add fences, are intel_pin_fenced_fb_obj() and
intel_unpin_fenced_fb_obj() are the versions that do.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 122f871..fd48c2e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1929,27 +1929,22 @@ static bool need_vtd_wa(struct drm_device *dev)
 	return false;
 }
 
-int
-intel_pin_and_fence_fb_obj(struct drm_device *dev,
-			   struct drm_i915_gem_object *obj,
-			   struct intel_ring_buffer *pipelined)
+static int intel_fb_obj_alignment(struct drm_device *dev,
+				  struct drm_i915_gem_object *obj,
+				  u32 *alignment)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 alignment;
-	int ret;
-
 	switch (obj->tiling_mode) {
 	case I915_TILING_NONE:
 		if (IS_BROADWATER(dev) || IS_CRESTLINE(dev))
-			alignment = 128 * 1024;
+			*alignment = 128 * 1024;
 		else if (INTEL_INFO(dev)->gen >= 4)
-			alignment = 4 * 1024;
+			*alignment = 4 * 1024;
 		else
-			alignment = 64 * 1024;
+			*alignment = 64 * 1024;
 		break;
 	case I915_TILING_X:
 		/* pin() will align the object as required by fence */
-		alignment = 0;
+		*alignment = 0;
 		break;
 	case I915_TILING_Y:
 		WARN(1, "Y tiled bo slipped through, driver bug!\n");
@@ -1963,8 +1958,23 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	 * we should always have valid PTE following the scanout preventing
 	 * the VT-d warning.
 	 */
-	if (need_vtd_wa(dev) && alignment < 256 * 1024)
-		alignment = 256 * 1024;
+	if (need_vtd_wa(dev) && *alignment < 256 * 1024)
+		*alignment = 256 * 1024;
+
+	return 0;
+}
+
+int intel_pin_fenced_fb_obj(struct drm_device *dev,
+			    struct drm_i915_gem_object *obj,
+			    struct intel_ring_buffer *pipelined)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 alignment;
+	int ret;
+
+	ret = intel_fb_obj_alignment(dev, obj, &alignment);
+	if (ret)
+		return ret;
 
 	dev_priv->mm.interruptible = false;
 	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
@@ -1992,12 +2002,36 @@ err_interruptible:
 	return ret;
 }
 
-void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
+void intel_unpin_fenced_fb_obj(struct drm_i915_gem_object *obj)
 {
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj);
 }
 
+int intel_pin_fb_obj(struct drm_device *dev,
+		     struct drm_i915_gem_object *obj,
+		     struct intel_ring_buffer *pipelined)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 alignment;
+	int ret;
+
+	ret = intel_fb_obj_alignment(dev, obj, &alignment);
+	if (ret)
+		return ret;
+
+	dev_priv->mm.interruptible = false;
+	ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
+
+	dev_priv->mm.interruptible = true;
+	return ret;
+}
+
+void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
+{
+	i915_gem_object_unpin_from_display_plane(obj);
+}
+
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
  * is assumed to be a power-of-two. */
 unsigned long intel_gen4_compute_page_offset(int *x, int *y,
@@ -2350,9 +2384,9 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	}
 
 	mutex_lock(&dev->struct_mutex);
-	ret = intel_pin_and_fence_fb_obj(dev,
-					 to_intel_framebuffer(fb)->obj,
-					 NULL);
+	ret = intel_pin_fenced_fb_obj(dev,
+				      to_intel_framebuffer(fb)->obj,
+				      NULL);
 	if (ret != 0) {
 		mutex_unlock(&dev->struct_mutex);
 		DRM_ERROR("pin & fence failed\n");
@@ -2392,7 +2426,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	ret = dev_priv->display.update_plane(crtc, fb, x, y);
 	if (ret) {
-		intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj);
+		intel_unpin_fenced_fb_obj(to_intel_framebuffer(fb)->obj);
 		mutex_unlock(&dev->struct_mutex);
 		DRM_ERROR("failed to update base address\n");
 		return ret;
@@ -2406,7 +2440,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	if (old_fb) {
 		if (intel_crtc->active && old_fb != fb)
 			intel_wait_for_vblank(dev, intel_crtc->pipe);
-		intel_unpin_fb_obj(to_intel_framebuffer(old_fb)->obj);
+		intel_unpin_fenced_fb_obj(to_intel_framebuffer(old_fb)->obj);
 	}
 
 	intel_update_fbc(dev);
@@ -4364,7 +4398,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 
 	if (crtc->fb) {
 		mutex_lock(&dev->struct_mutex);
-		intel_unpin_fb_obj(to_intel_framebuffer(crtc->fb)->obj);
+		intel_unpin_fenced_fb_obj(to_intel_framebuffer(crtc->fb)->obj);
 		mutex_unlock(&dev->struct_mutex);
 		crtc->fb = NULL;
 	}
@@ -8277,7 +8311,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	struct drm_device *dev = work->crtc->dev;
 
 	mutex_lock(&dev->struct_mutex);
-	intel_unpin_fb_obj(work->old_fb_obj);
+	intel_unpin_fenced_fb_obj(work->old_fb_obj);
 	drm_gem_object_unreference(&work->pending_flip_obj->base);
 	drm_gem_object_unreference(&work->old_fb_obj->base);
 
@@ -8386,7 +8420,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 	int ret;
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
+	ret = intel_pin_fenced_fb_obj(dev, obj, ring);
 	if (ret)
 		goto err;
 
@@ -8414,7 +8448,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
 	return 0;
 
 err_unpin:
-	intel_unpin_fb_obj(obj);
+	intel_unpin_fenced_fb_obj(obj);
 err:
 	return ret;
 }
@@ -8431,7 +8465,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
 	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 	int ret;
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
+	ret = intel_pin_fenced_fb_obj(dev, obj, ring);
 	if (ret)
 		goto err;
 
@@ -8456,7 +8490,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
 	return 0;
 
 err_unpin:
-	intel_unpin_fb_obj(obj);
+	intel_unpin_fenced_fb_obj(obj);
 err:
 	return ret;
 }
@@ -8473,7 +8507,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
 	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
 	int ret;
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
+	ret = intel_pin_fenced_fb_obj(dev, obj, ring);
 	if (ret)
 		goto err;
 
@@ -8505,7 +8539,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
 	return 0;
 
 err_unpin:
-	intel_unpin_fb_obj(obj);
+	intel_unpin_fenced_fb_obj(obj);
 err:
 	return ret;
 }
@@ -8522,7 +8556,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
 	uint32_t pf, pipesrc;
 	int ret;
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
+	ret = intel_pin_fenced_fb_obj(dev, obj, ring);
 	if (ret)
 		goto err;
 
@@ -8550,7 +8584,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
 	return 0;
 
 err_unpin:
-	intel_unpin_fb_obj(obj);
+	intel_unpin_fenced_fb_obj(obj);
 err:
 	return ret;
 }
@@ -8571,7 +8605,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	if (IS_VALLEYVIEW(dev) || ring == NULL || ring->id != RCS)
 		ring = &dev_priv->ring[BCS];
 
-	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
+	ret = intel_pin_fenced_fb_obj(dev, obj, ring);
 	if (ret)
 		goto err;
 
@@ -8630,7 +8664,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
 	return 0;
 
 err_unpin:
-	intel_unpin_fb_obj(obj);
+	intel_unpin_fenced_fb_obj(obj);
 err:
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 44067bc..913fdeb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -677,9 +677,13 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 				struct intel_load_detect_pipe *old);
 void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old);
-int intel_pin_and_fence_fb_obj(struct drm_device *dev,
-			       struct drm_i915_gem_object *obj,
-			       struct intel_ring_buffer *pipelined);
+int intel_pin_fenced_fb_obj(struct drm_device *dev,
+			    struct drm_i915_gem_object *obj,
+			    struct intel_ring_buffer *pipelined);
+void intel_unpin_fenced_fb_obj(struct drm_i915_gem_object *obj);
+int intel_pin_fb_obj(struct drm_device *dev,
+		     struct drm_i915_gem_object *obj,
+		     struct intel_ring_buffer *pipelined);
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj);
 int intel_framebuffer_init(struct drm_device *dev,
 			   struct intel_framebuffer *ifb,
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index d6a8a71..ea5ca6b 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -91,7 +91,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	}
 
 	/* Flush everything out, we'll be doing GTT only from now on */
-	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+	ret = intel_pin_fenced_fb_obj(dev, obj, NULL);
 	if (ret) {
 		DRM_ERROR("failed to pin fb: %d\n", ret);
 		goto out_unref;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..ae316c9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -852,7 +852,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * primary plane requires 256KiB alignment with 64 PTE padding,
 	 * the sprite planes only require 128KiB alignment and 32 PTE padding.
 	 */
-	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+	ret = intel_pin_fb_obj(dev, obj, NULL);
 
 	mutex_unlock(&dev->struct_mutex);
 
-- 
1.8.3.2

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

[PATCH 2/3] drm/i915: Fix fence leaks if fbdev setup fails

[ville.syrjala]

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

We pin and fence the object using intel_pin_fenced_fb_obj() but
just went ahead and unpinned it directly from the gtt. Make the
destruction symmetric by using intel_unpin_fenced_fb_obj().

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

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index ea5ca6b..51b41c5 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -104,7 +104,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	return 0;
 
 out_unpin:
-	i915_gem_object_ggtt_unpin(obj);
+	intel_unpin_fenced_fb_obj(obj);
 out_unref:
 	drm_gem_object_unreference(&obj->base);
 out:
@@ -208,7 +208,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	return 0;
 
 out_unpin:
-	i915_gem_object_ggtt_unpin(obj);
+	intel_unpin_fenced_fb_obj(obj);
 	drm_gem_object_unreference(&obj->base);
 out_unlock:
 	mutex_unlock(&dev->struct_mutex);
-- 
1.8.3.2

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

[PATCH 3/3] drm/i915: Don't waste fences for scanout when not needed

[ville.syrjala]

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

Gen2 and gen3 require fences for tiled scanout, but gen4+ only require a
fence for FBC GTT modification tracking. Install a fence for scanout
only when it might actually be required.

This could potentially be optimized even further, but this seems like
a fair comporomise for now.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fd48c2e..71dd041 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1972,6 +1972,13 @@ int intel_pin_fenced_fb_obj(struct drm_device *dev,
 	u32 alignment;
 	int ret;
 
+	/*
+	 * Pre-i965 always needs a fence for tiled scanout, whereas
+	 * 965+ only requires a fence if using framebuffer compression.
+	 */
+	if (INTEL_INFO(dev)->gen >= 4 && !HAS_FBC(dev))
+		return intel_pin_fb_obj(dev, obj, pipelined);
+
 	ret = intel_fb_obj_alignment(dev, obj, &alignment);
 	if (ret)
 		return ret;
@@ -1981,11 +1988,6 @@ int intel_pin_fenced_fb_obj(struct drm_device *dev,
 	if (ret)
 		goto err_interruptible;
 
-	/* Install a fence for tiled scan-out. Pre-i965 always needs a
-	 * fence, whereas 965+ only requires a fence if using
-	 * framebuffer compression.  For simplicity, we always install
-	 * a fence as the cost is not that onerous.
-	 */
 	ret = i915_gem_object_get_fence(obj);
 	if (ret)
 		goto err_unpin;
@@ -2004,6 +2006,13 @@ err_interruptible:
 
 void intel_unpin_fenced_fb_obj(struct drm_i915_gem_object *obj)
 {
+	struct drm_device *dev = obj->base.dev;
+
+	if (INTEL_INFO(dev)->gen >= 4 && !HAS_FBC(dev)) {
+		intel_unpin_fb_obj(obj);
+		return;
+	}
+
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj);
 }
-- 
1.8.3.2

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

Re: [PATCH 0/3] drm/i915: Reduce fence usage for scanout

[Chris Wilson]

On Tue, Jan 28, 2014 at 02:51:53PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> After staring at the scanout vs. fences situation a bit, I decided
> to reduce our scanout fence usage a bit.

I feel that is a glorious overreaction to a bug in another piece of
code, but now that you've done it, I can not complain. ;-)
 
> Ville Syrjälä (3):
>   drm/i915: Don't waste fences on sprite buffers

I suspect this is a little too generic and that there will be some
freaky old piece of hardware that requires a fence.

However, for the gen that currently use sprites, it should just work I
think. So which i-g-t hits tiled sprites?

>   drm/i915: Fix fence leaks if fbdev setup fails

If fbdev ever uses a tiled fb, I think we are in greater trouble. So
other than the leak is only a theorectical one, the patch looks sane.

>   drm/i915: Don't waste fences for scanout when not needed

But it's gen2/3 that is short of fences (and we've a... I feel this is a
micro-optimisation, but now that it is done...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 0/3] drm/i915: Reduce fence usage for scanout

[Ville Syrjälä]

On Tue, Jan 28, 2014 at 01:12:44PM +0000, Chris Wilson wrote:
> On Tue, Jan 28, 2014 at 02:51:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > After staring at the scanout vs. fences situation a bit, I decided
> > to reduce our scanout fence usage a bit.
> 
> I feel that is a glorious overreaction to a bug in another piece of
> code, but now that you've done it, I can not complain. ;-)
>  
> > Ville Syrjälä (3):
> >   drm/i915: Don't waste fences on sprite buffers
> 
> I suspect this is a little too generic and that there will be some
> freaky old piece of hardware that requires a fence.

I don't see a problem making such changes if and when needed. But if
you prefer I could adjust the code a bit to have one pin function for
primary planes (will check gen+FBC) and another for sprite/overlay
planes (will only check gen).

I suppose the current sprite code should work OKish for sprite B/C
on gen2/3, but for the video overlay the code would need more changes.
The hardware is more capable than the current code, and I'd hate not
taking full advantage of it.

> 
> However, for the gen that currently use sprites, it should just work I
> think. So which i-g-t hits tiled sprites?

There are none. I've been waiting for someone to write some sprite i-g-ts.
I do have my glplane thingy however which uses tiled buffers via gbm, and
it works equally well w/ and w/o fences.

> 
> >   drm/i915: Fix fence leaks if fbdev setup fails
> 
> If fbdev ever uses a tiled fb, I think we are in greater trouble. So
> other than the leak is only a theorectical one, the patch looks sane.

Hmm. Already forgot about that. Might be nice to make it tiled and
enable FBC for it :)

> >   drm/i915: Don't waste fences for scanout when not needed
> 
> But it's gen2/3 that is short of fences (and we've a... I feel this is a
> micro-optimisation, but now that it is done...

Yeah well, this will only "help" gen4 desktop parts and VLV since
everything else has FBC AFAIK.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/3] drm/i915: Reduce fence usage for scanout

[Daniel Vetter]

On Tue, Jan 28, 2014 at 03:51:06PM +0200, Ville Syrjälä wrote:
> On Tue, Jan 28, 2014 at 01:12:44PM +0000, Chris Wilson wrote:
> > On Tue, Jan 28, 2014 at 02:51:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > >   drm/i915: Don't waste fences for scanout when not needed
> > 
> > But it's gen2/3 that is short of fences (and we've a... I feel this is a
> > micro-optimisation, but now that it is done...
> 
> Yeah well, this will only "help" gen4 desktop parts and VLV since
> everything else has FBC AFAIK.

tbh I was afraid a bit that this will reduce testing coverage for that
rather nifty fence accounting bug in ppgtt. But it looks like we'll still
be ok.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch