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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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