Re: [PATCH 2/4] drm/amd/amdgpu: add support for iova_to_phys to replace TTM trace (v3)

2017-09-19 Thread Christian König 

Am 18.09.2017 um 19:33 schrieb Tom St Denis:

Signed-off-by: Tom St Denis 

(v2): Add domain to iova debugfs
(v3): Add true read/write methods to access system memory of pages
   mapped to the device
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 104 
  1 file changed, 104 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 50d20903de4f..02ae32378e1c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -43,6 +43,7 @@
  #include 
  #include 
  #include 
+#include 
  #include "amdgpu.h"
  #include "amdgpu_trace.h"
  #include "bif/bif_4_1_d.h"
@@ -1810,6 +1811,108 @@ static const struct file_operations amdgpu_ttm_gtt_fops 
= {
  #endif
  
  
+static void *transform_page(uint64_t phys)

+{
+   if (PageHighMem(pfn_to_page(PFN_DOWN(phys
+   return kmap(pfn_to_page(PFN_DOWN(phys)));
+   else
+   return __va(phys);
+}
+
+static void untransform_page(uint64_t phys)
+{
+   if (PageHighMem(pfn_to_page(PFN_DOWN(phys
+   return kunmap(pfn_to_page(PFN_DOWN(phys)));
+}


No need for the extra PageHighMem check, just use kmap()/kunmap() they 
should do the right thing IIRC.



+
+static ssize_t amdgpu_iova_to_phys_read(struct file *f, char __user *buf,
+  size_t size, loff_t *pos)
+{
+   struct amdgpu_device *adev = file_inode(f)->i_private;
+   ssize_t result, n;
+   int r;
+   uint64_t phys;
+   void *ptr;
+
+   result = 0;
+   while (size) {
+   // get physical address and map
+   phys = iommu_iova_to_phys(iommu_get_domain_for_dev(adev->dev), 
*pos);


Not sure what iommu_get_domain_for_dev does exactly, but the iommu 
domain for the device should always be the same so I would call the 
function only once before the loop.


Also failing with -ENODEV here when iommu_get_domain_for_dev() returns 
NULL sounds like a good idea to me.



+
+   // copy upto one page
+   if (size > PAGE_SIZE)
+   n = PAGE_SIZE;
+   else
+   n = size;
+
+   // to end of the page
+   if (((*pos & (PAGE_SIZE - 1)) + n) >= PAGE_SIZE)
+   n = PAGE_SIZE - (*pos & (PAGE_SIZE - 1));
+
+   ptr = transform_page(phys);
+   if (!ptr)
+   return -EFAULT;
+
+   r = copy_to_user(buf, ptr, n);
+   untransform_page(phys);
+   if (r)
+   return -EFAULT;
+
+   *pos += n;
+   size -= n;
+   result += n;
+   }
+
+   return result;
+}
+
+static ssize_t amdgpu_iova_to_phys_write(struct file *f, const char __user 
*buf,
+  size_t size, loff_t *pos)
+{
+   struct amdgpu_device *adev = file_inode(f)->i_private;
+   ssize_t result, n;
+   int r;
+   uint64_t phys;
+   void *ptr;
+
+   result = 0;
+   while (size) {
+   // get physical address and map
+   phys = iommu_iova_to_phys(iommu_get_domain_for_dev(adev->dev), 
*pos);


Same comment as above.

Apart from that looks good to me,
Christian.


+
+   // copy upto one page
+   if (size > PAGE_SIZE)
+   n = PAGE_SIZE;
+   else
+   n = size;
+
+   // to end of the page
+   if (((*pos & (PAGE_SIZE - 1)) + n) >= PAGE_SIZE)
+   n = PAGE_SIZE - (*pos & (PAGE_SIZE - 1));
+
+   ptr = transform_page(phys);
+   if (!ptr)
+   return -EFAULT;
+
+   r = copy_from_user(ptr, buf, n);
+   untransform_page(phys);
+   if (r)
+   return -EFAULT;
+
+   *pos += n;
+   size -= n;
+   result += n;
+   }
+
+   return result;
+}
+
+static const struct file_operations amdgpu_ttm_iova_fops = {
+   .owner = THIS_MODULE,
+   .read = amdgpu_iova_to_phys_read,
+   .write = amdgpu_iova_to_phys_write,
+   .llseek = default_llseek
+};
  
  static const struct {

char *name;
@@ -1820,6 +1923,7 @@ static const struct {
  #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
{ "amdgpu_gtt", _ttm_gtt_fops, TTM_PL_TT },
  #endif
+   { "amdgpu_iova", _ttm_iova_fops, TTM_PL_SYSTEM },
  };
  
  #endif



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

[PATCH 2/4] drm/amd/amdgpu: add support for iova_to_phys to replace TTM trace (v3)

2017-09-18 Thread Tom St Denis 
Signed-off-by: Tom St Denis 

(v2): Add domain to iova debugfs
(v3): Add true read/write methods to access system memory of pages
  mapped to the device
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 104 
 1 file changed, 104 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 50d20903de4f..02ae32378e1c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "amdgpu.h"
 #include "amdgpu_trace.h"
 #include "bif/bif_4_1_d.h"
@@ -1810,6 +1811,108 @@ static const struct file_operations amdgpu_ttm_gtt_fops 
= {
 #endif
 
 
+static void *transform_page(uint64_t phys)
+{
+   if (PageHighMem(pfn_to_page(PFN_DOWN(phys
+   return kmap(pfn_to_page(PFN_DOWN(phys)));
+   else
+   return __va(phys);
+}
+
+static void untransform_page(uint64_t phys)
+{
+   if (PageHighMem(pfn_to_page(PFN_DOWN(phys
+   return kunmap(pfn_to_page(PFN_DOWN(phys)));
+}
+
+static ssize_t amdgpu_iova_to_phys_read(struct file *f, char __user *buf,
+  size_t size, loff_t *pos)
+{
+   struct amdgpu_device *adev = file_inode(f)->i_private;
+   ssize_t result, n;
+   int r;
+   uint64_t phys;
+   void *ptr;
+
+   result = 0;
+   while (size) {
+   // get physical address and map
+   phys = iommu_iova_to_phys(iommu_get_domain_for_dev(adev->dev), 
*pos);
+
+   // copy upto one page
+   if (size > PAGE_SIZE)
+   n = PAGE_SIZE;
+   else
+   n = size;
+
+   // to end of the page
+   if (((*pos & (PAGE_SIZE - 1)) + n) >= PAGE_SIZE)
+   n = PAGE_SIZE - (*pos & (PAGE_SIZE - 1));
+
+   ptr = transform_page(phys);
+   if (!ptr)
+   return -EFAULT;
+
+   r = copy_to_user(buf, ptr, n);
+   untransform_page(phys);
+   if (r)
+   return -EFAULT;
+
+   *pos += n;
+   size -= n;
+   result += n;
+   }
+
+   return result;
+}
+
+static ssize_t amdgpu_iova_to_phys_write(struct file *f, const char __user 
*buf,
+  size_t size, loff_t *pos)
+{
+   struct amdgpu_device *adev = file_inode(f)->i_private;
+   ssize_t result, n;
+   int r;
+   uint64_t phys;
+   void *ptr;
+
+   result = 0;
+   while (size) {
+   // get physical address and map
+   phys = iommu_iova_to_phys(iommu_get_domain_for_dev(adev->dev), 
*pos);
+
+   // copy upto one page
+   if (size > PAGE_SIZE)
+   n = PAGE_SIZE;
+   else
+   n = size;
+
+   // to end of the page
+   if (((*pos & (PAGE_SIZE - 1)) + n) >= PAGE_SIZE)
+   n = PAGE_SIZE - (*pos & (PAGE_SIZE - 1));
+
+   ptr = transform_page(phys);
+   if (!ptr)
+   return -EFAULT;
+
+   r = copy_from_user(ptr, buf, n);
+   untransform_page(phys);
+   if (r)
+   return -EFAULT;
+
+   *pos += n;
+   size -= n;
+   result += n;
+   }
+
+   return result;
+}
+
+static const struct file_operations amdgpu_ttm_iova_fops = {
+   .owner = THIS_MODULE,
+   .read = amdgpu_iova_to_phys_read,
+   .write = amdgpu_iova_to_phys_write,
+   .llseek = default_llseek
+};
 
 static const struct {
char *name;
@@ -1820,6 +1923,7 @@ static const struct {
 #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
{ "amdgpu_gtt", _ttm_gtt_fops, TTM_PL_TT },
 #endif
+   { "amdgpu_iova", _ttm_iova_fops, TTM_PL_SYSTEM },
 };
 
 #endif
-- 
2.12.0

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

Re: [PATCH 2/4] drm/amd/amdgpu: add support for iova_to_phys to replace TTM trace

2017-09-18 Thread Tom St Denis 

On 18/09/17 08:52 AM, Christian König wrote:

Am 18.09.2017 um 14:35 schrieb Tom St Denis:

Signed-off-by: Tom St Denis 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 32 


  1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c

index 7848ffa99eb4..b4c298925e2a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -43,6 +43,7 @@
  #include 
  #include 
  #include 
+#include 
  #include "amdgpu.h"
  #include "amdgpu_trace.h"
  #include "bif/bif_4_1_d.h"
@@ -1810,6 +1811,36 @@ 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)
+{
+    struct amdgpu_device *adev = file_inode(f)->i_private;
+    int r;
+    uint64_t phys;
+
+    // always return 8 bytes
+    if (size != 8)
+    return -EINVAL;
+
+    // only accept page addresses
+    if (*pos & 0xFFF)
+    return -EINVAL;
+
+
+    phys = iommu_iova_to_phys(iommu_get_domain_for_dev(adev->dev), 
*pos);


Well how about adding directly read/write support for the page behind 
the address?


This way we won't need to fiddle with /dev/mem any more either.



Given the flak we got from requesting this from the iommu team I'm 
worried that might not be appreciated by the community (even though we 
maintain this part of the tree) and hurt our abilities to upstream.


I agree that adding a read/write method would be better though since we 
don't need config changes of /dev/fmem anymore.


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

Re: [PATCH 2/4] drm/amd/amdgpu: add support for iova_to_phys to replace TTM trace

2017-09-18 Thread Christian König 

Am 18.09.2017 um 14:35 schrieb Tom St Denis:

Signed-off-by: Tom St Denis 
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 32 
  1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 7848ffa99eb4..b4c298925e2a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -43,6 +43,7 @@
  #include 
  #include 
  #include 
+#include 
  #include "amdgpu.h"
  #include "amdgpu_trace.h"
  #include "bif/bif_4_1_d.h"
@@ -1810,6 +1811,36 @@ 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)
+{
+   struct amdgpu_device *adev = file_inode(f)->i_private;
+   int r;
+   uint64_t phys;
+
+   // always return 8 bytes
+   if (size != 8)
+   return -EINVAL;
+
+   // only accept page addresses
+   if (*pos & 0xFFF)
+   return -EINVAL;
+
+
+   phys = iommu_iova_to_phys(iommu_get_domain_for_dev(adev->dev), *pos);


Well how about adding directly read/write support for the page behind 
the address?


This way we won't need to fiddle with /dev/mem any more either.

Christian.


+
+   r = copy_to_user(buf, , 8);
+   if (r)
+   return -EFAULT;
+
+   return 8;
+}
+
+static const struct file_operations amdgpu_ttm_iova_fops = {
+   .owner = THIS_MODULE,
+   .read = amdgpu_iova_to_phys_read,
+   .llseek = default_llseek
+};
  
  static const struct {

char *name;
@@ -1819,6 +1850,7 @@ static const struct {
  #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
{ "amdgpu_gtt", _ttm_gtt_fops },
  #endif
+   { "amdgpu_iova", _ttm_iova_fops },
  };
  
  #endif



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

[PATCH 2/4] drm/amd/amdgpu: add support for iova_to_phys to replace TTM trace

2017-09-18 Thread Tom St Denis 
Signed-off-by: Tom St Denis 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 32 
 1 file changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 7848ffa99eb4..b4c298925e2a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "amdgpu.h"
 #include "amdgpu_trace.h"
 #include "bif/bif_4_1_d.h"
@@ -1810,6 +1811,36 @@ 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)
+{
+   struct amdgpu_device *adev = file_inode(f)->i_private;
+   int r;
+   uint64_t phys;
+
+   // always return 8 bytes 
+   if (size != 8)
+   return -EINVAL;
+
+   // only accept page addresses
+   if (*pos & 0xFFF)
+   return -EINVAL;
+
+
+   phys = iommu_iova_to_phys(iommu_get_domain_for_dev(adev->dev), *pos);
+
+   r = copy_to_user(buf, , 8);
+   if (r)
+   return -EFAULT;
+
+   return 8;
+}
+
+static const struct file_operations amdgpu_ttm_iova_fops = {
+   .owner = THIS_MODULE,
+   .read = amdgpu_iova_to_phys_read,
+   .llseek = default_llseek
+};
 
 static const struct {
char *name;
@@ -1819,6 +1850,7 @@ static const struct {
 #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
{ "amdgpu_gtt", _ttm_gtt_fops },
 #endif
+   { "amdgpu_iova", _ttm_iova_fops },
 };
 
 #endif
-- 
2.12.0

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