[Intel-gfx] [PATCH 11/17] drm/i915: Support to create write combined type vmaps

[Chris Wilson]

On Fri, Jul 15, 2016 at 12:31:23PM +0100, Tvrtko Ursulin wrote:

[snip good ideas, leaving the questions]

> >  /**
> >   * i915_gem_object_unpin_map - releases an earlier mapping
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 8f50919..c431b40 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2471,10 +2471,11 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
> >  	list_del(&obj->global_list);
> >
> >  	if (obj->mapping) {
> >-		if (is_vmalloc_addr(obj->mapping))
> >-			vunmap(obj->mapping);
> >+		void *ptr = (void *)((uintptr_t)obj->mapping & ~1);
> 
> How many bits we have to play with here? Is there a suitable define
> somewhere we could use for a mask instead of hardcoded "1" or we
> could add one if you think that would be better?

The mapping is always page-aligned, so bits 0-11 are available.

PAGE_MASK should do the trick

> >-	addr = vmap(pages, n_pages, 0, PAGE_KERNEL);
> >+	addr = vmap(pages, n_pages, VM_NO_GUARD,
> >+		    use_wc ? pgprot_writecombine(PAGE_KERNEL_IO) : PAGE_KERNEL);
> 
> For educational benefit, what is the importance and difference
> between PAGE_KERNEL and PAGE_KERNEL_IO here?

One day I'll find out. We've always tagged our WC mmapings as
PAGE_KERNEL_IO, so cargo-culted it in.
 
> >
> >  	if (pages != stack_pages)
> >  		drm_free_large(pages);
> >@@ -2684,27 +2687,55 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
> >  }
> >
> >  /* get, pin, and map the pages of the object into kernel space */
> >-void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj)
> >+void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, bool use_wc)
> >  {
> >+	void *ptr;
> >+	bool has_wc;
> >+	bool pinned;
> >  	int ret;
> >
> >  	lockdep_assert_held(&obj->base.dev->struct_mutex);
> >+	GEM_BUG_ON((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0);
> >
> >  	ret = i915_gem_object_get_pages(obj);
> >  	if (ret)
> >  		return ERR_PTR(ret);
> >
> >+	GEM_BUG_ON(obj->pages == NULL);
> 
> Looks like belongs to i915_gem_object_get_pages and not to callers.

This is from later where it did:

        pinned = true;
        if (!atomic_inc_not_zero(&obj->mm.pages_pin_count)) {
                ret = ____i915_gem_object_get_pages(obj);
                if (ret)
                        goto err_unlock;

                smp_mb__before_atomic();
                GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count));
                atomic_set(&obj->mm.pages_pin_count, 1);
                pinned = false;
        }

        GEM_BUG_ON(obj->mm.pages == NULL);

Right now the BUG_ON is superfluous.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre