Re: [PATCH 2/2] gpu/radeon: use HMM mirror for userptr buffer object.
Hi Jérôme,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc3 next-20180910]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/jglisse-redhat-com/Getting-rid-of-GUP-and-use-HMM-for-user-ptr-features/20180911-020741
config: x86_64-randconfig-x000-201836 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu/drm/radeon/radeon_mn.c:43:20: error: field 'mirror' has
incomplete type
struct hmm_mirror mirror;
^~
drivers/gpu/drm/radeon/radeon_mn.c: In function 'radeon_mn_destroy':
drivers/gpu/drm/radeon/radeon_mn.c:90:2: error: implicit declaration of
function 'hmm_mirror_unregister'; did you mean 'drm_dp_aux_unregister'?
[-Werror=implicit-function-declaration]
hmm_mirror_unregister(>mirror);
^
drm_dp_aux_unregister
In file included from include/linux/firmware.h:6:0,
from drivers/gpu/drm/radeon/radeon_mn.c:31:
drivers/gpu/drm/radeon/radeon_mn.c: In function 'radeon_mirror_release':
include/linux/kernel.h:997:32: error: dereferencing pointer to incomplete
type 'struct hmm_mirror'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~
include/linux/compiler.h:335:18: note: in definition of macro
'__compiletime_assert'
int __cond = !(condition);\
^
include/linux/compiler.h:358:2: note: in expansion of macro
'_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~
include/linux/build_bug.h:45:37: note: in expansion of macro
'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
include/linux/kernel.h:997:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~
include/linux/kernel.h:997:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~
drivers/gpu/drm/radeon/radeon_mn.c:103:26: note: in expansion of macro
'container_of'
struct radeon_mn *rmn = container_of(mirror, struct radeon_mn, mirror);
^~~~
drivers/gpu/drm/radeon/radeon_mn.c: At top level:
drivers/gpu/drm/radeon/radeon_mn.c:119:24: warning: 'struct hmm_update'
declared inside parameter list will not be visible outside of this definition
or declaration
const struct hmm_update *update)
^~
drivers/gpu/drm/radeon/radeon_mn.c: In function
'radeon_sync_cpu_device_pagetables':
drivers/gpu/drm/radeon/radeon_mn.c:128:14: error: dereferencing pointer to
incomplete type 'const struct hmm_update'
end = update->end - 1;
^~
drivers/gpu/drm/radeon/radeon_mn.c: At top level:
drivers/gpu/drm/radeon/radeon_mn.c:183:21: error: variable
'radeon_mirror_ops' has initializer but incomplete type
static const struct hmm_mirror_ops radeon_mirror_ops = {
^~
drivers/gpu/drm/radeon/radeon_mn.c:184:3: error: 'const struct
hmm_mirror_ops' has no member named 'sync_cpu_device_pagetables'
.sync_cpu_device_pagetables = _sync_cpu_device_pagetables,
^~
drivers/gpu/drm/radeon/radeon_mn.c:184:32: warning: excess elements in
struct initializer
.sync_cpu_device_pagetables = _sync_cpu_device_pagetables,
^
drivers/gpu/drm/radeon/radeon_mn.c:184:32: note: (near initialization for
'radeon_mirror_ops')
drivers/gpu/drm/radeon/radeon_mn.c:185:3: error: 'const struct
hmm_mirror_ops' has no member named 'release'
.release = _mirror_release,
^~~
drivers/gpu/drm/radeon/radeon_mn.c:185:13: warning: excess elements in
struct initializer
.release = _mirror_release,
^
drivers/gpu/drm/radeon/radeon_mn.c:185:13: note: (near initialization for
'radeon_mirror_ops')
drivers/gpu/drm/radeon/radeon_mn.c: In function 'radeon_mn_get':
drivers/gpu/drm/radeon/radeon_mn.c:224:6: error: implicit declaration of
function 'hmm_mirror_register'; did you mean 'drm_dp_aux_register'?
[-Werror=implicit-function-declaration]
r = hmm_mirror_register(>mirror, mm);
^~~
drm_dp_aux_register
drivers/gpu/drm/radeon/radeon_mn.c: In function 'radeon_mn_bo_map':
>> drivers/gpu/drm/radeon/radeon_mn.c:373:43: error: 'HMM_PFN_FLAG_MAX'
>> undeclared (first use in this function); did you mean 'TTM_PL_FLAG_TT'?
Re: [PATCH 2/2] gpu/radeon: use HMM mirror for userptr buffer object.
Hi Jérôme,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.19-rc3 next-20180910]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/jglisse-redhat-com/Getting-rid-of-GUP-and-use-HMM-for-user-ptr-features/20180911-020741
config: x86_64-randconfig-x017-201836 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu/drm/radeon/radeon_mn.c:43:20: error: field 'mirror' has
incomplete type
struct hmm_mirror mirror;
^~
drivers/gpu/drm/radeon/radeon_mn.c: In function 'radeon_mn_destroy':
drivers/gpu/drm/radeon/radeon_mn.c:90:2: error: implicit declaration of
function 'hmm_mirror_unregister'; did you mean 'drm_dp_aux_unregister'?
[-Werror=implicit-function-declaration]
hmm_mirror_unregister(>mirror);
^
drm_dp_aux_unregister
In file included from include/linux/firmware.h:6:0,
from drivers/gpu/drm/radeon/radeon_mn.c:31:
drivers/gpu/drm/radeon/radeon_mn.c: In function 'radeon_mirror_release':
include/linux/kernel.h:997:32: error: dereferencing pointer to incomplete
type 'struct hmm_mirror'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~
include/linux/compiler.h:335:18: note: in definition of macro
'__compiletime_assert'
int __cond = !(condition);\
^
include/linux/compiler.h:358:2: note: in expansion of macro
'_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~
include/linux/build_bug.h:45:37: note: in expansion of macro
'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
include/linux/kernel.h:997:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~
include/linux/kernel.h:997:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~
drivers/gpu/drm/radeon/radeon_mn.c:103:26: note: in expansion of macro
'container_of'
struct radeon_mn *rmn = container_of(mirror, struct radeon_mn, mirror);
^~~~
drivers/gpu/drm/radeon/radeon_mn.c: At top level:
drivers/gpu/drm/radeon/radeon_mn.c:119:24: warning: 'struct hmm_update'
declared inside parameter list will not be visible outside of this definition
or declaration
const struct hmm_update *update)
^~
drivers/gpu/drm/radeon/radeon_mn.c: In function
'radeon_sync_cpu_device_pagetables':
drivers/gpu/drm/radeon/radeon_mn.c:128:14: error: dereferencing pointer to
incomplete type 'const struct hmm_update'
end = update->end - 1;
^~
drivers/gpu/drm/radeon/radeon_mn.c: At top level:
drivers/gpu/drm/radeon/radeon_mn.c:183:21: error: variable
'radeon_mirror_ops' has initializer but incomplete type
static const struct hmm_mirror_ops radeon_mirror_ops = {
^~
drivers/gpu/drm/radeon/radeon_mn.c:184:3: error: 'const struct
hmm_mirror_ops' has no member named 'sync_cpu_device_pagetables'
.sync_cpu_device_pagetables = _sync_cpu_device_pagetables,
^~
drivers/gpu/drm/radeon/radeon_mn.c:184:32: warning: excess elements in
struct initializer
.sync_cpu_device_pagetables = _sync_cpu_device_pagetables,
^
drivers/gpu/drm/radeon/radeon_mn.c:184:32: note: (near initialization for
'radeon_mirror_ops')
drivers/gpu/drm/radeon/radeon_mn.c:185:3: error: 'const struct
hmm_mirror_ops' has no member named 'release'
.release = _mirror_release,
^~~
drivers/gpu/drm/radeon/radeon_mn.c:185:13: warning: excess elements in
struct initializer
.release = _mirror_release,
^
drivers/gpu/drm/radeon/radeon_mn.c:185:13: note: (near initialization for
'radeon_mirror_ops')
drivers/gpu/drm/radeon/radeon_mn.c: In function 'radeon_mn_get':
drivers/gpu/drm/radeon/radeon_mn.c:224:6: error: implicit declaration of
function 'hmm_mirror_register'; did you mean 'drm_dp_aux_register'?
[-Werror=implicit-function-declaration]
r = hmm_mirror_register(>mirror, mm);
^~~
drm_dp_aux_register
drivers/gpu/drm/radeon/radeon_mn.c: In function 'radeon_mn_bo_map':
>> drivers/gpu/drm/radeon/radeon_mn.c:373:43: error: 'HMM_PFN_FLAG_MAX'
>> undeclared (first use in this function); did you mean 'TTM_PL_FLAG_VRAM'?
[PATCH 2/2] gpu/radeon: use HMM mirror for userptr buffer object.
From: Jérôme Glisse
This replace existing code that rely on get_user_page() aka GUP with
code that now use HMM mirror to mirror a range of virtual address as
a buffer object accessible by the GPU. There is no functional changes
from userspace point of view.
From kernel point of view we no longer pin pages for userptr buffer
object which is a welcome change (i am assuming that everyone dislike
page pin as i do).
Signed-off-by: Jérôme Glisse
Cc: dri-devel@lists.freedesktop.org
Cc: Alex Deucher
Cc: Christian König
Cc: Felix Kuehling
Cc: David (ChunMing) Zhou
Cc: Nicolai Hähnle
Cc: amd-...@lists.freedesktop.org
Cc: David Airlie
Cc: Daniel Vetter
---
drivers/gpu/drm/radeon/radeon.h | 14 ++-
drivers/gpu/drm/radeon/radeon_gem.c | 16 +--
drivers/gpu/drm/radeon/radeon_mn.c | 157 +++-
drivers/gpu/drm/radeon/radeon_ttm.c | 129 +++
4 files changed, 196 insertions(+), 120 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 1a6f6edb3515..6c83bf911e9c 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -514,6 +514,8 @@ struct radeon_bo {
pid_t pid;
struct radeon_mn*mn;
+ uint64_t*pfns;
+ unsigned long userptr;
struct list_headmn_list;
};
#define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, gem_base)
@@ -1787,12 +1789,22 @@ void radeon_test_syncing(struct radeon_device *rdev);
#if defined(CONFIG_MMU_NOTIFIER)
int radeon_mn_register(struct radeon_bo *bo, unsigned long addr);
void radeon_mn_unregister(struct radeon_bo *bo);
+int radeon_mn_bo_map(struct radeon_bo *bo, struct ttm_dma_tt *dma, bool write);
+void radeon_mn_bo_unmap(struct radeon_bo *bo, struct ttm_dma_tt *dma,
+ bool write);
#else
static inline int radeon_mn_register(struct radeon_bo *bo, unsigned long addr)
{
return -ENODEV;
}
static inline void radeon_mn_unregister(struct radeon_bo *bo) {}
+static int radeon_mn_bo_map(struct radeon_bo *bo, struct ttm_dma_tt *dma,
+ bool write)
+{
+ return -ENODEV;
+}
+static void radeon_mn_bo_unmap(struct radeon_bo *bo, struct ttm_dma_tt *dma,
+ bool write) {}
#endif
/*
@@ -2818,7 +2830,7 @@ extern void radeon_legacy_set_clock_gating(struct
radeon_device *rdev, int enabl
extern void radeon_atom_set_clock_gating(struct radeon_device *rdev, int
enable);
extern void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32
domain);
extern bool radeon_ttm_bo_is_radeon_bo(struct ttm_buffer_object *bo);
-extern int radeon_ttm_tt_set_userptr(struct ttm_tt *ttm, uint64_t addr,
+extern int radeon_ttm_tt_set_userptr(struct ttm_tt *ttm, struct radeon_bo *bo,
uint32_t flags);
extern bool radeon_ttm_tt_has_userptr(struct ttm_tt *ttm);
extern bool radeon_ttm_tt_is_readonly(struct ttm_tt *ttm);
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c
b/drivers/gpu/drm/radeon/radeon_gem.c
index 27d8e7dd2d06..b489025086c4 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -323,15 +323,19 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void
*data,
goto handle_lockup;
bo = gem_to_radeon_bo(gobj);
- r = radeon_ttm_tt_set_userptr(bo->tbo.ttm, args->addr, args->flags);
+
+ /*
+* Always register an HMM mirror (if one is not already registered).
+* This means ignoring RADEON_GEM_USERPTR_REGISTER flag but that flag
+* is already made mandatory by flags sanity check above.
+*/
+ r = radeon_mn_register(bo, args->addr);
if (r)
goto release_object;
- if (args->flags & RADEON_GEM_USERPTR_REGISTER) {
- r = radeon_mn_register(bo, args->addr);
- if (r)
- goto release_object;
- }
+ r = radeon_ttm_tt_set_userptr(bo->tbo.ttm, bo, args->flags);
+ if (r)
+ goto release_object;
if (args->flags & RADEON_GEM_USERPTR_VALIDATE) {
down_read(>mm->mmap_sem);
diff --git a/drivers/gpu/drm/radeon/radeon_mn.c
b/drivers/gpu/drm/radeon/radeon_mn.c
index a3bf74c1a3fc..ff53ffa5deef 100644
--- a/drivers/gpu/drm/radeon/radeon_mn.c
+++ b/drivers/gpu/drm/radeon/radeon_mn.c
@@ -262,9 +262,18 @@ int radeon_mn_register(struct radeon_bo *bo, unsigned long
addr)
struct list_head bos;
struct interval_tree_node *it;
+ bo->userptr = addr;
+ bo->pfns = kvmalloc_array(bo->tbo.num_pages, sizeof(uint64_t),
+ GFP_KERNEL | __GFP_ZERO);
+ if (bo->pfns == NULL)
+ return -ENOMEM;
+
rmn = radeon_mn_get(rdev);
- if (IS_ERR(rmn))
+ if (IS_ERR(rmn)) {
+ kvfree(bo->pfns);
+
