[PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB

[PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB

[Ben Widawsky]

On Sandybridge we must set the "PPGTT Render Target Base Address Valid
for FBC" bit as noted in the programming guide. We did this at clock
gating init. Thisbit is not saved and restored with RC6 power context,
so the resetting it at ring flush should fix that.

The effect of not doing this should be corruption, and not a hang - as
has so often been the case.

Note that we should actually clear this bit as well when not blitting to
the scanout (using the blitter for other things), or else all operations

Cc: Stéphane Marchesin <marcheu@chromium.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c         |  2 --
 drivers/gpu/drm/i915/intel_ringbuffer.c | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ca9a778..67f460b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -193,8 +193,6 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
 	/* 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);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2dec134..ddd7681 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -278,6 +278,28 @@ gen7_render_ring_cs_stall_wa(struct intel_ring_buffer *ring)
 	return 0;
 }
 
+static int gen6_ring_fbc_flush(struct intel_ring_buffer *ring)
+{
+	int ret;
+
+	if (!ring->fbc_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);
+	intel_ring_emit(ring,
+			_MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
+	intel_ring_advance(ring);
+
+	ring->fbc_dirty = false;
+	return 0;
+}
+
 static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring, u32 value)
 {
 	int ret;
@@ -1712,6 +1734,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_advance(ring);
 
+	if (IS_GEN6(dev) && flush)
+		return gen6_ring_fbc_flush(ring);
+
 	if (IS_GEN7(dev) && flush)
 		return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
 
-- 
1.8.4.2

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

[PATCH 2/2] drm/i915: Disable blt tracking of fbc when not used

[Ben Widawsky]

When not blitting to scanout, we can save some power by not tracking
blits, and more importantly, unnecessarily invalidating lines which we
don't care a bout.

These instructions are explicitly spelled out in the spec, but it is how
I expect it to work. Unfortunately, I do not have power data for this.

Cc: Stéphane Marchesin <marcheu@chromium.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ddd7681..56be8cb 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -296,7 +296,8 @@ static int gen6_ring_fbc_flush(struct intel_ring_buffer *ring)
 			_MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
 	intel_ring_advance(ring);
 
-	ring->fbc_dirty = false;
+	/* We'll mark the fbc clean only after the operation has completed so we
+	 * can track when to disable the bit above */
 	return 0;
 }
 
@@ -642,11 +643,13 @@ gen6_add_request(struct intel_ring_buffer *ring)
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *useless;
-	int i, ret;
+	int i, ret, extra_dwords = 4;
 
+	if (ring->fbc_dirty && ring->id == BCS)
+		extra_dwords += 4;
 	ret = intel_ring_begin(ring, ((I915_NUM_RINGS-1) *
 				      MBOX_UPDATE_DWORDS) +
-				      4);
+				      extra_dwords);
 	if (ret)
 		return ret;
 #undef MBOX_UPDATE_DWORDS
@@ -661,6 +664,18 @@ gen6_add_request(struct intel_ring_buffer *ring)
 	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
 	intel_ring_emit(ring, ring->outstanding_lazy_seqno);
 	intel_ring_emit(ring, MI_USER_INTERRUPT);
+
+	if (ring->fbc_dirty && ring->id == BCS) {
+		intel_ring_emit(ring, MI_NOOP);
+		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+		intel_ring_emit(ring, GEN6_BLITTER_ECOSKPD);
+		intel_ring_emit(ring,
+				_MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
+		__intel_ring_advance(ring);
+
+		ring->fbc_dirty = false;
+	}
+
 	__intel_ring_advance(ring);
 
 	return 0;
-- 
1.8.4.2

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

[PATCH 2/2] [v2] drm/i915: Disable blt tracking of fbc when not used

[Ben Widawsky]

When not blitting to scanout, we can save some power by not tracking
blits, and more importantly, unnecessarily invalidating lines which we
don't care a bout.

These instructions are explicitly spelled out in the spec, but it is how
I expect it to work. Unfortunately, I do not have power data for this.

v2: Don't advance the ring twice (Stéphane)
Rename extra_dwords to dwrods (Stéphane)

Cc: Stéphane Marchesin <marcheu@chromium.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ddd7681..8c6e9b2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -296,7 +296,8 @@ static int gen6_ring_fbc_flush(struct intel_ring_buffer *ring)
 			_MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
 	intel_ring_advance(ring);
 
-	ring->fbc_dirty = false;
+	/* We'll mark the fbc clean only after the operation has completed so we
+	 * can track when to disable the bit above */
 	return 0;
 }
 
