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 at 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 at 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
--
Ben Widawsky, Intel Open Source Technology Center
