[PATCH 00/10] drm/i915: FBC fixes v2

[PATCH 00/10] drm/i915: FBC fixes v2

[ville.syrjala]

One more attempt at making FBC suck a bit less.

The main thing as before is getting the LRI based render/blitter
tracking in place.

In this updates series I decided the way to avoid the kms locks in
execbuffer is to keep an extra reference to the fb.

I added a bunch of locking (struct_mutex and crtc->mutex) to some
places to hopefully make it bit less racy. I think/hope that it doesn't
make it more likely to deadlock. We do grab both locks in the fbc work
now, and we do cancel the work while holding at least one of the locks,
so it seems a bit bad. But we cancel the work in a lazy fashion (no _sync
or anything) so I don't think it should deadlock due to that. And we
already grabbed struct_mutex there anyway, and we already held it while
cancelling the work, so it's no worse at least :P

The disable_fbc/update_fbc calls from the page flip code are lacking
kms lock protection, but we can't simply add it there due to possibility
of deadlocks.

So at least the render/blitter tracking seems to work now without
upsetting lockdep, so it seems a bit better than what we had at least.
Maybe someone else will get inspired by this and fix this mess up for
real...

I've only run this on my IVB, and just running X + some apps w/o
a compositor. No extensive tests or anything. But if I frob some
debug register to disable some tracking bits the screen gets
corrupted, so it seems to be doing its job.

I pushed it here in case someone is interested in a git tree:
git://gitorious.org/vsyrjala/linux.git fbc_fixes

Ville Syrjälä (10):
      drm/i915: Use plane_name() in gen7_enable_fbc()
      drm/i915: Grab struct_mutex around all FBC updates
      drm/i915: Have FBC keep references to the fb
      drm/i915: Grab crtc->mutex in intel_fbc_work_fn()
      drm/i915: Limit FBC flush to post batch flush
      drm/i915: Emit SRM after the MSG_FBC_REND_STATE LRI
      drm/i915: Implement LRI based FBC tracking
      drm/i915: Kill sandybridge_blit_fbc_update()
      drm/i915: Don't write ILK/IVB_FBC_RT_BASE directly
      drm/i915: Set has_fbc=true for all SNB+, except VLV

 drivers/gpu/drm/i915/i915_drv.c            |  6 +-
 drivers/gpu/drm/i915/i915_drv.h            |  6 +-
 drivers/gpu/drm/i915/i915_gem_context.c    |  7 +++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++
 drivers/gpu/drm/i915/i915_reg.h            |  1 +
 drivers/gpu/drm/i915/intel_display.c       | 27 +++++++-
 drivers/gpu/drm/i915/intel_drv.h           |  1 +
 drivers/gpu/drm/i915/intel_pm.c            | 98 ++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_ringbuffer.c    | 66 ++++++++++++++++++--
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  2 +
 10 files changed, 188 insertions(+), 57 deletions(-)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/10] drm/i915: Grab struct_mutex around all FBC updates

[ville.syrjala]

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

We need some protection for the FBC state, and since struct_mutex
is it currently in most places, make sure all FBC update/disable
calles are protected by it.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 12cf362..bce6e07 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3576,8 +3576,10 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
 	drm_vblank_off(dev, pipe);
 
 	/* FBC must be disabled before disabling the plane on HSW. */
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	hsw_disable_ips(intel_crtc);
 
@@ -3717,8 +3719,10 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_wait_for_pending_flips(crtc);
 	drm_vblank_off(dev, pipe);
 
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
@@ -4102,7 +4106,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4146,7 +4152,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
 	intel_crtc_dpms_overlay(intel_crtc, true);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4210,7 +4218,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)
-- 
1.8.1.5

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

Re: [PATCH 01/10] drm/i915: Grab struct_mutex around all FBC updates

[Rodrigo Vivi]

On Wed, Nov 06, 2013 at 11:02:16PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We need some protection for the FBC state, and since struct_mutex
> is it currently in most places, make sure all FBC update/disable
> calles are protected by it.

Why don't you create a new mutex only for fbc update?

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 12cf362..bce6e07 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3576,8 +3576,10 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
>  	drm_vblank_off(dev, pipe);
>  
>  	/* FBC must be disabled before disabling the plane on HSW. */
> +	mutex_lock(&dev->struct_mutex);
>  	if (dev_priv->fbc.plane == plane)
>  		intel_disable_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	hsw_disable_ips(intel_crtc);
>  
> @@ -3717,8 +3719,10 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	intel_crtc_wait_for_pending_flips(crtc);
>  	drm_vblank_off(dev, pipe);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	if (dev_priv->fbc.plane == plane)
>  		intel_disable_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_crtc_update_cursor(crtc, false);
>  	intel_disable_planes(crtc);
> @@ -4102,7 +4106,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4146,7 +4152,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	/* Give the overlay scaler a chance to enable if it's on this pipe */
>  	intel_crtc_dpms_overlay(intel_crtc, true);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4210,7 +4218,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  static void i9xx_crtc_off(struct drm_crtc *crtc)
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm/i915: Grab struct_mutex around all FBC updates

[Daniel Vetter]

On Wed, Nov 20, 2013 at 11:39 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Wed, Nov 06, 2013 at 11:02:16PM +0200, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> We need some protection for the FBC state, and since struct_mutex
>> is it currently in most places, make sure all FBC update/disable
>> calles are protected by it.
>
> Why don't you create a new mutex only for fbc update?

Yeah, if it's not core gem state please don't spread the usage of
dev->struct_mutex. Eventually we need to slash that one into pieces,
but until that happens making things worse doesn't help.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/10] drm/i915: Grab struct_mutex around all FBC updates

[Ville Syrjälä]

On Thu, Nov 21, 2013 at 09:22:43AM +0100, Daniel Vetter wrote:
> On Wed, Nov 20, 2013 at 11:39 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> > On Wed, Nov 06, 2013 at 11:02:16PM +0200, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> We need some protection for the FBC state, and since struct_mutex
> >> is it currently in most places, make sure all FBC update/disable
> >> calles are protected by it.
> >
> > Why don't you create a new mutex only for fbc update?
> 
> Yeah, if it's not core gem state please don't spread the usage of
> dev->struct_mutex. Eventually we need to slash that one into pieces,
> but until that happens making things worse doesn't help.

I don't think I'm making things worse. struct_mutex is the lock that
protects the fbc state currently, except we forgot to grab it in a some
places. I'd rather fix this first, and then as a followup someone can
start to think about using a new lock for fbc.

-- 
Ville Syrjälä
Intel OTC

[PATCH v2 01/10] drm/i915: Grab struct_mutex around all FBC updates

[ville.syrjala]

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

We need some protection for the FBC state, and since struct_mutex
is it currently in most places, make sure all FBC update/disable
calles are protected by it.

v2: Also protect intel_disable_fbc in i9xx_crtc_disable

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e85d838..326ceca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3600,8 +3600,10 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
 	drm_vblank_off(dev, pipe);
 
 	/* FBC must be disabled before disabling the plane on HSW. */
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	hsw_disable_ips(intel_crtc);
 
@@ -3741,8 +3743,10 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_wait_for_pending_flips(crtc);
 	drm_vblank_off(dev, pipe);
 
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
@@ -4126,7 +4130,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4170,7 +4176,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
 	intel_crtc_dpms_overlay(intel_crtc, true);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4210,8 +4218,10 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_wait_for_pending_flips(crtc);
 	drm_vblank_off(dev, pipe);
 
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
@@ -4234,7 +4244,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)
-- 
1.8.3.2

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

[PATCH 02/10] drm/i915: Have FBC keep references to the fb

[ville.syrjala]

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

In order to do the FBC tracking properly for execbuffer, we need to
figure out if the object being rendered to is the current front buffer.
However in order to do that purely based on the crtc, we'd need to
grab crtc->mutex, but we can't since we're already holding struct_mutex.

So let's keep the current fb around in dev_priv->fbc.fb and consult that
one. We're holding a reference to it for the entire time FBC is enabled,
so there's no danger of it disappearing while we're looking at it.
Assuming FBC updates always happens while holding struct_mutex that is.

