| Message ID | 1387535815-31880-1-git-send-email-inki.dae@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ok I've stumbled over the exynos mmap stuff again while cleaning up drm legacy cruft and I just don't get what you're doing and why exactly exynos needs to be special. _All_ other drm drivers happily get along with the vma offset manger stuff to handle mmaps, but somehow exynos does something really crazy. Can you please explain the design justification for this and why switching to the standard gem mmap support isn't possible? Thanks, Daniel On Fri, Dec 20, 2013 at 11:36 AM, Inki Dae <inki.dae@samsung.com> wrote: > This patch resolves potential deadlock issue that can be incurred > by changing file->f_op and filp->private_data to exynos specific > mapper ops and gem object temporarily. > > To resolve this issue, this patch creates a new anon file dedicated > to exynos specific mmaper, and making it used instead of existing one. > > Signed-off-by: Inki Dae <inki.dae@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_drv.c | 21 +++++++++ > drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + > drivers/gpu/drm/exynos/exynos_drm_gem.c | 74 ++++++------------------------- > drivers/gpu/drm/exynos/exynos_drm_gem.h | 3 ++ > 4 files changed, 38 insertions(+), 61 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > index 22b8f5e..b5e5957 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -14,6 +14,8 @@ > #include <drm/drmP.h> > #include <drm/drm_crtc_helper.h> > > +#include <linux/anon_inodes.h> > + > #include <drm/exynos_drm.h> > > #include "exynos_drm_drv.h" > @@ -150,9 +152,14 @@ static int exynos_drm_unload(struct drm_device *dev) > return 0; > } > > +static const struct file_operations exynos_drm_gem_fops = { > + .mmap = exynos_drm_gem_mmap_buffer, > +}; > + > static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) > { > struct drm_exynos_file_private *file_priv; > + struct file *anon_filp; > int ret; > > file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL); > @@ -167,6 +174,16 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) > file->driver_priv = NULL; > } > > + anon_filp = anon_inode_getfile("exynos_gem", &exynos_drm_gem_fops, > + NULL, 0); > + if (IS_ERR(anon_filp)) { > + kfree(file_priv); > + return PTR_ERR(anon_filp); > + } > + > + anon_filp->f_mode = FMODE_READ | FMODE_WRITE; > + file_priv->anon_filp = anon_filp; > + > return ret; > } > > @@ -179,6 +196,7 @@ static void exynos_drm_preclose(struct drm_device *dev, > static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) > { > struct exynos_drm_private *private = dev->dev_private; > + struct drm_exynos_file_private *file_priv; > struct drm_pending_vblank_event *v, *vt; > struct drm_pending_event *e, *et; > unsigned long flags; > @@ -204,6 +222,9 @@ static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) > } > spin_unlock_irqrestore(&dev->event_lock, flags); > > + file_priv = file->driver_priv; > + if (file_priv->anon_filp) > + fput(file_priv->anon_filp); > > kfree(file->driver_priv); > file->driver_priv = NULL; > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h > index eaa1966..0eaf5a2 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h > @@ -226,6 +226,7 @@ struct exynos_drm_ipp_private { > struct drm_exynos_file_private { > struct exynos_drm_g2d_private *g2d_priv; > struct exynos_drm_ipp_private *ipp_priv; > + struct file *anon_filp; > }; > > /* > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c > index 1ade191..49b8c9b 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c > @@ -338,46 +338,22 @@ int exynos_drm_gem_map_offset_ioctl(struct drm_device *dev, void *data, > &args->offset); > } > > -static struct drm_file *exynos_drm_find_drm_file(struct drm_device *drm_dev, > - struct file *filp) > -{ > - struct drm_file *file_priv; > - > - /* find current process's drm_file from filelist. */ > - list_for_each_entry(file_priv, &drm_dev->filelist, lhead) > - if (file_priv->filp == filp) > - return file_priv; > - > - WARN_ON(1); > - > - return ERR_PTR(-EFAULT); > -} > - > -static int exynos_drm_gem_mmap_buffer(struct file *filp, > +int exynos_drm_gem_mmap_buffer(struct file *filp, > struct vm_area_struct *vma) > { > struct drm_gem_object *obj = filp->private_data; > struct exynos_drm_gem_obj *exynos_gem_obj = to_exynos_gem_obj(obj); > struct drm_device *drm_dev = obj->dev; > struct exynos_drm_gem_buf *buffer; > - struct drm_file *file_priv; > unsigned long vm_size; > int ret; > > + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); > + > vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP; > vma->vm_private_data = obj; > vma->vm_ops = drm_dev->driver->gem_vm_ops; > > - /* restore it to driver's fops. */ > - filp->f_op = fops_get(drm_dev->driver->fops); > - > - file_priv = exynos_drm_find_drm_file(drm_dev, filp); > - if (IS_ERR(file_priv)) > - return PTR_ERR(file_priv); > - > - /* restore it to drm_file. */ > - filp->private_data = file_priv; > - > update_vm_cache_attr(exynos_gem_obj, vma); > > vm_size = vma->vm_end - vma->vm_start; > @@ -411,15 +387,13 @@ static int exynos_drm_gem_mmap_buffer(struct file *filp, > return 0; > } > > -static const struct file_operations exynos_drm_gem_fops = { > - .mmap = exynos_drm_gem_mmap_buffer, > -}; > - > int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv) > { > + struct drm_exynos_file_private *exynos_file_priv; > struct drm_exynos_gem_mmap *args = data; > struct drm_gem_object *obj; > + struct file *anon_filp; > unsigned long addr; > > if (!(dev->driver->driver_features & DRIVER_GEM)) { > @@ -427,47 +401,25 @@ int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, > return -ENODEV; > } > > + mutex_lock(&dev->struct_mutex); > + > obj = drm_gem_object_lookup(dev, file_priv, args->handle); > if (!obj) { > DRM_ERROR("failed to lookup gem object.\n"); > + mutex_unlock(&dev->struct_mutex); > return -EINVAL; > } > > - /* > - * We have to use gem object and its fops for specific mmaper, > - * but vm_mmap() can deliver only filp. So we have to change > - * filp->f_op and filp->private_data temporarily, then restore > - * again. So it is important to keep lock until restoration the > - * settings to prevent others from misuse of filp->f_op or > - * filp->private_data. > - */ > - mutex_lock(&dev->struct_mutex); > - > - /* > - * Set specific mmper's fops. And it will be restored by > - * exynos_drm_gem_mmap_buffer to dev->driver->fops. > - * This is used to call specific mapper temporarily. > - */ > - file_priv->filp->f_op = &exynos_drm_gem_fops; > - > - /* > - * Set gem object to private_data so that specific mmaper > - * can get the gem object. And it will be restored by > - * exynos_drm_gem_mmap_buffer to drm_file. > - */ > - file_priv->filp->private_data = obj; > + exynos_file_priv = file_priv->driver_priv; > + anon_filp = exynos_file_priv->anon_filp; > + anon_filp->private_data = obj; > > - addr = vm_mmap(file_priv->filp, 0, args->size, > - PROT_READ | PROT_WRITE, MAP_SHARED, 0); > + addr = vm_mmap(anon_filp, 0, args->size, PROT_READ | PROT_WRITE, > + MAP_SHARED, 0); > > drm_gem_object_unreference(obj); > > if (IS_ERR_VALUE(addr)) { > - /* check filp->f_op, filp->private_data are restored */ > - if (file_priv->filp->f_op == &exynos_drm_gem_fops) { > - file_priv->filp->f_op = fops_get(dev->driver->fops); > - file_priv->filp->private_data = file_priv; > - } > mutex_unlock(&dev->struct_mutex); > return (int)addr; > } > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h > index 702ec3a..fde860c 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h > @@ -122,6 +122,9 @@ int exynos_drm_gem_map_offset_ioctl(struct drm_device *dev, void *data, > int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv); > > +int exynos_drm_gem_mmap_buffer(struct file *filp, > + struct vm_area_struct *vma); > + > /* map user space allocated by malloc to pages. */ > int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv); > -- > 1.7.9.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2014? 09? 10? 18:01, Daniel Vetter wrote: > Ok I've stumbled over the exynos mmap stuff again while cleaning up > drm legacy cruft and I just don't get what you're doing and why > exactly exynos needs to be special. > > _All_ other drm drivers happily get along with the vma offset manger > stuff to handle mmaps, but somehow exynos does something really crazy. We are also using the vma offset manager stuff. We just added direct mapping interface specific to Exynos additionally. > > Can you please explain the design justification for this and why > switching to the standard gem mmap support isn't possible? As I mentioned above, we are using the standard gem mmap support. However, the standard gem mmap is required for on-demand paging mostly suitable for Desktop. In case of ARM SoC, whole memory region requested by userspace would be allocated once the gem creation interface is called. In this case, it wouldn't need to map userspace with physical page in page fault handler, and the use of the vma offset manager stuff would be unnecessary step. For the same question, Al Viro did, http://lists.freedesktop.org/archives/dri-devel/2013-September/046207.html Is there any issue I am missing , that could be incurred by Exynos codes? Thanks, Inki Dae > > Thanks, Daniel > > > On Fri, Dec 20, 2013 at 11:36 AM, Inki Dae <inki.dae@samsung.com> wrote: >> This patch resolves potential deadlock issue that can be incurred >> by changing file->f_op and filp->private_data to exynos specific >> mapper ops and gem object temporarily. >> >> To resolve this issue, this patch creates a new anon file dedicated >> to exynos specific mmaper, and making it used instead of existing one. >> >> Signed-off-by: Inki Dae <inki.dae@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_drv.c | 21 +++++++++ >> drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + >> drivers/gpu/drm/exynos/exynos_drm_gem.c | 74 ++++++------------------------- >> drivers/gpu/drm/exynos/exynos_drm_gem.h | 3 ++ >> 4 files changed, 38 insertions(+), 61 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> index 22b8f5e..b5e5957 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c >> @@ -14,6 +14,8 @@ >> #include <drm/drmP.h> >> #include <drm/drm_crtc_helper.h> >> >> +#include <linux/anon_inodes.h> >> + >> #include <drm/exynos_drm.h> >> >> #include "exynos_drm_drv.h" >> @@ -150,9 +152,14 @@ static int exynos_drm_unload(struct drm_device *dev) >> return 0; >> } >> >> +static const struct file_operations exynos_drm_gem_fops = { >> + .mmap = exynos_drm_gem_mmap_buffer, >> +}; >> + >> static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) >> { >> struct drm_exynos_file_private *file_priv; >> + struct file *anon_filp; >> int ret; >> >> file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL); >> @@ -167,6 +174,16 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) >> file->driver_priv = NULL; >> } >> >> + anon_filp = anon_inode_getfile("exynos_gem", &exynos_drm_gem_fops, >> + NULL, 0); >> + if (IS_ERR(anon_filp)) { >> + kfree(file_priv); >> + return PTR_ERR(anon_filp); >> + } >> + >> + anon_filp->f_mode = FMODE_READ | FMODE_WRITE; >> + file_priv->anon_filp = anon_filp; >> + >> return ret; >> } >> >> @@ -179,6 +196,7 @@ static void exynos_drm_preclose(struct drm_device *dev, >> static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) >> { >> struct exynos_drm_private *private = dev->dev_private; >> + struct drm_exynos_file_private *file_priv; >> struct drm_pending_vblank_event *v, *vt; >> struct drm_pending_event *e, *et; >> unsigned long flags; >> @@ -204,6 +222,9 @@ static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) >> } >> spin_unlock_irqrestore(&dev->event_lock, flags); >> >> + file_priv = file->driver_priv; >> + if (file_priv->anon_filp) >> + fput(file_priv->anon_filp); >> >> kfree(file->driver_priv); >> file->driver_priv = NULL; >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> index eaa1966..0eaf5a2 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h >> @@ -226,6 +226,7 @@ struct exynos_drm_ipp_private { >> struct drm_exynos_file_private { >> struct exynos_drm_g2d_private *g2d_priv; >> struct exynos_drm_ipp_private *ipp_priv; >> + struct file *anon_filp; >> }; >> >> /* >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c >> index 1ade191..49b8c9b 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c >> @@ -338,46 +338,22 @@ int exynos_drm_gem_map_offset_ioctl(struct drm_device *dev, void *data, >> &args->offset); >> } >> >> -static struct drm_file *exynos_drm_find_drm_file(struct drm_device *drm_dev, >> - struct file *filp) >> -{ >> - struct drm_file *file_priv; >> - >> - /* find current process's drm_file from filelist. */ >> - list_for_each_entry(file_priv, &drm_dev->filelist, lhead) >> - if (file_priv->filp == filp) >> - return file_priv; >> - >> - WARN_ON(1); >> - >> - return ERR_PTR(-EFAULT); >> -} >> - >> -static int exynos_drm_gem_mmap_buffer(struct file *filp, >> +int exynos_drm_gem_mmap_buffer(struct file *filp, >> struct vm_area_struct *vma) >> { >> struct drm_gem_object *obj = filp->private_data; >> struct exynos_drm_gem_obj *exynos_gem_obj = to_exynos_gem_obj(obj); >> struct drm_device *drm_dev = obj->dev; >> struct exynos_drm_gem_buf *buffer; >> - struct drm_file *file_priv; >> unsigned long vm_size; >> int ret; >> >> + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); >> + >> vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP; >> vma->vm_private_data = obj; >> vma->vm_ops = drm_dev->driver->gem_vm_ops; >> >> - /* restore it to driver's fops. */ >> - filp->f_op = fops_get(drm_dev->driver->fops); >> - >> - file_priv = exynos_drm_find_drm_file(drm_dev, filp); >> - if (IS_ERR(file_priv)) >> - return PTR_ERR(file_priv); >> - >> - /* restore it to drm_file. */ >> - filp->private_data = file_priv; >> - >> update_vm_cache_attr(exynos_gem_obj, vma); >> >> vm_size = vma->vm_end - vma->vm_start; >> @@ -411,15 +387,13 @@ static int exynos_drm_gem_mmap_buffer(struct file *filp, >> return 0; >> } >> >> -static const struct file_operations exynos_drm_gem_fops = { >> - .mmap = exynos_drm_gem_mmap_buffer, >> -}; >> - >> int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv) >> { >> + struct drm_exynos_file_private *exynos_file_priv; >> struct drm_exynos_gem_mmap *args = data; >> struct drm_gem_object *obj; >> + struct file *anon_filp; >> unsigned long addr; >> >> if (!(dev->driver->driver_features & DRIVER_GEM)) { >> @@ -427,47 +401,25 @@ int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, >> return -ENODEV; >> } >> >> + mutex_lock(&dev->struct_mutex); >> + >> obj = drm_gem_object_lookup(dev, file_priv, args->handle); >> if (!obj) { >> DRM_ERROR("failed to lookup gem object.\n"); >> + mutex_unlock(&dev->struct_mutex); >> return -EINVAL; >> } >> >> - /* >> - * We have to use gem object and its fops for specific mmaper, >> - * but vm_mmap() can deliver only filp. So we have to change >> - * filp->f_op and filp->private_data temporarily, then restore >> - * again. So it is important to keep lock until restoration the >> - * settings to prevent others from misuse of filp->f_op or >> - * filp->private_data. >> - */ >> - mutex_lock(&dev->struct_mutex); >> - >> - /* >> - * Set specific mmper's fops. And it will be restored by >> - * exynos_drm_gem_mmap_buffer to dev->driver->fops. >> - * This is used to call specific mapper temporarily. >> - */ >> - file_priv->filp->f_op = &exynos_drm_gem_fops; >> - >> - /* >> - * Set gem object to private_data so that specific mmaper >> - * can get the gem object. And it will be restored by >> - * exynos_drm_gem_mmap_buffer to drm_file. >> - */ >> - file_priv->filp->private_data = obj; >> + exynos_file_priv = file_priv->driver_priv; >> + anon_filp = exynos_file_priv->anon_filp; >> + anon_filp->private_data = obj; >> >> - addr = vm_mmap(file_priv->filp, 0, args->size, >> - PROT_READ | PROT_WRITE, MAP_SHARED, 0); >> + addr = vm_mmap(anon_filp, 0, args->size, PROT_READ | PROT_WRITE, >> + MAP_SHARED, 0); >> >> drm_gem_object_unreference(obj); >> >> if (IS_ERR_VALUE(addr)) { >> - /* check filp->f_op, filp->private_data are restored */ >> - if (file_priv->filp->f_op == &exynos_drm_gem_fops) { >> - file_priv->filp->f_op = fops_get(dev->driver->fops); >> - file_priv->filp->private_data = file_priv; >> - } >> mutex_unlock(&dev->struct_mutex); >> return (int)addr; >> } >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h >> index 702ec3a..fde860c 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h >> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h >> @@ -122,6 +122,9 @@ int exynos_drm_gem_map_offset_ioctl(struct drm_device *dev, void *data, >> int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv); >> >> +int exynos_drm_gem_mmap_buffer(struct file *filp, >> + struct vm_area_struct *vma); >> + >> /* map user space allocated by malloc to pages. */ >> int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data, >> struct drm_file *file_priv); >> -- >> 1.7.9.5 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > >
On Thu, Sep 11, 2014 at 11:16:53AM +0900, Inki Dae wrote: > On 2014? 09? 10? 18:01, Daniel Vetter wrote: > > Ok I've stumbled over the exynos mmap stuff again while cleaning up > > drm legacy cruft and I just don't get what you're doing and why > > exactly exynos needs to be special. > > > > _All_ other drm drivers happily get along with the vma offset manger > > stuff to handle mmaps, but somehow exynos does something really crazy. > > We are also using the vma offset manager stuff. We just added direct > mapping interface specific to Exynos additionally. > > > > > Can you please explain the design justification for this and why > > switching to the standard gem mmap support isn't possible? > > As I mentioned above, we are using the standard gem mmap support. > However, the standard gem mmap is required for on-demand paging mostly > suitable for Desktop. In case of ARM SoC, whole memory region requested > by userspace would be allocated once the gem creation interface is > called. In this case, it wouldn't need to map userspace with physical > page in page fault handler, and the use of the vma offset manager stuff > would be unnecessary step. You don't need to do demand paging at all, you can simply put in all the ptes in one go and then never unbind it. So strictly speaking you don't need to roll your own mmap, but otoh other drivers (including i915) do their own special mmap too. And since you now have it you must support it forever anyway. Aside: We have patches floating around for i915 to prefault aggressively, so you're not the only ones who noticed the faulting overhead. ARM SoC really aren't all that special compared to traditional desktop gpus, so if you stumble over such issues please raise them on dri-devel so that we could look into useful generic solutions next time around. > For the same question, Al Viro did, > http://lists.freedesktop.org/archives/dri-devel/2013-September/046207.html > > Is there any issue I am missing , that could be incurred by Exynos codes? I've stumbled over it again because you're reusing the drm_vm_open_locked function, which really should just be an implementation detail of the core drm/gem mmap support. If you want to roll your own mmap (and that's ok, i915 has it and ttm also does it) then imo you should not reuse any of the core mmap code, but implement your own set of vm_ops. You don't need a faul handler for this (since it will never fault), and open/close would just grabbing/dropping a reference of the underlying gem object. Instead of trying to reuse the same vm_ops you use for normal gem mmaps, which just doesn't make a lot of sense to me. If exynos stops using drm_vm_open_locked then I can move it into the new drm_internal.h header since this function really should be private to drm.ko. Thanks, Daniel
Hi, On 09/11/2014 03:22 PM, Daniel Vetter wrote: > On Thu, Sep 11, 2014 at 11:16:53AM +0900, Inki Dae wrote: >> On 2014? 09? 10? 18:01, Daniel Vetter wrote: >>> Ok I've stumbled over the exynos mmap stuff again while cleaning up >>> drm legacy cruft and I just don't get what you're doing and why >>> exactly exynos needs to be special. >>> >>> _All_ other drm drivers happily get along with the vma offset manger >>> stuff to handle mmaps, but somehow exynos does something really crazy. >> >> We are also using the vma offset manager stuff. We just added direct >> mapping interface specific to Exynos additionally. >> >>> >>> Can you please explain the design justification for this and why >>> switching to the standard gem mmap support isn't possible? >> >> As I mentioned above, we are using the standard gem mmap support. >> However, the standard gem mmap is required for on-demand paging mostly >> suitable for Desktop. In case of ARM SoC, whole memory region requested >> by userspace would be allocated once the gem creation interface is >> called. In this case, it wouldn't need to map userspace with physical >> page in page fault handler, and the use of the vma offset manager stuff >> would be unnecessary step. > > You don't need to do demand paging at all, you can simply put in all the > ptes in one go and then never unbind it. So strictly speaking you don't > need to roll your own mmap, but otoh other drivers (including i915) do > their own special mmap too. And since you now have it you must support it > forever anyway. I agree with Daniel. The exynos drm specific mmap ioctl can be substituted to standard gem mmap if exynos mmap is implemented for direct mapping, actually gem cma does it from drm_gem_cma_mmap_obj. Thanks. > > Aside: We have patches floating around for i915 to prefault aggressively, > so you're not the only ones who noticed the faulting overhead. ARM SoC > really aren't all that special compared to traditional desktop gpus, so if > you stumble over such issues please raise them on dri-devel so that we > could look into useful generic solutions next time around. > >> For the same question, Al Viro did, >> http://lists.freedesktop.org/archives/dri-devel/2013-September/046207.html >> >> Is there any issue I am missing , that could be incurred by Exynos codes? > > I've stumbled over it again because you're reusing the drm_vm_open_locked > function, which really should just be an implementation detail of the core > drm/gem mmap support. > > If you want to roll your own mmap (and that's ok, i915 has it and ttm also > does it) then imo you should not reuse any of the core mmap code, but > implement your own set of vm_ops. You don't need a faul handler for this > (since it will never fault), and open/close would just grabbing/dropping a > reference of the underlying gem object. Instead of trying to reuse the > same vm_ops you use for normal gem mmaps, which just doesn't make a lot of > sense to me. > > If exynos stops using drm_vm_open_locked then I can move it into the new > drm_internal.h header since this function really should be private to > drm.ko. > > Thanks, Daniel >
On 2014? 09? 11? 15:22, Daniel Vetter wrote: > On Thu, Sep 11, 2014 at 11:16:53AM +0900, Inki Dae wrote: >> On 2014? 09? 10? 18:01, Daniel Vetter wrote: >>> Ok I've stumbled over the exynos mmap stuff again while cleaning up >>> drm legacy cruft and I just don't get what you're doing and why >>> exactly exynos needs to be special. >>> >>> _All_ other drm drivers happily get along with the vma offset manger >>> stuff to handle mmaps, but somehow exynos does something really crazy. >> >> We are also using the vma offset manager stuff. We just added direct >> mapping interface specific to Exynos additionally. >> >>> >>> Can you please explain the design justification for this and why >>> switching to the standard gem mmap support isn't possible? >> >> As I mentioned above, we are using the standard gem mmap support. >> However, the standard gem mmap is required for on-demand paging mostly >> suitable for Desktop. In case of ARM SoC, whole memory region requested >> by userspace would be allocated once the gem creation interface is >> called. In this case, it wouldn't need to map userspace with physical >> page in page fault handler, and the use of the vma offset manager stuff >> would be unnecessary step. > > You don't need to do demand paging at all, you can simply put in all the > ptes in one go and then never unbind it. So strictly speaking you don't > need to roll your own mmap, but otoh other drivers (including i915) do > their own special mmap too. And since you now have it you must support it > forever anyway. > > Aside: We have patches floating around for i915 to prefault aggressively, > so you're not the only ones who noticed the faulting overhead. ARM SoC > really aren't all that special compared to traditional desktop gpus, so if > you stumble over such issues please raise them on dri-devel so that we > could look into useful generic solutions next time around. > >> For the same question, Al Viro did, >> http://lists.freedesktop.org/archives/dri-devel/2013-September/046207.html >> >> Is there any issue I am missing , that could be incurred by Exynos codes? > > I've stumbled over it again because you're reusing the drm_vm_open_locked > function, which really should just be an implementation detail of the core > drm/gem mmap support. Ah, right. that is critical issue. I shouldn't had used drm_vm_open_locked. Sorry for this. > > If you want to roll your own mmap (and that's ok, i915 has it and ttm also > does it) then imo you should not reuse any of the core mmap code, but > implement your own set of vm_ops. You don't need a faul handler for this > (since it will never fault), and open/close would just grabbing/dropping a > reference of the underlying gem object. Instead of trying to reuse the > same vm_ops you use for normal gem mmaps, which just doesn't make a lot of > sense to me. > > If exynos stops using drm_vm_open_locked then I can move it into the new > drm_internal.h header since this function really should be private to > drm.ko. Sorry for blocking you. I will fix it soon. Thanks, Inki Dae > > Thanks, Daniel >
On 2014? 09? 11? 16:01, Joonyoung Shim wrote: > Hi, > > On 09/11/2014 03:22 PM, Daniel Vetter wrote: >> On Thu, Sep 11, 2014 at 11:16:53AM +0900, Inki Dae wrote: >>> On 2014? 09? 10? 18:01, Daniel Vetter wrote: >>>> Ok I've stumbled over the exynos mmap stuff again while cleaning up >>>> drm legacy cruft and I just don't get what you're doing and why >>>> exactly exynos needs to be special. >>>> >>>> _All_ other drm drivers happily get along with the vma offset manger >>>> stuff to handle mmaps, but somehow exynos does something really crazy. >>> >>> We are also using the vma offset manager stuff. We just added direct >>> mapping interface specific to Exynos additionally. >>> >>>> >>>> Can you please explain the design justification for this and why >>>> switching to the standard gem mmap support isn't possible? >>> >>> As I mentioned above, we are using the standard gem mmap support. >>> However, the standard gem mmap is required for on-demand paging mostly >>> suitable for Desktop. In case of ARM SoC, whole memory region requested >>> by userspace would be allocated once the gem creation interface is >>> called. In this case, it wouldn't need to map userspace with physical >>> page in page fault handler, and the use of the vma offset manager stuff >>> would be unnecessary step. >> >> You don't need to do demand paging at all, you can simply put in all the >> ptes in one go and then never unbind it. So strictly speaking you don't >> need to roll your own mmap, but otoh other drivers (including i915) do >> their own special mmap too. And since you now have it you must support it >> forever anyway. > > I agree with Daniel. The exynos drm specific mmap ioctl can be > substituted to standard gem mmap if exynos mmap is implemented for > direct mapping, actually gem cma does it from drm_gem_cma_mmap_obj. Right, we don't need mmap ioctl specific to Exynos. What we have to is to call a Exynos function to do direct mapping at the end of exynos_drm_gem_mmap function. As Daniel mentioned above, this way we don't also need even page fault handler. We really visited here and there to stick to the use of mmap ioctl specfic to Exynos. :) Thanks, Inki Dae > > Thanks. > >> >> Aside: We have patches floating around for i915 to prefault aggressively, >> so you're not the only ones who noticed the faulting overhead. ARM SoC >> really aren't all that special compared to traditional desktop gpus, so if >> you stumble over such issues please raise them on dri-devel so that we >> could look into useful generic solutions next time around. >> >>> For the same question, Al Viro did, >>> http://lists.freedesktop.org/archives/dri-devel/2013-September/046207.html >>> >>> Is there any issue I am missing , that could be incurred by Exynos codes? >> >> I've stumbled over it again because you're reusing the drm_vm_open_locked >> function, which really should just be an implementation detail of the core >> drm/gem mmap support. >> >> If you want to roll your own mmap (and that's ok, i915 has it and ttm also >> does it) then imo you should not reuse any of the core mmap code, but >> implement your own set of vm_ops. You don't need a faul handler for this >> (since it will never fault), and open/close would just grabbing/dropping a >> reference of the underlying gem object. Instead of trying to reuse the >> same vm_ops you use for normal gem mmaps, which just doesn't make a lot of >> sense to me. >> >> If exynos stops using drm_vm_open_locked then I can move it into the new >> drm_internal.h header since this function really should be private to >> drm.ko. >> >> Thanks, Daniel >> > >
On Thu, Sep 11, 2014 at 04:22:00PM +0900, Inki Dae wrote: > On 2014? 09? 11? 15:22, Daniel Vetter wrote: > > On Thu, Sep 11, 2014 at 11:16:53AM +0900, Inki Dae wrote: > >> On 2014? 09? 10? 18:01, Daniel Vetter wrote: > >>> Ok I've stumbled over the exynos mmap stuff again while cleaning up > >>> drm legacy cruft and I just don't get what you're doing and why > >>> exactly exynos needs to be special. > >>> > >>> _All_ other drm drivers happily get along with the vma offset manger > >>> stuff to handle mmaps, but somehow exynos does something really crazy. > >> > >> We are also using the vma offset manager stuff. We just added direct > >> mapping interface specific to Exynos additionally. > >> > >>> > >>> Can you please explain the design justification for this and why > >>> switching to the standard gem mmap support isn't possible? > >> > >> As I mentioned above, we are using the standard gem mmap support. > >> However, the standard gem mmap is required for on-demand paging mostly > >> suitable for Desktop. In case of ARM SoC, whole memory region requested > >> by userspace would be allocated once the gem creation interface is > >> called. In this case, it wouldn't need to map userspace with physical > >> page in page fault handler, and the use of the vma offset manager stuff > >> would be unnecessary step. > > > > You don't need to do demand paging at all, you can simply put in all the > > ptes in one go and then never unbind it. So strictly speaking you don't > > need to roll your own mmap, but otoh other drivers (including i915) do > > their own special mmap too. And since you now have it you must support it > > forever anyway. > > > > Aside: We have patches floating around for i915 to prefault aggressively, > > so you're not the only ones who noticed the faulting overhead. ARM SoC > > really aren't all that special compared to traditional desktop gpus, so if > > you stumble over such issues please raise them on dri-devel so that we > > could look into useful generic solutions next time around. > > > >> For the same question, Al Viro did, > >> http://lists.freedesktop.org/archives/dri-devel/2013-September/046207.html > >> > >> Is there any issue I am missing , that could be incurred by Exynos codes? > > > > I've stumbled over it again because you're reusing the drm_vm_open_locked > > function, which really should just be an implementation detail of the core > > drm/gem mmap support. > > Ah, right. that is critical issue. I shouldn't had used > drm_vm_open_locked. Sorry for this. > > > > > If you want to roll your own mmap (and that's ok, i915 has it and ttm also > > does it) then imo you should not reuse any of the core mmap code, but > > implement your own set of vm_ops. You don't need a faul handler for this > > (since it will never fault), and open/close would just grabbing/dropping a > > reference of the underlying gem object. Instead of trying to reuse the > > same vm_ops you use for normal gem mmaps, which just doesn't make a lot of > > sense to me. > > > > If exynos stops using drm_vm_open_locked then I can move it into the new > > drm_internal.h header since this function really should be private to > > drm.ko. > > Sorry for blocking you. I will fix it soon. No need to rush really, but if you fix this please cc me so that I can throw the header cleanup patch on top. -Daniel
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 22b8f5e..b5e5957 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -14,6 +14,8 @@ #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> +#include <linux/anon_inodes.h> + #include <drm/exynos_drm.h> #include "exynos_drm_drv.h" @@ -150,9 +152,14 @@ static int exynos_drm_unload(struct drm_device *dev) return 0; } +static const struct file_operations exynos_drm_gem_fops = { + .mmap = exynos_drm_gem_mmap_buffer, +}; + static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) { struct drm_exynos_file_private *file_priv; + struct file *anon_filp; int ret; file_priv = kzalloc(sizeof(*file_priv), GFP_KERNEL); @@ -167,6 +174,16 @@ static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) file->driver_priv = NULL; } + anon_filp = anon_inode_getfile("exynos_gem", &exynos_drm_gem_fops, + NULL, 0); + if (IS_ERR(anon_filp)) { + kfree(file_priv); + return PTR_ERR(anon_filp); + } + + anon_filp->f_mode = FMODE_READ | FMODE_WRITE; + file_priv->anon_filp = anon_filp; + return ret; } @@ -179,6 +196,7 @@ static void exynos_drm_preclose(struct drm_device *dev, static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) { struct exynos_drm_private *private = dev->dev_private; + struct drm_exynos_file_private *file_priv; struct drm_pending_vblank_event *v, *vt; struct drm_pending_event *e, *et; unsigned long flags; @@ -204,6 +222,9 @@ static void exynos_drm_postclose(struct drm_device *dev, struct drm_file *file) } spin_unlock_irqrestore(&dev->event_lock, flags); + file_priv = file->driver_priv; + if (file_priv->anon_filp) + fput(file_priv->anon_filp); kfree(file->driver_priv); file->driver_priv = NULL; diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index eaa1966..0eaf5a2 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -226,6 +226,7 @@ struct exynos_drm_ipp_private { struct drm_exynos_file_private { struct exynos_drm_g2d_private *g2d_priv; struct exynos_drm_ipp_private *ipp_priv; + struct file *anon_filp; }; /* diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 1ade191..49b8c9b 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -338,46 +338,22 @@ int exynos_drm_gem_map_offset_ioctl(struct drm_device *dev, void *data, &args->offset); } -static struct drm_file *exynos_drm_find_drm_file(struct drm_device *drm_dev, - struct file *filp) -{ - struct drm_file *file_priv; - - /* find current process's drm_file from filelist. */ - list_for_each_entry(file_priv, &drm_dev->filelist, lhead) - if (file_priv->filp == filp) - return file_priv; - - WARN_ON(1); - - return ERR_PTR(-EFAULT); -} - -static int exynos_drm_gem_mmap_buffer(struct file *filp, +int exynos_drm_gem_mmap_buffer(struct file *filp, struct vm_area_struct *vma) { struct drm_gem_object *obj = filp->private_data; struct exynos_drm_gem_obj *exynos_gem_obj = to_exynos_gem_obj(obj); struct drm_device *drm_dev = obj->dev; struct exynos_drm_gem_buf *buffer; - struct drm_file *file_priv; unsigned long vm_size; int ret; + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); + vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP; vma->vm_private_data = obj; vma->vm_ops = drm_dev->driver->gem_vm_ops; - /* restore it to driver's fops. */ - filp->f_op = fops_get(drm_dev->driver->fops); - - file_priv = exynos_drm_find_drm_file(drm_dev, filp); - if (IS_ERR(file_priv)) - return PTR_ERR(file_priv); - - /* restore it to drm_file. */ - filp->private_data = file_priv; - update_vm_cache_attr(exynos_gem_obj, vma); vm_size = vma->vm_end - vma->vm_start; @@ -411,15 +387,13 @@ static int exynos_drm_gem_mmap_buffer(struct file *filp, return 0; } -static const struct file_operations exynos_drm_gem_fops = { - .mmap = exynos_drm_gem_mmap_buffer, -}; - int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { + struct drm_exynos_file_private *exynos_file_priv; struct drm_exynos_gem_mmap *args = data; struct drm_gem_object *obj; + struct file *anon_filp; unsigned long addr; if (!(dev->driver->driver_features & DRIVER_GEM)) { @@ -427,47 +401,25 @@ int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, return -ENODEV; } + mutex_lock(&dev->struct_mutex); + obj = drm_gem_object_lookup(dev, file_priv, args->handle); if (!obj) { DRM_ERROR("failed to lookup gem object.\n"); + mutex_unlock(&dev->struct_mutex); return -EINVAL; } - /* - * We have to use gem object and its fops for specific mmaper, - * but vm_mmap() can deliver only filp. So we have to change - * filp->f_op and filp->private_data temporarily, then restore - * again. So it is important to keep lock until restoration the - * settings to prevent others from misuse of filp->f_op or - * filp->private_data. - */ - mutex_lock(&dev->struct_mutex); - - /* - * Set specific mmper's fops. And it will be restored by - * exynos_drm_gem_mmap_buffer to dev->driver->fops. - * This is used to call specific mapper temporarily. - */ - file_priv->filp->f_op = &exynos_drm_gem_fops; - - /* - * Set gem object to private_data so that specific mmaper - * can get the gem object. And it will be restored by - * exynos_drm_gem_mmap_buffer to drm_file. - */ - file_priv->filp->private_data = obj; + exynos_file_priv = file_priv->driver_priv; + anon_filp = exynos_file_priv->anon_filp; + anon_filp->private_data = obj; - addr = vm_mmap(file_priv->filp, 0, args->size, - PROT_READ | PROT_WRITE, MAP_SHARED, 0); + addr = vm_mmap(anon_filp, 0, args->size, PROT_READ | PROT_WRITE, + MAP_SHARED, 0); drm_gem_object_unreference(obj); if (IS_ERR_VALUE(addr)) { - /* check filp->f_op, filp->private_data are restored */ - if (file_priv->filp->f_op == &exynos_drm_gem_fops) { - file_priv->filp->f_op = fops_get(dev->driver->fops); - file_priv->filp->private_data = file_priv; - } mutex_unlock(&dev->struct_mutex); return (int)addr; } diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h index 702ec3a..fde860c 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h @@ -122,6 +122,9 @@ int exynos_drm_gem_map_offset_ioctl(struct drm_device *dev, void *data, int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int exynos_drm_gem_mmap_buffer(struct file *filp, + struct vm_area_struct *vma); + /* map user space allocated by malloc to pages. */ int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv);