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;@@ -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);@@ -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