Also change the delayed enable mechanism to grab a reference to the fb.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c842928..42a4375 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -352,7 +352,9 @@ struct dpll;
 
 struct drm_i915_display_funcs {
 	bool (*fbc_enabled)(struct drm_device *dev);
-	void (*enable_fbc)(struct drm_crtc *crtc, unsigned long interval);
+	void (*enable_fbc)(struct drm_crtc *crtc,
+			   struct drm_framebuffer *fb,
+			   unsigned long interval);
 	void (*disable_fbc)(struct drm_device *dev);
 	int (*get_display_clock_speed)(struct drm_device *dev);
 	int (*get_fifo_size)(struct drm_device *dev, int plane);
@@ -652,7 +654,7 @@ struct i915_hw_context {
 
 struct i915_fbc {
 	unsigned long size;
-	unsigned int fb_id;
+	struct drm_framebuffer *fb;
 	enum plane plane;
 	int y;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dc306e1..8780b87 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -86,11 +86,12 @@ static void i8xx_disable_fbc(struct drm_device *dev)
 	DRM_DEBUG_KMS("disabled FBC\n");
 }
 
-static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void i8xx_enable_fbc(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_framebuffer *fb = crtc->fb;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -136,11 +137,12 @@ static bool i8xx_fbc_enabled(struct drm_device *dev)
 	return I915_READ(FBC_CONTROL) & FBC_CTL_EN;
 }
 
-static void g4x_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void g4x_enable_fbc(struct drm_crtc *crtc,
+			   struct drm_framebuffer *fb,
+			   unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_framebuffer *fb = crtc->fb;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -205,11 +207,12 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
 	gen6_gt_force_wake_put(dev_priv);
 }
 
-static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void ironlake_enable_fbc(struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_framebuffer *fb = crtc->fb;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -265,11 +268,12 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
 	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
-static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void gen7_enable_fbc(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_framebuffer *fb = crtc->fb;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -308,6 +312,22 @@ bool intel_fbc_enabled(struct drm_device *dev)
 	return dev_priv->display.fbc_enabled(dev);
 }
 
+static void intel_setup_fbc(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    unsigned long interval)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+
+	dev_priv->display.enable_fbc(crtc, fb, interval);
+
+	dev_priv->fbc.plane = to_intel_crtc(crtc)->plane;
+	drm_framebuffer_reference(fb);
+	if (dev_priv->fbc.fb)
+		drm_framebuffer_unreference(dev_priv->fbc.fb);
+	dev_priv->fbc.fb = fb;
+	dev_priv->fbc.y = crtc->y;
+}
+
 static void intel_fbc_work_fn(struct work_struct *__work)
 {
 	struct intel_fbc_work *work =
@@ -321,19 +341,14 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 		/* Double check that we haven't switched fb without cancelling
 		 * the prior work.
 		 */
-		if (work->crtc->fb == work->fb) {
-			dev_priv->display.enable_fbc(work->crtc,
-						     work->interval);
-
-			dev_priv->fbc.plane = to_intel_crtc(work->crtc)->plane;
-			dev_priv->fbc.fb_id = work->crtc->fb->base.id;
-			dev_priv->fbc.y = work->crtc->y;
-		}
-
+		if (work->crtc->fb == work->fb)
+			intel_setup_fbc(work->crtc, work->fb, work->interval);
 		dev_priv->fbc.fbc_work = NULL;
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+	drm_framebuffer_reference(work->fb);
+
 	kfree(work);
 }
 
@@ -348,9 +363,11 @@ static void intel_cancel_fbc_work(struct drm_i915_private *dev_priv)
 	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
 	 * entirely asynchronously.
 	 */
-	if (cancel_delayed_work(&dev_priv->fbc.fbc_work->work))
+	if (cancel_delayed_work(&dev_priv->fbc.fbc_work->work)) {
 		/* tasklet was killed before being run, clean up */
+		drm_framebuffer_unreference(dev_priv->fbc.fbc_work->fb);
 		kfree(dev_priv->fbc.fbc_work);
+	}
 
 	/* Mark the work as no longer wanted so that if it does
 	 * wake-up (because the work was already running and waiting
@@ -374,7 +391,7 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL) {
 		DRM_ERROR("Failed to allocate FBC work structure\n");
-		dev_priv->display.enable_fbc(crtc, interval);
+		intel_setup_fbc(crtc, crtc->fb, interval);
 		return;
 	}
 
@@ -383,6 +400,8 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	work->interval = interval;
 	INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
 
+	drm_framebuffer_reference(work->fb);
+
 	dev_priv->fbc.fbc_work = work;
 
 	/* Delay the actual enabling to let pageflipping cease and the
@@ -412,6 +431,10 @@ void intel_disable_fbc(struct drm_device *dev)
 
 	dev_priv->display.disable_fbc(dev);
 	dev_priv->fbc.plane = -1;
+	if (dev_priv->fbc.fb) {
+		drm_framebuffer_unreference(dev_priv->fbc.fb);
+		dev_priv->fbc.fb = NULL;
+	}
 }
 
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
@@ -563,7 +586,7 @@ void intel_update_fbc(struct drm_device *dev)
 	 * without first being decoupled from the scanout and FBC disabled.
 	 */
 	if (dev_priv->fbc.plane == intel_crtc->plane &&
-	    dev_priv->fbc.fb_id == fb->base.id &&
+	    dev_priv->fbc.fb == fb &&
 	    dev_priv->fbc.y == crtc->y)
 		return;
 
-- 
1.8.1.5

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

[PATCH v2 02/10] drm/i915: Have FBC keep references to the fb

[ville.syrjala]

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

In order to do the FBC tracking properly for execbuffer, we need to
figure out if the object being rendered to is the current front buffer.
However in order to do that purely based on the crtc, we'd need to
grab crtc->mutex, but we can't since we're already holding struct_mutex.

So let's keep the current fb around in dev_priv->fbc.fb and consult that
one. We're holding a reference to it for the entire time FBC is enabled,
so there's no danger of it disappearing while we're looking at it.
Assuming FBC updates always happens while holding struct_mutex that is.

Also change the delayed enable mechanism to grab a reference to the fb.

v2: Fix fb refcounting in intel_setup_fbc()

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 14f250a..becc95f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -371,7 +371,9 @@ struct dpll;
 
 struct drm_i915_display_funcs {
 	bool (*fbc_enabled)(struct drm_device *dev);
-	void (*enable_fbc)(struct drm_crtc *crtc, unsigned long interval);
+	void (*enable_fbc)(struct drm_crtc *crtc,
+			   struct drm_framebuffer *fb,
+			   unsigned long interval);
 	void (*disable_fbc)(struct drm_device *dev);
 	int (*get_display_clock_speed)(struct drm_device *dev);
 	int (*get_fifo_size)(struct drm_device *dev, int plane);
@@ -678,7 +680,7 @@ struct i915_hw_context {
 
 struct i915_fbc {
 	unsigned long size;
-	unsigned int fb_id;
+	struct drm_framebuffer *fb;
 	enum plane plane;
 	int y;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 04e9863..2d26c93 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -86,11 +86,12 @@ static void i8xx_disable_fbc(struct drm_device *dev)
 	DRM_DEBUG_KMS("disabled FBC\n");
 }
 
-static void i8xx_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void i8xx_enable_fbc(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_framebuffer *fb = crtc->fb;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -136,11 +137,12 @@ static bool i8xx_fbc_enabled(struct drm_device *dev)
 	return I915_READ(FBC_CONTROL) & FBC_CTL_EN;
 }
 
-static void g4x_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void g4x_enable_fbc(struct drm_crtc *crtc,
+			   struct drm_framebuffer *fb,
+			   unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_framebuffer *fb = crtc->fb;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -205,11 +207,12 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
 	gen6_gt_force_wake_put(dev_priv);
 }
 
-static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void ironlake_enable_fbc(struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_framebuffer *fb = crtc->fb;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -265,11 +268,12 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
 	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
-static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+static void gen7_enable_fbc(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    unsigned long interval)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_framebuffer *fb = crtc->fb;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -308,6 +312,22 @@ bool intel_fbc_enabled(struct drm_device *dev)
 	return dev_priv->display.fbc_enabled(dev);
 }
 
+static void intel_setup_fbc(struct drm_crtc *crtc,
+			    struct drm_framebuffer *fb,
+			    unsigned long interval)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+
+	dev_priv->display.enable_fbc(crtc, fb, interval);
+
+	dev_priv->fbc.plane = to_intel_crtc(crtc)->plane;
+	drm_framebuffer_reference(fb);
+	if (dev_priv->fbc.fb)
+		drm_framebuffer_unreference(dev_priv->fbc.fb);
+	dev_priv->fbc.fb = fb;
+	dev_priv->fbc.y = crtc->y;
+}
+
 static void intel_fbc_work_fn(struct work_struct *__work)
 {
 	struct intel_fbc_work *work =
@@ -321,19 +341,14 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 		/* Double check that we haven't switched fb without cancelling
 		 * the prior work.
 		 */
-		if (work->crtc->fb == work->fb) {
-			dev_priv->display.enable_fbc(work->crtc,
-						     work->interval);
-
-			dev_priv->fbc.plane = to_intel_crtc(work->crtc)->plane;
-			dev_priv->fbc.fb_id = work->crtc->fb->base.id;
-			dev_priv->fbc.y = work->crtc->y;
-		}
-
+		if (work->crtc->fb == work->fb)
+			intel_setup_fbc(work->crtc, work->fb, work->interval);
 		dev_priv->fbc.fbc_work = NULL;
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+	drm_framebuffer_unreference(work->fb);
+
 	kfree(work);
 }
 
@@ -348,9 +363,11 @@ static void intel_cancel_fbc_work(struct drm_i915_private *dev_priv)
 	 * dev_priv->fbc.fbc_work, so we can perform the cancellation
 	 * entirely asynchronously.
 	 */
-	if (cancel_delayed_work(&dev_priv->fbc.fbc_work->work))
+	if (cancel_delayed_work(&dev_priv->fbc.fbc_work->work)) {
 		/* tasklet was killed before being run, clean up */
+		drm_framebuffer_unreference(dev_priv->fbc.fbc_work->fb);
 		kfree(dev_priv->fbc.fbc_work);
+	}
 
 	/* Mark the work as no longer wanted so that if it does
 	 * wake-up (because the work was already running and waiting
@@ -374,7 +391,7 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL) {
 		DRM_ERROR("Failed to allocate FBC work structure\n");
-		dev_priv->display.enable_fbc(crtc, interval);
+		intel_setup_fbc(crtc, crtc->fb, interval);
 		return;
 	}
 
@@ -383,6 +400,8 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	work->interval = interval;
 	INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
 
+	drm_framebuffer_reference(work->fb);
+
 	dev_priv->fbc.fbc_work = work;
 
 	/* Delay the actual enabling to let pageflipping cease and the
@@ -412,6 +431,10 @@ void intel_disable_fbc(struct drm_device *dev)
 
 	dev_priv->display.disable_fbc(dev);
 	dev_priv->fbc.plane = -1;
+	if (dev_priv->fbc.fb) {
+		drm_framebuffer_unreference(dev_priv->fbc.fb);
+		dev_priv->fbc.fb = NULL;
+	}
 }
 
 static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
@@ -563,7 +586,7 @@ void intel_update_fbc(struct drm_device *dev)
 	 * without first being decoupled from the scanout and FBC disabled.
 	 */
 	if (dev_priv->fbc.plane == intel_crtc->plane &&
-	    dev_priv->fbc.fb_id == fb->base.id &&
+	    dev_priv->fbc.fb == fb &&
 	    dev_priv->fbc.y == crtc->y)
 		return;
 
-- 
1.8.3.2

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

[PATCH 03/10] drm/i915: Grab crtc->mutex in intel_fbc_work_fn()

[ville.syrjala]

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

Make the FBC code a bit less racy by grabbing crtc->mutex
in intel_fbc_work_fn() when we go digging through the crtc
state.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8780b87..dc65bb4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -333,9 +333,12 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 	struct intel_fbc_work *work =
 		container_of(to_delayed_work(__work),
 			     struct intel_fbc_work, work);
+	struct drm_crtc *crtc = work->crtc;
 	struct drm_device *dev = work->crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	mutex_lock(&crtc->mutex);
+
 	mutex_lock(&dev->struct_mutex);
 	if (work == dev_priv->fbc.fbc_work) {
 		/* Double check that we haven't switched fb without cancelling
@@ -347,6 +350,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+	mutex_unlock(&crtc->mutex);
+
 	drm_framebuffer_reference(work->fb);
 
 	kfree(work);
-- 
1.8.1.5

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

[PATCH 04/10] drm/i915: Limit FBC flush to post batch flush

[ville.syrjala]

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

Don't issue the FBC nuke/cache clean command when invalidate_domains!=0.
That would indicate that we're not being called for the post-batch
flush.

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

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e32c08a..752f208 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -354,7 +354,7 @@ gen7_render_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, 0);
 	intel_ring_advance(ring);
 
-	if (flush_domains)
+	if (!invalidate_domains && flush_domains)
 		return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
 
 	return 0;
@@ -1837,7 +1837,7 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
 	}
 	intel_ring_advance(ring);
 
-	if (IS_GEN7(dev) && flush)
+	if (IS_GEN7(dev) && !invalidate && flush)
 		return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
 
 	return 0;
-- 
1.8.1.5

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

Re: [PATCH 04/10] drm/i915: Limit FBC flush to post batch flush

[Rodrigo Vivi]

On Wed, Nov 06, 2013 at 11:02:19PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Don't issue the FBC nuke/cache clean command when invalidate_domains!=0.

Double negative almost confused me, but all right here.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

> That would indicate that we're not being called for the post-batch
> flush.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e32c08a..752f208 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -354,7 +354,7 @@ gen7_render_ring_flush(struct intel_ring_buffer *ring,
>  	intel_ring_emit(ring, 0);
>  	intel_ring_advance(ring);
>  
> -	if (flush_domains)
> +	if (!invalidate_domains && flush_domains)
>  		return gen7_ring_fbc_flush(ring, FBC_REND_NUKE);
>  
>  	return 0;
> @@ -1837,7 +1837,7 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
>  	}
>  	intel_ring_advance(ring);
>  
> -	if (IS_GEN7(dev) && flush)
> +	if (IS_GEN7(dev) && !invalidate && flush)
>  		return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
>  
>  	return 0;
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/10] drm/i915: Emit SRM after the MSG_FBC_REND_STATE LRI

[ville.syrjala]

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

The spec tells us that we need to emit an SRM after the LRI
to MSG_FBC_REND_STATE.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0719c8b..7a4d3e1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -235,6 +235,7 @@
  */
 #define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*x-1)
 #define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*x-1)
+#define  MI_SRM_LRM_GLOBAL_GTT		(1<<22)
 #define MI_FLUSH_DW		MI_INSTR(0x26, 1) /* for GEN6 */
 #define   MI_FLUSH_DW_STORE_INDEX	(1<<21)
 #define   MI_INVALIDATE_TLB		(1<<18)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 752f208..4649bf5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -285,14 +285,16 @@ static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring, u32 value)
 	if (!ring->fbc_dirty)
 		return 0;
 
-	ret = intel_ring_begin(ring, 4);
+	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;
-	intel_ring_emit(ring, MI_NOOP);
 	/* WaFbcNukeOn3DBlt:ivb/hsw */
 	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
 	intel_ring_emit(ring, MSG_FBC_REND_STATE);
 	intel_ring_emit(ring, value);
+	intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) | MI_SRM_LRM_GLOBAL_GTT);
+	intel_ring_emit(ring, MSG_FBC_REND_STATE);
+	intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
 	intel_ring_advance(ring);
 
 	ring->fbc_dirty = false;
-- 
1.8.1.5

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

Re: [PATCH 05/10] drm/i915: Emit SRM after the MSG_FBC_REND_STATE LRI

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Wed, Nov 06, 2013 at 11:02:20PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The spec tells us that we need to emit an SRM after the LRI
> to MSG_FBC_REND_STATE.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0719c8b..7a4d3e1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -235,6 +235,7 @@
>   */
>  #define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*x-1)
>  #define MI_STORE_REGISTER_MEM(x) MI_INSTR(0x24, 2*x-1)
> +#define  MI_SRM_LRM_GLOBAL_GTT		(1<<22)
>  #define MI_FLUSH_DW		MI_INSTR(0x26, 1) /* for GEN6 */
>  #define   MI_FLUSH_DW_STORE_INDEX	(1<<21)
>  #define   MI_INVALIDATE_TLB		(1<<18)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 752f208..4649bf5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -285,14 +285,16 @@ static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring, u32 value)
>  	if (!ring->fbc_dirty)
>  		return 0;
>  
> -	ret = intel_ring_begin(ring, 4);
> +	ret = intel_ring_begin(ring, 6);
>  	if (ret)
>  		return ret;
> -	intel_ring_emit(ring, MI_NOOP);
>  	/* WaFbcNukeOn3DBlt:ivb/hsw */
>  	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
>  	intel_ring_emit(ring, MSG_FBC_REND_STATE);
>  	intel_ring_emit(ring, value);
> +	intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) | MI_SRM_LRM_GLOBAL_GTT);
> +	intel_ring_emit(ring, MSG_FBC_REND_STATE);
> +	intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
>  	intel_ring_advance(ring);
>  
>  	ring->fbc_dirty = false;
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 06/10] drm/i915: Implement LRI based FBC tracking

