[Intel-gfx] [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 at 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 at chromium.org>
>> Signed-off-by: Ben Widawsky <ben at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br