On Fri, 2014-05-16 at 12:51 +0000, Ville Syrjälä wrote:
> 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@xxxxxxxxx wrote:
> > > > From: Sourab Gupta <sourab.gupta@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
> > > > ---
> > > > 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?
>
Hi Ville,
We looked at your patch and it looks good to fix the mmio vs mmio case
also.
We would want to incorporate that idea into our patch also.
So, should we send a second patch to address this race, on top of our
base mmio flip patch. Meaning that the second base patch will be
independent, and the second patch will be dependent on your patch. Is
this approach ok? Or should we add it into this patch only, which will
be built on top of your patch?
> >
> > 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?
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