[ville.syrjala]

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

As per the SNB and HSW PM guides, we should enable FBC render/blitter
tracking only during batches targetting the front buffer.

On SNB we must also update the FBC render tracking address whenever it
changes. And since the register in question is stored in the context,
we need to make sure we reload it with correct data after context
switches.

On IVB/HSW we use the render nuke mechanism, so no render tracking
address updates are needed. Hoever on the blitter side we need to
enable the blitter tracking like on SNB, and in addition we need
to issue the cache clean messages, which we already did.

v2: Introduce intel_fb_obj_has_fbc()
    Fix crtc locking around crtc->fb access
    Drop a hunk that was included by accident in v1
    Set fbc_address_dirty=false not true after emitting the LRI
v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
    need to upset lockdep anymore

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c    |  7 ++++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c       | 17 +++++++--
 drivers/gpu/drm/i915/intel_drv.h           |  1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    | 58 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  2 ++
 6 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 72a3df3..d438ea1 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -404,6 +404,13 @@ mi_set_context(struct intel_ring_buffer *ring,
 
 	intel_ring_advance(ring);
 
+	/*
+	 * FBC RT address is stored in the context, so we may have just
+	 * restored it to an old value. Make sure we emit a new LRI
+	 * to update the address.
+	 */
+	ring->fbc_address_dirty = true;
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 885d595..db25158 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -886,6 +886,35 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
 }
 
 static void
+i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring,
+				   struct list_head *vmas)
+{
+	struct i915_vma *vma;
+	struct drm_i915_gem_object *fbc_obj = NULL;
+	u32 fbc_address = -1;
+
+	list_for_each_entry(vma, vmas, exec_list) {
+		struct drm_i915_gem_object *obj = vma->obj;
+
+		if (obj->base.pending_write_domain &&
+		    intel_fb_obj_has_fbc(obj)) {
+			WARN_ON(fbc_obj && fbc_obj != obj);
+			fbc_obj = obj;
+		}
+	}
+
+	if (fbc_obj)
+		fbc_address = i915_gem_obj_ggtt_offset(fbc_obj);
+
+	/* need to nuke/cache_clean on IVB+? */
+	ring->fbc_dirty = fbc_obj != NULL;
+
+	/* need to update FBC tracking? */
+	ring->fbc_address_dirty = fbc_address != ring->fbc_address;
+	ring->fbc_address = fbc_address;
+}
+
+static void
 i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 				   struct intel_ring_buffer *ring)
 {
@@ -1150,6 +1179,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
 		i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
 
+	i915_gem_execbuffer_mark_fbc_dirty(ring, &eb->vmas);
+
 	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
 	if (ret)
 		goto err;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bce6e07..c29e9d4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8074,6 +8074,21 @@ void intel_mark_idle(struct drm_device *dev)
 		gen6_rps_idle(dev->dev_private);
 }
 
+bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* check for potential scanout */
+	if (!obj->pin_display)
+		return false;
+
+	if (!dev_priv->fbc.fb)
+		return false;
+
+	return to_intel_framebuffer(dev_priv->fbc.fb)->obj == obj;
+}
+
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_ring_buffer *ring)
 {
@@ -8091,8 +8106,6 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			continue;
 
 		intel_increase_pllclock(crtc);
-		if (ring && intel_fbc_enabled(dev))
-			ring->fbc_dirty = true;
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6d701e7..5c7e8b4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -614,6 +614,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 /* intel_display.c */
 int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
+bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj);
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_ring_buffer *ring);
 void intel_mark_idle(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4649bf5..64fbab5 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -207,6 +207,57 @@ intel_emit_post_sync_nonzero_flush(struct intel_ring_buffer *ring)
 	return 0;
 }
 
+static int gen6_blt_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, GEN6_BLITTER_ECOSKPD);
+	if (ring->fbc_address != -1)
+		intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
+	else
+		intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
+	intel_ring_advance(ring);
+
+	ring->fbc_address_dirty = false;
+
+	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)
@@ -256,6 +307,9 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, 0);
 	intel_ring_advance(ring);
 
+	if (invalidate_domains)
+		return gen6_render_fbc_tracking(ring);
+
 	return 0;
 }
 
@@ -1839,7 +1893,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
 	}
 	intel_ring_advance(ring);
 
