| Message ID | 1383350573-7427-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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
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
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);
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(-)