On Wed, Jul 3, 2019 at 7:34 AM Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote: > > Exported BOs might be imported back, then mmap()-ed to be written > too. Most drivers handle that by mmap()-ing the GEM handle after it's > been imported, but, according to [1], this is illegal. It's not illegal, but is supposed to go thru the dmabuf mmap functions. However, none of the driver I've looked at (etnaviv, msm, v3d, vgem) do that. It probably works because it's the same driver doing the import and export or both drivers have essentially the same implementations. > The panfrost > driver has recently switched to this generic helper (which was renamed > into drm_gem_map_offset() in the meantime) [2], and mmap()-ing > of imported BOs now fails. I think I'm going to revert this. > Now I'm wondering how this should be solved. I guess the first question > is, is mmap()-ing of imported BOs really forbidden for all BOs? I guess > calling mmap() on a buffer that's been exported by the DRM driver itself > then re-imported shouldn't hurt, so maybe we can check that before > failing. > > Now, if we really want to forbid mmap() on imported BOs, that means we > need a solution to mmap() the dmabuf object directly, and sometimes this > mapping will request RW permissions. The problem is, all function > exporting BOs in mesa are exporting them in RO-mode (resulting FD is > O_READ), thus preventing mmap()s in RW mode. > > This patch modifies all > drmPrimeHandleToFD()/ioctl(DRM_IOCTL_PRIME_HANDLE_TO_FD) call sites > to pass the DRM_RDWR flag so that what's described above becomes > possible. > > I'm not saying this is what we should do, it's more a way to start the > discussion. Feel free to propose alternives to this solution. > > [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_gem.c?h=v5.2-rc7#n318 > [2]https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/panfrost?id=583bbf46133c726bae277e8f4e32bfba2a528c7f > > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> > Cc: <dri-devel@xxxxxxxxxxxxxxxxxxxxx> > Cc: <mesa-dev@xxxxxxxxxxxxxxxxxxxxx> > Cc: Steven Price <steven.price@xxxxxxx> > Cc: Rob Herring <robh@xxxxxxxxxx> > --- > Cc-ing dri-devel is not a mistake, I really to have feedback from the DRM > maintainers, since this started with a kernel-side change. > --- > src/etnaviv/drm/etnaviv_bo.c | 4 ++-- > src/freedreno/drm/freedreno_bo.c | 4 ++-- > src/freedreno/vulkan/tu_drm.c | 2 +- > src/gallium/auxiliary/renderonly/renderonly.c | 2 +- > src/gallium/drivers/iris/iris_bufmgr.c | 2 +- > src/gallium/drivers/lima/lima_bo.c | 2 +- > src/gallium/drivers/panfrost/pan_drm.c | 2 +- > src/gallium/drivers/v3d/v3d_bufmgr.c | 2 +- > src/gallium/drivers/vc4/vc4_bufmgr.c | 2 +- > src/gallium/winsys/radeon/drm/radeon_drm_bo.c | 3 ++- > src/gallium/winsys/svga/drm/vmw_screen_dri.c | 5 +++-- > src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 2 +- > src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 3 ++- > src/intel/vulkan/anv_gem.c | 2 +- > src/mesa/drivers/dri/i965/brw_bufmgr.c | 2 +- > 15 files changed, 21 insertions(+), 18 deletions(-) > > diff --git a/src/etnaviv/drm/etnaviv_bo.c b/src/etnaviv/drm/etnaviv_bo.c > index 6e952fa47858..92634141b580 100644 > --- a/src/etnaviv/drm/etnaviv_bo.c > +++ b/src/etnaviv/drm/etnaviv_bo.c > @@ -294,8 +294,8 @@ int etna_bo_dmabuf(struct etna_bo *bo) > { > int ret, prime_fd; > > - ret = drmPrimeHandleToFD(bo->dev->fd, bo->handle, DRM_CLOEXEC, > - &prime_fd); > + ret = drmPrimeHandleToFD(bo->dev->fd, bo->handle, > + DRM_CLOEXEC | DRM_RDWR, &prime_fd); > if (ret) { > ERROR_MSG("failed to get dmabuf fd: %d", ret); > return ret; > diff --git a/src/freedreno/drm/freedreno_bo.c b/src/freedreno/drm/freedreno_bo.c > index 7449160f1371..ba19b08d7c54 100644 > --- a/src/freedreno/drm/freedreno_bo.c > +++ b/src/freedreno/drm/freedreno_bo.c > @@ -318,8 +318,8 @@ int fd_bo_dmabuf(struct fd_bo *bo) > { > int ret, prime_fd; > > - ret = drmPrimeHandleToFD(bo->dev->fd, bo->handle, DRM_CLOEXEC, > - &prime_fd); > + ret = drmPrimeHandleToFD(bo->dev->fd, bo->handle, > + DRM_CLOEXEC | DRM_RDWR, &prime_fd); > if (ret) { > ERROR_MSG("failed to get dmabuf fd: %d", ret); > return ret; > diff --git a/src/freedreno/vulkan/tu_drm.c b/src/freedreno/vulkan/tu_drm.c > index 9b2e6f78879e..6bef3012ddb5 100644 > --- a/src/freedreno/vulkan/tu_drm.c > +++ b/src/freedreno/vulkan/tu_drm.c > @@ -147,7 +147,7 @@ tu_gem_export_dmabuf(const struct tu_device *dev, uint32_t gem_handle) > { > int prime_fd; > int ret = drmPrimeHandleToFD(dev->physical_device->local_fd, gem_handle, > - DRM_CLOEXEC, &prime_fd); > + DRM_CLOEXEC | DRM_RDWR, &prime_fd); > > return ret == 0 ? prime_fd : -1; > } > diff --git a/src/gallium/auxiliary/renderonly/renderonly.c b/src/gallium/auxiliary/renderonly/renderonly.c > index d6a344009378..c1cc31115105 100644 > --- a/src/gallium/auxiliary/renderonly/renderonly.c > +++ b/src/gallium/auxiliary/renderonly/renderonly.c > @@ -101,7 +101,7 @@ renderonly_create_kms_dumb_buffer_for_resource(struct pipe_resource *rsc, > out_handle->type = WINSYS_HANDLE_TYPE_FD; > out_handle->stride = create_dumb.pitch; > > - err = drmPrimeHandleToFD(ro->kms_fd, create_dumb.handle, O_CLOEXEC, > + err = drmPrimeHandleToFD(ro->kms_fd, create_dumb.handle, O_CLOEXEC | O_RDWR, > (int *)&out_handle->handle); > if (err < 0) { > fprintf(stderr, "failed to export dumb buffer: %s\n", strerror(errno)); > diff --git a/src/gallium/drivers/iris/iris_bufmgr.c b/src/gallium/drivers/iris/iris_bufmgr.c > index d80945824098..68e5d8a8bb13 100644 > --- a/src/gallium/drivers/iris/iris_bufmgr.c > +++ b/src/gallium/drivers/iris/iris_bufmgr.c > @@ -1369,7 +1369,7 @@ iris_bo_export_dmabuf(struct iris_bo *bo, int *prime_fd) > iris_bo_make_external(bo); > > if (drmPrimeHandleToFD(bufmgr->fd, bo->gem_handle, > - DRM_CLOEXEC, prime_fd) != 0) > + DRM_CLOEXEC | DRM_RDWR, prime_fd) != 0) > return -errno; > > bo->reusable = false; > diff --git a/src/gallium/drivers/lima/lima_bo.c b/src/gallium/drivers/lima/lima_bo.c > index 1d6dd720602a..df0114fc67ed 100644 > --- a/src/gallium/drivers/lima/lima_bo.c > +++ b/src/gallium/drivers/lima/lima_bo.c > @@ -205,7 +205,7 @@ bool lima_bo_export(struct lima_bo *bo, struct winsys_handle *handle) > return true; > > case WINSYS_HANDLE_TYPE_FD: > - if (drmPrimeHandleToFD(screen->fd, bo->handle, DRM_CLOEXEC, > + if (drmPrimeHandleToFD(screen->fd, bo->handle, DRM_CLOEXEC | DRM_RDWR, > (int*)&handle->handle)) > return false; > > diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c > index 8de4f483435c..a086575c453a 100644 > --- a/src/gallium/drivers/panfrost/pan_drm.c > +++ b/src/gallium/drivers/panfrost/pan_drm.c > @@ -181,7 +181,7 @@ panfrost_drm_export_bo(struct panfrost_screen *screen, const struct panfrost_bo > { > struct drm_prime_handle args = { > .handle = bo->gem_handle, > - .flags = DRM_CLOEXEC, > + .flags = DRM_CLOEXEC | DRM_RDWR, > }; > > int ret = drmIoctl(screen->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &args); > diff --git a/src/gallium/drivers/v3d/v3d_bufmgr.c b/src/gallium/drivers/v3d/v3d_bufmgr.c > index f0df1b983737..0c78994d899f 100644 > --- a/src/gallium/drivers/v3d/v3d_bufmgr.c > +++ b/src/gallium/drivers/v3d/v3d_bufmgr.c > @@ -424,7 +424,7 @@ v3d_bo_get_dmabuf(struct v3d_bo *bo) > { > int fd; > int ret = drmPrimeHandleToFD(bo->screen->fd, bo->handle, > - O_CLOEXEC, &fd); > + DRM_CLOEXEC | DRM_RDWR, &fd); > if (ret != 0) { > fprintf(stderr, "Failed to export gem bo %d to dmabuf\n", > bo->handle); > diff --git a/src/gallium/drivers/vc4/vc4_bufmgr.c b/src/gallium/drivers/vc4/vc4_bufmgr.c > index 716ca50ea069..c7e89b6a517e 100644 > --- a/src/gallium/drivers/vc4/vc4_bufmgr.c > +++ b/src/gallium/drivers/vc4/vc4_bufmgr.c > @@ -462,7 +462,7 @@ vc4_bo_get_dmabuf(struct vc4_bo *bo) > { > int fd; > int ret = drmPrimeHandleToFD(bo->screen->fd, bo->handle, > - O_CLOEXEC, &fd); > + DRM_CLOEXEC | DRM_RDWR, &fd); > if (ret != 0) { > fprintf(stderr, "Failed to export gem bo %d to dmabuf\n", > bo->handle); > diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_bo.c b/src/gallium/winsys/radeon/drm/radeon_drm_bo.c > index d1e2a8685ba9..ae7958a8b892 100644 > --- a/src/gallium/winsys/radeon/drm/radeon_drm_bo.c > +++ b/src/gallium/winsys/radeon/drm/radeon_drm_bo.c > @@ -1320,7 +1320,8 @@ static bool radeon_winsys_bo_get_handle(struct pb_buffer *buffer, > } else if (whandle->type == WINSYS_HANDLE_TYPE_KMS) { > whandle->handle = bo->handle; > } else if (whandle->type == WINSYS_HANDLE_TYPE_FD) { > - if (drmPrimeHandleToFD(ws->fd, bo->handle, DRM_CLOEXEC, (int*)&whandle->handle)) > + if (drmPrimeHandleToFD(ws->fd, bo->handle, DRM_CLOEXEC | DRM_RDWR, > + (int*)&whandle->handle)) > return false; > } > > diff --git a/src/gallium/winsys/svga/drm/vmw_screen_dri.c b/src/gallium/winsys/svga/drm/vmw_screen_dri.c > index a85ee18e45b9..24f6ddac0890 100644 > --- a/src/gallium/winsys/svga/drm/vmw_screen_dri.c > +++ b/src/gallium/winsys/svga/drm/vmw_screen_dri.c > @@ -345,8 +345,9 @@ vmw_drm_surface_get_handle(struct svga_winsys_screen *sws, > whandle->handle = vsrf->sid; > break; > case WINSYS_HANDLE_TYPE_FD: > - ret = drmPrimeHandleToFD(vws->ioctl.drm_fd, vsrf->sid, DRM_CLOEXEC, > - (int *)&whandle->handle); > + ret = drmPrimeHandleToFD(vws->ioctl.drm_fd, vsrf->sid, > + DRM_CLOEXEC | DRM_RDWR, > + (int *)&whandle->handle); > if (ret) { > vmw_error("Failed to get file descriptor from prime.\n"); > return FALSE; > diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c > index d9b417dc4dad..ce47e66ab1e5 100644 > --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c > +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c > @@ -446,7 +446,7 @@ kms_sw_displaytarget_get_handle(struct sw_winsys *winsys, > return TRUE; > case WINSYS_HANDLE_TYPE_FD: > if (!drmPrimeHandleToFD(kms_sw->fd, kms_sw_dt->handle, > - DRM_CLOEXEC, (int*)&whandle->handle)) { > + DRM_CLOEXEC | DRM_RDWR, (int*)&whandle->handle)) { > whandle->stride = plane->stride; > whandle->offset = plane->offset; > return TRUE; > diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c > index 9eec92f57363..07fab2735e25 100644 > --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c > +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c > @@ -418,7 +418,8 @@ static boolean virgl_drm_winsys_resource_get_handle(struct virgl_winsys *qws, > } else if (whandle->type == WINSYS_HANDLE_TYPE_KMS) { > whandle->handle = res->bo_handle; > } else if (whandle->type == WINSYS_HANDLE_TYPE_FD) { > - if (drmPrimeHandleToFD(qdws->fd, res->bo_handle, DRM_CLOEXEC, (int*)&whandle->handle)) > + if (drmPrimeHandleToFD(qdws->fd, res->bo_handle, DRM_CLOEXEC | DRM_RDWR, > + (int*)&whandle->handle)) > return FALSE; > mtx_lock(&qdws->bo_handles_mutex); > util_hash_table_set(qdws->bo_handles, (void *)(uintptr_t)res->bo_handle, res); > diff --git a/src/intel/vulkan/anv_gem.c b/src/intel/vulkan/anv_gem.c > index 1bdf040c1a37..8f07928638ce 100644 > --- a/src/intel/vulkan/anv_gem.c > +++ b/src/intel/vulkan/anv_gem.c > @@ -399,7 +399,7 @@ anv_gem_handle_to_fd(struct anv_device *device, uint32_t gem_handle) > { > struct drm_prime_handle args = { > .handle = gem_handle, > - .flags = DRM_CLOEXEC, > + .flags = DRM_CLOEXEC | DRM_RDWR, > }; > > int ret = anv_ioctl(device->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &args); > diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c > index a7c684063158..ee639d2cf23a 100644 > --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c > +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c > @@ -1489,7 +1489,7 @@ brw_bo_gem_export_to_prime(struct brw_bo *bo, int *prime_fd) > brw_bo_make_external(bo); > > if (drmPrimeHandleToFD(bufmgr->fd, bo->gem_handle, > - DRM_CLOEXEC, prime_fd) != 0) > + DRM_CLOEXEC | DRM_RDWR, prime_fd) != 0) > return -errno; > > bo->reusable = false; > -- > 2.21.0 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