-	if (IS_GEN7(dev) && !invalidate && flush)
+	if (invalidate)
+		return gen6_blt_fbc_tracking(ring);
+	else if (flush && IS_GEN7(dev))
 		return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 71a73f4..1e5bbd6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -143,8 +143,10 @@ struct  intel_ring_buffer {
 	 */
 	struct drm_i915_gem_request *preallocated_lazy_request;
 	u32 outstanding_lazy_seqno;
+	u32 fbc_address;
 	bool gpu_caches_dirty;
 	bool fbc_dirty;
+	bool fbc_address_dirty;
 
 	wait_queue_head_t irq_queue;
 
-- 
1.8.1.5

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

Re: [PATCH v3 06/10] drm/i915: Implement LRI based FBC tracking

[Rodrigo Vivi]

On Wed, Nov 06, 2013 at 11:02:21PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> As per the SNB and HSW PM guides, we should enable FBC render/blitter
> tracking only during batches targetting the front buffer.

You improved things a lot here, but I'm just not convinced this is tracking only and all front buffer touches.

> 
> On SNB we must also update the FBC render tracking address whenever it
> changes. And since the register in question is stored in the context,
> we need to make sure we reload it with correct data after context
> switches.
> 
> On IVB/HSW we use the render nuke mechanism, so no render tracking
> address updates are needed. Hoever on the blitter side we need to
> enable the blitter tracking like on SNB, and in addition we need
> to issue the cache clean messages, which we already did.
> 
> v2: Introduce intel_fb_obj_has_fbc()
>     Fix crtc locking around crtc->fb access
>     Drop a hunk that was included by accident in v1
>     Set fbc_address_dirty=false not true after emitting the LRI
> v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
>     need to upset lockdep anymore
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c    |  7 ++++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c       | 17 +++++++--
>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c    | 58 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  2 ++
>  6 files changed, 113 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 72a3df3..d438ea1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -404,6 +404,13 @@ mi_set_context(struct intel_ring_buffer *ring,
>  
>  	intel_ring_advance(ring);
>  
> +	/*
> +	 * FBC RT address is stored in the context, so we may have just
> +	 * restored it to an old value. Make sure we emit a new LRI
> +	 * to update the address.
> +	 */
> +	ring->fbc_address_dirty = true;
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 885d595..db25158 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -886,6 +886,35 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
>  }
>  
>  static void
> +i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring,
> +				   struct list_head *vmas)
> +{
> +	struct i915_vma *vma;
> +	struct drm_i915_gem_object *fbc_obj = NULL;
> +	u32 fbc_address = -1;
> +
> +	list_for_each_entry(vma, vmas, exec_list) {
> +		struct drm_i915_gem_object *obj = vma->obj;
> +
> +		if (obj->base.pending_write_domain &&
> +		    intel_fb_obj_has_fbc(obj)) {
> +			WARN_ON(fbc_obj && fbc_obj != obj);
> +			fbc_obj = obj;
> +		}
> +	}
> +
> +	if (fbc_obj)
> +		fbc_address = i915_gem_obj_ggtt_offset(fbc_obj);
> +
> +	/* need to nuke/cache_clean on IVB+? */
> +	ring->fbc_dirty = fbc_obj != NULL;
> +
> +	/* need to update FBC tracking? */
> +	ring->fbc_address_dirty = fbc_address != ring->fbc_address;
> +	ring->fbc_address = fbc_address;
> +}
> +
> +static void
>  i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>  				   struct intel_ring_buffer *ring)
>  {
> @@ -1150,6 +1179,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
>  		i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
>  
> +	i915_gem_execbuffer_mark_fbc_dirty(ring, &eb->vmas);
> +
>  	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
>  	if (ret)
>  		goto err;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bce6e07..c29e9d4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8074,6 +8074,21 @@ void intel_mark_idle(struct drm_device *dev)
>  		gen6_rps_idle(dev->dev_private);
>  }
>  
> +bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_device *dev = obj->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* check for potential scanout */
> +	if (!obj->pin_display)
> +		return false;
> +
> +	if (!dev_priv->fbc.fb)
> +		return false;
> +
> +	return to_intel_framebuffer(dev_priv->fbc.fb)->obj == obj;
> +}
> +
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>  			struct intel_ring_buffer *ring)
>  {
> @@ -8091,8 +8106,6 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>  			continue;
>  
>  		intel_increase_pllclock(crtc);
> -		if (ring && intel_fbc_enabled(dev))
> -			ring->fbc_dirty = true;
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6d701e7..5c7e8b4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -614,6 +614,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  /* intel_display.c */
>  int intel_pch_rawclk(struct drm_device *dev);
>  void intel_mark_busy(struct drm_device *dev);
> +bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj);
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>  			struct intel_ring_buffer *ring);
>  void intel_mark_idle(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4649bf5..64fbab5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -207,6 +207,57 @@ intel_emit_post_sync_nonzero_flush(struct intel_ring_buffer *ring)
>  	return 0;
>  }
>  
> +static int gen6_blt_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, GEN6_BLITTER_ECOSKPD);
> +	if (ring->fbc_address != -1)
> +		intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
> +	else
> +		intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
> +	intel_ring_advance(ring);
> +
> +	ring->fbc_address_dirty = false;
> +
> +	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)
> @@ -256,6 +307,9 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
>  	intel_ring_emit(ring, 0);
>  	intel_ring_advance(ring);
>  
> +	if (invalidate_domains)
> +		return gen6_render_fbc_tracking(ring);
> +
>  	return 0;
>  }
>  
> @@ -1839,7 +1893,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
>  	}
>  	intel_ring_advance(ring);
>  
> -	if (IS_GEN7(dev) && !invalidate && flush)
> +	if (invalidate)
> +		return gen6_blt_fbc_tracking(ring);
> +	else if (flush && IS_GEN7(dev))
>  		return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 71a73f4..1e5bbd6 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -143,8 +143,10 @@ struct  intel_ring_buffer {
>  	 */
>  	struct drm_i915_gem_request *preallocated_lazy_request;
>  	u32 outstanding_lazy_seqno;
> +	u32 fbc_address;
>  	bool gpu_caches_dirty;
>  	bool fbc_dirty;
> +	bool fbc_address_dirty;
>  
>  	wait_queue_head_t irq_queue;
>  
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 06/10] drm/i915: Implement LRI based FBC tracking

[Chris Wilson]

On Wed, Nov 20, 2013 at 02:55:57PM -0800, Rodrigo Vivi wrote:
> On Wed, Nov 06, 2013 at 11:02:21PM +0200, ville.syrjala@linux.intel.com wrote:
> >  static void
> > +i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring,
> > +				   struct list_head *vmas)
> > +{
> > +	struct i915_vma *vma;
> > +	struct drm_i915_gem_object *fbc_obj = NULL;
> > +	u32 fbc_address = -1;
> > +
> > +	list_for_each_entry(vma, vmas, exec_list) {
> > +		struct drm_i915_gem_object *obj = vma->obj;
> > +
> > +		if (obj->base.pending_write_domain &&
> > +		    intel_fb_obj_has_fbc(obj)) {
> > +			WARN_ON(fbc_obj && fbc_obj != obj);
> > +			fbc_obj = obj;
> > +		}
> > +	}
> > +
> > +	if (fbc_obj)
> > +		fbc_address = i915_gem_obj_ggtt_offset(fbc_obj);
> > +
> > +	/* need to nuke/cache_clean on IVB+? */
> > +	ring->fbc_dirty = fbc_obj != NULL;
> > +
> > +	/* need to update FBC tracking? */
> > +	ring->fbc_address_dirty = fbc_address != ring->fbc_address;
> > +	ring->fbc_address = fbc_address;

There's a risk here that we restart the execbuffer and on the second
pass we no longer treat the fbc_address as requiring an update.
ring->fb_address_dirty |= fbc_address != ring->fbc_address
wins for paranoia, and also makes the ordering with set_context a
non-issue.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 06/10] drm/i915: Implement LRI based FBC tracking

[Rodrigo Vivi]

ops, I just noticed that by mistake I replied the v1-series....
but I actually looked to v2 seires... Sorry about that

On Wed, Nov 20, 2013 at 3:17 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Nov 20, 2013 at 02:55:57PM -0800, Rodrigo Vivi wrote:
>> On Wed, Nov 06, 2013 at 11:02:21PM +0200, ville.syrjala@linux.intel.com wrote:
>> >  static void
>> > +i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring,
>> > +                              struct list_head *vmas)
>> > +{
>> > +   struct i915_vma *vma;
>> > +   struct drm_i915_gem_object *fbc_obj = NULL;
>> > +   u32 fbc_address = -1;
>> > +
>> > +   list_for_each_entry(vma, vmas, exec_list) {
>> > +           struct drm_i915_gem_object *obj = vma->obj;
>> > +
>> > +           if (obj->base.pending_write_domain &&
>> > +               intel_fb_obj_has_fbc(obj)) {
>> > +                   WARN_ON(fbc_obj && fbc_obj != obj);
>> > +                   fbc_obj = obj;
>> > +           }
>> > +   }
>> > +
>> > +   if (fbc_obj)
>> > +           fbc_address = i915_gem_obj_ggtt_offset(fbc_obj);
>> > +
>> > +   /* need to nuke/cache_clean on IVB+? */
>> > +   ring->fbc_dirty = fbc_obj != NULL;
>> > +
>> > +   /* need to update FBC tracking? */
>> > +   ring->fbc_address_dirty = fbc_address != ring->fbc_address;
>> > +   ring->fbc_address = fbc_address;
>
> There's a risk here that we restart the execbuffer and on the second
> pass we no longer treat the fbc_address as requiring an update.
> ring->fb_address_dirty |= fbc_address != ring->fbc_address
> wins for paranoia, and also makes the ordering with set_context a
> non-issue.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH v4 06/10] drm/i915: Implement LRI based FBC tracking

[ville.syrjala]

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

As per the SNB and HSW PM guides, we should enable FBC render/blitter
tracking only during batches targetting the front buffer.

On SNB we must also update the FBC render tracking address whenever it
changes. And since the register in question is stored in the context,
we need to make sure we reload it with correct data after context
switches.

On IVB/HSW we use the render nuke mechanism, so no render tracking
address updates are needed. Hoever on the blitter side we need to
enable the blitter tracking like on SNB, and in addition we need
to issue the cache clean messages, which we already did.

v2: Introduce intel_fb_obj_has_fbc()
    Fix crtc locking around crtc->fb access
    Drop a hunk that was included by accident in v1
    Set fbc_address_dirty=false not true after emitting the LRI
v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
    need to upset lockdep anymore
v4: Use |= instead of = to update fbc_address_dirty

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c    |  7 ++++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c       | 17 +++++++--
 drivers/gpu/drm/i915/intel_drv.h           |  1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c    | 58 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  2 ++
 6 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 2ec122a..4b55471 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -402,6 +402,13 @@ mi_set_context(struct intel_ring_buffer *ring,
 
 	intel_ring_advance(ring);
 
+	/*
+	 * FBC RT address is stored in the context, so we may have just
+	 * restored it to an old value. Make sure we emit a new LRI
+	 * to update the address.
+	 */
+	ring->fbc_address_dirty = true;
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 885d595..2d96edf 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -886,6 +886,35 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
 }
 
 static void
+i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring,
+				   struct list_head *vmas)
+{
+	struct i915_vma *vma;
+	struct drm_i915_gem_object *fbc_obj = NULL;
+	u32 fbc_address = -1;
+
+	list_for_each_entry(vma, vmas, exec_list) {
+		struct drm_i915_gem_object *obj = vma->obj;
+
+		if (obj->base.pending_write_domain &&
+		    intel_fb_obj_has_fbc(obj)) {
+			WARN_ON(fbc_obj && fbc_obj != obj);
+			fbc_obj = obj;
+		}
+	}
+
+	if (fbc_obj)
+		fbc_address = i915_gem_obj_ggtt_offset(fbc_obj);
+
+	/* need to nuke/cache_clean on IVB+? */
+	ring->fbc_dirty = fbc_obj != NULL;
+
+	/* need to update FBC tracking? */
+	ring->fbc_address_dirty |= fbc_address != ring->fbc_address;
+	ring->fbc_address = fbc_address;
+}
+
+static void
 i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 				   struct intel_ring_buffer *ring)
 {
@@ -1150,6 +1179,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
 		i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
 
+	i915_gem_execbuffer_mark_fbc_dirty(ring, &eb->vmas);
+
 	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
 	if (ret)
 		goto err;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 326ceca..4155814 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8152,6 +8152,21 @@ void intel_mark_idle(struct drm_device *dev)
 		gen6_rps_idle(dev->dev_private);
 }
 
+bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* check for potential scanout */
+	if (!obj->pin_display)
+		return false;
+
+	if (!dev_priv->fbc.fb)
+		return false;
+
+	return to_intel_framebuffer(dev_priv->fbc.fb)->obj == obj;
+}
+
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_ring_buffer *ring)
 {
@@ -8169,8 +8184,6 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			continue;
 
 		intel_increase_pllclock(crtc);
-		if (ring && intel_fbc_enabled(dev))
-			ring->fbc_dirty = true;
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0231281..119bb95 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -625,6 +625,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 /* intel_display.c */
 int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
+bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj);
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_ring_buffer *ring);
 void intel_mark_idle(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b65f4d7..2e62d76 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -207,6 +207,57 @@ intel_emit_post_sync_nonzero_flush(struct intel_ring_buffer *ring)
 	return 0;
 }
 
