On Tue, Feb 27, 2018 at 12:49:57PM +0100, Christian König wrote:

Most of the time we only need the dma addresses.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/drm_prime.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index c38dacda6119..7856a9b3f8a8 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -922,40 +922,40 @@ EXPORT_SYMBOL(drm_prime_pages_to_sg); /**

• drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
• @sgt: scatter-gather table to convert

• • @pages: array of page pointers to store the page array in

• • @pages: optional array of page pointers to store the page array in
  • @addrs: optional array to store the dma bus address of each page

• • @max_pages: size of both the passed-in arrays

• • @max_entries: size of both the passed-in arrays
  • Exports an sg table into an array of pages and addresses. This is currently
  • required by the TTM driver in order to do correct fault handling.
  */

Can't we just teach ttm to use sgts wherever needed, and deprecate exporting dma-bufs to page arrays (which really breaks the abstraction entirely and was just a quick hack to get things going that stuck around for years). Last time I looked into ttm the only thing it did is convert it back to sgts again (after calling dma_map once more, which the exporter should have done already for you). -Daniel

int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,

• 		     dma_addr_t *addrs, int max_pages)
  

• 		     dma_addr_t *addrs, int max_entries)
  

{ unsigned count; struct scatterlist *sg; struct page *page;

• u32 len;
• int pg_index;

• u32 len, index; dma_addr_t addr;

• pg_index = 0;

• index = 0; for_each_sg(sgt->sgl, sg, sgt->nents, count) { len = sg->length; page = sg_page(sg); addr = sg_dma_address(sg);

  while (len > 0) {

• 	if (WARN_ON(pg_index >= max_pages))
  

• 	if (WARN_ON(index >= max_entries))
  	return -1;
  

• 	pages[pg_index] = page;
  

• 	if (pages)
  

• 		pages[index] = page;
  if (addrs)
  

• 		addrs[pg_index] = addr;
  

• 		addrs[index] = addr;
  
  page++;
  addr += PAGE_SIZE;
  len -= PAGE_SIZE;
  

• 	pg_index++;
  

• 	index++;
  

  } } return 0;

-- 2.14.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Mar 06, 2018 at 10:25:03AM +0100, Christian König wrote:

Am 06.03.2018 um 10:21 schrieb Daniel Vetter:

...

On Tue, Feb 27, 2018 at 12:49:57PM +0100, Christian König wrote:

...

Most of the time we only need the dma addresses.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/drm_prime.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index c38dacda6119..7856a9b3f8a8 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -922,40 +922,40 @@ EXPORT_SYMBOL(drm_prime_pages_to_sg); /**

• drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
• @sgt: scatter-gather table to convert

• • @pages: array of page pointers to store the page array in

• • @pages: optional array of page pointers to store the page array in
  • @addrs: optional array to store the dma bus address of each page

• • @max_pages: size of both the passed-in arrays

• • @max_entries: size of both the passed-in arrays
  • Exports an sg table into an array of pages and addresses. This is currently
  • required by the TTM driver in order to do correct fault handling.
  */

Can't we just teach ttm to use sgts wherever needed, and deprecate exporting dma-bufs to page arrays (which really breaks the abstraction entirely and was just a quick hack to get things going that stuck around for years). Last time I looked into ttm the only thing it did is convert it back to sgts again (after calling dma_map once more, which the exporter should have done already for you).

Thought about that as well, but the problem here isn't TTM.

We need to be able to access the SGT by an index in amdgpu to be able to build up the VM page tables and that is not possible because the SGT is potentially chained.

We could add a new sg_table access helper function to work around that thought.

There's some neat per-page sgt iter functions that we've build for i915. See i915_gem_gtt.c. But yeah that's probably a pile more work, but imo from the i915 code shuffling the end result looks fairly neat. -Daniel

BTW: TTM isn't mapping anything in that case, we just fill in the arrays from the SGT.

Christian.

...

-Daniel

...

int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,

• 		     dma_addr_t *addrs, int max_pages)
  

• 		     dma_addr_t *addrs, int max_entries)
  

  { unsigned count; struct scatterlist *sg; struct page *page;

• u32 len;
• int pg_index;

• u32 len, index; dma_addr_t addr;

• pg_index = 0;

• index = 0; for_each_sg(sgt->sgl, sg, sgt->nents, count) { len = sg->length; page = sg_page(sg); addr = sg_dma_address(sg); while (len > 0) {

• 	if (WARN_ON(pg_index >= max_pages))
  

• 	if (WARN_ON(index >= max_entries))
  	return -1;
  

• 	pages[pg_index] = page;
  

• 	if (pages)
  

• 		pages[index] = page;
  if (addrs)
  

• 		addrs[pg_index] = addr;
  

• 		addrs[index] = addr;
  page++;
  addr += PAGE_SIZE;
  len -= PAGE_SIZE;
  

• 	pg_index++;
  

• 	index++;
  

  } } return 0;

-- 2.14.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi Michel & Thomas,

any more comments on this? Or can I commit it?

Thanks, Christian.

Am 27.02.2018 um 12:49 schrieb Christian König:

Let's stop mangling everything in a single header and create one header per object instead.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/ttm/ttm_tt.c | 6 - include/drm/ttm/ttm_bo_driver.h | 237 +--------------------------------- include/drm/ttm/ttm_tt.h | 272 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 273 insertions(+), 242 deletions(-) create mode 100644 include/drm/ttm/ttm_tt.h

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 0ee3b8f11605..8e0b525cda00 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -31,17 +31,11 @@ #define pr_fmt(fmt) "[TTM] " fmt

#include <linux/sched.h> -#include <linux/highmem.h> #include <linux/pagemap.h> #include <linux/shmem_fs.h> #include <linux/file.h> -#include <linux/swap.h> -#include <linux/slab.h> -#include <linux/export.h> #include <drm/drm_cache.h> -#include <drm/ttm/ttm_module.h> #include <drm/ttm/ttm_bo_driver.h> -#include <drm/ttm/ttm_placement.h> #include <drm/ttm/ttm_page_alloc.h> #ifdef CONFIG_X86 #include <asm/set_memory.h> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 4312b5326f0b..f8e2515b401f 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -42,111 +42,10 @@ #include "ttm_memory.h" #include "ttm_module.h" #include "ttm_placement.h" +#include "ttm_tt.h"

#define TTM_MAX_BO_PRIORITY 4U

-struct ttm_backend_func {

• /**

• * struct ttm_backend_func member bind
  

• *
  

• * @ttm: Pointer to a struct ttm_tt.
  

• * @bo_mem: Pointer to a struct ttm_mem_reg describing the
  

• * memory type and location for binding.
  

• *
  

• * Bind the backend pages into the aperture in the location
  

• * indicated by @bo_mem. This function should be able to handle
  

• * differences between aperture and system page sizes.
  

• */
  

• int (*bind) (struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem);
• /**

• * struct ttm_backend_func member unbind
  

• *
  

• * @ttm: Pointer to a struct ttm_tt.
  

• *
  

• * Unbind previously bound backend pages. This function should be
  

• * able to handle differences between aperture and system page sizes.
  

• */
  

• int (*unbind) (struct ttm_tt *ttm);
• /**

• * struct ttm_backend_func member destroy
  

• *
  

• * @ttm: Pointer to a struct ttm_tt.
  

• *
  

• * Destroy the backend. This will be call back from ttm_tt_destroy so
  

• * don't call ttm_tt_destroy from the callback or infinite loop.
  

• */
  

• void (*destroy) (struct ttm_tt *ttm);

-};

-#define TTM_PAGE_FLAG_WRITE (1 << 3) -#define TTM_PAGE_FLAG_SWAPPED (1 << 4) -#define TTM_PAGE_FLAG_PERSISTENT_SWAP (1 << 5) -#define TTM_PAGE_FLAG_ZERO_ALLOC (1 << 6) -#define TTM_PAGE_FLAG_DMA32 (1 << 7) -#define TTM_PAGE_FLAG_SG (1 << 8) -#define TTM_PAGE_FLAG_NO_RETRY (1 << 9)

-enum ttm_caching_state {

• tt_uncached,
• tt_wc,
• tt_cached

-};

-/**

• • struct ttm_tt
• • @bdev: Pointer to a struct ttm_bo_device.
• • @func: Pointer to a struct ttm_backend_func that describes
• • the backend methods.
• • pointer.
• • @pages: Array of pages backing the data.
• • @num_pages: Number of pages in the page array.
• • @bdev: Pointer to the current struct ttm_bo_device.
• • @be: Pointer to the ttm backend.
• • @swap_storage: Pointer to shmem struct file for swap storage.
• • @caching_state: The current caching state of the pages.
• • @state: The current binding state of the pages.
• • This is a structure holding the pages, caching- and aperture binding
• • status for a buffer object that isn't backed by fixed (VRAM / AGP)
• • memory.
• */

-struct ttm_tt {

• struct ttm_bo_device *bdev;
• struct ttm_backend_func *func;
• struct page **pages;
• uint32_t page_flags;
• unsigned long num_pages;
• struct sg_table *sg; /* for SG objects via dma-buf */
• struct file *swap_storage;
• enum ttm_caching_state caching_state;
• enum {

• tt_bound,
  

• tt_unbound,
  

• tt_unpopulated,
  

• } state;

-};

-/**

• • struct ttm_dma_tt
• • @ttm: Base ttm_tt struct.
• • @dma_address: The DMA (bus) addresses of the pages
• • @pages_list: used by some page allocation backend
• • This is a structure holding the pages, caching- and aperture binding
• • status for a buffer object that isn't backed by fixed (VRAM / AGP)
• • memory.
• */

-struct ttm_dma_tt {

• struct ttm_tt ttm;
• dma_addr_t *dma_address;
• struct list_head pages_list;

-};

• #define TTM_MEMTYPE_FLAG_FIXED (1 << 0) /* Fixed (on-card) PCI memory */ #define TTM_MEMTYPE_FLAG_MAPPABLE (1 << 1) /* Memory mappable */ #define TTM_MEMTYPE_FLAG_CMA (1 << 3) /* Can't map aperture */

@@ -610,117 +509,6 @@ ttm_flag_masked(uint32_t *old, uint32_t new, uint32_t mask) return *old; }