@@ -642,11 +643,13 @@ gen6_add_request(struct intel_ring_buffer *ring)
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_ring_buffer *useless;
-	int i, ret;
+	int i, ret, dwords = 4;
 
+	if (ring->fbc_dirty && ring->id == BCS)
+		dwords += 4;
 	ret = intel_ring_begin(ring, ((I915_NUM_RINGS-1) *
 				      MBOX_UPDATE_DWORDS) +
-				      4);
+				      dwords);
 	if (ret)
 		return ret;
 #undef MBOX_UPDATE_DWORDS
@@ -661,6 +664,17 @@ gen6_add_request(struct intel_ring_buffer *ring)
 	intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
 	intel_ring_emit(ring, ring->outstanding_lazy_seqno);
 	intel_ring_emit(ring, MI_USER_INTERRUPT);
+
+	if (ring->fbc_dirty && ring->id == BCS) {
+		intel_ring_emit(ring, MI_NOOP);
+		intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+		intel_ring_emit(ring, GEN6_BLITTER_ECOSKPD);
+		intel_ring_emit(ring,
+				_MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
+
+		ring->fbc_dirty = false;
+	}
+
 	__intel_ring_advance(ring);
 
 	return 0;
-- 
1.8.4.2

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

Re: [PATCH 2/2] [v2] drm/i915: Disable blt tracking of fbc when not used

[Stéphane Marchesin]

On Fri, Nov 1, 2013 at 5:04 PM, Ben Widawsky
<benjamin.widawsky@intel.com> wrote:
> When not blitting to scanout, we can save some power by not tracking
> blits, and more importantly, unnecessarily invalidating lines which we
> don't care a bout.
>
> These instructions are explicitly spelled out in the spec, but it is how
> I expect it to work. Unfortunately, I do not have power data for this.
>
> v2: Don't advance the ring twice (Stéphane)
> Rename extra_dwords to dwrods (Stéphane)
>
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

For both:
Tested-by: Stéphane Marchesin <marcheu@chromium.org>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>

Stéphane

> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ddd7681..8c6e9b2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -296,7 +296,8 @@ static int gen6_ring_fbc_flush(struct intel_ring_buffer *ring)
>                         _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
>         intel_ring_advance(ring);
>
> -       ring->fbc_dirty = false;
> +       /* We'll mark the fbc clean only after the operation has completed so we
> +        * can track when to disable the bit above */
>         return 0;
>  }
>
> @@ -642,11 +643,13 @@ gen6_add_request(struct intel_ring_buffer *ring)
>         struct drm_device *dev = ring->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_ring_buffer *useless;
> -       int i, ret;
> +       int i, ret, dwords = 4;
>
> +       if (ring->fbc_dirty && ring->id == BCS)
> +               dwords += 4;
>         ret = intel_ring_begin(ring, ((I915_NUM_RINGS-1) *
>                                       MBOX_UPDATE_DWORDS) +
> -                                     4);
> +                                     dwords);
>         if (ret)
>                 return ret;
>  #undef MBOX_UPDATE_DWORDS
> @@ -661,6 +664,17 @@ gen6_add_request(struct intel_ring_buffer *ring)
>         intel_ring_emit(ring, I915_GEM_HWS_INDEX << MI_STORE_DWORD_INDEX_SHIFT);
>         intel_ring_emit(ring, ring->outstanding_lazy_seqno);
>         intel_ring_emit(ring, MI_USER_INTERRUPT);
> +
> +       if (ring->fbc_dirty && ring->id == BCS) {
> +               intel_ring_emit(ring, MI_NOOP);
> +               intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +               intel_ring_emit(ring, GEN6_BLITTER_ECOSKPD);
> +               intel_ring_emit(ring,
> +                               _MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
> +
> +               ring->fbc_dirty = false;
> +       }
> +
>         __intel_ring_advance(ring);
>
>         return 0;
> --
> 1.8.4.2
>

Re: [PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB

[Chris Wilson]

On Fri, Nov 01, 2013 at 05:02:52PM -0700, Ben Widawsky wrote:
> On Sandybridge we must set the "PPGTT Render Target Base Address Valid
> for FBC" bit as noted in the programming guide. We did this at clock
> gating init. Thisbit is not saved and restored with RC6 power context,
> so the resetting it at ring flush should fix that.
> 
> The effect of not doing this should be corruption, and not a hang - as
> has so often been the case.
> 
> Note that we should actually clear this bit as well when not blitting to
> the scanout (using the blitter for other things), or else all operations

This needs to be enabled during the invalidate phase, so that the
tracking is active for the batch. However, the fbc_dirty flag is
currently set later.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB

[Ville Syrjälä]

On Fri, Nov 01, 2013 at 05:02:52PM -0700, Ben Widawsky wrote:
> On Sandybridge we must set the "PPGTT Render Target Base Address Valid
> for FBC" bit as noted in the programming guide. We did this at clock
> gating init. Thisbit is not saved and restored with RC6 power context,
> so the resetting it at ring flush should fix that.
> 
> The effect of not doing this should be corruption, and not a hang - as
> has so often been the case.
> 
> Note that we should actually clear this bit as well when not blitting to
> the scanout (using the blitter for other things), or else all operations
> 
> Cc: Stéphane Marchesin <marcheu@chromium.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_pm.c         |  2 --
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 25 +++++++++++++++++++++++++
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ca9a778..67f460b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -193,8 +193,6 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
>  	/* 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);

Why leave the other FBC_NOTIFY frobbing in place? Since you don't set
the mask bit anymore the rest isn't guaranteed to do anything.

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2dec134..ddd7681 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -278,6 +278,28 @@ gen7_render_ring_cs_stall_wa(struct intel_ring_buffer *ring)
>  	return 0;
>  }
>  
> +static int gen6_ring_fbc_flush(struct intel_ring_buffer *ring)
> +{
> +	int ret;
> +
> +	if (!ring->fbc_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);
> +	intel_ring_emit(ring,
> +			_MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
> +	intel_ring_advance(ring);
> +
> +	ring->fbc_dirty = false;
> +	return 0;
> +}
> +
>  static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring, u32 value)
>  {
>  	int ret;
> @@ -1712,6 +1734,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
>  	intel_ring_emit(ring, MI_NOOP);
>  	intel_ring_advance(ring);
>  
> +	if (IS_GEN6(dev) && flush)
> +		return gen6_ring_fbc_flush(ring);
> +

What Chris said about doing this before the batch is dispatched.

Afer a bit of thought I think the following logic would work nicely:

execbuffer() {
	ring->new_fbc_obj = NULL;
	for_each_obj() {
		if (is_crtc_fb(obj) && obj.write_domains)
			ring->new_fbc_obj = obj;
	if (gen >= 7) {
		if (ring->new_fbc_obj)
			ring->fbc_dirty = true;
	} else {
		if (ring->new_fb_obj != ring->fbc_obj) {
			ring->fbc_obj = ring->new_fbc_obj;
			ring->fbc_dirty = true;
		}
	}

	invalidate() {
		if (gen < 7 && ring->fbc_dirty) {
			if (ring->fbc_obj)
				enable_tracking;
			else
				disable_tracking;
		}
	}

	dispatch()

	flush()	{
		if (gen >= 7 && ring->fbc_dirty)
			fbc_nuke();
		ring->fbc_dirty = false;
	}
}

I think that same logic would work for both blitter and render. The
difference between the two is that for render we also need to update
the address, for blitter we just need to set the notify bit.

Also since we could update the render tracking for every batch, the
problem of having the render fbc tracking address in the context
would also be solved by simply setting fbc_dirty=true on context
switch.

I don't recall excatly how we're supposed to do blitter tracking on
on gen7+. I seem to recall that it also had a nuke mechanism, but
I don't see it being used in out code ATM.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB

[Rodrigo Vivi]

I'm just on going with another -collector update and since this patch
fixes a bug I think it would be a good one to include.

But since it was bikeshedded it is better to ask Ville and Chris if
their comments was a NAck or I can consider to get for -collector.

Thanks

On Sat, Nov 2, 2013 at 9:10 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Nov 01, 2013 at 05:02:52PM -0700, Ben Widawsky wrote:
>> On Sandybridge we must set the "PPGTT Render Target Base Address Valid
>> for FBC" bit as noted in the programming guide. We did this at clock
>> gating init. Thisbit is not saved and restored with RC6 power context,
>> so the resetting it at ring flush should fix that.
>>
>> The effect of not doing this should be corruption, and not a hang - as
>> has so often been the case.
>>
>> Note that we should actually clear this bit as well when not blitting to
>> the scanout (using the blitter for other things), or else all operations
>>
>> Cc: Stéphane Marchesin <marcheu@chromium.org>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c         |  2 --
>>  drivers/gpu/drm/i915/intel_ringbuffer.c | 25 +++++++++++++++++++++++++
>>  2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index ca9a778..67f460b 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -193,8 +193,6 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
>>       /* 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);
>
> Why leave the other FBC_NOTIFY frobbing in place? Since you don't set
> the mask bit anymore the rest isn't guaranteed to do anything.
>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 2dec134..ddd7681 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -278,6 +278,28 @@ gen7_render_ring_cs_stall_wa(struct intel_ring_buffer *ring)
>>       return 0;
>>  }
>>
>> +static int gen6_ring_fbc_flush(struct intel_ring_buffer *ring)
>> +{
>> +     int ret;
>> +
>> +     if (!ring->fbc_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);
>> +     intel_ring_emit(ring,
>> +                     _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
>> +     intel_ring_advance(ring);
>> +
>> +     ring->fbc_dirty = false;
>> +     return 0;
>> +}
>> +
>>  static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring, u32 value)
>>  {
>>       int ret;
>> @@ -1712,6 +1734,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
>>       intel_ring_emit(ring, MI_NOOP);
>>       intel_ring_advance(ring);
>>
>> +     if (IS_GEN6(dev) && flush)
>> +             return gen6_ring_fbc_flush(ring);
>> +
>
> What Chris said about doing this before the batch is dispatched.
>
> Afer a bit of thought I think the following logic would work nicely:
>
> execbuffer() {
>         ring->new_fbc_obj = NULL;
>         for_each_obj() {
>                 if (is_crtc_fb(obj) && obj.write_domains)
>                         ring->new_fbc_obj = obj;
>         if (gen >= 7) {
>                 if (ring->new_fbc_obj)
>                         ring->fbc_dirty = true;
>         } else {
>                 if (ring->new_fb_obj != ring->fbc_obj) {
>                         ring->fbc_obj = ring->new_fbc_obj;
>                         ring->fbc_dirty = true;
>                 }
>         }
>
>         invalidate() {
>                 if (gen < 7 && ring->fbc_dirty) {
>                         if (ring->fbc_obj)
>                                 enable_tracking;
>                         else
>                                 disable_tracking;
>                 }
>         }
>
>         dispatch()
>
>         flush() {
>                 if (gen >= 7 && ring->fbc_dirty)
>                         fbc_nuke();
>                 ring->fbc_dirty = false;
>         }
> }
>
> I think that same logic would work for both blitter and render. The
> difference between the two is that for render we also need to update
> the address, for blitter we just need to set the notify bit.
>
> Also since we could update the render tracking for every batch, the
> problem of having the render fbc tracking address in the context
> would also be solved by simply setting fbc_dirty=true on context
> switch.
>
> I don't recall excatly how we're supposed to do blitter tracking on
> on gen7+. I seem to recall that it also had a nuke mechanism, but
> I don't see it being used in out code ATM.
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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

Re: [PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB

[Daniel Vetter]

On Tue, Nov 19, 2013 at 3:08 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> I'm just on going with another -collector update and since this patch
> fixes a bug I think it would be a good one to include.
>
> But since it was bikeshedded it is better to ask Ville and Chris if
> their comments was a NAck or I can consider to get for -collector.

It's more a meh, since as long as we don't enable fbc by default it
won't really benefit upstream much at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB

[Stéphane Marchesin]

On Tue, Nov 19, 2013 at 2:39 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Nov 19, 2013 at 3:08 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>> I'm just on going with another -collector update and since this patch
>> fixes a bug I think it would be a good one to include.
>>
>> But since it was bikeshedded it is better to ask Ville and Chris if
>> their comments was a NAck or I can consider to get for -collector.
>
> It's more a meh, since as long as we don't enable fbc by default it
> won't really benefit upstream much at all.

These two patches fix real FBC issues here, letting us enable it and
save power. I think it's worth considering them for inclusion.

Stéphane

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB

[Ville Syrjälä]

On Mon, Nov 18, 2013 at 06:08:15PM -0800, Rodrigo Vivi wrote:
> I'm just on going with another -collector update and since this patch
> fixes a bug I think it would be a good one to include.
> 
> But since it was bikeshedded it is better to ask Ville and Chris if
> their comments was a NAck or I can consider to get for -collector.

My FBC series makes this obsolete. I think I have a few more updates to
that series that I didn't post yet. I'll try to get those out today.
And I also have a crc based igt for this in the works.

> 
> Thanks
> 
> On Sat, Nov 2, 2013 at 9:10 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Nov 01, 2013 at 05:02:52PM -0700, Ben Widawsky wrote:
> >> On Sandybridge we must set the "PPGTT Render Target Base Address Valid
> >> for FBC" bit as noted in the programming guide. We did this at clock
> >> gating init. Thisbit is not saved and restored with RC6 power context,
> >> so the resetting it at ring flush should fix that.
> >>
> >> The effect of not doing this should be corruption, and not a hang - as
> >> has so often been the case.
> >>
> >> Note that we should actually clear this bit as well when not blitting to
> >> the scanout (using the blitter for other things), or else all operations
> >>
> >> Cc: Stéphane Marchesin <marcheu@chromium.org>
> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >> ---
> >>  drivers/gpu/drm/i915/intel_pm.c         |  2 --
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c | 25 +++++++++++++++++++++++++
> >>  2 files changed, 25 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index ca9a778..67f460b 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -193,8 +193,6 @@ static void sandybridge_blit_fbc_update(struct drm_device *dev)
> >>       /* 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);
> >
> > Why leave the other FBC_NOTIFY frobbing in place? Since you don't set
> > the mask bit anymore the rest isn't guaranteed to do anything.
> >
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index 2dec134..ddd7681 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -278,6 +278,28 @@ gen7_render_ring_cs_stall_wa(struct intel_ring_buffer *ring)
> >>       return 0;
> >>  }
> >>
> >> +static int gen6_ring_fbc_flush(struct intel_ring_buffer *ring)
> >> +{
> >> +     int ret;
> >> +
> >> +     if (!ring->fbc_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);
> >> +     intel_ring_emit(ring,
> >> +                     _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
> >> +     intel_ring_advance(ring);
> >> +
> >> +     ring->fbc_dirty = false;
> >> +     return 0;
> >> +}
> >> +
> >>  static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring, u32 value)
> >>  {
> >>       int ret;
> >> @@ -1712,6 +1734,9 @@ static int gen6_ring_flush(struct intel_ring_buffer *ring,
> >>       intel_ring_emit(ring, MI_NOOP);
> >>       intel_ring_advance(ring);
> >>
> >> +     if (IS_GEN6(dev) && flush)
> >> +             return gen6_ring_fbc_flush(ring);
> >> +
> >
> > What Chris said about doing this before the batch is dispatched.
> >
> > Afer a bit of thought I think the following logic would work nicely:
> >
> > execbuffer() {
> >         ring->new_fbc_obj = NULL;
> >         for_each_obj() {
> >                 if (is_crtc_fb(obj) && obj.write_domains)
> >                         ring->new_fbc_obj = obj;
> >         if (gen >= 7) {
> >                 if (ring->new_fbc_obj)
> >                         ring->fbc_dirty = true;
> >         } else {
> >                 if (ring->new_fb_obj != ring->fbc_obj) {
> >                         ring->fbc_obj = ring->new_fbc_obj;
> >                         ring->fbc_dirty = true;
> >                 }
> >         }
> >
> >         invalidate() {
> >                 if (gen < 7 && ring->fbc_dirty) {
> >                         if (ring->fbc_obj)
> >                                 enable_tracking;
> >                         else
> >                                 disable_tracking;
> >                 }
> >         }
> >
> >         dispatch()
> >
> >         flush() {
> >                 if (gen >= 7 && ring->fbc_dirty)
> >                         fbc_nuke();
> >                 ring->fbc_dirty = false;
> >         }
> > }
> >
> > I think that same logic would work for both blitter and render. The
> > difference between the two is that for render we also need to update
> > the address, for blitter we just need to set the notify bit.
> >
> > Also since we could update the render tracking for every batch, the
> > problem of having the render fbc tracking address in the context
> > would also be solved by simply setting fbc_dirty=true on context
> > switch.
> >
> > I don't recall excatly how we're supposed to do blitter tracking on
> > on gen7+. I seem to recall that it also had a nuke mechanism, but
> > I don't see it being used in out code ATM.
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > 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

-- 
Ville Syrjälä
Intel OTC