+static int gen6_blt_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, GEN6_BLITTER_ECOSKPD);
+	if (ring->fbc_address != -1)
+		intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
+	else
+		intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
+	intel_ring_advance(ring);
+
+	ring->fbc_address_dirty = false;
+
+	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)
@@ -256,6 +307,9 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, 0);
 	intel_ring_advance(ring);
 
+	if (invalidate_domains)
+		return gen6_render_fbc_tracking(ring);
+
 	return 0;
 }
 
@@ -1840,7 +1894,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
 	}
 	intel_ring_advance(ring);
 
-	if (IS_GEN7(dev) && !invalidate && flush)
+	if (invalidate)
+		return gen6_blt_fbc_tracking(ring);
+	else if (flush && IS_GEN7(dev))
 		return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 71a73f4..1e5bbd6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -143,8 +143,10 @@ struct  intel_ring_buffer {
 	 */
 	struct drm_i915_gem_request *preallocated_lazy_request;
 	u32 outstanding_lazy_seqno;
+	u32 fbc_address;
 	bool gpu_caches_dirty;
 	bool fbc_dirty;
+	bool fbc_address_dirty;
 
 	wait_queue_head_t irq_queue;
 
-- 
1.8.3.2

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

Re: [PATCH v4 06/10] drm/i915: Implement LRI based FBC tracking

[Chris Wilson]

On Thu, Nov 21, 2013 at 01:14:10PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> As per the SNB and HSW PM guides, we should enable FBC render/blitter
> tracking only during batches targetting the front buffer.
> 
> On SNB we must also update the FBC render tracking address whenever it
> changes. And since the register in question is stored in the context,
> we need to make sure we reload it with correct data after context
> switches.
> 
> On IVB/HSW we use the render nuke mechanism, so no render tracking
> address updates are needed. Hoever on the blitter side we need to
> enable the blitter tracking like on SNB, and in addition we need
> to issue the cache clean messages, which we already did.
> 
> v2: Introduce intel_fb_obj_has_fbc()
>     Fix crtc locking around crtc->fb access
>     Drop a hunk that was included by accident in v1
>     Set fbc_address_dirty=false not true after emitting the LRI
> v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
>     need to upset lockdep anymore
> v4: Use |= instead of = to update fbc_address_dirty

Ah, should we do the same for fbc_dirty? In the past we could dispatch a
batchbuffer but fail to add the request (and so fail to flush the
rendering/fbc). We currently preallocate the request so that failure
path is history, but we will more than likely be caught out again in the
future.

Like pc8, I'd like for a device (or crtc if you must) property whether
or not indirect rendering is preferred.

Other than that and the bikeshed to kill the redundant fbc_obj local
variable and pack the dirty bits, it looks good to me.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v4 06/10] drm/i915: Implement LRI based FBC tracking

[Ville Syrjälä]

On Thu, Nov 21, 2013 at 11:49:47AM +0000, Chris Wilson wrote:
> On Thu, Nov 21, 2013 at 01:14:10PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > As per the SNB and HSW PM guides, we should enable FBC render/blitter
> > tracking only during batches targetting the front buffer.
> > 
> > On SNB we must also update the FBC render tracking address whenever it
> > changes. And since the register in question is stored in the context,
> > we need to make sure we reload it with correct data after context
> > switches.
> > 
> > On IVB/HSW we use the render nuke mechanism, so no render tracking
> > address updates are needed. Hoever on the blitter side we need to
> > enable the blitter tracking like on SNB, and in addition we need
> > to issue the cache clean messages, which we already did.
> > 
> > v2: Introduce intel_fb_obj_has_fbc()
> >     Fix crtc locking around crtc->fb access
> >     Drop a hunk that was included by accident in v1
> >     Set fbc_address_dirty=false not true after emitting the LRI
> > v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
> >     need to upset lockdep anymore
> > v4: Use |= instead of = to update fbc_address_dirty
> 
> Ah, should we do the same for fbc_dirty? In the past we could dispatch a
> batchbuffer but fail to add the request (and so fail to flush the
> rendering/fbc). We currently preallocate the request so that failure
> path is history, but we will more than likely be caught out again in the
> future.

I guess we could do it for fbc_dirty as well, but I don't think that
should actually change anything. Either we're rendering to the FBC
scanout buffer, or we're not.

I did start pondering if I should actually move the fbc_address to live
under the context once that's where it actually belongs. If we'd track
it per-context we might be able to avoid emitting the LRI for every
context switch.

> 
> Like pc8, I'd like for a device (or crtc if you must) property whether
> or not indirect rendering is preferred.
> 
> Other than that and the bikeshed to kill the redundant fbc_obj local
> variable and pack the dirty bits, it looks good to me.

Actually I just fired it up for real on SNB and it failed. The problem
is that we end up doing the invalidate before the context switch. So
we've not yet forced fbc_address_dirty=true due to the context switch
when we do the invalidate. The most straightforward fix would be to
simply move i915_gem_execbuffer_move_to_gpu() to be called after
i915_switch_context(). I did that on my machine and now it passes my
context related FBC tests. But I do wonder if the order of these
operations was chose for a reason, and whether the reordering might
cause other problems.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v4 06/10] drm/i915: Implement LRI based FBC tracking

[Chris Wilson]

On Thu, Nov 21, 2013 at 06:33:21PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 21, 2013 at 11:49:47AM +0000, Chris Wilson wrote:
> > On Thu, Nov 21, 2013 at 01:14:10PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > As per the SNB and HSW PM guides, we should enable FBC render/blitter
> > > tracking only during batches targetting the front buffer.
> > > 
> > > On SNB we must also update the FBC render tracking address whenever it
> > > changes. And since the register in question is stored in the context,
> > > we need to make sure we reload it with correct data after context
> > > switches.
> > > 
> > > On IVB/HSW we use the render nuke mechanism, so no render tracking
> > > address updates are needed. Hoever on the blitter side we need to
> > > enable the blitter tracking like on SNB, and in addition we need
> > > to issue the cache clean messages, which we already did.
> > > 
> > > v2: Introduce intel_fb_obj_has_fbc()
> > >     Fix crtc locking around crtc->fb access
> > >     Drop a hunk that was included by accident in v1
> > >     Set fbc_address_dirty=false not true after emitting the LRI
> > > v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
> > >     need to upset lockdep anymore
> > > v4: Use |= instead of = to update fbc_address_dirty
> > 
> > Ah, should we do the same for fbc_dirty? In the past we could dispatch a
> > batchbuffer but fail to add the request (and so fail to flush the
> > rendering/fbc). We currently preallocate the request so that failure
> > path is history, but we will more than likely be caught out again in the
> > future.
> 
> I guess we could do it for fbc_dirty as well, but I don't think that
> should actually change anything. Either we're rendering to the FBC
> scanout buffer, or we're not.

The scenario I worry about here is the missing flush after the
rendering. It has been possible for us to lose it (under memory
pressure, other constaints etc) and to issue a catch-up flush on the
next batch. Without the or, we'd lose the FBC flush. It is just a
potential issue for the future.
 
> I did start pondering if I should actually move the fbc_address to live
> under the context once that's where it actually belongs. If we'd track
> it per-context we might be able to avoid emitting the LRI for every
> context switch.
> 
> > 
> > Like pc8, I'd like for a device (or crtc if you must) property whether
> > or not indirect rendering is preferred.
> > 
> > Other than that and the bikeshed to kill the redundant fbc_obj local
> > variable and pack the dirty bits, it looks good to me.
> 
> Actually I just fired it up for real on SNB and it failed. The problem
> is that we end up doing the invalidate before the context switch. So
> we've not yet forced fbc_address_dirty=true due to the context switch
> when we do the invalidate. The most straightforward fix would be to
> simply move i915_gem_execbuffer_move_to_gpu() to be called after
> i915_switch_context(). I did that on my machine and now it passes my
> context related FBC tests. But I do wonder if the order of these
> operations was chose for a reason, and whether the reordering might
> cause other problems.

Ben, opinion on whether the ordering was just convenience?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v4 06/10] drm/i915: Implement LRI based FBC tracking

[Ben Widawsky]

On Thu, Nov 21, 2013 at 04:39:43PM +0000, Chris Wilson wrote:
> On Thu, Nov 21, 2013 at 06:33:21PM +0200, Ville Syrjälä wrote:
> > On Thu, Nov 21, 2013 at 11:49:47AM +0000, Chris Wilson wrote:
> > > On Thu, Nov 21, 2013 at 01:14:10PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > As per the SNB and HSW PM guides, we should enable FBC render/blitter
> > > > tracking only during batches targetting the front buffer.
> > > > 
> > > > On SNB we must also update the FBC render tracking address whenever it
> > > > changes. And since the register in question is stored in the context,
> > > > we need to make sure we reload it with correct data after context
> > > > switches.
> > > > 
> > > > On IVB/HSW we use the render nuke mechanism, so no render tracking
> > > > address updates are needed. Hoever on the blitter side we need to
> > > > enable the blitter tracking like on SNB, and in addition we need
> > > > to issue the cache clean messages, which we already did.
> > > > 
> > > > v2: Introduce intel_fb_obj_has_fbc()
> > > >     Fix crtc locking around crtc->fb access
> > > >     Drop a hunk that was included by accident in v1
> > > >     Set fbc_address_dirty=false not true after emitting the LRI
> > > > v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
> > > >     need to upset lockdep anymore
> > > > v4: Use |= instead of = to update fbc_address_dirty
> > > 
> > > Ah, should we do the same for fbc_dirty? In the past we could dispatch a
> > > batchbuffer but fail to add the request (and so fail to flush the
> > > rendering/fbc). We currently preallocate the request so that failure
> > > path is history, but we will more than likely be caught out again in the
> > > future.
> > 
> > I guess we could do it for fbc_dirty as well, but I don't think that
> > should actually change anything. Either we're rendering to the FBC
> > scanout buffer, or we're not.
> 
> The scenario I worry about here is the missing flush after the
> rendering. It has been possible for us to lose it (under memory
> pressure, other constaints etc) and to issue a catch-up flush on the
> next batch. Without the or, we'd lose the FBC flush. It is just a
> potential issue for the future.
>  
> > I did start pondering if I should actually move the fbc_address to live
> > under the context once that's where it actually belongs. If we'd track
> > it per-context we might be able to avoid emitting the LRI for every
> > context switch.
> > 
> > > 
> > > Like pc8, I'd like for a device (or crtc if you must) property whether
> > > or not indirect rendering is preferred.
> > > 
> > > Other than that and the bikeshed to kill the redundant fbc_obj local
> > > variable and pack the dirty bits, it looks good to me.
> > 
> > Actually I just fired it up for real on SNB and it failed. The problem
> > is that we end up doing the invalidate before the context switch. So
> > we've not yet forced fbc_address_dirty=true due to the context switch
> > when we do the invalidate. The most straightforward fix would be to
> > simply move i915_gem_execbuffer_move_to_gpu() to be called after
> > i915_switch_context(). I did that on my machine and now it passes my
> > context related FBC tests. But I do wonder if the order of these
> > operations was chose for a reason, and whether the reordering might
> > cause other problems.
> 
> Ben, opinion on whether the ordering was just convenience?
> -Chris

