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@xxxxxxxxx>
> > Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> > Cc: nouveau@xxxxxxxxxxxxxxxxxxxxx
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
