From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 5/6] drm/i915: Do remaps for all contexts Date: Wed, 18 Sep 2013 10:48:49 +0300 Message-ID: <20130918074849.GB4531@intel.com> References: <1379477575-2164-1-git-send-email-benjamin.widawsky@intel.com> <1379477575-2164-5-git-send-email-benjamin.widawsky@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id D2E41E5C9B for ; Wed, 18 Sep 2013 00:48:53 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1379477575-2164-5-git-send-email-benjamin.widawsky@intel.com> 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: Ben Widawsky Cc: Intel GFX , Bryan Bell , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Tue, Sep 17, 2013 at 09:12:46PM -0700, Ben Widawsky wrote: > On both Ivybridge and Haswell, row remapping information is saved and > restored with context. This means, we never actually properly supported > the l3 remapping because our sysfs interface is asynchronous (and not > tied to any context), and the known faulty HW would be reused by the > next context to run. > = > Not that due to the asynchronous nature of the sysfs entry, there is no > point modifying the registers for the existing context. Instead we set a > flag for all contexts to load the correct remapping information on the > next run. Interested clients can use debugfs to determine whether or not > the row has been remapped. > = > One could propose at this point that we just do the remapping in the > kernel. I guess since we have to maintain the sysfs interface anyway, > I'm not sure how useful it is, and I do like keeping the policy in > userspace; (it wasn't my original decision to make the > interface the way it is, so I'm not attached). > = > v2: Force a context switch when we have a remap on the next switch. > (Ville) > Don't let userspace use the interface with disabled contexts. > = > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_debugfs.c | 8 +++++++ > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem_context.c | 22 ++++++++++++++++---- > drivers/gpu/drm/i915/i915_sysfs.c | 37 +++++++++++++--------------= ------ > 4 files changed, 41 insertions(+), 27 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i= 915_debugfs.c > index ada0950..80bed69 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -145,6 +145,13 @@ describe_obj(struct seq_file *m, struct drm_i915_gem= _object *obj) > seq_printf(m, " (%s)", obj->ring->name); > } > = > +static void describe_ctx(struct seq_file *m, struct i915_hw_context *ctx) > +{ > + seq_putc(m, ctx->is_initialized ? 'I' : 'i'); > + seq_putc(m, ctx->remap_slice ? 'R' : 'r'); > + seq_putc(m, ' '); > +} > + > static int i915_gem_object_list_info(struct seq_file *m, void *data) > { > struct drm_info_node *node =3D (struct drm_info_node *) m->private; > @@ -1463,6 +1470,7 @@ static int i915_context_status(struct seq_file *m, = void *unused) > = > list_for_each_entry(ctx, &dev_priv->context_list, link) { > seq_puts(m, "HW context "); > + describe_ctx(m, ctx); > for_each_ring(ring, dev_priv, i) > if (ring->default_context =3D=3D ctx) > seq_printf(m, "(default context %s) ", ring->name); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index 1795927..015df52 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -601,6 +601,7 @@ struct i915_hw_context { > struct kref ref; > int id; > bool is_initialized; > + uint8_t remap_slice; > struct drm_i915_file_private *file_priv; > struct intel_ring_buffer *ring; > struct drm_i915_gem_object *obj; > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i9= 15/i915_gem_context.c > index 2bbdce8..7e138cc 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -140,7 +140,7 @@ create_hw_context(struct drm_device *dev, > { > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct i915_hw_context *ctx; > - int ret; > + int ret, i; > = > ctx =3D kzalloc(sizeof(*ctx), GFP_KERNEL); > if (ctx =3D=3D NULL) > @@ -181,6 +181,8 @@ create_hw_context(struct drm_device *dev, > = > ctx->file_priv =3D file_priv; > ctx->id =3D ret; > + for (i =3D 0; i < NUM_L3_SLICES(dev); i++) > + ctx->remap_slice |=3D 1 << 1; ^ Still broken. > = > return ctx; > = > @@ -396,11 +398,11 @@ static int do_switch(struct i915_hw_context *to) > struct intel_ring_buffer *ring =3D to->ring; > struct i915_hw_context *from =3D ring->last_context; > u32 hw_flags =3D 0; > - int ret; > + int ret, i; > = > BUG_ON(from !=3D NULL && from->obj !=3D NULL && from->obj->pin_count = =3D=3D 0); > = > - if (from =3D=3D to) > + if (from =3D=3D to && !to->remap_slice) > return 0; > = > ret =3D i915_gem_obj_ggtt_pin(to->obj, CONTEXT_ALIGN, false, false); > @@ -423,7 +425,7 @@ static int do_switch(struct i915_hw_context *to) > = > if (!to->is_initialized || is_default_context(to)) > hw_flags |=3D MI_RESTORE_INHIBIT; > - else if (WARN_ON_ONCE(from =3D=3D to)) /* not yet expected */ > + else if (from =3D=3D to) > hw_flags |=3D MI_FORCE_RESTORE; Hmm. Why do we need to restore actually? Could just leave MI_SET_CONTEXT to be effectively a nop I think. > = > ret =3D mi_set_context(ring, to, hw_flags); > @@ -432,6 +434,18 @@ static int do_switch(struct i915_hw_context *to) > return ret; > } > = > + for (i =3D 0; i < MAX_L3_SLICES; i++) { > + if (!(to->remap_slice & (1< + continue; > + > + ret =3D i915_gem_l3_remap(ring, i); > + if (!ret) { > + to->remap_slice &=3D ~(1< + /* If it failed, try again next round */ > + DRM_DEBUG_DRIVER("L3 remapping failed\n"); > + } The debug message is on the wrong branch. > + } > + > /* The backing object for the context is done after switching to the > * *next* context. Therefore we cannot retire the previous context until > * the next context has already started running. In fact, the below code > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i91= 5_sysfs.c > index b07bdfb..deb8787 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -118,9 +118,8 @@ i915_l3_read(struct file *filp, struct kobject *kobj, > struct drm_minor *dminor =3D container_of(dev, struct drm_minor, kdev); > struct drm_device *drm_dev =3D dminor->dev; > struct drm_i915_private *dev_priv =3D drm_dev->dev_private; > - uint32_t misccpctl; > int slice =3D (int)(uintptr_t)attr->private; > - int i, ret; > + int ret; > = > count =3D round_down(count, 4); > = > @@ -134,26 +133,13 @@ i915_l3_read(struct file *filp, struct kobject *kob= j, > if (ret) > return ret; > = > - if (IS_HASWELL(drm_dev)) { > - if (dev_priv->l3_parity.remap_info[slice]) > - memcpy(buf, > - dev_priv->l3_parity.remap_info[slice] + (offset/4), > - count); > - else > - memset(buf, 0, count); > - > - goto out; > - } > - > - misccpctl =3D I915_READ(GEN7_MISCCPCTL); > - I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE); > - > - for (i =3D 0; i < count; i +=3D 4) > - *((uint32_t *)(&buf[i])) =3D I915_READ(GEN7_L3LOG_BASE + offset + i); > - > - I915_WRITE(GEN7_MISCCPCTL, misccpctl); > + if (dev_priv->l3_parity.remap_info[slice]) > + memcpy(buf, > + dev_priv->l3_parity.remap_info[slice] + (offset/4), > + count); > + else > + memset(buf, 0, count); > = > -out: > mutex_unlock(&drm_dev->struct_mutex); > = > return count; > @@ -168,6 +154,7 @@ i915_l3_write(struct file *filp, struct kobject *kobj, > struct drm_minor *dminor =3D container_of(dev, struct drm_minor, kdev); > struct drm_device *drm_dev =3D dminor->dev; > struct drm_i915_private *dev_priv =3D drm_dev->dev_private; > + struct i915_hw_context *ctx; > u32 *temp =3D NULL; /* Just here to make handling failures easy */ > int slice =3D (int)(uintptr_t)attr->private; > int ret; > @@ -176,6 +163,9 @@ i915_l3_write(struct file *filp, struct kobject *kobj, > if (ret) > return ret; > = > + if (dev_priv->hw_contexts_disabled) > + return -ENXIO; > + > ret =3D i915_mutex_lock_interruptible(drm_dev); > if (ret) > return ret; > @@ -204,8 +194,9 @@ i915_l3_write(struct file *filp, struct kobject *kobj, > = > memcpy(dev_priv->l3_parity.remap_info[slice] + (offset/4), buf, count); > = > - if (i915_gem_l3_remap(&dev_priv->ring[RCS], slice)) > - count =3D 0; > + /* NB: We defer the remapping until we switch to the context */ > + list_for_each_entry(ctx, &dev_priv->context_list, link) > + ctx->remap_slice |=3D (1< = > mutex_unlock(&drm_dev->struct_mutex); > = > -- = > 1.8.4 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC