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(-)
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) { -- 1.9.3
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..
BR, -R
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/driver...
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/driver...
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
Currently DRM_IOCTL_PRIME_HANDLE_TO_FD rejects all flags except (DRM|O)_CLOEXEC making it hard for the userspace to generate a file descriptor that can be used by mmap().
It is easy to relax the restriction and allow read/write permissions. This should be safe because the flags are seldom touched by drm; mostly they are passed verbatim to dma_buf calls.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- drivers/gpu/drm/drm_prime.c | 9 +++------ include/uapi/drm/drm.h | 1 + 2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 78ca30808422..8467b17c8053 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -331,7 +331,7 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { * drm_gem_prime_export - helper library implemention of the export callback * @dev: drm_device to export from * @obj: GEM object to export - * @flags: flags like DRM_CLOEXEC + * @flags: flags like DRM_CLOEXEC and DRM_RDWR * * This is the implementation of the gem_prime_export functions for GEM drivers * using the PRIME helpers. @@ -635,14 +635,11 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, return -ENOSYS;
/* check flags are valid */ - if (args->flags & ~DRM_CLOEXEC) + if (args->flags & ~(DRM_CLOEXEC | DRM_RDWR)) return -EINVAL;
- /* we only want to pass DRM_CLOEXEC which is == O_CLOEXEC */ - flags = args->flags & DRM_CLOEXEC; - return dev->driver->prime_handle_to_fd(dev, file_priv, - args->handle, flags, &args->fd); + args->handle, args->flags, &args->fd); }
int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index b0b855613641..89c2b68ddc51 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -660,6 +660,7 @@ struct drm_set_client_cap { __u64 value; };
+#define DRM_RDWR O_RDWR #define DRM_CLOEXEC O_CLOEXEC struct drm_prime_handle { __u32 handle;
On Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson daniel.thompson@linaro.org wrote:
Currently DRM_IOCTL_PRIME_HANDLE_TO_FD rejects all flags except (DRM|O)_CLOEXEC making it hard for the userspace to generate a file descriptor that can be used by mmap().
It is easy to relax the restriction and allow read/write permissions. This should be safe because the flags are seldom touched by drm; mostly they are passed verbatim to dma_buf calls.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
Reviewed-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_prime.c | 9 +++------ include/uapi/drm/drm.h | 1 + 2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 78ca30808422..8467b17c8053 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -331,7 +331,7 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
- drm_gem_prime_export - helper library implemention of the export callback
- @dev: drm_device to export from
- @obj: GEM object to export
- @flags: flags like DRM_CLOEXEC
- @flags: flags like DRM_CLOEXEC and DRM_RDWR
- This is the implementation of the gem_prime_export functions for GEM drivers
- using the PRIME helpers.
@@ -635,14 +635,11 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, return -ENOSYS;
/* check flags are valid */
if (args->flags & ~DRM_CLOEXEC)
if (args->flags & ~(DRM_CLOEXEC | DRM_RDWR)) return -EINVAL;
/* we only want to pass DRM_CLOEXEC which is == O_CLOEXEC */flags = args->flags & DRM_CLOEXEC;return dev->driver->prime_handle_to_fd(dev, file_priv,args->handle, flags, &args->fd);
args->handle, args->flags, &args->fd);}
int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index b0b855613641..89c2b68ddc51 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -660,6 +660,7 @@ struct drm_set_client_cap { __u64 value; };
+#define DRM_RDWR O_RDWR #define DRM_CLOEXEC O_CLOEXEC struct drm_prime_handle { __u32 handle; -- 1.9.3
Hello,
On Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson daniel.thompson@linaro.org wrote:
Currently DRM_IOCTL_PRIME_HANDLE_TO_FD rejects all flags except (DRM|O)_CLOEXEC making it hard for the userspace to generate a file descriptor that can be used by mmap().
It is easy to relax the restriction and allow read/write permissions. This should be safe because the flags are seldom touched by drm; mostly they are passed verbatim to dma_buf calls.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
Reviewed-by: Rob Clark robdclark@gmail.com
It's a little bit old by now, but I'm wondering if someone call tell me whether this patch is likely to be merged sometime, or has it been (should it be?) abandoned.
Thank you, Damian
drivers/gpu/drm/drm_prime.c | 9 +++------ include/uapi/drm/drm.h | 1 + 2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 78ca30808422..8467b17c8053 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -331,7 +331,7 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
- drm_gem_prime_export - helper library implemention of the export callback
- @dev: drm_device to export from
- @obj: GEM object to export
- @flags: flags like DRM_CLOEXEC
- @flags: flags like DRM_CLOEXEC and DRM_RDWR
- This is the implementation of the gem_prime_export functions for GEM drivers
- using the PRIME helpers.
@@ -635,14 +635,11 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, return -ENOSYS;
/* check flags are valid */
if (args->flags & ~DRM_CLOEXEC)
if (args->flags & ~(DRM_CLOEXEC | DRM_RDWR)) return -EINVAL;
/* we only want to pass DRM_CLOEXEC which is == O_CLOEXEC */flags = args->flags & DRM_CLOEXEC;return dev->driver->prime_handle_to_fd(dev, file_priv,args->handle, flags, &args->fd);
args->handle, args->flags, &args->fd);}
int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index b0b855613641..89c2b68ddc51 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -660,6 +660,7 @@ struct drm_set_client_cap { __u64 value; };
+#define DRM_RDWR O_RDWR #define DRM_CLOEXEC O_CLOEXEC struct drm_prime_handle { __u32 handle; -- 1.9.3
On 27/05/15 04:15, Damian Hobson-Garcia wrote:
Hello,
On Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson daniel.thompson@linaro.org wrote:
Currently DRM_IOCTL_PRIME_HANDLE_TO_FD rejects all flags except (DRM|O)_CLOEXEC making it hard for the userspace to generate a file descriptor that can be used by mmap().
It is easy to relax the restriction and allow read/write permissions. This should be safe because the flags are seldom touched by drm; mostly they are passed verbatim to dma_buf calls.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
Reviewed-by: Rob Clark robdclark@gmail.com
It's a little bit old by now, but I'm wondering if someone call tell me whether this patch is likely to be merged sometime, or has it been (should it be?) abandoned.
For me, this code remains useful and it would be good to merge it.
I accidentally removed the whole patchset from my "keep-resending-these-patches" when patch 3 went upstream... I'll rebase this when I get a chance.
Daniel.
PS Damn it all..., Rob may (or may not) remember my saying I had trouble getting the DRM GFX code to come up on my Android/IFC6410 port. Losing track of this patch would certainly explain it!
gem_prime_map is not currently described in the DRM manual, lets document it.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- drivers/gpu/drm/drm_prime.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 8467b17c8053..5d00c8726133 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -309,7 +309,7 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { * Drivers can implement @gem_prime_export and @gem_prime_import in terms of * simpler APIs by using the helper functions @drm_gem_prime_export and * @drm_gem_prime_import. These functions implement dma-buf support in terms of - * five lower-level driver callbacks: + * six lower-level driver callbacks: * * Export callbacks: * @@ -321,6 +321,8 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { * * - @gem_prime_vunmap: vunmap a buffer exported by your driver * + * - @gem_prime_mmap (optional): mmap a buffer exported by your driver + * * Import callback: * * - @gem_prime_import_sg_table (import): produce a GEM object from another
On Wed, Nov 12, 2014 at 11:38:13AM +0000, Daniel Thompson wrote:
gem_prime_map is not currently described in the DRM manual, lets document it.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
Patches 1&2 are Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Nit: Since this fixes an oversight in an earlier patch I usally cc the people of the offending patch:
commit 7c397cd97b8f46659698396b420bd48c3e6703e6 Author: Joonyoung Shim jy0922.shim@samsung.com Date: Fri Jun 28 14:24:53 2013 +0900
drm: add mmap function to prime helpers
Cheers, Daniel
On Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson daniel.thompson@linaro.org wrote:
gem_prime_map is not currently described in the DRM manual, lets document it.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
Reviewed-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_prime.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 8467b17c8053..5d00c8726133 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -309,7 +309,7 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
- Drivers can implement @gem_prime_export and @gem_prime_import in terms of
- simpler APIs by using the helper functions @drm_gem_prime_export and
- @drm_gem_prime_import. These functions implement dma-buf support in terms of
- five lower-level driver callbacks:
- six lower-level driver callbacks:
- Export callbacks:
@@ -321,6 +321,8 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
- @gem_prime_vunmap: vunmap a buffer exported by your driver
- @gem_prime_mmap (optional): mmap a buffer exported by your driver
- Import callback:
- @gem_prime_import_sg_table (import): produce a GEM object from another
-- 1.9.3
Currently msm does not implement gem_prime_mmap. Without this it is not possible to draw onto a dma-buf from userspace (making its very hard to implement the Android rendering model).
Fixing this is just a matter of adding a little boilerplate.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- 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 +++++++++++++ 3 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index b67ef5985125..5717d4ec1a2c 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -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..1a7344066154 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); @@ -170,6 +172,7 @@ int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, 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..dd7a7ab603e2 100644 --- a/drivers/gpu/drm/msm/msm_gem_prime.c +++ b/drivers/gpu/drm/msm/msm_gem_prime.c @@ -37,6 +37,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) {
On Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson daniel.thompson@linaro.org wrote:
Currently msm does not implement gem_prime_mmap. Without this it is not possible to draw onto a dma-buf from userspace (making its very hard to implement the Android rendering model).
Fixing this is just a matter of adding a little boilerplate.
looks good.. I'll queue it up for my 3.19 pull req. Thanks!
BR, -R
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org
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 +++++++++++++ 3 files changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index b67ef5985125..5717d4ec1a2c 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -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..1a7344066154 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); @@ -170,6 +172,7 @@ int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, 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..dd7a7ab603e2 100644 --- a/drivers/gpu/drm/msm/msm_gem_prime.c +++ b/drivers/gpu/drm/msm/msm_gem_prime.c @@ -37,6 +37,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) { -- 1.9.3
Currently the sti drm driver forcible applies O_RDWR when it exports a prime handle. This is because it was not previously possible for user requests to create the fd with O_RDWR passed into drivers.
This is a cleanup to remove this code. This change has obvious impact upon the userspace which must change the flags passed to DRM_IOCTL_PRIME_HANDLE_TO_FD. However at present only a tiny handful of developers run this userspace and, if they don't complain, nobody else will.
Signed-off-by: Daniel Thompson daniel.thompson@linaro.org --- drivers/gpu/drm/sti/sti_drm_drv.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_drm_drv.c b/drivers/gpu/drm/sti/sti_drm_drv.c index 223d93c3a05d..320167e1520a 100644 --- a/drivers/gpu/drm/sti/sti_drm_drv.c +++ b/drivers/gpu/drm/sti/sti_drm_drv.c @@ -93,15 +93,6 @@ static const struct file_operations sti_drm_driver_fops = { .release = drm_release, };
-static struct dma_buf *sti_drm_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); -} - static struct drm_driver sti_drm_driver = { .driver_features = DRIVER_HAVE_IRQ | DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME, @@ -119,7 +110,7 @@ static struct drm_driver sti_drm_driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, - .gem_prime_export = sti_drm_gem_prime_export, + .gem_prime_export = drm_gem_prime_export, .gem_prime_import = drm_gem_prime_import, .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table, .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
dri-devel@lists.freedesktop.org
-
Benjamin Gaignard -
Damian Hobson-Garcia -
Daniel Thompson -
Daniel Vetter -
Rob Clark