Re: [PATCH 2/3] drm/nouveau: slowpath for pushbuf ioctl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 21, 2019 at 4:50 PM Daniel Vetter <daniel.vetter@xxxxxxxx> 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: Ilia Mirkin <imirkin@xxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx>
> Cc: nouveau@xxxxxxxxxxxxxxxxxxxxx

Ping for review/testing/ack, I'd really like to get this in.
-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 1324c19f4e5c..05ec8edd6a8b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -484,12 +484,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;
>
> @@ -533,10 +530,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;
>                 }
>         }
>
> @@ -547,8 +540,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;
> @@ -565,7 +558,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");
> @@ -605,16 +598,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;
> @@ -693,11 +682,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;
> @@ -755,7 +746,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)
> @@ -765,7 +757,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;
> @@ -851,6 +854,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
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux