[PATCH 0/9] drm/i915: Some more FBC stuff

[PATCH 0/9] drm/i915: Some more FBC stuff

[ville.syrjala]

Another set of FBC patches, which should fit on top of the previous set:
"[PATCH 00/10] drm/i915: FBC fixes v2"

The persistent mode and HT tracking bit stuff is a bit unclear in the docs,
but I can remove it all, and everything still seems to work fine.

The page flip and dirtyfb stuff is maybe a bit raw, but I'll post anyway
now since it seems to work for me.

I'll post my igt test case that tries to stress all this shortly. It passes
for me on ILK, SNB and IVB. On ILK it's a bit limited since there are no
contexts (didn't try the ILK context patches w/ this) and we're missing
a gen5 rendercopy, so I couldn't test the render tracking using igt. But I
don't get any screen corruption w/ FBC enabled, so it must be working.

The only FBC1 capable hardware on my desk is a MGM, but someone was a bit
too conservative when they implemented FBC1 support and enabled it only 
for CL. I was too lazy to read through the code to see if it should work
for MGM.

Ville Syrjälä (9):
      drm/i915: Don't set the fence number in DPFC_CTL on SNB
      drm/i915: Don't set persistent FBC mode on ILK/SNB
      drm/i915: Don't set DPFC_HT_MODIFY bit on CTG/ILK/SNB
      drm/i915: Use LRI based FBC render tracking for ILK
      drm/i915: Reorder i915_gem_execbuffer_move_to_gpu() and i915_switch_context()
      drm/i915: Improve page flip vs. FBC interaction
      drm: Push dirtyfb ioctl kms locking down to drivers
      drm/i915: Hook up dirtyfb ioctl for FBC nuke
      drm/i915: Flush caches for scanout during cpu->gtt move

 drivers/gpu/drm/drm_crtc.c                 |   2 -
 drivers/gpu/drm/i915/i915_gem.c            |   2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   8 +-
 drivers/gpu/drm/i915/i915_reg.h            |   2 +-
 drivers/gpu/drm/i915/intel_display.c       |  24 ++++-
 drivers/gpu/drm/i915/intel_drv.h           |   5 ++
 drivers/gpu/drm/i915/intel_pm.c            | 136 +++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.c    |  57 ++++++------
 drivers/gpu/drm/omapdrm/omap_fb.c          |   4 +
 drivers/gpu/drm/qxl/qxl_display.c          |   9 +-
 drivers/gpu/drm/udl/udl_fb.c               |  12 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c        |  18 +++-
 12 files changed, 228 insertions(+), 51 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/9] drm/i915: Don't set the fence number in DPFC_CTL on SNB

[ville.syrjala]

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

SNB has another register where the actual FBC CPU fence number is
stored. The documenation explicitly states that the fence number
in DPFC_CTL must be 0 on SNB. And in fact when it's not zero,
the GTT tracking simply doesn't work.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 53ba1a2..c9d1b0c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -205,7 +205,9 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
 	dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
 	/* Set persistent mode for front-buffer rendering, ala X. */
 	dpfc_ctl |= DPFC_CTL_PERSISTENT_MODE;
-	dpfc_ctl |= (DPFC_CTL_FENCE_EN | obj->fence_reg);
+	dpfc_ctl |= DPFC_CTL_FENCE_EN;
+	if (IS_GEN5(dev))
+		dpfc_ctl |= obj->fence_reg;
 	I915_WRITE(ILK_DPFC_CHICKEN, DPFC_HT_MODIFY);
 
 	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
-- 
1.8.3.2

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

Re: [PATCH 1/9] drm/i915: Don't set the fence number in DPFC_CTL on SNB

[Chris Wilson]

On Thu, Nov 21, 2013 at 09:29:45PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> SNB has another register where the actual FBC CPU fence number is
> stored. The documenation explicitly states that the fence number
> in DPFC_CTL must be 0 on SNB. And in fact when it's not zero,
> the GTT tracking simply doesn't work.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/9] drm/i915: Don't set the fence number in DPFC_CTL on SNB

[Daniel Vetter]

On Thu, Nov 21, 2013 at 11:22:14PM +0000, Chris Wilson wrote:
> On Thu, Nov 21, 2013 at 09:29:45PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > SNB has another register where the actual FBC CPU fence number is
> > stored. The documenation explicitly states that the fence number
> > in DPFC_CTL must be 0 on SNB. And in fact when it's not zero,
> > the GTT tracking simply doesn't work.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 2/9] drm/i915: Don't set persistent FBC mode on ILK/SNB

[ville.syrjala]

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

The ILK/SNB docs are a bit unclear what the persistent mode does, but
the CTG docs clearly state that it was meant to be used when we're
tracking back buffer modifications. We never do that, so leave it in
non-persistent mode.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c9d1b0c..8cea92e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -203,8 +203,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
 	dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
 	dpfc_ctl &= DPFC_RESERVED;
 	dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
-	/* Set persistent mode for front-buffer rendering, ala X. */
-	dpfc_ctl |= DPFC_CTL_PERSISTENT_MODE;
 	dpfc_ctl |= DPFC_CTL_FENCE_EN;
 	if (IS_GEN5(dev))
 		dpfc_ctl |= obj->fence_reg;
-- 
1.8.3.2

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

[PATCH 3/9] drm/i915: Don't set DPFC_HT_MODIFY bit on CTG/ILK/SNB

[ville.syrjala]

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

The ILK/SNB docs don't really mention the the DPFC_HT_MODIFY bit.
CTG docs clearly state that it should be set only when tracking
back buffer modification in persistent mode. The bit is supposed
to be set by software after the first CPU modification to the
back buffer, and it would get automagically cleared by the hardware
on the next page flip.

