Hi
Some rather random cleanups on drm_file and whatever I found on the way. The idea here really is to get to a point where we can allocate drm_file from a kernel context to get in-kernel rendering to work. This would allow allocating dumb-buffers, dma-bufs, etc. from within the kernel and get fbdev generically working, as well as panic renderers et al.
The only critical thing is drm_file->filp, which points to the underlying "struct file". We really don't want to fake a "struct file" for in-kernel renderers (even though it is possible), so this series tries to get rid of all non-legacy users of it. The only real place left is vm_mmap() in drm_bufs.c.
Tested on i915 only.
Thanks David
David Herrmann (8): drm: rename DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY drm: remove redundant drm_file->uid drm: reduce GETCLIENT to a minimum drm: make vma-manager entries untyped drm: use drm_file to tag vm-bos drm: use priv->pid to deduce task EUID drm: rename drm_file.filp to drm_file.legacy_filp drm: provide management functions for drm_file
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 3 +- drivers/gpu/drm/ast/ast_ttm.c | 3 +- drivers/gpu/drm/bochs/bochs_mm.c | 3 +- drivers/gpu/drm/cirrus/cirrus_ttm.c | 3 +- drivers/gpu/drm/drm_bufs.c | 7 +- drivers/gpu/drm/drm_drv.c | 149 ++++++++++++++++++++++++++++++-- drivers/gpu/drm/drm_fops.c | 133 ++-------------------------- drivers/gpu/drm/drm_gem.c | 8 +- drivers/gpu/drm/drm_info.c | 4 +- drivers/gpu/drm/drm_internal.h | 4 + drivers/gpu/drm/drm_ioctl.c | 5 +- drivers/gpu/drm/drm_vma_manager.c | 40 ++++----- drivers/gpu/drm/mgag200/mgag200_ttm.c | 3 +- drivers/gpu/drm/nouveau/nouveau_bo.c | 3 +- drivers/gpu/drm/qxl/qxl_ttm.c | 3 +- drivers/gpu/drm/radeon/radeon_ttm.c | 3 +- include/drm/drmP.h | 7 +- include/drm/drm_vma_manager.h | 18 ++-- 18 files changed, 213 insertions(+), 186 deletions(-)
The minor referred to by "DRM_MINOR_LEGACY" is called 'dev->primary' and gets 'cardX' as name assigned. Lets reduce this magnificent number of names for the same concept by one and rename DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY (to match the actual struct-member name).
Furthermore, this is in no way a legacy node, so lets not call it that.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_drv.c | 14 +++++++------- include/drm/drmP.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index be27ed3..57ce973 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -112,7 +112,7 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, unsigned int type) { switch (type) { - case DRM_MINOR_LEGACY: + case DRM_MINOR_PRIMARY: return &dev->primary; case DRM_MINOR_RENDER: return &dev->render; @@ -512,7 +512,7 @@ int drm_dev_init(struct drm_device *dev, goto err_minors; }
- ret = drm_minor_alloc(dev, DRM_MINOR_LEGACY); + ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY); if (ret) goto err_minors;
@@ -545,7 +545,7 @@ err_ctxbitmap: drm_legacy_ctxbitmap_cleanup(dev); drm_ht_remove(&dev->map_hash); err_minors: - drm_minor_free(dev, DRM_MINOR_LEGACY); + drm_minor_free(dev, DRM_MINOR_PRIMARY); drm_minor_free(dev, DRM_MINOR_RENDER); drm_minor_free(dev, DRM_MINOR_CONTROL); drm_fs_inode_free(dev->anon_inode); @@ -608,7 +608,7 @@ static void drm_dev_release(struct kref *ref) drm_ht_remove(&dev->map_hash); drm_fs_inode_free(dev->anon_inode);
- drm_minor_free(dev, DRM_MINOR_LEGACY); + drm_minor_free(dev, DRM_MINOR_PRIMARY); drm_minor_free(dev, DRM_MINOR_RENDER); drm_minor_free(dev, DRM_MINOR_CONTROL);
@@ -684,7 +684,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) if (ret) goto err_minors;
- ret = drm_minor_register(dev, DRM_MINOR_LEGACY); + ret = drm_minor_register(dev, DRM_MINOR_PRIMARY); if (ret) goto err_minors;
@@ -701,7 +701,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) goto out_unlock;
err_minors: - drm_minor_unregister(dev, DRM_MINOR_LEGACY); + drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); drm_minor_unregister(dev, DRM_MINOR_CONTROL); out_unlock: @@ -741,7 +741,7 @@ void drm_dev_unregister(struct drm_device *dev) list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) drm_legacy_rmmap(dev, r_list->map);
- drm_minor_unregister(dev, DRM_MINOR_LEGACY); + drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); drm_minor_unregister(dev, DRM_MINOR_CONTROL); } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d377865..d488a72 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -642,7 +642,7 @@ struct drm_driver { };
enum drm_minor_type { - DRM_MINOR_LEGACY, + DRM_MINOR_PRIMARY, DRM_MINOR_CONTROL, DRM_MINOR_RENDER, DRM_MINOR_CNT, @@ -856,7 +856,7 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv)
static inline bool drm_is_primary_client(const struct drm_file *file_priv) { - return file_priv->minor->type == DRM_MINOR_LEGACY; + return file_priv->minor->type == DRM_MINOR_PRIMARY; }
/******************************************************************/
On 03/08/16 19:04, David Herrmann wrote:
The minor referred to by "DRM_MINOR_LEGACY" is called 'dev->primary' and gets 'cardX' as name assigned. Lets reduce this magnificent number of names for the same concept by one and rename DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY (to match the actual struct-member name).
Furthermore, this is in no way a legacy node, so lets not call it that.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
Reviewed-by: Frank Binns frank.binns@imgtec.com
drivers/gpu/drm/drm_drv.c | 14 +++++++------- include/drm/drmP.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index be27ed3..57ce973 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -112,7 +112,7 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, unsigned int type) { switch (type) {
- case DRM_MINOR_LEGACY:
- case DRM_MINOR_PRIMARY: return &dev->primary; case DRM_MINOR_RENDER: return &dev->render;
@@ -512,7 +512,7 @@ int drm_dev_init(struct drm_device *dev, goto err_minors; }
- ret = drm_minor_alloc(dev, DRM_MINOR_LEGACY);
- ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY); if (ret) goto err_minors;
@@ -545,7 +545,7 @@ err_ctxbitmap: drm_legacy_ctxbitmap_cleanup(dev); drm_ht_remove(&dev->map_hash); err_minors:
- drm_minor_free(dev, DRM_MINOR_LEGACY);
- drm_minor_free(dev, DRM_MINOR_PRIMARY); drm_minor_free(dev, DRM_MINOR_RENDER); drm_minor_free(dev, DRM_MINOR_CONTROL); drm_fs_inode_free(dev->anon_inode);
@@ -608,7 +608,7 @@ static void drm_dev_release(struct kref *ref) drm_ht_remove(&dev->map_hash); drm_fs_inode_free(dev->anon_inode);
- drm_minor_free(dev, DRM_MINOR_LEGACY);
- drm_minor_free(dev, DRM_MINOR_PRIMARY); drm_minor_free(dev, DRM_MINOR_RENDER); drm_minor_free(dev, DRM_MINOR_CONTROL);
@@ -684,7 +684,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) if (ret) goto err_minors;
- ret = drm_minor_register(dev, DRM_MINOR_LEGACY);
- ret = drm_minor_register(dev, DRM_MINOR_PRIMARY); if (ret) goto err_minors;
@@ -701,7 +701,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) goto out_unlock;
err_minors:
- drm_minor_unregister(dev, DRM_MINOR_LEGACY);
- drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); drm_minor_unregister(dev, DRM_MINOR_CONTROL); out_unlock:
@@ -741,7 +741,7 @@ void drm_dev_unregister(struct drm_device *dev) list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) drm_legacy_rmmap(dev, r_list->map);
- drm_minor_unregister(dev, DRM_MINOR_LEGACY);
- drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); drm_minor_unregister(dev, DRM_MINOR_CONTROL); }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d377865..d488a72 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -642,7 +642,7 @@ struct drm_driver { };
enum drm_minor_type {
- DRM_MINOR_LEGACY,
- DRM_MINOR_PRIMARY, DRM_MINOR_CONTROL, DRM_MINOR_RENDER, DRM_MINOR_CNT,
@@ -856,7 +856,7 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv)
static inline bool drm_is_primary_client(const struct drm_file *file_priv) {
- return file_priv->minor->type == DRM_MINOR_LEGACY;
return file_priv->minor->type == DRM_MINOR_PRIMARY; }
/******************************************************************/
On Thu, Aug 04, 2016 at 08:45:25AM +0100, Frank Binns wrote:
On 03/08/16 19:04, David Herrmann wrote:
The minor referred to by "DRM_MINOR_LEGACY" is called 'dev->primary' and gets 'cardX' as name assigned. Lets reduce this magnificent number of names for the same concept by one and rename DRM_MINOR_LEGACY to DRM_MINOR_PRIMARY (to match the actual struct-member name).
Furthermore, this is in no way a legacy node, so lets not call it that.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
Reviewed-by: Frank Binns frank.binns@imgtec.com
Queued up for -misc, will push out once -rc1 hits. -Daniel
drivers/gpu/drm/drm_drv.c | 14 +++++++------- include/drm/drmP.h | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index be27ed3..57ce973 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -112,7 +112,7 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, unsigned int type) { switch (type) {
- case DRM_MINOR_LEGACY:
- case DRM_MINOR_PRIMARY: return &dev->primary; case DRM_MINOR_RENDER: return &dev->render;
@@ -512,7 +512,7 @@ int drm_dev_init(struct drm_device *dev, goto err_minors; }
- ret = drm_minor_alloc(dev, DRM_MINOR_LEGACY);
- ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY); if (ret) goto err_minors;
@@ -545,7 +545,7 @@ err_ctxbitmap: drm_legacy_ctxbitmap_cleanup(dev); drm_ht_remove(&dev->map_hash); err_minors:
- drm_minor_free(dev, DRM_MINOR_LEGACY);
- drm_minor_free(dev, DRM_MINOR_PRIMARY); drm_minor_free(dev, DRM_MINOR_RENDER); drm_minor_free(dev, DRM_MINOR_CONTROL); drm_fs_inode_free(dev->anon_inode);
@@ -608,7 +608,7 @@ static void drm_dev_release(struct kref *ref) drm_ht_remove(&dev->map_hash); drm_fs_inode_free(dev->anon_inode);
- drm_minor_free(dev, DRM_MINOR_LEGACY);
- drm_minor_free(dev, DRM_MINOR_PRIMARY); drm_minor_free(dev, DRM_MINOR_RENDER); drm_minor_free(dev, DRM_MINOR_CONTROL);
@@ -684,7 +684,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) if (ret) goto err_minors;
- ret = drm_minor_register(dev, DRM_MINOR_LEGACY);
- ret = drm_minor_register(dev, DRM_MINOR_PRIMARY); if (ret) goto err_minors;
@@ -701,7 +701,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) goto out_unlock; err_minors:
- drm_minor_unregister(dev, DRM_MINOR_LEGACY);
- drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); drm_minor_unregister(dev, DRM_MINOR_CONTROL); out_unlock:
@@ -741,7 +741,7 @@ void drm_dev_unregister(struct drm_device *dev) list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) drm_legacy_rmmap(dev, r_list->map);
- drm_minor_unregister(dev, DRM_MINOR_LEGACY);
- drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); drm_minor_unregister(dev, DRM_MINOR_CONTROL); }
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d377865..d488a72 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -642,7 +642,7 @@ struct drm_driver { }; enum drm_minor_type {
- DRM_MINOR_LEGACY,
- DRM_MINOR_PRIMARY, DRM_MINOR_CONTROL, DRM_MINOR_RENDER, DRM_MINOR_CNT,
@@ -856,7 +856,7 @@ static inline bool drm_is_control_client(const struct drm_file *file_priv) static inline bool drm_is_primary_client(const struct drm_file *file_priv) {
- return file_priv->minor->type == DRM_MINOR_LEGACY;
- return file_priv->minor->type == DRM_MINOR_PRIMARY; } /******************************************************************/
Each DRM file-context caches the EUID of the process that opened the file. It is used exclusively for debugging purposes in /proc/dri/ and friends.
Note, however, that "struct file" already caches the credentials of a process at open-time. So lets just use drm_file->filp->f_cred->euid if available. The pointer-chasing will not hurt us, since it is only about debugging, anyway.
Check priv->filp for NULL before deref, to prepare for in-kernel contexts (if they ever appear). We should not rely on "struct file" contexts to be around at all times, anyway.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_fops.c | 1 - drivers/gpu/drm/drm_info.c | 4 +++- drivers/gpu/drm/drm_ioctl.c | 4 +++- include/drm/drmP.h | 1 - 4 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 323c238..e9d66f5 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -199,7 +199,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
filp->private_data = priv; priv->filp = filp; - priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = minor;
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 9ae353f..247ba2b 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -80,6 +80,7 @@ int drm_clients_info(struct seq_file *m, void *data) struct drm_info_node *node = (struct drm_info_node *) m->private; struct drm_device *dev = node->minor->dev; struct drm_file *priv; + kuid_t uid;
seq_printf(m, "%20s %5s %3s master a %5s %10s\n", @@ -98,13 +99,14 @@ int drm_clients_info(struct seq_file *m, void *data)
rcu_read_lock(); /* locks pid_task()->comm */ task = pid_task(priv->pid, PIDTYPE_PID); + uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID; seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", task ? task->comm : "<unknown>", pid_vnr(priv->pid), priv->minor->index, drm_is_current_master(priv) ? 'y' : 'n', priv->authenticated ? 'y' : 'n', - from_kuid_munged(seq_user_ns(m), priv->uid), + from_kuid_munged(seq_user_ns(m), uid), priv->magic); rcu_read_unlock(); } diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 33af4a5..49cd835 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -191,7 +191,9 @@ static int drm_getclient(struct drm_device *dev, void *data, client->auth = file_priv->authenticated; client->pid = pid_vnr(file_priv->pid); client->uid = from_kuid_munged(current_user_ns(), - file_priv->uid); + file_priv->filp ? + file_priv->filp->f_cred->euid : + GLOBAL_ROOT_UID); client->magic = 0; client->iocs = 0;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d488a72..0f69f56 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -320,7 +320,6 @@ struct drm_file { unsigned is_master:1;
struct pid *pid; - kuid_t uid; drm_magic_t magic; struct list_head lhead; struct drm_minor *minor;
On Wed, Aug 03, 2016 at 08:04:26PM +0200, David Herrmann wrote:
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 323c238..e9d66f5 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -199,7 +199,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
filp->private_data = priv; priv->filp = filp;
- priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = minor;
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 9ae353f..247ba2b 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -98,13 +99,14 @@ int drm_clients_info(struct seq_file *m, void *data)
rcu_read_lock(); /* locks pid_task()->comm */ task = pid_task(priv->pid, PIDTYPE_PID);
uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID;
uid = task_euid(task);
seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", task ? task->comm : "<unknown>", pid_vnr(priv->pid), priv->minor->index, drm_is_current_master(priv) ? 'y' : 'n', priv->authenticated ? 'y' : 'n',
from_kuid_munged(seq_user_ns(m), priv->uid),
from_kuid_munged(seq_user_ns(m), uid),
from_kuid_munged(seq_user_ns(m), task_euid(task)),
Just fits. -Chris
On Wed, Aug 03, 2016 at 08:04:26PM +0200, David Herrmann wrote:
@@ -98,13 +99,14 @@ int drm_clients_info(struct seq_file *m, void *data)
rcu_read_lock(); /* locks pid_task()->comm */ task = pid_task(priv->pid, PIDTYPE_PID);
uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID;
from_kuid_munged(seq_user_ns(m), priv->uid),
from_kuid_munged(seq_user_ns(m), uid), priv->magic);diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 33af4a5..49cd835 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -191,7 +191,9 @@ static int drm_getclient(struct drm_device *dev, void *data, client->auth = file_priv->authenticated; client->pid = pid_vnr(file_priv->pid); client->uid = from_kuid_munged(current_user_ns(),
file_priv->uid);
file_priv->filp ?file_priv->filp->f_cred->euid :GLOBAL_ROOT_UID);
Why can't we use task_euid(pid_task(file_priv->pid)) here as well? -Chris
Hey
On Wed, Aug 3, 2016 at 9:01 PM, Chris Wilson chris@chris-wilson.co.uk wrote:
On Wed, Aug 03, 2016 at 08:04:26PM +0200, David Herrmann wrote:
@@ -98,13 +99,14 @@ int drm_clients_info(struct seq_file *m, void *data)
rcu_read_lock(); /* locks pid_task()->comm */ task = pid_task(priv->pid, PIDTYPE_PID);
uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID; seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", task ? task->comm : "<unknown>", pid_vnr(priv->pid), priv->minor->index, drm_is_current_master(priv) ? 'y' : 'n', priv->authenticated ? 'y' : 'n',
from_kuid_munged(seq_user_ns(m), priv->uid),
from_kuid_munged(seq_user_ns(m), uid), priv->magic); rcu_read_unlock(); }diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 33af4a5..49cd835 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -191,7 +191,9 @@ static int drm_getclient(struct drm_device *dev, void *data, client->auth = file_priv->authenticated; client->pid = pid_vnr(file_priv->pid); client->uid = from_kuid_munged(current_user_ns(),
file_priv->uid);
file_priv->filp ?file_priv->filp->f_cred->euid :GLOBAL_ROOT_UID);Why can't we use task_euid(pid_task(file_priv->pid)) here as well?
task_euid() changes semantics. With this patch I just tried to get rid of "filp" usage, but keep semantics, so the ABI does not suddently change.
Note that both calls are actually changed in follow-ups. "uid" is just cleared to "overflowuid", since no-one ever looks at that field. And "pid" is set to "pid_vnr(current)". However, I explicitly split those patches to make sure it is easier to bisect.
Thanks David
The *only* known user of GETCLIENT is libva, which uses it to check whether its own context is authenticated. It used to iterate all clients, look for one that matches its own pid and then check its state.
The entire purpose for us to still have a GETCLIENT implementation is to serve libva. So lets not pretend we do anything else: Make this function return information on the caller's context only, fake the PID to the caller's pid so they always match, and just fill in the "authenticated" bit, nothing else.
This patch reduces the complexity of GETCLIENT to a bare minimum, avoids any dependency on priv->uid or priv->pid (allows us to get rid of them), and makes libva happy by always *exactly* returning the information it wants.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_ioctl.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 49cd835..bc5c65e 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -189,11 +189,8 @@ static int drm_getclient(struct drm_device *dev, void *data, */ if (client->idx == 0) { client->auth = file_priv->authenticated; - client->pid = pid_vnr(file_priv->pid); - client->uid = from_kuid_munged(current_user_ns(), - file_priv->filp ? - file_priv->filp->f_cred->euid : - GLOBAL_ROOT_UID); + client->pid = task_pid_vnr(current); + client->uid = overflowuid; client->magic = 0; client->iocs = 0;
Right now, the vma-manager requires all entries to provide a "struct file*" as identifier. This is confusing, since there is no inherent connection to "struct file" in the vma-manager at all, but it is exclusively used as tag.
Replace its usage with a simple "void *tag" and make sure callers can use arbitrary tags.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_vma_manager.c | 40 +++++++++++++++++++-------------------- include/drm/drm_vma_manager.h | 18 ++++++++---------- 2 files changed, 27 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c index f306c88..954248e 100644 --- a/drivers/gpu/drm/drm_vma_manager.c +++ b/drivers/gpu/drm/drm_vma_manager.c @@ -25,7 +25,6 @@ #include <drm/drmP.h> #include <drm/drm_mm.h> #include <drm/drm_vma_manager.h> -#include <linux/fs.h> #include <linux/mm.h> #include <linux/module.h> #include <linux/rbtree.h> @@ -277,9 +276,9 @@ EXPORT_SYMBOL(drm_vma_offset_remove); /** * drm_vma_node_allow - Add open-file to list of allowed users * @node: Node to modify - * @filp: Open file to add + * @tag: Tag of file to remove * - * Add @filp to the list of allowed open-files for this node. If @filp is + * Add @tag to the list of allowed open-files for this node. If @tag is * already on this list, the ref-count is incremented. * * The list of allowed-users is preserved across drm_vma_offset_add() and @@ -294,7 +293,7 @@ EXPORT_SYMBOL(drm_vma_offset_remove); * RETURNS: * 0 on success, negative error code on internal failure (out-of-mem) */ -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct file *filp) +int drm_vma_node_allow(struct drm_vma_offset_node *node, void *tag) { struct rb_node **iter; struct rb_node *parent = NULL; @@ -315,10 +314,10 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct file *filp) parent = *iter; entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
- if (filp == entry->vm_filp) { + if (tag == entry->vm_tag) { entry->vm_count++; goto unlock; - } else if (filp > entry->vm_filp) { + } else if (tag > entry->vm_tag) { iter = &(*iter)->rb_right; } else { iter = &(*iter)->rb_left; @@ -330,7 +329,7 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct file *filp) goto unlock; }
- new->vm_filp = filp; + new->vm_tag = tag; new->vm_count = 1; rb_link_node(&new->vm_rb, parent, iter); rb_insert_color(&new->vm_rb, &node->vm_files); @@ -346,17 +345,17 @@ EXPORT_SYMBOL(drm_vma_node_allow); /** * drm_vma_node_revoke - Remove open-file from list of allowed users * @node: Node to modify - * @filp: Open file to remove + * @tag: Tag of file to remove * - * Decrement the ref-count of @filp in the list of allowed open-files on @node. - * If the ref-count drops to zero, remove @filp from the list. You must call - * this once for every drm_vma_node_allow() on @filp. + * Decrement the ref-count of @tag in the list of allowed open-files on @node. + * If the ref-count drops to zero, remove @tag from the list. You must call + * this once for every drm_vma_node_allow() on @tag. * * This is locked against concurrent access internally. * - * If @filp is not on the list, nothing is done. + * If @tag is not on the list, nothing is done. */ -void drm_vma_node_revoke(struct drm_vma_offset_node *node, struct file *filp) +void drm_vma_node_revoke(struct drm_vma_offset_node *node, void *tag) { struct drm_vma_offset_file *entry; struct rb_node *iter; @@ -366,13 +365,13 @@ void drm_vma_node_revoke(struct drm_vma_offset_node *node, struct file *filp) iter = node->vm_files.rb_node; while (likely(iter)) { entry = rb_entry(iter, struct drm_vma_offset_file, vm_rb); - if (filp == entry->vm_filp) { + if (tag == entry->vm_tag) { if (!--entry->vm_count) { rb_erase(&entry->vm_rb, &node->vm_files); kfree(entry); } break; - } else if (filp > entry->vm_filp) { + } else if (tag > entry->vm_tag) { iter = iter->rb_right; } else { iter = iter->rb_left; @@ -386,9 +385,9 @@ EXPORT_SYMBOL(drm_vma_node_revoke); /** * drm_vma_node_is_allowed - Check whether an open-file is granted access * @node: Node to check - * @filp: Open-file to check for + * @tag: Tag of file to remove * - * Search the list in @node whether @filp is currently on the list of allowed + * Search the list in @node whether @tag is currently on the list of allowed * open-files (see drm_vma_node_allow()). * * This is locked against concurrent access internally. @@ -396,8 +395,7 @@ EXPORT_SYMBOL(drm_vma_node_revoke); * RETURNS: * true iff @filp is on the list */ -bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node, - struct file *filp) +bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node, void *tag) { struct drm_vma_offset_file *entry; struct rb_node *iter; @@ -407,9 +405,9 @@ bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node, iter = node->vm_files.rb_node; while (likely(iter)) { entry = rb_entry(iter, struct drm_vma_offset_file, vm_rb); - if (filp == entry->vm_filp) + if (tag == entry->vm_tag) break; - else if (filp > entry->vm_filp) + else if (tag > entry->vm_tag) iter = iter->rb_right; else iter = iter->rb_left; diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h index 06ea8e07..6edc968 100644 --- a/include/drm/drm_vma_manager.h +++ b/include/drm/drm_vma_manager.h @@ -24,7 +24,6 @@ */
#include <drm/drm_mm.h> -#include <linux/fs.h> #include <linux/mm.h> #include <linux/module.h> #include <linux/rbtree.h> @@ -33,7 +32,7 @@
struct drm_vma_offset_file { struct rb_node vm_rb; - struct file *vm_filp; + void *vm_tag; unsigned long vm_count; };
@@ -62,10 +61,9 @@ int drm_vma_offset_add(struct drm_vma_offset_manager *mgr, void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr, struct drm_vma_offset_node *node);
-int drm_vma_node_allow(struct drm_vma_offset_node *node, struct file *filp); -void drm_vma_node_revoke(struct drm_vma_offset_node *node, struct file *filp); -bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node, - struct file *filp); +int drm_vma_node_allow(struct drm_vma_offset_node *node, void *tag); +void drm_vma_node_revoke(struct drm_vma_offset_node *node, void *tag); +bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node, void *tag);
/** * drm_vma_offset_exact_lookup_locked() - Look up node by exact address @@ -216,9 +214,9 @@ static inline void drm_vma_node_unmap(struct drm_vma_offset_node *node, /** * drm_vma_node_verify_access() - Access verification helper for TTM * @node: Offset node - * @filp: Open-file + * @tag: Tag of file to check * - * This checks whether @filp is granted access to @node. It is the same as + * This checks whether @tag is granted access to @node. It is the same as * drm_vma_node_is_allowed() but suitable as drop-in helper for TTM * verify_access() callbacks. * @@ -226,9 +224,9 @@ static inline void drm_vma_node_unmap(struct drm_vma_offset_node *node, * 0 if access is granted, -EACCES otherwise. */ static inline int drm_vma_node_verify_access(struct drm_vma_offset_node *node, - struct file *filp) + void *tag) { - return drm_vma_node_is_allowed(node, filp) ? 0 : -EACCES; + return drm_vma_node_is_allowed(node, tag) ? 0 : -EACCES; }
#endif /* __DRM_VMA_MANAGER_H__ */
On Wed, Aug 03, 2016 at 08:04:28PM +0200, David Herrmann wrote:
Right now, the vma-manager requires all entries to provide a "struct file*" as identifier. This is confusing, since there is no inherent connection to "struct file" in the vma-manager at all, but it is exclusively used as tag.
Replace its usage with a simple "void *tag" and make sure callers can use arbitrary tags.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
struct drm_vma_offset_file {
Now a misnomer.
Lgtm, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On 3 August 2016 at 19:04, David Herrmann dh.herrmann@gmail.com wrote:
@@ -277,9 +276,9 @@ EXPORT_SYMBOL(drm_vma_offset_remove); /**
- drm_vma_node_allow - Add open-file to list of allowed users
- @node: Node to modify
- @filp: Open file to add
- @tag: Tag of file to remove
s/to remove/to add/
-Emil
Rather than using "struct file*", use "struct drm_file*" as tag VM tag for BOs. This will pave the way for "struct drm_file*" without any "struct file*" back-pointer.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 3 ++- drivers/gpu/drm/ast/ast_ttm.c | 3 ++- drivers/gpu/drm/bochs/bochs_mm.c | 3 ++- drivers/gpu/drm/cirrus/cirrus_ttm.c | 3 ++- drivers/gpu/drm/drm_gem.c | 8 ++++---- drivers/gpu/drm/mgag200/mgag200_ttm.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_bo.c | 3 ++- drivers/gpu/drm/qxl/qxl_ttm.c | 3 ++- drivers/gpu/drm/radeon/radeon_ttm.c | 3 ++- 9 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index b7742e6..58099c9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -225,7 +225,8 @@ static int amdgpu_verify_access(struct ttm_buffer_object *bo, struct file *filp)
if (amdgpu_ttm_tt_get_usermm(bo->ttm)) return -EPERM; - return drm_vma_node_verify_access(&rbo->gem_base.vma_node, filp); + return drm_vma_node_verify_access(&rbo->gem_base.vma_node, + filp->private_data); }
static void amdgpu_move_null(struct ttm_buffer_object *bo, diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c index b29a412..608df4c 100644 --- a/drivers/gpu/drm/ast/ast_ttm.c +++ b/drivers/gpu/drm/ast/ast_ttm.c @@ -150,7 +150,8 @@ static int ast_bo_verify_access(struct ttm_buffer_object *bo, struct file *filp) { struct ast_bo *astbo = ast_bo(bo);
- return drm_vma_node_verify_access(&astbo->gem.vma_node, filp); + return drm_vma_node_verify_access(&astbo->gem.vma_node, + filp->private_data); }
static int ast_ttm_io_mem_reserve(struct ttm_bo_device *bdev, diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c index 5c5638a..269cfca 100644 --- a/drivers/gpu/drm/bochs/bochs_mm.c +++ b/drivers/gpu/drm/bochs/bochs_mm.c @@ -128,7 +128,8 @@ static int bochs_bo_verify_access(struct ttm_buffer_object *bo, { struct bochs_bo *bochsbo = bochs_bo(bo);
- return drm_vma_node_verify_access(&bochsbo->gem.vma_node, filp); + return drm_vma_node_verify_access(&bochsbo->gem.vma_node, + filp->private_data); }
static int bochs_ttm_io_mem_reserve(struct ttm_bo_device *bdev, diff --git a/drivers/gpu/drm/cirrus/cirrus_ttm.c b/drivers/gpu/drm/cirrus/cirrus_ttm.c index 1cc9ee6..bb2438d 100644 --- a/drivers/gpu/drm/cirrus/cirrus_ttm.c +++ b/drivers/gpu/drm/cirrus/cirrus_ttm.c @@ -150,7 +150,8 @@ static int cirrus_bo_verify_access(struct ttm_buffer_object *bo, struct file *fi { struct cirrus_bo *cirrusbo = cirrus_bo(bo);
- return drm_vma_node_verify_access(&cirrusbo->gem.vma_node, filp); + return drm_vma_node_verify_access(&cirrusbo->gem.vma_node, + filp->private_data); }
static int cirrus_ttm_io_mem_reserve(struct ttm_bo_device *bdev, diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 9134ae1..465bacd 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -257,7 +257,7 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
if (drm_core_check_feature(dev, DRIVER_PRIME)) drm_gem_remove_prime_handles(obj, file_priv); - drm_vma_node_revoke(&obj->vma_node, file_priv->filp); + drm_vma_node_revoke(&obj->vma_node, file_priv);
if (dev->driver->gem_close_object) dev->driver->gem_close_object(obj, file_priv); @@ -372,7 +372,7 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
handle = ret;
- ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp); + ret = drm_vma_node_allow(&obj->vma_node, file_priv); if (ret) goto err_remove;
@@ -386,7 +386,7 @@ drm_gem_handle_create_tail(struct drm_file *file_priv, return 0;
err_revoke: - drm_vma_node_revoke(&obj->vma_node, file_priv->filp); + drm_vma_node_revoke(&obj->vma_node, file_priv); err_remove: spin_lock(&file_priv->table_lock); idr_remove(&file_priv->object_idr, handle); @@ -991,7 +991,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) if (!obj) return -EINVAL;
- if (!drm_vma_node_is_allowed(node, filp)) { + if (!drm_vma_node_is_allowed(node, priv)) { drm_gem_object_unreference_unlocked(obj); return -EACCES; } diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c index 68268e5..919b35f 100644 --- a/drivers/gpu/drm/mgag200/mgag200_ttm.c +++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c @@ -150,7 +150,8 @@ static int mgag200_bo_verify_access(struct ttm_buffer_object *bo, struct file *f { struct mgag200_bo *mgabo = mgag200_bo(bo);
- return drm_vma_node_verify_access(&mgabo->gem.vma_node, filp); + return drm_vma_node_verify_access(&mgabo->gem.vma_node, + filp->private_data); }
static int mgag200_ttm_io_mem_reserve(struct ttm_bo_device *bdev, diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 528bdef..4fea5da 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -1315,7 +1315,8 @@ nouveau_bo_verify_access(struct ttm_buffer_object *bo, struct file *filp) { struct nouveau_bo *nvbo = nouveau_bo(bo);
- return drm_vma_node_verify_access(&nvbo->gem.vma_node, filp); + return drm_vma_node_verify_access(&nvbo->gem.vma_node, + filp->private_data); }
static int diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c index d50c967..3f6dee3 100644 --- a/drivers/gpu/drm/qxl/qxl_ttm.c +++ b/drivers/gpu/drm/qxl/qxl_ttm.c @@ -210,7 +210,8 @@ static int qxl_verify_access(struct ttm_buffer_object *bo, struct file *filp) { struct qxl_bo *qbo = to_qxl_bo(bo);
- return drm_vma_node_verify_access(&qbo->gem_base.vma_node, filp); + return drm_vma_node_verify_access(&qbo->gem_base.vma_node, + filp->private_data); }
static int qxl_ttm_io_mem_reserve(struct ttm_bo_device *bdev, diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index ffdad81..30a1994 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -237,7 +237,8 @@ static int radeon_verify_access(struct ttm_buffer_object *bo, struct file *filp)
if (radeon_ttm_tt_has_userptr(bo->ttm)) return -EPERM; - return drm_vma_node_verify_access(&rbo->gem_base.vma_node, filp); + return drm_vma_node_verify_access(&rbo->gem_base.vma_node, + filp->private_data); }
static void radeon_move_null(struct ttm_buffer_object *bo,
On Wed, Aug 03, 2016 at 08:04:29PM +0200, David Herrmann wrote:
Rather than using "struct file*", use "struct drm_file*" as tag VM tag for BOs. This will pave the way for "struct drm_file*" without any "struct file*" back-pointer.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
Ok, the danger of untyped is having to check each and trying to spot any missed conversions.
Couldn't find a mistake or omission, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On Wed, Aug 03, 2016 at 08:08:19PM +0100, Chris Wilson wrote:
On Wed, Aug 03, 2016 at 08:04:29PM +0200, David Herrmann wrote:
Rather than using "struct file*", use "struct drm_file*" as tag VM tag for BOs. This will pave the way for "struct drm_file*" without any "struct file*" back-pointer.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
Ok, the danger of untyped is having to check each and trying to spot any missed conversions.
Hm yeah ... any reason we can't just switch to struct drm_file instead? At least I can't come up with any other use case where we might supply something else, and it's always nice to enlist the compiler for a little bit of help. -Daniel
Couldn't find a mistake or omission, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
-- Chris Wilson, Intel Open Source Technology Centre
Hey
On Thu, Aug 4, 2016 at 10:16 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Aug 03, 2016 at 08:08:19PM +0100, Chris Wilson wrote:
On Wed, Aug 03, 2016 at 08:04:29PM +0200, David Herrmann wrote:
Rather than using "struct file*", use "struct drm_file*" as tag VM tag for BOs. This will pave the way for "struct drm_file*" without any "struct file*" back-pointer.
Signed-off-by: David Herrmann dh.herrmann@gmail.com
Ok, the danger of untyped is having to check each and trying to spot any missed conversions.
Hm yeah ... any reason we can't just switch to struct drm_file instead? At least I can't come up with any other use case where we might supply something else, and it's always nice to enlist the compiler for a little bit of help.
Fair enough. Will change this. Will send v2 once I'm back from vacation.
Thanks David
Rather than accessing priv->filp->f_cred, use priv->pid->task->creds. We want to get rid of "priv->filp", so lets avoid it if possible.
Since we already are in an rcu-read-side, we can use __task_cred() rather than task_cred_xxx().
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 247ba2b..1df2d33 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -99,7 +99,7 @@ int drm_clients_info(struct seq_file *m, void *data)
rcu_read_lock(); /* locks pid_task()->comm */ task = pid_task(priv->pid, PIDTYPE_PID); - uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID; + uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID; seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n", task ? task->comm : "<unknown>", pid_vnr(priv->pid),
On Wed, Aug 03, 2016 at 08:04:30PM +0200, David Herrmann wrote:
Rather than accessing priv->filp->f_cred, use priv->pid->task->creds. We want to get rid of "priv->filp", so lets avoid it if possible.
Since we already are in an rcu-read-side, we can use __task_cred() rather than task_cred_xxx().
Signed-off-by: David Herrmann dh.herrmann@gmail.com
drivers/gpu/drm/drm_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 247ba2b..1df2d33 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -99,7 +99,7 @@ int drm_clients_info(struct seq_file *m, void *data)
rcu_read_lock(); /* locks pid_task()->comm */ task = pid_task(priv->pid, PIDTYPE_PID);
uid = priv->filp ? priv->filp->f_cred->euid : GLOBAL_ROOT_UID;
uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
Squash this into patch 2, so the poor reader doesn't have to learn about f_cred. -Chris
We don't want anyone but legacy DRM1 code to use drm_file.filp. Especially for in-kernel contexts, this might be set to NULL, so lets make sure no-one accesses it, ever.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_bufs.c | 7 ++++--- drivers/gpu/drm/drm_fops.c | 2 +- include/drm/drmP.h | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index c3a12cd..d2803de 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -1456,7 +1456,7 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_HAVE_DMA)) return -EINVAL;
- if (!dma) + if (!dma || !file_priv->legacy_filp) return -EINVAL;
spin_lock(&dev->buf_lock); @@ -1478,12 +1478,13 @@ int drm_legacy_mapbufs(struct drm_device *dev, void *data, retcode = -EINVAL; goto done; } - virtual = vm_mmap(file_priv->filp, 0, map->size, + virtual = vm_mmap(file_priv->legacy_filp, 0, map->size, PROT_READ | PROT_WRITE, MAP_SHARED, token); } else { - virtual = vm_mmap(file_priv->filp, 0, dma->byte_count, + virtual = vm_mmap(file_priv->legacy_filp, 0, + dma->byte_count, PROT_READ | PROT_WRITE, MAP_SHARED, 0); } diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index e9d66f5..69ef23c 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -198,7 +198,7 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) return -ENOMEM;
filp->private_data = priv; - priv->filp = filp; + priv->legacy_filp = filp; priv->pid = get_pid(task_pid(current)); priv->minor = minor;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0f69f56..2197ab1 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -330,7 +330,7 @@ struct drm_file { /** Lock for synchronization of access to object_idr. */ spinlock_t table_lock;
- struct file *filp; + struct file *legacy_filp; /* might be NULL! */ void *driver_priv;
struct drm_master *master; /* master this node is currently associated with
Rather than doing drm_file allocation/destruction right in the fops, lets provide separate helpers. This decouples drm_file management from the still-mandatory drm-fops. It prepares for use of drm_file without the fops, both by possible separate fops implementations and APIs (not that I am aware of any such plans), and more importantly from in-kernel use where no real file is available.
Signed-off-by: David Herrmann dh.herrmann@gmail.com --- drivers/gpu/drm/drm_drv.c | 135 +++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_fops.c | 132 +++------------------------------------- drivers/gpu/drm/drm_internal.h | 4 ++ 3 files changed, 147 insertions(+), 124 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 57ce973..9ab0016 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -95,6 +95,141 @@ void drm_ut_debug_printk(const char *function_name, const char *format, ...) } EXPORT_SYMBOL(drm_ut_debug_printk);
+/** + * drm_file_alloc - allocate file context + * @minor: minor to allocate on + * + * This allocates a new DRM file context. It is not linked into any context and + * can be used by the caller freely. Note that the context keeps a pointer to + * @minor, so it must be freed before @minor is. + * + * The legacy paths might require the drm_global_mutex to be held. + * + * RETURNS: + * Pointer to newly allocated context, ERR_PTR on failure. + */ +struct drm_file *drm_file_alloc(struct drm_minor *minor) +{ + struct drm_device *dev = minor->dev; + struct drm_file *file; + int ret; + + file = kzalloc(sizeof(*file), GFP_KERNEL); + if (!file) + return ERR_PTR(-ENOMEM); + + file->pid = get_pid(task_pid(current)); + file->minor = minor; + file->authenticated = capable(CAP_SYS_ADMIN); /* legacy compat */ + INIT_LIST_HEAD(&file->lhead); + INIT_LIST_HEAD(&file->fbs); + mutex_init(&file->fbs_lock); + INIT_LIST_HEAD(&file->blobs); + INIT_LIST_HEAD(&file->pending_event_list); + INIT_LIST_HEAD(&file->event_list); + init_waitqueue_head(&file->event_wait); + file->event_space = 4096; /* set aside 4k for event buffer */ + mutex_init(&file->event_read_lock); + + if (drm_core_check_feature(dev, DRIVER_GEM)) + drm_gem_open(dev, file); + if (drm_core_check_feature(dev, DRIVER_PRIME)) + drm_prime_init_file_private(&file->prime); + + if (dev->driver->open) { + ret = dev->driver->open(dev, file); + if (ret < 0) + goto out_prime_destroy; + } + + if (drm_is_primary_client(file)) { + ret = drm_master_open(file); + if (ret) + goto out_close; + } + + return file; + +out_close: + if (dev->driver->postclose) + dev->driver->postclose(dev, file); +out_prime_destroy: + if (drm_core_check_feature(dev, DRIVER_PRIME)) + drm_prime_destroy_file_private(&file->prime); + if (drm_core_check_feature(dev, DRIVER_GEM)) + drm_gem_release(dev, file); + put_pid(file->pid); + kfree(file); + return ERR_PTR(ret); +} + +/** + * drm_file_free - free file context + * @file: context to free, or NULL + * + * This destroys and deallocates a DRM file context previously allocated via + * drm_file_alloc(). The caller must make sure to unlink it from any contexts + * before calling this. + * + * The legacy paths might require the drm_global_mutex to be held. + * + * If NULL is passed, this is a no-op. + * + * RETURNS: + * 0 on success, or error code on failure. + */ +void drm_file_free(struct drm_file *file) +{ + struct drm_pending_event *e; + struct drm_device *dev; + + if (!file) + return; + + dev = file->minor->dev; + + if (dev->driver->preclose) + dev->driver->preclose(dev, file); + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + drm_legacy_lock_release(dev, file->legacy_filp); + if (drm_core_check_feature(dev, DRIVER_HAVE_DMA)) + drm_legacy_reclaim_buffers(dev, file); + + spin_lock_irq(&dev->event_lock); + while ((e = list_first_entry_or_null(&file->pending_event_list, + struct drm_pending_event, + pending_link))) { + list_del(&e->pending_link); + e->file_priv = NULL; + } + while ((e = list_first_entry_or_null(&file->event_list, + struct drm_pending_event, link))) { + list_del(&e->link); + kfree(e); + } + spin_unlock_irq(&dev->event_lock); + + drm_legacy_ctxbitmap_flush(dev, file); + + if (drm_core_check_feature(dev, DRIVER_MODESET)) { + drm_fb_release(file); + drm_property_destroy_user_blobs(dev, file); + } + + if (drm_core_check_feature(dev, DRIVER_GEM)) + drm_gem_release(dev, file); + if (drm_is_primary_client(file)) + drm_master_release(file); + if (dev->driver->postclose) + dev->driver->postclose(dev, file); + if (drm_core_check_feature(dev, DRIVER_PRIME)) + drm_prime_destroy_file_private(&file->prime); + + WARN_ON(!list_empty(&file->event_list)); + put_pid(file->pid); + kfree(file); +} + /* * DRM Minors * A DRM device can provide several char-dev interfaces on the DRM-Major. Each diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 69ef23c..359c636 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -182,7 +182,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) { struct drm_device *dev = minor->dev; struct drm_file *priv; - int ret;
if (filp->f_flags & O_EXCL) return -EBUSY; /* No exclusive opens */ @@ -193,47 +192,12 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor->index);
- priv = kzalloc(sizeof(*priv), GFP_KERNEL); - if (!priv) - return -ENOMEM; + priv = drm_file_alloc(minor); + if (IS_ERR(priv)) + return PTR_ERR(priv);
- filp->private_data = priv; priv->legacy_filp = filp; - priv->pid = get_pid(task_pid(current)); - priv->minor = minor; - - /* for compatibility root is always authenticated */ - priv->authenticated = capable(CAP_SYS_ADMIN); - priv->lock_count = 0; - - INIT_LIST_HEAD(&priv->lhead); - INIT_LIST_HEAD(&priv->fbs); - mutex_init(&priv->fbs_lock); - INIT_LIST_HEAD(&priv->blobs); - INIT_LIST_HEAD(&priv->pending_event_list); - INIT_LIST_HEAD(&priv->event_list); - init_waitqueue_head(&priv->event_wait); - priv->event_space = 4096; /* set aside 4k for event buffer */ - - mutex_init(&priv->event_read_lock); - - if (drm_core_check_feature(dev, DRIVER_GEM)) - drm_gem_open(dev, priv); - - if (drm_core_check_feature(dev, DRIVER_PRIME)) - drm_prime_init_file_private(&priv->prime); - - if (dev->driver->open) { - ret = dev->driver->open(dev, priv); - if (ret < 0) - goto out_prime_destroy; - } - - if (drm_is_primary_client(priv)) { - ret = drm_master_open(priv); - if (ret) - goto out_close; - } + filp->private_data = priv;
mutex_lock(&dev->filelist_mutex); list_add(&priv->lhead, &dev->filelist); @@ -260,43 +224,6 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) #endif
return 0; - -out_close: - if (dev->driver->postclose) - dev->driver->postclose(dev, priv); -out_prime_destroy: - if (drm_core_check_feature(dev, DRIVER_PRIME)) - drm_prime_destroy_file_private(&priv->prime); - if (drm_core_check_feature(dev, DRIVER_GEM)) - drm_gem_release(dev, priv); - put_pid(priv->pid); - kfree(priv); - filp->private_data = NULL; - return ret; -} - -static void drm_events_release(struct drm_file *file_priv) -{ - struct drm_device *dev = file_priv->minor->dev; - struct drm_pending_event *e, *et; - unsigned long flags; - - spin_lock_irqsave(&dev->event_lock, flags); - - /* Unlink pending events */ - list_for_each_entry_safe(e, et, &file_priv->pending_event_list, - pending_link) { - list_del(&e->pending_link); - e->file_priv = NULL; - } - - /* Remove unconsumed events */ - list_for_each_entry_safe(e, et, &file_priv->event_list, link) { - list_del(&e->link); - kfree(e); - } - - spin_unlock_irqrestore(&dev->event_lock, flags); }
/* @@ -370,59 +297,16 @@ int drm_release(struct inode *inode, struct file *filp)
mutex_lock(&drm_global_mutex);
- DRM_DEBUG("open_count = %d\n", dev->open_count); - - mutex_lock(&dev->filelist_mutex); - list_del(&file_priv->lhead); - mutex_unlock(&dev->filelist_mutex); - - if (dev->driver->preclose) - dev->driver->preclose(dev, file_priv); - - /* ======================================================== - * Begin inline drm_release - */ - DRM_DEBUG("pid = %d, device = 0x%lx, open_count = %d\n", task_pid_nr(current), (long)old_encode_dev(file_priv->minor->kdev->devt), dev->open_count);
- if (!drm_core_check_feature(dev, DRIVER_MODESET)) - drm_legacy_lock_release(dev, filp); - - if (drm_core_check_feature(dev, DRIVER_HAVE_DMA)) - drm_legacy_reclaim_buffers(dev, file_priv); - - drm_events_release(file_priv); - - if (drm_core_check_feature(dev, DRIVER_MODESET)) { - drm_fb_release(file_priv); - drm_property_destroy_user_blobs(dev, file_priv); - } - - if (drm_core_check_feature(dev, DRIVER_GEM)) - drm_gem_release(dev, file_priv); - - drm_legacy_ctxbitmap_flush(dev, file_priv); - - if (drm_is_primary_client(file_priv)) - drm_master_release(file_priv); - - if (dev->driver->postclose) - dev->driver->postclose(dev, file_priv); - - if (drm_core_check_feature(dev, DRIVER_PRIME)) - drm_prime_destroy_file_private(&file_priv->prime); - - WARN_ON(!list_empty(&file_priv->event_list)); - - put_pid(file_priv->pid); - kfree(file_priv); + mutex_lock(&dev->filelist_mutex); + list_del(&file_priv->lhead); + mutex_unlock(&dev->filelist_mutex);
- /* ======================================================== - * End inline drm_release - */ + drm_file_free(file_priv);
if (!--dev->open_count) { drm_lastclose(dev); diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index b86dc9b..9b66cc4 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -28,6 +28,10 @@ extern unsigned int drm_timestamp_monotonic; extern struct mutex drm_global_mutex; void drm_lastclose(struct drm_device *dev);
+/* drm_drv.c */ +struct drm_file *drm_file_alloc(struct drm_minor *minor); +void drm_file_free(struct drm_file *file); + /* drm_pci.c */ int drm_irq_by_busid(struct drm_device *dev, void *data, struct drm_file *file_priv);
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
David Herrmann -
Emil Velikov -
Frank Binns