From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 4/5] drm/i915: remap l3 on hw init Date: Fri, 25 May 2012 11:41:04 -0700 Message-ID: <20120525114104.6d536b5f@bwidawsk.net> References: <1335573622-10646-1-git-send-email-ben@bwidawsk.net> <1335573622-10646-5-git-send-email-ben@bwidawsk.net> <20120525103957.5440e847@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id DAD209E744 for ; Fri, 25 May 2012 11:41:11 -0700 (PDT) In-Reply-To: <20120525103957.5440e847@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 25 May 2012 10:39:57 -0700 Jesse Barnes wrote: > On Fri, 27 Apr 2012 17:40:20 -0700 > Ben Widawsky wrote: > > > If any l3 rows have been previously remapped, we must remap them after > > GPU reset/resume too. > > > > Signed-off-by: Ben Widawsky > > --- > > drivers/gpu/drm/i915/i915_drv.h | 3 +++ > > drivers/gpu/drm/i915/i915_gem.c | 26 ++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > > 3 files changed, 32 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 9505fc0..e9efe17 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -641,6 +641,8 @@ typedef struct drm_i915_private { > > /** PPGTT used for aliasing the PPGTT with the GTT */ > > struct i915_hw_ppgtt *aliasing_ppgtt; > > > > + u32 *l3_remap_info; > > + > > struct shrinker inactive_shrinker; > > > > /** > > @@ -1290,6 +1292,7 @@ int __must_check i915_gem_object_set_domain(struct drm_i915_gem_object *obj, > > uint32_t write_domain); > > int __must_check i915_gem_object_finish_gpu(struct drm_i915_gem_object *obj); > > int __must_check i915_gem_init_hw(struct drm_device *dev); > > +void i915_gem_l3_remap(struct drm_device *dev); > > void i915_gem_init_swizzling(struct drm_device *dev); > > void i915_gem_init_ppgtt(struct drm_device *dev); > > void i915_gem_cleanup_ringbuffer(struct drm_device *dev); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 7bc4a40..bb3ef9f 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3475,6 +3475,30 @@ i915_gem_idle(struct drm_device *dev) > > return 0; > > } > > > > +void i915_gem_l3_remap(struct drm_device *dev) > > +{ > > + drm_i915_private_t *dev_priv = dev->dev_private; > > + u32 misccpctl; > > + int i; > > + > > + if (!dev_priv->mm.l3_remap_info) > > + return; > > + > > + WARN_ON(!IS_IVYBRIDGE(dev)); > > Should just be a return? I had this here since it's a public function, and we have a history of calling functions on the wrong generation. So it just helps us find out a little sooner we're doing the wrong thing. As you point out below though, this doesn't give the behavior we want. Thanks for catching it. > > > + > > + misccpctl = I915_READ(GEN7_MISCCPCTL); > > + I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE); > > + POSTING_READ(GEN7_MISCCPCTL); > > + > > + for (i = 0; i < GEN7_L3LOG_SIZE; i += 4) > > + I915_WRITE(GEN7_L3LOG_BASE + i, dev_priv->mm.l3_remap_info[i/4]); > > + > > + /* Make sure all the writes land before disabling dop clock gating */ > > + POSTING_READ(GEN7_L3LOG_BASE); > > Same comment as before on the DOP gating... I think we're good here now too. > > > + > > + I915_WRITE(GEN7_MISCCPCTL, misccpctl); > > +} > > + > > void i915_gem_init_swizzling(struct drm_device *dev) > > { > > drm_i915_private_t *dev_priv = dev->dev_private; > > @@ -3566,6 +3590,8 @@ i915_gem_init_hw(struct drm_device *dev) > > drm_i915_private_t *dev_priv = dev->dev_private; > > int ret; > > > > + i915_gem_l3_remap(dev); > > Since this looks unconditional, so we'll get the backtrace on every > non-IVB system? Yes. That seems like a problem. > > > Reviewed-by? -- Ben Widawsky, Intel Open Source Technology Center