-/**

• • ttm_tt_create
• • @bo: pointer to a struct ttm_buffer_object
• • @zero_alloc: true if allocated pages needs to be zeroed
• • Make sure we have a TTM structure allocated for the given BO.
• • No pages are actually allocated.
• */

-int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc);

-/**

• • ttm_tt_init
• • @ttm: The struct ttm_tt.
• • @bdev: pointer to a struct ttm_bo_device:
• • @size: Size of the data needed backing.
• • @page_flags: Page flags as identified by TTM_PAGE_FLAG_XX flags.
• • Create a struct ttm_tt to back data with system memory pages.
• • No pages are actually allocated.
• • Returns:
• • NULL: Out of memory.
• */

-int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

• unsigned long size, uint32_t page_flags);
  

-int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev,

•     unsigned long size, uint32_t page_flags);
  

-/**

• • ttm_tt_fini
• • @ttm: the ttm_tt structure.
• • Free memory of ttm_tt structure
• */

-void ttm_tt_fini(struct ttm_tt *ttm); -void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma);

-/**

• • ttm_ttm_bind:
• • @ttm: The struct ttm_tt containing backing pages.
• • @bo_mem: The struct ttm_mem_reg identifying the binding location.
• • Bind the pages of @ttm to an aperture location identified by @bo_mem
• */

-int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem,

• struct ttm_operation_ctx *ctx);
  

-/**

• • ttm_ttm_destroy:
• • @ttm: The struct ttm_tt.
• • Unbind, unpopulate and destroy common struct ttm_tt.
• */

-void ttm_tt_destroy(struct ttm_tt *ttm);

-/**

• • ttm_ttm_unbind:
• • @ttm: The struct ttm_tt.
• • Unbind a struct ttm_tt.
• */

-void ttm_tt_unbind(struct ttm_tt *ttm);

-/**

• • ttm_tt_swapin:
• • @ttm: The struct ttm_tt.
• • Swap in a previously swap out ttm_tt.
• */

-int ttm_tt_swapin(struct ttm_tt *ttm);

-/**

• • ttm_tt_set_placement_caching:
• • @ttm A struct ttm_tt the backing pages of which will change caching policy.
• • @placement: Flag indicating the desired caching policy.
• • This function will change caching policy of any default kernel mappings of
• • the pages backing @ttm. If changing from cached to uncached or
• • write-combined,
• • all CPU caches will first be flushed to make sure the data of the pages
• • hit RAM. This function may be very costly as it involves global TLB
• • and cache flushes and potential page splitting / combining.
• */

-int ttm_tt_set_placement_caching(struct ttm_tt *ttm, uint32_t placement); -int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage);

-/**

• • ttm_tt_populate - allocate pages for a ttm
• • @ttm: Pointer to the ttm_tt structure
• • Calls the driver method to allocate pages for a ttm
• */

-int ttm_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx);

-/**

• • ttm_tt_unpopulate - free pages from a ttm
• • @ttm: Pointer to the ttm_tt structure
• • Calls the driver method to free all pages from a ttm
• */

-void ttm_tt_unpopulate(struct ttm_tt *ttm);

• /*
  • ttm_bo.c
  */

@@ -1074,27 +862,4 @@ pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp);

extern const struct ttm_mem_type_manager_func ttm_bo_manager_func;

-#if IS_ENABLED(CONFIG_AGP) -#include <linux/agp_backend.h>

-/**

• • ttm_agp_tt_create
• • @bdev: Pointer to a struct ttm_bo_device.
• • @bridge: The agp bridge this device is sitting on.
• • @size: Size of the data needed backing.
• • @page_flags: Page flags as identified by TTM_PAGE_FLAG_XX flags.
• • Create a TTM backend that uses the indicated AGP bridge as an aperture
• • for TT memory. This function uses the linux agpgart interface to
• • bind and unbind memory backing a ttm_tt.
• */

-struct ttm_tt *ttm_agp_tt_create(struct ttm_bo_device *bdev,

• 		 struct agp_bridge_data *bridge,
  

• 		 unsigned long size, uint32_t page_flags);
  

-int ttm_agp_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx); -void ttm_agp_tt_unpopulate(struct ttm_tt *ttm); -#endif

• #endif

diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h new file mode 100644 index 000000000000..9c78556b488e --- /dev/null +++ b/include/drm/ttm/ttm_tt.h @@ -0,0 +1,272 @@ +/**************************************************************************

• • Copyright (c) 2006-2009 Vmware, Inc., Palo Alto, CA., USA
• • All Rights Reserved.
• • Permission is hereby granted, free of charge, to any person obtaining a
• • copy of this software and associated documentation files (the
• • "Software"), to deal in the Software without restriction, including
• • without limitation the rights to use, copy, modify, merge, publish,
• • distribute, sub license, and/or sell copies of the Software, and to
• • permit persons to whom the Software is furnished to do so, subject to
• • the following conditions:
• • The above copyright notice and this permission notice (including the
• • next paragraph) shall be included in all copies or substantial portions
• • of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
• • THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
• • DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
• • OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
• • USE OR OTHER DEALINGS IN THE SOFTWARE.
• **************************************************************************/

+#ifndef _TTM_TT_H_ +#define _TTM_TT_H_

+#include <linux/types.h>

+struct ttm_tt; +struct ttm_mem_reg; +struct ttm_buffer_object; +struct ttm_operation_ctx;

+#define TTM_PAGE_FLAG_WRITE (1 << 3) +#define TTM_PAGE_FLAG_SWAPPED (1 << 4) +#define TTM_PAGE_FLAG_PERSISTENT_SWAP (1 << 5) +#define TTM_PAGE_FLAG_ZERO_ALLOC (1 << 6) +#define TTM_PAGE_FLAG_DMA32 (1 << 7) +#define TTM_PAGE_FLAG_SG (1 << 8) +#define TTM_PAGE_FLAG_NO_RETRY (1 << 9)

+enum ttm_caching_state {

• tt_uncached,
• tt_wc,
• tt_cached

+};

+struct ttm_backend_func {

• /**

• * struct ttm_backend_func member bind
  

• *
  

• * @ttm: Pointer to a struct ttm_tt.
  

• * @bo_mem: Pointer to a struct ttm_mem_reg describing the
  

• * memory type and location for binding.
  

• *
  

• * Bind the backend pages into the aperture in the location
  

• * indicated by @bo_mem. This function should be able to handle
  

• * differences between aperture and system page sizes.
  

• */
  

• int (*bind) (struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem);
• /**

• * struct ttm_backend_func member unbind
  

• *
  

• * @ttm: Pointer to a struct ttm_tt.
  

• *
  

• * Unbind previously bound backend pages. This function should be
  

• * able to handle differences between aperture and system page sizes.
  

• */
  

• int (*unbind) (struct ttm_tt *ttm);
• /**

• * struct ttm_backend_func member destroy
  

• *
  

• * @ttm: Pointer to a struct ttm_tt.
  

• *
  

• * Destroy the backend. This will be call back from ttm_tt_destroy so
  

• * don't call ttm_tt_destroy from the callback or infinite loop.
  

• */
  

• void (*destroy) (struct ttm_tt *ttm);

+};

+/**

• • struct ttm_tt
• • @bdev: Pointer to a struct ttm_bo_device.
• • @func: Pointer to a struct ttm_backend_func that describes
• • the backend methods.
• • pointer.
• • @pages: Array of pages backing the data.
• • @num_pages: Number of pages in the page array.
• • @bdev: Pointer to the current struct ttm_bo_device.
• • @be: Pointer to the ttm backend.
• • @swap_storage: Pointer to shmem struct file for swap storage.
• • @caching_state: The current caching state of the pages.
• • @state: The current binding state of the pages.
• • This is a structure holding the pages, caching- and aperture binding
• • status for a buffer object that isn't backed by fixed (VRAM / AGP)
• • memory.
• */

+struct ttm_tt {

• struct ttm_bo_device *bdev;
• struct ttm_backend_func *func;
• struct page **pages;
• uint32_t page_flags;
• unsigned long num_pages;
• struct sg_table *sg; /* for SG objects via dma-buf */
• struct file *swap_storage;
• enum ttm_caching_state caching_state;
• enum {

• tt_bound,
  

• tt_unbound,
  

• tt_unpopulated,
  

• } state;

+};

+/**

• • struct ttm_dma_tt
• • @ttm: Base ttm_tt struct.
• • @dma_address: The DMA (bus) addresses of the pages
• • @pages_list: used by some page allocation backend
• • This is a structure holding the pages, caching- and aperture binding
• • status for a buffer object that isn't backed by fixed (VRAM / AGP)
• • memory.
• */

+struct ttm_dma_tt {

• struct ttm_tt ttm;
• dma_addr_t *dma_address;
• struct list_head pages_list;

+};

+/**

• • ttm_tt_create
• • @bo: pointer to a struct ttm_buffer_object
• • @zero_alloc: true if allocated pages needs to be zeroed
• • Make sure we have a TTM structure allocated for the given BO.
• • No pages are actually allocated.
• */

+int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc);

+/**

• • ttm_tt_init
• • @ttm: The struct ttm_tt.
• • @bdev: pointer to a struct ttm_bo_device:
• • @size: Size of the data needed backing.
• • @page_flags: Page flags as identified by TTM_PAGE_FLAG_XX flags.
• • Create a struct ttm_tt to back data with system memory pages.
• • No pages are actually allocated.
• • Returns:
• • NULL: Out of memory.
• */

+int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

• unsigned long size, uint32_t page_flags);
  

+int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev,

•     unsigned long size, uint32_t page_flags);
  

+/**

• • ttm_tt_fini
• • @ttm: the ttm_tt structure.
• • Free memory of ttm_tt structure
• */

+void ttm_tt_fini(struct ttm_tt *ttm); +void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma);

+/**

• • ttm_ttm_bind:
• • @ttm: The struct ttm_tt containing backing pages.
• • @bo_mem: The struct ttm_mem_reg identifying the binding location.
• • Bind the pages of @ttm to an aperture location identified by @bo_mem
• */

+int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem,

• struct ttm_operation_ctx *ctx);
  

+/**

• • ttm_ttm_destroy:
• • @ttm: The struct ttm_tt.
• • Unbind, unpopulate and destroy common struct ttm_tt.
• */

+void ttm_tt_destroy(struct ttm_tt *ttm);

+/**

• • ttm_ttm_unbind:
• • @ttm: The struct ttm_tt.
• • Unbind a struct ttm_tt.
• */

+void ttm_tt_unbind(struct ttm_tt *ttm);

+/**

• • ttm_tt_swapin:
• • @ttm: The struct ttm_tt.
• • Swap in a previously swap out ttm_tt.
• */

+int ttm_tt_swapin(struct ttm_tt *ttm);

+/**

• • ttm_tt_set_placement_caching:
• • @ttm A struct ttm_tt the backing pages of which will change caching policy.
• • @placement: Flag indicating the desired caching policy.
• • This function will change caching policy of any default kernel mappings of
• • the pages backing @ttm. If changing from cached to uncached or
• • write-combined,
• • all CPU caches will first be flushed to make sure the data of the pages
• • hit RAM. This function may be very costly as it involves global TLB
• • and cache flushes and potential page splitting / combining.
• */

+int ttm_tt_set_placement_caching(struct ttm_tt *ttm, uint32_t placement); +int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage);

+/**

• • ttm_tt_populate - allocate pages for a ttm
• • @ttm: Pointer to the ttm_tt structure
• • Calls the driver method to allocate pages for a ttm
• */

+int ttm_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx);

