On Fri, May 04, 2012 at 02:46:54PM +0100, Dave Airlie wrote:
> From: Dave Airlie <airlied@xxxxxxxxxx>
>
> This adds the ability for ttm common code to take an SG table
> and use it as the backing for a slave TTM object.
I took a look at both patches from the perspective of the TTM DMA pool
code and it looks fine (thought I would prefer to test this first - but
I don't have the hardware).
What I am curious is that you are allowing to map two or more
of SG pages to different PCI devices. And for that you are using
the nouveau_gem_map_dma_buf, which does this (roughly)
ttm->sg = sg_alloc_table().
for_each_sg()
sg->dma_address[i] = dma_map(..)
So I am seeing two potential issues:
1). ttm->sg = sg_alloc() is called on every new other device.
In other words, if you end up with three of these attachment PCI devices
you are going to cause an memory leak of the sg_alloc() and over-write
the ttm->sg every time. But I might be misreading how the import code
works.
2). You are going to put in sg->dma_address() in of different PCI devices.
Meaning - the dma address might have a different value depending on
whether there is an different IOMMU for each of the devices.
On x86 that is usually not the case (the odd ball being the IBM high-end
boxes that have an Calgary-X IOMMU for PCI buses).
What this means is that the DMA address you are programming in
the sub-sequent PCI devices might the DMA address for a previous PCI
device.
But both of these issues are only a problem if the slave cards have
a different PCI device. If they are the same - then I think you are OK
(except the multiple calls to import which would cause a memory leak).
>
> The drivers can then populate their GTT tables using the SG object.
>
> Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx>
> ---
> drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +-
> drivers/gpu/drm/radeon/radeon_object.c | 2 +-
> drivers/gpu/drm/ttm/ttm_bo.c | 14 +++++++++++++-
> drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 2 +-
> include/drm/ttm/ttm_bo_api.h | 9 ++++++++-
> include/drm/ttm/ttm_bo_driver.h | 2 ++
> 6 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 7d15a77..f8aa501 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -121,7 +121,7 @@ nouveau_bo_new(struct drm_device *dev, int size, int align,
>
> ret = ttm_bo_init(&dev_priv->ttm.bdev, &nvbo->bo, size,
> ttm_bo_type_device, &nvbo->placement,
> - align >> PAGE_SHIFT, 0, false, NULL, acc_size,
> + align >> PAGE_SHIFT, 0, false, NULL, acc_size, NULL,
> nouveau_bo_del_ttm);
> if (ret) {
> /* ttm will call nouveau_bo_del_ttm if it fails.. */
> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index df6a4db..1affbc9 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -155,7 +155,7 @@ retry:
> mutex_lock(&rdev->vram_mutex);
> r = ttm_bo_init(&rdev->mman.bdev, &bo->tbo, size, type,
> &bo->placement, page_align, 0, !kernel, NULL,
> - acc_size, &radeon_ttm_bo_destroy);
> + acc_size, NULL, &radeon_ttm_bo_destroy);
> mutex_unlock(&rdev->vram_mutex);
> if (unlikely(r != 0)) {
> if (r != -ERESTARTSYS) {
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 1f5c67c..289e27b 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -343,6 +343,16 @@ static int ttm_bo_add_ttm(struct ttm_buffer_object *bo, bool zero_alloc)
> if (unlikely(bo->ttm == NULL))
> ret = -ENOMEM;
> break;
> + case ttm_bo_type_sg:
> + bo->ttm = bdev->driver->ttm_tt_create(bdev, bo->num_pages << PAGE_SHIFT,
> + page_flags | TTM_PAGE_FLAG_SG,
> + glob->dummy_read_page);
> + if (unlikely(bo->ttm == NULL)) {
> + ret = -ENOMEM;
> + break;
> + }
> + bo->ttm->sg = bo->sg;
> + break;
> default:
> pr_err("Illegal buffer object type\n");
> ret = -EINVAL;
> @@ -1169,6 +1179,7 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
> bool interruptible,
> struct file *persistent_swap_storage,
> size_t acc_size,
> + struct sg_table *sg,
> void (*destroy) (struct ttm_buffer_object *))
> {
> int ret = 0;
> @@ -1223,6 +1234,7 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
> bo->seq_valid = false;
> bo->persistent_swap_storage = persistent_swap_storage;
> bo->acc_size = acc_size;
> + bo->sg = sg;
> atomic_inc(&bo->glob->bo_count);
>
> ret = ttm_bo_check_placement(bo, placement);
> @@ -1312,7 +1324,7 @@ int ttm_bo_create(struct ttm_bo_device *bdev,
>
> ret = ttm_bo_init(bdev, bo, size, type, placement, page_alignment,
> buffer_start, interruptible,
> - persistent_swap_storage, acc_size, NULL);
> + persistent_swap_storage, acc_size, NULL, NULL);
> if (likely(ret == 0))
> *p_bo = bo;
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> index a37abb5..22bf9a2 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> @@ -1567,7 +1567,7 @@ int vmw_dmabuf_init(struct vmw_private *dev_priv,
> ret = ttm_bo_init(bdev, &vmw_bo->base, size,
> ttm_bo_type_device, placement,
> 0, 0, interruptible,
> - NULL, acc_size, bo_free);
> + NULL, acc_size, NULL, bo_free);
> return ret;
> }
>
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 974c8f8..e15f2a8 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -124,11 +124,15 @@ struct ttm_mem_reg {
> *
> * @ttm_bo_type_kernel: These buffers are like ttm_bo_type_device buffers,
> * but they cannot be accessed from user-space. For kernel-only use.
> + *
> + * @ttm_bo_type_sg: Buffer made from dmabuf sg table shared with another
> + * driver.
> */
>
> enum ttm_bo_type {
> ttm_bo_type_device,
> - ttm_bo_type_kernel
> + ttm_bo_type_kernel,
> + ttm_bo_type_sg
> };
>
> struct ttm_tt;
> @@ -271,6 +275,8 @@ struct ttm_buffer_object {
>
> unsigned long offset;
> uint32_t cur_placement;
> +
> + struct sg_table *sg;
> };
>
> /**
> @@ -503,6 +509,7 @@ extern int ttm_bo_init(struct ttm_bo_device *bdev,
> bool interrubtible,
> struct file *persistent_swap_storage,
> size_t acc_size,
> + struct sg_table *sg,
> void (*destroy) (struct ttm_buffer_object *));
>
> /**
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index d43e892..a05f1b5 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -81,6 +81,7 @@ struct ttm_backend_func {
> #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)
>
> enum ttm_caching_state {
> tt_uncached,
> @@ -116,6 +117,7 @@ struct ttm_tt {
> struct page **pages;
> uint32_t page_flags;
> unsigned long num_pages;
> + struct sg_table *sg; /* for SG objects via dma-buf */
> struct ttm_bo_global *glob;
> struct ttm_backend *be;
> struct file *swap_storage;
> --
> 1.7.6
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