If it was anything but convenience, I've long since forgotten. I guess
as long as we do it in two patches we can always bisect, and it's all
good.

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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

[PATCH v5 06/10] drm/i915: Implement LRI based FBC tracking

[ville.syrjala]

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

As per the SNB and HSW PM guides, we should enable FBC render/blitter
tracking only during batches targetting the front buffer.

On SNB we must also update the FBC render tracking address whenever it
changes. And since the register in question is stored in the context,
we need to make sure we reload it with correct data after context
switches.

On IVB/HSW we use the render nuke mechanism, so no render tracking
address updates are needed. Hoever on the blitter side we need to
enable the blitter tracking like on SNB, and in addition we need
to issue the cache clean messages, which we already did.

v2: Introduce intel_fb_obj_has_fbc()
    Fix crtc locking around crtc->fb access
    Drop a hunk that was included by accident in v1
    Set fbc_address_dirty=false not true after emitting the LRI
v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
    need to upset lockdep anymore
v4: Use |= instead of = to update fbc_address_dirty
v5: |= for fbc_dirty too, kill fbc_obj variable, pack the
    intel_ringbuffer dirty bits using bitfields, skip ILK_FBC_RT_BASE
    write on SNB+, kill sandybridge_blit_fbc_update(), reorganize
    code to make future ILK FBC RT LRI support easier

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c    |  7 ++++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 28 ++++++++++++++
 drivers/gpu/drm/i915/intel_display.c       | 17 ++++++++-
 drivers/gpu/drm/i915/intel_drv.h           |  1 +
 drivers/gpu/drm/i915/intel_pm.c            | 26 +------------
 drivers/gpu/drm/i915/intel_ringbuffer.c    | 59 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h    |  6 ++-
 7 files changed, 115 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 4187704..44a1c62 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -402,6 +402,13 @@ mi_set_context(struct intel_ring_buffer *ring,
 
 	intel_ring_advance(ring);
 
+	/*
+	 * FBC RT address is stored in the context, so we may have just
+	 * restored it to an old value. Make sure we emit a new LRI
+	 * to update the address.
+	 */
+	ring->fbc_address_dirty = true;
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3c90dd1..8db3d87 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -889,6 +889,32 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
 }
 
 static void
+i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring,
+				   struct list_head *vmas)
+{
+	struct i915_vma *vma;
+	u32 fbc_address = -1;
+
+	list_for_each_entry(vma, vmas, exec_list) {
+		struct drm_i915_gem_object *obj = vma->obj;
+
+		if (obj->base.pending_write_domain &&
+		    intel_fb_obj_has_fbc(obj)) {
+			WARN_ON(fbc_address != -1 &&
+				fbc_address != i915_gem_obj_ggtt_offset(obj));
+			fbc_address = i915_gem_obj_ggtt_offset(obj);
+		}
+	}
+
+	/* need to nuke/cache_clean on IVB+? */
+	ring->fbc_dirty |= fbc_address != -1;
+
+	/* need to update FBC tracking? */
+	ring->fbc_address_dirty |= fbc_address != ring->fbc_address;
+	ring->fbc_address = fbc_address;
+}
+
+static void
 i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 				   struct intel_ring_buffer *ring)
 {
@@ -1153,6 +1179,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
 		i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
 
+	i915_gem_execbuffer_mark_fbc_dirty(ring, &eb->vmas);
+
 	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
 	if (ret)
 		goto err;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7e7348f..1400df2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8157,6 +8157,21 @@ void intel_mark_idle(struct drm_device *dev)
 		gen6_rps_idle(dev->dev_private);
 }
 
+bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* check for potential scanout */
+	if (!obj->pin_display)
+		return false;
+
+	if (!dev_priv->fbc.fb)
+		return false;
+
+	return to_intel_framebuffer(dev_priv->fbc.fb)->obj == obj;
+}
+
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_ring_buffer *ring)
 {
@@ -8174,8 +8189,6 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			continue;
 
 		intel_increase_pllclock(crtc);
-		if (ring && intel_fbc_enabled(dev))
-			ring->fbc_dirty = true;
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0231281..119bb95 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -625,6 +625,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 /* intel_display.c */
 int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
+bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj);
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_ring_buffer *ring);
 void intel_mark_idle(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ed35c35..5619898 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -187,26 +187,6 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
 	return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
-static void sandybridge_blit_fbc_update(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 blt_ecoskpd;
-
-	/* Make sure blitter notifies FBC of writes */
-	gen6_gt_force_wake_get(dev_priv);
-	blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
-	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
-		GEN6_BLITTER_LOCK_SHIFT;
-	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
-	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
-	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
-	blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
-			 GEN6_BLITTER_LOCK_SHIFT);
-	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
-	POSTING_READ(GEN6_BLITTER_ECOSKPD);
-	gen6_gt_force_wake_put(dev_priv);
-}
-
 static void ironlake_enable_fbc(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				unsigned long interval)
@@ -234,7 +214,8 @@ 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);
-	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
+	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);
 
@@ -242,7 +223,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
 		I915_WRITE(SNB_DPFC_CTL_SA,
 			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
-		sandybridge_blit_fbc_update(dev);
 	}
 
 	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
@@ -299,8 +279,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc,
 		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 	I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
 
-	sandybridge_blit_fbc_update(dev);
-
 	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 69589e4..426d868 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -51,6 +51,57 @@ 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)
+{
+	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_blt_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, GEN6_BLITTER_ECOSKPD);
+	if (ring->fbc_address != -1)
+		intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
+	else
+		intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
+	intel_ring_advance(ring);
+
+	ring->fbc_address_dirty = false;
+
+	return 0;
+}
+
 static int
 gen2_render_ring_flush(struct intel_ring_buffer *ring,
 		       u32	invalidate_domains,
@@ -256,6 +307,9 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, 0);
 	intel_ring_advance(ring);
 
+	if (invalidate_domains)
+		return gen6_render_fbc_tracking(ring);
+
 	return 0;
 }
 
@@ -298,6 +352,7 @@ static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring, u32 value)
 	intel_ring_advance(ring);
 
 	ring->fbc_dirty = false;
+
 	return 0;
 }
 
@@ -1833,7 +1888,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
 	}
 	intel_ring_advance(ring);
 
-	if (IS_GEN7(dev) && !invalidate && flush)
+	if (invalidate)
+		return gen6_blt_fbc_tracking(ring);
+	else if (flush && IS_GEN7(dev))
 		return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 71a73f4..e19d7d3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -143,8 +143,10 @@ struct  intel_ring_buffer {
 	 */
 	struct drm_i915_gem_request *preallocated_lazy_request;
 	u32 outstanding_lazy_seqno;
-	bool gpu_caches_dirty;
-	bool fbc_dirty;
+	u32 fbc_address;
+	bool gpu_caches_dirty:1;
+	bool fbc_dirty:1;
+	bool fbc_address_dirty:1;
 
 	wait_queue_head_t irq_queue;
 
-- 
1.8.3.2

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

Re: [PATCH v5 06/10] drm/i915: Implement LRI based FBC tracking

[Chris Wilson]

On Wed, Nov 27, 2013 at 05:22:55PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> As per the SNB and HSW PM guides, we should enable FBC render/blitter
> tracking only during batches targetting the front buffer.
> 
> On SNB we must also update the FBC render tracking address whenever it
> changes. And since the register in question is stored in the context,
> we need to make sure we reload it with correct data after context
> switches.
> 
> On IVB/HSW we use the render nuke mechanism, so no render tracking
> address updates are needed. Hoever on the blitter side we need to
> enable the blitter tracking like on SNB, and in addition we need
> to issue the cache clean messages, which we already did.
> 
> v2: Introduce intel_fb_obj_has_fbc()
>     Fix crtc locking around crtc->fb access
>     Drop a hunk that was included by accident in v1
>     Set fbc_address_dirty=false not true after emitting the LRI
> v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
>     need to upset lockdep anymore
> v4: Use |= instead of = to update fbc_address_dirty
> v5: |= for fbc_dirty too, kill fbc_obj variable, pack the
>     intel_ringbuffer dirty bits using bitfields, skip ILK_FBC_RT_BASE
>     write on SNB+, kill sandybridge_blit_fbc_update(), reorganize
>     code to make future ILK FBC RT LRI support easier
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I'm content with the patch itself. I'd like to know what impact it has,
but I can live with my belief that it has to better than what we have
right now.

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 06/10] drm/i915: Implement LRI based FBC tracking

[Rodrigo Vivi]

actually just ignore my last msg... alternate between gmail and mutt
confused me...