+/**

• • ttm_tt_unpopulate - free pages from a ttm
• • @ttm: Pointer to the ttm_tt structure
• • Calls the driver method to free all pages from a ttm
• */

+void ttm_tt_unpopulate(struct ttm_tt *ttm);

+#if IS_ENABLED(CONFIG_AGP) +#include <linux/agp_backend.h>

+/**

• • ttm_agp_tt_create
• • @bdev: Pointer to a struct ttm_bo_device.
• • @bridge: The agp bridge this device is sitting on.
• • @size: Size of the data needed backing.
• • @page_flags: Page flags as identified by TTM_PAGE_FLAG_XX flags.
• • Create a TTM backend that uses the indicated AGP bridge as an aperture
• • for TT memory. This function uses the linux agpgart interface to
• • bind and unbind memory backing a ttm_tt.
• */

+struct ttm_tt *ttm_agp_tt_create(struct ttm_bo_device *bdev,

• 		 struct agp_bridge_data *bridge,
  

• 		 unsigned long size, uint32_t page_flags);
  

+int ttm_agp_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx); +void ttm_agp_tt_unpopulate(struct ttm_tt *ttm); +#endif

+#endif

Acked-by: Thomas Hellstrom thellstrom@vmware.com

On 03/06/2018 10:13 AM, Christian König wrote:

Hi Michel & Thomas,

any more comments on this? Or can I commit it?

Thanks, Christian.

Am 27.02.2018 um 12:49 schrieb Christian König:

...

Let's stop mangling everything in a single header and create one header per object instead.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/ttm/ttm_tt.c    |   6 -   include/drm/ttm/ttm_bo_driver.h | 237 +---------------------------------   include/drm/ttm/ttm_tt.h        | 272 ++++++++++++++++++++++++++++++++++++++++   3 files changed, 273 insertions(+), 242 deletions(-)   create mode 100644 include/drm/ttm/ttm_tt.h

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 0ee3b8f11605..8e0b525cda00 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -31,17 +31,11 @@   #define pr_fmt(fmt) "[TTM] " fmt     #include <linux/sched.h> -#include <linux/highmem.h>   #include <linux/pagemap.h>   #include <linux/shmem_fs.h>   #include <linux/file.h> -#include <linux/swap.h> -#include <linux/slab.h> -#include <linux/export.h>   #include <drm/drm_cache.h> -#include <drm/ttm/ttm_module.h>   #include <drm/ttm/ttm_bo_driver.h> -#include <drm/ttm/ttm_placement.h>   #include <drm/ttm/ttm_page_alloc.h>   #ifdef CONFIG_X86   #include <asm/set_memory.h> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 4312b5326f0b..f8e2515b401f 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -42,111 +42,10 @@   #include "ttm_memory.h"   #include "ttm_module.h"   #include "ttm_placement.h" +#include "ttm_tt.h"     #define TTM_MAX_BO_PRIORITY    4U   -struct ttm_backend_func { -    /** -     * struct ttm_backend_func member bind -     * -     * @ttm: Pointer to a struct ttm_tt. -     * @bo_mem: Pointer to a struct ttm_mem_reg describing the -     * memory type and location for binding. -     * -     * Bind the backend pages into the aperture in the location -     * indicated by @bo_mem. This function should be able to handle -     * differences between aperture and system page sizes. -     */ -    int (*bind) (struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem);

-    /** -     * struct ttm_backend_func member unbind -     * -     * @ttm: Pointer to a struct ttm_tt. -     * -     * Unbind previously bound backend pages. This function should be -     * able to handle differences between aperture and system page sizes. -     */ -    int (*unbind) (struct ttm_tt *ttm);

-    /** -     * struct ttm_backend_func member destroy -     * -     * @ttm: Pointer to a struct ttm_tt. -     * -     * Destroy the backend. This will be call back from ttm_tt_destroy so -     * don't call ttm_tt_destroy from the callback or infinite loop. -     */ -    void (*destroy) (struct ttm_tt *ttm); -};

-#define TTM_PAGE_FLAG_WRITE           (1 << 3) -#define TTM_PAGE_FLAG_SWAPPED         (1 << 4) -#define TTM_PAGE_FLAG_PERSISTENT_SWAP (1 << 5) -#define TTM_PAGE_FLAG_ZERO_ALLOC      (1 << 6) -#define TTM_PAGE_FLAG_DMA32           (1 << 7) -#define TTM_PAGE_FLAG_SG              (1 << 8) -#define TTM_PAGE_FLAG_NO_RETRY          (1 << 9)

-enum ttm_caching_state { -    tt_uncached, -    tt_wc, -    tt_cached -};

-/**

• • struct ttm_tt
• • @bdev: Pointer to a struct ttm_bo_device.
• • @func: Pointer to a struct ttm_backend_func that describes
• • the backend methods.
• • pointer.
• • @pages: Array of pages backing the data.
• • @num_pages: Number of pages in the page array.
• • @bdev: Pointer to the current struct ttm_bo_device.
• • @be: Pointer to the ttm backend.
• • @swap_storage: Pointer to shmem struct file for swap storage.
• • @caching_state: The current caching state of the pages.
• • @state: The current binding state of the pages.
• • This is a structure holding the pages, caching- and aperture binding
• • status for a buffer object that isn't backed by fixed (VRAM / AGP)
• • memory.
• */

-struct ttm_tt { -    struct ttm_bo_device *bdev; -    struct ttm_backend_func *func; -    struct page **pages; -    uint32_t page_flags; -    unsigned long num_pages; -    struct sg_table *sg; /* for SG objects via dma-buf */ -    struct file *swap_storage; -    enum ttm_caching_state caching_state; -    enum { -        tt_bound, -        tt_unbound, -        tt_unpopulated, -    } state; -};

-/**

• • struct ttm_dma_tt
• • @ttm: Base ttm_tt struct.
• • @dma_address: The DMA (bus) addresses of the pages
• • @pages_list: used by some page allocation backend
• • This is a structure holding the pages, caching- and aperture binding
• • status for a buffer object that isn't backed by fixed (VRAM / AGP)
• • memory.
• */

-struct ttm_dma_tt { -    struct ttm_tt ttm; -    dma_addr_t *dma_address; -    struct list_head pages_list; -};

#define TTM_MEMTYPE_FLAG_FIXED         (1 << 0)    /* Fixed (on-card) PCI memory */   #define TTM_MEMTYPE_FLAG_MAPPABLE      (1 << 1)    /* Memory mappable */   #define TTM_MEMTYPE_FLAG_CMA           (1 << 3)    /* Can't map aperture */ @@ -610,117 +509,6 @@ ttm_flag_masked(uint32_t *old, uint32_t new, uint32_t mask)       return *old;   }   -/**

• • ttm_tt_create
• • @bo: pointer to a struct ttm_buffer_object
• • @zero_alloc: true if allocated pages needs to be zeroed
• • Make sure we have a TTM structure allocated for the given BO.
• • No pages are actually allocated.
• */

-int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc);

-/**

• • ttm_tt_init
• • @ttm: The struct ttm_tt.
• • @bdev: pointer to a struct ttm_bo_device:
• • @size: Size of the data needed backing.
• • @page_flags: Page flags as identified by TTM_PAGE_FLAG_XX flags.
• • Create a struct ttm_tt to back data with system memory pages.
• • No pages are actually allocated.
• • Returns:
• • NULL: Out of memory.
• */

