| Message ID | 20191106100716.18181-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm: Expose a method for creating anonymous struct file around drm_minor | expand |
On Wed, Nov 06, 2019 at 10:07:16AM +0000, Chris Wilson wrote: > Currently the drm_prime mmap fallback uses a mock struct file to provide > the file pointer into the backend mmap routine. Now that we can create > fully fledged anonymous struct file around the drm device, put it to > use. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/drm_prime.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 0814211b0f3f..5faa63713ec8 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -709,8 +709,7 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); > */ > int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > { > - struct drm_file *priv; > - struct file *fil; > + struct file *file; > int ret; > > if (obj->funcs && obj->funcs->mmap) { obj->funcs->mmap is the new way of doing this (and hopefully finally something clean), I'd really like to retire the below hack outright. Plus I'm not sure why you need an anon inode here? If a driver needs this for unmap_mapping_range or similar I think it'd be better to try and make something work cleanly for obj->funcs->mmap. -Daniel > @@ -722,30 +721,21 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > return 0; > } > > - priv = kzalloc(sizeof(*priv), GFP_KERNEL); > - fil = kzalloc(sizeof(*fil), GFP_KERNEL); > - if (!priv || !fil) { > - ret = -ENOMEM; > - goto out; > - } > + file = anon_drm_getfile(obj->dev->primary, O_RDWR); > + if (IS_ERR(file)) > + return PTR_ERR(file); > > - /* Used by drm_gem_mmap() to lookup the GEM object */ > - priv->minor = obj->dev->primary; > - fil->private_data = priv; > - > - ret = drm_vma_node_allow(&obj->vma_node, priv); > + ret = drm_vma_node_allow(&obj->vma_node, file->private_data); > if (ret) > goto out; > > vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > > - ret = obj->dev->driver->fops->mmap(fil, vma); > + ret = file->f_op->mmap(file, vma); > > - drm_vma_node_revoke(&obj->vma_node, priv); > + drm_vma_node_revoke(&obj->vma_node, file->private_data); > out: > - kfree(priv); > - kfree(fil); > - > + fput(file); > return ret; > } > EXPORT_SYMBOL(drm_gem_prime_mmap); > -- > 2.24.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoting Daniel Vetter (2019-11-06 10:21:57) > On Wed, Nov 06, 2019 at 10:07:16AM +0000, Chris Wilson wrote: > > Currently the drm_prime mmap fallback uses a mock struct file to provide > > the file pointer into the backend mmap routine. Now that we can create > > fully fledged anonymous struct file around the drm device, put it to > > use. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/drm_prime.c | 26 ++++++++------------------ > > 1 file changed, 8 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > > index 0814211b0f3f..5faa63713ec8 100644 > > --- a/drivers/gpu/drm/drm_prime.c > > +++ b/drivers/gpu/drm/drm_prime.c > > @@ -709,8 +709,7 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); > > */ > > int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > { > > - struct drm_file *priv; > > - struct file *fil; > > + struct file *file; > > int ret; > > > > if (obj->funcs && obj->funcs->mmap) { > > obj->funcs->mmap is the new way of doing this (and hopefully finally > something clean), I'd really like to retire the below hack outright. > > Plus I'm not sure why you need an anon inode here? If a driver needs this > for unmap_mapping_range or similar I think it'd be better to try and make > something work cleanly for obj->funcs->mmap. It's faking one currently. If the fake is not good enough, you are playing whack-a-mole until you finally do create a fully fledged file. If you are going to the trouble of having to create a struct file to provide to the fallback routines, might as well avoid stinky code :) -Chris
On Wed, Nov 6, 2019 at 11:45 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Daniel Vetter (2019-11-06 10:21:57) > > On Wed, Nov 06, 2019 at 10:07:16AM +0000, Chris Wilson wrote: > > > Currently the drm_prime mmap fallback uses a mock struct file to provide > > > the file pointer into the backend mmap routine. Now that we can create > > > fully fledged anonymous struct file around the drm device, put it to > > > use. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > --- > > > drivers/gpu/drm/drm_prime.c | 26 ++++++++------------------ > > > 1 file changed, 8 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > > > index 0814211b0f3f..5faa63713ec8 100644 > > > --- a/drivers/gpu/drm/drm_prime.c > > > +++ b/drivers/gpu/drm/drm_prime.c > > > @@ -709,8 +709,7 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); > > > */ > > > int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > > { > > > - struct drm_file *priv; > > > - struct file *fil; > > > + struct file *file; > > > int ret; > > > > > > if (obj->funcs && obj->funcs->mmap) { > > > > obj->funcs->mmap is the new way of doing this (and hopefully finally > > something clean), I'd really like to retire the below hack outright. > > > > Plus I'm not sure why you need an anon inode here? If a driver needs this > > for unmap_mapping_range or similar I think it'd be better to try and make > > something work cleanly for obj->funcs->mmap. > > It's faking one currently. If the fake is not good enough, you are > playing whack-a-mole until you finally do create a fully fledged file. > > If you are going to the trouble of having to create a struct file to > provide to the fallback routines, might as well avoid stinky code :) We're currently not faking the inode at all, we're just using the one that comes with the dma-buf. So distinct from the drm_device file, and hence unmap_mapping_range won't work (or at least doing that on the drm_device inode wont shoot down the ptes for redirected dma-buf mmaps). obj->funcs->mmap has the same issue. But since all current users of this don't expect unmap_mapping_range to work correctly, it's not an real issue. If that changes then imo we should fix up the obj->funcs->mmap path to have the correct inode, not the deprecated path you're updating here. But since there's no patch 4 in this series to start using this for i915 or someone else, I'm not seeing the point. Or am I blind? At least slightly confused, -Daniel
Quoting Daniel Vetter (2019-11-06 13:06:26) > On Wed, Nov 6, 2019 at 11:45 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Quoting Daniel Vetter (2019-11-06 10:21:57) > > > On Wed, Nov 06, 2019 at 10:07:16AM +0000, Chris Wilson wrote: > > > > Currently the drm_prime mmap fallback uses a mock struct file to provide > > > > the file pointer into the backend mmap routine. Now that we can create > > > > fully fledged anonymous struct file around the drm device, put it to > > > > use. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > --- > > > > drivers/gpu/drm/drm_prime.c | 26 ++++++++------------------ > > > > 1 file changed, 8 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > > > > index 0814211b0f3f..5faa63713ec8 100644 > > > > --- a/drivers/gpu/drm/drm_prime.c > > > > +++ b/drivers/gpu/drm/drm_prime.c > > > > @@ -709,8 +709,7 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); > > > > */ > > > > int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > > > { > > > > - struct drm_file *priv; > > > > - struct file *fil; > > > > + struct file *file; > > > > int ret; > > > > > > > > if (obj->funcs && obj->funcs->mmap) { > > > > > > obj->funcs->mmap is the new way of doing this (and hopefully finally > > > something clean), I'd really like to retire the below hack outright. > > > > > > Plus I'm not sure why you need an anon inode here? If a driver needs this > > > for unmap_mapping_range or similar I think it'd be better to try and make > > > something work cleanly for obj->funcs->mmap. > > > > It's faking one currently. If the fake is not good enough, you are > > playing whack-a-mole until you finally do create a fully fledged file. > > > > If you are going to the trouble of having to create a struct file to > > provide to the fallback routines, might as well avoid stinky code :) > > We're currently not faking the inode at all, we're just using the one > that comes with the dma-buf. So distinct from the drm_device file, and > hence unmap_mapping_range won't work (or at least doing that on the > drm_device inode wont shoot down the ptes for redirected dma-buf > mmaps). obj->funcs->mmap has the same issue. > > But since all current users of this don't expect unmap_mapping_range > to work correctly, it's not an real issue. If that changes then imo we > should fix up the obj->funcs->mmap path to have the correct inode, not > the deprecated path you're updating here. But since there's no patch 4 > in this series to start using this for i915 or someone else, I'm not > seeing the point. There's a bug in anon_drm_inode() in that it requires an extra: + /* Everyone shares a single global address space */ + file->f_mapping = dev->anon_inode->i_mapping; I'm up to 5 patches now, but only i915/selftests & this here fallback as direct users. -Chris
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0814211b0f3f..5faa63713ec8 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -709,8 +709,7 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); */ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) { - struct drm_file *priv; - struct file *fil; + struct file *file; int ret; if (obj->funcs && obj->funcs->mmap) { @@ -722,30 +721,21 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) return 0; } - priv = kzalloc(sizeof(*priv), GFP_KERNEL); - fil = kzalloc(sizeof(*fil), GFP_KERNEL); - if (!priv || !fil) { - ret = -ENOMEM; - goto out; - } + file = anon_drm_getfile(obj->dev->primary, O_RDWR); + if (IS_ERR(file)) + return PTR_ERR(file); - /* Used by drm_gem_mmap() to lookup the GEM object */ - priv->minor = obj->dev->primary; - fil->private_data = priv; - - ret = drm_vma_node_allow(&obj->vma_node, priv); + ret = drm_vma_node_allow(&obj->vma_node, file->private_data); if (ret) goto out; vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); - ret = obj->dev->driver->fops->mmap(fil, vma); + ret = file->f_op->mmap(file, vma); - drm_vma_node_revoke(&obj->vma_node, priv); + drm_vma_node_revoke(&obj->vma_node, file->private_data); out: - kfree(priv); - kfree(fil); - + fput(file); return ret; } EXPORT_SYMBOL(drm_gem_prime_mmap);
Currently the drm_prime mmap fallback uses a mock struct file to provide the file pointer into the backend mmap routine. Now that we can create fully fledged anonymous struct file around the drm device, put it to use. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/drm_prime.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)