| Message ID | 20190821215030.31660-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] dma_resv: prime lockdep annotations | expand |
On Wed, Aug 21, 2019 at 11:50:29PM +0200, Daniel Vetter wrote: > We can't copy_*_user while holding reservations, that will (soon even > for nouveau) lead to deadlocks. And it breaks the cross-driver > contract around dma_resv. > > Fix this by adding a slowpath for when we need relocations, and by > pushing the writeback of the new presumed offsets to the very end. > > Aside from "it compiles" entirely untested unfortunately. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: nouveau@lists.freedesktop.org Ping for some review/testing (apparently needs pre-nv50). I'd really like to land this series here, it should help a lot in making sure everyone uses dma_resv in a compatible way across drivers. Thanks, Daniel > --- > drivers/gpu/drm/nouveau/nouveau_gem.c | 57 ++++++++++++++++++--------- > 1 file changed, 38 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c > index c77302f969e8..60309b997951 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c > @@ -482,12 +482,9 @@ validate_init(struct nouveau_channel *chan, struct drm_file *file_priv, > > static int > validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli, > - struct list_head *list, struct drm_nouveau_gem_pushbuf_bo *pbbo, > - uint64_t user_pbbo_ptr) > + struct list_head *list, struct drm_nouveau_gem_pushbuf_bo *pbbo) > { > struct nouveau_drm *drm = chan->drm; > - struct drm_nouveau_gem_pushbuf_bo __user *upbbo = > - (void __force __user *)(uintptr_t)user_pbbo_ptr; > struct nouveau_bo *nvbo; > int ret, relocs = 0; > > @@ -531,10 +528,6 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli, > b->presumed.offset = nvbo->bo.offset; > b->presumed.valid = 0; > relocs++; > - > - if (copy_to_user(&upbbo[nvbo->pbbo_index].presumed, > - &b->presumed, sizeof(b->presumed))) > - return -EFAULT; > } > } > > @@ -545,8 +538,8 @@ static int > nouveau_gem_pushbuf_validate(struct nouveau_channel *chan, > struct drm_file *file_priv, > struct drm_nouveau_gem_pushbuf_bo *pbbo, > - uint64_t user_buffers, int nr_buffers, > - struct validate_op *op, int *apply_relocs) > + int nr_buffers, > + struct validate_op *op, bool *apply_relocs) > { > struct nouveau_cli *cli = nouveau_cli(file_priv); > int ret; > @@ -563,7 +556,7 @@ nouveau_gem_pushbuf_validate(struct nouveau_channel *chan, > return ret; > } > > - ret = validate_list(chan, cli, &op->list, pbbo, user_buffers); > + ret = validate_list(chan, cli, &op->list, pbbo); > if (unlikely(ret < 0)) { > if (ret != -ERESTARTSYS) > NV_PRINTK(err, cli, "validating bo list\n"); > @@ -603,16 +596,12 @@ u_memcpya(uint64_t user, unsigned nmemb, unsigned size) > static int > nouveau_gem_pushbuf_reloc_apply(struct nouveau_cli *cli, > struct drm_nouveau_gem_pushbuf *req, > + struct drm_nouveau_gem_pushbuf_reloc *reloc, > struct drm_nouveau_gem_pushbuf_bo *bo) > { > - struct drm_nouveau_gem_pushbuf_reloc *reloc = NULL; > int ret = 0; > unsigned i; > > - reloc = u_memcpya(req->relocs, req->nr_relocs, sizeof(*reloc)); > - if (IS_ERR(reloc)) > - return PTR_ERR(reloc); > - > for (i = 0; i < req->nr_relocs; i++) { > struct drm_nouveau_gem_pushbuf_reloc *r = &reloc[i]; > struct drm_nouveau_gem_pushbuf_bo *b; > @@ -691,11 +680,13 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, > struct nouveau_drm *drm = nouveau_drm(dev); > struct drm_nouveau_gem_pushbuf *req = data; > struct drm_nouveau_gem_pushbuf_push *push; > + struct drm_nouveau_gem_pushbuf_reloc *reloc = NULL; > struct drm_nouveau_gem_pushbuf_bo *bo; > struct nouveau_channel *chan = NULL; > struct validate_op op; > struct nouveau_fence *fence = NULL; > - int i, j, ret = 0, do_reloc = 0; > + int i, j, ret = 0; > + bool do_reloc = false; > > if (unlikely(!abi16)) > return -ENOMEM; > @@ -753,7 +744,8 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, > } > > /* Validate buffer list */ > - ret = nouveau_gem_pushbuf_validate(chan, file_priv, bo, req->buffers, > +revalidate: > + ret = nouveau_gem_pushbuf_validate(chan, file_priv, bo, > req->nr_buffers, &op, &do_reloc); > if (ret) { > if (ret != -ERESTARTSYS) > @@ -763,7 +755,18 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, > > /* Apply any relocations that are required */ > if (do_reloc) { > - ret = nouveau_gem_pushbuf_reloc_apply(cli, req, bo); > + if (!reloc) { > + validate_fini(&op, chan, NULL, bo); > + reloc = u_memcpya(req->relocs, req->nr_relocs, sizeof(*reloc)); > + if (IS_ERR(reloc)) { > + ret = PTR_ERR(reloc); > + goto out_prevalid; > + } > + > + goto revalidate; > + } > + > + ret = nouveau_gem_pushbuf_reloc_apply(cli, req, reloc, bo); > if (ret) { > NV_PRINTK(err, cli, "reloc apply: %d\n", ret); > goto out; > @@ -849,6 +852,22 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, > validate_fini(&op, chan, fence, bo); > nouveau_fence_unref(&fence); > > + if (do_reloc) { > + struct drm_nouveau_gem_pushbuf_bo __user *upbbo = > + u64_to_user_ptr(req->buffers); > + > + for (i = 0; i < req->nr_buffers; i++) { > + if (bo[i].presumed.valid) > + continue; > + > + if (copy_to_user(&upbbo[i].presumed, &bo[i].presumed, > + sizeof(bo[i].presumed))) { > + ret = -EFAULT; > + break; > + } > + } > + u_free(reloc); > + } > out_prevalid: > u_free(bo); > u_free(push); > -- > 2.23.0.rc1 >
On Tue, Sep 03, 2019 at 10:17:14AM +0200, Daniel Vetter wrote: > On Wed, Aug 21, 2019 at 11:50:29PM +0200, Daniel Vetter wrote: > > We can't copy_*_user while holding reservations, that will (soon even > > for nouveau) lead to deadlocks. And it breaks the cross-driver > > contract around dma_resv. > > > > Fix this by adding a slowpath for when we need relocations, and by > > pushing the writeback of the new presumed offsets to the very end. > > > > Aside from "it compiles" entirely untested unfortunately. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Ben Skeggs <bskeggs@redhat.com> > > Cc: nouveau@lists.freedesktop.org > > Ping for some review/testing (apparently needs pre-nv50). I'd really like > to land this series here, it should help a lot in making sure everyone > uses dma_resv in a compatible way across drivers. Now that the gem/ttm fallout is fixed, ping for testing on this one here ... Also need some r-b to get this landed. Thanks, Daniel > > --- > > drivers/gpu/drm/nouveau/nouveau_gem.c | 57 ++++++++++++++++++--------- > > 1 file changed, 38 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c > > index c77302f969e8..60309b997951 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c > > @@ -482,12 +482,9 @@ validate_init(struct nouveau_channel *chan, struct drm_file *file_priv, > > > > static int > > validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli, > > - struct list_head *list, struct drm_nouveau_gem_pushbuf_bo *pbbo, > > - uint64_t user_pbbo_ptr) > > + struct list_head *list, struct drm_nouveau_gem_pushbuf_bo *pbbo) > > { > > struct nouveau_drm *drm = chan->drm; > > - struct drm_nouveau_gem_pushbuf_bo __user *upbbo = > > - (void __force __user *)(uintptr_t)user_pbbo_ptr; > > struct nouveau_bo *nvbo; > > int ret, relocs = 0; > > > > @@ -531,10 +528,6 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli, > > b->presumed.offset = nvbo->bo.offset; > > b->presumed.valid = 0; > > relocs++; > > - > > - if (copy_to_user(&upbbo[nvbo->pbbo_index].presumed, > > - &b->presumed, sizeof(b->presumed))) > > - return -EFAULT; > > } > > } > > > > @@ -545,8 +538,8 @@ static int > > nouveau_gem_pushbuf_validate(struct nouveau_channel *chan, > > struct drm_file *file_priv, > > struct drm_nouveau_gem_pushbuf_bo *pbbo, > > - uint64_t user_buffers, int nr_buffers, > > - struct validate_op *op, int *apply_relocs) > > + int nr_buffers, > > + struct validate_op *op, bool *apply_relocs) > > { > > struct nouveau_cli *cli = nouveau_cli(file_priv); > > int ret; > > @@ -563,7 +556,7 @@ nouveau_gem_pushbuf_validate(struct nouveau_channel *chan, > > return ret; > > } > > > > - ret = validate_list(chan, cli, &op->list, pbbo, user_buffers); > > + ret = validate_list(chan, cli, &op->list, pbbo); > > if (unlikely(ret < 0)) { > > if (ret != -ERESTARTSYS) > > NV_PRINTK(err, cli, "validating bo list\n"); > > @@ -603,16 +596,12 @@ u_memcpya(uint64_t user, unsigned nmemb, unsigned size) > > static int > > nouveau_gem_pushbuf_reloc_apply(struct nouveau_cli *cli, > > struct drm_nouveau_gem_pushbuf *req, > > + struct drm_nouveau_gem_pushbuf_reloc *reloc, > > struct drm_nouveau_gem_pushbuf_bo *bo) > > { > > - struct drm_nouveau_gem_pushbuf_reloc *reloc = NULL; > > int ret = 0; > > unsigned i; > > > > - reloc = u_memcpya(req->relocs, req->nr_relocs, sizeof(*reloc)); > > - if (IS_ERR(reloc)) > > - return PTR_ERR(reloc); > > - > > for (i = 0; i < req->nr_relocs; i++) { > > struct drm_nouveau_gem_pushbuf_reloc *r = &reloc[i]; > > struct drm_nouveau_gem_pushbuf_bo *b; > > @@ -691,11 +680,13 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, > > struct nouveau_drm *drm = nouveau_drm(dev); > > struct drm_nouveau_gem_pushbuf *req = data; > > struct drm_nouveau_gem_pushbuf_push *push; > > + struct drm_nouveau_gem_pushbuf_reloc *reloc = NULL; > > struct drm_nouveau_gem_pushbuf_bo *bo; > > struct nouveau_channel *chan = NULL; > > struct validate_op op; > > struct nouveau_fence *fence = NULL; > > - int i, j, ret = 0, do_reloc = 0; > > + int i, j, ret = 0; > > + bool do_reloc = false; > > > > if (unlikely(!abi16)) > > return -ENOMEM; > > @@ -753,7 +744,8 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, > > } > > > > /* Validate buffer list */ > > - ret = nouveau_gem_pushbuf_validate(chan, file_priv, bo, req->buffers, > > +revalidate: > > + ret = nouveau_gem_pushbuf_validate(chan, file_priv, bo, > > req->nr_buffers, &op, &do_reloc); > > if (ret) { > > if (ret != -ERESTARTSYS) > > @@ -763,7 +755,18 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, > > > > /* Apply any relocations that are required */ > > if (do_reloc) { > > - ret = nouveau_gem_pushbuf_reloc_apply(cli, req, bo); > > + if (!reloc) { > > + validate_fini(&op, chan, NULL, bo); > > + reloc = u_memcpya(req->relocs, req->nr_relocs, sizeof(*reloc)); > > + if (IS_ERR(reloc)) { > > + ret = PTR_ERR(reloc); > > + goto out_prevalid; > > + } > > + > > + goto revalidate; > > + } > > + > > + ret = nouveau_gem_pushbuf_reloc_apply(cli, req, reloc, bo); > > if (ret) { > > NV_PRINTK(err, cli, "reloc apply: %d\n", ret); > > goto out; > > @@ -849,6 +852,22 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, > > validate_fini(&op, chan, fence, bo); > > nouveau_fence_unref(&fence); > > > > + if (do_reloc) { > > + struct drm_nouveau_gem_pushbuf_bo __user *upbbo = > > + u64_to_user_ptr(req->buffers); > > + > > + for (i = 0; i < req->nr_buffers; i++) { > > + if (bo[i].presumed.valid) > > + continue; > > + > > + if (copy_to_user(&upbbo[i].presumed, &bo[i].presumed, > > + sizeof(bo[i].presumed))) { > > + ret = -EFAULT; > > + break; > > + } > > + } > > + u_free(reloc); > > + } > > out_prevalid: > > u_free(bo); > > u_free(push); > > -- > > 2.23.0.rc1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index c77302f969e8..60309b997951 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -482,12 +482,9 @@ validate_init(struct nouveau_channel *chan, struct drm_file *file_priv, static int validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli, - struct list_head *list, struct drm_nouveau_gem_pushbuf_bo *pbbo, - uint64_t user_pbbo_ptr) + struct list_head *list, struct drm_nouveau_gem_pushbuf_bo *pbbo) { struct nouveau_drm *drm = chan->drm; - struct drm_nouveau_gem_pushbuf_bo __user *upbbo = - (void __force __user *)(uintptr_t)user_pbbo_ptr; struct nouveau_bo *nvbo; int ret, relocs = 0; @@ -531,10 +528,6 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli, b->presumed.offset = nvbo->bo.offset; b->presumed.valid = 0; relocs++; - - if (copy_to_user(&upbbo[nvbo->pbbo_index].presumed, - &b->presumed, sizeof(b->presumed))) - return -EFAULT; } } @@ -545,8 +538,8 @@ static int nouveau_gem_pushbuf_validate(struct nouveau_channel *chan, struct drm_file *file_priv, struct drm_nouveau_gem_pushbuf_bo *pbbo, - uint64_t user_buffers, int nr_buffers, - struct validate_op *op, int *apply_relocs) + int nr_buffers, + struct validate_op *op, bool *apply_relocs) { struct nouveau_cli *cli = nouveau_cli(file_priv); int ret; @@ -563,7 +556,7 @@ nouveau_gem_pushbuf_validate(struct nouveau_channel *chan, return ret; } - ret = validate_list(chan, cli, &op->list, pbbo, user_buffers); + ret = validate_list(chan, cli, &op->list, pbbo); if (unlikely(ret < 0)) { if (ret != -ERESTARTSYS) NV_PRINTK(err, cli, "validating bo list\n"); @@ -603,16 +596,12 @@ u_memcpya(uint64_t user, unsigned nmemb, unsigned size) static int nouveau_gem_pushbuf_reloc_apply(struct nouveau_cli *cli, struct drm_nouveau_gem_pushbuf *req, + struct drm_nouveau_gem_pushbuf_reloc *reloc, struct drm_nouveau_gem_pushbuf_bo *bo) { - struct drm_nouveau_gem_pushbuf_reloc *reloc = NULL; int ret = 0; unsigned i; - reloc = u_memcpya(req->relocs, req->nr_relocs, sizeof(*reloc)); - if (IS_ERR(reloc)) - return PTR_ERR(reloc); - for (i = 0; i < req->nr_relocs; i++) { struct drm_nouveau_gem_pushbuf_reloc *r = &reloc[i]; struct drm_nouveau_gem_pushbuf_bo *b; @@ -691,11 +680,13 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, struct nouveau_drm *drm = nouveau_drm(dev); struct drm_nouveau_gem_pushbuf *req = data; struct drm_nouveau_gem_pushbuf_push *push; + struct drm_nouveau_gem_pushbuf_reloc *reloc = NULL; struct drm_nouveau_gem_pushbuf_bo *bo; struct nouveau_channel *chan = NULL; struct validate_op op; struct nouveau_fence *fence = NULL; - int i, j, ret = 0, do_reloc = 0; + int i, j, ret = 0; + bool do_reloc = false; if (unlikely(!abi16)) return -ENOMEM; @@ -753,7 +744,8 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, } /* Validate buffer list */ - ret = nouveau_gem_pushbuf_validate(chan, file_priv, bo, req->buffers, +revalidate: + ret = nouveau_gem_pushbuf_validate(chan, file_priv, bo, req->nr_buffers, &op, &do_reloc); if (ret) { if (ret != -ERESTARTSYS) @@ -763,7 +755,18 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, /* Apply any relocations that are required */ if (do_reloc) { - ret = nouveau_gem_pushbuf_reloc_apply(cli, req, bo); + if (!reloc) { + validate_fini(&op, chan, NULL, bo); + reloc = u_memcpya(req->relocs, req->nr_relocs, sizeof(*reloc)); + if (IS_ERR(reloc)) { + ret = PTR_ERR(reloc); + goto out_prevalid; + } + + goto revalidate; + } + + ret = nouveau_gem_pushbuf_reloc_apply(cli, req, reloc, bo); if (ret) { NV_PRINTK(err, cli, "reloc apply: %d\n", ret); goto out; @@ -849,6 +852,22 @@ nouveau_gem_ioctl_pushbuf(struct drm_device *dev, void *data, validate_fini(&op, chan, fence, bo); nouveau_fence_unref(&fence); + if (do_reloc) { + struct drm_nouveau_gem_pushbuf_bo __user *upbbo = + u64_to_user_ptr(req->buffers); + + for (i = 0; i < req->nr_buffers; i++) { + if (bo[i].presumed.valid) + continue; + + if (copy_to_user(&upbbo[i].presumed, &bo[i].presumed, + sizeof(bo[i].presumed))) { + ret = -EFAULT; + break; + } + } + u_free(reloc); + } out_prevalid: u_free(bo); u_free(push);
We can't copy_*_user while holding reservations, that will (soon even for nouveau) lead to deadlocks. And it breaks the cross-driver contract around dma_resv. Fix this by adding a slowpath for when we need relocations, and by pushing the writeback of the new presumed offsets to the very end. Aside from "it compiles" entirely untested unfortunately. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Ben Skeggs <bskeggs@redhat.com> Cc: nouveau@lists.freedesktop.org --- drivers/gpu/drm/nouveau/nouveau_gem.c | 57 ++++++++++++++++++--------- 1 file changed, 38 insertions(+), 19 deletions(-)