-int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev, -        unsigned long size, uint32_t page_flags); -int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, -            unsigned long size, uint32_t page_flags);

-/**

• • ttm_tt_fini
• • @ttm: the ttm_tt structure.
• • Free memory of ttm_tt structure
• */

-void ttm_tt_fini(struct ttm_tt *ttm); -void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma);

-/**

• • ttm_ttm_bind:
• • @ttm: The struct ttm_tt containing backing pages.
• • @bo_mem: The struct ttm_mem_reg identifying the binding location.
• • Bind the pages of @ttm to an aperture location identified by @bo_mem
• */

-int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem, -        struct ttm_operation_ctx *ctx);

-/**

• • ttm_ttm_destroy:
• • @ttm: The struct ttm_tt.
• • Unbind, unpopulate and destroy common struct ttm_tt.
• */

-void ttm_tt_destroy(struct ttm_tt *ttm);

-/**

• • ttm_ttm_unbind:
• • @ttm: The struct ttm_tt.
• • Unbind a struct ttm_tt.
• */

-void ttm_tt_unbind(struct ttm_tt *ttm);

-/**

• • ttm_tt_swapin:
• • @ttm: The struct ttm_tt.
• • Swap in a previously swap out ttm_tt.
• */

-int ttm_tt_swapin(struct ttm_tt *ttm);

-/**

• • ttm_tt_set_placement_caching:
• • @ttm A struct ttm_tt the backing pages of which will change

caching policy.

• • @placement: Flag indicating the desired caching policy.
• • This function will change caching policy of any default kernel

mappings of

• • the pages backing @ttm. If changing from cached to uncached or
• • write-combined,
• • all CPU caches will first be flushed to make sure the data of the

pages

• • hit RAM. This function may be very costly as it involves global TLB
• • and cache flushes and potential page splitting / combining.
• */

-int ttm_tt_set_placement_caching(struct ttm_tt *ttm, uint32_t placement); -int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage);

-/**

• • ttm_tt_populate - allocate pages for a ttm
• • @ttm: Pointer to the ttm_tt structure
• • Calls the driver method to allocate pages for a ttm
• */

-int ttm_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx);

-/**

• • ttm_tt_unpopulate - free pages from a ttm
• • @ttm: Pointer to the ttm_tt structure
• • Calls the driver method to free all pages from a ttm
• */

-void ttm_tt_unpopulate(struct ttm_tt *ttm);

/*    * ttm_bo.c    */ @@ -1074,27 +862,4 @@ pgprot_t ttm_io_prot(uint32_t caching_flags, pgprot_t tmp);     extern const struct ttm_mem_type_manager_func ttm_bo_manager_func;   -#if IS_ENABLED(CONFIG_AGP) -#include <linux/agp_backend.h>

-/**

• • ttm_agp_tt_create
• • @bdev: Pointer to a struct ttm_bo_device.
• • @bridge: The agp bridge this device is sitting on.
• • @size: Size of the data needed backing.
• • @page_flags: Page flags as identified by TTM_PAGE_FLAG_XX flags.
• • Create a TTM backend that uses the indicated AGP bridge as an

aperture

• • for TT memory. This function uses the linux agpgart interface to
• • bind and unbind memory backing a ttm_tt.
• */

-struct ttm_tt *ttm_agp_tt_create(struct ttm_bo_device *bdev, -                 struct agp_bridge_data *bridge, -                 unsigned long size, uint32_t page_flags); -int ttm_agp_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx); -void ttm_agp_tt_unpopulate(struct ttm_tt *ttm); -#endif

#endif diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h new file mode 100644 index 000000000000..9c78556b488e --- /dev/null +++ b/include/drm/ttm/ttm_tt.h @@ -0,0 +1,272 @@ +/**************************************************************************

• • Copyright (c) 2006-2009 Vmware, Inc., Palo Alto, CA., USA
• • All Rights Reserved.
• • Permission is hereby granted, free of charge, to any person

obtaining a

• • copy of this software and associated documentation files (the
• • "Software"), to deal in the Software without restriction, including
• • without limitation the rights to use, copy, modify, merge, publish,
• • distribute, sub license, and/or sell copies of the Software, and to
• • permit persons to whom the Software is furnished to do so,

subject to

• • the following conditions:
• • The above copyright notice and this permission notice (including the
• • next paragraph) shall be included in all copies or substantial

portions

• • of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,

EXPRESS OR

• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF

MERCHANTABILITY,

• • FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO

EVENT SHALL

• • THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR

ANY CLAIM,

• • DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,

TORT OR

• • OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE

SOFTWARE OR THE

• • USE OR OTHER DEALINGS IN THE SOFTWARE.

**************************************************************************/ +#ifndef _TTM_TT_H_ +#define _TTM_TT_H_

+#include <linux/types.h>

+struct ttm_tt; +struct ttm_mem_reg; +struct ttm_buffer_object; +struct ttm_operation_ctx;

+#define TTM_PAGE_FLAG_WRITE           (1 << 3) +#define TTM_PAGE_FLAG_SWAPPED         (1 << 4) +#define TTM_PAGE_FLAG_PERSISTENT_SWAP (1 << 5) +#define TTM_PAGE_FLAG_ZERO_ALLOC      (1 << 6) +#define TTM_PAGE_FLAG_DMA32           (1 << 7) +#define TTM_PAGE_FLAG_SG              (1 << 8) +#define TTM_PAGE_FLAG_NO_RETRY          (1 << 9)

+enum ttm_caching_state { +    tt_uncached, +    tt_wc, +    tt_cached +};

+struct ttm_backend_func { +    /** +     * struct ttm_backend_func member bind +     * +     * @ttm: Pointer to a struct ttm_tt. +     * @bo_mem: Pointer to a struct ttm_mem_reg describing the +     * memory type and location for binding. +     * +     * Bind the backend pages into the aperture in the location +     * indicated by @bo_mem. This function should be able to handle +     * differences between aperture and system page sizes. +     */ +    int (*bind) (struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem);

+    /** +     * struct ttm_backend_func member unbind +     * +     * @ttm: Pointer to a struct ttm_tt. +     * +     * Unbind previously bound backend pages. This function should be +     * able to handle differences between aperture and system page sizes. +     */ +    int (*unbind) (struct ttm_tt *ttm);

+    /** +     * struct ttm_backend_func member destroy +     * +     * @ttm: Pointer to a struct ttm_tt. +     * +     * Destroy the backend. This will be call back from ttm_tt_destroy so +     * don't call ttm_tt_destroy from the callback or infinite loop. +     */ +    void (*destroy) (struct ttm_tt *ttm); +};

+/**

• • struct ttm_tt
• • @bdev: Pointer to a struct ttm_bo_device.
• • @func: Pointer to a struct ttm_backend_func that describes
• • the backend methods.
• • pointer.
• • @pages: Array of pages backing the data.
• • @num_pages: Number of pages in the page array.
• • @bdev: Pointer to the current struct ttm_bo_device.
• • @be: Pointer to the ttm backend.
• • @swap_storage: Pointer to shmem struct file for swap storage.
• • @caching_state: The current caching state of the pages.
• • @state: The current binding state of the pages.
• • This is a structure holding the pages, caching- and aperture binding
• • status for a buffer object that isn't backed by fixed (VRAM / AGP)
• • memory.
• */