Since we only track front buffer modification we don't need to set
this bit. GTT modification tracking still appears to work on ILK
and SNB with the bit unset. I don't have a CTG to verify how that
behaves.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8cea92e..f46bb56 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -152,7 +152,6 @@ static void g4x_enable_fbc(struct drm_crtc *crtc,
 
 	dpfc_ctl = plane | DPFC_SR_EN | DPFC_CTL_LIMIT_1X;
 	dpfc_ctl |= DPFC_CTL_FENCE_EN | obj->fence_reg;
-	I915_WRITE(DPFC_CHICKEN, DPFC_HT_MODIFY);
 
 	I915_WRITE(DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
 		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
@@ -206,7 +205,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
 	dpfc_ctl |= DPFC_CTL_FENCE_EN;
 	if (IS_GEN5(dev))
 		dpfc_ctl |= obj->fence_reg;
-	I915_WRITE(ILK_DPFC_CHICKEN, DPFC_HT_MODIFY);
 
 	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
 		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
-- 
1.8.3.2

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

[PATCH 4/9] drm/i915: Use LRI based FBC render tracking for ILK

[ville.syrjala]

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

ILK should work pretty much the same as SNB, except it
doesn't have the blitter, so we only care about render tracking.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         |  2 +-
 drivers/gpu/drm/i915/intel_pm.c         |  2 --
 drivers/gpu/drm/i915/intel_ringbuffer.c | 57 +++++++++++++++++----------------
 3 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1777beb..db532cf 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1091,7 +1091,7 @@
 #define ILK_DPFC_CHICKEN	0x43224
 #define ILK_FBC_RT_BASE		0x2128
 #define   ILK_FBC_RT_VALID	(1<<0)
-#define   SNB_FBC_FRONT_BUFFER	(1<<1)
+#define   ILK_FBC_FRONT_BUFFER	(1<<1)
 
 #define ILK_DISPLAY_CHICKEN1	0x42000
 #define   ILK_FBCQ_DIS		(1<<22)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f46bb56..4039aa3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -210,8 +210,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
 		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
 		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
 	I915_WRITE(ILK_DPFC_FENCE_YOFF, crtc->y);
-	if (IS_GEN5(dev))
-		I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
 	/* enable it... */
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2e62d76..a02d472 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -77,6 +77,32 @@ gen2_render_ring_flush(struct intel_ring_buffer *ring,
 	return 0;
 }
 
+static int gen5_render_fbc_tracking(struct intel_ring_buffer *ring)
+{
+	int ret;
+
+	if (!ring->fbc_address_dirty)
+		return 0;
+
+	ret = intel_ring_begin(ring, 4);
+	if (ret)
+		return ret;
+
+	intel_ring_emit(ring, MI_NOOP);
+	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+	intel_ring_emit(ring, ILK_FBC_RT_BASE);
+	if (ring->fbc_address != -1)
+		intel_ring_emit(ring, ring->fbc_address |
+				ILK_FBC_FRONT_BUFFER | ILK_FBC_RT_VALID);
+	else
+		intel_ring_emit(ring, 0);
+	intel_ring_advance(ring);
+
+	ring->fbc_address_dirty = false;
+
+	return 0;
+}
+
 static int
 gen4_render_ring_flush(struct intel_ring_buffer *ring,
 		       u32	invalidate_domains,
@@ -132,6 +158,9 @@ gen4_render_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
 
+	if (invalidate_domains && IS_GEN5(dev))
+		return gen5_render_fbc_tracking(ring);
+
 	return 0;
 }
 
@@ -232,32 +261,6 @@ static int gen6_blt_fbc_tracking(struct intel_ring_buffer *ring)
 	return 0;
 }
 
-static int gen6_render_fbc_tracking(struct intel_ring_buffer *ring)
-{
-	int ret;
-
-	if (!ring->fbc_address_dirty)
-		return 0;
-
-	ret = intel_ring_begin(ring, 4);
-	if (ret)
-		return ret;
-
-	intel_ring_emit(ring, MI_NOOP);
-	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-	intel_ring_emit(ring, ILK_FBC_RT_BASE);
-	if (ring->fbc_address != -1)
-		intel_ring_emit(ring, ring->fbc_address |
-				SNB_FBC_FRONT_BUFFER | ILK_FBC_RT_VALID);
-	else
-		intel_ring_emit(ring, 0);
-	intel_ring_advance(ring);
-
-	ring->fbc_address_dirty = false;
-
-	return 0;
-}
-
 static int
 gen6_render_ring_flush(struct intel_ring_buffer *ring,
                          u32 invalidate_domains, u32 flush_domains)
@@ -308,7 +311,7 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_advance(ring);
 
 	if (invalidate_domains)
-		return gen6_render_fbc_tracking(ring);
+		return gen5_render_fbc_tracking(ring);
 
 	return 0;
 }
-- 
1.8.3.2

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

[PATCH v2 4/9] drm/i915: Use LRI based FBC render tracking for ILK

[ville.syrjala]

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

ILK should work pretty much the same as SNB, except it
doesn't have the blitter, so we only care about render tracking.

v2: Rebased against earlier changes

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 2 +-
 drivers/gpu/drm/i915/intel_pm.c         | 2 --
 drivers/gpu/drm/i915/intel_ringbuffer.c | 9 ++++++---
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f2104f5..6de26eb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1091,7 +1091,7 @@
 #define ILK_DPFC_CHICKEN	0x43224
 #define ILK_FBC_RT_BASE		0x2128
 #define   ILK_FBC_RT_VALID	(1<<0)
-#define   SNB_FBC_FRONT_BUFFER	(1<<1)
+#define   ILK_FBC_FRONT_BUFFER	(1<<1)
 
 #define ILK_DISPLAY_CHICKEN1	0x42000
 #define   ILK_FBCQ_DIS		(1<<22)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 059245e..d0f554e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -214,8 +214,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
 		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
 		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
 	I915_WRITE(ILK_DPFC_FENCE_YOFF, crtc->y);
-	if (IS_GEN5(dev))
-		I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
 	/* enable it... */
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 426d868..c0db05e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -51,7 +51,7 @@ void __intel_ring_advance(struct intel_ring_buffer *ring)
 	ring->write_tail(ring, ring->tail);
 }
 
-static int gen6_render_fbc_tracking(struct intel_ring_buffer *ring)
+static int gen5_render_fbc_tracking(struct intel_ring_buffer *ring)
 {
 	int ret;
 
@@ -67,7 +67,7 @@ static int gen6_render_fbc_tracking(struct intel_ring_buffer *ring)
 	intel_ring_emit(ring, ILK_FBC_RT_BASE);
 	if (ring->fbc_address != -1)
 		intel_ring_emit(ring, ring->fbc_address |
-				SNB_FBC_FRONT_BUFFER | ILK_FBC_RT_VALID);
+				ILK_FBC_FRONT_BUFFER | ILK_FBC_RT_VALID);
 	else
 		intel_ring_emit(ring, 0);
 	intel_ring_advance(ring);
@@ -183,6 +183,9 @@ gen4_render_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
 
+	if (invalidate_domains && IS_GEN5(dev))
+		return gen5_render_fbc_tracking(ring);
+
 	return 0;
 }
 
@@ -308,7 +311,7 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_advance(ring);
 
 	if (invalidate_domains)
-		return gen6_render_fbc_tracking(ring);
+		return gen5_render_fbc_tracking(ring);
 
 	return 0;
 }
-- 
1.8.3.2

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

Re: [PATCH v2 4/9] drm/i915: Use LRI based FBC render tracking for ILK

[Chris Wilson]

On Wed, Nov 27, 2013 at 05:24:45PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> ILK should work pretty much the same as SNB, except it
> doesn't have the blitter, so we only care about render tracking.
> 
> v2: Rebased against earlier changes
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 5/9] drm/i915: Reorder i915_gem_execbuffer_move_to_gpu() and i915_switch_context()

[ville.syrjala]

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

The FBC RT address is stored in the context, and thus needs to be
rewritten after a context switch before any batches are run. We emit
the LRI to update the FBC RT address when we call the ring ->flush
function to invalidate the caches.

When a context switch is being performed we currently do the
invalidate before the context switch, which means we're not updating
the FBC RT address correctly. Move the i915_gem_execbuffer_move_to_gpu()
call to happen after i915_switch_context() to make the LRI happen
after the MI_SET_CONTEXT.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2d96edf..7389e7a 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1181,10 +1181,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	i915_gem_execbuffer_mark_fbc_dirty(ring, &eb->vmas);
 
-	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
-	if (ret)
-		goto err;
-
 	hs = i915_gem_context_get_hang_stats(dev, file, ctx_id);
 	if (IS_ERR(hs)) {
 		ret = PTR_ERR(hs);
@@ -1200,6 +1196,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (ret)
 		goto err;
 
+	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
+	if (ret)
+		goto err;
+
 	if (ring == &dev_priv->ring[RCS] &&
 	    mode != dev_priv->relative_constants_mode) {
 		ret = intel_ring_begin(ring, 4);
-- 
1.8.3.2

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

[PATCH 6/9] drm/i915: Improve page flip vs. FBC interaction

[ville.syrjala]

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

On FBC2 (CTG+) page flips will automagically nuke FBC, so the only thing
we need to do on page flip is update the CPU fence information.

On FBC1 we need to to disable+re-enable FBC around page flips. Previously
we just called intel_disable_fbc() + intel_update_fbc() to do that.
However intel_update_fbc() would actually need to grab all the modeset
locks, which we can't do from the driver workqueue. Make things a bit
more straightforward by simply disabling FBC only if it's active/scheduled
to be active on the current CRTC, and restart it on the same CRTC
after the flip.

FIXME: Several FBC1 docs actually seem to indicate that sync flips will
also cause a nuke on FBC1. Need to verify this.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  5 +-
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++
 drivers/gpu/drm/i915/intel_pm.c      | 96 +++++++++++++++++++++++++++++++++++-
 3 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4155814..af0eafe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8222,7 +8222,7 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	drm_gem_object_unreference(&work->pending_flip_obj->base);
 	drm_gem_object_unreference(&work->old_fb_obj->base);
 
-	intel_update_fbc(dev);
+	intel_fbc_flip_end(work->crtc, work->crtc->fb);
 	mutex_unlock(&dev->struct_mutex);
 
 	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
@@ -8661,7 +8661,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_pending;
 
-	intel_disable_fbc(dev);
+	intel_fbc_flip_start(crtc, fb);
+
 	intel_mark_fb_busy(obj, NULL);
 	mutex_unlock(&dev->struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 119bb95..f9e9ca0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -836,6 +836,10 @@ void intel_update_sprite_watermarks(struct drm_plane *plane,
 				    bool enabled, bool scaled);
 void intel_init_pm(struct drm_device *dev);
 bool intel_fbc_enabled(struct drm_device *dev);
+void intel_fbc_flip_start(struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb);
+void intel_fbc_flip_end(struct drm_crtc *crtc,
+			struct drm_framebuffer *fb);
 void intel_update_fbc(struct drm_device *dev);
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 void intel_gpu_ips_teardown(void);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4039aa3..2a613ac 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -363,6 +363,8 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	if (!dev_priv->display.enable_fbc)
 		return;
 
+	WARN_ON(dev_priv->fbc.fbc_work && dev_priv->fbc.plane != -1);
+
 	intel_cancel_fbc_work(dev_priv);
 
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
@@ -401,6 +403,8 @@ void intel_disable_fbc(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	WARN_ON(dev_priv->fbc.fbc_work && dev_priv->fbc.plane != -1);
+
 	intel_cancel_fbc_work(dev_priv);
 
 	if (!dev_priv->display.disable_fbc)
@@ -414,6 +418,96 @@ void intel_disable_fbc(struct drm_device *dev)
 	}
 }
 
+static void intel_update_fbc_fb(struct drm_crtc *crtc,
+				struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_fbc_work *work = dev_priv->fbc.fbc_work;
+
+	/*
+	 * If we've just scheduled an fbc work,
+	 * simply update the fb for the work.
+	 */
+	if (work) {
+		WARN_ON(dev_priv->fbc.plane != -1);
+
+		if (work->crtc != crtc)
+			return;
+
+		drm_framebuffer_reference(fb);
+		drm_framebuffer_unreference(work->fb);
+		work->fb = fb;
+	}
+
+	if (dev_priv->fbc.plane == to_intel_crtc(crtc)->plane) {
+		WARN_ON(dev_priv->fbc.fbc_work);
+
+		if (dev_priv->fbc.fb == fb &&
+		    dev_priv->fbc.y == crtc->y)
+			return;
+
+		intel_setup_fbc(crtc, fb, 500);
+	}
+}
+
+void intel_fbc_flip_start(struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_fbc_work *work = dev_priv->fbc.fbc_work;
+
+	if (!I915_HAS_FBC(dev))
+		return;
+
+	WARN_ON(work && dev_priv->fbc.plane != -1);
+
+	/* just need to update the CPU fence on FBC2 */
+	if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
+		intel_update_fbc_fb(crtc, fb);
+		return;
+	}
+
+	if (work && work->crtc == crtc) {
+		intel_cancel_fbc_work(dev_priv);
+
+		/*
+		 * set fbc.plane so that we can re-enable
+		 * FBC with just intel_fbc_flip_end().
+		 */
+		dev_priv->fbc.plane = to_intel_crtc(crtc)->plane;
+		return;
+	}
+
+	if (dev_priv->fbc.plane == to_intel_crtc(crtc)->plane) {
+		dev_priv->display.disable_fbc(dev);
+
+		/*
+		 * don't clear fbc.plane so that we can
+		 * re-enable FBC with just intel_fbc_flip_end().
+		 */
+	}
+}
+
+void intel_fbc_flip_end(struct drm_crtc *crtc,
+			struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!I915_HAS_FBC(dev))
+		return;
+
+	WARN_ON(dev_priv->fbc.fbc_work && dev_priv->fbc.plane != -1);
+
+	/* nothing to do on FBC2 */
+	if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5)
+		return;
+
+	intel_update_fbc_fb(crtc, fb);
+}
+
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      enum no_fbc_reason reason)
 {
@@ -567,7 +661,7 @@ void intel_update_fbc(struct drm_device *dev)
 	    dev_priv->fbc.y == crtc->y)
 		return;
 
