Re: [PATCH 6/6] drm/i915: Create VMAs

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, Jul 18, 2013 at 01:12:17AM +0100, Chris Wilson wrote:
> On Wed, Jul 17, 2013 at 12:19:03PM -0700, Ben Widawsky wrote:
> > Formerly: "drm/i915: Create VMAs (part 1)"
> > 
> > In a previous patch, the notion of a VM was introduced. A VMA describes
> > an area of part of the VM address space. A VMA is similar to the concept
> > in the linux mm. However, instead of representing regular memory, a VMA
> > is backed by a GEM BO. There may be many VMAs for a given object, one
> > for each VM the object is to be used in. This may occur through flink,
> > dma-buf, or a number of other transient states.
> > 
> > Currently the code depends on only 1 VMA per object, for the global GTT
> > (and aliasing PPGTT). The following patches will address this and make
> > the rest of the infrastructure more suited
> > 
> > v2: s/i915_obj/i915_gem_obj (Chris)
> > 
> > v3: Only move an object to the now global unbound list if there are no
> > more VMAs for the object which are bound into a VM (ie. the list is
> > empty).
> > 
> > v4: killed obj->gtt_space
> > some reworks due to rebase
> > 
> > v5: Free vma on error path (Imre)
> > 
> > v6: Another missed vma free in i915_gem_object_bind_to_gtt error path
> > (Imre)
> > Fixed vma freeing in stolen preallocation (Imre)
> 
> Big-bada-boom;

Just from looking at the code I think I see a bug. A bug which didn't
exist in the original version of the code, and doesn't exist after the
very next patch in the overall series.

Now I am terribly curious - why in the world (if that's indeed the bug)
can I not seem to hit this locally on my machine? I'll send the patch
for the fix now, but I'd really like to know what's different in our
setup. I've tried, UXA, SNA, and the igt test suite...

> set-cache-level needs to iterate over vma, and in
> particular should not dereference a non-existent one. Or if we decided
> that set-cache-level was a ggtt only property, just not explode if there
> is no global vma.
> -Chris

The current state is that cache level is still per object, but this
function itself will iterate over all VMAs. As I said, it happens in the
very next patch in the series. In the original patch series cover
letter, I made cache levels per VMA an optional TODO. For the time
being, I don't see much benefit.

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center