| Message ID | AM4PR0802MB219470E3CD458E74258ABCC797780@AM4PR0802MB2194.eurprd08.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-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
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-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
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-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! / > --------------- > ¯\_(ツ)_/¯
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! / > > --------------- > > ¯\_(ツ)_/¯ > > >
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);