-	if (intel_fbc_enabled(dev)) {
+	if (intel_fbc_enabled(dev) || dev_priv->fbc.plane != -1) {
 		/* We update FBC along two paths, after changing fb/crtc
 		 * configuration (modeswitching) and after page-flipping
 		 * finishes. For the latter, we know that not only did
-- 
1.8.3.2

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

[PATCH 7/9] drm: Push dirtyfb ioctl kms locking down to drivers

[ville.syrjala]

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

Not all drivers will need take all the modeset locks for dirtyfb, so
push the locking down to the drivers.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc.c          |  2 --
 drivers/gpu/drm/omapdrm/omap_fb.c   |  4 ++++
 drivers/gpu/drm/qxl/qxl_display.c   |  9 ++++++++-
 drivers/gpu/drm/udl/udl_fb.c        | 12 +++++++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 18 ++++++++++++++++--
 5 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d6cf77c..266a01d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2767,10 +2767,8 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
 	}
 
 	if (fb->funcs->dirty) {
-		drm_modeset_lock_all(dev);
 		ret = fb->funcs->dirty(fb, file_priv, flags, r->color,
 				       clips, num_clips);
-		drm_modeset_unlock_all(dev);
 	} else {
 		ret = -ENOSYS;
 	}
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index f2b8f06..f466c4a 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -123,12 +123,16 @@ static int omap_framebuffer_dirty(struct drm_framebuffer *fb,
 {
 	int i;
 
+	drm_modeset_lock_all(fb->dev);
+
 	for (i = 0; i < num_clips; i++) {
 		omap_framebuffer_flush(fb, clips[i].x1, clips[i].y1,
 					clips[i].x2 - clips[i].x1,
 					clips[i].y2 - clips[i].y1);
 	}
 
+	drm_modeset_unlock_all(fb->dev);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 5e827c2..b8f3bc7 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -399,10 +399,14 @@ static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
 	struct qxl_bo *qobj;
 	int inc = 1;
 
+	drm_modeset_lock_all(fb->dev);
+
 	qobj = gem_to_qxl_bo(qxl_fb->obj);
 	/* if we aren't primary surface ignore this */
-	if (!qobj->is_primary)
+	if (!qobj->is_primary) {
+		drm_modeset_unlock_all(fb->dev);
 		return 0;
+	}
 
 	if (!num_clips) {
 		num_clips = 1;
@@ -417,6 +421,9 @@ static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
 
 	qxl_draw_dirty_fb(qdev, qxl_fb, qobj, flags, color,
 			  clips, num_clips, inc);
+
+	drm_modeset_unlock_all(fb->dev);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 97e9d61..dbadd49 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -403,15 +403,17 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
 	int i;
 	int ret = 0;
 
+	drm_modeset_lock_all(fb->dev);
+
 	if (!ufb->active_16)
-		return 0;
+		goto unlock;
 
 	if (ufb->obj->base.import_attach) {
 		ret = dma_buf_begin_cpu_access(ufb->obj->base.import_attach->dmabuf,
 					       0, ufb->obj->base.size,
 					       DMA_FROM_DEVICE);
 		if (ret)
-			return ret;
+			goto unlock;
 	}
 
 	for (i = 0; i < num_clips; i++) {
@@ -419,7 +421,7 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
 				  clips[i].x2 - clips[i].x1,
 				  clips[i].y2 - clips[i].y1);
 		if (ret)
-			break;
+			goto unlock;
 	}
 
 	if (ufb->obj->base.import_attach) {
@@ -427,6 +429,10 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
 				       0, ufb->obj->base.size,
 				       DMA_FROM_DEVICE);
 	}
+
+ unlock:
+	drm_modeset_unlock_all(fb->dev);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index ecb3d86..ab0b88f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -608,9 +608,13 @@ int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,
 	if (!dev_priv->sou_priv)
 		return -EINVAL;
 
+	drm_modeset_lock_all(dev_priv->dev);
+
 	ret = ttm_read_lock(&vmaster->lock, true);
-	if (unlikely(ret != 0))
+	if (unlikely(ret != 0)) {
+		drm_modeset_unlock_all(dev_priv->dev);
 		return ret;
+	}
 
 	if (!num_clips) {
 		num_clips = 1;
@@ -628,6 +632,9 @@ int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,
 				   clips, num_clips, inc, NULL);
 
 	ttm_read_unlock(&vmaster->lock);
+
+	drm_modeset_unlock_all(dev_priv->dev);
+
 	return 0;
 }
 
@@ -952,9 +959,13 @@ int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer,
 	struct drm_clip_rect norect;
 	int ret, increment = 1;
 
+	drm_modeset_lock_all(dev_priv->dev);
+
 	ret = ttm_read_lock(&vmaster->lock, true);
-	if (unlikely(ret != 0))
+	if (unlikely(ret != 0)) {
+		drm_modeset_unlock_all(dev_priv->dev);
 		return ret;
+	}
 
 	if (!num_clips) {
 		num_clips = 1;
@@ -978,6 +989,9 @@ int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer,
 	}
 
 	ttm_read_unlock(&vmaster->lock);
+
+	drm_modeset_unlock_all(dev_priv->dev);
+
 	return ret;
 }
 
-- 
1.8.3.2

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

Re: [PATCH 7/9] drm: Push dirtyfb ioctl kms locking down to drivers

[Daniel Vetter]

