From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v4] drm/i915: Replaced Blitter ring based flips with MMIO flips for VLV Date: Fri, 16 May 2014 15:51:15 +0300 Message-ID: <20140516125115.GR27580@intel.com> References: <20140509171848.GA18465@intel.com> <1400134657-2417-1-git-send-email-sourab.gupta@intel.com> <20140515122746.GA27580@intel.com> <1400243696.9396.66.camel@sourabgu-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 31D7D6E88F for ; Fri, 16 May 2014 05:51:20 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1400243696.9396.66.camel@sourabgu-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Gupta, Sourab" Cc: "intel-gfx@lists.freedesktop.org" , "Goel, Akash" List-Id: intel-gfx@lists.freedesktop.org On Fri, May 16, 2014 at 12:34:08PM +0000, Gupta, Sourab wrote: > On Thu, 2014-05-15 at 12:27 +0000, Ville Syrj=E4l=E4 wrote: > > On Thu, May 15, 2014 at 11:47:37AM +0530, sourab.gupta@intel.com wrote: > > > From: Sourab Gupta > > > = > > > Using MMIO based flips on Gen5+ for Media power well residency optimi= zation. > > > The blitter ring is currently being used just for command streamer ba= sed > > > 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 wel= l. > > > = > > > In a subsequent patch, we can make the selection between CS vs MMIO f= lip > > > 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 > > > Signed-off-by: Akash Goel > > > --- > > > 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/i= 915_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, un= signed 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 =3D 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/i= 915_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_devi= ce *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 *err= or) > > > { > > > 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_st= ate_buf *e, > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i= 915_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/i= 915_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=E4l=E4 Intel OTC