On Wed, Nov 6, 2013 at 1:02 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> As per the SNB and HSW PM guides, we should enable FBC render/blitter
> tracking only during batches targetting the front buffer.
>
> On SNB we must also update the FBC render tracking address whenever it
> changes. And since the register in question is stored in the context,
> we need to make sure we reload it with correct data after context
> switches.
>
> On IVB/HSW we use the render nuke mechanism, so no render tracking
> address updates are needed. Hoever on the blitter side we need to
> enable the blitter tracking like on SNB, and in addition we need
> to issue the cache clean messages, which we already did.
>
> v2: Introduce intel_fb_obj_has_fbc()
>     Fix crtc locking around crtc->fb access
>     Drop a hunk that was included by accident in v1
>     Set fbc_address_dirty=false not true after emitting the LRI
> v3: Now that fbc hangs on to the fb intel_fb_obj_has_fbc() doesn't
>     need to upset lockdep anymore
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c    |  7 ++++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c       | 17 +++++++--
>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c    | 58 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  2 ++
>  6 files changed, 113 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 72a3df3..d438ea1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -404,6 +404,13 @@ mi_set_context(struct intel_ring_buffer *ring,
>
>         intel_ring_advance(ring);
>
> +       /*
> +        * FBC RT address is stored in the context, so we may have just
> +        * restored it to an old value. Make sure we emit a new LRI
> +        * to update the address.
> +        */
> +       ring->fbc_address_dirty = true;
> +
>         return ret;
>  }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 885d595..db25158 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -886,6 +886,35 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
>  }
>
>  static void
> +i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring,
> +                                  struct list_head *vmas)
> +{
> +       struct i915_vma *vma;
> +       struct drm_i915_gem_object *fbc_obj = NULL;
> +       u32 fbc_address = -1;
> +
> +       list_for_each_entry(vma, vmas, exec_list) {
> +               struct drm_i915_gem_object *obj = vma->obj;
> +
> +               if (obj->base.pending_write_domain &&
> +                   intel_fb_obj_has_fbc(obj)) {
> +                       WARN_ON(fbc_obj && fbc_obj != obj);
> +                       fbc_obj = obj;
> +               }
> +       }
> +
> +       if (fbc_obj)
> +               fbc_address = i915_gem_obj_ggtt_offset(fbc_obj);
> +
> +       /* need to nuke/cache_clean on IVB+? */
> +       ring->fbc_dirty = fbc_obj != NULL;
> +
> +       /* need to update FBC tracking? */
> +       ring->fbc_address_dirty = fbc_address != ring->fbc_address;
> +       ring->fbc_address = fbc_address;
> +}
> +
> +static void
>  i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>                                    struct intel_ring_buffer *ring)
>  {
> @@ -1150,6 +1179,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>         if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
>                 i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
>
> +       i915_gem_execbuffer_mark_fbc_dirty(ring, &eb->vmas);
> +
>         ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
>         if (ret)
>                 goto err;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bce6e07..c29e9d4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8074,6 +8074,21 @@ void intel_mark_idle(struct drm_device *dev)
>                 gen6_rps_idle(dev->dev_private);
>  }
>
> +bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj)
> +{
> +       struct drm_device *dev = obj->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       /* check for potential scanout */
> +       if (!obj->pin_display)
> +               return false;
> +
> +       if (!dev_priv->fbc.fb)
> +               return false;
> +
> +       return to_intel_framebuffer(dev_priv->fbc.fb)->obj == obj;
> +}
> +
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>                         struct intel_ring_buffer *ring)
>  {
> @@ -8091,8 +8106,6 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>                         continue;
>
>                 intel_increase_pllclock(crtc);
> -               if (ring && intel_fbc_enabled(dev))
> -                       ring->fbc_dirty = true;
>         }
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6d701e7..5c7e8b4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -614,6 +614,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  /* intel_display.c */
>  int intel_pch_rawclk(struct drm_device *dev);
>  void intel_mark_busy(struct drm_device *dev);
> +bool intel_fb_obj_has_fbc(struct drm_i915_gem_object *obj);
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>                         struct intel_ring_buffer *ring);
>  void intel_mark_idle(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4649bf5..64fbab5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -207,6 +207,57 @@ intel_emit_post_sync_nonzero_flush(struct intel_ring_buffer *ring)
>         return 0;
>  }
>
> +static int gen6_blt_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, GEN6_BLITTER_ECOSKPD);
> +       if (ring->fbc_address != -1)
> +               intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
> +       else
> +               intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
> +       intel_ring_advance(ring);
> +
> +       ring->fbc_address_dirty = false;
> +
> +       return 0;
> +}
> +
> +static int gen6_render_fbc_tracking(struct intel_ring_buffer *ring)
> +{
> +       int ret;
> +
> +       if (!ring->fbc_address_dirty)
> +               return 0;

Anyway I forgot to say that I noticed that even if FBC is disabled it
will do at least once this LRI

> +
> +       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)
> @@ -256,6 +307,9 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring,
>         intel_ring_emit(ring, 0);
>         intel_ring_advance(ring);
>
> +       if (invalidate_domains)
> +               return gen6_render_fbc_tracking(ring);
> +
>         return 0;
>  }
>
> @@ -1839,7 +1893,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
>         }
>         intel_ring_advance(ring);
>
> -       if (IS_GEN7(dev) && !invalidate && flush)
> +       if (invalidate)
> +               return gen6_blt_fbc_tracking(ring);
> +       else if (flush && IS_GEN7(dev))
>                 return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
>
>         return 0;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 71a73f4..1e5bbd6 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -143,8 +143,10 @@ struct  intel_ring_buffer {
>          */
>         struct drm_i915_gem_request *preallocated_lazy_request;
>         u32 outstanding_lazy_seqno;
> +       u32 fbc_address;
>         bool gpu_caches_dirty;
>         bool fbc_dirty;
> +       bool fbc_address_dirty;
>
>         wait_queue_head_t irq_queue;
>
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH v2 07/10] drm/i915: Kill sandybridge_blit_fbc_update()

[ville.syrjala]

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

No longer needed since the LRIs do the work.

v2: Rebased due to hunk getting dropped from an ealier patch

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dc65bb4..3f600ba 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -187,26 +187,6 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
 	return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
-static void sandybridge_blit_fbc_update(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 blt_ecoskpd;
-
-	/* Make sure blitter notifies FBC of writes */
-	gen6_gt_force_wake_get(dev_priv);
-	blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
-	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
-		GEN6_BLITTER_LOCK_SHIFT;
-	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
-	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
-	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
-	blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
-			 GEN6_BLITTER_LOCK_SHIFT);
-	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
-	POSTING_READ(GEN6_BLITTER_ECOSKPD);
-	gen6_gt_force_wake_put(dev_priv);
-}
-
 static void ironlake_enable_fbc(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				unsigned long interval)
@@ -240,7 +220,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
 		I915_WRITE(SNB_DPFC_CTL_SA,
 			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
-		sandybridge_blit_fbc_update(dev);
 	}
 
 	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
@@ -297,8 +276,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc,
 		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 	I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
 
-	sandybridge_blit_fbc_update(dev);
-
 	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
 }
 
-- 
1.8.1.5

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

Re: [PATCH v2 07/10] drm/i915: Kill sandybridge_blit_fbc_update()

[Rodrigo Vivi]

On Wed, Nov 06, 2013 at 11:02:22PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> No longer needed since the LRIs do the work.

Shouldn't this be part of previous patch?

> 
> v2: Rebased due to hunk getting dropped from an ealier patch
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 23 -----------------------
>  1 file changed, 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index dc65bb4..3f600ba 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -187,26 +187,6 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
>  	return I915_READ(DPFC_CONTROL) & DPFC_CTL_EN;
>  }
>  
> -static void sandybridge_blit_fbc_update(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 blt_ecoskpd;
> -
> -	/* Make sure blitter notifies FBC of writes */
> -	gen6_gt_force_wake_get(dev_priv);
> -	blt_ecoskpd = I915_READ(GEN6_BLITTER_ECOSKPD);
> -	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY <<
> -		GEN6_BLITTER_LOCK_SHIFT;
> -	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> -	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
> -	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> -	blt_ecoskpd &= ~(GEN6_BLITTER_FBC_NOTIFY <<
> -			 GEN6_BLITTER_LOCK_SHIFT);
> -	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> -	POSTING_READ(GEN6_BLITTER_ECOSKPD);
> -	gen6_gt_force_wake_put(dev_priv);
> -}
> -
>  static void ironlake_enable_fbc(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				unsigned long interval)
> @@ -240,7 +220,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
>  		I915_WRITE(SNB_DPFC_CTL_SA,
>  			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>  		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> -		sandybridge_blit_fbc_update(dev);
>  	}
>  
>  	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
> @@ -297,8 +276,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc,
>  		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>  	I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
>  
> -	sandybridge_blit_fbc_update(dev);
> -
>  	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
>  }
>  
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 08/10] drm/i915: Don't write ILK/IVB_FBC_RT_BASE directly

[ville.syrjala]

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

We use LRIs to enable/disable the render tracking as needed. So leave
ILK_FBC_RT_BASE alone when enabling FBC on SNB.

While at it, kill the IVB_FBC_RT_BASE completely since we don't use
render tracking on IVB+.

TODO: Make ILK use the LRI mechanism too?

v2: Drop the IVB_FBC_RT_BASE write too

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3f600ba..4f5293e7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -212,7 +212,8 @@ 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);
-	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
+	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);
 
@@ -257,8 +258,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	I915_WRITE(IVB_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj));
-
 	I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
 		   IVB_DPFC_CTL_FENCE_EN |
 		   intel_crtc->plane << IVB_DPFC_CTL_PLANE_SHIFT);
-- 
1.8.1.5

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

Re: [PATCH v2 08/10] drm/i915: Don't write ILK/IVB_FBC_RT_BASE directly

[Rodrigo Vivi]

On Wed, Nov 06, 2013 at 11:02:23PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We use LRIs to enable/disable the render tracking as needed. So leave
> ILK_FBC_RT_BASE alone when enabling FBC on SNB.

Shouldn't this be part of patch 6
> 
> While at it, kill the IVB_FBC_RT_BASE completely since we don't use
> render tracking on IVB+.

and this a new patch?

> 
> TODO: Make ILK use the LRI mechanism too?
> 
> v2: Drop the IVB_FBC_RT_BASE write too
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3f600ba..4f5293e7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -212,7 +212,8 @@ 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);
> -	I915_WRITE(ILK_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj) | ILK_FBC_RT_VALID);
> +	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);
>  
> @@ -257,8 +258,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc,
>  	struct drm_i915_gem_object *obj = intel_fb->obj;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> -	I915_WRITE(IVB_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj));
> -
>  	I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
>  		   IVB_DPFC_CTL_FENCE_EN |
>  		   intel_crtc->plane << IVB_DPFC_CTL_PLANE_SHIFT);
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 08/10] drm/i915: Don't write IVB_FBC_RT_BASE

[ville.syrjala]

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

We use nuking instead of render tracking on IVB+, so there's
no point in writing IVB_FBC_RT_BASE.

v2: Drop the IVB_FBC_RT_BASE write too
v3: Move the SNB stuff elsewhere, leaving only IVB+ here

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 5619898..059245e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -260,8 +260,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	I915_WRITE(IVB_FBC_RT_BASE, i915_gem_obj_ggtt_offset(obj));
-
 	I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
 		   IVB_DPFC_CTL_FENCE_EN |
 		   intel_crtc->plane << IVB_DPFC_CTL_PLANE_SHIFT);
-- 
1.8.3.2

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

Re: [PATCH v3 08/10] drm/i915: Don't write IVB_FBC_RT_BASE

[Chris Wilson]

On Wed, Nov 27, 2013 at 05:24:05PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We use nuking instead of render tracking on IVB+, so there's
> no point in writing IVB_FBC_RT_BASE.
> 
> v2: Drop the IVB_FBC_RT_BASE write too
> v3: Move the SNB stuff elsewhere, leaving only IVB+ here
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

You've convinced me.
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 09/10] drm/i915: Set has_fbc=true for all SNB+, except VLV

[ville.syrjala]

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