+struct ttm_tt { +    struct ttm_bo_device *bdev; +    struct ttm_backend_func *func; +    struct page **pages; +    uint32_t page_flags; +    unsigned long num_pages; +    struct sg_table *sg; /* for SG objects via dma-buf */ +    struct file *swap_storage; +    enum ttm_caching_state caching_state; +    enum { +        tt_bound, +        tt_unbound, +        tt_unpopulated, +    } state; +};

+/**

• • struct ttm_dma_tt
• • @ttm: Base ttm_tt struct.
• • @dma_address: The DMA (bus) addresses of the pages
• • @pages_list: used by some page allocation backend
• • This is a structure holding the pages, caching- and aperture binding
• • status for a buffer object that isn't backed by fixed (VRAM / AGP)
• • memory.
• */

+struct ttm_dma_tt { +    struct ttm_tt ttm; +    dma_addr_t *dma_address; +    struct list_head pages_list; +};

+/**

• • ttm_tt_create
• • @bo: pointer to a struct ttm_buffer_object
• • @zero_alloc: true if allocated pages needs to be zeroed
• • Make sure we have a TTM structure allocated for the given BO.
• • No pages are actually allocated.
• */

+int ttm_tt_create(struct ttm_buffer_object *bo, bool zero_alloc);

+/**

• • ttm_tt_init
• • @ttm: The struct ttm_tt.
• • @bdev: pointer to a struct ttm_bo_device:
• • @size: Size of the data needed backing.
• • @page_flags: Page flags as identified by TTM_PAGE_FLAG_XX flags.
• • Create a struct ttm_tt to back data with system memory pages.
• • No pages are actually allocated.
• • Returns:
• • NULL: Out of memory.
• */

+int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev, +        unsigned long size, uint32_t page_flags); +int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, +            unsigned long size, uint32_t page_flags);

+/**

• • ttm_tt_fini
• • @ttm: the ttm_tt structure.
• • Free memory of ttm_tt structure
• */

+void ttm_tt_fini(struct ttm_tt *ttm); +void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma);

+/**

• • ttm_ttm_bind:
• • @ttm: The struct ttm_tt containing backing pages.
• • @bo_mem: The struct ttm_mem_reg identifying the binding location.
• • Bind the pages of @ttm to an aperture location identified by @bo_mem
• */

+int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem, +        struct ttm_operation_ctx *ctx);

+/**

• • ttm_ttm_destroy:
• • @ttm: The struct ttm_tt.
• • Unbind, unpopulate and destroy common struct ttm_tt.
• */

+void ttm_tt_destroy(struct ttm_tt *ttm);

+/**

• • ttm_ttm_unbind:
• • @ttm: The struct ttm_tt.
• • Unbind a struct ttm_tt.
• */

+void ttm_tt_unbind(struct ttm_tt *ttm);

+/**

• • ttm_tt_swapin:
• • @ttm: The struct ttm_tt.
• • Swap in a previously swap out ttm_tt.
• */

+int ttm_tt_swapin(struct ttm_tt *ttm);

+/**

• • ttm_tt_set_placement_caching:
• • @ttm A struct ttm_tt the backing pages of which will change

caching policy.

• • @placement: Flag indicating the desired caching policy.
• • This function will change caching policy of any default kernel

mappings of

• • the pages backing @ttm. If changing from cached to uncached or
• • write-combined,
• • all CPU caches will first be flushed to make sure the data of the

pages

• • hit RAM. This function may be very costly as it involves global TLB
• • and cache flushes and potential page splitting / combining.
• */

+int ttm_tt_set_placement_caching(struct ttm_tt *ttm, uint32_t placement); +int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage);

+/**

• • ttm_tt_populate - allocate pages for a ttm
• • @ttm: Pointer to the ttm_tt structure
• • Calls the driver method to allocate pages for a ttm
• */

+int ttm_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx);

+/**

• • ttm_tt_unpopulate - free pages from a ttm
• • @ttm: Pointer to the ttm_tt structure
• • Calls the driver method to free all pages from a ttm
• */

+void ttm_tt_unpopulate(struct ttm_tt *ttm);

+#if IS_ENABLED(CONFIG_AGP) +#include <linux/agp_backend.h>

+/**

• • ttm_agp_tt_create
• • @bdev: Pointer to a struct ttm_bo_device.
• • @bridge: The agp bridge this device is sitting on.
• • @size: Size of the data needed backing.
• • @page_flags: Page flags as identified by TTM_PAGE_FLAG_XX flags.
• • Create a TTM backend that uses the indicated AGP bridge as an

aperture

• • for TT memory. This function uses the linux agpgart interface to
• • bind and unbind memory backing a ttm_tt.
• */

+struct ttm_tt *ttm_agp_tt_create(struct ttm_bo_device *bdev, +                 struct agp_bridge_data *bridge, +                 unsigned long size, uint32_t page_flags); +int ttm_agp_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx); +void ttm_agp_tt_unpopulate(struct ttm_tt *ttm); +#endif

+#endif

Hi guys,

at least on amdgpu and radeon the page array allocated by ttm_dma_tt_init is completely unused in the case of DMA-buf sharing. So I'm trying to get rid of that by only allocating the DMA address array.

Now the only other user of DMA-buf together with ttm_dma_tt_init is Nouveau. So my question is are you guys using the page array anywhere in your kernel driver in case of a DMA-buf sharing?

If no then I could just make this the default behavior for all drivers and save quite a bit of memory for everybody.

Thanks, Christian.

Am 27.02.2018 um 12:49 schrieb Christian König:

This allows drivers to only allocate dma addresses, but not a page array.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/ttm/ttm_tt.c | 54 ++++++++++++++++++++++++++++++++++++-------- include/drm/ttm/ttm_tt.h | 2 ++ 2 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 8e0b525cda00..971133106ec2 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -108,6 +108,16 @@ static int ttm_dma_tt_alloc_page_directory(struct ttm_dma_tt *ttm) return 0; }

+static int ttm_sg_tt_alloc_page_directory(struct ttm_dma_tt *ttm) +{

• ttm->dma_address = kvmalloc_array(ttm->ttm.num_pages,

• 			  sizeof(*ttm->dma_address),
  

• 			  GFP_KERNEL | __GFP_ZERO);
  

• if (!ttm->dma_address)

• return -ENOMEM;
  

• return 0;

+}

• #ifdef CONFIG_X86 static inline int ttm_tt_set_page_caching(struct page *p, enum ttm_caching_state c_old,

@@ -227,8 +237,8 @@ void ttm_tt_destroy(struct ttm_tt *ttm) ttm->func->destroy(ttm); }

-int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

• unsigned long size, uint32_t page_flags)
  

+void ttm_tt_init_fields(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

• 	unsigned long size, uint32_t page_flags)
  

  { ttm->bdev = bdev; ttm->num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;

@@ -236,6 +246,12 @@ int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev, ttm->page_flags = page_flags; ttm->state = tt_unpopulated; ttm->swap_storage = NULL; +}

+int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

• unsigned long size, uint32_t page_flags)
  

