| Message ID | 1407161913-31040-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > Sent: Monday, August 04, 2014 3:19 PM > To: Intel Graphics Development > Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä > Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt > > Stuffing this into the context setup code doesn't make a lot of sense. > Also reusing the real ppgtt setup code makes even less sense since the > aliasing ppgtt isn't a real address space. Leaving all that stuff > unitialized will make sure that we catch any abusers promptly. > > This is also a prep work to clean up the context->ppgtt link. > > v2: Fix up the logic fail, I've fumbled it so badly to completely > disable ppgtt on gen6. Spotted by Ville and Michel. Also move around > the pde write into the gen6 init function, since otherwise it won't > work at all. > > Cc: "Thierry, Michel" <michel.thierry@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 13 +--------- > drivers/gpu/drm/i915/i915_gem_gtt.c | 42 +++++++++++++++++++++++- > --------- > 2 files changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index 3b8367aa8404..7a455fcee3a7 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev, > goto err_unpin; > } else > ctx->vm = &ppgtt->base; > - > - /* This case is reserved for the global default context and > - * should only happen once. */ > - if (is_global_default_ctx) { > - if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) { > - ret = -EEXIST; > - goto err_unpin; > - } > - > - dev_priv->mm.aliasing_ppgtt = ppgtt; > - } I expect some problems with full ppgtt & this (in some places, the driver is still making some decisions based on dev_priv->mm.aliasing_ppgtt, which now will be null). Should I address these problems in a subsequent patch? > } else if (USES_PPGTT(dev)) { > /* For platforms which only have aliasing PPGTT, we fake the > * address space and refcounting. */ > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev) > } > } > > - ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev)); > + ctx = i915_gem_create_context(dev, NULL, > USES_FULL_PPGTT(dev)); > if (IS_ERR(ctx)) { > DRM_ERROR("Failed to create default global context (error > %ld)\n", > PTR_ERR(ctx)); > } > > -- > 1.9.3
On Wed, Aug 06, 2014 at 08:18:52AM +0000, Thierry, Michel wrote: > > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > > Sent: Monday, August 04, 2014 3:19 PM > > To: Intel Graphics Development > > Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä > > Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt > > > > Stuffing this into the context setup code doesn't make a lot of sense. > > Also reusing the real ppgtt setup code makes even less sense since the > > aliasing ppgtt isn't a real address space. Leaving all that stuff > > unitialized will make sure that we catch any abusers promptly. > > > > This is also a prep work to clean up the context->ppgtt link. > > > > v2: Fix up the logic fail, I've fumbled it so badly to completely > > disable ppgtt on gen6. Spotted by Ville and Michel. Also move around > > the pde write into the gen6 init function, since otherwise it won't > > work at all. > > > > Cc: "Thierry, Michel" <michel.thierry@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 13 +--------- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 42 +++++++++++++++++++++++- > > --------- > > 2 files changed, 31 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > > b/drivers/gpu/drm/i915/i915_gem_context.c > > index 3b8367aa8404..7a455fcee3a7 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev, > > goto err_unpin; > > } else > > ctx->vm = &ppgtt->base; > > - > > - /* This case is reserved for the global default context and > > - * should only happen once. */ > > - if (is_global_default_ctx) { > > - if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) { > > - ret = -EEXIST; > > - goto err_unpin; > > - } > > - > > - dev_priv->mm.aliasing_ppgtt = ppgtt; > > - } > > I expect some problems with full ppgtt & this (in some places, the > driver is still making some decisions based on > dev_priv->mm.aliasing_ppgtt, which now will be null). Should I address > these problems in a subsequent patch? Oh, good catch. I've done a bit a review and found two cases: - Driver unload code. Already fairly broken, made much worse by my series here. I've fixed that up yesterday and will resend the series with those additional patches and revised patches. - cmd parser. I guess that should be a fixup patch on top. I'll also do that. Have you spotted any other places? Thanks, Daniel > > > } else if (USES_PPGTT(dev)) { > > /* For platforms which only have aliasing PPGTT, we fake the > > * address space and refcounting. */ > > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev) > > } > > } > > > > - ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev)); > > + ctx = i915_gem_create_context(dev, NULL, > > USES_FULL_PPGTT(dev)); > > if (IS_ERR(ctx)) { > > DRM_ERROR("Failed to create default global context (error > > %ld)\n", > > PTR_ERR(ctx)); > > } > > > > -- > > 1.9.3 >
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Wednesday, August 06, 2014 9:31 AM > To: Thierry, Michel > Cc: Daniel Vetter; Intel Graphics Development > Subject: Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt > > On Wed, Aug 06, 2014 at 08:18:52AM +0000, Thierry, Michel wrote: > > > > > > > -----Original Message----- > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > > > Sent: Monday, August 04, 2014 3:19 PM > > > To: Intel Graphics Development > > > Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä > > > Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt > > > > > > Stuffing this into the context setup code doesn't make a lot of sense. > > > Also reusing the real ppgtt setup code makes even less sense since the > > > aliasing ppgtt isn't a real address space. Leaving all that stuff > > > unitialized will make sure that we catch any abusers promptly. > > > > > > This is also a prep work to clean up the context->ppgtt link. > > > > > > v2: Fix up the logic fail, I've fumbled it so badly to completely > > > disable ppgtt on gen6. Spotted by Ville and Michel. Also move around > > > the pde write into the gen6 init function, since otherwise it won't > > > work at all. > > > > > > Cc: "Thierry, Michel" <michel.thierry@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > drivers/gpu/drm/i915/i915_gem_context.c | 13 +--------- > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 42 > +++++++++++++++++++++++- > > > --------- > > > 2 files changed, 31 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > > > b/drivers/gpu/drm/i915/i915_gem_context.c > > > index 3b8367aa8404..7a455fcee3a7 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device > *dev, > > > goto err_unpin; > > > } else > > > ctx->vm = &ppgtt->base; > > > - > > > - /* This case is reserved for the global default context and > > > - * should only happen once. */ > > > - if (is_global_default_ctx) { > > > - if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) { > > > - ret = -EEXIST; > > > - goto err_unpin; > > > - } > > > - > > > - dev_priv->mm.aliasing_ppgtt = ppgtt; > > > - } > > > > I expect some problems with full ppgtt & this (in some places, the > > driver is still making some decisions based on > > dev_priv->mm.aliasing_ppgtt, which now will be null). Should I address > > these problems in a subsequent patch? > > Oh, good catch. I've done a bit a review and found two cases: > - Driver unload code. Already fairly broken, made much worse by my series > here. I've fixed that up yesterday and will resend the series with those > additional patches and revised patches. > - cmd parser. I guess that should be a fixup patch on top. I'll also do > that. > > Have you spotted any other places? The other place is gen8_ring_dispatch_execbuffer, it won't set the ppgtt flag in MI_BATCH_BUFFER_START. > > Thanks, Daniel > > > > > > } else if (USES_PPGTT(dev)) { > > > /* For platforms which only have aliasing PPGTT, we fake the > > > * address space and refcounting. */ > > > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device > *dev) > > > } > > > } > > > > > > - ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev)); > > > + ctx = i915_gem_create_context(dev, NULL, > > > USES_FULL_PPGTT(dev)); > > > if (IS_ERR(ctx)) { > > > DRM_ERROR("Failed to create default global context (error > > > %ld)\n", > > > PTR_ERR(ctx)); > > > } > > > > > > -- > > > 1.9.3 > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Aug 06, 2014 at 08:44:34AM +0000, Thierry, Michel wrote: > > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Wednesday, August 06, 2014 9:31 AM > > To: Thierry, Michel > > Cc: Daniel Vetter; Intel Graphics Development > > Subject: Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of > global gtt > > > > On Wed, Aug 06, 2014 at 08:18:52AM +0000, Thierry, Michel wrote: > > > > > > > > > > -----Original Message----- > > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] > > > > Sent: Monday, August 04, 2014 3:19 PM > > > > To: Intel Graphics Development > > > > Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä > > > > Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of > global gtt > > > > > > > > Stuffing this into the context setup code doesn't make a lot of sense. > > > > Also reusing the real ppgtt setup code makes even less sense since the > > > > aliasing ppgtt isn't a real address space. Leaving all that stuff > > > > unitialized will make sure that we catch any abusers promptly. > > > > > > > > This is also a prep work to clean up the context->ppgtt link. > > > > > > > > v2: Fix up the logic fail, I've fumbled it so badly to completely > > > > disable ppgtt on gen6. Spotted by Ville and Michel. Also move around > > > > the pde write into the gen6 init function, since otherwise it won't > > > > work at all. > > > > > > > > Cc: "Thierry, Michel" <michel.thierry@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > --- > > > > drivers/gpu/drm/i915/i915_gem_context.c | 13 +--------- > > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 42 > > +++++++++++++++++++++++- > > > > --------- > > > > 2 files changed, 31 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > > > > b/drivers/gpu/drm/i915/i915_gem_context.c > > > > index 3b8367aa8404..7a455fcee3a7 100644 > > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > > > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device > > *dev, > > > > goto err_unpin; > > > > } else > > > > ctx->vm = &ppgtt->base; > > > > - > > > > - /* This case is reserved for the global default context and > > > > - * should only happen once. */ > > > > - if (is_global_default_ctx) { > > > > - if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) { > > > > - ret = -EEXIST; > > > > - goto err_unpin; > > > > - } > > > > - > > > > - dev_priv->mm.aliasing_ppgtt = ppgtt; > > > > - } > > > > > > I expect some problems with full ppgtt & this (in some places, the > > > driver is still making some decisions based on > > > dev_priv->mm.aliasing_ppgtt, which now will be null). Should I address > > > these problems in a subsequent patch? > > > > Oh, good catch. I've done a bit a review and found two cases: > > - Driver unload code. Already fairly broken, made much worse by my series > > here. I've fixed that up yesterday and will resend the series with those > > additional patches and revised patches. > > - cmd parser. I guess that should be a fixup patch on top. I'll also do > > that. > > > > Have you spotted any other places? > The other place is gen8_ring_dispatch_execbuffer, it won't set the ppgtt > flag in MI_BATCH_BUFFER_START. Indeed. I've also found a few other really fishy places. Will follow up with revised patches. -Daniel > > > > > Thanks, Daniel > > > > > > > > > } else if (USES_PPGTT(dev)) { > > > > /* For platforms which only have aliasing PPGTT, we fake the > > > > * address space and refcounting. */ > > > > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device > > *dev) > > > > } > > > > } > > > > > > > > - ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev)); > > > > + ctx = i915_gem_create_context(dev, NULL, > > > > USES_FULL_PPGTT(dev)); > > > > if (IS_ERR(ctx)) { > > > > DRM_ERROR("Failed to create default global context (error > > > > %ld)\n", > > > > PTR_ERR(ctx)); > > > > } > > > > > > > > -- > > > > 1.9.3 > > > > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 3b8367aa8404..7a455fcee3a7 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device *dev, goto err_unpin; } else ctx->vm = &ppgtt->base; - - /* This case is reserved for the global default context and - * should only happen once. */ - if (is_global_default_ctx) { - if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) { - ret = -EEXIST; - goto err_unpin; - } - - dev_priv->mm.aliasing_ppgtt = ppgtt; - } } else if (USES_PPGTT(dev)) { /* For platforms which only have aliasing PPGTT, we fake the * address space and refcounting. */ @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device *dev) } } - ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev)); + ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev)); if (IS_ERR(ctx)) { DRM_ERROR("Failed to create default global context (error %ld)\n", PTR_ERR(ctx)); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 4fa7807ed4d5..a4c1740d79be 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1188,35 +1188,38 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) ppgtt->node.size >> 20, ppgtt->node.start / PAGE_SIZE); + gen6_write_pdes(ppgtt); + DRM_DEBUG("Adding PPGTT at offset %x\n", + ppgtt->pd_offset << 10); + return 0; } -int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) +int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) { struct drm_i915_private *dev_priv = dev->dev_private; - int ret = 0; ppgtt->base.dev = dev; ppgtt->base.scratch = dev_priv->gtt.base.scratch; if (INTEL_INFO(dev)->gen < 8) - ret = gen6_ppgtt_init(ppgtt); + return gen6_ppgtt_init(ppgtt); else if (IS_GEN8(dev)) - ret = gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total); + return gen8_ppgtt_init(ppgtt, dev_priv->gtt.base.total); else BUG(); +} +int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int ret = 0; - if (!ret) { - struct drm_i915_private *dev_priv = dev->dev_private; + ret = __hw_ppgtt_init(dev, ppgtt); + if (ret == 0) { kref_init(&ppgtt->ref); drm_mm_init(&ppgtt->base.mm, ppgtt->base.start, ppgtt->base.total); i915_init_vm(dev_priv, &ppgtt->base); - if (INTEL_INFO(dev)->gen < 8) { - gen6_write_pdes(ppgtt); - DRM_DEBUG("Adding PPGTT at offset %x\n", - ppgtt->pd_offset << 10); - } } return ret; @@ -1728,6 +1731,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev, struct drm_mm_node *entry; struct drm_i915_gem_object *obj; unsigned long hole_start, hole_end; + int ret; BUG_ON(mappable_end > end); @@ -1739,7 +1743,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev, /* Mark any preallocated objects as occupied */ list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { struct i915_vma *vma = i915_gem_obj_to_vma(obj, ggtt_vm); - int ret; + DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n", i915_gem_obj_ggtt_offset(obj), obj->base.size); @@ -1766,6 +1770,20 @@ int i915_gem_setup_global_gtt(struct drm_device *dev, /* And finally clear the reserved guard page */ ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true); + if (HAS_ALIASING_PPGTT(dev) && !USES_FULL_PPGTT(dev)) { + struct i915_hw_ppgtt *ppgtt; + + ppgtt = kzalloc(sizeof(*ppgtt), GFP_KERNEL); + if (!ppgtt) + return -ENOMEM; + + ret = __hw_ppgtt_init(dev, ppgtt); + if (ret != 0) + return ret; + + dev_priv->mm.aliasing_ppgtt = ppgtt; + } + return 0; }
Stuffing this into the context setup code doesn't make a lot of sense. Also reusing the real ppgtt setup code makes even less sense since the aliasing ppgtt isn't a real address space. Leaving all that stuff unitialized will make sure that we catch any abusers promptly. This is also a prep work to clean up the context->ppgtt link. v2: Fix up the logic fail, I've fumbled it so badly to completely disable ppgtt on gen6. Spotted by Ville and Michel. Also move around the pde write into the gen6 init function, since otherwise it won't work at all. Cc: "Thierry, Michel" <michel.thierry@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem_context.c | 13 +--------- drivers/gpu/drm/i915/i915_gem_gtt.c | 42 +++++++++++++++++++++++---------- 2 files changed, 31 insertions(+), 24 deletions(-)