[Intel-gfx] [PATCH v3] drm/i915: Replaced Blitter ring based flips with MMIO flips for VLV

[Ville Syrjälä]

On Fri, May 09, 2014 at 02:59:42PM +0300, Ville Syrjälä wrote:
> On Sun, Mar 23, 2014 at 02:31:05PM +0530, sourab.gupta at intel.com wrote:
> > From: Sourab Gupta <sourab.gupta at intel.com>
> > 
> > Using MMIO based flips on VLV for Media power well residency optimization.
> > The blitter ring is currently being used just for command streamer based
> > flip calls. For pure 3D workloads, with MMIO flips, there will be no use
> > of blitter ring and this will ensure the 100% residency for Media well.
> 
> Sorry for dragging my feet with reviewing this. I'm hoping this is the
> latest version...
> 
> > 
> > v2: The MMIO flips now use the interrupt driven mechanism for issuing the
> > flips when target seqno is reached. (Incorporating Ville's idea)
> > 
> > v3: Rebasing on latest code. Code restructuring after incorporating
> > Damien's comments
> > 
> > Signed-off-by: Sourab Gupta <sourab.gupta at intel.com>
> > Signed-off-by: Akash Goel <akash.goel at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |   1 +
> >  drivers/gpu/drm/i915/i915_drv.h      |   7 ++
> >  drivers/gpu/drm/i915/i915_irq.c      |   2 +
> >  drivers/gpu/drm/i915/intel_display.c | 124 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |   7 ++
> >  5 files changed, 141 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 4e0a26a..bca3c5a 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1569,6 +1569,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	spin_lock_init(&dev_priv->backlight_lock);
> >  	spin_lock_init(&dev_priv->uncore.lock);
> >  	spin_lock_init(&dev_priv->mm.object_stat_lock);
> > +	spin_lock_init(&dev_priv->mmio_flip_lock);
> >  	mutex_init(&dev_priv->dpio_lock);
> >  	mutex_init(&dev_priv->modeset_restore_lock);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3f62be0..678d31d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1621,6 +1621,10 @@ typedef struct drm_i915_private {
> >  	struct i915_dri1_state dri1;
> >  	/* Old ums support infrastructure, same warning applies. */
> >  	struct i915_ums_state ums;
> > +
> > +	/* protects the mmio flip data */
> > +	spinlock_t mmio_flip_lock;
> > +
> >  } drm_i915_private_t;
> >  
> >  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> > @@ -2657,6 +2661,9 @@ int i915_reg_read_ioctl(struct drm_device *dev, void *data,
> >  int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
> >  			       struct drm_file *file);
> >  
> > +void intel_notify_mmio_flip(struct drm_device *dev,
> > +			struct intel_ring_buffer *ring);
> > +
> >  /* overlay */
> >  extern struct intel_overlay_error_state *intel_overlay_capture_error_state(struct drm_device *dev);
> >  extern void intel_overlay_print_error_state(struct drm_i915_error_state_buf *e,
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 4b4aeb3..ad26abe 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1080,6 +1080,8 @@ static void notify_ring(struct drm_device *dev,
> >  
> >  	trace_i915_gem_request_complete(ring);
> >  
> > +	intel_notify_mmio_flip(dev, ring);
> > +
> >  	wake_up_all(&ring->irq_queue);
> >  	i915_queue_hangcheck(dev);
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7e4ea8d..19004bf 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8782,6 +8782,125 @@ err:
> >  	return ret;
> >  }
> >  
> > +static int intel_do_mmio_flip(struct drm_device *dev,
> > +			struct drm_crtc *crtc)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc;
> > +
> > +	intel_crtc = to_intel_crtc(crtc);
> 
> nit: could be part of intel_crtc declaration
> 
> > +
> > +	intel_mark_page_flip_active(intel_crtc);
> > +	return dev_priv->display.update_primary_plane(crtc, crtc->fb, 0, 0);
> 
> Needs to pass crtc->{x,y} instead of 0,0.
> 
> I was a bit worried crtc->fb might be changed already at this point, but
> after thinking a bit it should be fine since the presense of unpin_work
> will keep intel_crtc_page_flip() from frobbing with it and we always
> call intel_crtc_wait_for_pending_flips() before set_base.
> 
> Just need to update to use crtc->primary->fb now.
> 
> I'm thinking we also have a small race here with a flip done interrupt
> from a previous set_base. Probably we need to sort it out using the 
> SURFLIVE and/or flip counter like I did for the mmio vs. cs flip
> race. But I need to think on this a bit more. Perhaps you want to also
> look at those patches a bit.

Oh another thing here is that we update_primary_plane() isn't guaranteed
to be atomic. We could start to use the atomic pipe update mechanism here
already but then we need to schedule a work so that we can perform the
vblank evade trick. That's going to be needed for the nuclear flip
anyway.

Another option would be to only update the base address and leave the
offset registers alone just like the CS flip does.

-- 
Ville Syrjälä
Intel OTC