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>
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");
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.
> ---
> drivers/char/agp/intel-gtt.c | 51 +++++-------
> drivers/gpu/drm/drm_cache.c | 23 ++++++
> drivers/gpu/drm/i915/i915_drv.h | 18 +++--
> drivers/gpu/drm/i915/i915_gem.c | 79 ++++++++++++------
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 99 +++++++++++++++--------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c | 121 ++++++----------------------
> drivers/gpu/drm/i915/i915_gem_tiling.c | 16 ++--
> drivers/gpu/drm/i915/i915_irq.c | 25 +++---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 9 ++-
> include/drm/drmP.h | 1 +
> include/drm/intel-gtt.h | 10 +--
> 12 files changed, 236 insertions(+), 219 deletions(-)
>
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 58e32f7..511d1b1 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -84,40 +84,33 @@ static struct _intel_private {
> #define IS_IRONLAKE intel_private.driver->is_ironlake
> #define HAS_PGTBL_EN intel_private.driver->has_pgtbl_enable
>
> -int intel_gtt_map_memory(struct page **pages, unsigned int num_entries,
> - struct scatterlist **sg_list, int *num_sg)
> +static int intel_gtt_map_memory(struct page **pages,
> + unsigned int num_entries,
> + struct sg_table *st)
> {
> - struct sg_table st;
> struct scatterlist *sg;
> int i;
>
> - if (*sg_list)
> - return 0; /* already mapped (for e.g. resume */
> -
> DBG("try mapping %lu pages\n", (unsigned long)num_entries);
>
> - if (sg_alloc_table(&st, num_entries, GFP_KERNEL))
> + if (sg_alloc_table(st, num_entries, GFP_KERNEL))
> goto err;
>
> - *sg_list = sg = st.sgl;
> -
> - for (i = 0 ; i < num_entries; i++, sg = sg_next(sg))
> + for_each_sg(st->sgl, sg, num_entries,i)
> sg_set_page(sg, pages[i], PAGE_SIZE, 0);
>
> - *num_sg = pci_map_sg(intel_private.pcidev, *sg_list,
> - num_entries, PCI_DMA_BIDIRECTIONAL);
> - if (unlikely(!*num_sg))
> + if (!pci_map_sg(intel_private.pcidev,
> + st->sgl, st->nents, PCI_DMA_BIDIRECTIONAL))
> goto err;
>
> return 0;
>
> err:
> - sg_free_table(&st);
> + sg_free_table(st);
> return -ENOMEM;
> }
> -EXPORT_SYMBOL(intel_gtt_map_memory);
>
> -void intel_gtt_unmap_memory(struct scatterlist *sg_list, int num_sg)
> +static void intel_gtt_unmap_memory(struct scatterlist *sg_list, int num_sg)
> {
> struct sg_table st;
> DBG("try unmapping %lu pages\n", (unsigned long)mem->page_count);
> @@ -130,7 +123,6 @@ void intel_gtt_unmap_memory(struct scatterlist *sg_list, int num_sg)
>
> sg_free_table(&st);
> }
> -EXPORT_SYMBOL(intel_gtt_unmap_memory);
>
> static void intel_fake_agp_enable(struct agp_bridge_data *bridge, u32 mode)
> {
> @@ -879,8 +871,7 @@ static bool i830_check_flags(unsigned int flags)
> return false;
> }
>
> -void intel_gtt_insert_sg_entries(struct scatterlist *sg_list,
> - unsigned int sg_len,
> +void intel_gtt_insert_sg_entries(struct sg_table *st,
> unsigned int pg_start,
> unsigned int flags)
> {
> @@ -892,12 +883,11 @@ void intel_gtt_insert_sg_entries(struct scatterlist *sg_list,
>
> /* sg may merge pages, but we have to separate
> * per-page addr for GTT */
> - for_each_sg(sg_list, sg, sg_len, i) {
> + for_each_sg(st->sgl, sg, st->nents, i) {
> len = sg_dma_len(sg) >> PAGE_SHIFT;
> for (m = 0; m < len; m++) {
> dma_addr_t addr = sg_dma_address(sg) + (m << PAGE_SHIFT);
> - intel_private.driver->write_entry(addr,
> - j, flags);
> + intel_private.driver->write_entry(addr, j, flags);
> j++;
> }
> }
> @@ -905,8 +895,10 @@ void intel_gtt_insert_sg_entries(struct scatterlist *sg_list,
> }
> EXPORT_SYMBOL(intel_gtt_insert_sg_entries);
>
> -void intel_gtt_insert_pages(unsigned int first_entry, unsigned int num_entries,
> - struct page **pages, unsigned int flags)
> +static void intel_gtt_insert_pages(unsigned int first_entry,
> + unsigned int num_entries,
> + struct page **pages,
> + unsigned int flags)
> {
> int i, j;
>
> @@ -917,7 +909,6 @@ void intel_gtt_insert_pages(unsigned int first_entry, unsigned int num_entries,
> }
> readl(intel_private.gtt+j-1);
> }
> -EXPORT_SYMBOL(intel_gtt_insert_pages);
>
> static int intel_fake_agp_insert_entries(struct agp_memory *mem,
> off_t pg_start, int type)
> @@ -953,13 +944,15 @@ static int intel_fake_agp_insert_entries(struct agp_memory *mem,
> global_cache_flush();
>
> 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?
> } else
> intel_gtt_insert_pages(pg_start, mem->page_count, mem->pages,
> type);
> diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
> index 08758e0..628a2e0 100644
> --- a/drivers/gpu/drm/drm_cache.c
> +++ b/drivers/gpu/drm/drm_cache.c
> @@ -100,6 +100,29 @@ drm_clflush_pages(struct page *pages[], unsigned long num_pages)
> EXPORT_SYMBOL(drm_clflush_pages);
>
> void
> +drm_clflush_sg(struct sg_table *st)
> +{
> +#if defined(CONFIG_X86)
> + if (cpu_has_clflush) {
> + struct scatterlist *sg;
> + int i;
> +
> + mb();
> + for_each_sg(st->sgl, sg, st->nents, i)
> + drm_clflush_page(sg_page(sg));
> + mb();
> + }
> +
> + if (on_each_cpu(drm_clflush_ipi_handler, NULL, 1) != 0)
> + printk(KERN_ERR "Timed out waiting for cache flush.\n");
> +#else
> + printk(KERN_ERR "Architecture has no drm_cache.c support\n");
> + WARN_ON_ONCE(1);
> +#endif
> +}
> +EXPORT_SYMBOL(drm_clflush_sg);
> +
> +void
> drm_clflush_virt_range(char *addr, unsigned long length)
> {
> #if defined(CONFIG_X86)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0747472..1a714fa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -992,16 +992,11 @@ struct drm_i915_gem_object {
>
> unsigned int has_aliasing_ppgtt_mapping:1;
> unsigned int has_global_gtt_mapping:1;
> + unsigned int has_dma_mapping:1;
>
> - struct page **pages;
> + struct sg_table *pages;
> int pages_pin_count;
>
> - /**
> - * DMAR support
> - */
> - struct scatterlist *sg_list;
> - int num_sg;
> -
> /* prime dma-buf support */
> struct sg_table *sg_table;
> void *dma_buf_vmapping;
> @@ -1328,6 +1323,15 @@ void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
> void i915_gem_lastclose(struct drm_device *dev);
>
> int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
> +static inline struct page *i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
> +{
> + struct scatterlist *sg = obj->pages->sgl;
> + while (n >= SG_MAX_SINGLE_ALLOC) {
> + sg = sg_chain_ptr(sg + SG_MAX_SINGLE_ALLOC - 1);
> + n -= SG_MAX_SINGLE_ALLOC - 1;
> + }
> + return sg_page(sg+n);
> +}
> static inline void i915_gem_object_pin_pages(struct drm_i915_gem_object *obj)
> {
> BUG_ON(obj->pages == NULL);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 171bc51..06589a9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -411,6 +411,8 @@ i915_gem_shmem_pread(struct drm_device *dev,
> int hit_slowpath = 0;
> int prefaulted = 0;
> int needs_clflush = 0;
> + struct scatterlist *sg;
> + int i;
>
> user_data = (char __user *) (uintptr_t) args->data_ptr;
> remain = args->size;
> @@ -439,9 +441,15 @@ i915_gem_shmem_pread(struct drm_device *dev,
>
> offset = args->offset;
>
> - while (remain > 0) {
> + for_each_sg(obj->pages->sgl, sg, obj->pages->nents, i) {
> struct page *page;
>
> + if (i < offset >> PAGE_SHIFT)
> + continue;
> +
> + if (remain <= 0)
> + break;
> +
> /* Operation in this page
> *
> * shmem_page_offset = offset within page in shmem file
> @@ -452,7 +460,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
> if ((shmem_page_offset + page_length) > PAGE_SIZE)
> page_length = PAGE_SIZE - shmem_page_offset;
>
> - page = obj->pages[offset >> PAGE_SHIFT];
> + page = sg_page(sg);
> page_do_bit17_swizzling = obj_do_bit17_swizzling &&
> (page_to_phys(page) & (1 << 17)) != 0;
>
> @@ -731,6 +739,8 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> int hit_slowpath = 0;
> int needs_clflush_after = 0;
> int needs_clflush_before = 0;
> + int i;
> + struct scatterlist *sg;
>
> user_data = (char __user *) (uintptr_t) args->data_ptr;
> remain = args->size;
> @@ -765,10 +775,16 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> offset = args->offset;
> obj->dirty = 1;
>
> - while (remain > 0) {
> + for_each_sg(obj->pages->sgl, sg, obj->pages->nents, i) {
> struct page *page;
> int partial_cacheline_write;
>
> + if (i < offset >> PAGE_SHIFT)
> + continue;
> +
> + if (remain <= 0)
> + break;
> +
> /* Operation in this page
> *
> * shmem_page_offset = offset within page in shmem file
> @@ -787,7 +803,7 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> ((shmem_page_offset | page_length)
> & (boot_cpu_data.x86_clflush_size - 1));
>
> - page = obj->pages[offset >> PAGE_SHIFT];
> + page = sg_page(sg);
> page_do_bit17_swizzling = obj_do_bit17_swizzling &&
> (page_to_phys(page) & (1 << 17)) != 0;
>
> @@ -1633,6 +1649,7 @@ static void
> i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
> {
> int page_count = obj->base.size / PAGE_SIZE;
> + struct scatterlist *sg;
> int ret, i;
>
> BUG_ON(obj->madv == __I915_MADV_PURGED);
> @@ -1653,19 +1670,21 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
> if (obj->madv == I915_MADV_DONTNEED)
> obj->dirty = 0;
>
> - for (i = 0; i < page_count; i++) {
> + for_each_sg(obj->pages->sgl, sg, page_count, i) {
> + struct page *page = sg_page(sg);
> +
> if (obj->dirty)
> - set_page_dirty(obj->pages[i]);
> + set_page_dirty(page);
>
> if (obj->madv == I915_MADV_WILLNEED)
> - mark_page_accessed(obj->pages[i]);
> + mark_page_accessed(page);
>
> - page_cache_release(obj->pages[i]);
> + page_cache_release(page);
> }
> obj->dirty = 0;
>
> - drm_free_large(obj->pages);
> - obj->pages = NULL;
> + sg_free_table(obj->pages);
> + kfree(obj->pages);
> }
>
> static int
> @@ -1682,6 +1701,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
> return -EBUSY;
>
> ops->put_pages(obj);
> + obj->pages = NULL;
>
> list_del(&obj->gtt_list);
> if (i915_gem_object_is_purgeable(obj))
> @@ -1739,6 +1759,8 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> int page_count, i;
> struct address_space *mapping;
> + struct sg_table *st;
> + struct scatterlist *sg;
> struct page *page;
> gfp_t gfp;
>
> @@ -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.
>
> - /* Fail silently without starting the shrinker */
> + /* Get the list of pages out of our struct file. They'll be pinned
> + * at this point until we release them.
> + *
> + * Fail silently without starting the shrinker
> + */
> mapping = obj->base.filp->f_path.dentry->d_inode->i_mapping;
> gfp = mapping_gfp_mask(mapping);
> gfp |= __GFP_NORETRY | __GFP_NOWARN;
> gfp &= ~(__GFP_IO | __GFP_WAIT);
> - for (i = 0; i < page_count; i++) {
> + for_each_sg(st->sgl, sg, page_count, i) {
> page = shmem_read_mapping_page_gfp(mapping, i, gfp);
> if (IS_ERR(page)) {
> i915_gem_purge(dev_priv, page_count);
> @@ -1785,20 +1814,20 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
> gfp &= ~(__GFP_IO | __GFP_WAIT);
> }
>
> - obj->pages[i] = page;
> + sg_set_page(sg, page, PAGE_SIZE, 0);
> }
>
> if (i915_gem_object_needs_bit17_swizzle(obj))
> i915_gem_object_do_bit_17_swizzle(obj);
>
> + obj->pages = st;
> return 0;
>
> err_pages:
> - while (i--)
> - page_cache_release(obj->pages[i]);
> -
> - drm_free_large(obj->pages);
> - obj->pages = NULL;
> + for_each_sg(st->sgl, sg, i, page_count)
> + page_cache_release(sg_page(sg));
> + sg_free_table(st);
> + kfree(st);
> return PTR_ERR(page);
> }
>
> @@ -2974,7 +3003,7 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj)
>
> trace_i915_gem_object_clflush(obj);
>
> - drm_clflush_pages(obj->pages, obj->base.size / PAGE_SIZE);
> + drm_clflush_sg(obj->pages);
> }
>
> /** Flushes the GTT write domain for the object if it's dirty. */
> @@ -3724,6 +3753,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> i915_gem_object_put_pages(obj);
> i915_gem_object_free_mmap_offset(obj);
>
> + BUG_ON(obj->pages);
> +
> drm_gem_object_release(&obj->base);
> i915_gem_info_remove_obj(dev_priv, obj->base.size);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index eca4726..4bb1b94 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -28,33 +28,57 @@
> #include <linux/dma-buf.h>
>
> static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachment,
> - enum dma_data_direction dir)
> + enum dma_data_direction dir)
> {
> struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
> - struct drm_device *dev = obj->base.dev;
> - int npages = obj->base.size / PAGE_SIZE;
> - struct sg_table *sg;
> - int ret;
> - int nents;
> + struct sg_table *st;
> + struct scatterlist *src, *dst;
> + int ret, i;
>
> - ret = i915_mutex_lock_interruptible(dev);
> + ret = i915_mutex_lock_interruptible(obj->base.dev);
> if (ret)
> return ERR_PTR(ret);
>
> ret = i915_gem_object_get_pages(obj);
> if (ret) {
> - sg = ERR_PTR(ret);
> + st = ERR_PTR(ret);
> + goto out;
> + }
> +
> + /* Copy sg so that we make an independent mapping */
> + st = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
> + if (st == NULL) {
> + st = ERR_PTR(-ENOMEM);
> + goto out;
> + }
> +
> + ret = sg_alloc_table(st, obj->pages->nents, GFP_KERNEL);
> + if (ret) {
> + kfree(st);
> + st = ERR_PTR(ret);
> + goto out;
> + }
> +
> + src = obj->pages->sgl;
> + dst = st->sgl;
> + for (i = 0; i < obj->pages->nents; i++) {
> + sg_set_page(dst, sg_page(src), PAGE_SIZE, 0);
> + dst = sg_next(dst);
> + src = sg_next(src);
> + }
> +
> + if (!dma_map_sg(attachment->dev, st->sgl, st->nents, dir)) {
> + sg_free_table(st);
> + kfree(st);
> + st = ERR_PTR(-ENOMEM);
> goto out;
> }
>
> - /* 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>
The lifetime description we discussed on IRC would have helped here as
well.
>
> out:
> - mutex_unlock(&dev->struct_mutex);
> - return sg;
> + mutex_unlock(&obj->base.dev->struct_mutex);
> + return st;
> }
>
> 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.
> @@ -184,22 +221,19 @@ static const struct dma_buf_ops i915_dmabuf_ops = {
> };
>
> struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
> - struct drm_gem_object *gem_obj, int flags)
> + struct drm_gem_object *gem_obj, int flags)
> {
> struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
>
> - return dma_buf_export(obj, &i915_dmabuf_ops,
> - obj->base.size, 0600);
> + return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, 0600);
> }
>
> struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
> - struct dma_buf *dma_buf)
> + struct dma_buf *dma_buf)
> {
> struct dma_buf_attachment *attach;
> struct sg_table *sg;
> struct drm_i915_gem_object *obj;
> - int npages;
> - int size;
> int ret;
>
> /* is this one of own objects? */
> @@ -223,21 +257,19 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
> goto fail_detach;
> }
>
> - size = dma_buf->size;
> - npages = size / PAGE_SIZE;
> -
> obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> if (obj == NULL) {
> ret = -ENOMEM;
> goto fail_unmap;
> }
>
> - ret = drm_gem_private_object_init(dev, &obj->base, size);
> + ret = drm_gem_private_object_init(dev, &obj->base, dma_buf->size);
> if (ret) {
> kfree(obj);
> goto fail_unmap;
> }
>
> + obj->has_dma_mapping = true;
> obj->sg_table = sg;
> obj->base.import_attach = attach;
>
> @@ -249,3 +281,4 @@ fail_detach:
> dma_buf_detach(dma_buf, attach);
> return ERR_PTR(ret);
> }
> +
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index e6b2205..4ab0083 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -210,7 +210,8 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
> if (ret)
> return ret;
>
> - vaddr = kmap_atomic(obj->pages[reloc->offset >> PAGE_SHIFT]);
> + vaddr = kmap_atomic(i915_gem_object_get_page(obj,
> + reloc->offset >> PAGE_SHIFT));
> *(uint32_t *)(vaddr + page_offset) = reloc->delta;
> kunmap_atomic(vaddr);
> } else {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 1847731..6746109 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -167,8 +167,7 @@ void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev)
> }
>
> static void i915_ppgtt_insert_sg_entries(struct i915_hw_ppgtt *ppgtt,
> - struct scatterlist *sg_list,
> - unsigned sg_len,
> + const struct sg_table *pages,
> unsigned first_entry,
> uint32_t pte_flags)
> {
> @@ -180,12 +179,12 @@ static void i915_ppgtt_insert_sg_entries(struct i915_hw_ppgtt *ppgtt,
> struct scatterlist *sg;
>
> /* init sg walking */
> - sg = sg_list;
> + sg = pages->sgl;
> i = 0;
> segment_len = sg_dma_len(sg) >> PAGE_SHIFT;
> m = 0;
>
> - while (i < sg_len) {
> + while (i < pages->nents) {
> pt_vaddr = kmap_atomic(ppgtt->pt_pages[act_pd]);
>
> for (j = first_pte; j < I915_PPGTT_PT_ENTRIES; j++) {
> @@ -194,13 +193,11 @@ static void i915_ppgtt_insert_sg_entries(struct i915_hw_ppgtt *ppgtt,
> pt_vaddr[j] = pte | pte_flags;
>
> /* grab the next page */
> - m++;
> - if (m == segment_len) {
> - sg = sg_next(sg);
> - i++;
> - if (i == sg_len)
> + if (++m == segment_len) {
> + if (++i == pages->nents)
> break;
>
> + sg = sg_next(sg);
> segment_len = sg_dma_len(sg) >> PAGE_SHIFT;
> m = 0;
> }
> @@ -213,44 +210,10 @@ static void i915_ppgtt_insert_sg_entries(struct i915_hw_ppgtt *ppgtt,
> }
> }
>
> -static void i915_ppgtt_insert_pages(struct i915_hw_ppgtt *ppgtt,
> - unsigned first_entry, unsigned num_entries,
> - struct page **pages, uint32_t pte_flags)
> -{
> - uint32_t *pt_vaddr, pte;
> - unsigned act_pd = first_entry / I915_PPGTT_PT_ENTRIES;
> - unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
> - unsigned last_pte, i;
> - dma_addr_t page_addr;
> -
> - while (num_entries) {
> - last_pte = first_pte + num_entries;
> - last_pte = min_t(unsigned, last_pte, I915_PPGTT_PT_ENTRIES);
> -
> - pt_vaddr = kmap_atomic(ppgtt->pt_pages[act_pd]);
> -
> - for (i = first_pte; i < last_pte; i++) {
> - page_addr = page_to_phys(*pages);
> - pte = GEN6_PTE_ADDR_ENCODE(page_addr);
> - pt_vaddr[i] = pte | pte_flags;
> -
> - pages++;
> - }
> -
> - kunmap_atomic(pt_vaddr);
> -
> - num_entries -= last_pte - first_pte;
> - first_pte = 0;
> - act_pd++;
> - }
> -}
> -
> void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
> struct drm_i915_gem_object *obj,
> enum i915_cache_level cache_level)
> {
> - struct drm_device *dev = obj->base.dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> uint32_t pte_flags = GEN6_PTE_VALID;
>
> switch (cache_level) {
Methinks this isn't what you wanted to do.
> @@ -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?
>
> void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt,
> @@ -361,44 +308,26 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
>
> int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
> {
> - struct drm_device *dev = obj->base.dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - /* don't map imported dma buf objects */
> - if (dev_priv->mm.gtt->needs_dmar && !obj->sg_table)
> - return intel_gtt_map_memory(obj->pages,
> - obj->base.size >> PAGE_SHIFT,
> - &obj->sg_list,
> - &obj->num_sg);
> - else
> + if (obj->has_dma_mapping)
> return 0;
> +
> + if (!dma_map_sg(&obj->base.dev->pdev->dev,
> + obj->pages->sgl, obj->pages->nents,
> + PCI_DMA_BIDIRECTIONAL))
> + return -ENOSPC;
> +
> + return 0;
> }
>
> void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj,
> enum i915_cache_level cache_level)
> {
> struct drm_device *dev = obj->base.dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> unsigned int agp_type = cache_level_to_agp_type(dev, cache_level);
>
> - if (obj->sg_table) {
> - intel_gtt_insert_sg_entries(obj->sg_table->sgl,
> - obj->sg_table->nents,
> - obj->gtt_space->start >> PAGE_SHIFT,
> - agp_type);
> - } else if (dev_priv->mm.gtt->needs_dmar) {
> - BUG_ON(!obj->sg_list);
> -
> - intel_gtt_insert_sg_entries(obj->sg_list,
> - obj->num_sg,
> - obj->gtt_space->start >> PAGE_SHIFT,
> - agp_type);
> - } else
> - intel_gtt_insert_pages(obj->gtt_space->start >> PAGE_SHIFT,
> - obj->base.size >> PAGE_SHIFT,
> - obj->pages,
> - agp_type);
> -
> + intel_gtt_insert_sg_entries(obj->sg_table ?: obj->pages,
> + obj->gtt_space->start >> PAGE_SHIFT,
> + agp_type);
> obj->has_global_gtt_mapping = 1;
> }
>
> @@ -418,10 +347,10 @@ void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
>
> interruptible = do_idling(dev_priv);
>
> - if (obj->sg_list) {
> - intel_gtt_unmap_memory(obj->sg_list, obj->num_sg);
> - obj->sg_list = NULL;
> - }
> + if (!obj->has_dma_mapping)
> + dma_unmap_sg(&dev->pdev->dev,
> + obj->pages->sgl, obj->pages->nents,
> + PCI_DMA_BIDIRECTIONAL);
>
> undo_idling(dev_priv, interruptible);
> }
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index b964df5..8093ecd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -470,18 +470,20 @@ i915_gem_swizzle_page(struct page *page)
> void
> i915_gem_object_do_bit_17_swizzle(struct drm_i915_gem_object *obj)
> {
> + struct scatterlist *sg;
> int page_count = obj->base.size >> PAGE_SHIFT;
> int i;
>
> if (obj->bit_17 == NULL)
> return;
>
> - for (i = 0; i < page_count; i++) {
> - char new_bit_17 = page_to_phys(obj->pages[i]) >> 17;
> + for_each_sg(obj->pages->sgl, sg, page_count, i) {
> + struct page *page = sg_page(sg);
> + char new_bit_17 = page_to_phys(page) >> 17;
> if ((new_bit_17 & 0x1) !=
> (test_bit(i, obj->bit_17) != 0)) {
> - i915_gem_swizzle_page(obj->pages[i]);
> - set_page_dirty(obj->pages[i]);
> + i915_gem_swizzle_page(page);
> + set_page_dirty(page);
> }
> }
> }
> @@ -489,6 +491,7 @@ i915_gem_object_do_bit_17_swizzle(struct drm_i915_gem_object *obj)
> void
> i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj)
> {
> + struct scatterlist *sg;
> int page_count = obj->base.size >> PAGE_SHIFT;
> int i;
>
> @@ -502,8 +505,9 @@ i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj)
> }
> }
>
> - for (i = 0; i < page_count; i++) {
> - if (page_to_phys(obj->pages[i]) & (1 << 17))
> + for_each_sg(obj->pages->sgl, sg, page_count, i) {
> + struct page *page = sg_page(sg);
> + if (page_to_phys(page) & (1 << 17))
> __set_bit(i, obj->bit_17);
> else
> __clear_bit(i, obj->bit_17);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d601013..dd49046 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -888,20 +888,20 @@ i915_error_object_create(struct drm_i915_private *dev_priv,
> struct drm_i915_gem_object *src)
> {
> struct drm_i915_error_object *dst;
> - int page, page_count;
> + int i, count;
> u32 reloc_offset;
>
> if (src == NULL || src->pages == NULL)
> return NULL;
>
> - page_count = src->base.size / PAGE_SIZE;
> + count = src->base.size / PAGE_SIZE;
>
> - dst = kmalloc(sizeof(*dst) + page_count * sizeof(u32 *), GFP_ATOMIC);
> + dst = kmalloc(sizeof(*dst) + count * sizeof(u32 *), GFP_ATOMIC);
> if (dst == NULL)
> return NULL;
>
> reloc_offset = src->gtt_offset;
> - for (page = 0; page < page_count; page++) {
> + for (i = 0; i < count; i++) {
> unsigned long flags;
> void *d;
>
> @@ -924,30 +924,33 @@ i915_error_object_create(struct drm_i915_private *dev_priv,
> memcpy_fromio(d, s, PAGE_SIZE);
> io_mapping_unmap_atomic(s);
> } else {
> + struct page *page;
> void *s;
>
> - drm_clflush_pages(&src->pages[page], 1);
> + page = i915_gem_object_get_page(src, i);
> +
> + drm_clflush_pages(&page, 1);
>
> - s = kmap_atomic(src->pages[page]);
> + s = kmap_atomic(page);
> memcpy(d, s, PAGE_SIZE);
> kunmap_atomic(s);
>
> - drm_clflush_pages(&src->pages[page], 1);
> + drm_clflush_pages(&page, 1);
> }
> local_irq_restore(flags);
>
> - dst->pages[page] = d;
> + dst->pages[i] = d;
>
> reloc_offset += PAGE_SIZE;
> }
> - dst->page_count = page_count;
> + dst->page_count = count;
> dst->gtt_offset = src->gtt_offset;
>
> return dst;
>
> unwind:
> - while (page--)
> - kfree(dst->pages[page]);
> + while (i--)
> + kfree(dst->pages[i]);
> kfree(dst);
> return NULL;
> }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 55cdb4d..984a0c5 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -464,7 +464,7 @@ init_pipe_control(struct intel_ring_buffer *ring)
> goto err_unref;
>
> pc->gtt_offset = obj->gtt_offset;
> - pc->cpu_page = kmap(obj->pages[0]);
> + pc->cpu_page = kmap(sg_page(obj->pages->sgl));
> if (pc->cpu_page == NULL)
> goto err_unpin;
>
> @@ -491,7 +491,8 @@ cleanup_pipe_control(struct intel_ring_buffer *ring)
> return;
>
> obj = pc->obj;
> - kunmap(obj->pages[0]);
> +
> + kunmap(sg_page(obj->pages->sgl));
> i915_gem_object_unpin(obj);
> drm_gem_object_unreference(&obj->base);
>
> @@ -1026,7 +1027,7 @@ static void cleanup_status_page(struct intel_ring_buffer *ring)
> if (obj == NULL)
> return;
>
> - kunmap(obj->pages[0]);
> + kunmap(sg_page(obj->pages->sgl));
> i915_gem_object_unpin(obj);
> drm_gem_object_unreference(&obj->base);
> ring->status_page.obj = NULL;
> @@ -1053,7 +1054,7 @@ static int init_status_page(struct intel_ring_buffer *ring)
> }
>
> ring->status_page.gfx_addr = obj->gtt_offset;
> - ring->status_page.page_addr = kmap(obj->pages[0]);
> + ring->status_page.page_addr = kmap(sg_page(obj->pages->sgl));
> if (ring->status_page.page_addr == NULL) {
> ret = -ENOMEM;
> goto err_unpin;
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d6b67bb..d5f0c16 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1367,6 +1367,7 @@ extern int drm_remove_magic(struct drm_master *master, drm_magic_t magic);
>
> /* Cache management (drm_cache.c) */
> void drm_clflush_pages(struct page *pages[], unsigned long num_pages);
> +void drm_clflush_sg(struct sg_table *st);
> void drm_clflush_virt_range(char *addr, unsigned long length);
>
> /* Locking IOCTL support (drm_lock.h) */
> diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
> index 8e29d55..2e37e9f 100644
> --- a/include/drm/intel-gtt.h
> +++ b/include/drm/intel-gtt.h
> @@ -30,16 +30,10 @@ void intel_gmch_remove(void);
> bool intel_enable_gtt(void);
>
> void intel_gtt_chipset_flush(void);
> -void intel_gtt_unmap_memory(struct scatterlist *sg_list, int num_sg);
> -void intel_gtt_clear_range(unsigned int first_entry, unsigned int num_entries);
> -int intel_gtt_map_memory(struct page **pages, unsigned int num_entries,
> - struct scatterlist **sg_list, int *num_sg);
> -void intel_gtt_insert_sg_entries(struct scatterlist *sg_list,
> - unsigned int sg_len,
> +void intel_gtt_insert_sg_entries(struct sg_table *st,
> unsigned int pg_start,
> unsigned int flags);
> -void intel_gtt_insert_pages(unsigned int first_entry, unsigned int num_entries,
> - struct page **pages, unsigned int flags);
> +void intel_gtt_clear_range(unsigned int first_entry, unsigned int num_entries);
>
> /* Special gtt memory types */
> #define AGP_DCACHE_MEMORY 1
--
Ben Widawsky, Intel Open Source Technology Center