On Thu, Nov 21, 2013 at 09:29:51PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Not all drivers will need take all the modeset locks for dirtyfb, so
> push the locking down to the drivers.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yeah, makes tons of sense and has definitely step 0 on my list to make
dirtyfb useful for i915.ko.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc.c          |  2 --
>  drivers/gpu/drm/omapdrm/omap_fb.c   |  4 ++++
>  drivers/gpu/drm/qxl/qxl_display.c   |  9 ++++++++-
>  drivers/gpu/drm/udl/udl_fb.c        | 12 +++++++++---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 18 ++++++++++++++++--
>  5 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d6cf77c..266a01d 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2767,10 +2767,8 @@ int drm_mode_dirtyfb_ioctl(struct drm_device *dev,
>  	}
>  
>  	if (fb->funcs->dirty) {
> -		drm_modeset_lock_all(dev);
>  		ret = fb->funcs->dirty(fb, file_priv, flags, r->color,
>  				       clips, num_clips);
> -		drm_modeset_unlock_all(dev);
>  	} else {
>  		ret = -ENOSYS;
>  	}
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index f2b8f06..f466c4a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -123,12 +123,16 @@ static int omap_framebuffer_dirty(struct drm_framebuffer *fb,
>  {
>  	int i;
>  
> +	drm_modeset_lock_all(fb->dev);
> +
>  	for (i = 0; i < num_clips; i++) {
>  		omap_framebuffer_flush(fb, clips[i].x1, clips[i].y1,
>  					clips[i].x2 - clips[i].x1,
>  					clips[i].y2 - clips[i].y1);
>  	}
>  
> +	drm_modeset_unlock_all(fb->dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 5e827c2..b8f3bc7 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -399,10 +399,14 @@ static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
>  	struct qxl_bo *qobj;
>  	int inc = 1;
>  
> +	drm_modeset_lock_all(fb->dev);
> +
>  	qobj = gem_to_qxl_bo(qxl_fb->obj);
>  	/* if we aren't primary surface ignore this */
> -	if (!qobj->is_primary)
> +	if (!qobj->is_primary) {
> +		drm_modeset_unlock_all(fb->dev);
>  		return 0;
> +	}
>  
>  	if (!num_clips) {
>  		num_clips = 1;
> @@ -417,6 +421,9 @@ static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
>  
>  	qxl_draw_dirty_fb(qdev, qxl_fb, qobj, flags, color,
>  			  clips, num_clips, inc);
> +
> +	drm_modeset_unlock_all(fb->dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index 97e9d61..dbadd49 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -403,15 +403,17 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
>  	int i;
>  	int ret = 0;
>  
> +	drm_modeset_lock_all(fb->dev);
> +
>  	if (!ufb->active_16)
> -		return 0;
> +		goto unlock;
>  
>  	if (ufb->obj->base.import_attach) {
>  		ret = dma_buf_begin_cpu_access(ufb->obj->base.import_attach->dmabuf,
>  					       0, ufb->obj->base.size,
>  					       DMA_FROM_DEVICE);
>  		if (ret)
> -			return ret;
> +			goto unlock;
>  	}
>  
>  	for (i = 0; i < num_clips; i++) {
> @@ -419,7 +421,7 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
>  				  clips[i].x2 - clips[i].x1,
>  				  clips[i].y2 - clips[i].y1);
>  		if (ret)
> -			break;
> +			goto unlock;
>  	}
>  
>  	if (ufb->obj->base.import_attach) {
> @@ -427,6 +429,10 @@ static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
>  				       0, ufb->obj->base.size,
>  				       DMA_FROM_DEVICE);
>  	}
> +
> + unlock:
> +	drm_modeset_unlock_all(fb->dev);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index ecb3d86..ab0b88f 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -608,9 +608,13 @@ int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,
>  	if (!dev_priv->sou_priv)
>  		return -EINVAL;
>  
> +	drm_modeset_lock_all(dev_priv->dev);
> +
>  	ret = ttm_read_lock(&vmaster->lock, true);
> -	if (unlikely(ret != 0))
> +	if (unlikely(ret != 0)) {
> +		drm_modeset_unlock_all(dev_priv->dev);
>  		return ret;
> +	}
>  
>  	if (!num_clips) {
>  		num_clips = 1;
> @@ -628,6 +632,9 @@ int vmw_framebuffer_surface_dirty(struct drm_framebuffer *framebuffer,
>  				   clips, num_clips, inc, NULL);
>  
>  	ttm_read_unlock(&vmaster->lock);
> +
> +	drm_modeset_unlock_all(dev_priv->dev);
> +
>  	return 0;
>  }
>  
> @@ -952,9 +959,13 @@ int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer,
>  	struct drm_clip_rect norect;
>  	int ret, increment = 1;
>  
> +	drm_modeset_lock_all(dev_priv->dev);
> +
>  	ret = ttm_read_lock(&vmaster->lock, true);
> -	if (unlikely(ret != 0))
> +	if (unlikely(ret != 0)) {
> +		drm_modeset_unlock_all(dev_priv->dev);
>  		return ret;
> +	}
>  
>  	if (!num_clips) {
>  		num_clips = 1;
> @@ -978,6 +989,9 @@ int vmw_framebuffer_dmabuf_dirty(struct drm_framebuffer *framebuffer,
>  	}
>  
>  	ttm_read_unlock(&vmaster->lock);
> +
> +	drm_modeset_unlock_all(dev_priv->dev);
> +
>  	return ret;
>  }
>  
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 8/9] drm/i915: Hook up dirtyfb ioctl for FBC nuke

[ville.syrjala]

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

FBC host modification tracking only works through GTT mmaps, so any
direct CPU access needs to manually nuke the compressed framebuffer
on modifications. Hook up the dirtyfb ioctl to do just that.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_pm.c      | 30 ++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af0eafe..e5de929 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10385,8 +10385,27 @@ static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
 	return drm_gem_handle_create(file, &obj->base, handle);
 }
 
+static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
+					struct drm_file *file_priv,
+					unsigned flags, unsigned color,
+					struct drm_clip_rect *clips,
+					unsigned num_clips)
+{
+	struct drm_device *dev = fb->dev;
+
+	if (!I915_HAS_FBC(dev))
+		return 0;
+
+	mutex_lock(&dev->struct_mutex);
+	intel_fbc_nuke(fb);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
 static const struct drm_framebuffer_funcs intel_fb_funcs = {
 	.destroy = intel_user_framebuffer_destroy,
+	.dirty = intel_user_framebuffer_dirty,
 	.create_handle = intel_user_framebuffer_create_handle,
 };
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f9e9ca0..c11ef8a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -840,6 +840,7 @@ void intel_fbc_flip_start(struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb);
 void intel_fbc_flip_end(struct drm_crtc *crtc,
 			struct drm_framebuffer *fb);
+void intel_fbc_nuke(struct drm_framebuffer *fb);
 void intel_update_fbc(struct drm_device *dev);
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 void intel_gpu_ips_teardown(void);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2a613ac..d84630a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -508,6 +508,36 @@ void intel_fbc_flip_end(struct drm_crtc *crtc,
 	intel_update_fbc_fb(crtc, fb);
 }
 
+void intel_fbc_nuke(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+
+	if (dev_priv->fbc.fbc_work) {
+		if (dev_priv->fbc.fbc_work->fb != fb)
+			return;
+
+		crtc = dev_priv->fbc.fbc_work->crtc;
+	} else {
+		if (dev_priv->fbc.fb != fb)
+			return;
+
+		if (WARN_ON(dev_priv->fbc.plane < 0))
+			return;
+
+		crtc = dev_priv->plane_to_crtc_mapping[dev_priv->fbc.plane];
+	}
+
+	intel_disable_fbc(dev);
+
+	/*
+	 * Must wait until the next vblank before re-enabling
+	 * otherwise the nuking won't actually happen.
+	 */
+	intel_enable_fbc(crtc, 500);
+}
+
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      enum no_fbc_reason reason)
 {
-- 
1.8.3.2

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

Re: [PATCH 8/9] drm/i915: Hook up dirtyfb ioctl for FBC nuke

[Chris Wilson]

On Thu, Nov 21, 2013 at 09:29:52PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> FBC host modification tracking only works through GTT mmaps, so any
> direct CPU access needs to manually nuke the compressed framebuffer
> on modifications. Hook up the dirtyfb ioctl to do just that.

But direct CPU access (not GTT) notification is done through SW_FINISH
ioctl. Overzealously nuking FBC here makes it inconvenient for userpsace
to use fb_dirty as part of its periodic-flush-scanout procedure.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 8/9] drm/i915: Hook up dirtyfb ioctl for FBC nuke

