| Message ID | 1463333573-25112-7-git-send-email-zhi.a.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 16, 2016 at 01:32:44AM +0800, Zhi Wang wrote: > GVT context will use its own shadow PPGTT, and it doesn't need a > i915_hw_ppgtt. > > This patch adds a "has_ppgtt" param to i915_gem_context(), which > allows the caller to create a context without PPGTT > > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 34 ++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 21498e5..b952e37 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -298,7 +298,8 @@ err_out: > */ > static struct intel_context * > i915_gem_create_context(struct drm_device *dev, > - struct drm_i915_file_private *file_priv) > + struct drm_i915_file_private *file_priv, > + bool has_ppgtt) > { > struct drm_i915_private *dev_priv = dev->dev_private; > const bool is_global_default_ctx = file_priv == NULL; > @@ -327,19 +328,22 @@ i915_gem_create_context(struct drm_device *dev, > } > } > > - if (USES_FULL_PPGTT(dev)) { > - struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv); > + if (has_ppgtt) { > + if (USES_FULL_PPGTT(dev)) { > + struct i915_hw_ppgtt *ppgtt = > + i915_ppgtt_create(dev, file_priv); > > - if (IS_ERR_OR_NULL(ppgtt)) { > - DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", > - PTR_ERR(ppgtt)); > - ret = PTR_ERR(ppgtt); > - goto err_unpin; > - } > + if (IS_ERR_OR_NULL(ppgtt)) { > + DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", > + PTR_ERR(ppgtt)); > + ret = PTR_ERR(ppgtt); > + goto err_unpin; > + } > > - ctx->ppgtt = ppgtt; > - } else > - ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; > + ctx->ppgtt = ppgtt; > + } else > + ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; You have to first go through the driver and update the sematics for ctx->ppgtt == NULL. (Note in some instances you have to use the ggtt pointer and not the appgtt, just a minor case in execbuf!). Then tell us how you didn't spot the explosion when trying to use the aliasing_ppgtt after freeing it. -Chris
Yes. Thanks! Seems there is an assumption if ctx->ppgtt == NULL then go back to use aliasing ppgtt. I see. I will drop that patch, without that patch we could also work -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Monday, May 16, 2016 8:13 AM To: Wang, Zhi A <zhi.a.wang@intel.com> Cc: intel-gfx@lists.freedesktop.org; Gordon, David S <david.s.gordon@intel.com>; joonas.lahtinen@linux.intel.com; Tian, Kevin <kevin.tian@intel.com>; Lv, Zhiyuan <zhiyuan.lv@intel.com> Subject: Re: [Intel-gfx] [PATCH 06/15] drm/i915: Allow the caller to create a intel_context without PPGTT On Mon, May 16, 2016 at 01:32:44AM +0800, Zhi Wang wrote: > GVT context will use its own shadow PPGTT, and it doesn't need a > i915_hw_ppgtt. > > This patch adds a "has_ppgtt" param to i915_gem_context(), which > allows the caller to create a context without PPGTT > > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 34 > ++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index 21498e5..b952e37 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -298,7 +298,8 @@ err_out: > */ > static struct intel_context * > i915_gem_create_context(struct drm_device *dev, > - struct drm_i915_file_private *file_priv) > + struct drm_i915_file_private *file_priv, > + bool has_ppgtt) > { > struct drm_i915_private *dev_priv = dev->dev_private; > const bool is_global_default_ctx = file_priv == NULL; @@ -327,19 > +328,22 @@ i915_gem_create_context(struct drm_device *dev, > } > } > > - if (USES_FULL_PPGTT(dev)) { > - struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv); > + if (has_ppgtt) { > + if (USES_FULL_PPGTT(dev)) { > + struct i915_hw_ppgtt *ppgtt = > + i915_ppgtt_create(dev, file_priv); > > - if (IS_ERR_OR_NULL(ppgtt)) { > - DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", > - PTR_ERR(ppgtt)); > - ret = PTR_ERR(ppgtt); > - goto err_unpin; > - } > + if (IS_ERR_OR_NULL(ppgtt)) { > + DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", > + PTR_ERR(ppgtt)); > + ret = PTR_ERR(ppgtt); > + goto err_unpin; > + } > > - ctx->ppgtt = ppgtt; > - } else > - ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; > + ctx->ppgtt = ppgtt; > + } else > + ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; You have to first go through the driver and update the sematics for ctx->ppgtt == NULL. (Note in some instances you have to use the ggtt pointer and not the appgtt, just a minor case in execbuf!). Then tell us how you didn't spot the explosion when trying to use the aliasing_ppgtt after freeing it. -Chris -- Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 21498e5..b952e37 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -298,7 +298,8 @@ err_out: */ static struct intel_context * i915_gem_create_context(struct drm_device *dev, - struct drm_i915_file_private *file_priv) + struct drm_i915_file_private *file_priv, + bool has_ppgtt) { struct drm_i915_private *dev_priv = dev->dev_private; const bool is_global_default_ctx = file_priv == NULL; @@ -327,19 +328,22 @@ i915_gem_create_context(struct drm_device *dev, } } - if (USES_FULL_PPGTT(dev)) { - struct i915_hw_ppgtt *ppgtt = i915_ppgtt_create(dev, file_priv); + if (has_ppgtt) { + if (USES_FULL_PPGTT(dev)) { + struct i915_hw_ppgtt *ppgtt = + i915_ppgtt_create(dev, file_priv); - if (IS_ERR_OR_NULL(ppgtt)) { - DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", - PTR_ERR(ppgtt)); - ret = PTR_ERR(ppgtt); - goto err_unpin; - } + if (IS_ERR_OR_NULL(ppgtt)) { + DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", + PTR_ERR(ppgtt)); + ret = PTR_ERR(ppgtt); + goto err_unpin; + } - ctx->ppgtt = ppgtt; - } else - ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; + ctx->ppgtt = ppgtt; + } else + ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; + } trace_i915_context_create(ctx); @@ -416,7 +420,7 @@ int i915_gem_context_init(struct drm_device *dev) } } - ctx = i915_gem_create_context(dev, NULL); + ctx = i915_gem_create_context(dev, NULL, true); if (IS_ERR(ctx)) { DRM_ERROR("Failed to create default global context (error %ld)\n", PTR_ERR(ctx)); @@ -478,7 +482,7 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file) idr_init(&file_priv->context_idr); mutex_lock(&dev->struct_mutex); - ctx = i915_gem_create_context(dev, file_priv); + ctx = i915_gem_create_context(dev, file_priv, true); mutex_unlock(&dev->struct_mutex); if (IS_ERR(ctx)) { @@ -912,7 +916,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, if (ret) return ret; - ctx = i915_gem_create_context(dev, file_priv); + ctx = i915_gem_create_context(dev, file_priv, true); mutex_unlock(&dev->struct_mutex); if (IS_ERR(ctx)) return PTR_ERR(ctx);
GVT context will use its own shadow PPGTT, and it doesn't need a i915_hw_ppgtt. This patch adds a "has_ppgtt" param to i915_gem_context(), which allows the caller to create a context without PPGTT Signed-off-by: Zhi Wang <zhi.a.wang@intel.com> --- drivers/gpu/drm/i915/i915_gem_context.c | 34 ++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)