* [PATCH 1/2] drm: split out buffer lookup from fake VMA offset @ 2018-05-25 13:42 Lucas Stach 2018-05-25 13:42 ` [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers Lucas Stach 0 siblings, 1 reply; 9+ messages in thread From: Lucas Stach @ 2018-05-25 13:42 UTC (permalink / raw) To: Tomi Valkeinen; +Cc: Julien Boulnois, dri-devel Drivers that want to implement mmap handling different to what drm_gem_mmap provides need a way to look up the GEM BO from the fake VMA offset. Split this out to make it reusable. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/gpu/drm/drm_gem.c | 56 +++++++++++++++++++++++++++------------ include/drm/drm_gem.h | 2 ++ 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 4975ba9a7bc8..f476bc45266f 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -979,30 +979,21 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, EXPORT_SYMBOL(drm_gem_mmap_obj); /** - * drm_gem_mmap - memory map routine for GEM objects + * drm_gem_bo_vm_lookup - look up a GEM object based on its offset in the DRM VM * @filp: DRM file pointer - * @vma: VMA for the area to be mapped - * - * If a driver supports GEM object mapping, mmap calls on the DRM file - * descriptor will end up here. + * @vma: VMA containing the mmap offset of the buffer * * Look up the GEM object based on the offset passed in (vma->vm_pgoff will * contain the fake offset we created when the GTT map ioctl was called on - * the object) and map it with a call to drm_gem_mmap_obj(). - * - * If the caller is not granted access to the buffer object, the mmap will fail - * with EACCES. Please see the vma manager for more information. + * the object). */ -int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) +struct drm_gem_object * +drm_gem_bo_vm_lookup(struct file *filp, struct vm_area_struct *vma) { struct drm_file *priv = filp->private_data; struct drm_device *dev = priv->minor->dev; - struct drm_gem_object *obj = NULL; struct drm_vma_offset_node *node; - int ret; - - if (drm_dev_is_unplugged(dev)) - return -ENODEV; + struct drm_gem_object *obj = NULL; drm_vma_offset_lock_lookup(dev->vma_offset_manager); node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager, @@ -1025,15 +1016,46 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) } drm_vma_offset_unlock_lookup(dev->vma_offset_manager); + return obj; +} +EXPORT_SYMBOL(drm_gem_bo_vm_lookup); + +/** + * drm_gem_mmap - memory map routine for GEM objects + * @filp: DRM file pointer + * @vma: VMA for the area to be mapped + * + * If a driver supports GEM object mapping, mmap calls on the DRM file + * descriptor will end up here. + * + * Look up the GEM object based on the offset passed in (vma->vm_pgoff will + * contain the fake offset we created when the GTT map ioctl was called on + * the object) and map it with a call to drm_gem_mmap_obj(). + * + * If the caller is not granted access to the buffer object, the mmap will fail + * with EACCES. Please see the vma manager for more information. + */ +int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) +{ + struct drm_file *priv = filp->private_data; + struct drm_device *dev = priv->minor->dev; + struct drm_gem_object *obj = NULL; + int ret; + + if (drm_dev_is_unplugged(dev)) + return -ENODEV; + + obj = drm_gem_bo_vm_lookup(filp, vma); if (!obj) return -EINVAL; - if (!drm_vma_node_is_allowed(node, priv)) { + if (!drm_vma_node_is_allowed(&obj->vma_node, priv)) { drm_gem_object_put_unlocked(obj); return -EACCES; } - ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT, + ret = drm_gem_mmap_obj(obj, + drm_vma_node_size(&obj->vma_node) << PAGE_SHIFT, vma); drm_gem_object_put_unlocked(obj); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 3583b98a1718..ae3eccc9f8cb 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -182,6 +182,8 @@ void drm_gem_private_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); void drm_gem_vm_open(struct vm_area_struct *vma); void drm_gem_vm_close(struct vm_area_struct *vma); +struct drm_gem_object * +drm_gem_bo_vm_lookup(struct file *filp, struct vm_area_struct *vma); int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, struct vm_area_struct *vma); int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma); -- 2.17.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers 2018-05-25 13:42 [PATCH 1/2] drm: split out buffer lookup from fake VMA offset Lucas Stach @ 2018-05-25 13:42 ` Lucas Stach 2018-05-29 8:20 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Lucas Stach @ 2018-05-25 13:42 UTC (permalink / raw) To: Tomi Valkeinen; +Cc: Julien Boulnois, dri-devel The intention of the existing code was to deflect the actual work of mmaping a dma-buf to the exporter, as that one probably knows best how to handle the buffer. Unfortunately the call to drm_gem_mmap did more than what etnaviv needs in this case by actually setting up the mapping. Move mapping setup to the shm buffer type mmap implementation so we only need to look up the BO and call the buffer type mmap function from the handler. Fixes mmap behavior with dma-buf imported and userptr buffers. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 30 ++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index fcc969fa0e69..f38989960d7f 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -138,6 +138,13 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, struct vm_area_struct *vma) { pgprot_t vm_page_prot; + int ret; + + ret = drm_gem_mmap_obj(&etnaviv_obj->base, + drm_vma_node_size(&etnaviv_obj->base.vma_node) << PAGE_SHIFT, + vma); + if (ret) + return ret; vma->vm_flags &= ~VM_PFNMAP; vma->vm_flags |= VM_MIXEDMAP; @@ -167,17 +174,26 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma) { - struct etnaviv_gem_object *obj; + struct drm_file *priv = filp->private_data; + struct etnaviv_gem_object *etnaviv_obj; + struct drm_gem_object *obj; int ret; - ret = drm_gem_mmap(filp, vma); - if (ret) { - DBG("mmap failed: %d", ret); - return ret; + obj = drm_gem_bo_vm_lookup(filp, vma); + if (!obj) + return -EINVAL; + + if (!drm_vma_node_is_allowed(&obj->vma_node, priv)) { + drm_gem_object_put_unlocked(obj); + return -EACCES; } - obj = to_etnaviv_bo(vma->vm_private_data); - return obj->ops->mmap(obj, vma); + etnaviv_obj = to_etnaviv_bo(obj); + + ret = etnaviv_obj->ops->mmap(etnaviv_obj, vma); + drm_gem_object_put_unlocked(obj); + + return ret; } int etnaviv_gem_fault(struct vm_fault *vmf) -- 2.17.0 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers 2018-05-25 13:42 ` [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers Lucas Stach @ 2018-05-29 8:20 ` Daniel Vetter 2018-05-29 9:08 ` Lucas Stach 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2018-05-29 8:20 UTC (permalink / raw) To: Lucas Stach; +Cc: Julien Boulnois, Tomi Valkeinen, dri-devel On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote: > The intention of the existing code was to deflect the actual work > of mmaping a dma-buf to the exporter, as that one probably knows best > how to handle the buffer. Unfortunately the call to drm_gem_mmap did > more than what etnaviv needs in this case by actually setting up the > mapping. > > Move mapping setup to the shm buffer type mmap implementation so we > only need to look up the BO and call the buffer type mmap function > from the handler. > > Fixes mmap behavior with dma-buf imported and userptr buffers. You allow mmap on userptr buffers? That sounds really nasty ... Also not really thrilled about dma-buf mmap forwarding either, since you don't seem to forward the begin/end_cpu_access stuff either. -Daniel > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/gpu/drm/etnaviv/etnaviv_gem.c | 30 ++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c > index fcc969fa0e69..f38989960d7f 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c > @@ -138,6 +138,13 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, > struct vm_area_struct *vma) > { > pgprot_t vm_page_prot; > + int ret; > + > + ret = drm_gem_mmap_obj(&etnaviv_obj->base, > + drm_vma_node_size(&etnaviv_obj->base.vma_node) << PAGE_SHIFT, > + vma); > + if (ret) > + return ret; > > vma->vm_flags &= ~VM_PFNMAP; > vma->vm_flags |= VM_MIXEDMAP; > @@ -167,17 +174,26 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, > > int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma) > { > - struct etnaviv_gem_object *obj; > + struct drm_file *priv = filp->private_data; > + struct etnaviv_gem_object *etnaviv_obj; > + struct drm_gem_object *obj; > int ret; > > - ret = drm_gem_mmap(filp, vma); > - if (ret) { > - DBG("mmap failed: %d", ret); > - return ret; > + obj = drm_gem_bo_vm_lookup(filp, vma); > + if (!obj) > + return -EINVAL; > + > + if (!drm_vma_node_is_allowed(&obj->vma_node, priv)) { > + drm_gem_object_put_unlocked(obj); > + return -EACCES; > } > > - obj = to_etnaviv_bo(vma->vm_private_data); > - return obj->ops->mmap(obj, vma); > + etnaviv_obj = to_etnaviv_bo(obj); > + > + ret = etnaviv_obj->ops->mmap(etnaviv_obj, vma); > + drm_gem_object_put_unlocked(obj); > + > + return ret; > } > > int etnaviv_gem_fault(struct vm_fault *vmf) > -- > 2.17.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers 2018-05-29 8:20 ` Daniel Vetter @ 2018-05-29 9:08 ` Lucas Stach 2018-05-29 12:34 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Lucas Stach @ 2018-05-29 9:08 UTC (permalink / raw) To: Daniel Vetter; +Cc: Julien Boulnois, Tomi Valkeinen, dri-devel Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter: > On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote: > > The intention of the existing code was to deflect the actual work > > of mmaping a dma-buf to the exporter, as that one probably knows best > > how to handle the buffer. Unfortunately the call to drm_gem_mmap did > > more than what etnaviv needs in this case by actually setting up the > > mapping. > > > > Move mapping setup to the shm buffer type mmap implementation so we > > only need to look up the BO and call the buffer type mmap function > > from the handler. > > > > Fixes mmap behavior with dma-buf imported and userptr buffers. > > You allow mmap on userptr buffers? That sounds really nasty ... No, we don't because that's obviously crazy, even more so on ARM with it's special rules about aliasing mappings. The current code is buggy in that it first sets up the mapping and then tells userspace the mmap failed. After this patch we properly reject userptr mmap outright. > Also not really thrilled about dma-buf mmap forwarding either, since you > don't seem to forward the begin/end_cpu_access stuff either. Yeah, that's still missing, but IMHO more of a correctness fix (which can be done in a follow on patch), distinct of the bugfix in this patch. Regards, Lucas > -Daniel > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > drivers/gpu/drm/etnaviv/etnaviv_gem.c | 30 ++++++++++++++++++++------- > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c > > index fcc969fa0e69..f38989960d7f 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c > > @@ -138,6 +138,13 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, > > struct vm_area_struct *vma) > > { > > pgprot_t vm_page_prot; > > + int ret; > > + > > + ret = drm_gem_mmap_obj(&etnaviv_obj->base, > > + drm_vma_node_size(&etnaviv_obj->base.vma_node) << PAGE_SHIFT, > > + vma); > > + if (ret) > > + return ret; > > > > vma->vm_flags &= ~VM_PFNMAP; > > vma->vm_flags |= VM_MIXEDMAP; > > @@ -167,17 +174,26 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, > > > > int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma) > > { > > - struct etnaviv_gem_object *obj; > > + struct drm_file *priv = filp->private_data; > > + struct etnaviv_gem_object *etnaviv_obj; > > + struct drm_gem_object *obj; > > int ret; > > > > - ret = drm_gem_mmap(filp, vma); > > - if (ret) { > > - DBG("mmap failed: %d", ret); > > - return ret; > > + obj = drm_gem_bo_vm_lookup(filp, vma); > > + if (!obj) > > + return -EINVAL; > > + > > + if (!drm_vma_node_is_allowed(&obj->vma_node, priv)) { > > + drm_gem_object_put_unlocked(obj); > > + return -EACCES; > > } > > > > - obj = to_etnaviv_bo(vma->vm_private_data); > > - return obj->ops->mmap(obj, vma); > > + etnaviv_obj = to_etnaviv_bo(obj); > > + > > + ret = etnaviv_obj->ops->mmap(etnaviv_obj, vma); > > + drm_gem_object_put_unlocked(obj); > > + > > + return ret; > > } > > > > int etnaviv_gem_fault(struct vm_fault *vmf) > > -- > > 2.17.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers 2018-05-29 9:08 ` Lucas Stach @ 2018-05-29 12:34 ` Daniel Vetter 2018-05-31 8:41 ` Lucas Stach 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2018-05-29 12:34 UTC (permalink / raw) To: Lucas Stach; +Cc: Julien Boulnois, Tomi Valkeinen, dri-devel On Tue, May 29, 2018 at 11:08 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter: >> On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote: >> > The intention of the existing code was to deflect the actual work >> > of mmaping a dma-buf to the exporter, as that one probably knows best >> > how to handle the buffer. Unfortunately the call to drm_gem_mmap did >> > more than what etnaviv needs in this case by actually setting up the >> > mapping. >> > >> > Move mapping setup to the shm buffer type mmap implementation so we >> > only need to look up the BO and call the buffer type mmap function >> > from the handler. >> > >> > Fixes mmap behavior with dma-buf imported and userptr buffers. >> >> You allow mmap on userptr buffers? That sounds really nasty ... > > No, we don't because that's obviously crazy, even more so on ARM with > it's special rules about aliasing mappings. The current code is buggy > in that it first sets up the mapping and then tells userspace the mmap > failed. After this patch we properly reject userptr mmap outright. Ah, I didn't realize that. It sounded more like making userptr mmap work, not rejecting it. Can't you instead just never register the mmap offset for userptr objects? That would catch the invalid request even earlier, and make it more obvious that mmap is not ok for userptr. Would also remove the need to hand-roll an etnaviv version of drm_gem_obj_mmap. >> Also not really thrilled about dma-buf mmap forwarding either, since you >> don't seem to forward the begin/end_cpu_access stuff either. > > Yeah, that's still missing, but IMHO more of a correctness fix (which > can be done in a follow on patch), distinct of the bugfix in this > patch. Yeah drm_gem_obj_mmap should check for obj->import_attach imo and scream. Maybe we should even have that check when allocating the mmap offset, since having an mmap offset for something you can never mmap is silly. And obj->import_attach isn't allowed to change over the lifetime of an object. -Daniel > > Regards, > Lucas >> -Daniel >> >> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> >> > --- >> > drivers/gpu/drm/etnaviv/etnaviv_gem.c | 30 ++++++++++++++++++++------- >> > 1 file changed, 23 insertions(+), 7 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c >> > index fcc969fa0e69..f38989960d7f 100644 >> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c >> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c >> > @@ -138,6 +138,13 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, >> > struct vm_area_struct *vma) >> > { >> > pgprot_t vm_page_prot; >> > + int ret; >> > + >> > + ret = drm_gem_mmap_obj(&etnaviv_obj->base, >> > + drm_vma_node_size(&etnaviv_obj->base.vma_node) << PAGE_SHIFT, >> > + vma); >> > + if (ret) >> > + return ret; >> > >> > vma->vm_flags &= ~VM_PFNMAP; >> > vma->vm_flags |= VM_MIXEDMAP; >> > @@ -167,17 +174,26 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, >> > >> > int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma) >> > { >> > - struct etnaviv_gem_object *obj; >> > + struct drm_file *priv = filp->private_data; >> > + struct etnaviv_gem_object *etnaviv_obj; >> > + struct drm_gem_object *obj; >> > int ret; >> > >> > - ret = drm_gem_mmap(filp, vma); >> > - if (ret) { >> > - DBG("mmap failed: %d", ret); >> > - return ret; >> > + obj = drm_gem_bo_vm_lookup(filp, vma); >> > + if (!obj) >> > + return -EINVAL; >> > + >> > + if (!drm_vma_node_is_allowed(&obj->vma_node, priv)) { >> > + drm_gem_object_put_unlocked(obj); >> > + return -EACCES; >> > } >> > >> > - obj = to_etnaviv_bo(vma->vm_private_data); >> > - return obj->ops->mmap(obj, vma); >> > + etnaviv_obj = to_etnaviv_bo(obj); >> > + >> > + ret = etnaviv_obj->ops->mmap(etnaviv_obj, vma); >> > + drm_gem_object_put_unlocked(obj); >> > + >> > + return ret; >> > } >> > >> > int etnaviv_gem_fault(struct vm_fault *vmf) >> > -- >> > 2.17.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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers 2018-05-29 12:34 ` Daniel Vetter @ 2018-05-31 8:41 ` Lucas Stach 2018-06-18 8:06 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Lucas Stach @ 2018-05-31 8:41 UTC (permalink / raw) To: Daniel Vetter; +Cc: Julien Boulnois, Tomi Valkeinen, dri-devel Am Dienstag, den 29.05.2018, 14:34 +0200 schrieb Daniel Vetter: > On Tue, May 29, 2018 at 11:08 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter: > > > On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote: > > > > The intention of the existing code was to deflect the actual work > > > > of mmaping a dma-buf to the exporter, as that one probably knows best > > > > how to handle the buffer. Unfortunately the call to drm_gem_mmap did > > > > more than what etnaviv needs in this case by actually setting up the > > > > mapping. > > > > > > > > Move mapping setup to the shm buffer type mmap implementation so we > > > > only need to look up the BO and call the buffer type mmap function > > > > from the handler. > > > > > > > > Fixes mmap behavior with dma-buf imported and userptr buffers. > > > > > > You allow mmap on userptr buffers? That sounds really nasty ... > > > > No, we don't because that's obviously crazy, even more so on ARM with > > it's special rules about aliasing mappings. The current code is buggy > > in that it first sets up the mapping and then tells userspace the mmap > > failed. After this patch we properly reject userptr mmap outright. > > Ah, I didn't realize that. It sounded more like making userptr mmap > work, not rejecting it. Can't you instead just never register the mmap > offset for userptr objects? That would catch the invalid request even > earlier, and make it more obvious that mmap is not ok for userptr. > Would also remove the need to hand-roll an etnaviv version of > drm_gem_obj_mmap. Makes sense for userptr, not so much for imported dma-bufs, see below. > > > Also not really thrilled about dma-buf mmap forwarding either, since you > > > don't seem to forward the begin/end_cpu_access stuff either. > > > > Yeah, that's still missing, but IMHO more of a correctness fix (which > > can be done in a follow on patch), distinct of the bugfix in this > > patch. > > Yeah drm_gem_obj_mmap should check for obj->import_attach imo and > scream. Maybe we should even have that check when allocating the mmap > offset, since having an mmap offset for something you can never mmap > is silly. And obj->import_attach isn't allowed to change over the > lifetime of an object. Agreed for drm_gem_obj_mmap, but I don't follow why we would reject mmap of an imported dma-buf. This seems to be a feature we want today for drivers that need to talk to multiple DRM devices, like Etnaviv, Tegra and VC4 in some cases. Making the mmap look uniform by allowing the mmap on one device and internally deflecting to the exporter is a big pro from the userspace driver writer PoV. Also if you think about stuff like ION (not that I would agree that ION is a good idea in general, but whatever), a lot of buffers end up being allocated by ION. I don't want driver writers to care where a buffer was allocated, but rather call the usual mmap on the device they are working with and do the right thing in the kernel. Maybe we can just generalize the deflecting to the exporter and implement it in the DRM core, rather than rolling our own version in etnaviv. Regards, Lucas _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers 2018-05-31 8:41 ` Lucas Stach @ 2018-06-18 8:06 ` Daniel Vetter 2019-07-03 10:47 ` Lucas Stach 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2018-06-18 8:06 UTC (permalink / raw) To: Lucas Stach; +Cc: Julien Boulnois, Tomi Valkeinen, dri-devel On Thu, May 31, 2018 at 10:41:25AM +0200, Lucas Stach wrote: > Am Dienstag, den 29.05.2018, 14:34 +0200 schrieb Daniel Vetter: > > On Tue, May 29, 2018 at 11:08 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > > > Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter: > > > > On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote: > > > > > The intention of the existing code was to deflect the actual work > > > > > of mmaping a dma-buf to the exporter, as that one probably knows best > > > > > how to handle the buffer. Unfortunately the call to drm_gem_mmap did > > > > > more than what etnaviv needs in this case by actually setting up the > > > > > mapping. > > > > > > > > > > Move mapping setup to the shm buffer type mmap implementation so we > > > > > only need to look up the BO and call the buffer type mmap function > > > > > from the handler. > > > > > > > > > > Fixes mmap behavior with dma-buf imported and userptr buffers. > > > > > > > > You allow mmap on userptr buffers? That sounds really nasty ... > > > > > > No, we don't because that's obviously crazy, even more so on ARM with > > > it's special rules about aliasing mappings. The current code is buggy > > > in that it first sets up the mapping and then tells userspace the mmap > > > failed. After this patch we properly reject userptr mmap outright. > > > > Ah, I didn't realize that. It sounded more like making userptr mmap > > work, not rejecting it. Can't you instead just never register the mmap > > offset for userptr objects? That would catch the invalid request even > > earlier, and make it more obvious that mmap is not ok for userptr. > > Would also remove the need to hand-roll an etnaviv version of > > drm_gem_obj_mmap. > > Makes sense for userptr, not so much for imported dma-bufs, see below. > > > > > Also not really thrilled about dma-buf mmap forwarding either, since you > > > > don't seem to forward the begin/end_cpu_access stuff either. > > > > > > Yeah, that's still missing, but IMHO more of a correctness fix (which > > > can be done in a follow on patch), distinct of the bugfix in this > > > patch. > > > > Yeah drm_gem_obj_mmap should check for obj->import_attach imo and > > scream. Maybe we should even have that check when allocating the mmap > > offset, since having an mmap offset for something you can never mmap > > is silly. And obj->import_attach isn't allowed to change over the > > lifetime of an object. > > Agreed for drm_gem_obj_mmap, but I don't follow why we would reject > mmap of an imported dma-buf. This seems to be a feature we want today > for drivers that need to talk to multiple DRM devices, like Etnaviv, > Tegra and VC4 in some cases. Making the mmap look uniform by allowing > the mmap on one device and internally deflecting to the exporter is a > big pro from the userspace driver writer PoV. > > Also if you think about stuff like ION (not that I would agree that ION > is a good idea in general, but whatever), a lot of buffers end up being > allocated by ION. I don't want driver writers to care where a buffer > was allocated, but rather call the usual mmap on the device they are > working with and do the right thing in the kernel. > > Maybe we can just generalize the deflecting to the exporter and > implement it in the DRM core, rather than rolling our own version in > etnaviv. The problem is more that most driver uapi for mmap doesn't bother much with cache coherency ioctls (I think yours does), which makes dma-buf mmap in the general case a no-go. So relying on it working seems ill-advised. But in the end it's a matter of making stuff work in reality, not for the full general case, and for that I guess you sufficiently control all the pieces (e.g. you know you'll only ever deal with coherent buffers) to make this work. I guess if there's demand, a general mmap reflector for gem would be much better than the state of the art of everyone copypasting the same logic. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers 2018-06-18 8:06 ` Daniel Vetter @ 2019-07-03 10:47 ` Lucas Stach 2019-07-03 13:16 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Lucas Stach @ 2019-07-03 10:47 UTC (permalink / raw) To: Daniel Vetter; +Cc: Julien Boulnois, Tomi Valkeinen, dri-devel Hi Daniel, discussion on this somehow died quite a while ago. As the bug is still present in etnaviv, I would like to get it going again. Am Montag, den 18.06.2018, 10:06 +0200 schrieb Daniel Vetter: > On Thu, May 31, 2018 at 10:41:25AM +0200, Lucas Stach wrote: > > Am Dienstag, den 29.05.2018, 14:34 +0200 schrieb Daniel Vetter: > > > On Tue, May 29, 2018 at 11:08 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter: > > > > > On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote: > > > > > > The intention of the existing code was to deflect the actual work > > > > > > of mmaping a dma-buf to the exporter, as that one probably knows best > > > > > > how to handle the buffer. Unfortunately the call to drm_gem_mmap did > > > > > > more than what etnaviv needs in this case by actually setting up the > > > > > > mapping. > > > > > > > > > > > > Move mapping setup to the shm buffer type mmap implementation so we > > > > > > only need to look up the BO and call the buffer type mmap function > > > > > > from the handler. > > > > > > > > > > > > Fixes mmap behavior with dma-buf imported and userptr buffers. > > > > > > > > > > You allow mmap on userptr buffers? That sounds really nasty ... > > > > > > > > No, we don't because that's obviously crazy, even more so on ARM with > > > > it's special rules about aliasing mappings. The current code is buggy > > > > in that it first sets up the mapping and then tells userspace the mmap > > > > failed. After this patch we properly reject userptr mmap outright. > > > > > > Ah, I didn't realize that. It sounded more like making userptr mmap > > > work, not rejecting it. Can't you instead just never register the mmap > > > offset for userptr objects? That would catch the invalid request even > > > earlier, and make it more obvious that mmap is not ok for userptr. > > > Would also remove the need to hand-roll an etnaviv version of > > > drm_gem_obj_mmap. > > > > Makes sense for userptr, not so much for imported dma-bufs, see below. > > > > > > > Also not really thrilled about dma-buf mmap forwarding either, since you > > > > > don't seem to forward the begin/end_cpu_access stuff either. > > > > > > > > Yeah, that's still missing, but IMHO more of a correctness fix (which > > > > can be done in a follow on patch), distinct of the bugfix in this > > > > patch. > > > > > > Yeah drm_gem_obj_mmap should check for obj->import_attach imo and > > > scream. Maybe we should even have that check when allocating the mmap > > > offset, since having an mmap offset for something you can never mmap > > > is silly. And obj->import_attach isn't allowed to change over the > > > lifetime of an object. > > > > Agreed for drm_gem_obj_mmap, but I don't follow why we would reject > > mmap of an imported dma-buf. This seems to be a feature we want today > > for drivers that need to talk to multiple DRM devices, like Etnaviv, > > Tegra and VC4 in some cases. Making the mmap look uniform by allowing > > the mmap on one device and internally deflecting to the exporter is a > > big pro from the userspace driver writer PoV. > > > > Also if you think about stuff like ION (not that I would agree that ION > > is a good idea in general, but whatever), a lot of buffers end up being > > allocated by ION. I don't want driver writers to care where a buffer > > was allocated, but rather call the usual mmap on the device they are > > working with and do the right thing in the kernel. > > > > Maybe we can just generalize the deflecting to the exporter and > > implement it in the DRM core, rather than rolling our own version in > > etnaviv. > > The problem is more that most driver uapi for mmap doesn't bother much > with cache coherency ioctls (I think yours does), which makes dma-buf mmap > in the general case a no-go. So relying on it working seems ill-advised. Cache coherency is something that should be dealt with in the begin/end_cpu_access functions. I guess every GPU driver has something like this, as you need it to synchronize CPU access with the GPU anyways. Currently this isn't hooked up in DRM for exported dma-bufs, but shouldn't be a big deal to do. > But in the end it's a matter of making stuff work in reality, not for the > full general case, and for that I guess you sufficiently control all the > pieces (e.g. you know you'll only ever deal with coherent buffers) to make > this work. > > I guess if there's demand, a general mmap reflector for gem would be much > better than the state of the art of everyone copypasting the same logic. I don't think a generic mmap reflector will work out, specifically because drivers need to hook the dma-buf begin/end_cpu_access stuff into their drivers specific cpu_prep/fini ioctls to make this work properly. That said would you be okay with me just merging this series and going from there? It's definitely an improvement of the status-quo on etnaviv, as currently we allow to mmap dma-bufs, but then leak a reference. Regards, Lucas _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers 2019-07-03 10:47 ` Lucas Stach @ 2019-07-03 13:16 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2019-07-03 13:16 UTC (permalink / raw) To: Lucas Stach; +Cc: Julien Boulnois, Tomi Valkeinen, dri-devel On Wed, Jul 3, 2019 at 12:47 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Daniel, > > discussion on this somehow died quite a while ago. As the bug is still > present in etnaviv, I would like to get it going again. > > Am Montag, den 18.06.2018, 10:06 +0200 schrieb Daniel Vetter: > > On Thu, May 31, 2018 at 10:41:25AM +0200, Lucas Stach wrote: > > > Am Dienstag, den 29.05.2018, 14:34 +0200 schrieb Daniel Vetter: > > > > On Tue, May 29, 2018 at 11:08 AM, Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > Am Dienstag, den 29.05.2018, 10:20 +0200 schrieb Daniel Vetter: > > > > > > On Fri, May 25, 2018 at 03:42:54PM +0200, Lucas Stach wrote: > > > > > > > The intention of the existing code was to deflect the actual work > > > > > > > of mmaping a dma-buf to the exporter, as that one probably knows best > > > > > > > how to handle the buffer. Unfortunately the call to drm_gem_mmap did > > > > > > > more than what etnaviv needs in this case by actually setting up the > > > > > > > mapping. > > > > > > > > > > > > > > Move mapping setup to the shm buffer type mmap implementation so we > > > > > > > only need to look up the BO and call the buffer type mmap function > > > > > > > from the handler. > > > > > > > > > > > > > > Fixes mmap behavior with dma-buf imported and userptr buffers. > > > > > > > > > > > > You allow mmap on userptr buffers? That sounds really nasty ... > > > > > > > > > > No, we don't because that's obviously crazy, even more so on ARM with > > > > > it's special rules about aliasing mappings. The current code is buggy > > > > > in that it first sets up the mapping and then tells userspace the mmap > > > > > failed. After this patch we properly reject userptr mmap outright. > > > > > > > > Ah, I didn't realize that. It sounded more like making userptr mmap > > > > work, not rejecting it. Can't you instead just never register the mmap > > > > offset for userptr objects? That would catch the invalid request even > > > > earlier, and make it more obvious that mmap is not ok for userptr. > > > > Would also remove the need to hand-roll an etnaviv version of > > > > drm_gem_obj_mmap. > > > > > > Makes sense for userptr, not so much for imported dma-bufs, see below. > > > > > > > > > Also not really thrilled about dma-buf mmap forwarding either, since you > > > > > > don't seem to forward the begin/end_cpu_access stuff either. > > > > > > > > > > Yeah, that's still missing, but IMHO more of a correctness fix (which > > > > > can be done in a follow on patch), distinct of the bugfix in this > > > > > patch. > > > > > > > > Yeah drm_gem_obj_mmap should check for obj->import_attach imo and > > > > scream. Maybe we should even have that check when allocating the mmap > > > > offset, since having an mmap offset for something you can never mmap > > > > is silly. And obj->import_attach isn't allowed to change over the > > > > lifetime of an object. > > > > > > Agreed for drm_gem_obj_mmap, but I don't follow why we would reject > > > mmap of an imported dma-buf. This seems to be a feature we want today > > > for drivers that need to talk to multiple DRM devices, like Etnaviv, > > > Tegra and VC4 in some cases. Making the mmap look uniform by allowing > > > the mmap on one device and internally deflecting to the exporter is a > > > big pro from the userspace driver writer PoV. > > > > > > Also if you think about stuff like ION (not that I would agree that ION > > > is a good idea in general, but whatever), a lot of buffers end up being > > > allocated by ION. I don't want driver writers to care where a buffer > > > was allocated, but rather call the usual mmap on the device they are > > > working with and do the right thing in the kernel. > > > > > > Maybe we can just generalize the deflecting to the exporter and > > > implement it in the DRM core, rather than rolling our own version in > > > etnaviv. > > > > The problem is more that most driver uapi for mmap doesn't bother much > > with cache coherency ioctls (I think yours does), which makes dma-buf mmap > > in the general case a no-go. So relying on it working seems ill-advised. > > Cache coherency is something that should be dealt with in the > begin/end_cpu_access functions. I guess every GPU driver has something > like this, as you need it to synchronize CPU access with the GPU > anyways. Hm, still not a fan of allowing gem mmap on imported buffers. I have no idea why you'd want that, since I expect userspace will only render into such buffers with the gpu. But I guess if you use that already, *shrug* > Currently this isn't hooked up in DRM for exported dma-bufs, but > shouldn't be a big deal to do. Well the drm_prime "helpers" exist so the nv blob can dma-buf export without touching EXPORT_SYMBOL_GPL stuff :-) Just ignore it and roll your own stuff (you can still reuse most of it if you want, it's a true helper in that regard), you can do begin/end_cpu_access easily that way. And no problems for you with not being gpl licensed ... > > But in the end it's a matter of making stuff work in reality, not for the > > full general case, and for that I guess you sufficiently control all the > > pieces (e.g. you know you'll only ever deal with coherent buffers) to make > > this work. > > > > I guess if there's demand, a general mmap reflector for gem would be much > > better than the state of the art of everyone copypasting the same logic. > > I don't think a generic mmap reflector will work out, specifically > because drivers need to hook the dma-buf begin/end_cpu_access stuff > into their drivers specific cpu_prep/fini ioctls to make this work > properly. For anyone who uses coherent dma (i.e. most display-only drivers) it'll work perfectly. And since you don't have begin/end_cpu_access wired up, that shouldn't be worse for you? Since we've had this discussion this generic mmap reflector actually landed. But the reflector only works for exporting, on importing we reject gem mmap on dma-bufs now, at least in the helpers. > That said would you be okay with me just merging this series and going > from there? It's definitely an improvement of the status-quo on > etnaviv, as currently we allow to mmap dma-bufs, but then leak a > reference. *shrug* Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-03 13:17 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-25 13:42 [PATCH 1/2] drm: split out buffer lookup from fake VMA offset Lucas Stach 2018-05-25 13:42 ` [PATCH 2/2] drm/etnaviv: properly implement mmaping of imported buffers Lucas Stach 2018-05-29 8:20 ` Daniel Vetter 2018-05-29 9:08 ` Lucas Stach 2018-05-29 12:34 ` Daniel Vetter 2018-05-31 8:41 ` Lucas Stach 2018-06-18 8:06 ` Daniel Vetter 2019-07-03 10:47 ` Lucas Stach 2019-07-03 13:16 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).