[Ville Syrjälä]

On Thu, Nov 21, 2013 at 11:18:02PM +0000, Chris Wilson wrote:
> On Thu, Nov 21, 2013 at 09:29:52PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > FBC host modification tracking only works through GTT mmaps, so any
> > direct CPU access needs to manually nuke the compressed framebuffer
> > on modifications. Hook up the dirtyfb ioctl to do just that.
> 
> But direct CPU access (not GTT) notification is done through SW_FINISH
> ioctl. Overzealously nuking FBC here makes it inconvenient for userpsace
> to use fb_dirty as part of its periodic-flush-scanout procedure.

I see. I can move the fbc nuke to sw_finish. Could we actually document
the rules for these ioctls somewhere?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 8/9] drm/i915: Hook up dirtyfb ioctl for FBC nuke

[Daniel Vetter]

On Fri, Nov 22, 2013 at 05:19:01PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 21, 2013 at 11:18:02PM +0000, Chris Wilson wrote:
> > On Thu, Nov 21, 2013 at 09:29:52PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > FBC host modification tracking only works through GTT mmaps, so any
> > > direct CPU access needs to manually nuke the compressed framebuffer
> > > on modifications. Hook up the dirtyfb ioctl to do just that.
> > 
> > But direct CPU access (not GTT) notification is done through SW_FINISH
> > ioctl. Overzealously nuking FBC here makes it inconvenient for userpsace
> > to use fb_dirty as part of its periodic-flush-scanout procedure.
> 
> I see. I can move the fbc nuke to sw_finish. Could we actually document
> the rules for these ioctls somewhere?

Yeah, I'll do that when reviewing your testcase. Imo we should have two
subtests for each access method: One with the legacy way of doing things
(where we only care about correctness and could e.g. opt to completely
nuke things), one using drmDirtyFb (if we decide to go down this road).

Note that imo drmDirtyFB is only useful if we decide to ditch the hw based
tracking and go with sw tracking. Otherwise we start to sprinkle usage all
over the place, which means we're probably doing something stupid (which
is caught by the hw tracking for now). That usually leads to baked-in abi
stupidity for the next few years ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2 8/9] drm/i915: Nuke FBC from SW_FINISH ioctl

[ville.syrjala]

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

FBC host modification tracking only works through GTT mmaps, so any
direct CPU access needs to manually nuke the compressed framebuffer
on modifications. Do the nuking from the SW_FINISH ioctl.

v2: nuke from SW_FINISH insted of DIRTYFB ioctl

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c  |  2 ++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 30 ++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 40d9dcf..fdd7279 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1309,6 +1309,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (obj->pin_display)
 		i915_gem_object_flush_cpu_write_domain(obj, true);
 
+	intel_fbc_nuke(obj);
+
 	drm_gem_object_unreference(&obj->base);
 unlock:
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f9e9ca0..0bdcad6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -840,6 +840,7 @@ void intel_fbc_flip_start(struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb);
 void intel_fbc_flip_end(struct drm_crtc *crtc,
 			struct drm_framebuffer *fb);
+void intel_fbc_nuke(struct drm_i915_gem_object *obj);
 void intel_update_fbc(struct drm_device *dev);
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 void intel_gpu_ips_teardown(void);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2a613ac..40a4e8d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -508,6 +508,36 @@ void intel_fbc_flip_end(struct drm_crtc *crtc,
 	intel_update_fbc_fb(crtc, fb);
 }
 
+void intel_fbc_nuke(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+
+	if (dev_priv->fbc.fbc_work) {
+		if (to_intel_framebuffer(dev_priv->fbc.fbc_work->fb)->obj != obj)
+			return;
+
+		crtc = dev_priv->fbc.fbc_work->crtc;
+	} else {
+		if (to_intel_framebuffer(dev_priv->fbc.fb)->obj != obj)
+			return;
+
+		if (WARN_ON(dev_priv->fbc.plane < 0))
+			return;
+
+		crtc = dev_priv->plane_to_crtc_mapping[dev_priv->fbc.plane];
+	}
+
+	intel_disable_fbc(dev);
+
+	/*
+	 * Must wait until the next vblank before re-enabling
+	 * otherwise the nuking won't actually happen.
+	 */
+	intel_enable_fbc(crtc, 500);
+}
+
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      enum no_fbc_reason reason)
 {
-- 
1.8.3.2

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

Re: [PATCH v2 8/9] drm/i915: Nuke FBC from SW_FINISH ioctl

[Chris Wilson]

On Mon, Nov 25, 2013 at 04:54:48PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> FBC host modification tracking only works through GTT mmaps, so any
> direct CPU access needs to manually nuke the compressed framebuffer
> on modifications. Do the nuking from the SW_FINISH ioctl.
> 
> v2: nuke from SW_FINISH insted of DIRTYFB ioctl
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c  |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 40d9dcf..fdd7279 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1309,6 +1309,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>  	if (obj->pin_display)
>  		i915_gem_object_flush_cpu_write_domain(obj, true);
>  
> +	intel_fbc_nuke(obj);

All though it doesn't matter (since intel_fbc_nuke does sufficient
checks itself), it would seem clear to me to put this in the same
if(obj->pin_display) block as the cpu flush.

Notwithstanding that,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Do we have any i-g-t to make sure that this path remains correct?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3 8/9] drm/i915: Nuke FBC from SW_FINISH ioctl

[ville.syrjala]

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

FBC host modification tracking only works through GTT mmaps, so any
direct CPU access needs to manually nuke the compressed framebuffer
on modifications. Do the nuking from the SW_FINISH ioctl.

v2: nuke from SW_FINISH insted of DIRTYFB ioctl
v3: Call intel_fbc_nuke() only when pin_display is true

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c  |  4 +++-
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 30 ++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 40d9dcf..a97f58e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1306,8 +1306,10 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	}
 
 	/* Pinned buffers may be scanout, so flush the cache */
-	if (obj->pin_display)
+	if (obj->pin_display) {
 		i915_gem_object_flush_cpu_write_domain(obj, true);
+		intel_fbc_nuke(obj);
+	}
 
 	drm_gem_object_unreference(&obj->base);
 unlock:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f9e9ca0..0bdcad6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -840,6 +840,7 @@ void intel_fbc_flip_start(struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb);
 void intel_fbc_flip_end(struct drm_crtc *crtc,
 			struct drm_framebuffer *fb);
