thr3ads.net - Nouveau - [Nouveau] [PATCH 2/3] drm/nouveau: slowpath for pushbuf ioctl [Sep 2019]

If this information is useful, please help other people find it:
Share via:

Daniel Vetter

2019-Aug-21 21:50 UTC

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

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 at intel.com>
Cc: Ben Skeggs <bskeggs at redhat.com>
Cc: nouveau at lists.freedesktop.org
---
 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

2019-Sep-03 08:17 UTC

head link

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

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 at intel.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: nouveau at 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
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Daniel Vetter

2019-Sep-18 09:29 UTC

head link

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

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 at intel.com>
> > Cc: Ben Skeggs <bskeggs at redhat.com>
> > Cc: nouveau at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Apparently Analagous Threads

Search for more possibly parallel threads

Nouveau - Sep 2019 - [PATCH 2/3] drm/nouveau: slowpath for pushbuf ioctl

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

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

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

Apparently Analagous Threads