On Wed, Jul 03, 2019 at 05:47:24PM +0200, Daniel Vetter wrote: > On Wed, Jul 03, 2019 at 07:45:32AM -0600, Rob Herring wrote: > > 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. > > For self-import we short-circuit out the underlying dma-buf and directly > go to the gem_bo again. > > > > 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. > > Can't we change the helper to force the rw mode to make this work? > > Aside: The reason I'm not a huge fan of forwarding gem mmap to dma-buf > mmap is that generally the begin/end_cpu_access stuff gets lots on the way > there. Which mostly doesn't matter since everyone prefers to share > coherent buffers, except when it totally does matter. So by forwarding > mmap and pretending it all keeps working as-is we're digging ourselves a > bit into a hole I think :-/ > > Since the same discussion is happening with etnaviv: Why exactly does mesa > need to mmap imported buffers? Also, for dumb mmap I very much want to keep this requirement. If you import a dma-buf and then mmap it through the dumb buffer interfaces, you're just doing all the things wrong :-) -Daniel > -Daniel > > > > > > 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
