On Mon, Sep 10, 2012 at 05:34:48PM +0100, Chris Wilson wrote:
> On Thu, 6 Sep 2012 18:49:24 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> > On Tue, 4 Sep 2012 21:02:57 +0100
> > Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >
> > > Rather than have multiple data structures for describing our page layout
> > > in conjunction with the array of pages, we can migrate all users over to
> > > a scatterlist.
> > >
> > > One major advantage, other than unifying the page tracking structures,
> > > this offers is that we replace the vmalloc'ed array (which can be up to
> > > a megabyte in size) with a chain of individual pages which helps reduce
> > > memory pressure.
> > >
> > > The disadvantage is that we then do not have a simple array to iterate,
> > > or to access randomly. The common case for this is in the relocation
> > > processing, which will typically fit within a single scatterlist page
> > > and so be almost the same cost as the simple array. For iterating over
> > > the array, the extra function call could be optimised away, but in
> > > reality is an insignificant cost of either binding the pages, or
> > > performing the pwrite/pread.
> > >
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Ok, I agree with Chris' comments here and slurped in patches 1-5.
Thanks, Daniel
> >
> >
> > Now that my eyes are done bleeding, easy ones:
> >
> > ERROR: space required after that ',' (ctx:VxV)
> > #69: FILE: drivers/char/agp/intel-gtt.c:99:
> > + for_each_sg(st->sgl, sg, num_entries,i)
> > ^
> >
> > WARNING: Prefer pr_err(... to printk(KERN_ERR, ...
> > #189: FILE: drivers/gpu/drm/drm_cache.c:117:
> > + printk(KERN_ERR "Timed out waiting for cache
> > flush.\n");
> >
> > WARNING: Prefer pr_err(... to printk(KERN_ERR, ...
> > #191: FILE: drivers/gpu/drm/drm_cache.c:119:
> > + printk(KERN_ERR "Architecture has no drm_cache.c support\n");
>
> Hmm, the drm_cache one is tricky as it is a continuation of the style of
> the file and so is probably best kept and then the whole file fixed to
> follow the new conventions.
>
> > In addition to the inline comments, it would have been even slightly
> > easier to review without the s/page/i since it seems to just be for no
> > compelling reason anyway.
>
> It was motivated by using the common idiom for for_each_sg() and by
> allowing 'struct page *page' as being the natural local variable within
> the loop. So I think the end result justifies the small amount of extra
> churn in the patch.
>
> > > if (intel_private.base.needs_dmar) {
> > > - ret = intel_gtt_map_memory(mem->pages, mem->page_count,
> > > - &mem->sg_list, &mem->num_sg);
> > > + struct sg_table st;
> > > +
> > > + ret = intel_gtt_map_memory(mem->pages, mem->page_count, &st);
> > > if (ret != 0)
> > > return ret;
> > >
> > > - intel_gtt_insert_sg_entries(mem->sg_list, mem->num_sg,
> > > - pg_start, type);
> > > + intel_gtt_insert_sg_entries(&st, pg_start, type);
> > > + mem->sg_list = st.sgl;
> > > + mem->num_sg = st.nents;
> >
> > Can you explain how the corresponding free for the sg_table gets called
> > here?
>
> The sg_table is just a small placeholder that is reconstructed in
> intel_gtt_unmap_memory() for sg_free_table().
>
> > > @@ -1749,20 +1771,27 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> > > BUG_ON(obj->base.read_domains & I915_GEM_GPU_DOMAINS);
> > > BUG_ON(obj->base.write_domain & I915_GEM_GPU_DOMAINS);
> > >
> > > - /* Get the list of pages out of our struct file. They'll be pinned
> > > - * at this point until we release them.
> > > - */
> > > + st = kmalloc(sizeof(*st), GFP_KERNEL);
> > > + if (st == NULL)
> > > + return -ENOMEM;
> > > +
> > > page_count = obj->base.size / PAGE_SIZE;
> > > - obj->pages = drm_malloc_ab(page_count, sizeof(struct page *));
> > > - if (obj->pages == NULL)
> > > + if (sg_alloc_table(st, page_count, GFP_KERNEL)) {
> > > + sg_free_table(st);
> > > + kfree(st);
> > > return -ENOMEM;
> > > + }
> >
> > I think the call here to sg_free_table is bogus.
>
> Experience says otherwise ;-)
>
> The reason is that the sg_alloc_table chains together its individual
> page allocations but doesn't perform any unwind if one fails before
> reporting the error. sg_free_table() does the right thing in those
> circumstances.
>
> > > - /* link the pages into an SG then map the sg */
> > > - sg = drm_prime_pages_to_sg(obj->pages, npages);
> > > - nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
> > > i915_gem_object_pin_pages(obj);
> >
> > <bikeshed>
> > I think the right way to go about this is to add rm_prime_pages_to_st
> > since you're pushing the whole st>sg thing, other drivers can leverage
> > it.
> > </bikeshed>
>
> Quite possibly true, but the code will change later and lose some of its
> generality. Or at least no else is like i915 yet.
>
> > The lifetime description we discussed on IRC would have helped here as
> > well.
>
> > > static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
> > > @@ -80,7 +104,9 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> > > {
> > > struct drm_i915_gem_object *obj = dma_buf->priv;
> > > struct drm_device *dev = obj->base.dev;
> > > - int ret;
> > > + struct scatterlist *sg;
> > > + struct page **pages;
> > > + int ret, i;
> > >
> > > ret = i915_mutex_lock_interruptible(dev);
> > > if (ret)
> > > @@ -92,22 +118,33 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> > > }
> > >
> > > ret = i915_gem_object_get_pages(obj);
> > > - if (ret) {
> > > - mutex_unlock(&dev->struct_mutex);
> > > - return ERR_PTR(ret);
> > > - }
> > > + if (ret)
> > > + goto error;
> > >
> > > - obj->dma_buf_vmapping = vmap(obj->pages, obj->base.size / PAGE_SIZE, 0, PAGE_KERNEL);
> > > - if (!obj->dma_buf_vmapping) {
> > > - DRM_ERROR("failed to vmap object\n");
> > > - goto out_unlock;
> > > - }
> > > + ret = -ENOMEM;
> > > +
> > > + pages = drm_malloc_ab(obj->pages->nents, sizeof(struct page *));
> > > + if (pages == NULL)
> > > + goto error;
> > > +
> > > + for_each_sg(obj->pages->sgl, sg, obj->pages->nents, i)
> > > + pages[i] = sg_page(sg);
> > > +
> > > + obj->dma_buf_vmapping = vmap(pages, obj->pages->nents, 0, PAGE_KERNEL);
> > > + drm_free_large(pages);
> > > +
> > > + if (!obj->dma_buf_vmapping)
> > > + goto error;
> > >
> > > obj->vmapping_count = 1;
> > > i915_gem_object_pin_pages(obj);
> > > out_unlock:
> > > mutex_unlock(&dev->struct_mutex);
> > > return obj->dma_buf_vmapping;
> > > +
> > > +error:
> > > + mutex_unlock(&dev->struct_mutex);
> > > + return ERR_PTR(ret);
> > > }
> > >
> > > static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
> >
> > The return on vmap failing looks incorrect to me here. Also, I think
> > leaving the DRM_ERROR would have been nice.
>
> Since we already return the ERR_PTR(-ENOMEM) we are not breaking any
> semantics by reporting the oom for vmap as well. And yes it would be
> nice if vmap gave a specific error as well. So other than the change to
> an explicit errno, I'm not sure what mistake you are point out.
>
> In this case the DRM_ERROR has an obvious errno returned to userspace,
> much more informative.
> >
> > > @@ -270,26 +233,10 @@ void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
> > > BUG();
> > > }
> > >
> > > - if (obj->sg_table) {
> > > - i915_ppgtt_insert_sg_entries(ppgtt,
> > > - obj->sg_table->sgl,
> > > - obj->sg_table->nents,
> > > - obj->gtt_space->start >> PAGE_SHIFT,
> > > - pte_flags);
> > > - } else if (dev_priv->mm.gtt->needs_dmar) {
> > > - BUG_ON(!obj->sg_list);
> > > -
> > > - i915_ppgtt_insert_sg_entries(ppgtt,
> > > - obj->sg_list,
> > > - obj->num_sg,
> > > - obj->gtt_space->start >> PAGE_SHIFT,
> > > - pte_flags);
> > > - } else
> > > - i915_ppgtt_insert_pages(ppgtt,
> > > - obj->gtt_space->start >> PAGE_SHIFT,
> > > - obj->base.size >> PAGE_SHIFT,
> > > - obj->pages,
> > > - pte_flags);
> > > + i915_ppgtt_insert_sg_entries(ppgtt,
> > > + obj->sg_table ?: obj->pages,
> > > + obj->gtt_space->start >> PAGE_SHIFT,
> > > + pte_flags);
> > > }
> >
> > I got lost here. Is it, if there is a prime sg_table use that, otherwise
> > just use the object's sgt? If so, I think has_dma_mapping is more
> > readable.
> > Also, I wonder if ?: pissed off the clang people?
>
> Right, this is just a step along the path to enlightment. 2 out of the 3
> paths now use obj->pages with the dmabuf being the only exception to
> still create an obj->sg_table scatterlist. '?:' is widely used by the
> kernel, if clang doesn't yet support it, that's their problem. But rest
> assured it is removed in a couple of patches after migrating dmabuf over
> to the page ops.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> 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