+{

• ttm_tt_init_fields(ttm, bdev, size, page_flags);

  if (ttm_tt_alloc_page_directory(ttm)) { ttm_tt_destroy(ttm);

@@ -258,12 +274,7 @@ int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, { struct ttm_tt *ttm = &ttm_dma->ttm;

• ttm->bdev = bdev;
• ttm->num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
• ttm->caching_state = tt_cached;
• ttm->page_flags = page_flags;
• ttm->state = tt_unpopulated;
• ttm->swap_storage = NULL;

• ttm_tt_init_fields(ttm, bdev, size, page_flags);

  INIT_LIST_HEAD(&ttm_dma->pages_list); if (ttm_dma_tt_alloc_page_directory(ttm_dma)) {

@@ -275,11 +286,36 @@ int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, } EXPORT_SYMBOL(ttm_dma_tt_init);

+int ttm_sg_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev,

•    unsigned long size, uint32_t page_flags)
  

+{

• struct ttm_tt *ttm = &ttm_dma->ttm;
• int ret;
• ttm_tt_init_fields(ttm, bdev, size, page_flags);
• INIT_LIST_HEAD(&ttm_dma->pages_list);
• if (page_flags & TTM_PAGE_FLAG_SG)

• ret = ttm_sg_tt_alloc_page_directory(ttm_dma);
  

• else

• ret = ttm_dma_tt_alloc_page_directory(ttm_dma);
  

• if (ret) {

• ttm_tt_destroy(ttm);
  

• pr_err("Failed allocating page table\n");
  

• return -ENOMEM;
  

• }
• return 0;

+} +EXPORT_SYMBOL(ttm_sg_tt_init);

• void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma) { struct ttm_tt *ttm = &ttm_dma->ttm;

• kvfree(ttm->pages);

• if (ttm->pages)

• kvfree(ttm->pages);
  

• else

• kvfree(ttm_dma->dma_address);
  

  ttm->pages = NULL; ttm_dma->dma_address = NULL; }

diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h index 9c78556b488e..1cf316a4257c 100644 --- a/include/drm/ttm/ttm_tt.h +++ b/include/drm/ttm/ttm_tt.h @@ -163,6 +163,8 @@ int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev, unsigned long size, uint32_t page_flags); int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, unsigned long size, uint32_t page_flags); +int ttm_sg_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev,

