| Message ID | MWHPR1201MB012743AB6452692097481D08FDFE0@MWHPR1201MB0127.namprd12.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >> >
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