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

2013-11-19 Thread 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.syrj...@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 marc...@chromium.org
  Signed-off-by: Ben Widawsky b...@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

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

2013-11-19 Thread Stéphane Marchesin 
On Tue, Nov 19, 2013 at 2:39 AM, Daniel Vetter dan...@ffwll.ch wrote:
 On Tue, Nov 19, 2013 at 3:08 AM, Rodrigo Vivi rodrigo.v...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2013-11-02 Thread 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2013-11-02 Thread 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 marc...@chromium.org
 Signed-off-by: Ben Widawsky b...@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

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

2013-11-01 Thread 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 marc...@chromium.org
Signed-off-by: Ben Widawsky b...@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