| Message ID | 1408373642-16514-1-git-send-email-thomas.daniel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 18, 2014 at 03:54:02PM +0100, Thomas Daniel wrote: > A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops > on boot. Add a check so that is full PPGTT is not in use the context is > populated with the aliasing PPGTT. > > Issue: VIZ-4278 > Signed-off-by: Thomas Daniel <thomas.daniel@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index c096b9b..79a6b91 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1452,12 +1452,19 @@ static int > populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj, > struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf) > { > + struct drm_device *dev = ring->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_gem_object *ring_obj = ringbuf->obj; > struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; It's a bit weird to leave ppgtt initialized here when you're going to always override it below. > struct page *page; > uint32_t *reg_state; > int ret; > > + if (USES_FULL_PPGTT(dev)) > + ppgtt = ctx->ppgtt; > + else > + ppgtt = dev_priv->mm.aliasing_ppgtt; > + The patch drom daniel you mention removes the usage of USES_FULL_PPGTT() to directly test ctx->ppgtt. You may want to reproduce that here ie. ppgtt = ctx->ppgtt; if (!ppgtt) ppgtt = dev_priv->mm.aliasing_ppgtt; doesn't really matter either way I guess, but can we at least not set then always override ppgtt? Thanks,
Ok, things start to dawn upon me - let's blame it on the lack of decent coffee here ;-) I mixed up USES_PPGTT with USES_FULL_PPGTT (again) so didn't realize that execlist works with just aliasing ppgtt. So I guess there's still room to clean up these macros a bit, maybe do an s/USES_PPGTT/USES_HW_PPGTT/ or so, to make it clear that this isn't about multiple address spaces. I do like Damien's suggestion though since imo depending upon runtime state is less fragile than rechecking driver option. -Daniel On Mon, Aug 18, 2014 at 6:49 PM, Damien Lespiau <damien.lespiau@intel.com> wrote: > On Mon, Aug 18, 2014 at 03:54:02PM +0100, Thomas Daniel wrote: >> A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops >> on boot. Add a check so that is full PPGTT is not in use the context is >> populated with the aliasing PPGTT. >> >> Issue: VIZ-4278 >> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com> >> --- >> drivers/gpu/drm/i915/intel_lrc.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index c096b9b..79a6b91 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -1452,12 +1452,19 @@ static int >> populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj, >> struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf) >> { >> + struct drm_device *dev = ring->dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> struct drm_i915_gem_object *ring_obj = ringbuf->obj; >> struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; > > It's a bit weird to leave ppgtt initialized here when you're going to > always override it below. > >> struct page *page; >> uint32_t *reg_state; >> int ret; >> >> + if (USES_FULL_PPGTT(dev)) >> + ppgtt = ctx->ppgtt; >> + else >> + ppgtt = dev_priv->mm.aliasing_ppgtt; >> + > > The patch drom daniel you mention removes the usage of USES_FULL_PPGTT() > to directly test ctx->ppgtt. You may want to reproduce that here ie. > > ppgtt = ctx->ppgtt; > if (!ppgtt) > ppgtt = dev_priv->mm.aliasing_ppgtt; > > doesn't really matter either way I guess, but can we at least not set > then always override ppgtt? > > Thanks, > > -- > Damien > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index c096b9b..79a6b91 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1452,12 +1452,19 @@ static int populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj, struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf) { + struct drm_device *dev = ring->dev; + struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_gem_object *ring_obj = ringbuf->obj; struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; struct page *page; uint32_t *reg_state; int ret; + if (USES_FULL_PPGTT(dev)) + ppgtt = ctx->ppgtt; + else + ppgtt = dev_priv->mm.aliasing_ppgtt; + ret = i915_gem_object_set_to_cpu_domain(ctx_obj, true); if (ret) { DRM_DEBUG_DRIVER("Could not set to CPU domain\n");
A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops on boot. Add a check so that is full PPGTT is not in use the context is populated with the aliasing PPGTT. Issue: VIZ-4278 Signed-off-by: Thomas Daniel <thomas.daniel@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 7 +++++++ 1 file changed, 7 insertions(+)