+void intel_fbc_nuke(struct drm_i915_gem_object *obj);
 void intel_update_fbc(struct drm_device *dev);
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 void intel_gpu_ips_teardown(void);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2a613ac..40a4e8d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -508,6 +508,36 @@ void intel_fbc_flip_end(struct drm_crtc *crtc,
 	intel_update_fbc_fb(crtc, fb);
 }
 
+void intel_fbc_nuke(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+
+	if (dev_priv->fbc.fbc_work) {
+		if (to_intel_framebuffer(dev_priv->fbc.fbc_work->fb)->obj != obj)
+			return;
+
+		crtc = dev_priv->fbc.fbc_work->crtc;
+	} else {
+		if (to_intel_framebuffer(dev_priv->fbc.fb)->obj != obj)
+			return;
+
+		if (WARN_ON(dev_priv->fbc.plane < 0))
+			return;
+
+		crtc = dev_priv->plane_to_crtc_mapping[dev_priv->fbc.plane];
+	}
+
+	intel_disable_fbc(dev);
+
+	/*
+	 * Must wait until the next vblank before re-enabling
+	 * otherwise the nuking won't actually happen.
+	 */
+	intel_enable_fbc(crtc, 500);
+}
+
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      enum no_fbc_reason reason)
 {
-- 
1.8.3.2

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

[PATCH v4 8/9] drm/i915: Nuke FBC from SW_FINISH ioctl

[ville.syrjala]

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

FBC host modification tracking only works through GTT mmaps, so any
direct CPU access needs to manually nuke the compressed framebuffer
on modifications. Do the nuking from the SW_FINISH ioctl.

v2: nuke from SW_FINISH insted of DIRTYFB ioctl
v3: Call intel_fbc_nuke() only when pin_display is true
v4: Don't oops if dev_priv->fbc.fb is NULL

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c  |  4 +++-
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0ecc735..b5395aa 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1306,8 +1306,10 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	}
 
 	/* Pinned buffers may be scanout, so flush the cache */
-	if (obj->pin_display)
+	if (obj->pin_display) {
 		i915_gem_object_flush_cpu_write_domain(obj, true);
+		intel_fbc_nuke(obj);
+	}
 
 	drm_gem_object_unreference(&obj->base);
 unlock:
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2fa843f..2baf0dd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -836,6 +836,7 @@ void intel_update_sprite_watermarks(struct drm_plane *plane,
 				    bool enabled, bool scaled);
 void intel_init_pm(struct drm_device *dev);
 bool intel_fbc_enabled(struct drm_device *dev);
+void intel_fbc_nuke(struct drm_i915_gem_object *obj);
 void intel_update_fbc(struct drm_device *dev);
 void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 void intel_gpu_ips_teardown(void);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4f274ca..f46bc36 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -418,6 +418,39 @@ void intel_disable_fbc(struct drm_device *dev)
 	}
 }
 
