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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> >> Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