At least since SNB (perhaps even earlier) even the desktop parts
should have FBC.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c0ab5d4..9b8c9bf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -265,6 +265,7 @@ static const struct intel_device_info intel_ironlake_m_info = {
 static const struct intel_device_info intel_sandybridge_d_info = {
 	.gen = 6, .num_pipes = 2,
 	.need_gfx_hws = 1, .has_hotplug = 1,
+	.has_fbc = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING,
 	.has_llc = 1,
 };
@@ -280,6 +281,7 @@ static const struct intel_device_info intel_sandybridge_m_info = {
 #define GEN7_FEATURES  \
 	.gen = 7, .num_pipes = 3, \
 	.need_gfx_hws = 1, .has_hotplug = 1, \
+	.has_fbc = 1, \
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
 	.has_llc = 1
 
@@ -292,7 +294,6 @@ static const struct intel_device_info intel_ivybridge_m_info = {
 	GEN7_FEATURES,
 	.is_ivybridge = 1,
 	.is_mobile = 1,
-	.has_fbc = 1,
 };
 
 static const struct intel_device_info intel_ivybridge_q_info = {
@@ -307,6 +308,7 @@ static const struct intel_device_info intel_valleyview_m_info = {
 	.num_pipes = 2,
 	.is_valleyview = 1,
 	.display_mmio_offset = VLV_DISPLAY_BASE,
+	.has_fbc = 0, /* legal, last one wins */
 	.has_llc = 0, /* legal, last one wins */
 };
 
@@ -315,6 +317,7 @@ static const struct intel_device_info intel_valleyview_d_info = {
 	.num_pipes = 2,
 	.is_valleyview = 1,
 	.display_mmio_offset = VLV_DISPLAY_BASE,
+	.has_fbc = 0, /* legal, last one wins */
 	.has_llc = 0, /* legal, last one wins */
 };
 
@@ -332,7 +335,6 @@ static const struct intel_device_info intel_haswell_m_info = {
 	.is_mobile = 1,
 	.has_ddi = 1,
 	.has_fpga_dbg = 1,
-	.has_fbc = 1,
 	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
 };
 
-- 
1.8.1.5

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

Re: [PATCH 09/10] drm/i915: Set has_fbc=true for all SNB+, except VLV

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Wed, Nov 06, 2013 at 11:02:24PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> At least since SNB (perhaps even earlier) even the desktop parts
> should have FBC.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c0ab5d4..9b8c9bf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -265,6 +265,7 @@ static const struct intel_device_info intel_ironlake_m_info = {
>  static const struct intel_device_info intel_sandybridge_d_info = {
>  	.gen = 6, .num_pipes = 2,
>  	.need_gfx_hws = 1, .has_hotplug = 1,
> +	.has_fbc = 1,
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING,
>  	.has_llc = 1,
>  };
> @@ -280,6 +281,7 @@ static const struct intel_device_info intel_sandybridge_m_info = {
>  #define GEN7_FEATURES  \
>  	.gen = 7, .num_pipes = 3, \
>  	.need_gfx_hws = 1, .has_hotplug = 1, \
> +	.has_fbc = 1, \
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING, \
>  	.has_llc = 1
>  
> @@ -292,7 +294,6 @@ static const struct intel_device_info intel_ivybridge_m_info = {
>  	GEN7_FEATURES,
>  	.is_ivybridge = 1,
>  	.is_mobile = 1,
> -	.has_fbc = 1,
>  };
>  
>  static const struct intel_device_info intel_ivybridge_q_info = {
> @@ -307,6 +308,7 @@ static const struct intel_device_info intel_valleyview_m_info = {
>  	.num_pipes = 2,
>  	.is_valleyview = 1,
>  	.display_mmio_offset = VLV_DISPLAY_BASE,
> +	.has_fbc = 0, /* legal, last one wins */
>  	.has_llc = 0, /* legal, last one wins */
>  };
>  
> @@ -315,6 +317,7 @@ static const struct intel_device_info intel_valleyview_d_info = {
>  	.num_pipes = 2,
>  	.is_valleyview = 1,
>  	.display_mmio_offset = VLV_DISPLAY_BASE,
> +	.has_fbc = 0, /* legal, last one wins */
>  	.has_llc = 0, /* legal, last one wins */
>  };
>  
> @@ -332,7 +335,6 @@ static const struct intel_device_info intel_haswell_m_info = {
>  	.is_mobile = 1,
>  	.has_ddi = 1,
>  	.has_fpga_dbg = 1,
> -	.has_fbc = 1,
>  	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
>  };
>  
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/10] drm/i915: Use plane_name() in gen7_enable_fbc()

[ville.syrjala]

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

All the other .enable_fbc() funcs use plane_name(). Make
gen7_enable_fbc() do the same.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4f5293e7..48f6eda 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -275,7 +275,7 @@ static void gen7_enable_fbc(struct drm_crtc *crtc,
 		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 	I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
 
-	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
+	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
 }
 
 bool intel_fbc_enabled(struct drm_device *dev)
-- 
1.8.1.5

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

Re: [PATCH 10/10] drm/i915: Use plane_name() in gen7_enable_fbc()

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Wed, Nov 06, 2013 at 11:02:25PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> All the other .enable_fbc() funcs use plane_name(). Make
> gen7_enable_fbc() do the same.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4f5293e7..48f6eda 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -275,7 +275,7 @@ static void gen7_enable_fbc(struct drm_crtc *crtc,
>  		   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>  	I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
>  
> -	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
> +	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
>  }
>  
>  bool intel_fbc_enabled(struct drm_device *dev)
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/10] drm/i915: Use plane_name() in gen7_enable_fbc()

[Daniel Vetter]

On Wed, Nov 20, 2013 at 03:01:03PM -0800, Rodrigo Vivi wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> 
> On Wed, Nov 06, 2013 at 11:02:25PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > All the other .enable_fbc() funcs use plane_name(). Make
> > gen7_enable_fbc() do the same.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've picked up a few of the reviewed patches here. Overall we still have
the issue that enabling the fbc tracking totally wreaks havoc with the
blitter performance on gen6+ ... I still think the way to fix that is to
do s/w based tracking and forgo all the neat hw support.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 10/10] drm/i915: Use plane_name() in gen7_enable_fbc()

[Chris Wilson]

On Thu, Nov 21, 2013 at 09:08:44AM +0100, Daniel Vetter wrote:
> On Wed, Nov 20, 2013 at 03:01:03PM -0800, Rodrigo Vivi wrote:
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > 
> > On Wed, Nov 06, 2013 at 11:02:25PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > All the other .enable_fbc() funcs use plane_name(). Make
> > > gen7_enable_fbc() do the same.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I've picked up a few of the reviewed patches here. Overall we still have
> the issue that enabling the fbc tracking totally wreaks havoc with the
> blitter performance on gen6+ ... I still think the way to fix that is to
> do s/w based tracking and forgo all the neat hw support.

For the record, I noticed that FBC was enabled on Haswell because of the
dramatic impact it had on RENDER performance...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 10/10] drm/i915: Use plane_name() in gen7_enable_fbc()

[Ville Syrjälä]

On Thu, Nov 21, 2013 at 09:08:44AM +0100, Daniel Vetter wrote:
> On Wed, Nov 20, 2013 at 03:01:03PM -0800, Rodrigo Vivi wrote:
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > 
> > On Wed, Nov 06, 2013 at 11:02:25PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > All the other .enable_fbc() funcs use plane_name(). Make
> > > gen7_enable_fbc() do the same.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I've picked up a few of the reviewed patches here. Overall we still have
> the issue that enabling the fbc tracking totally wreaks havoc with the
> blitter performance on gen6+ ... I still think the way to fix that is to
> do s/w based tracking and forgo all the neat hw support.

Maybe we can do both. The tracking code is not that intrusive IMO, so
giving the user a choice would make it easier to figure out if there
is a performance vs. power tradeoff between hardware and software
tracking.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 00/10] drm/i915: FBC fixes v2

[Rodrigo Vivi]

(just noticed mutt behind tsocks missed this email)

On Wed, Nov 6, 2013 at 1:02 PM,  <ville.syrjala@linux.intel.com> wrote:
> One more attempt at making FBC suck a bit less.
>
> The main thing as before is getting the LRI based render/blitter
> tracking in place.
>
> In this updates series I decided the way to avoid the kms locks in
> execbuffer is to keep an extra reference to the fb.
>
> I added a bunch of locking (struct_mutex and crtc->mutex) to some
> places to hopefully make it bit less racy. I think/hope that it doesn't
> make it more likely to deadlock. We do grab both locks in the fbc work
> now, and we do cancel the work while holding at least one of the locks,
> so it seems a bit bad. But we cancel the work in a lazy fashion (no _sync
> or anything) so I don't think it should deadlock due to that. And we
> already grabbed struct_mutex there anyway, and we already held it while
> cancelling the work, so it's no worse at least :P
>
> The disable_fbc/update_fbc calls from the page flip code are lacking
> kms lock protection, but we can't simply add it there due to possibility
> of deadlocks.
>
> So at least the render/blitter tracking seems to work now without
> upsetting lockdep, so it seems a bit better than what we had at least.
> Maybe someone else will get inspired by this and fix this mess up for
> real...
>
> I've only run this on my IVB, and just running X + some apps w/o
> a compositor. No extensive tests or anything. But if I frob some
> debug register to disable some tracking bits the screen gets
> corrupted, so it seems to be doing its job.
>
> I pushed it here in case someone is interested in a git tree:
> git://gitorious.org/vsyrjala/linux.git fbc_fixes

I did quick test here on my IVB as well and everything worked fine.
I was afraid that this could regress 1 bug on gnome environment fixed
by that lri but it didn't! :)

Also I think this series fixes:
https://bugs.freedesktop.org/show_bug.cgi?id=65396

And if I'm wrong about not any and all frontbuffer rend this also
closes jira 3018.

Besides that: Some time ago I started a fbc rework getting it tied to
pipe_config, but than Daniel suggested that maybe it would be better
to put fbc and psr in new plane_config you were doing instead of
pipe_config. What do you think about it?
And what the latest on plane_config?

Thanks,
Rodrigo.

>
> Ville Syrjälä (10):
>       drm/i915: Use plane_name() in gen7_enable_fbc()
>       drm/i915: Grab struct_mutex around all FBC updates
>       drm/i915: Have FBC keep references to the fb
>       drm/i915: Grab crtc->mutex in intel_fbc_work_fn()
>       drm/i915: Limit FBC flush to post batch flush
>       drm/i915: Emit SRM after the MSG_FBC_REND_STATE LRI
>       drm/i915: Implement LRI based FBC tracking
>       drm/i915: Kill sandybridge_blit_fbc_update()
>       drm/i915: Don't write ILK/IVB_FBC_RT_BASE directly
>       drm/i915: Set has_fbc=true for all SNB+, except VLV
>
>  drivers/gpu/drm/i915/i915_drv.c            |  6 +-
>  drivers/gpu/drm/i915/i915_drv.h            |  6 +-
>  drivers/gpu/drm/i915/i915_gem_context.c    |  7 +++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++
>  drivers/gpu/drm/i915/i915_reg.h            |  1 +
>  drivers/gpu/drm/i915/intel_display.c       | 27 +++++++-
>  drivers/gpu/drm/i915/intel_drv.h           |  1 +
>  drivers/gpu/drm/i915/intel_pm.c            | 98 ++++++++++++++++--------------
>  drivers/gpu/drm/i915/intel_ringbuffer.c    | 66 ++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h    |  2 +
>  10 files changed, 188 insertions(+), 57 deletions(-)
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br