Re: [PATCH v4] drm/i915: Replaced Blitter ring based flips with MMIO flips for VLV

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

On Fri, May 16, 2014 at 12:34:08PM +0000, Gupta, Sourab wrote:
> On Thu, 2014-05-15 at 12:27 +0000, Ville Syrjälä wrote:
> > On Thu, May 15, 2014 at 11:47:37AM +0530, sourab.gupta@intel.com wrote:
> > > From: Sourab Gupta <sourab.gupta@intel.com>
> > > 
> > > Using MMIO based flips on Gen5+ 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.
> > > 
> > > In a subsequent patch, we can make the selection between CS vs MMIO flip
> > > based on a module parameter to give more testing coverage.
> > > 
> > > 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
> > > 
> > > v4: Addressing Ville's review comments
> > >     -general cleanup
> > >     -updating only base addr instead of calling update_primary_plane
> > >     -extending patch for gen5+ platforms
> > > 
> > > Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> > > Signed-off-by: Akash Goel <akash.goel@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c      |   1 +
> > >  drivers/gpu/drm/i915/i915_drv.h      |   6 ++
> > >  drivers/gpu/drm/i915/i915_gem.c      |   2 +-
> > >  drivers/gpu/drm/i915/i915_irq.c      |   2 +
> > >  drivers/gpu/drm/i915/intel_display.c | 115 +++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |   6 ++
> > >  6 files changed, 131 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 46f1dec..672c28f 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1570,6 +1570,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);
> > >  	dev_priv->ring_index = 0;
> > >  	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 4006dfe..38c0820 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1543,6 +1543,8 @@ struct drm_i915_private {
> > >  	struct i915_ums_state ums;
> > >  	/* the indicator for dispatch video commands on two BSD rings */
> > >  	int ring_index;
> > > +	/* protects the mmio flip data */
> > > +	spinlock_t mmio_flip_lock;
> > >  };
> > >  
> > >  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> > > @@ -2209,6 +2211,7 @@ bool i915_gem_retire_requests(struct drm_device *dev);
> > >  void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
> > >  int __must_check i915_gem_check_wedge(struct i915_gpu_error *error,
> > >  				      bool interruptible);
> > > +int i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno);
> > >  static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
> > >  {
> > >  	return unlikely(atomic_read(&error->reset_counter)
> > > @@ -2580,6 +2583,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_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index fa5b5ab..5b4e953 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -975,7 +975,7 @@ i915_gem_check_wedge(struct i915_gpu_error *error,
> > >   * Compare seqno against outstanding lazy request. Emit a request if they are
> > >   * equal.
> > >   */
> > > -static int
> > > +int
> > >  i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno)
> > >  {
> > >  	int ret;
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index b10fbde..a353693 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -1084,6 +1084,8 @@ static void notify_ring(struct drm_device *dev,
> > >  
> > >  	trace_i915_gem_request_complete(ring);
> > >  
> > > +	intel_notify_mmio_flip(dev, ring);
> > > +
> > 
> > Hmm. How badly is this going to explode with UMS?
> 
> Hi Ville,
> It seems there would be a small race between the page filp done intr and
> the flip done interrupt from previous set base. But it seems to be the
> case for CS flips also. In both cases, once we do the
> mark_page_flip_active, there may be a window in which page flip intr
> from previous set base may arrive.
> Have we interpreted the race correctly? Or are we missing something
> here?

Yes. See here for my patches to fix the mmio vs. CS race:
http://lists.freedesktop.org/archives/intel-gfx/2014-April/043759.html
Feel free to review that stuff if you have a bit of time.

I've not had time to think about the mmio vs. mmio case yet. Perhaps my
patches would fix that too?

> 
> Also, notify_mmio_flip is being called from notify_ring function.
> Not sure of the scenario in which it may explode with UMS. Can you
> please elaborate more.

With UMS we have no modeset structures (drm_crtcs and whatnot). So the
crtc list walk will probably explode.

Hmm. I guess we could just init all the mode_config lists even w/ UMS,
so that the code will just see an empty list. Does anyone see any
problems with that?

-- 
Ville Syrjälä
Intel OTC