| Message ID | 20180525134254.30686-2-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 > >
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 >> >>
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
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
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
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
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)
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(-)