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

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

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