| Message ID | 1444742405-9322-1-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 13, 2015 at 02:20:05PM +0100, Dave Gordon wrote: > 'relative_constants_mode' has always been tracked per-device, but this > is wrong in execlists (or GuC) mode, as INSTPM is saved and restored > with the logical context, and the per-context value could therefore get > out of sync with the tracked value. This patch moves the tracking > element from the dev_priv structure into the intel_context structure, > with corresponding adjustments to the code that initialises and uses it. > > Test case (if anyone wants to write it) would be to create two contexts, > submit a batch with a non-default mode in the first context, submit a > batch with the default mode in the other context, submit another batch > in the first context, but this time in default mode. The driver will > fail to insert the instructions to reset INSTPM into the first context's > ringbuffer. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448 > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> Doesn't this break pre-gen8? Or maybe legacy mode instead of execlist, dunno. If it's pre-gen8 we need a check, if it's execlist vs. legacy then we could just move this into the ring instead. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++-- > drivers/gpu/drm/i915/i915_gem.c | 2 -- > drivers/gpu/drm/i915/i915_gem_context.c | 3 +++ > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +++--- > drivers/gpu/drm/i915/intel_lrc.c | 6 +++--- > 5 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 51eea29..2917370 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -872,6 +872,8 @@ struct intel_context { > struct i915_ctx_hang_stats hang_stats; > struct i915_hw_ppgtt *ppgtt; > > + int relative_constants_mode; > + > /* Legacy ring buffer submission */ > struct { > struct drm_i915_gem_object *rcs_state; > @@ -1707,8 +1709,6 @@ struct drm_i915_private { > > const struct intel_device_info info; > > - int relative_constants_mode; > - > void __iomem *regs; > > struct intel_uncore uncore; > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f0cfbb9..374af2d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4894,8 +4894,6 @@ i915_gem_load(struct drm_device *dev) > i915_gem_idle_work_handler); > init_waitqueue_head(&dev_priv->gpu_error.reset_queue); > > - dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL; > - > if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) > dev_priv->num_fence_regs = 32; > else if (INTEL_INFO(dev)->gen >= 4 || IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 74aa0c9..465e3e0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -222,6 +222,9 @@ __create_hw_context(struct drm_device *dev, > * is no remap info, it will be a NOP. */ > ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1; > > + /* First execbuffer will override this */ > + ctx->relative_constants_mode = -1; > + > ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD; > > return ctx; > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index edc17be..9833c8a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1283,7 +1283,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > goto error; > } > > - if (instp_mode != dev_priv->relative_constants_mode) { > + if (instp_mode != params->ctx->relative_constants_mode) { > if (INTEL_INFO(dev)->gen < 4) { > DRM_DEBUG("no rel constants on pre-gen4\n"); > ret = -EINVAL; > @@ -1309,7 +1309,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > } > > if (ring == &dev_priv->ring[RCS] && > - instp_mode != dev_priv->relative_constants_mode) { > + instp_mode != params->ctx->relative_constants_mode) { > ret = intel_ring_begin(params->request, 4); > if (ret) > goto error; > @@ -1320,7 +1320,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > intel_ring_emit(ring, instp_mask << 16 | instp_mode); > intel_ring_advance(ring); > > - dev_priv->relative_constants_mode = instp_mode; > + params->ctx->relative_constants_mode = instp_mode; > } > > if (args->flags & I915_EXEC_GEN7_SOL_RESET) { > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 825fa7a..9ff409d 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -889,7 +889,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, > return -EINVAL; > } > > - if (instp_mode != dev_priv->relative_constants_mode) { > + if (instp_mode != params->ctx->relative_constants_mode) { > if (instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) { > DRM_DEBUG("rel surface constants mode invalid on gen5+\n"); > return -EINVAL; > @@ -929,7 +929,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, > return ret; > > if (ring == &dev_priv->ring[RCS] && > - instp_mode != dev_priv->relative_constants_mode) { > + instp_mode != params->ctx->relative_constants_mode) { > ret = intel_logical_ring_begin(params->request, 4); > if (ret) > return ret; > @@ -940,7 +940,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, > intel_logical_ring_emit(ringbuf, instp_mask << 16 | instp_mode); > intel_logical_ring_advance(ringbuf); > > - dev_priv->relative_constants_mode = instp_mode; > + params->ctx->relative_constants_mode = instp_mode; > } > > exec_start = params->batch_obj_vm_offset + > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 13/10/15 16:28, Daniel Vetter wrote: > On Tue, Oct 13, 2015 at 02:20:05PM +0100, Dave Gordon wrote: >> 'relative_constants_mode' has always been tracked per-device, but this >> is wrong in execlists (or GuC) mode, as INSTPM is saved and restored >> with the logical context, and the per-context value could therefore get >> out of sync with the tracked value. This patch moves the tracking >> element from the dev_priv structure into the intel_context structure, >> with corresponding adjustments to the code that initialises and uses it. >> >> Test case (if anyone wants to write it) would be to create two contexts, >> submit a batch with a non-default mode in the first context, submit a >> batch with the default mode in the other context, submit another batch >> in the first context, but this time in default mode. The driver will >> fail to insert the instructions to reset INSTPM into the first context's >> ringbuffer. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448 >> >> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > Doesn't this break pre-gen8? Or maybe legacy mode instead of execlist, > dunno. If it's pre-gen8 we need a check, if it's execlist vs. legacy then > we could just move this into the ring instead. > -Daniel I don't think it will break anything. Pre-gen6 there's only one h/w context, and you can't create or use additional ones. So every request will use the same default context, and therefore storing the last mode in the (one and only) context will be no different from storing it at the device level. For gen6+, the INSTPM register containing this bit is saved & restored with the context, in both legacy (MI_SET_CONTEXT) and execlist modes, so tracking it in the intel_context is necessary to keep the s/w in sync with what the h/w is doing (if it applied per-engine we'd have to move it down another level, but it's render-only; and discontinued in SKL+ too). AFAICS this has been broken since gen6 introduced H/W contexts and the s/w didn't get updated to match! Which rather suggests that nobody uses this and we should just delete the whole lot instead of fixing it. .Dave. >> --- >> drivers/gpu/drm/i915/i915_drv.h | 4 ++-- >> drivers/gpu/drm/i915/i915_gem.c | 2 -- >> drivers/gpu/drm/i915/i915_gem_context.c | 3 +++ >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +++--- >> drivers/gpu/drm/i915/intel_lrc.c | 6 +++--- >> 5 files changed, 11 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 51eea29..2917370 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -872,6 +872,8 @@ struct intel_context { >> struct i915_ctx_hang_stats hang_stats; >> struct i915_hw_ppgtt *ppgtt; >> >> + int relative_constants_mode; >> + >> /* Legacy ring buffer submission */ >> struct { >> struct drm_i915_gem_object *rcs_state; >> @@ -1707,8 +1709,6 @@ struct drm_i915_private { >> >> const struct intel_device_info info; >> >> - int relative_constants_mode; >> - >> void __iomem *regs; >> >> struct intel_uncore uncore; >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index f0cfbb9..374af2d 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -4894,8 +4894,6 @@ i915_gem_load(struct drm_device *dev) >> i915_gem_idle_work_handler); >> init_waitqueue_head(&dev_priv->gpu_error.reset_queue); >> >> - dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL; >> - >> if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) >> dev_priv->num_fence_regs = 32; >> else if (INTEL_INFO(dev)->gen >= 4 || IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) >> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c >> index 74aa0c9..465e3e0 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >> @@ -222,6 +222,9 @@ __create_hw_context(struct drm_device *dev, >> * is no remap info, it will be a NOP. */ >> ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1; >> >> + /* First execbuffer will override this */ >> + ctx->relative_constants_mode = -1; >> + >> ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD; >> >> return ctx; >> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> index edc17be..9833c8a 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -1283,7 +1283,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, >> goto error; >> } >> >> - if (instp_mode != dev_priv->relative_constants_mode) { >> + if (instp_mode != params->ctx->relative_constants_mode) { >> if (INTEL_INFO(dev)->gen < 4) { >> DRM_DEBUG("no rel constants on pre-gen4\n"); >> ret = -EINVAL; >> @@ -1309,7 +1309,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, >> } >> >> if (ring == &dev_priv->ring[RCS] && >> - instp_mode != dev_priv->relative_constants_mode) { >> + instp_mode != params->ctx->relative_constants_mode) { >> ret = intel_ring_begin(params->request, 4); >> if (ret) >> goto error; >> @@ -1320,7 +1320,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, >> intel_ring_emit(ring, instp_mask << 16 | instp_mode); >> intel_ring_advance(ring); >> >> - dev_priv->relative_constants_mode = instp_mode; >> + params->ctx->relative_constants_mode = instp_mode; >> } >> >> if (args->flags & I915_EXEC_GEN7_SOL_RESET) { >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index 825fa7a..9ff409d 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -889,7 +889,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, >> return -EINVAL; >> } >> >> - if (instp_mode != dev_priv->relative_constants_mode) { >> + if (instp_mode != params->ctx->relative_constants_mode) { >> if (instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) { >> DRM_DEBUG("rel surface constants mode invalid on gen5+\n"); >> return -EINVAL; >> @@ -929,7 +929,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, >> return ret; >> >> if (ring == &dev_priv->ring[RCS] && >> - instp_mode != dev_priv->relative_constants_mode) { >> + instp_mode != params->ctx->relative_constants_mode) { >> ret = intel_logical_ring_begin(params->request, 4); >> if (ret) >> return ret; >> @@ -940,7 +940,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, >> intel_logical_ring_emit(ringbuf, instp_mask << 16 | instp_mode); >> intel_logical_ring_advance(ringbuf); >> >> - dev_priv->relative_constants_mode = instp_mode; >> + params->ctx->relative_constants_mode = instp_mode; >> } >> >> exec_start = params->batch_obj_vm_offset + >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Oct 19, 2015 at 01:16:18PM +0100, Dave Gordon wrote: > On 13/10/15 16:28, Daniel Vetter wrote: > >On Tue, Oct 13, 2015 at 02:20:05PM +0100, Dave Gordon wrote: > >>'relative_constants_mode' has always been tracked per-device, but this > >>is wrong in execlists (or GuC) mode, as INSTPM is saved and restored > >>with the logical context, and the per-context value could therefore get > >>out of sync with the tracked value. This patch moves the tracking > >>element from the dev_priv structure into the intel_context structure, > >>with corresponding adjustments to the code that initialises and uses it. > >> > >>Test case (if anyone wants to write it) would be to create two contexts, > >>submit a batch with a non-default mode in the first context, submit a > >>batch with the default mode in the other context, submit another batch > >>in the first context, but this time in default mode. The driver will > >>fail to insert the instructions to reset INSTPM into the first context's > >>ringbuffer. > >> > >>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448 > >> > >>Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > > >Doesn't this break pre-gen8? Or maybe legacy mode instead of execlist, > >dunno. If it's pre-gen8 we need a check, if it's execlist vs. legacy then > >we could just move this into the ring instead. > >-Daniel > > I don't think it will break anything. Pre-gen6 there's only one h/w context, > and you can't create or use additional ones. So every request will use the > same default context, and therefore storing the last mode in the (one and > only) context will be no different from storing it at the device level. > > For gen6+, the INSTPM register containing this bit is saved & restored with > the context, in both legacy (MI_SET_CONTEXT) and execlist modes, so tracking > it in the intel_context is necessary to keep the s/w in sync with what the > h/w is doing (if it applied per-engine we'd have to move it down another > level, but it's render-only; and discontinued in SKL+ too). > > AFAICS this has been broken since gen6 introduced H/W contexts and the s/w > didn't get updated to match! Which rather suggests that nobody uses this and > we should just delete the whole lot instead of fixing it. Nah, much more likely is that it might or might not see the random pile of hangs we've been seeing ever since contexts have been enabled by default. But the above is crucial information to assess correctness of your patch, so should be included in the commit message. -Daniel > > .Dave. > > >>--- > >> drivers/gpu/drm/i915/i915_drv.h | 4 ++-- > >> drivers/gpu/drm/i915/i915_gem.c | 2 -- > >> drivers/gpu/drm/i915/i915_gem_context.c | 3 +++ > >> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +++--- > >> drivers/gpu/drm/i915/intel_lrc.c | 6 +++--- > >> 5 files changed, 11 insertions(+), 10 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >>index 51eea29..2917370 100644 > >>--- a/drivers/gpu/drm/i915/i915_drv.h > >>+++ b/drivers/gpu/drm/i915/i915_drv.h > >>@@ -872,6 +872,8 @@ struct intel_context { > >> struct i915_ctx_hang_stats hang_stats; > >> struct i915_hw_ppgtt *ppgtt; > >> > >>+ int relative_constants_mode; > >>+ > >> /* Legacy ring buffer submission */ > >> struct { > >> struct drm_i915_gem_object *rcs_state; > >>@@ -1707,8 +1709,6 @@ struct drm_i915_private { > >> > >> const struct intel_device_info info; > >> > >>- int relative_constants_mode; > >>- > >> void __iomem *regs; > >> > >> struct intel_uncore uncore; > >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >>index f0cfbb9..374af2d 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem.c > >>+++ b/drivers/gpu/drm/i915/i915_gem.c > >>@@ -4894,8 +4894,6 @@ i915_gem_load(struct drm_device *dev) > >> i915_gem_idle_work_handler); > >> init_waitqueue_head(&dev_priv->gpu_error.reset_queue); > >> > >>- dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL; > >>- > >> if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) > >> dev_priv->num_fence_regs = 32; > >> else if (INTEL_INFO(dev)->gen >= 4 || IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) > >>diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > >>index 74aa0c9..465e3e0 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem_context.c > >>+++ b/drivers/gpu/drm/i915/i915_gem_context.c > >>@@ -222,6 +222,9 @@ __create_hw_context(struct drm_device *dev, > >> * is no remap info, it will be a NOP. */ > >> ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1; > >> > >>+ /* First execbuffer will override this */ > >>+ ctx->relative_constants_mode = -1; > >>+ > >> ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD; > >> > >> return ctx; > >>diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>index edc17be..9833c8a 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > >>@@ -1283,7 +1283,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > >> goto error; > >> } > >> > >>- if (instp_mode != dev_priv->relative_constants_mode) { > >>+ if (instp_mode != params->ctx->relative_constants_mode) { > >> if (INTEL_INFO(dev)->gen < 4) { > >> DRM_DEBUG("no rel constants on pre-gen4\n"); > >> ret = -EINVAL; > >>@@ -1309,7 +1309,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > >> } > >> > >> if (ring == &dev_priv->ring[RCS] && > >>- instp_mode != dev_priv->relative_constants_mode) { > >>+ instp_mode != params->ctx->relative_constants_mode) { > >> ret = intel_ring_begin(params->request, 4); > >> if (ret) > >> goto error; > >>@@ -1320,7 +1320,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, > >> intel_ring_emit(ring, instp_mask << 16 | instp_mode); > >> intel_ring_advance(ring); > >> > >>- dev_priv->relative_constants_mode = instp_mode; > >>+ params->ctx->relative_constants_mode = instp_mode; > >> } > >> > >> if (args->flags & I915_EXEC_GEN7_SOL_RESET) { > >>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >>index 825fa7a..9ff409d 100644 > >>--- a/drivers/gpu/drm/i915/intel_lrc.c > >>+++ b/drivers/gpu/drm/i915/intel_lrc.c > >>@@ -889,7 +889,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, > >> return -EINVAL; > >> } > >> > >>- if (instp_mode != dev_priv->relative_constants_mode) { > >>+ if (instp_mode != params->ctx->relative_constants_mode) { > >> if (instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) { > >> DRM_DEBUG("rel surface constants mode invalid on gen5+\n"); > >> return -EINVAL; > >>@@ -929,7 +929,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, > >> return ret; > >> > >> if (ring == &dev_priv->ring[RCS] && > >>- instp_mode != dev_priv->relative_constants_mode) { > >>+ instp_mode != params->ctx->relative_constants_mode) { > >> ret = intel_logical_ring_begin(params->request, 4); > >> if (ret) > >> return ret; > >>@@ -940,7 +940,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, > >> intel_logical_ring_emit(ringbuf, instp_mask << 16 | instp_mode); > >> intel_logical_ring_advance(ringbuf); > >> > >>- dev_priv->relative_constants_mode = instp_mode; > >>+ params->ctx->relative_constants_mode = instp_mode; > >> } > >> > >> exec_start = params->batch_obj_vm_offset + > >>-- > >>1.9.1 > >> > >>_______________________________________________ > >>Intel-gfx mailing list > >>Intel-gfx@lists.freedesktop.org > >>http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 51eea29..2917370 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -872,6 +872,8 @@ struct intel_context { struct i915_ctx_hang_stats hang_stats; struct i915_hw_ppgtt *ppgtt; + int relative_constants_mode; + /* Legacy ring buffer submission */ struct { struct drm_i915_gem_object *rcs_state; @@ -1707,8 +1709,6 @@ struct drm_i915_private { const struct intel_device_info info; - int relative_constants_mode; - void __iomem *regs; struct intel_uncore uncore; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f0cfbb9..374af2d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4894,8 +4894,6 @@ i915_gem_load(struct drm_device *dev) i915_gem_idle_work_handler); init_waitqueue_head(&dev_priv->gpu_error.reset_queue); - dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL; - if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev)) dev_priv->num_fence_regs = 32; else if (INTEL_INFO(dev)->gen >= 4 || IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev)) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 74aa0c9..465e3e0 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -222,6 +222,9 @@ __create_hw_context(struct drm_device *dev, * is no remap info, it will be a NOP. */ ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1; + /* First execbuffer will override this */ + ctx->relative_constants_mode = -1; + ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD; return ctx; diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index edc17be..9833c8a 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1283,7 +1283,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, goto error; } - if (instp_mode != dev_priv->relative_constants_mode) { + if (instp_mode != params->ctx->relative_constants_mode) { if (INTEL_INFO(dev)->gen < 4) { DRM_DEBUG("no rel constants on pre-gen4\n"); ret = -EINVAL; @@ -1309,7 +1309,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, } if (ring == &dev_priv->ring[RCS] && - instp_mode != dev_priv->relative_constants_mode) { + instp_mode != params->ctx->relative_constants_mode) { ret = intel_ring_begin(params->request, 4); if (ret) goto error; @@ -1320,7 +1320,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, intel_ring_emit(ring, instp_mask << 16 | instp_mode); intel_ring_advance(ring); - dev_priv->relative_constants_mode = instp_mode; + params->ctx->relative_constants_mode = instp_mode; } if (args->flags & I915_EXEC_GEN7_SOL_RESET) { diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 825fa7a..9ff409d 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -889,7 +889,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, return -EINVAL; } - if (instp_mode != dev_priv->relative_constants_mode) { + if (instp_mode != params->ctx->relative_constants_mode) { if (instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) { DRM_DEBUG("rel surface constants mode invalid on gen5+\n"); return -EINVAL; @@ -929,7 +929,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, return ret; if (ring == &dev_priv->ring[RCS] && - instp_mode != dev_priv->relative_constants_mode) { + instp_mode != params->ctx->relative_constants_mode) { ret = intel_logical_ring_begin(params->request, 4); if (ret) return ret; @@ -940,7 +940,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params, intel_logical_ring_emit(ringbuf, instp_mask << 16 | instp_mode); intel_logical_ring_advance(ringbuf); - dev_priv->relative_constants_mode = instp_mode; + params->ctx->relative_constants_mode = instp_mode; } exec_start = params->batch_obj_vm_offset +
'relative_constants_mode' has always been tracked per-device, but this is wrong in execlists (or GuC) mode, as INSTPM is saved and restored with the logical context, and the per-context value could therefore get out of sync with the tracked value. This patch moves the tracking element from the dev_priv structure into the intel_context structure, with corresponding adjustments to the code that initialises and uses it. Test case (if anyone wants to write it) would be to create two contexts, submit a batch with a non-default mode in the first context, submit a batch with the default mode in the other context, submit another batch in the first context, but this time in default mode. The driver will fail to insert the instructions to reset INSTPM into the first context's ringbuffer. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448 Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++-- drivers/gpu/drm/i915/i915_gem.c | 2 -- drivers/gpu/drm/i915/i915_gem_context.c | 3 +++ drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +++--- drivers/gpu/drm/i915/intel_lrc.c | 6 +++--- 5 files changed, 11 insertions(+), 10 deletions(-)