| Message ID | 1372375867-1003-25-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 27, 2013 at 04:30:25PM -0700, Ben Widawsky wrote: > for file in `ls drivers/gpu/drm/i915/*.c` ; do > sed -i "s/mm.aliasing/gtt.aliasing/" $file; > done Commit message should explain _why_ we do something. Again I'm asking since I'm unclear about how things fit all together and what the different responsibilities are. I think I understand your design here to make both real ppgtt work and keep aliasing ppgtt going, but I'd like you to explain this in your words ;-) One thing which looks a bit peculiar at the end is that struct i915_hw_ppgtt is actually used as the real ppgtt object (since it subclases i915_address space). My original plan was that we'll add a new struct i915_ppgtt { struct i915_address_space base; struct i915_hw_ppgtt hw_ppgtt; } To fit into your design the alising ppgtt pointer in dev_priv->gtt would then only point at a hw_ppgtt struct, not the full deal with address space and everything else around attached. Cheers, Daniel > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- > drivers/gpu/drm/i915/i915_dma.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 6 +++--- > drivers/gpu/drm/i915/i915_gem.c | 12 ++++++------ > drivers/gpu/drm/i915/i915_gem_context.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++--- > 7 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index c10a690..f3c76ab 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1816,8 +1816,8 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) > seq_printf(m, "PP_DIR_BASE_READ: 0x%08x\n", I915_READ(RING_PP_DIR_BASE_READ(ring))); > seq_printf(m, "PP_DIR_DCLV: 0x%08x\n", I915_READ(RING_PP_DIR_DCLV(ring))); > } > - if (dev_priv->mm.aliasing_ppgtt) { > - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; > + if (dev_priv->gtt.aliasing_ppgtt) { > + struct i915_hw_ppgtt *ppgtt = dev_priv->gtt.aliasing_ppgtt; > > seq_printf(m, "aliasing PPGTT:\n"); > seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset); > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 3535ced..ef00847 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -977,7 +977,7 @@ static int i915_getparam(struct drm_device *dev, void *data, > value = HAS_LLC(dev); > break; > case I915_PARAM_HAS_ALIASING_PPGTT: > - if (intel_enable_ppgtt(dev) && dev_priv->mm.aliasing_ppgtt) > + if (intel_enable_ppgtt(dev) && dev_priv->gtt.aliasing_ppgtt) > value = 1; > break; > case I915_PARAM_HAS_WAIT_TIMEOUT: > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7016074..0fa7a21 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -482,6 +482,9 @@ struct i915_gtt { > struct io_mapping *mappable; /* Mapping to our CPU mappable region */ > phys_addr_t mappable_base; /* PA of our GMADR */ > > + /** PPGTT used for aliasing the PPGTT with the GTT */ > + struct i915_hw_ppgtt *aliasing_ppgtt; > + > /** "Graphics Stolen Memory" holds the global PTEs */ > void __iomem *gsm; > > @@ -843,9 +846,6 @@ struct i915_gem_mm { > */ > struct list_head unbound_list; > > - /** PPGTT used for aliasing the PPGTT with the GTT */ > - struct i915_hw_ppgtt *aliasing_ppgtt; > - > struct shrinker inactive_shrinker; > bool shrinker_no_lock_stealing; > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index e31ed47..eb78c5b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2620,7 +2620,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj) > if (obj->has_global_gtt_mapping) > i915_gem_gtt_unbind_object(obj); > if (obj->has_aliasing_ppgtt_mapping) { > - i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); > + i915_ppgtt_unbind_object(dev_priv->gtt.aliasing_ppgtt, obj); > obj->has_aliasing_ppgtt_mapping = 0; > } > i915_gem_gtt_finish_object(obj); > @@ -3359,7 +3359,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, > if (obj->has_global_gtt_mapping) > i915_gem_gtt_bind_object(obj, cache_level); > if (obj->has_aliasing_ppgtt_mapping) > - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt, > + i915_ppgtt_bind_object(dev_priv->gtt.aliasing_ppgtt, > obj, cache_level); > > obj->gtt_space->color = cache_level; > @@ -3668,7 +3668,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, > if (ret) > return ret; > > - if (!dev_priv->mm.aliasing_ppgtt) > + if (!dev_priv->gtt.aliasing_ppgtt) > i915_gem_gtt_bind_object(obj, obj->cache_level); > } > > @@ -4191,10 +4191,10 @@ i915_gem_init_hw(struct drm_device *dev) > * the do_switch), but before enabling PPGTT. So don't move this. > */ > ret = i915_gem_context_enable(dev_priv); > - if (ret || !dev_priv->mm.aliasing_ppgtt) > + if (ret || !dev_priv->gtt.aliasing_ppgtt) > goto disable_ctx_out; > > - ret = dev_priv->mm.aliasing_ppgtt->enable(dev); > + ret = dev_priv->gtt.aliasing_ppgtt->enable(dev); > if (ret) > goto disable_ctx_out; > > @@ -4236,7 +4236,7 @@ int i915_gem_init(struct drm_device *dev) > dev_priv->hw_contexts_disabled = true; > > ggtt_only: > - if (!dev_priv->mm.aliasing_ppgtt) { > + if (!dev_priv->gtt.aliasing_ppgtt) { > if (HAS_HW_CONTEXTS(dev)) > DRM_DEBUG_DRIVER("Context setup failed %d\n", ret); > i915_gem_setup_global_gtt(dev, 0, dev_priv->gtt.mappable_end, > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index d92f121..aa4fc4a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -226,7 +226,7 @@ static int create_default_context(struct drm_i915_private *dev_priv) > } > > dev_priv->ring[RCS].default_context = ctx; > - dev_priv->mm.aliasing_ppgtt = &ctx->ppgtt; > + dev_priv->gtt.aliasing_ppgtt = &ctx->ppgtt; > > DRM_DEBUG_DRIVER("Default HW context loaded\n"); > return 0; > @@ -300,7 +300,7 @@ void i915_gem_context_fini(struct drm_device *dev) > i915_gem_context_unreference(dctx); > dev_priv->ring[RCS].default_context = NULL; > dev_priv->ring[RCS].last_context = NULL; > - dev_priv->mm.aliasing_ppgtt = NULL; > + dev_priv->gtt.aliasing_ppgtt = NULL; > } > > int i915_gem_context_enable(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 7fcd6c0..93870bb 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -429,8 +429,8 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj, > } > > /* Ensure ppgtt mapping exists if needed */ > - if (dev_priv->mm.aliasing_ppgtt && !obj->has_aliasing_ppgtt_mapping) { > - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt, > + if (dev_priv->gtt.aliasing_ppgtt && !obj->has_aliasing_ppgtt_mapping) { > + i915_ppgtt_bind_object(dev_priv->gtt.aliasing_ppgtt, > obj, obj->cache_level); > > obj->has_aliasing_ppgtt_mapping = 1; > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 6de75c7..18820cb 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -127,7 +127,7 @@ static int gen6_ppgtt_enable(struct drm_device *dev) > drm_i915_private_t *dev_priv = dev->dev_private; > uint32_t pd_offset; > struct intel_ring_buffer *ring; > - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; > + struct i915_hw_ppgtt *ppgtt = dev_priv->gtt.aliasing_ppgtt; > int i; > > BUG_ON(ppgtt->pd_offset & 0x3f); > @@ -445,8 +445,8 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) > i915_gtt_vm->start / PAGE_SIZE, > i915_gtt_vm->total / PAGE_SIZE); > > - if (dev_priv->mm.aliasing_ppgtt) > - gen6_write_pdes(dev_priv->mm.aliasing_ppgtt); > + if (dev_priv->gtt.aliasing_ppgtt) > + gen6_write_pdes(dev_priv->gtt.aliasing_ppgtt); > > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { > i915_gem_clflush_object(obj); > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sun, Jun 30, 2013 at 03:27:44PM +0200, Daniel Vetter wrote: > On Thu, Jun 27, 2013 at 04:30:25PM -0700, Ben Widawsky wrote: > > for file in `ls drivers/gpu/drm/i915/*.c` ; do > > sed -i "s/mm.aliasing/gtt.aliasing/" $file; > > done > > Commit message should explain _why_ we do something. Again I'm asking > since I'm unclear about how things fit all together and what the different > responsibilities are. I think I understand your design here to make both > real ppgtt work and keep aliasing ppgtt going, but I'd like you to explain > this in your words ;-) That's fair. > > One thing which looks a bit peculiar at the end is that struct > i915_hw_ppgtt is actually used as the real ppgtt object (since it > subclases i915_address space). My original plan was that we'll add a new > struct i915_ppgtt { > struct i915_address_space base; > struct i915_hw_ppgtt hw_ppgtt; > } > > To fit into your design the alising ppgtt pointer in dev_priv->gtt would > then only point at a hw_ppgtt struct, not the full deal with address space > and everything else around attached. > > Cheers, Daniel I don't think creating a struct i915_ppgtt is necessary or buys much. We can rename i915_hw_ppgtt to i915_ppgtt, and it accomplishes the same thing. Same for the i915_hw_context for that matter. I wanted to do any sort of renaming after the rest of the series. Can you explain why we'd want to keep the hw_ppgtt, and ppgtt with the track lists etc. distinct? > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- > > drivers/gpu/drm/i915/i915_dma.c | 2 +- > > drivers/gpu/drm/i915/i915_drv.h | 6 +++--- > > drivers/gpu/drm/i915/i915_gem.c | 12 ++++++------ > > drivers/gpu/drm/i915/i915_gem_context.c | 4 ++-- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 ++-- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++--- > > 7 files changed, 19 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > index c10a690..f3c76ab 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -1816,8 +1816,8 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) > > seq_printf(m, "PP_DIR_BASE_READ: 0x%08x\n", I915_READ(RING_PP_DIR_BASE_READ(ring))); > > seq_printf(m, "PP_DIR_DCLV: 0x%08x\n", I915_READ(RING_PP_DIR_DCLV(ring))); > > } > > - if (dev_priv->mm.aliasing_ppgtt) { > > - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; > > + if (dev_priv->gtt.aliasing_ppgtt) { > > + struct i915_hw_ppgtt *ppgtt = dev_priv->gtt.aliasing_ppgtt; > > > > seq_printf(m, "aliasing PPGTT:\n"); > > seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset); > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > index 3535ced..ef00847 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -977,7 +977,7 @@ static int i915_getparam(struct drm_device *dev, void *data, > > value = HAS_LLC(dev); > > break; > > case I915_PARAM_HAS_ALIASING_PPGTT: > > - if (intel_enable_ppgtt(dev) && dev_priv->mm.aliasing_ppgtt) > > + if (intel_enable_ppgtt(dev) && dev_priv->gtt.aliasing_ppgtt) > > value = 1; > > break; > > case I915_PARAM_HAS_WAIT_TIMEOUT: > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 7016074..0fa7a21 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -482,6 +482,9 @@ struct i915_gtt { > > struct io_mapping *mappable; /* Mapping to our CPU mappable region */ > > phys_addr_t mappable_base; /* PA of our GMADR */ > > > > + /** PPGTT used for aliasing the PPGTT with the GTT */ > > + struct i915_hw_ppgtt *aliasing_ppgtt; > > + > > /** "Graphics Stolen Memory" holds the global PTEs */ > > void __iomem *gsm; > > > > @@ -843,9 +846,6 @@ struct i915_gem_mm { > > */ > > struct list_head unbound_list; > > > > - /** PPGTT used for aliasing the PPGTT with the GTT */ > > - struct i915_hw_ppgtt *aliasing_ppgtt; > > - > > struct shrinker inactive_shrinker; > > bool shrinker_no_lock_stealing; > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index e31ed47..eb78c5b 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -2620,7 +2620,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj) > > if (obj->has_global_gtt_mapping) > > i915_gem_gtt_unbind_object(obj); > > if (obj->has_aliasing_ppgtt_mapping) { > > - i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); > > + i915_ppgtt_unbind_object(dev_priv->gtt.aliasing_ppgtt, obj); > > obj->has_aliasing_ppgtt_mapping = 0; > > } > > i915_gem_gtt_finish_object(obj); > > @@ -3359,7 +3359,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, > > if (obj->has_global_gtt_mapping) > > i915_gem_gtt_bind_object(obj, cache_level); > > if (obj->has_aliasing_ppgtt_mapping) > > - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt, > > + i915_ppgtt_bind_object(dev_priv->gtt.aliasing_ppgtt, > > obj, cache_level); > > > > obj->gtt_space->color = cache_level; > > @@ -3668,7 +3668,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, > > if (ret) > > return ret; > > > > - if (!dev_priv->mm.aliasing_ppgtt) > > + if (!dev_priv->gtt.aliasing_ppgtt) > > i915_gem_gtt_bind_object(obj, obj->cache_level); > > } > > > > @@ -4191,10 +4191,10 @@ i915_gem_init_hw(struct drm_device *dev) > > * the do_switch), but before enabling PPGTT. So don't move this. > > */ > > ret = i915_gem_context_enable(dev_priv); > > - if (ret || !dev_priv->mm.aliasing_ppgtt) > > + if (ret || !dev_priv->gtt.aliasing_ppgtt) > > goto disable_ctx_out; > > > > - ret = dev_priv->mm.aliasing_ppgtt->enable(dev); > > + ret = dev_priv->gtt.aliasing_ppgtt->enable(dev); > > if (ret) > > goto disable_ctx_out; > > > > @@ -4236,7 +4236,7 @@ int i915_gem_init(struct drm_device *dev) > > dev_priv->hw_contexts_disabled = true; > > > > ggtt_only: > > - if (!dev_priv->mm.aliasing_ppgtt) { > > + if (!dev_priv->gtt.aliasing_ppgtt) { > > if (HAS_HW_CONTEXTS(dev)) > > DRM_DEBUG_DRIVER("Context setup failed %d\n", ret); > > i915_gem_setup_global_gtt(dev, 0, dev_priv->gtt.mappable_end, > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > index d92f121..aa4fc4a 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -226,7 +226,7 @@ static int create_default_context(struct drm_i915_private *dev_priv) > > } > > > > dev_priv->ring[RCS].default_context = ctx; > > - dev_priv->mm.aliasing_ppgtt = &ctx->ppgtt; > > + dev_priv->gtt.aliasing_ppgtt = &ctx->ppgtt; > > > > DRM_DEBUG_DRIVER("Default HW context loaded\n"); > > return 0; > > @@ -300,7 +300,7 @@ void i915_gem_context_fini(struct drm_device *dev) > > i915_gem_context_unreference(dctx); > > dev_priv->ring[RCS].default_context = NULL; > > dev_priv->ring[RCS].last_context = NULL; > > - dev_priv->mm.aliasing_ppgtt = NULL; > > + dev_priv->gtt.aliasing_ppgtt = NULL; > > } > > > > int i915_gem_context_enable(struct drm_i915_private *dev_priv) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index 7fcd6c0..93870bb 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -429,8 +429,8 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj, > > } > > > > /* Ensure ppgtt mapping exists if needed */ > > - if (dev_priv->mm.aliasing_ppgtt && !obj->has_aliasing_ppgtt_mapping) { > > - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt, > > + if (dev_priv->gtt.aliasing_ppgtt && !obj->has_aliasing_ppgtt_mapping) { > > + i915_ppgtt_bind_object(dev_priv->gtt.aliasing_ppgtt, > > obj, obj->cache_level); > > > > obj->has_aliasing_ppgtt_mapping = 1; > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index 6de75c7..18820cb 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -127,7 +127,7 @@ static int gen6_ppgtt_enable(struct drm_device *dev) > > drm_i915_private_t *dev_priv = dev->dev_private; > > uint32_t pd_offset; > > struct intel_ring_buffer *ring; > > - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; > > + struct i915_hw_ppgtt *ppgtt = dev_priv->gtt.aliasing_ppgtt; > > int i; > > > > BUG_ON(ppgtt->pd_offset & 0x3f); > > @@ -445,8 +445,8 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) > > i915_gtt_vm->start / PAGE_SIZE, > > i915_gtt_vm->total / PAGE_SIZE); > > > > - if (dev_priv->mm.aliasing_ppgtt) > > - gen6_write_pdes(dev_priv->mm.aliasing_ppgtt); > > + if (dev_priv->gtt.aliasing_ppgtt) > > + gen6_write_pdes(dev_priv->gtt.aliasing_ppgtt); > > > > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { > > i915_gem_clflush_object(obj); > > -- > > 1.8.3.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jul 1, 2013 at 8:52 PM, Ben Widawsky <ben@bwidawsk.net> wrote: >> One thing which looks a bit peculiar at the end is that struct >> i915_hw_ppgtt is actually used as the real ppgtt object (since it >> subclases i915_address space). My original plan was that we'll add a new >> struct i915_ppgtt { >> struct i915_address_space base; >> struct i915_hw_ppgtt hw_ppgtt; >> } >> >> To fit into your design the alising ppgtt pointer in dev_priv->gtt would >> then only point at a hw_ppgtt struct, not the full deal with address space >> and everything else around attached. >> >> Cheers, Daniel > > I don't think creating a struct i915_ppgtt is necessary or buys much. We > can rename i915_hw_ppgtt to i915_ppgtt, and it accomplishes the same > thing. Same for the i915_hw_context for that matter. I wanted to do any > sort of renaming after the rest of the series. > > Can you explain why we'd want to keep the hw_ppgtt, and ppgtt with the > track lists etc. distinct? The difference is that for the aliasing ppgtt we don't really need a address space object to track object offsets since those are identical to the ggtt offsets. Hence we only need the hw part of the ppgtt, i.e. writing/clearing ptes. Full ppgtt otoh need the address space of course. The above structure layout would allow that. It means that aliasing ppgtt will stick out a bit like a sore thumb but imo that's ok since it really _is_ a bit an odd thing. I've just noticed that you've partially disabled the has_aliasing_ppgtt_mapping logic in your branch, so I wonder a bit how aliasing ppgtt works there. But I guess the simplest approach would be to simply keep that stuff around as-is, with a few twists to wrestle it into the new world: Batches would be executed as if they'd run in the global gtt (i.e. for eviction/binding purposes) with the simple difference that we'd check whether the global gtt has the aliasing_ppgtt pointer set up, and then write the ptes there. Note that unconditionally writing both ppgtt and global gtt ptes isn't too good, since in some virtualized enviroments tracking those separately is a pretty substantial win apparently. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jul 01, 2013 at 09:06:20PM +0200, Daniel Vetter wrote: > On Mon, Jul 1, 2013 at 8:52 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > >> One thing which looks a bit peculiar at the end is that struct > >> i915_hw_ppgtt is actually used as the real ppgtt object (since it > >> subclases i915_address space). My original plan was that we'll add a new > >> struct i915_ppgtt { > >> struct i915_address_space base; > >> struct i915_hw_ppgtt hw_ppgtt; > >> } > >> > >> To fit into your design the alising ppgtt pointer in dev_priv->gtt would > >> then only point at a hw_ppgtt struct, not the full deal with address space > >> and everything else around attached. > >> > >> Cheers, Daniel > > > > I don't think creating a struct i915_ppgtt is necessary or buys much. We > > can rename i915_hw_ppgtt to i915_ppgtt, and it accomplishes the same > > thing. Same for the i915_hw_context for that matter. I wanted to do any > > sort of renaming after the rest of the series. > > > > Can you explain why we'd want to keep the hw_ppgtt, and ppgtt with the > > track lists etc. distinct? > > The difference is that for the aliasing ppgtt we don't really need a > address space object to track object offsets since those are identical > to the ggtt offsets. Hence we only need the hw part of the ppgtt, i.e. > writing/clearing ptes. I actually did attempt to make the aliasing ppgtt a real address space (so the ggtt was the special case instead of vice versa). It proved too much work, so I dropped it. > > Full ppgtt otoh need the address space of course. > > The above structure layout would allow that. It means that aliasing > ppgtt will stick out a bit like a sore thumb but imo that's ok since > it really _is_ a bit an odd thing. I've just noticed that you've > partially disabled the has_aliasing_ppgtt_mapping logic in your > branch, so I wonder a bit how aliasing ppgtt works there. It doesn't. The intent was to completely eclipse aliasing PPGTT. If we keep the pin interface (as discussed in the other thread), I am not certain that is still true, though I personally have no issue forcing a performance penalty on users that wish to use that interface. > But I guess > the simplest approach would be to simply keep that stuff around as-is, > with a few twists to wrestle it into the new world: Batches would be > executed as if they'd run in the global gtt (i.e. for eviction/binding > purposes) with the simple difference that we'd check whether the > global gtt has the aliasing_ppgtt pointer set up, and then write the > ptes there. Note that unconditionally writing both ppgtt and global > gtt ptes isn't too good, since in some virtualized enviroments > tracking those separately is a pretty substantial win apparently. > -Daniel I'm still not convinced that having distinct i915_ppgtt and i915_hw_ppgtt buys us anything. > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Jul 1, 2013 at 9:48 PM, Ben Widawsky <ben@bwidawsk.net> wrote: >> Full ppgtt otoh need the address space of course. >> >> The above structure layout would allow that. It means that aliasing >> ppgtt will stick out a bit like a sore thumb but imo that's ok since >> it really _is_ a bit an odd thing. I've just noticed that you've >> partially disabled the has_aliasing_ppgtt_mapping logic in your >> branch, so I wonder a bit how aliasing ppgtt works there. > > It doesn't. The intent was to completely eclipse aliasing PPGTT. If we > keep the pin interface (as discussed in the other thread), I am not > certain that is still true, though I personally have no issue forcing a > performance penalty on users that wish to use that interface. Ah, that explains my confusion ;-) I guess we simply have to be careful to keep this working properly when merging the patches. Like I've said I'm pretty sure that we can keep all the major concepts unchanged and just bolt aliasing ppgtt onto the side like it's done today. For i915_ppgtt vs. i915_hw_ppgtt I think we can revisite that we the in-depth review of the relevant patches is on the table. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index c10a690..f3c76ab 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1816,8 +1816,8 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) seq_printf(m, "PP_DIR_BASE_READ: 0x%08x\n", I915_READ(RING_PP_DIR_BASE_READ(ring))); seq_printf(m, "PP_DIR_DCLV: 0x%08x\n", I915_READ(RING_PP_DIR_DCLV(ring))); } - if (dev_priv->mm.aliasing_ppgtt) { - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; + if (dev_priv->gtt.aliasing_ppgtt) { + struct i915_hw_ppgtt *ppgtt = dev_priv->gtt.aliasing_ppgtt; seq_printf(m, "aliasing PPGTT:\n"); seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset); diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 3535ced..ef00847 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -977,7 +977,7 @@ static int i915_getparam(struct drm_device *dev, void *data, value = HAS_LLC(dev); break; case I915_PARAM_HAS_ALIASING_PPGTT: - if (intel_enable_ppgtt(dev) && dev_priv->mm.aliasing_ppgtt) + if (intel_enable_ppgtt(dev) && dev_priv->gtt.aliasing_ppgtt) value = 1; break; case I915_PARAM_HAS_WAIT_TIMEOUT: diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7016074..0fa7a21 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -482,6 +482,9 @@ struct i915_gtt { struct io_mapping *mappable; /* Mapping to our CPU mappable region */ phys_addr_t mappable_base; /* PA of our GMADR */ + /** PPGTT used for aliasing the PPGTT with the GTT */ + struct i915_hw_ppgtt *aliasing_ppgtt; + /** "Graphics Stolen Memory" holds the global PTEs */ void __iomem *gsm; @@ -843,9 +846,6 @@ struct i915_gem_mm { */ struct list_head unbound_list; - /** PPGTT used for aliasing the PPGTT with the GTT */ - struct i915_hw_ppgtt *aliasing_ppgtt; - struct shrinker inactive_shrinker; bool shrinker_no_lock_stealing; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e31ed47..eb78c5b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2620,7 +2620,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj) if (obj->has_global_gtt_mapping) i915_gem_gtt_unbind_object(obj); if (obj->has_aliasing_ppgtt_mapping) { - i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); + i915_ppgtt_unbind_object(dev_priv->gtt.aliasing_ppgtt, obj); obj->has_aliasing_ppgtt_mapping = 0; } i915_gem_gtt_finish_object(obj); @@ -3359,7 +3359,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, if (obj->has_global_gtt_mapping) i915_gem_gtt_bind_object(obj, cache_level); if (obj->has_aliasing_ppgtt_mapping) - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt, + i915_ppgtt_bind_object(dev_priv->gtt.aliasing_ppgtt, obj, cache_level); obj->gtt_space->color = cache_level; @@ -3668,7 +3668,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, if (ret) return ret; - if (!dev_priv->mm.aliasing_ppgtt) + if (!dev_priv->gtt.aliasing_ppgtt) i915_gem_gtt_bind_object(obj, obj->cache_level); } @@ -4191,10 +4191,10 @@ i915_gem_init_hw(struct drm_device *dev) * the do_switch), but before enabling PPGTT. So don't move this. */ ret = i915_gem_context_enable(dev_priv); - if (ret || !dev_priv->mm.aliasing_ppgtt) + if (ret || !dev_priv->gtt.aliasing_ppgtt) goto disable_ctx_out; - ret = dev_priv->mm.aliasing_ppgtt->enable(dev); + ret = dev_priv->gtt.aliasing_ppgtt->enable(dev); if (ret) goto disable_ctx_out; @@ -4236,7 +4236,7 @@ int i915_gem_init(struct drm_device *dev) dev_priv->hw_contexts_disabled = true; ggtt_only: - if (!dev_priv->mm.aliasing_ppgtt) { + if (!dev_priv->gtt.aliasing_ppgtt) { if (HAS_HW_CONTEXTS(dev)) DRM_DEBUG_DRIVER("Context setup failed %d\n", ret); i915_gem_setup_global_gtt(dev, 0, dev_priv->gtt.mappable_end, diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index d92f121..aa4fc4a 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -226,7 +226,7 @@ static int create_default_context(struct drm_i915_private *dev_priv) } dev_priv->ring[RCS].default_context = ctx; - dev_priv->mm.aliasing_ppgtt = &ctx->ppgtt; + dev_priv->gtt.aliasing_ppgtt = &ctx->ppgtt; DRM_DEBUG_DRIVER("Default HW context loaded\n"); return 0; @@ -300,7 +300,7 @@ void i915_gem_context_fini(struct drm_device *dev) i915_gem_context_unreference(dctx); dev_priv->ring[RCS].default_context = NULL; dev_priv->ring[RCS].last_context = NULL; - dev_priv->mm.aliasing_ppgtt = NULL; + dev_priv->gtt.aliasing_ppgtt = NULL; } int i915_gem_context_enable(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 7fcd6c0..93870bb 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -429,8 +429,8 @@ i915_gem_execbuffer_reserve_object(struct drm_i915_gem_object *obj, } /* Ensure ppgtt mapping exists if needed */ - if (dev_priv->mm.aliasing_ppgtt && !obj->has_aliasing_ppgtt_mapping) { - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt, + if (dev_priv->gtt.aliasing_ppgtt && !obj->has_aliasing_ppgtt_mapping) { + i915_ppgtt_bind_object(dev_priv->gtt.aliasing_ppgtt, obj, obj->cache_level); obj->has_aliasing_ppgtt_mapping = 1; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 6de75c7..18820cb 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -127,7 +127,7 @@ static int gen6_ppgtt_enable(struct drm_device *dev) drm_i915_private_t *dev_priv = dev->dev_private; uint32_t pd_offset; struct intel_ring_buffer *ring; - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; + struct i915_hw_ppgtt *ppgtt = dev_priv->gtt.aliasing_ppgtt; int i; BUG_ON(ppgtt->pd_offset & 0x3f); @@ -445,8 +445,8 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) i915_gtt_vm->start / PAGE_SIZE, i915_gtt_vm->total / PAGE_SIZE); - if (dev_priv->mm.aliasing_ppgtt) - gen6_write_pdes(dev_priv->mm.aliasing_ppgtt); + if (dev_priv->gtt.aliasing_ppgtt) + gen6_write_pdes(dev_priv->gtt.aliasing_ppgtt); list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { i915_gem_clflush_object(obj);
for file in `ls drivers/gpu/drm/i915/*.c` ; do sed -i "s/mm.aliasing/gtt.aliasing/" $file; done Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- drivers/gpu/drm/i915/i915_dma.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 6 +++--- drivers/gpu/drm/i915/i915_gem.c | 12 ++++++------ drivers/gpu/drm/i915/i915_gem_context.c | 4 ++-- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 ++-- drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++--- 7 files changed, 19 insertions(+), 19 deletions(-)