| Message ID | 1463333573-25112-6-git-send-email-zhi.a.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/05/16 18:32, Zhi Wang wrote: > Currently ctx->ppgtt would only be initialized when full PPGTT is used. > For aliasing PPGTT mode, ctx->ppgtt will be set when LRC context is > populated. > > This patch moves the assignment into i915_gem_create_context() for better > code structure. Hm, it doesn't move the assignment it adds it. Previously ctx->ppgtt was always NULL when !USES_FULL_PPGTT. Now it will point to aliasing ppgtt. Since there are various places in the code which do "if (ctx->ppgtt)" (more or less) I think you need to explain why those will still work OK. i915_gem_context_free for example? Regards, Tvrtko > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 4 +++- > drivers/gpu/drm/i915/intel_lrc.c | 3 --- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 2aedd18..21498e5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -300,6 +300,7 @@ static struct intel_context * > i915_gem_create_context(struct drm_device *dev, > struct drm_i915_file_private *file_priv) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > const bool is_global_default_ctx = file_priv == NULL; > struct intel_context *ctx; > int ret = 0; > @@ -337,7 +338,8 @@ i915_gem_create_context(struct drm_device *dev, > } > > ctx->ppgtt = ppgtt; > - } > + } else > + ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; > > trace_i915_context_create(ctx); > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index db10c96..397fe65 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -2286,9 +2286,6 @@ populate_lr_context(struct intel_context *ctx, > u32 *reg_state; > int ret; > > - if (!ppgtt) > - 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"); >
I understand what you say. Let me think about other approach. My requirement is to do nothing if a context doesn't have a ppgtt, but ctx->ppgtt == NULL means context is using aliasing ppgtt, so I need to find another way to achieve this. -----Original Message----- From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] Sent: Monday, May 16, 2016 6:31 AM To: Wang, Zhi A <zhi.a.wang@intel.com>; 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 05/15] drm/i915: Set ctx->ppgtt for aliasing PPGTT in context creation On 15/05/16 18:32, Zhi Wang wrote: > Currently ctx->ppgtt would only be initialized when full PPGTT is used. > For aliasing PPGTT mode, ctx->ppgtt will be set when LRC context is > populated. > > This patch moves the assignment into i915_gem_create_context() for > better code structure. Hm, it doesn't move the assignment it adds it. Previously ctx->ppgtt was always NULL when !USES_FULL_PPGTT. Now it will point to aliasing ppgtt. Since there are various places in the code which do "if (ctx->ppgtt)" (more or less) I think you need to explain why those will still work OK. i915_gem_context_free for example? Regards, Tvrtko > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 4 +++- > drivers/gpu/drm/i915/intel_lrc.c | 3 --- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index 2aedd18..21498e5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -300,6 +300,7 @@ static struct intel_context * > i915_gem_create_context(struct drm_device *dev, > struct drm_i915_file_private *file_priv) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > const bool is_global_default_ctx = file_priv == NULL; > struct intel_context *ctx; > int ret = 0; > @@ -337,7 +338,8 @@ i915_gem_create_context(struct drm_device *dev, > } > > ctx->ppgtt = ppgtt; > - } > + } else > + ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; > > trace_i915_context_create(ctx); > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index db10c96..397fe65 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -2286,9 +2286,6 @@ populate_lr_context(struct intel_context *ctx, > u32 *reg_state; > int ret; > > - if (!ppgtt) > - 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"); >
Looks like I can drop this patch without harm. As I will load the root pointer myself. And there is patch to check if ppgtt == something then populate PDPs, so under FULL_PPGTT && !NO_PPGTT_CTX it won't load anything. Under !FULL_PPGTT && !NO_PPGTT_CTX, it will load aliasing_ppgtt_mm, but then I will load PDPs myself, so it's also OK. -----Original Message----- From: Wang, Zhi A Sent: Monday, May 16, 2016 7:16 AM To: 'Tvrtko Ursulin' <tvrtko.ursulin@linux.intel.com>; 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 05/15] drm/i915: Set ctx->ppgtt for aliasing PPGTT in context creation I understand what you say. Let me think about other approach. My requirement is to do nothing if a context doesn't have a ppgtt, but ctx->ppgtt == NULL means context is using aliasing ppgtt, so I need to find another way to achieve this. -----Original Message----- From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] Sent: Monday, May 16, 2016 6:31 AM To: Wang, Zhi A <zhi.a.wang@intel.com>; 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 05/15] drm/i915: Set ctx->ppgtt for aliasing PPGTT in context creation On 15/05/16 18:32, Zhi Wang wrote: > Currently ctx->ppgtt would only be initialized when full PPGTT is used. > For aliasing PPGTT mode, ctx->ppgtt will be set when LRC context is > populated. > > This patch moves the assignment into i915_gem_create_context() for > better code structure. Hm, it doesn't move the assignment it adds it. Previously ctx->ppgtt was always NULL when !USES_FULL_PPGTT. Now it will point to aliasing ppgtt. Since there are various places in the code which do "if (ctx->ppgtt)" (more or less) I think you need to explain why those will still work OK. i915_gem_context_free for example? Regards, Tvrtko > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 4 +++- > drivers/gpu/drm/i915/intel_lrc.c | 3 --- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index 2aedd18..21498e5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -300,6 +300,7 @@ static struct intel_context * > i915_gem_create_context(struct drm_device *dev, > struct drm_i915_file_private *file_priv) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > const bool is_global_default_ctx = file_priv == NULL; > struct intel_context *ctx; > int ret = 0; > @@ -337,7 +338,8 @@ i915_gem_create_context(struct drm_device *dev, > } > > ctx->ppgtt = ppgtt; > - } > + } else > + ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; > > trace_i915_context_create(ctx); > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index db10c96..397fe65 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -2286,9 +2286,6 @@ populate_lr_context(struct intel_context *ctx, > u32 *reg_state; > int ret; > > - if (!ppgtt) > - 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"); >
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 2aedd18..21498e5 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -300,6 +300,7 @@ static struct intel_context * i915_gem_create_context(struct drm_device *dev, struct drm_i915_file_private *file_priv) { + struct drm_i915_private *dev_priv = dev->dev_private; const bool is_global_default_ctx = file_priv == NULL; struct intel_context *ctx; int ret = 0; @@ -337,7 +338,8 @@ i915_gem_create_context(struct drm_device *dev, } ctx->ppgtt = ppgtt; - } + } else + ctx->ppgtt = dev_priv->mm.aliasing_ppgtt; trace_i915_context_create(ctx); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index db10c96..397fe65 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2286,9 +2286,6 @@ populate_lr_context(struct intel_context *ctx, u32 *reg_state; int ret; - if (!ppgtt) - 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");
Currently ctx->ppgtt would only be initialized when full PPGTT is used. For aliasing PPGTT mode, ctx->ppgtt will be set when LRC context is populated. This patch moves the assignment into i915_gem_create_context() for better code structure. Signed-off-by: Zhi Wang <zhi.a.wang@intel.com> --- drivers/gpu/drm/i915/i915_gem_context.c | 4 +++- drivers/gpu/drm/i915/intel_lrc.c | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-)