•    unsigned long size, uint32_t page_flags);
  
  

  /**

  • ttm_tt_fini

On Mon, Mar 5, 2018 at 10:06 PM, Christian König christian.koenig@amd.com wrote:

Ping?

On a quick look, it looks like both are used sometimes. I believe this had something to do with the addition of Tegra support, but I'll need some time to look into the details of why/how these things are used again.

Ben.

Am 27.02.2018 um 13:07 schrieb Christian König:

...

Hi guys,

at least on amdgpu and radeon the page array allocated by ttm_dma_tt_init is completely unused in the case of DMA-buf sharing. So I'm trying to get rid of that by only allocating the DMA address array.

Now the only other user of DMA-buf together with ttm_dma_tt_init is Nouveau. So my question is are you guys using the page array anywhere in your kernel driver in case of a DMA-buf sharing?

If no then I could just make this the default behavior for all drivers and save quite a bit of memory for everybody.

Thanks, Christian.

Am 27.02.2018 um 12:49 schrieb Christian König:

...

This allows drivers to only allocate dma addresses, but not a page array.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/ttm/ttm_tt.c | 54 ++++++++++++++++++++++++++++++++++++-------- include/drm/ttm/ttm_tt.h | 2 ++ 2 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 8e0b525cda00..971133106ec2 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -108,6 +108,16 @@ static int ttm_dma_tt_alloc_page_directory(struct ttm_dma_tt *ttm) return 0; } +static int ttm_sg_tt_alloc_page_directory(struct ttm_dma_tt *ttm) +{

• ttm->dma_address = kvmalloc_array(ttm->ttm.num_pages,

•                  sizeof(*ttm->dma_address),
  

•                  GFP_KERNEL | __GFP_ZERO);
  

• if (!ttm->dma_address)

•    return -ENOMEM;
  

• return 0;

+}

• #ifdef CONFIG_X86 static inline int ttm_tt_set_page_caching(struct page *p, enum ttm_caching_state c_old,

@@ -227,8 +237,8 @@ void ttm_tt_destroy(struct ttm_tt *ttm) ttm->func->destroy(ttm); } -int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

•    unsigned long size, uint32_t page_flags)
  

+void ttm_tt_init_fields(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

•        unsigned long size, uint32_t page_flags)
  

  { ttm->bdev = bdev; ttm->num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;

@@ -236,6 +246,12 @@ int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev, ttm->page_flags = page_flags; ttm->state = tt_unpopulated; ttm->swap_storage = NULL; +}

+int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

•    unsigned long size, uint32_t page_flags)
  

+{

• ttm_tt_init_fields(ttm, bdev, size, page_flags); if (ttm_tt_alloc_page_directory(ttm)) { ttm_tt_destroy(ttm);

@@ -258,12 +274,7 @@ int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, { struct ttm_tt *ttm = &ttm_dma->ttm;

• ttm->bdev = bdev;
• ttm->num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
• ttm->caching_state = tt_cached;
• ttm->page_flags = page_flags;
• ttm->state = tt_unpopulated;
• ttm->swap_storage = NULL;

• ttm_tt_init_fields(ttm, bdev, size, page_flags); INIT_LIST_HEAD(&ttm_dma->pages_list); if (ttm_dma_tt_alloc_page_directory(ttm_dma)) {

@@ -275,11 +286,36 @@ int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, } EXPORT_SYMBOL(ttm_dma_tt_init); +int ttm_sg_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev,

•       unsigned long size, uint32_t page_flags)
  

+{

• struct ttm_tt *ttm = &ttm_dma->ttm;
• int ret;
• ttm_tt_init_fields(ttm, bdev, size, page_flags);
• INIT_LIST_HEAD(&ttm_dma->pages_list);
• if (page_flags & TTM_PAGE_FLAG_SG)

•    ret = ttm_sg_tt_alloc_page_directory(ttm_dma);
  

• else

•    ret = ttm_dma_tt_alloc_page_directory(ttm_dma);
  

• if (ret) {

•    ttm_tt_destroy(ttm);
  

•    pr_err("Failed allocating page table\n");
  

•    return -ENOMEM;
  

• }
• return 0;

+} +EXPORT_SYMBOL(ttm_sg_tt_init);

• void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma) { struct ttm_tt *ttm = &ttm_dma->ttm;
  • kvfree(ttm->pages);
• if (ttm->pages)

•    kvfree(ttm->pages);
  

• else

•    kvfree(ttm_dma->dma_address);
  ttm->pages = NULL;
  ttm_dma->dma_address = NULL;
  

  }

diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h index 9c78556b488e..1cf316a4257c 100644 --- a/include/drm/ttm/ttm_tt.h +++ b/include/drm/ttm/ttm_tt.h @@ -163,6 +163,8 @@ int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev, unsigned long size, uint32_t page_flags); int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, unsigned long size, uint32_t page_flags); +int ttm_sg_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev,

•       unsigned long size, uint32_t page_flags);
  

  /**
  • ttm_tt_fini

On Tue, Feb 27, 2018 at 01:07:06PM +0100, Christian König wrote:

Hi guys,

at least on amdgpu and radeon the page array allocated by ttm_dma_tt_init is completely unused in the case of DMA-buf sharing. So I'm trying to get rid of that by only allocating the DMA address array.

Now the only other user of DMA-buf together with ttm_dma_tt_init is Nouveau. So my question is are you guys using the page array anywhere in your kernel driver in case of a DMA-buf sharing?

If no then I could just make this the default behavior for all drivers and save quite a bit of memory for everybody.

+1 on teaching ttm to no longer look at the struct page * in the dma-buf sgt, but only the dma_buf address.

If there's still some need for in-kernel cpu or userspace mmap access then imo ttm needs to be fixed to delegate all that to the right dma-buf interfaces. The ttm abstraction is already there, it's just not passed through.

I don't pretend to now enough of the details to review this stuff :-) -Daniel

Thanks, Christian.

Am 27.02.2018 um 12:49 schrieb Christian König:

...

This allows drivers to only allocate dma addresses, but not a page array.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/ttm/ttm_tt.c | 54 ++++++++++++++++++++++++++++++++++++-------- include/drm/ttm/ttm_tt.h | 2 ++ 2 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 8e0b525cda00..971133106ec2 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -108,6 +108,16 @@ static int ttm_dma_tt_alloc_page_directory(struct ttm_dma_tt *ttm) return 0; } +static int ttm_sg_tt_alloc_page_directory(struct ttm_dma_tt *ttm) +{

• ttm->dma_address = kvmalloc_array(ttm->ttm.num_pages,

• 			  sizeof(*ttm->dma_address),
  

• 			  GFP_KERNEL | __GFP_ZERO);
  

• if (!ttm->dma_address)

• return -ENOMEM;
  

• return 0;

+}

• #ifdef CONFIG_X86 static inline int ttm_tt_set_page_caching(struct page *p, enum ttm_caching_state c_old,

@@ -227,8 +237,8 @@ void ttm_tt_destroy(struct ttm_tt *ttm) ttm->func->destroy(ttm); } -int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

• unsigned long size, uint32_t page_flags)
  

+void ttm_tt_init_fields(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

• 	unsigned long size, uint32_t page_flags)
  

  { ttm->bdev = bdev; ttm->num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;

@@ -236,6 +246,12 @@ int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev, ttm->page_flags = page_flags; ttm->state = tt_unpopulated; ttm->swap_storage = NULL; +}

+int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,

• unsigned long size, uint32_t page_flags)
  

+{

• ttm_tt_init_fields(ttm, bdev, size, page_flags); if (ttm_tt_alloc_page_directory(ttm)) { ttm_tt_destroy(ttm);

@@ -258,12 +274,7 @@ int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, { struct ttm_tt *ttm = &ttm_dma->ttm;

• ttm->bdev = bdev;
• ttm->num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
• ttm->caching_state = tt_cached;
• ttm->page_flags = page_flags;
• ttm->state = tt_unpopulated;
• ttm->swap_storage = NULL;

• ttm_tt_init_fields(ttm, bdev, size, page_flags); INIT_LIST_HEAD(&ttm_dma->pages_list); if (ttm_dma_tt_alloc_page_directory(ttm_dma)) {

@@ -275,11 +286,36 @@ int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, } EXPORT_SYMBOL(ttm_dma_tt_init); +int ttm_sg_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev,

•    unsigned long size, uint32_t page_flags)
  

+{

• struct ttm_tt *ttm = &ttm_dma->ttm;
• int ret;
• ttm_tt_init_fields(ttm, bdev, size, page_flags);
• INIT_LIST_HEAD(&ttm_dma->pages_list);
• if (page_flags & TTM_PAGE_FLAG_SG)

• ret = ttm_sg_tt_alloc_page_directory(ttm_dma);
  

• else

• ret = ttm_dma_tt_alloc_page_directory(ttm_dma);
  

• if (ret) {

• ttm_tt_destroy(ttm);
  

• pr_err("Failed allocating page table\n");
  

• return -ENOMEM;
  

• }
• return 0;

+} +EXPORT_SYMBOL(ttm_sg_tt_init);

• void ttm_dma_tt_fini(struct ttm_dma_tt *ttm_dma) { struct ttm_tt *ttm = &ttm_dma->ttm;

• kvfree(ttm->pages);

• if (ttm->pages)

• kvfree(ttm->pages);
  

• else

• kvfree(ttm_dma->dma_address);
  

  ttm->pages = NULL; ttm_dma->dma_address = NULL; }

diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h index 9c78556b488e..1cf316a4257c 100644 --- a/include/drm/ttm/ttm_tt.h +++ b/include/drm/ttm/ttm_tt.h @@ -163,6 +163,8 @@ int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev, unsigned long size, uint32_t page_flags); int ttm_dma_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev, unsigned long size, uint32_t page_flags); +int ttm_sg_tt_init(struct ttm_dma_tt *ttm_dma, struct ttm_bo_device *bdev,

•    unsigned long size, uint32_t page_flags);
  

  /**
  • ttm_tt_fini

---

Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau

On Wed, Feb 28, 2018 at 11:25:59AM +0100, Christian König wrote:

Am 28.02.2018 um 10:48 schrieb Lucas Stach:

...

Hi Christian,

Am Dienstag, den 27.02.2018, 12:49 +0100 schrieb Christian König:

...

Unpin the GEM object only after freeing the sg table.

What is the race that is being fixed here? The SG table is private to the importer and the importer should hopefully only call map_detach if it is done with all operations using the SG table. Thus it shouldn't matter that the SG table might point to moved pages during execution of this function.

Exactly, it shouldn't matter. This is just a precaution.

When the device driver is buggy I want proper error messages from IOMMU and not accessing pages which might already be reused for something else.

Please add this to the commit message, rather crucial to understand the motivation. With that fixed you can have my

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

And pls push to drm-misc. -Daniel

Regards, Christian.

...

Regards, Lucas

...

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/drm_prime.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index e82a976f0fba..c38dacda6119 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -230,26 +230,26 @@ void drm_gem_map_detach(struct dma_buf *dma_buf, struct drm_prime_attachment *prime_attach = attach->priv; struct drm_gem_object *obj = dma_buf->priv; struct drm_device *dev = obj->dev;

• struct sg_table *sgt;
• if (dev->driver->gem_prime_unpin)

• dev->driver->gem_prime_unpin(obj);
  

• if (prime_attach) {

• struct sg_table *sgt = prime_attach->sgt;
  

• if (!prime_attach)

• return;
  

• sgt = prime_attach->sgt;
• if (sgt) {

• if (prime_attach->dir != DMA_NONE)
  

• 	dma_unmap_sg_attrs(attach->dev, sgt->sgl,
  

sgt->nents,

• 			   prime_attach->dir,
  

• 			   DMA_ATTR_SKIP_CPU_SYNC);
  

• sg_free_table(sgt);
  

• if (sgt) {
  

• 	if (prime_attach->dir != DMA_NONE)
  

• 		dma_unmap_sg_attrs(attach->dev, sgt-
  

...

sgl,

• 				   sgt->nents,
  

• 				   prime_attach-
  

...

dir,

• 				   DMA_ATTR_SKIP_CPU
  

_SYNC);

• 	sg_free_table(sgt);
  

• }
  

• kfree(sgt);
  

• kfree(prime_attach);
  

• attach->priv = NULL;
  

  }

• kfree(sgt);
• kfree(prime_attach);
• attach->priv = NULL;

• if (dev->driver->gem_prime_unpin)

• dev->driver->gem_prime_unpin(obj);
  

  } EXPORT_SYMBOL(drm_gem_map_detach);

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel