[PATCH 1/5] drm/ttm: add ttm_tt_populate wrapper

[PATCH 1/5] drm/ttm: add ttm_tt_populate wrapper

[Christian König]

Stop calling the driver callback directly.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 12 +++++-------
 drivers/gpu/drm/ttm/ttm_bo_vm.c   |  2 +-
 drivers/gpu/drm/ttm/ttm_tt.c      | 10 +++++++++-
 include/drm/ttm/ttm_bo_driver.h   |  9 +++++++++
 4 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 33ffe286f3a5..38da6903cae9 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -375,8 +375,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
 	/*
 	 * TTM might be null for moves within the same region.
 	 */
-	if (ttm && ttm->state == tt_unpopulated) {
-		ret = ttm->bdev->driver->ttm_tt_populate(ttm, ctx);
+	if (ttm) {
+		ret = ttm_tt_populate(ttm, ctx);
 		if (ret)
 			goto out1;
 	}
@@ -557,11 +557,9 @@ static int ttm_bo_kmap_ttm(struct ttm_buffer_object *bo,
 
 	BUG_ON(!ttm);
 
-	if (ttm->state == tt_unpopulated) {
-		ret = ttm->bdev->driver->ttm_tt_populate(ttm, &ctx);
-		if (ret)
-			return ret;
-	}
+	ret = ttm_tt_populate(ttm, &ctx);
+	if (ret)
+		return ret;
 
 	if (num_pages == 1 && (mem->placement & TTM_PL_FLAG_CACHED)) {
 		/*
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 716e724ac710..610d6714042a 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -234,7 +234,7 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)
 						cvma.vm_page_prot);
 
 		/* Allocate all page at once, most common usage */
-		if (ttm->bdev->driver->ttm_tt_populate(ttm, &ctx)) {
+		if (ttm_tt_populate(ttm, &ctx)) {
 			ret = VM_FAULT_OOM;
 			goto out_io_unlock;
 		}
diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
index 95a77dab8cc9..39c44e301c72 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -276,7 +276,7 @@ int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem,
 	if (ttm->state == tt_bound)
 		return 0;
 
-	ret = ttm->bdev->driver->ttm_tt_populate(ttm, ctx);
+	ret = ttm_tt_populate(ttm, ctx);
 	if (ret)
 		return ret;
 
@@ -392,6 +392,14 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage)
 	return ret;
 }
 
+int ttm_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
+{
+	if (ttm->state != tt_unpopulated)
+		return 0;
+
+	return ttm->bdev->driver->ttm_tt_populate(ttm, ctx);
+}
+
 static void ttm_tt_clear_mapping(struct ttm_tt *ttm)
 {
 	pgoff_t i;
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 9b417eb2df20..2bac25a6cf90 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -700,6 +700,15 @@ int ttm_tt_swapin(struct ttm_tt *ttm);
 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
  *
-- 
2.14.1

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

[PATCH 2/5] drm/amdgpu: remove extra TT unpopulated check

[Christian König]

The subsystem chould check that, not the driver.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 95f990140f2a..648c449aaa79 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -997,9 +997,6 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm,
 	struct amdgpu_ttm_tt *gtt = (void *)ttm;
 	bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
 
-	if (ttm->state != tt_unpopulated)
-		return 0;
-
 	if (gtt && gtt->userptr) {
 		ttm->sg = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
 		if (!ttm->sg)
-- 
2.14.1

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

[PATCH 3/5] drm/radeon: remove extra TT unpopulated check

[Christian König]

The subsystem chould check that, not the driver.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index a0a839bc39bf..42e3ee81a96e 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -728,9 +728,6 @@ static int radeon_ttm_tt_populate(struct ttm_tt *ttm,
 	struct radeon_device *rdev;
 	bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
 
-	if (ttm->state != tt_unpopulated)
-		return 0;
-
 	if (gtt && gtt->userptr) {
 		ttm->sg = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
 		if (!ttm->sg)
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/5] drm/ttm: set page mapping during allocation

[Christian König]

To aid debugging set the page mapping during allocation instead of
during VM faults.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo_vm.c |  1 -
 drivers/gpu/drm/ttm/ttm_tt.c    | 18 +++++++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 610d6714042a..121f017ac7ca 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -257,7 +257,6 @@ static int ttm_bo_vm_fault(struct vm_fault *vmf)
 			} else if (unlikely(!page)) {
 				break;
 			}
-			page->mapping = vma->vm_file->f_mapping;
 			page->index = drm_vma_node_start(&bo->vma_node) +
 				page_offset;
 			pfn = page_to_pfn(page);
diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
index 39c44e301c72..9fd7115a013a 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -392,12 +392,28 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage)
 	return ret;
 }
 
+static void ttm_tt_add_mapping(struct ttm_tt *ttm)
+{
+	pgoff_t i;
+
+	if (ttm->page_flags & TTM_PAGE_FLAG_SG)
+		return;
+
+	for (i = 0; i < ttm->num_pages; ++i)
+		ttm->pages[i]->mapping = ttm->bdev->dev_mapping;
+}
+
 int ttm_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
 {
+	int ret;
+
 	if (ttm->state != tt_unpopulated)
 		return 0;
 
-	return ttm->bdev->driver->ttm_tt_populate(ttm, ctx);
+	ret = ttm->bdev->driver->ttm_tt_populate(ttm, ctx);
+	if (!ret)
+		ttm_tt_add_mapping(ttm);
+	return ret;
 }
 
 static void ttm_tt_clear_mapping(struct ttm_tt *ttm)
-- 
2.14.1

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

[PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Christian König]

This allows access to pages allocated through the driver with optional
IOMMU mapping.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 57 ++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 648c449aaa79..795ceaeb82d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1929,38 +1929,51 @@ static const struct file_operations amdgpu_ttm_gtt_fops = {
 
 #endif
 
-static ssize_t amdgpu_iova_to_phys_read(struct file *f, char __user *buf,
-				   size_t size, loff_t *pos)
+static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
+				 size_t size, loff_t *pos)
 {
 	struct amdgpu_device *adev = file_inode(f)->i_private;
-	int r;
-	uint64_t phys;
 	struct iommu_domain *dom;
+	ssize_t result = 0;
+	int r;
 
-	// always return 8 bytes
-	if (size != 8)
-		return -EINVAL;
+	dom = iommu_get_domain_for_dev(adev->dev);
 
-	// only accept page addresses
-	if (*pos & 0xFFF)
-		return -EINVAL;
+	while (size) {
+		phys_addr_t addr = *pos & PAGE_MASK;
+		loff_t off = *pos & ~PAGE_MASK;
+		size_t bytes = PAGE_SIZE - off;
+		unsigned long pfn;
+		struct page *p;
+		void *ptr;
 
-	dom = iommu_get_domain_for_dev(adev->dev);
-	if (dom)
-		phys = iommu_iova_to_phys(dom, *pos);
-	else
-		phys = *pos;
+		addr = dom ? iommu_iova_to_phys(dom, addr) : addr;
 
-	r = copy_to_user(buf, &phys, 8);
-	if (r)
-		return -EFAULT;
+		pfn = addr >> PAGE_SHIFT;
+		if (!pfn_valid(pfn))
+			return -EPERM;
+
+		p = pfn_to_page(pfn);
+		if (p->mapping != adev->mman.bdev.dev_mapping)
+			return -EPERM;
+
+		ptr = kmap(p);
+		r = copy_to_user(buf, ptr, bytes);
+		kunmap(p);
+		if (r)
+			return -EFAULT;
 
-	return 8;
+		size -= bytes;
+		*pos += bytes;
+		result += bytes;
+	}
+
+	return result;
 }
 
-static const struct file_operations amdgpu_ttm_iova_fops = {
+static const struct file_operations amdgpu_ttm_iomem_fops = {
 	.owner = THIS_MODULE,
-	.read = amdgpu_iova_to_phys_read,
+	.read = amdgpu_iomem_read,
 	.llseek = default_llseek
 };
 
@@ -1973,7 +1986,7 @@ static const struct {
 #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
 	{ "amdgpu_gtt", &amdgpu_ttm_gtt_fops, TTM_PL_TT },
 #endif
-	{ "amdgpu_iova", &amdgpu_ttm_iova_fops, TTM_PL_SYSTEM },
+	{ "amdgpu_iomem", &amdgpu_ttm_iomem_fops, TTM_PL_SYSTEM },
 };
 
 #endif
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[StDenis, Tom]

I haven't tried the patch but just like to point out this breaks umr :-)  I'll have to craft something on Monday to support this and iova in parallel until the iova kernels are realistically EOL'ed.

On the other hand I support this idea since it eliminates the need for an fmem hack.  So much appreciated.

Cheers,
Tom

________________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Friday, February 2, 2018 14:09
To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

This allows access to pages allocated through the driver with optional
IOMMU mapping.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 57 ++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 648c449aaa79..795ceaeb82d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1929,38 +1929,51 @@ static const struct file_operations amdgpu_ttm_gtt_fops = {

 #endif

-static ssize_t amdgpu_iova_to_phys_read(struct file *f, char __user *buf,
-                                  size_t size, loff_t *pos)
+static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
+                                size_t size, loff_t *pos)
 {
        struct amdgpu_device *adev = file_inode(f)->i_private;
-       int r;
-       uint64_t phys;
        struct iommu_domain *dom;
+       ssize_t result = 0;
+       int r;

-       // always return 8 bytes
-       if (size != 8)
-               return -EINVAL;
+       dom = iommu_get_domain_for_dev(adev->dev);

-       // only accept page addresses
-       if (*pos & 0xFFF)
-               return -EINVAL;
+       while (size) {
+               phys_addr_t addr = *pos & PAGE_MASK;
+               loff_t off = *pos & ~PAGE_MASK;
+               size_t bytes = PAGE_SIZE - off;
+               unsigned long pfn;
+               struct page *p;
+               void *ptr;

-       dom = iommu_get_domain_for_dev(adev->dev);
-       if (dom)
-               phys = iommu_iova_to_phys(dom, *pos);
-       else
-               phys = *pos;
+               addr = dom ? iommu_iova_to_phys(dom, addr) : addr;

-       r = copy_to_user(buf, &phys, 8);
-       if (r)
-               return -EFAULT;
+               pfn = addr >> PAGE_SHIFT;
+               if (!pfn_valid(pfn))
+                       return -EPERM;
+
+               p = pfn_to_page(pfn);
+               if (p->mapping != adev->mman.bdev.dev_mapping)
+                       return -EPERM;
+
+               ptr = kmap(p);
+               r = copy_to_user(buf, ptr, bytes);
+               kunmap(p);
+               if (r)
+                       return -EFAULT;

-       return 8;
+               size -= bytes;
+               *pos += bytes;
+               result += bytes;
+       }
+
+       return result;
 }

-static const struct file_operations amdgpu_ttm_iova_fops = {
+static const struct file_operations amdgpu_ttm_iomem_fops = {
        .owner = THIS_MODULE,
-       .read = amdgpu_iova_to_phys_read,
+       .read = amdgpu_iomem_read,
        .llseek = default_llseek
 };

@@ -1973,7 +1986,7 @@ static const struct {
 #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
        { "amdgpu_gtt", &amdgpu_ttm_gtt_fops, TTM_PL_TT },
 #endif
-       { "amdgpu_iova", &amdgpu_ttm_iova_fops, TTM_PL_SYSTEM },
+       { "amdgpu_iomem", &amdgpu_ttm_iomem_fops, TTM_PL_SYSTEM },
 };

 #endif
--
2.14.1

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

RE: [PATCH 3/5] drm/radeon: remove extra TT unpopulated check

[He, Roger]

-----Original Message-----
From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Christian K?nig
Sent: Saturday, February 03, 2018 3:10 AM
To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 3/5] drm/radeon: remove extra TT unpopulated check

The subsystem chould check that, not the driver.

Commit log typo, should be "should" rather than " chould".
With that fix,  this patch is Reviewed-by: Roger He <Hongbo.He@amd.com>

Thanks
Roger(Hongbo.He)

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index a0a839bc39bf..42e3ee81a96e 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -728,9 +728,6 @@ static int radeon_ttm_tt_populate(struct ttm_tt *ttm,
 	struct radeon_device *rdev;
 	bool slave = !!(ttm->page_flags & TTM_PAGE_FLAG_SG);
 
-	if (ttm->state != tt_unpopulated)
-		return 0;
-
 	if (gtt && gtt->userptr) {
 		ttm->sg = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
 		if (!ttm->sg)
-- 
2.14.1

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

RE: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[He, Roger]

Patch1 & 2 & 4,   Reviewed-by: Roger He <Hongbo.He@amd.com>
Patch 5:  Acked-by: Roger He <Hongbo.He@amd.com>

-----Original Message-----
From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Christian K?nig
Sent: Saturday, February 03, 2018 3:10 AM
To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Subject: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

This allows access to pages allocated through the driver with optional IOMMU mapping.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 57 ++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 648c449aaa79..795ceaeb82d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1929,38 +1929,51 @@ static const struct file_operations amdgpu_ttm_gtt_fops = {
 
 #endif
 
-static ssize_t amdgpu_iova_to_phys_read(struct file *f, char __user *buf,
-				   size_t size, loff_t *pos)
+static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
+				 size_t size, loff_t *pos)
 {
 	struct amdgpu_device *adev = file_inode(f)->i_private;
-	int r;
-	uint64_t phys;
 	struct iommu_domain *dom;
+	ssize_t result = 0;
+	int r;
 
-	// always return 8 bytes
-	if (size != 8)
-		return -EINVAL;
+	dom = iommu_get_domain_for_dev(adev->dev);
 
-	// only accept page addresses
-	if (*pos & 0xFFF)
-		return -EINVAL;
+	while (size) {
+		phys_addr_t addr = *pos & PAGE_MASK;
+		loff_t off = *pos & ~PAGE_MASK;
+		size_t bytes = PAGE_SIZE - off;
+		unsigned long pfn;
+		struct page *p;
+		void *ptr;
 
-	dom = iommu_get_domain_for_dev(adev->dev);
-	if (dom)
-		phys = iommu_iova_to_phys(dom, *pos);
-	else
-		phys = *pos;
+		addr = dom ? iommu_iova_to_phys(dom, addr) : addr;
 
-	r = copy_to_user(buf, &phys, 8);
-	if (r)
-		return -EFAULT;
+		pfn = addr >> PAGE_SHIFT;
+		if (!pfn_valid(pfn))
+			return -EPERM;
+
+		p = pfn_to_page(pfn);
+		if (p->mapping != adev->mman.bdev.dev_mapping)
+			return -EPERM;
+
+		ptr = kmap(p);
+		r = copy_to_user(buf, ptr, bytes);
+		kunmap(p);
+		if (r)
+			return -EFAULT;
 
-	return 8;
+		size -= bytes;
+		*pos += bytes;
+		result += bytes;
+	}
+
+	return result;
 }
 
-static const struct file_operations amdgpu_ttm_iova_fops = {
+static const struct file_operations amdgpu_ttm_iomem_fops = {
 	.owner = THIS_MODULE,
-	.read = amdgpu_iova_to_phys_read,
+	.read = amdgpu_iomem_read,
 	.llseek = default_llseek
 };
 
@@ -1973,7 +1986,7 @@ static const struct {  #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
 	{ "amdgpu_gtt", &amdgpu_ttm_gtt_fops, TTM_PL_TT },  #endif
-	{ "amdgpu_iova", &amdgpu_ttm_iova_fops, TTM_PL_SYSTEM },
+	{ "amdgpu_iomem", &amdgpu_ttm_iomem_fops, TTM_PL_SYSTEM },
 };
 
 #endif
--
2.14.1

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

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Tom St Denis]

Another thing that occurred to me is this will break write access to GPU 
bound memory.  Previously we relied on iova to translate the address and 
then /dev/mem or /dev/fmem to read/write it.  But since this is 
literally a read only method obviously there's no write support.

Tom


On 04/02/18 09:56 PM, He, Roger wrote:
> Patch1 & 2 & 4,   Reviewed-by: Roger He <Hongbo.He@amd.com>
> Patch 5:  Acked-by: Roger He <Hongbo.He@amd.com>
> 
> -----Original Message-----
> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Christian K?nig
> Sent: Saturday, February 03, 2018 3:10 AM
> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem
> 
> This allows access to pages allocated through the driver with optional IOMMU mapping.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 57 ++++++++++++++++++++-------------
>   1 file changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 648c449aaa79..795ceaeb82d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1929,38 +1929,51 @@ static const struct file_operations amdgpu_ttm_gtt_fops = {
>   
>   #endif
>   
> -static ssize_t amdgpu_iova_to_phys_read(struct file *f, char __user *buf,
> -				   size_t size, loff_t *pos)
> +static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
> +				 size_t size, loff_t *pos)
>   {
>   	struct amdgpu_device *adev = file_inode(f)->i_private;
> -	int r;
> -	uint64_t phys;
>   	struct iommu_domain *dom;
> +	ssize_t result = 0;
> +	int r;
>   
> -	// always return 8 bytes
> -	if (size != 8)
> -		return -EINVAL;
> +	dom = iommu_get_domain_for_dev(adev->dev);
>   
> -	// only accept page addresses
> -	if (*pos & 0xFFF)
> -		return -EINVAL;
> +	while (size) {
> +		phys_addr_t addr = *pos & PAGE_MASK;
> +		loff_t off = *pos & ~PAGE_MASK;
> +		size_t bytes = PAGE_SIZE - off;
> +		unsigned long pfn;
> +		struct page *p;
> +		void *ptr;
>   
> -	dom = iommu_get_domain_for_dev(adev->dev);
> -	if (dom)
> -		phys = iommu_iova_to_phys(dom, *pos);
> -	else
> -		phys = *pos;
> +		addr = dom ? iommu_iova_to_phys(dom, addr) : addr;
>   
> -	r = copy_to_user(buf, &phys, 8);
> -	if (r)
> -		return -EFAULT;
> +		pfn = addr >> PAGE_SHIFT;
> +		if (!pfn_valid(pfn))
> +			return -EPERM;
> +
> +		p = pfn_to_page(pfn);
> +		if (p->mapping != adev->mman.bdev.dev_mapping)
> +			return -EPERM;
> +
> +		ptr = kmap(p);
> +		r = copy_to_user(buf, ptr, bytes);
> +		kunmap(p);
> +		if (r)
> +			return -EFAULT;
>   
> -	return 8;
> +		size -= bytes;
> +		*pos += bytes;
> +		result += bytes;
> +	}
> +
> +	return result;
>   }
>   
> -static const struct file_operations amdgpu_ttm_iova_fops = {
> +static const struct file_operations amdgpu_ttm_iomem_fops = {
>   	.owner = THIS_MODULE,
> -	.read = amdgpu_iova_to_phys_read,
> +	.read = amdgpu_iomem_read,
>   	.llseek = default_llseek
>   };
>   
> @@ -1973,7 +1986,7 @@ static const struct {  #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
>   	{ "amdgpu_gtt", &amdgpu_ttm_gtt_fops, TTM_PL_TT },  #endif
> -	{ "amdgpu_iova", &amdgpu_ttm_iova_fops, TTM_PL_SYSTEM },
> +	{ "amdgpu_iomem", &amdgpu_ttm_iomem_fops, TTM_PL_SYSTEM },
>   };
>   
>   #endif
> --
> 2.14.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Christian König]

Well adding write support is trivial.

What I'm more concerned about is if setting page->mapping during 
allocation of the page could have any negative effect?

I of hand don't see any since the page isn't reclaimable directly 
anyway, but I'm not 100% sure of that.

Christian.

Am 05.02.2018 um 12:49 schrieb Tom St Denis:
> Another thing that occurred to me is this will break write access to 
> GPU bound memory.  Previously we relied on iova to translate the 
> address and then /dev/mem or /dev/fmem to read/write it.  But since 
> this is literally a read only method obviously there's no write support.
>
> Tom
>
>
> On 04/02/18 09:56 PM, He, Roger wrote:
>> Patch1 & 2 & 4,   Reviewed-by: Roger He <Hongbo.He@amd.com>
>> Patch 5:  Acked-by: Roger He <Hongbo.He@amd.com>
>>
>> -----Original Message-----
>> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On 
>> Behalf Of Christian K?nig
>> Sent: Saturday, February 03, 2018 3:10 AM
>> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
>> Subject: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem
>>
>> This allows access to pages allocated through the driver with 
>> optional IOMMU mapping.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 57 
>> ++++++++++++++++++++-------------
>>   1 file changed, 35 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 648c449aaa79..795ceaeb82d5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1929,38 +1929,51 @@ static const struct file_operations 
>> amdgpu_ttm_gtt_fops = {
>>     #endif
>>   -static ssize_t amdgpu_iova_to_phys_read(struct file *f, char 
>> __user *buf,
>> -                   size_t size, loff_t *pos)
>> +static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
>> +                 size_t size, loff_t *pos)
>>   {
>>       struct amdgpu_device *adev = file_inode(f)->i_private;
>> -    int r;
>> -    uint64_t phys;
>>       struct iommu_domain *dom;
>> +    ssize_t result = 0;
>> +    int r;
>>   -    // always return 8 bytes
>> -    if (size != 8)
>> -        return -EINVAL;
>> +    dom = iommu_get_domain_for_dev(adev->dev);
>>   -    // only accept page addresses
>> -    if (*pos & 0xFFF)
>> -        return -EINVAL;
>> +    while (size) {
>> +        phys_addr_t addr = *pos & PAGE_MASK;
>> +        loff_t off = *pos & ~PAGE_MASK;
>> +        size_t bytes = PAGE_SIZE - off;
>> +        unsigned long pfn;
>> +        struct page *p;
>> +        void *ptr;
>>   -    dom = iommu_get_domain_for_dev(adev->dev);
>> -    if (dom)
>> -        phys = iommu_iova_to_phys(dom, *pos);
>> -    else
>> -        phys = *pos;
>> +        addr = dom ? iommu_iova_to_phys(dom, addr) : addr;
>>   -    r = copy_to_user(buf, &phys, 8);
>> -    if (r)
>> -        return -EFAULT;
>> +        pfn = addr >> PAGE_SHIFT;
>> +        if (!pfn_valid(pfn))
>> +            return -EPERM;
>> +
>> +        p = pfn_to_page(pfn);
>> +        if (p->mapping != adev->mman.bdev.dev_mapping)
>> +            return -EPERM;
>> +
>> +        ptr = kmap(p);
>> +        r = copy_to_user(buf, ptr, bytes);
>> +        kunmap(p);
>> +        if (r)
>> +            return -EFAULT;
>>   -    return 8;
>> +        size -= bytes;
>> +        *pos += bytes;
>> +        result += bytes;
>> +    }
>> +
>> +    return result;
>>   }
>>   -static const struct file_operations amdgpu_ttm_iova_fops = {
>> +static const struct file_operations amdgpu_ttm_iomem_fops = {
>>       .owner = THIS_MODULE,
>> -    .read = amdgpu_iova_to_phys_read,
>> +    .read = amdgpu_iomem_read,
>>       .llseek = default_llseek
>>   };
>>   @@ -1973,7 +1986,7 @@ static const struct {  #ifdef 
>> CONFIG_DRM_AMDGPU_GART_DEBUGFS
>>       { "amdgpu_gtt", &amdgpu_ttm_gtt_fops, TTM_PL_TT }, #endif
>> -    { "amdgpu_iova", &amdgpu_ttm_iova_fops, TTM_PL_SYSTEM },
>> +    { "amdgpu_iomem", &amdgpu_ttm_iomem_fops, TTM_PL_SYSTEM },
>>   };
>>     #endif
>> -- 
>> 2.14.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Tom St Denis]

[-- Attachment #1: Type: text/plain, Size: 5412 bytes --]

Attached is a patch for umr{master} which should in theory support both 
iova and iomem.

I can add the write method if you want since ya it should be fairly 
simple to copy/pasta that up.

Cheers,
Tom

On 05/02/18 07:07 AM, Christian König wrote:
> Well adding write support is trivial.
> 
> What I'm more concerned about is if setting page->mapping during 
> allocation of the page could have any negative effect?
> 
> I of hand don't see any since the page isn't reclaimable directly 
> anyway, but I'm not 100% sure of that.
> 
> Christian.
> 
> Am 05.02.2018 um 12:49 schrieb Tom St Denis:
>> Another thing that occurred to me is this will break write access to 
>> GPU bound memory.  Previously we relied on iova to translate the 
>> address and then /dev/mem or /dev/fmem to read/write it.  But since 
>> this is literally a read only method obviously there's no write support.
>>
>> Tom
>>
>>
>> On 04/02/18 09:56 PM, He, Roger wrote:
>>> Patch1 & 2 & 4,   Reviewed-by: Roger He <Hongbo.He-5C7GfCeVMHo@public.gmane.org>
>>> Patch 5:  Acked-by: Roger He <Hongbo.He-5C7GfCeVMHo@public.gmane.org>
>>>
>>> -----Original Message-----
>>> From: dri-devel [mailto:dri-devel-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] On 
>>> Behalf Of Christian K?nig
>>> Sent: Saturday, February 03, 2018 3:10 AM
>>> To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>> Subject: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem
>>>
>>> This allows access to pages allocated through the driver with 
>>> optional IOMMU mapping.
>>>
>>> Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 57 
>>> ++++++++++++++++++++-------------
>>>   1 file changed, 35 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 648c449aaa79..795ceaeb82d5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -1929,38 +1929,51 @@ static const struct file_operations 
>>> amdgpu_ttm_gtt_fops = {
>>>     #endif
>>>   -static ssize_t amdgpu_iova_to_phys_read(struct file *f, char 
>>> __user *buf,
>>> -                   size_t size, loff_t *pos)
>>> +static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
>>> +                 size_t size, loff_t *pos)
>>>   {
>>>       struct amdgpu_device *adev = file_inode(f)->i_private;
>>> -    int r;
>>> -    uint64_t phys;
>>>       struct iommu_domain *dom;
>>> +    ssize_t result = 0;
>>> +    int r;
>>>   -    // always return 8 bytes
>>> -    if (size != 8)
>>> -        return -EINVAL;
>>> +    dom = iommu_get_domain_for_dev(adev->dev);
>>>   -    // only accept page addresses
>>> -    if (*pos & 0xFFF)
>>> -        return -EINVAL;
>>> +    while (size) {
>>> +        phys_addr_t addr = *pos & PAGE_MASK;
>>> +        loff_t off = *pos & ~PAGE_MASK;
>>> +        size_t bytes = PAGE_SIZE - off;
>>> +        unsigned long pfn;
>>> +        struct page *p;
>>> +        void *ptr;
>>>   -    dom = iommu_get_domain_for_dev(adev->dev);
>>> -    if (dom)
>>> -        phys = iommu_iova_to_phys(dom, *pos);
>>> -    else
>>> -        phys = *pos;
>>> +        addr = dom ? iommu_iova_to_phys(dom, addr) : addr;
>>>   -    r = copy_to_user(buf, &phys, 8);
>>> -    if (r)
>>> -        return -EFAULT;
>>> +        pfn = addr >> PAGE_SHIFT;
>>> +        if (!pfn_valid(pfn))
>>> +            return -EPERM;
>>> +
>>> +        p = pfn_to_page(pfn);
>>> +        if (p->mapping != adev->mman.bdev.dev_mapping)
>>> +            return -EPERM;
>>> +
>>> +        ptr = kmap(p);
>>> +        r = copy_to_user(buf, ptr, bytes);
>>> +        kunmap(p);
>>> +        if (r)
>>> +            return -EFAULT;
>>>   -    return 8;
>>> +        size -= bytes;
>>> +        *pos += bytes;
>>> +        result += bytes;
>>> +    }
>>> +
>>> +    return result;
>>>   }
>>>   -static const struct file_operations amdgpu_ttm_iova_fops = {
>>> +static const struct file_operations amdgpu_ttm_iomem_fops = {
>>>       .owner = THIS_MODULE,
>>> -    .read = amdgpu_iova_to_phys_read,
>>> +    .read = amdgpu_iomem_read,
>>>       .llseek = default_llseek
>>>   };
>>>   @@ -1973,7 +1986,7 @@ static const struct {  #ifdef 
>>> CONFIG_DRM_AMDGPU_GART_DEBUGFS
>>>       { "amdgpu_gtt", &amdgpu_ttm_gtt_fops, TTM_PL_TT }, #endif
>>> -    { "amdgpu_iova", &amdgpu_ttm_iova_fops, TTM_PL_SYSTEM },
>>> +    { "amdgpu_iomem", &amdgpu_ttm_iomem_fops, TTM_PL_SYSTEM },
>>>   };
>>>     #endif
>>> -- 
>>> 2.14.1
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>
> 


[-- Attachment #2: 0001-add-support-for-new-iomem-debugfs-entry.patch --]
[-- Type: text/x-patch, Size: 4335 bytes --]

>From 67703a62763dfb2107bd503c5ae76414a50c50a4 Mon Sep 17 00:00:00 2001
From: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org>
Date: Mon, 5 Feb 2018 08:53:40 -0500
Subject: [PATCH umr] add support for new iomem debugfs entry

Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org>
---
 src/lib/close_asic.c |  1 +
 src/lib/discover.c   |  3 +++
 src/lib/read_vram.c  | 29 +++++++++++++++++++----------
 src/umr.h            |  3 ++-
 4 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/lib/close_asic.c b/src/lib/close_asic.c
index 782b1a0d029b..6b220cd578e9 100644
--- a/src/lib/close_asic.c
+++ b/src/lib/close_asic.c
@@ -57,6 +57,7 @@ void umr_close_asic(struct umr_asic *asic)
 		cond_close(asic->fd.gpr);
 		cond_close(asic->fd.drm);
 		cond_close(asic->fd.iova);
+		cond_close(asic->fd.iomem);
 		umr_free_asic(asic);
 	}
 }
diff --git a/src/lib/discover.c b/src/lib/discover.c
index 4af3733c8af8..dedcedc776ab 100644
--- a/src/lib/discover.c
+++ b/src/lib/discover.c
@@ -232,6 +232,8 @@ struct umr_asic *umr_discover_asic(struct umr_options *options)
 			asic->fd.gpr = open(fname, O_RDWR);
 			snprintf(fname, sizeof(fname)-1, "/sys/kernel/debug/dri/%d/amdgpu_iova", asic->instance);
 			asic->fd.iova = open(fname, O_RDWR);
+			snprintf(fname, sizeof(fname)-1, "/sys/kernel/debug/dri/%d/amdgpu_iomem", asic->instance);
+			asic->fd.iomem = open(fname, O_RDWR);
 			asic->fd.drm = -1; // default to closed
 			// if appending to the fd list remember to update close_asic() and discover_by_did()...
 		} else {
@@ -246,6 +248,7 @@ struct umr_asic *umr_discover_asic(struct umr_options *options)
 			asic->fd.gpr = -1;
 			asic->fd.drm = -1;
 			asic->fd.iova = -1;
+			asic->fd.iomem = -1;
 		}
 
 		if (options->use_pci) {
diff --git a/src/lib/read_vram.c b/src/lib/read_vram.c
index 25ffec93f54d..c685955e5050 100644
--- a/src/lib/read_vram.c
+++ b/src/lib/read_vram.c
@@ -73,30 +73,39 @@ static void access_vram_via_mmio(struct umr_asic *asic, uint64_t address, uint32
 #define DEBUG(...)
 #endif
 
-static int umr_access_sram(uint64_t address, uint32_t size, void *dst, int write_en)
+static int umr_access_sram(struct umr_asic *asic, uint64_t address, uint32_t size, void *dst, int write_en)
 {
-	int fd;
+	int fd, need_close=0;
 
 	DEBUG("Reading physical sys addr: 0x%llx\n", (unsigned long long)address);
 
-	fd = open("/dev/fmem", O_RDWR);
-	if (fd < 0)
-		fd = open("/dev/mem", O_RDWR | O_DSYNC);
+	if (asic->fd.iomem >= 0) {
+		fd = asic->fd.iomem;
+	} else {
+		need_close = 1;
+
+		fd = open("/dev/fmem", O_RDWR);
+		if (fd < 0)
+			fd = open("/dev/mem", O_RDWR | O_DSYNC);
+	}
 	if (fd >= 0) {
 		lseek(fd, address, SEEK_SET);
 		if (write_en == 0) {
 			memset(dst, 0xFF, size);
 			if (read(fd, dst, size) != size) {
-				close(fd);
+				if (need_close)
+					close(fd);
 				return -1;
 			}
 		} else {
 			if (write(fd, dst, size) != size) {
-				close(fd);
+				if (need_close)
+					close(fd);
 				return -1;
 			}
 		}
-		close(fd);
+		if (need_close)
+			close(fd);
 		return 0;
 	}
 	return -1;
@@ -292,7 +301,7 @@ next_page:
 		// allow destination to be NULL to simply use decoder
 		if (pdst) {
 			if (pte_fields.system) {
-				if (umr_access_sram(start_addr, chunk_size, pdst, write_en) < 0) {
+				if (umr_access_sram(asic, start_addr, chunk_size, pdst, write_en) < 0) {
 					fprintf(stderr, "[ERROR]: Cannot access system ram, perhaps CONFIG_STRICT_DEVMEM is set in your kernel config?\n");
 					fprintf(stderr, "[ERROR]: Alternatively download and install /dev/fmem\n");
 					return -1;
@@ -663,7 +672,7 @@ next_page:
 		if (pte_fields.valid) {
 			if (pdst) {
 				if (pte_fields.system) {
-					if (umr_access_sram(start_addr, chunk_size, pdst, write_en) < 0) {
+					if (umr_access_sram(asic, start_addr, chunk_size, pdst, write_en) < 0) {
 						fprintf(stderr, "[ERROR]: Cannot access system ram, perhaps CONFIG_STRICT_DEVMEM is set in your kernel config?\n");
 						fprintf(stderr, "[ERROR]: Alternatively download and install /dev/fmem\n");
 						return -1;
diff --git a/src/umr.h b/src/umr.h
index 9c006f00cf45..da67abf6dc2b 100644
--- a/src/umr.h
+++ b/src/umr.h
@@ -233,7 +233,8 @@ struct umr_asic {
 		    wave,
 		    vram,
 		    gpr,
-		    iova;
+		    iova,
+		    iomem;
 	} fd;
 	struct {
 		struct pci_device *pdevice;
-- 
2.14.3


[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Tom St Denis]

On 02/02/18 02:09 PM, Christian König wrote:
> This allows access to pages allocated through the driver with optional
> IOMMU mapping.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 57 ++++++++++++++++++++-------------
>   1 file changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 648c449aaa79..795ceaeb82d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1929,38 +1929,51 @@ static const struct file_operations amdgpu_ttm_gtt_fops = {
>   
>   #endif
>   
> -static ssize_t amdgpu_iova_to_phys_read(struct file *f, char __user *buf,
> -				   size_t size, loff_t *pos)
> +static ssize_t amdgpu_iomem_read(struct file *f, char __user *buf,
> +				 size_t size, loff_t *pos)
>   {
>   	struct amdgpu_device *adev = file_inode(f)->i_private;
> -	int r;
> -	uint64_t phys;
>   	struct iommu_domain *dom;
> +	ssize_t result = 0;
> +	int r;
>   
> -	// always return 8 bytes
> -	if (size != 8)
> -		return -EINVAL;
> +	dom = iommu_get_domain_for_dev(adev->dev);
>   
> -	// only accept page addresses
> -	if (*pos & 0xFFF)
> -		return -EINVAL;
> +	while (size) {
> +		phys_addr_t addr = *pos & PAGE_MASK;
> +		loff_t off = *pos & ~PAGE_MASK;
> +		size_t bytes = PAGE_SIZE - off;
> +		unsigned long pfn;
> +		struct page *p;
> +		void *ptr;
>   
> -	dom = iommu_get_domain_for_dev(adev->dev);
> -	if (dom)
> -		phys = iommu_iova_to_phys(dom, *pos);
> -	else
> -		phys = *pos;
> +		addr = dom ? iommu_iova_to_phys(dom, addr) : addr;
>   
> -	r = copy_to_user(buf, &phys, 8);
> -	if (r)
> -		return -EFAULT;
> +		pfn = addr >> PAGE_SHIFT;
> +		if (!pfn_valid(pfn))
> +			return -EPERM;
> +
> +		p = pfn_to_page(pfn);
> +		if (p->mapping != adev->mman.bdev.dev_mapping)
> +			return -EPERM;

This comparison fails for both IOMMU and non-IOMMU devices in my 
carrizo+polaris10 box.

The address being read from is what the VM decodes to (checked with strace).

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

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Christian König]

Am 09.02.2018 um 14:32 schrieb Tom St Denis:
> On 02/02/18 02:09 PM, Christian König wrote:
>> [SNIP]
>> +        if (p->mapping != adev->mman.bdev.dev_mapping)
>> +            return -EPERM;
>
> This comparison fails for both IOMMU and non-IOMMU devices in my 
> carrizo+polaris10 box.
>
> The address being read from is what the VM decodes to (checked with 
> strace).

Have you applied the whole series? That patches before this one are 
necessary to initialize p->mapping when there isn't any userspace 
mapping for the page.

Christian.

>
> Tom

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Tom St Denis]

On 09/02/18 08:56 AM, Christian König wrote:
> Am 09.02.2018 um 14:32 schrieb Tom St Denis:
>> On 02/02/18 02:09 PM, Christian König wrote:
>>> [SNIP]
>>> +        if (p->mapping != adev->mman.bdev.dev_mapping)
>>> +            return -EPERM;
>>
>> This comparison fails for both IOMMU and non-IOMMU devices in my 
>> carrizo+polaris10 box.
>>
>> The address being read from is what the VM decodes to (checked with 
>> strace).
> 
> Have you applied the whole series? That patches before this one are 
> necessary to initialize p->mapping when there isn't any userspace 
> mapping for the page.


Yes, I have the entire 5 pages applied to a temp branch based on the tip 
of drm-next

$ git log --oneline HEAD~10..
405bc1dc85db (HEAD -> iomem) wip
a06d7a6f29e4 drm/amdgpu: replace iova debugfs file with iomem
d324c21f2c5e drm/ttm: set page mapping during allocation
9f440ee91c58 drm/radeon: remove extra TT unpopulated check
f55d505b0387 drm/amdgpu: remove extra TT unpopulated check
37d705119ea8 drm/ttm: add ttm_tt_populate wrapper
53af6035d04b (origin/amd-staging-drm-next, amd-staging-drm-next) 
drm/radeon: only enable swiotlb path when need v2

(the wip is me adding printks to see which error path is taken).

I don't see an init call for adev->mman.bdev.man[TTM_PL_SYSTEM] 
anywhere.  Maybe that's related?

Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Christian König]

Am 09.02.2018 um 15:02 schrieb Tom St Denis:
> On 09/02/18 08:56 AM, Christian König wrote:
>> Am 09.02.2018 um 14:32 schrieb Tom St Denis:
>>> On 02/02/18 02:09 PM, Christian König wrote:
>>>> [SNIP]
>>>> +        if (p->mapping != adev->mman.bdev.dev_mapping)
>>>> +            return -EPERM;
>>>
>>> This comparison fails for both IOMMU and non-IOMMU devices in my 
>>> carrizo+polaris10 box.
>>>
>>> The address being read from is what the VM decodes to (checked with 
>>> strace).
>>
>> Have you applied the whole series? That patches before this one are 
>> necessary to initialize p->mapping when there isn't any userspace 
>> mapping for the page.
>
>
> Yes, I have the entire 5 pages applied to a temp branch based on the 
> tip of drm-next
>
> $ git log --oneline HEAD~10..
> 405bc1dc85db (HEAD -> iomem) wip
> a06d7a6f29e4 drm/amdgpu: replace iova debugfs file with iomem
> d324c21f2c5e drm/ttm: set page mapping during allocation
> 9f440ee91c58 drm/radeon: remove extra TT unpopulated check
> f55d505b0387 drm/amdgpu: remove extra TT unpopulated check
> 37d705119ea8 drm/ttm: add ttm_tt_populate wrapper
> 53af6035d04b (origin/amd-staging-drm-next, amd-staging-drm-next) 
> drm/radeon: only enable swiotlb path when need v2
>
> (the wip is me adding printks to see which error path is taken).
>
> I don't see an init call for adev->mman.bdev.man[TTM_PL_SYSTEM] 
> anywhere.  Maybe that's related?

No, there is simply no need to initialize the system domain. What are 
the values of p->mapping and adev->mman.bdev.dev_mapping when they don't 
match? Maybe we are allocating memory before initializing 
adev->mman.bdev.dev_mapping.

Or do you have more than one GPU in the system? E.g. APU+dGPU? Could it 
be that you read through the wrong device?

Christian.

>
> Tom

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

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Tom St Denis]

On 09/02/18 09:12 AM, Christian König wrote:
> No, there is simply no need to initialize the system domain. What are 
> the values of p->mapping and adev->mman.bdev.dev_mapping when they don't 
> match? Maybe we are allocating memory before initializing 
> adev->mman.bdev.dev_mapping.

In my test setup I'm running test 3 from libdrm (suite 1) with a pause 
before the unmap/free call.  So the IB should still be mapped.  Indeed 
the VM PTE decoding has the V bit set.

> Or do you have more than one GPU in the system? E.g. APU+dGPU? Could it 
> be that you read through the wrong device?

I found the issue:

	while (size) {
		phys_addr_t addr = *pos & PAGE_MASK;
		loff_t off = *pos & ~PAGE_MASK;
		size_t bytes = PAGE_SIZE - off;

"bytes" should be limited by the 'size' parameter passed in.  What is 
happening instead is it's reading the entire PTB until it hits a V=0 
page and then returns an error and in the process is doing "fun things" 
to the user mode application (by copying more data than I asked for).


Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Christian König]

Am 09.02.2018 um 15:51 schrieb Tom St Denis:
> On 09/02/18 09:12 AM, Christian König wrote:
>> No, there is simply no need to initialize the system domain. What are 
>> the values of p->mapping and adev->mman.bdev.dev_mapping when they 
>> don't match? Maybe we are allocating memory before initializing 
>> adev->mman.bdev.dev_mapping.
>
> In my test setup I'm running test 3 from libdrm (suite 1) with a pause 
> before the unmap/free call.  So the IB should still be mapped.  Indeed 
> the VM PTE decoding has the V bit set.
>
>> Or do you have more than one GPU in the system? E.g. APU+dGPU? Could 
>> it be that you read through the wrong device?
>
> I found the issue:
>
>     while (size) {
>         phys_addr_t addr = *pos & PAGE_MASK;
>         loff_t off = *pos & ~PAGE_MASK;
>         size_t bytes = PAGE_SIZE - off;
>
> "bytes" should be limited by the 'size' parameter passed in.  What is 
> happening instead is it's reading the entire PTB until it hits a V=0 
> page and then returns an error and in the process is doing "fun 
> things" to the user mode application (by copying more data than I 
> asked for).

Ah, obvious problem.

Do you want to fix it or should I take a look? You wanted to add write 
support as well anyway IIRC.

I've just pushed the first three patches from that series to 
amd-staging-drm-next.

Thanks for testing,
Christian.

>
>
> Tom

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

Re: [PATCH 5/5] drm/amdgpu: replace iova debugfs file with iomem

[Tom St Denis]

On 09/02/18 09:56 AM, Christian König wrote:
> Am 09.02.2018 um 15:51 schrieb Tom St Denis:
>> On 09/02/18 09:12 AM, Christian König wrote:
>>> No, there is simply no need to initialize the system domain. What are 
>>> the values of p->mapping and adev->mman.bdev.dev_mapping when they 
>>> don't match? Maybe we are allocating memory before initializing 
>>> adev->mman.bdev.dev_mapping.
>>
>> In my test setup I'm running test 3 from libdrm (suite 1) with a pause 
>> before the unmap/free call.  So the IB should still be mapped.  Indeed 
>> the VM PTE decoding has the V bit set.
>>
>>> Or do you have more than one GPU in the system? E.g. APU+dGPU? Could 
>>> it be that you read through the wrong device?
>>
>> I found the issue:
>>
>>     while (size) {
>>         phys_addr_t addr = *pos & PAGE_MASK;
>>         loff_t off = *pos & ~PAGE_MASK;
>>         size_t bytes = PAGE_SIZE - off;
>>
>> "bytes" should be limited by the 'size' parameter passed in.  What is 
>> happening instead is it's reading the entire PTB until it hits a V=0 
>> page and then returns an error and in the process is doing "fun 
>> things" to the user mode application (by copying more data than I 
>> asked for).
> 
> Ah, obvious problem.
> 
> Do you want to fix it or should I take a look? You wanted to add write 
> support as well anyway IIRC.

Yup, I can tackle this this afternoon.

I'll take your read only patch and make it do both read/write (and fix 
the minor error).

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