Re: [PATCH 3/6] drm/i915: Implement LRI based FBC tracking

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

On Wed, Nov 06, 2013 at 04:06:29PM +0000, Chris Wilson wrote:
> On Wed, Nov 06, 2013 at 05:39:02PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > As per the SNB and HSW PM guides, we should enable FBC render/blitter
> > tracking only during batches targetting the front buffer.
> > 
> > On SNB we must also update the FBC render tracking address whenever it
> > changes. And since the register in question is stored in the context,
> > we need to make sure we reload it with correct data after context
> > switches.
> > 
> > On IVB/HSW we use the render nuke mechanism, so no render tracking
> > address updates are needed. Hoever on the blitter side we need to
> > enable the blitter tracking like on SNB, and in addition we need
> > to issue the cache clean messages, which we already did.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c    |  7 ++++
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c       | 28 +++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h           |  3 ++
> >  drivers/gpu/drm/i915/intel_pm.c            |  2 --
> >  drivers/gpu/drm/i915/intel_ringbuffer.c    | 58 +++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_ringbuffer.h    |  2 ++
> >  7 files changed, 126 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 72a3df3..d438ea1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -404,6 +404,13 @@ mi_set_context(struct intel_ring_buffer *ring,
> >  
> >  	intel_ring_advance(ring);
> >  
> > +	/*
> > +	 * FBC RT address is stored in the context, so we may have just
> > +	 * restored it to an old value. Make sure we emit a new LRI
> > +	 * to update the address.
> > +	 */
> > +	ring->fbc_address_dirty = true;
> > +
> >  	return ret;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 885d595..a8f8634 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -886,6 +886,35 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
> >  }
> >  
> >  static void
> > +i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring,
> > +				   struct list_head *vmas)
> > +{
> > +	struct i915_vma *vma;
> > +	struct drm_i915_gem_object *fbc_obj = NULL;
> > +	u32 fbc_address = -1;
> > +
> > +	list_for_each_entry(vma, vmas, exec_list) {
> > +		struct drm_i915_gem_object *obj = vma->obj;
> > +
> > +		if (obj->base.pending_write_domain == 0)
> > +			continue;
> > +
> > +		if (obj->pin_count) /* check for potential scanout */
> > +			intel_mark_fbc_dirty(obj, ring, &fbc_obj);
> 
> if (obj->pin_display && intel_fbc_active(obj))
> 	fbc_address = i915_gem_obj_ggtt_offset(obj);

Ah yeah, pin_display is good. Not sure I like intel_fbc_active() much.
But it needs to get replaced w/ some pipe config stuff or something
anyway eventually. I do already check fbc.plane == crtc->plane in
intel_mark_fbc_dirty() which should catch the case when fbc is disabled
anyway.

> 
> > +	}
> > +
> > +	if (fbc_obj)
> > +		fbc_address = i915_gem_obj_ggtt_offset(fbc_obj);
> > +
> > +	/* need to nuke/cache_clean on IVB+? */
> > +	ring->fbc_dirty = fbc_obj != NULL;
> > +
> > +	/* need to update FBC tracking? */
> > +	ring->fbc_address_dirty = fbc_address != ring->fbc_address;
> 
> I'm not going to mention the unused bits in fbc_address....

Just preparing myself for full 32bit addres space :)

> 
> > +	ring->fbc_address = fbc_address;
> > +}
> > +
> > +static void
> >  i915_gem_execbuffer_move_to_active(struct list_head *vmas,
> >  				   struct intel_ring_buffer *ring)
> >  {
> > @@ -1150,6 +1179,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  	if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping)
> >  		i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level);
> >  
> > +	i915_gem_execbuffer_mark_fbc_dirty(ring, &eb->vmas);
> > +
> >  	ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas);
> >  	if (ret)
> >  		goto err;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 12cf362..49c40aa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8064,6 +8064,32 @@ void intel_mark_idle(struct drm_device *dev)
> >  		gen6_rps_idle(dev->dev_private);
> >  }
> >  
> > +void intel_mark_fbc_dirty(struct drm_i915_gem_object *obj,
> > +			  struct intel_ring_buffer *ring,
> > +			  struct drm_i915_gem_object **fbc_obj)
> > +{
> > +	struct drm_device *dev = obj->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc;
> > +
> > +	if (!i915_powersave)
> > +		return;
> > +
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		if (!crtc->fb)
> > +			continue;
> > +
> > +		if (dev_priv->fbc.plane != to_intel_crtc(crtc)->plane)
> > +			continue;
> > +
> > +		if (to_intel_framebuffer(crtc->fb)->obj != obj)
> > +			continue;
> > +
> > +		WARN_ON(*fbc_obj && *fbc_obj != obj);
> > +		*fbc_obj = obj;
> > +	}
> > +}
> > +
> >  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> >  			struct intel_ring_buffer *ring)
> >  {
> > @@ -8081,8 +8107,6 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> >  			continue;
> >  
> >  		intel_increase_pllclock(crtc);
> > -		if (ring && intel_fbc_enabled(dev))
> > -			ring->fbc_dirty = true;
> >  	}
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 6d701e7..5841f60 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -614,6 +614,9 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> >  /* intel_display.c */
> >  int intel_pch_rawclk(struct drm_device *dev);
> >  void intel_mark_busy(struct drm_device *dev);
> > +void intel_mark_fbc_dirty(struct drm_i915_gem_object *obj,
> > +			  struct intel_ring_buffer *ring,
> > +			  struct drm_i915_gem_object **fbc_obj);
> >  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> >  			struct intel_ring_buffer *ring);
> >  void intel_mark_idle(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index dc306e1..bfec348 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);
> 
> You can kill this write here, right?

I'm killing the whole function later. This was a squash accident since
I had Ben's earlier patches on the bottom originally. I'll drop this
hunk.

> 
> 
> >  	blt_ecoskpd |= GEN6_BLITTER_FBC_NOTIFY;
> >  	I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd);
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 4649bf5..43f7eab 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -207,6 +207,57 @@ intel_emit_post_sync_nonzero_flush(struct intel_ring_buffer *ring)
> >  	return 0;
> >  }
> >  
> > +static int gen6_blt_fbc_tracking(struct intel_ring_buffer *ring)
> > +{
> > +	int ret;
> > +
> > +	if (!ring->fbc_address_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);
> > +	if (ring->fbc_address != -1)
> > +		intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY));
> > +	else
> > +		intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY));
> > +	intel_ring_advance(ring);
> > +
> > +	ring->fbc_address_dirty = true;
> 
> false, surely?

Oops, yeah. And now I need to go and re-rest to make sure that didn't
make it look like it works even when it doesn't. But it shouldn't
since we recompute it from scratch for every execbuffer.

-- 
Ville Syrjälä
Intel OTC