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 <[email protected]>
---
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
<[email protected]> 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
> <[email protected]> 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
<[email protected]> wrote:
> On 10/11/14 17:36, Rob Clark wrote:
>> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson
>> <[email protected]> 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 <[email protected]>:
> On Tue, Nov 11, 2014 at 9:28 AM, Daniel Thompson
> <[email protected]> wrote:
>> On 10/11/14 17:36, Rob Clark wrote:
>>> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson
>>> <[email protected]> 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.
--
Benjamin Gaignard
Graphic Working Group
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
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 <[email protected]>:
>> On Tue, Nov 11, 2014 at 9:28 AM, Daniel Thompson
>> <[email protected]> wrote:
>>> On 10/11/14 17:36, Rob Clark wrote:
>>>> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson
>>>> <[email protected]> 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 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 <[email protected]>
---
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 <[email protected]>
---
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,
--
1.9.3
gem_prime_map is not currently described in the DRM manual, lets document
it.
Signed-off-by: Daniel Thompson <[email protected]>
---
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 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 <[email protected]>
---
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 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 <[email protected]>
Patches 1&2 are Reviewed-by: Daniel Vetter <[email protected]>
Nit: Since this fixes an oversight in an earlier patch I usally cc the
people of the offending patch:
commit 7c397cd97b8f46659698396b420bd48c3e6703e6
Author: Joonyoung Shim <[email protected]>
Date: Fri Jun 28 14:24:53 2013 +0900
drm: add mmap function to prime helpers
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson
<[email protected]> 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 <[email protected]>
> ---
> 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
>
On Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson
<[email protected]> 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 <[email protected]>
Reviewed-by: Rob Clark <[email protected]>
> ---
> 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 Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson
<[email protected]> wrote:
> gem_prime_map is not currently described in the DRM manual, lets document
> it.
>
> Signed-off-by: Daniel Thompson <[email protected]>
Reviewed-by: Rob Clark <[email protected]>
> ---
> 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
>
Hello,
>On Wed, Nov 12, 2014 at 6:38 AM, Daniel Thompson <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Rob Clark <[email protected]>
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
>>