On Fri, Jul 12, 2013 at 09:45:54PM -0700, Ben Widawsky wrote:
> As we plumb the code with more VM information, it has become more
> obvious that the easiest way to deal with bind and unbind is to simply
> put the function pointers in the vm, and let those choose the correct
> way to handle the page table updates. This change allows many places in
> the code to simply be vm->bind, and not have to worry about
> distinguishing PPGTT vs GGTT.
>
> NOTE: At some point in the future, brining back insert_entries may in
> fact be desirable in order to use 1 bind/unbind for multiple generations
> of PPGTT. For now however, it's just not necessary.
I need to check the -internal tree again, but I'm rather sure that we need
->insert_entries. In that case I don't want to remove it here in the
upstream tree since I have no intention to carry the re-add patch in
-internal ;-)
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 9 +++++
> drivers/gpu/drm/i915/i915_gem_gtt.c | 72 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 81 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e6694ae..c2a9c98 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -484,9 +484,18 @@ struct i915_address_space {
> /* FIXME: Need a more generic return type */
> gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr,
> enum i915_cache_level level);
> +
> + /** Unmap an object from an address space. This usually consists of
> + * setting the valid PTE entries to a reserved scratch page. */
> + void (*unbind_object)(struct i915_address_space *vm,
> + struct drm_i915_gem_object *obj);
void (*unbind_vma)(struct i915_vma *vma);
void (*bind_vma)(struct i915_vma *vma,
enum i915_cache_level cache_level);
I think if you do this as a follow-up we might as well bikeshed the
interface a bit. Again (I know, broken record) for me it feels
semantically much cleaner to talk about binding/unbindinig a vma instead
of an (obj, vm) pair ...
> void (*clear_range)(struct i915_address_space *vm,
> unsigned int first_entry,
> unsigned int num_entries);
> + /* Map an object into an address space with the given cache flags. */
> + void (*bind_object)(struct i915_address_space *vm,
> + struct drm_i915_gem_object *obj,
> + enum i915_cache_level cache_level);
> void (*insert_entries)(struct i915_address_space *vm,
> struct sg_table *st,
> unsigned int first_entry,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index c0d0223..31ff971 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -45,6 +45,12 @@
> #define GEN6_PTE_CACHE_LLC_MLC (3 << 1)
> #define GEN6_PTE_ADDR_ENCODE(addr) GEN6_GTT_ADDR_ENCODE(addr)
>
> +static void gen6_ppgtt_bind_object(struct i915_address_space *vm,
> + struct drm_i915_gem_object *obj,
> + enum i915_cache_level cache_level);
> +static void gen6_ppgtt_unbind_object(struct i915_address_space *vm,
> + struct drm_i915_gem_object *obj);
> +
> static gen6_gtt_pte_t gen6_pte_encode(dma_addr_t addr,
> enum i915_cache_level level)
> {
> @@ -285,7 +291,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> }
> ppgtt->num_pd_entries = GEN6_PPGTT_PD_ENTRIES;
> ppgtt->enable = gen6_ppgtt_enable;
> + ppgtt->base.unbind_object = gen6_ppgtt_unbind_object;
> ppgtt->base.clear_range = gen6_ppgtt_clear_range;
> + ppgtt->base.bind_object = gen6_ppgtt_bind_object;
> ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
> ppgtt->base.cleanup = gen6_ppgtt_cleanup;
> ppgtt->base.scratch = dev_priv->gtt.base.scratch;
> @@ -397,6 +405,17 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
> cache_level);
> }
>
> +static void gen6_ppgtt_bind_object(struct i915_address_space *vm,
> + struct drm_i915_gem_object *obj,
> + enum i915_cache_level cache_level)
> +{
> + const unsigned long entry = i915_gem_obj_offset(obj, vm);
> +
> + gen6_ppgtt_insert_entries(vm, obj->pages, entry >> PAGE_SHIFT,
> + cache_level);
> + obj->has_aliasing_ppgtt_mapping = 1;
Since this is the bind function for ppgtt the aliasing ppgtt stuff looks a
bit wrong here. Either we do the ppgtt insert_entries call as part of the
global gtt bind call (if vm->aliasing_ppgtt is set) or we have a special
global gtt binding call for execbuf.
Thinking about this some more we might need bind flags with
#define VMA_BIND_CPU (1<<0) /* ensure ggtt mapping exists for aliasing ppgtt */
#define VMA_BIND_GPU (1<<1) /* ensure ppgtt mappings exists for aliasing ppgtt */
since otherwise we can't properly encapsulate the aliasing ppgtt binding
logic into vm->bind. So in the end we'd have
void ggtt_bind_vma(vma, bind_flags, cache_level)
{
ggtt_vm = vma->vm;
WARN_ON(ggtt_vm != &dev_priv->gtt.base);
if ((!ggtt_vm->aliasing_ppgtt || (bind_flags & BIND_CPU)) &&
!obj->has_global_gtt_mapping) {
ggtt_vm->insert_entries(vma->obj, vma->node.start, cache_leve);
vma->obj->has_global_gtt_mapping = true;
}
if ((ggtt_vm->aliasing_ppgtt && (bind_flags & BIND_GPU)) &&
!obj->has_ppgtt_mapping) {
ggtt_vm->aliasing_ppgtt->insert_entries(vma->obj,
vma->node.start,
cache_leve);
vma->obj->has_ppgtt_mapping = true;
}
}
Obviously completely untested, but I hope I could get the idea accross.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
