From: Tu Vuong tu.vuong@arm.com
When a CMA GEM object is exported via DRM PRIME it should be possible to mmap the object using an offset. However drm_gem_cma_mmap_obj always zeroed vm_pgoff.
Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which is only used for non-PRIME mmap) and correct the size parameter in the call to dma_mmap_wc as the offset may not be non-zero.
Signed-off-by: Tu Vuong tu.vuong@arm.com Signed-off-by: Steven Price steven.price@arm.com Reviewed-by: Brian Starkey brian.starkey@arm.com CC: Brian Starkey brian.starkey@arm.com CC: Liviu Dudau Liviu.Dudau@arm.com --- drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 373e33f22be4..25828b33c5be 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, int ret;
/* - * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the - * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map - * the whole buffer. + * Clear the VM_PFNMAP flag that was set by drm_gem_mmap() */ vma->vm_flags &= ~VM_PFNMAP; - vma->vm_pgoff = 0;
ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr, - cma_obj->paddr, vma->vm_end - vma->vm_start); + cma_obj->paddr, cma_obj->base.size); if (ret) drm_gem_vm_close(vma);
@@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) gem_obj = vma->vm_private_data; cma_obj = to_drm_gem_cma_obj(gem_obj);
+ /* + * Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we + * want to map the whole buffer. + */ + vma->vm_pgoff = 0; + return drm_gem_cma_mmap_obj(cma_obj, vma); } EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
On Wed, Sep 27, 2017 at 4:01 PM, Steven Price Steven.Price@arm.com wrote:
From: Tu Vuong tu.vuong@arm.com
When a CMA GEM object is exported via DRM PRIME it should be possible to mmap the object using an offset. However drm_gem_cma_mmap_obj always zeroed vm_pgoff.
No, at least no one ever had run into this. Pretty much all drivers work like this.
Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which is only used for non-PRIME mmap) and correct the size parameter in the call to dma_mmap_wc as the offset may not be non-zero.
Since this is an uabi change we need the corresponding patch to upstream open source userspace. Reviewed and all that and ready for acceptance. See
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace...
Signed-off-by: Tu Vuong tu.vuong@arm.com Signed-off-by: Steven Price steven.price@arm.com Reviewed-by: Brian Starkey brian.starkey@arm.com CC: Brian Starkey brian.starkey@arm.com CC: Liviu Dudau Liviu.Dudau@arm.com
Thanks, Daniel
drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 373e33f22be4..25828b33c5be 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, int ret;
/*
* Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the* vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map* the whole buffer.
* Clear the VM_PFNMAP flag that was set by drm_gem_mmap() */ vma->vm_flags &= ~VM_PFNMAP;
vma->vm_pgoff = 0; ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,cma_obj->paddr, vma->vm_end - vma->vm_start);
cma_obj->paddr, cma_obj->base.size); if (ret) drm_gem_vm_close(vma);@@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) gem_obj = vma->vm_private_data; cma_obj = to_drm_gem_cma_obj(gem_obj);
/** Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we* want to map the whole buffer.*/vma->vm_pgoff = 0;return drm_gem_cma_mmap_obj(cma_obj, vma);} EXPORT_SYMBOL_GPL(drm_gem_cma_mmap); -- 2.11.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel,
On Thu, Sep 28, 2017 at 09:40:35AM +0200, Daniel Vetter wrote:
On Wed, Sep 27, 2017 at 4:01 PM, Steven Price Steven.Price@arm.com wrote:
From: Tu Vuong tu.vuong@arm.com
When a CMA GEM object is exported via DRM PRIME it should be possible to mmap the object using an offset. However drm_gem_cma_mmap_obj always zeroed vm_pgoff.
No, at least no one ever had run into this. Pretty much all drivers work like this.
Can you be a bit more specific with "work like this"? You mean all drivers expect mmap-ed calles to have zero offset or the fact that they all zero vm_pgoff therefore ignore the offset?
Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which is only used for non-PRIME mmap) and correct the size parameter in the call to dma_mmap_wc as the offset may not be non-zero.
Since this is an uabi change we need the corresponding patch to upstream open source userspace. Reviewed and all that and ready for acceptance. See
If ignoring the offset is considered uabi, then I agree. Otherwise it looks to me more like a fix, rather than an uabi change.
Maybe we should have first a patch warning when vm_pgoff is not zero so that we can find out how many in the userspace use that?
Best regards, Liviu
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace...
Signed-off-by: Tu Vuong tu.vuong@arm.com Signed-off-by: Steven Price steven.price@arm.com Reviewed-by: Brian Starkey brian.starkey@arm.com CC: Brian Starkey brian.starkey@arm.com CC: Liviu Dudau Liviu.Dudau@arm.com
Thanks, Daniel
drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 373e33f22be4..25828b33c5be 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, int ret;
/*
* Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the* vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map* the whole buffer.
* Clear the VM_PFNMAP flag that was set by drm_gem_mmap() */ vma->vm_flags &= ~VM_PFNMAP;
vma->vm_pgoff = 0; ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,cma_obj->paddr, vma->vm_end - vma->vm_start);
cma_obj->paddr, cma_obj->base.size); if (ret) drm_gem_vm_close(vma);@@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) gem_obj = vma->vm_private_data; cma_obj = to_drm_gem_cma_obj(gem_obj);
/** Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we* want to map the whole buffer.*/vma->vm_pgoff = 0;return drm_gem_cma_mmap_obj(cma_obj, vma);} EXPORT_SYMBOL_GPL(drm_gem_cma_mmap); -- 2.11.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Sep 28, 2017 at 10:23 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:
Hi Daniel,
On Thu, Sep 28, 2017 at 09:40:35AM +0200, Daniel Vetter wrote:
On Wed, Sep 27, 2017 at 4:01 PM, Steven Price Steven.Price@arm.com wrote:
From: Tu Vuong tu.vuong@arm.com
When a CMA GEM object is exported via DRM PRIME it should be possible to mmap the object using an offset. However drm_gem_cma_mmap_obj always zeroed vm_pgoff.
No, at least no one ever had run into this. Pretty much all drivers work like this.
Can you be a bit more specific with "work like this"? You mean all drivers expect mmap-ed calles to have zero offset or the fact that they all zero vm_pgoff therefore ignore the offset?
Iirc some drivers even outright reject the mmap if it's not aligned to the full obj.
Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which is only used for non-PRIME mmap) and correct the size parameter in the call to dma_mmap_wc as the offset may not be non-zero.
Since this is an uabi change we need the corresponding patch to upstream open source userspace. Reviewed and all that and ready for acceptance. See
If ignoring the offset is considered uabi, then I agree. Otherwise it looks to me more like a fix, rather than an uabi change.
Maybe we should have first a patch warning when vm_pgoff is not zero so that we can find out how many in the userspace use that?
Not sure warning is a good idea, because it still means we get to audit everything. Might as well go ahead an reject it with -EINVAL, and make sure that's consistent across drivers. But that's all work, so what exactly is the use-case you have and want to do sub-mmapping for? Once we have that it's much easier to discuss what to do, instead of abstraction discussions without the full stack at hand.
Thanks, Daniel
Best regards, Liviu
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace...
Signed-off-by: Tu Vuong tu.vuong@arm.com Signed-off-by: Steven Price steven.price@arm.com Reviewed-by: Brian Starkey brian.starkey@arm.com CC: Brian Starkey brian.starkey@arm.com CC: Liviu Dudau Liviu.Dudau@arm.com
Thanks, Daniel
drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 373e33f22be4..25828b33c5be 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, int ret;
/*
* Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the* vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map* the whole buffer.
* Clear the VM_PFNMAP flag that was set by drm_gem_mmap() */ vma->vm_flags &= ~VM_PFNMAP;
vma->vm_pgoff = 0; ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,cma_obj->paddr, vma->vm_end - vma->vm_start);
cma_obj->paddr, cma_obj->base.size); if (ret) drm_gem_vm_close(vma);@@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) gem_obj = vma->vm_private_data; cma_obj = to_drm_gem_cma_obj(gem_obj);
/** Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we* want to map the whole buffer.*/vma->vm_pgoff = 0;return drm_gem_cma_mmap_obj(cma_obj, vma);} EXPORT_SYMBOL_GPL(drm_gem_cma_mmap); -- 2.11.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
| I would like to | | fix the world, | | but they're not | | giving me the | \ source code! /
¯\_(ツ)_/¯
On Thu, 2017-09-28 at 10:29 +0200, Daniel Vetter wrote:
On Thu, Sep 28, 2017 at 10:23 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:
Hi Daniel,
On Thu, Sep 28, 2017 at 09:40:35AM +0200, Daniel Vetter wrote:
On Wed, Sep 27, 2017 at 4:01 PM, Steven Price <Steven.Price@arm.c om> wrote:
From: Tu Vuong tu.vuong@arm.com
When a CMA GEM object is exported via DRM PRIME it should be possible to mmap the object using an offset. However drm_gem_cma_mmap_obj always zeroed vm_pgoff.
No, at least no one ever had run into this. Pretty much all drivers work like this.
Can you be a bit more specific with "work like this"? You mean all drivers expect mmap-ed calles to have zero offset or the fact that they all zero vm_pgoff therefore ignore the offset?
Iirc some drivers even outright reject the mmap if it's not aligned to the full obj.
The mmap directly on the GEM object should behave as you say, however if user space has used the PRIME ioctl to export the object to a dma_buf object then surely it should behave like any other dma_buf object?
The problem here is that the dma_buf object might be handed off to another piece of code which has no idea where it has come from and expects to be able to map with an offset (as it would be able to if the dma_buf had come from another driver).
Fix this by moving the zeroing of vm_pgoff to drm_gem_cma_mmap (which is only used for non-PRIME mmap) and correct the size parameter in the call to dma_mmap_wc as the offset may not be non-zero.
Since this is an uabi change we need the corresponding patch to upstream open source userspace. Reviewed and all that and ready for acceptance. See
If ignoring the offset is considered uabi, then I agree. Otherwise it looks to me more like a fix, rather than an uabi change.
Maybe we should have first a patch warning when vm_pgoff is not zero so that we can find out how many in the userspace use that?
Not sure warning is a good idea, because it still means we get to audit everything. Might as well go ahead an reject it with -EINVAL, and make sure that's consistent across drivers. But that's all work, so what exactly is the use-case you have and want to do sub-mmapping for? Once we have that it's much easier to discuss what to do, instead of abstraction discussions without the full stack at hand.
This was initially discovered with the Mali Midgard driver. It uses sub-mmapping when there are multiple 'frames' within one buffer (e.g. using a display driver that only supports panning around a single pre- allocated buffer). It may also make sense in some cases for the likes of glReadPixels if the region specified is small and in a subset of the pages.
Currently there's no way the caller of mmap can find out that the offset has been ignored. One option would be to "fix" this by simply rejecting attempts to mmap with an offset (return -EINVAL). However I do feel this is a useful feature as it avoids the need to mmap a large region when the caller knows only a small part of it will be needed.
I don't believe this is a uabi change as such because the current functionality is "useless" - although obviously it is possible that some driver is feeding garbage in to the offset and happens to work because of the current implementation. Unless any user space drivers have bugs of this sort there wouldn't be any need to change them (and they would already be broken with other dma_buf exporters).
Thanks,
Steven
Thanks, Daniel
Best regards, Liviu
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-sourc e-userspace-requirements
Signed-off-by: Tu Vuong tu.vuong@arm.com Signed-off-by: Steven Price steven.price@arm.com Reviewed-by: Brian Starkey brian.starkey@arm.com CC: Brian Starkey brian.starkey@arm.com CC: Liviu Dudau Liviu.Dudau@arm.com
Thanks, Daniel
drivers/gpu/drm/drm_gem_cma_helper.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 373e33f22be4..25828b33c5be 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -276,15 +276,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, int ret;
/* - * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the - * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map - * the whole buffer. + * Clear the VM_PFNMAP flag that was set by drm_gem_mmap() */ vma->vm_flags &= ~VM_PFNMAP; - vma->vm_pgoff = 0;
ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj-
vaddr,
- cma_obj->paddr, vma->vm_end - vma-
vm_start);
+ cma_obj->paddr, cma_obj->base.size); if (ret) drm_gem_vm_close(vma);
@@ -322,6 +319,12 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) gem_obj = vma->vm_private_data; cma_obj = to_drm_gem_cma_obj(gem_obj);
+ /* + * Set the vm_pgoff (used as a fake buffer offset by DRM) to 0 as we + * want to map the whole buffer. + */ + vma->vm_pgoff = 0;
return drm_gem_cma_mmap_obj(cma_obj, vma); } EXPORT_SYMBOL_GPL(drm_gem_cma_mmap); -- 2.11.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
I would like to | fix the world, | but they're not | giving me the |
\ source code! / --------------- ¯_(ツ)_/¯
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Liviu Dudau -
Steven Price