| Message ID | 1415639805-17477-1-git-send-email-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 10/11/14 17:36, Rob Clark wrote: > On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson > <daniel.thompson@linaro.org> wrote: >> diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c >> index ad772fe36115..4e4fa5828d5d 100644 >> --- a/drivers/gpu/drm/msm/msm_gem_prime.c >> +++ b/drivers/gpu/drm/msm/msm_gem_prime.c >> @@ -20,6 +20,14 @@ >> >> #include <linux/dma-buf.h> >> >> +struct dma_buf *msm_gem_prime_export(struct drm_device *dev, >> + struct drm_gem_object *obj, int flags) >> +{ >> + /* we want to be able to write in mmapped buffer */ >> + flags |= O_RDWR; >> + return drm_gem_prime_export(dev, obj, flags); >> +} >> + > > seems like this probably should be done more centrally.. and in fact, > might be better to have something like this in > drm_prime_handle_to_fd_ioctl: > > /* check flags are valid */ > - if (args->flags & ~DRM_CLOEXEC) > + if (args->flags & ~(DRM_CLOEXEC | O_RDWR)) > return -EINVAL; > > so exporter can specify whether to allow mmap or not. That makes sense I'll try this. Do we need to wrap O_RDWR in the same way we wrap O_CLOEXEC? (I don't really understand why DRM_CLOEXEC exists; even the patch description from when the symbol was introduced names it O_CLOEXEC). Also the "flags |= O_RDWR" approach is copied from the sti driver. I'll share a patch to remove it but that will definitely needs Benjamin's ack because it will stop some userspaces working correctly (however I suspect that Benjamin may be the only person currently with such a userspace and that he can be persuaded not to call it a regression). Daniel.
On Tue, Nov 11, 2014 at 9:28 AM, Daniel Thompson <daniel.thompson@linaro.org> wrote: > On 10/11/14 17:36, Rob Clark wrote: >> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson >> <daniel.thompson@linaro.org> wrote: >>> diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c >>> index ad772fe36115..4e4fa5828d5d 100644 >>> --- a/drivers/gpu/drm/msm/msm_gem_prime.c >>> +++ b/drivers/gpu/drm/msm/msm_gem_prime.c >>> @@ -20,6 +20,14 @@ >>> >>> #include <linux/dma-buf.h> >>> >>> +struct dma_buf *msm_gem_prime_export(struct drm_device *dev, >>> + struct drm_gem_object *obj, int flags) >>> +{ >>> + /* we want to be able to write in mmapped buffer */ >>> + flags |= O_RDWR; >>> + return drm_gem_prime_export(dev, obj, flags); >>> +} >>> + >> >> seems like this probably should be done more centrally.. and in fact, >> might be better to have something like this in >> drm_prime_handle_to_fd_ioctl: >> >> /* check flags are valid */ >> - if (args->flags & ~DRM_CLOEXEC) >> + if (args->flags & ~(DRM_CLOEXEC | O_RDWR)) >> return -EINVAL; >> >> so exporter can specify whether to allow mmap or not. > > That makes sense I'll try this. > > Do we need to wrap O_RDWR in the same way we wrap O_CLOEXEC? (I don't > really understand why DRM_CLOEXEC exists; even the patch description > from when the symbol was introduced names it O_CLOEXEC). I *think* wrapping it w/ DRM_CLOEXEC was mostly just for purposes of making it clear which flags are appropriate.. probably best to do the same w/ a DRM_RDWR I guess BR, -R > Also the "flags |= O_RDWR" approach is copied from the sti driver. I'll > share a patch to remove it but that will definitely needs Benjamin's ack > because it will stop some userspaces working correctly (however I > suspect that Benjamin may be the only person currently with such a > userspace and that he can be persuaded not to call it a regression). > > > Daniel.
The same kind of issue has been fixed in v4l2: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/drivers/media/v4l2-core/videobuf2-core.c?id=c1b96a236e94d49d9396d0bbceb5524519c5c66e I'm ok to add a DRM_RDWR flag, just do not remove the code from sti driver until I have been able to test it. Benjamin 2014-11-11 17:26 GMT+01:00 Rob Clark <robdclark@gmail.com>: > On Tue, Nov 11, 2014 at 9:28 AM, Daniel Thompson > <daniel.thompson@linaro.org> wrote: >> On 10/11/14 17:36, Rob Clark wrote: >>> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson >>> <daniel.thompson@linaro.org> wrote: >>>> diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c >>>> index ad772fe36115..4e4fa5828d5d 100644 >>>> --- a/drivers/gpu/drm/msm/msm_gem_prime.c >>>> +++ b/drivers/gpu/drm/msm/msm_gem_prime.c >>>> @@ -20,6 +20,14 @@ >>>> >>>> #include <linux/dma-buf.h> >>>> >>>> +struct dma_buf *msm_gem_prime_export(struct drm_device *dev, >>>> + struct drm_gem_object *obj, int flags) >>>> +{ >>>> + /* we want to be able to write in mmapped buffer */ >>>> + flags |= O_RDWR; >>>> + return drm_gem_prime_export(dev, obj, flags); >>>> +} >>>> + >>> >>> seems like this probably should be done more centrally.. and in fact, >>> might be better to have something like this in >>> drm_prime_handle_to_fd_ioctl: >>> >>> /* check flags are valid */ >>> - if (args->flags & ~DRM_CLOEXEC) >>> + if (args->flags & ~(DRM_CLOEXEC | O_RDWR)) >>> return -EINVAL; >>> >>> so exporter can specify whether to allow mmap or not. >> >> That makes sense I'll try this. >> >> Do we need to wrap O_RDWR in the same way we wrap O_CLOEXEC? (I don't >> really understand why DRM_CLOEXEC exists; even the patch description >> from when the symbol was introduced names it O_CLOEXEC). > > I *think* wrapping it w/ DRM_CLOEXEC was mostly just for purposes of > making it clear which flags are appropriate.. probably best to do the > same w/ a DRM_RDWR I guess > > BR, > -R > >> Also the "flags |= O_RDWR" approach is copied from the sti driver. I'll >> share a patch to remove it but that will definitely needs Benjamin's ack >> because it will stop some userspaces working correctly (however I >> suspect that Benjamin may be the only person currently with such a >> userspace and that he can be persuaded not to call it a regression). >> >> >> Daniel.
On 12/11/14 09:41, Benjamin Gaignard wrote: > The same kind of issue has been fixed in v4l2: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/patch/drivers/media/v4l2-core/videobuf2-core.c?id=c1b96a236e94d49d9396d0bbceb5524519c5c66e > > I'm ok to add a DRM_RDWR flag, just do not remove the code from sti > driver until I have been able to test it. Don't worry. I'll do that as a separate patch so you can ack/nack as you need to. Daniel. > > Benjamin > > > 2014-11-11 17:26 GMT+01:00 Rob Clark <robdclark@gmail.com>: >> On Tue, Nov 11, 2014 at 9:28 AM, Daniel Thompson >> <daniel.thompson@linaro.org> wrote: >>> On 10/11/14 17:36, Rob Clark wrote: >>>> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson >>>> <daniel.thompson@linaro.org> wrote: >>>>> diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c >>>>> index ad772fe36115..4e4fa5828d5d 100644 >>>>> --- a/drivers/gpu/drm/msm/msm_gem_prime.c >>>>> +++ b/drivers/gpu/drm/msm/msm_gem_prime.c >>>>> @@ -20,6 +20,14 @@ >>>>> >>>>> #include <linux/dma-buf.h> >>>>> >>>>> +struct dma_buf *msm_gem_prime_export(struct drm_device *dev, >>>>> + struct drm_gem_object *obj, int flags) >>>>> +{ >>>>> + /* we want to be able to write in mmapped buffer */ >>>>> + flags |= O_RDWR; >>>>> + return drm_gem_prime_export(dev, obj, flags); >>>>> +} >>>>> + >>>> >>>> seems like this probably should be done more centrally.. and in fact, >>>> might be better to have something like this in >>>> drm_prime_handle_to_fd_ioctl: >>>> >>>> /* check flags are valid */ >>>> - if (args->flags & ~DRM_CLOEXEC) >>>> + if (args->flags & ~(DRM_CLOEXEC | O_RDWR)) >>>> return -EINVAL; >>>> >>>> so exporter can specify whether to allow mmap or not. >>> >>> That makes sense I'll try this. >>> >>> Do we need to wrap O_RDWR in the same way we wrap O_CLOEXEC? (I don't >>> really understand why DRM_CLOEXEC exists; even the patch description >>> from when the symbol was introduced names it O_CLOEXEC). >> >> I *think* wrapping it w/ DRM_CLOEXEC was mostly just for purposes of >> making it clear which flags are appropriate.. probably best to do the >> same w/ a DRM_RDWR I guess >> >> BR, >> -R >> >>> Also the "flags |= O_RDWR" approach is copied from the sti driver. I'll >>> share a patch to remove it but that will definitely needs Benjamin's ack >>> because it will stop some userspaces working correctly (however I >>> suspect that Benjamin may be the only person currently with such a >>> userspace and that he can be persuaded not to call it a regression). >>> >>> >>> Daniel. > > >
This patch set started out as a single patch with a trivial bit of boilerplate to add dmabuf mmap support to the msm driver. Each of the change remains fairly trivial but I've split it out by topic. Patches 1, 2 and 3 in this series should be good to go but please don't take patch 4 (which has a small effect on userspace) without an explicit ack from Benjamin Gaignard. I've tested this both with a rather hacked about Android userspace and with a fairly small test case run from debian. Both bits of code currently use dumb buffers. Thanks to Benjamin for his help in finding this bit of code. v2: * Modified DRM_PRIME_HANDLE_TO_FD to honour the O_RDRW from the user and removed code to workaround this from the sti driver (Rob Clark). * Added a patch to (rather spartanly) document gem_prime_mmap. Only tacked into this series 'cos I spotted it was missing when I was checking whether I needed to describe DRM_RDRWR anywhere. Daniel Thompson (4): drm: prime: Honour O_RDWR during prime-handle-to-fd drm: prime: Document gem_prime_mmap drm: msm: Allow exported dma-bufs to be mapped drm: sti: Honour O_RDWR during prime-handle-to-fd drivers/gpu/drm/drm_prime.c | 13 ++++++------- drivers/gpu/drm/msm/msm_drv.c | 1 + drivers/gpu/drm/msm/msm_drv.h | 3 +++ drivers/gpu/drm/msm/msm_gem_prime.c | 13 +++++++++++++ drivers/gpu/drm/sti/sti_drm_drv.c | 11 +---------- include/uapi/drm/drm.h | 1 + 6 files changed, 25 insertions(+), 17 deletions(-) -- 1.9.3
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index b67ef5985125..f0d77ee482a5 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -824,7 +824,7 @@ static struct drm_driver msm_driver = { .dumb_destroy = drm_gem_dumb_destroy, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, - .gem_prime_export = drm_gem_prime_export, + .gem_prime_export = msm_gem_prime_export, .gem_prime_import = drm_gem_prime_import, .gem_prime_pin = msm_gem_prime_pin, .gem_prime_unpin = msm_gem_prime_unpin, @@ -832,6 +832,7 @@ static struct drm_driver msm_driver = { .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, .gem_prime_vmap = msm_gem_prime_vmap, .gem_prime_vunmap = msm_gem_prime_vunmap, + .gem_prime_mmap = msm_gem_prime_mmap, #ifdef CONFIG_DEBUG_FS .debugfs_init = msm_debugfs_init, .debugfs_cleanup = msm_debugfs_cleanup, diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 67f9d0a2332c..3a1c85f7f241 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -154,6 +154,8 @@ void msm_update_fence(struct drm_device *dev, uint32_t fence); int msm_ioctl_gem_submit(struct drm_device *dev, void *data, struct drm_file *file); +int msm_gem_mmap_obj(struct drm_gem_object *obj, + struct vm_area_struct *vma); int msm_gem_mmap(struct file *filp, struct vm_area_struct *vma); int msm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf); uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj); @@ -167,9 +169,12 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, uint32_t handle, uint64_t *offset); +struct dma_buf *msm_gem_prime_export(struct drm_device *dev, + struct drm_gem_object *obj, int flags); struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj); void *msm_gem_prime_vmap(struct drm_gem_object *obj); void msm_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr); +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev, struct dma_buf_attachment *attach, struct sg_table *sg); int msm_gem_prime_pin(struct drm_gem_object *obj); diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c index ad772fe36115..4e4fa5828d5d 100644 --- a/drivers/gpu/drm/msm/msm_gem_prime.c +++ b/drivers/gpu/drm/msm/msm_gem_prime.c @@ -20,6 +20,14 @@ #include <linux/dma-buf.h> +struct dma_buf *msm_gem_prime_export(struct drm_device *dev, + struct drm_gem_object *obj, int flags) +{ + /* we want to be able to write in mmapped buffer */ + flags |= O_RDWR; + return drm_gem_prime_export(dev, obj, flags); +} + struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj) { struct msm_gem_object *msm_obj = to_msm_bo(obj); @@ -37,6 +45,19 @@ void msm_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr) /* TODO msm_gem_vunmap() */ } +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) +{ + int ret; + + mutex_lock(&obj->dev->struct_mutex); + ret = drm_gem_mmap_obj(obj, obj->size, vma); + mutex_unlock(&obj->dev->struct_mutex); + if (ret < 0) + return ret; + + return msm_gem_mmap_obj(vma->vm_private_data, vma); +} + struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev, struct dma_buf_attachment *attach, struct sg_table *sg) {
Currently msm does not implement gem_prime_mmap. Without this it is not possible to draw onto a dma-buf from another process (making its very hard to implement the Android rendering model). Implementing this mostly just providing some boilerplate code. However in addition to providing the implementation of mmap itself we must also interfere with the flags during the export. Without this the mmap() will fail the permission checks early in the syscall handling and msm_gem_prime_mmap() will never be called. Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> --- Notes: I've tested this both with a rather hacked about Android userspace and with a fairly small test case run from debian. Both currently use dumb buffers. Thanks to Benjamin for his help in finding this bit of code. drivers/gpu/drm/msm/msm_drv.c | 3 ++- drivers/gpu/drm/msm/msm_drv.h | 5 +++++ drivers/gpu/drm/msm/msm_gem_prime.c | 21 +++++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) -- 1.9.3