+void intel_fbc_nuke(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+
+	if (dev_priv->fbc.fbc_work) {
+		if (to_intel_framebuffer(dev_priv->fbc.fbc_work->fb)->obj != obj)
+			return;
+
+		crtc = dev_priv->fbc.fbc_work->crtc;
+	} else {
+		if (!dev_priv->fbc.fb)
+			return;
+
+		if (to_intel_framebuffer(dev_priv->fbc.fb)->obj != obj)
+			return;
+
+		if (WARN_ON(dev_priv->fbc.plane < 0))
+			return;
+
+		crtc = dev_priv->plane_to_crtc_mapping[dev_priv->fbc.plane];
+	}
+
+	intel_disable_fbc(dev);
+
+	/*
+	 * Must wait until the next vblank before re-enabling
+	 * otherwise the nuking won't actually happen.
+	 */
+	intel_enable_fbc(crtc, 500);
+}
+
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
 			      enum no_fbc_reason reason)
 {
-- 
1.8.3.2

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

[PATCH 9/9] drm/i915: Flush caches for scanout during cpu->gtt move

[ville.syrjala]

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

Flush the caches when moving a scanout buffer from CPU to GTT domain.
This allows us to move a scanout buffer to CPU write domain, do some
writes, and move it back to the GTT read domain. The display will then
see the correct data. In addition we still need to do the dirtyfb
ioctl to nuke FBC if that's enabled.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 40d9dcf..d6e354d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3428,7 +3428,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 	if (ret)
 		return ret;
 
-	i915_gem_object_flush_cpu_write_domain(obj, false);
+	i915_gem_object_flush_cpu_write_domain(obj, obj->pin_display);
 
 	/* Serialise direct access to this object with the barriers for
 	 * coherent writes from the GPU, by effectively invalidating the
-- 
1.8.3.2

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

Re: [PATCH 9/9] drm/i915: Flush caches for scanout during cpu->gtt move

[Chris Wilson]

On Thu, Nov 21, 2013 at 09:29:53PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Flush the caches when moving a scanout buffer from CPU to GTT domain.
> This allows us to move a scanout buffer to CPU write domain, do some
> writes, and move it back to the GTT read domain. The display will then
> see the correct data. In addition we still need to do the dirtyfb
> ioctl to nuke FBC if that's enabled.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

But we could actually rework flush_cpu_write_domain to drop the force
parameter now that we have obj->pin_display.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 9/9] drm/i915: Flush caches for scanout during cpu->gtt move

[Daniel Vetter]

On Thu, Nov 21, 2013 at 11:20:58PM +0000, Chris Wilson wrote:
> On Thu, Nov 21, 2013 at 09:29:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Flush the caches when moving a scanout buffer from CPU to GTT domain.
> > This allows us to move a scanout buffer to CPU write domain, do some
> > writes, and move it back to the GTT read domain. The display will then
> > see the correct data. In addition we still need to do the dirtyfb
> > ioctl to nuke FBC if that's enabled.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Isn't this what sw_finish is for?
> 
> But we could actually rework flush_cpu_write_domain to drop the force
> parameter now that we have obj->pin_display.

Although it doesn't really fit into the new world any longer ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 9/9] drm/i915: Flush caches for scanout during cpu->gtt move

[Chris Wilson]

On Mon, Nov 25, 2013 at 09:47:28AM +0100, Daniel Vetter wrote:
> On Thu, Nov 21, 2013 at 11:20:58PM +0000, Chris Wilson wrote:
> > On Thu, Nov 21, 2013 at 09:29:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Flush the caches when moving a scanout buffer from CPU to GTT domain.
> > > This allows us to move a scanout buffer to CPU write domain, do some
> > > writes, and move it back to the GTT read domain. The display will then
> > > see the correct data. In addition we still need to do the dirtyfb
> > > ioctl to nuke FBC if that's enabled.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Isn't this what sw_finish is for?

I was more concerned about making sure the code was reasonably
self-consistent in applying our own coherency rules. As you point out,
it may be userspace making a mistake, but so many paths can end up here,
and almost never have pin_display, that it would seem to be preferrable
to do the extra flush rather than have the discrepancy.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 9/9] drm/i915: Flush caches for scanout during cpu->gtt move

[Ville Syrjälä]

On Mon, Nov 25, 2013 at 11:04:48AM +0000, Chris Wilson wrote:
> On Mon, Nov 25, 2013 at 09:47:28AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 21, 2013 at 11:20:58PM +0000, Chris Wilson wrote:
> > > On Thu, Nov 21, 2013 at 09:29:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Flush the caches when moving a scanout buffer from CPU to GTT domain.
> > > > This allows us to move a scanout buffer to CPU write domain, do some
> > > > writes, and move it back to the GTT read domain. The display will then
> > > > see the correct data. In addition we still need to do the dirtyfb
> > > > ioctl to nuke FBC if that's enabled.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Isn't this what sw_finish is for?
> 
> I was more concerned about making sure the code was reasonably
> self-consistent in applying our own coherency rules. As you point out,
> it may be userspace making a mistake, but so many paths can end up here,
> and almost never have pin_display, that it would seem to be preferrable
> to do the extra flush rather than have the discrepancy.

If we don't do the flush in CPU->GTT move, we might miss it completely.
Eg. if we do things in this order:

set_domain(CPU,CPU)
write some data
set_domain(GTT,0)
sw_finish()

sw_finish would not do the flush since the object is no longer in the
CPU write domain.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 9/9] drm/i915: Flush caches for scanout during cpu->gtt move

[Daniel Vetter]

On Mon, Nov 25, 2013 at 04:40:22PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 25, 2013 at 11:04:48AM +0000, Chris Wilson wrote:
> > On Mon, Nov 25, 2013 at 09:47:28AM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 21, 2013 at 11:20:58PM +0000, Chris Wilson wrote:
> > > > On Thu, Nov 21, 2013 at 09:29:53PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > Flush the caches when moving a scanout buffer from CPU to GTT domain.
> > > > > This allows us to move a scanout buffer to CPU write domain, do some
> > > > > writes, and move it back to the GTT read domain. The display will then
> > > > > see the correct data. In addition we still need to do the dirtyfb
> > > > > ioctl to nuke FBC if that's enabled.
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > Isn't this what sw_finish is for?
> > 
> > I was more concerned about making sure the code was reasonably
> > self-consistent in applying our own coherency rules. As you point out,
> > it may be userspace making a mistake, but so many paths can end up here,
> > and almost never have pin_display, that it would seem to be preferrable
> > to do the extra flush rather than have the discrepancy.
> 
> If we don't do the flush in CPU->GTT move, we might miss it completely.
> Eg. if we do things in this order:
> 
> set_domain(CPU,CPU)
> write some data
> set_domain(GTT,0)

The question is: Who's doing the set_domain? Atm the sw_finish is done as
part of the libdrm unmap interface, so if userspace is wreaking havoc by
interleaving a set_domain here then that's Just a Bug.

If otoh the kernel is doing a set_domain then that means userspace is
racing access with some other thread concurrently and again gets to keep
both pieces.

We obviously need to keep things working, but beding over backwards to
make even insane cases work is imo not a good approach: The resulting
overkill tends to duct-tape over real issues which then hurt us
long-term. I much prefer a simple model for all the bo access paths we
have (and want to support) and shrug all the insane cases off with a
"you're doing it wrong".

Cheers, Daniel

> sw_finish()
> 
> sw_finish would not do the flush since the object is no longer in the
> CPU write domain.
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/9] drm/i915: Some more FBC stuff

[Daniel Vetter]

On Thu, Nov 21, 2013 at 09:29:44PM +0200, ville.syrjala@linux.intel.com wrote:
> Another set of FBC patches, which should fit on top of the previous set:
> "[PATCH 00/10] drm/i915: FBC fixes v2"
> 
> The persistent mode and HT tracking bit stuff is a bit unclear in the docs,
> but I can remove it all, and everything still seems to work fine.
> 
> The page flip and dirtyfb stuff is maybe a bit raw, but I'll post anyway
> now since it seems to work for me.
> 
> I'll post my igt test case that tries to stress all this shortly. It passes
> for me on ILK, SNB and IVB. On ILK it's a bit limited since there are no
> contexts (didn't try the ILK context patches w/ this) and we're missing
> a gen5 rendercopy, so I couldn't test the render tracking using igt. But I
> don't get any screen corruption w/ FBC enabled, so it must be working.
> 
> The only FBC1 capable hardware on my desk is a MGM, but someone was a bit
> too conservative when they implemented FBC1 support and enabled it only 
> for CL. I was too lazy to read through the code to see if it should work
> for MGM.
> 
> Ville Syrjälä (9):
>       drm/i915: Don't set the fence number in DPFC_CTL on SNB
>       drm/i915: Don't set persistent FBC mode on ILK/SNB
>       drm/i915: Don't set DPFC_HT_MODIFY bit on CTG/ILK/SNB
>       drm/i915: Use LRI based FBC render tracking for ILK
>       drm/i915: Reorder i915_gem_execbuffer_move_to_gpu() and i915_switch_context()
>       drm/i915: Improve page flip vs. FBC interaction
>       drm: Push dirtyfb ioctl kms locking down to drivers
>       drm/i915: Hook up dirtyfb ioctl for FBC nuke
>       drm/i915: Flush caches for scanout during cpu->gtt move

I've punted on merging the 2 sw flush patches Chris' reviewed - I'd kinda
like to see the full picture for fbc2 frontbuffer tracking first,
including the current performance regression addressed. I just want to
avoid half-merging one approach that later on turns out to be fully
inadequate.

Also, please enable fbc on lots of platforms by default at the end, I'm
not a fan of dead code ;-)

Cheers, Daniel
> 
>  drivers/gpu/drm/drm_crtc.c                 |   2 -
>  drivers/gpu/drm/i915/i915_gem.c            |   2 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   8 +-
>  drivers/gpu/drm/i915/i915_reg.h            |   2 +-
>  drivers/gpu/drm/i915/intel_display.c       |  24 ++++-
>  drivers/gpu/drm/i915/intel_drv.h           |   5 ++
>  drivers/gpu/drm/i915/intel_pm.c            | 136 +++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.c    |  57 ++++++------
>  drivers/gpu/drm/omapdrm/omap_fb.c          |   4 +
>  drivers/gpu/drm/qxl/qxl_display.c          |   9 +-
>  drivers/gpu/drm/udl/udl_fb.c               |  12 ++-
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c        |  18 +++-
>  12 files changed, 228 insertions(+), 51 deletions(-)
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch