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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
