[PATCH v3.18-rc3] drm: msm: Allow exported dma-bufs to be mapped

[Benjamin Gaignard]

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 :
> On Tue, Nov 11, 2014 at 9:28 AM, Daniel Thompson
>  wrote:
>> On 10/11/14 17:36, Rob Clark wrote:
>>> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson
>>>  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 

 +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

[PATCH v3.18-rc3] drm: msm: Allow exported dma-bufs to be mapped

[Daniel Thompson]

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 :
>> On Tue, Nov 11, 2014 at 9:28 AM, Daniel Thompson
>>  wrote:
>>> On 10/11/14 17:36, Rob Clark wrote:
 On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson
  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 
>
> +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.
> 
> 
> 

[PATCH v3.18-rc3] drm: msm: Allow exported dma-bufs to be mapped

[Daniel Thompson]

On 10/11/14 17:36, Rob Clark wrote:
> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson
>  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 
>>
>> +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.

[PATCH v3.18-rc3] drm: msm: Allow exported dma-bufs to be mapped

[Rob Clark]

On Tue, Nov 11, 2014 at 9:28 AM, Daniel Thompson
 wrote:
> On 10/11/14 17:36, Rob Clark wrote:
>> On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson
>>  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 
>>>
>>> +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.

[PATCH v3.18-rc3] drm: msm: Allow exported dma-bufs to be mapped

[Daniel Thompson]

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 
---

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 

+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(>dev->struct_mutex);
+   ret = drm_gem_mmap_obj(obj, obj->size, vma);
+   mutex_unlock(>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

[PATCH v3.18-rc3] drm: msm: Allow exported dma-bufs to be mapped

[Rob Clark]

On Mon, Nov 10, 2014 at 12:16 PM, Daniel Thompson
 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 
>
> +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