* [Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 13:03 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index 347488685f74..591738545eba 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) if (ret) return -EINVAL; - return 0; + if (nvbo->bo.moving) { + ret = dma_fence_wait(nvbo->bo.moving, true); + if (ret) + nouveau_bo_unpin(nvbo); + } + + return ret; } void nouveau_gem_prime_unpin(struct drm_gem_object *obj) -- 2.25.1 _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 13:03 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index 347488685f74..591738545eba 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) if (ret) return -EINVAL; - return 0; + if (nvbo->bo.moving) { + ret = dma_fence_wait(nvbo->bo.moving, true); + if (ret) + nouveau_bo_unpin(nvbo); + } + + return ret; } void nouveau_gem_prime_unpin(struct drm_gem_object *obj) -- 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 13:03 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index 347488685f74..591738545eba 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) if (ret) return -EINVAL; - return 0; + if (nvbo->bo.moving) { + ret = dma_fence_wait(nvbo->bo.moving, true); + if (ret) + nouveau_bo_unpin(nvbo); + } + + return ret; } void nouveau_gem_prime_unpin(struct drm_gem_object *obj) -- 2.25.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Nouveau] [PATCH 2/3] drm/radeon: wait for moving fence after pinning 2021-06-21 13:03 ` Christian König (?) @ 2021-06-21 13:03 ` Christian König -1 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/radeon/radeon_prime.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 42a87948e28c..4a90807351e7 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -77,9 +77,19 @@ int radeon_gem_prime_pin(struct drm_gem_object *obj) /* pin buffer into GTT */ ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL); - if (likely(ret == 0)) - bo->prime_shared_count++; - + if (unlikely(ret)) + goto error; + + if (bo->tbo.moving) { + ret = dma_fence_wait(bo->tbo.moving, false); + if (unlikely(ret)) { + radeon_bo_unpin(bo); + goto error; + } + } + + bo->prime_shared_count++; +error: radeon_bo_unreserve(bo); return ret; } -- 2.25.1 _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/3] drm/radeon: wait for moving fence after pinning @ 2021-06-21 13:03 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/radeon/radeon_prime.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 42a87948e28c..4a90807351e7 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -77,9 +77,19 @@ int radeon_gem_prime_pin(struct drm_gem_object *obj) /* pin buffer into GTT */ ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL); - if (likely(ret == 0)) - bo->prime_shared_count++; - + if (unlikely(ret)) + goto error; + + if (bo->tbo.moving) { + ret = dma_fence_wait(bo->tbo.moving, false); + if (unlikely(ret)) { + radeon_bo_unpin(bo); + goto error; + } + } + + bo->prime_shared_count++; +error: radeon_bo_unreserve(bo); return ret; } -- 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/3] drm/radeon: wait for moving fence after pinning @ 2021-06-21 13:03 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/radeon/radeon_prime.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 42a87948e28c..4a90807351e7 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -77,9 +77,19 @@ int radeon_gem_prime_pin(struct drm_gem_object *obj) /* pin buffer into GTT */ ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL); - if (likely(ret == 0)) - bo->prime_shared_count++; - + if (unlikely(ret)) + goto error; + + if (bo->tbo.moving) { + ret = dma_fence_wait(bo->tbo.moving, false); + if (unlikely(ret)) { + radeon_bo_unpin(bo); + goto error; + } + } + + bo->prime_shared_count++; +error: radeon_bo_unreserve(bo); return ret; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Nouveau] [PATCH 2/3] drm/radeon: wait for moving fence after pinning 2021-06-21 13:03 ` Christian König (?) @ 2021-06-21 14:55 ` Daniel Vetter -1 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:55 UTC (permalink / raw) To: Christian König; +Cc: amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:27PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/radeon/radeon_prime.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c > index 42a87948e28c..4a90807351e7 100644 > --- a/drivers/gpu/drm/radeon/radeon_prime.c > +++ b/drivers/gpu/drm/radeon/radeon_prime.c > @@ -77,9 +77,19 @@ int radeon_gem_prime_pin(struct drm_gem_object *obj) > > /* pin buffer into GTT */ > ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL); > - if (likely(ret == 0)) > - bo->prime_shared_count++; > - > + if (unlikely(ret)) > + goto error; > + > + if (bo->tbo.moving) { > + ret = dma_fence_wait(bo->tbo.moving, false); Here we wait whil holding the reservation, so we should be all fine. Maybe not the nicest to wait while locked, but also I don't think it'll matter. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + if (unlikely(ret)) { > + radeon_bo_unpin(bo); > + goto error; > + } > + } > + > + bo->prime_shared_count++; > +error: > radeon_bo_unreserve(bo); > return ret; > } > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] drm/radeon: wait for moving fence after pinning @ 2021-06-21 14:55 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:55 UTC (permalink / raw) To: Christian König; +Cc: daniel.vetter, amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:27PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/radeon/radeon_prime.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c > index 42a87948e28c..4a90807351e7 100644 > --- a/drivers/gpu/drm/radeon/radeon_prime.c > +++ b/drivers/gpu/drm/radeon/radeon_prime.c > @@ -77,9 +77,19 @@ int radeon_gem_prime_pin(struct drm_gem_object *obj) > > /* pin buffer into GTT */ > ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL); > - if (likely(ret == 0)) > - bo->prime_shared_count++; > - > + if (unlikely(ret)) > + goto error; > + > + if (bo->tbo.moving) { > + ret = dma_fence_wait(bo->tbo.moving, false); Here we wait whil holding the reservation, so we should be all fine. Maybe not the nicest to wait while locked, but also I don't think it'll matter. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + if (unlikely(ret)) { > + radeon_bo_unpin(bo); > + goto error; > + } > + } > + > + bo->prime_shared_count++; > +error: > radeon_bo_unreserve(bo); > return ret; > } > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/3] drm/radeon: wait for moving fence after pinning @ 2021-06-21 14:55 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:55 UTC (permalink / raw) To: Christian König; +Cc: daniel.vetter, amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:27PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/radeon/radeon_prime.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c > index 42a87948e28c..4a90807351e7 100644 > --- a/drivers/gpu/drm/radeon/radeon_prime.c > +++ b/drivers/gpu/drm/radeon/radeon_prime.c > @@ -77,9 +77,19 @@ int radeon_gem_prime_pin(struct drm_gem_object *obj) > > /* pin buffer into GTT */ > ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL); > - if (likely(ret == 0)) > - bo->prime_shared_count++; > - > + if (unlikely(ret)) > + goto error; > + > + if (bo->tbo.moving) { > + ret = dma_fence_wait(bo->tbo.moving, false); Here we wait whil holding the reservation, so we should be all fine. Maybe not the nicest to wait while locked, but also I don't think it'll matter. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + if (unlikely(ret)) { > + radeon_bo_unpin(bo); > + goto error; > + } > + } > + > + bo->prime_shared_count++; > +error: > radeon_bo_unreserve(bo); > return ret; > } > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* [Nouveau] [PATCH 3/3] drm/amdgpu: wait for moving fence after pinning 2021-06-21 13:03 ` Christian König (?) @ 2021-06-21 13:03 ` Christian König -1 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index baa980a477d9..37ec59365080 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -214,9 +214,21 @@ static int amdgpu_dma_buf_pin(struct dma_buf_attachment *attach) { struct drm_gem_object *obj = attach->dmabuf->priv; struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); + int r; /* pin buffer into GTT */ - return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); + r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); + if (r) + return r; + + if (bo->tbo.moving) { + r = dma_fence_wait(bo->tbo.moving, true); + if (r) { + amdgpu_bo_unpin(bo); + return r; + } + } + return 0; } /** -- 2.25.1 _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/3] drm/amdgpu: wait for moving fence after pinning @ 2021-06-21 13:03 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index baa980a477d9..37ec59365080 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -214,9 +214,21 @@ static int amdgpu_dma_buf_pin(struct dma_buf_attachment *attach) { struct drm_gem_object *obj = attach->dmabuf->priv; struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); + int r; /* pin buffer into GTT */ - return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); + r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); + if (r) + return r; + + if (bo->tbo.moving) { + r = dma_fence_wait(bo->tbo.moving, true); + if (r) { + amdgpu_bo_unpin(bo); + return r; + } + } + return 0; } /** -- 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/3] drm/amdgpu: wait for moving fence after pinning @ 2021-06-21 13:03 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 13:03 UTC (permalink / raw) To: dri-devel, daniel.vetter, nouveau, amd-gfx We actually need to wait for the moving fence after pinning the BO to make sure that the pin is completed. Signed-off-by: Christian König <christian.koenig@amd.com> CC: stable@kernel.org --- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index baa980a477d9..37ec59365080 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -214,9 +214,21 @@ static int amdgpu_dma_buf_pin(struct dma_buf_attachment *attach) { struct drm_gem_object *obj = attach->dmabuf->priv; struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); + int r; /* pin buffer into GTT */ - return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); + r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); + if (r) + return r; + + if (bo->tbo.moving) { + r = dma_fence_wait(bo->tbo.moving, true); + if (r) { + amdgpu_bo_unpin(bo); + return r; + } + } + return 0; } /** -- 2.25.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Nouveau] [PATCH 3/3] drm/amdgpu: wait for moving fence after pinning 2021-06-21 13:03 ` Christian König (?) @ 2021-06-21 14:56 ` Daniel Vetter -1 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:56 UTC (permalink / raw) To: Christian König; +Cc: amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:28PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > index baa980a477d9..37ec59365080 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > @@ -214,9 +214,21 @@ static int amdgpu_dma_buf_pin(struct dma_buf_attachment *attach) > { > struct drm_gem_object *obj = attach->dmabuf->priv; > struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > + int r; > > /* pin buffer into GTT */ > - return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); > + r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); > + if (r) > + return r; > + > + if (bo->tbo.moving) { dma-buf.c guarantees we have the reservation here, so we're fine. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + r = dma_fence_wait(bo->tbo.moving, true); > + if (r) { > + amdgpu_bo_unpin(bo); > + return r; > + } > + } > + return 0; > } > > /** > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: wait for moving fence after pinning @ 2021-06-21 14:56 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:56 UTC (permalink / raw) To: Christian König; +Cc: daniel.vetter, amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:28PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > index baa980a477d9..37ec59365080 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > @@ -214,9 +214,21 @@ static int amdgpu_dma_buf_pin(struct dma_buf_attachment *attach) > { > struct drm_gem_object *obj = attach->dmabuf->priv; > struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > + int r; > > /* pin buffer into GTT */ > - return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); > + r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); > + if (r) > + return r; > + > + if (bo->tbo.moving) { dma-buf.c guarantees we have the reservation here, so we're fine. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + r = dma_fence_wait(bo->tbo.moving, true); > + if (r) { > + amdgpu_bo_unpin(bo); > + return r; > + } > + } > + return 0; > } > > /** > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/3] drm/amdgpu: wait for moving fence after pinning @ 2021-06-21 14:56 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:56 UTC (permalink / raw) To: Christian König; +Cc: daniel.vetter, amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:28PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > index baa980a477d9..37ec59365080 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > @@ -214,9 +214,21 @@ static int amdgpu_dma_buf_pin(struct dma_buf_attachment *attach) > { > struct drm_gem_object *obj = attach->dmabuf->priv; > struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > + int r; > > /* pin buffer into GTT */ > - return amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); > + r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT); > + if (r) > + return r; > + > + if (bo->tbo.moving) { dma-buf.c guarantees we have the reservation here, so we're fine. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + r = dma_fence_wait(bo->tbo.moving, true); > + if (r) { > + amdgpu_bo_unpin(bo); > + return r; > + } > + } > + return 0; > } > > /** > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning 2021-06-21 13:03 ` Christian König (?) @ 2021-06-21 14:54 ` Daniel Vetter -1 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:54 UTC (permalink / raw) To: Christian König; +Cc: amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > index 347488685f74..591738545eba 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > if (ret) > return -EINVAL; > > - return 0; > + if (nvbo->bo.moving) { Don't we need to hold the dma_resv to read this? We can grab a reference and then unlock, but I think just unlocked wait can go boom pretty easily (since we don't hold a reference or lock so someone else can jump in and free the moving fence). -Daniel > + ret = dma_fence_wait(nvbo->bo.moving, true); > + if (ret) > + nouveau_bo_unpin(nvbo); > + } > + > + return ret; > } > > void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 14:54 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:54 UTC (permalink / raw) To: Christian König; +Cc: daniel.vetter, amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > index 347488685f74..591738545eba 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > if (ret) > return -EINVAL; > > - return 0; > + if (nvbo->bo.moving) { Don't we need to hold the dma_resv to read this? We can grab a reference and then unlock, but I think just unlocked wait can go boom pretty easily (since we don't hold a reference or lock so someone else can jump in and free the moving fence). -Daniel > + ret = dma_fence_wait(nvbo->bo.moving, true); > + if (ret) > + nouveau_bo_unpin(nvbo); > + } > + > + return ret; > } > > void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 14:54 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 14:54 UTC (permalink / raw) To: Christian König; +Cc: daniel.vetter, amd-gfx, dri-devel, nouveau On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > We actually need to wait for the moving fence after pinning > the BO to make sure that the pin is completed. > > Signed-off-by: Christian König <christian.koenig@amd.com> > CC: stable@kernel.org > --- > drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > index 347488685f74..591738545eba 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > if (ret) > return -EINVAL; > > - return 0; > + if (nvbo->bo.moving) { Don't we need to hold the dma_resv to read this? We can grab a reference and then unlock, but I think just unlocked wait can go boom pretty easily (since we don't hold a reference or lock so someone else can jump in and free the moving fence). -Daniel > + ret = dma_fence_wait(nvbo->bo.moving, true); > + if (ret) > + nouveau_bo_unpin(nvbo); > + } > + > + return ret; > } > > void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning 2021-06-21 14:54 ` Daniel Vetter (?) @ 2021-06-21 15:49 ` Christian König -1 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 15:49 UTC (permalink / raw) To: Daniel Vetter; +Cc: amd-gfx, dri-devel, nouveau Am 21.06.21 um 16:54 schrieb Daniel Vetter: > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: >> We actually need to wait for the moving fence after pinning >> the BO to make sure that the pin is completed. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> CC: stable@kernel.org >> --- >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c >> index 347488685f74..591738545eba 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) >> if (ret) >> return -EINVAL; >> >> - return 0; >> + if (nvbo->bo.moving) { > Don't we need to hold the dma_resv to read this? We can grab a reference > and then unlock, but I think just unlocked wait can go boom pretty easily > (since we don't hold a reference or lock so someone else can jump in and > free the moving fence). The moving fence is only modified while the BO is moved and since we have just successfully pinned it.... But in general I agree that it would be better to avoid this. I just didn't wanted to open a bigger can of worms by changing nouveau so much. Christian. > -Daniel > >> + ret = dma_fence_wait(nvbo->bo.moving, true); >> + if (ret) >> + nouveau_bo_unpin(nvbo); >> + } >> + >> + return ret; >> } >> >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) >> -- >> 2.25.1 >> _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 15:49 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 15:49 UTC (permalink / raw) To: Daniel Vetter; +Cc: daniel.vetter, amd-gfx, dri-devel, nouveau Am 21.06.21 um 16:54 schrieb Daniel Vetter: > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: >> We actually need to wait for the moving fence after pinning >> the BO to make sure that the pin is completed. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> CC: stable@kernel.org >> --- >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c >> index 347488685f74..591738545eba 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) >> if (ret) >> return -EINVAL; >> >> - return 0; >> + if (nvbo->bo.moving) { > Don't we need to hold the dma_resv to read this? We can grab a reference > and then unlock, but I think just unlocked wait can go boom pretty easily > (since we don't hold a reference or lock so someone else can jump in and > free the moving fence). The moving fence is only modified while the BO is moved and since we have just successfully pinned it.... But in general I agree that it would be better to avoid this. I just didn't wanted to open a bigger can of worms by changing nouveau so much. Christian. > -Daniel > >> + ret = dma_fence_wait(nvbo->bo.moving, true); >> + if (ret) >> + nouveau_bo_unpin(nvbo); >> + } >> + >> + return ret; >> } >> >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) >> -- >> 2.25.1 >> _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 15:49 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-21 15:49 UTC (permalink / raw) To: Daniel Vetter; +Cc: daniel.vetter, amd-gfx, dri-devel, nouveau Am 21.06.21 um 16:54 schrieb Daniel Vetter: > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: >> We actually need to wait for the moving fence after pinning >> the BO to make sure that the pin is completed. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> CC: stable@kernel.org >> --- >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c >> index 347488685f74..591738545eba 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) >> if (ret) >> return -EINVAL; >> >> - return 0; >> + if (nvbo->bo.moving) { > Don't we need to hold the dma_resv to read this? We can grab a reference > and then unlock, but I think just unlocked wait can go boom pretty easily > (since we don't hold a reference or lock so someone else can jump in and > free the moving fence). The moving fence is only modified while the BO is moved and since we have just successfully pinned it.... But in general I agree that it would be better to avoid this. I just didn't wanted to open a bigger can of worms by changing nouveau so much. Christian. > -Daniel > >> + ret = dma_fence_wait(nvbo->bo.moving, true); >> + if (ret) >> + nouveau_bo_unpin(nvbo); >> + } >> + >> + return ret; >> } >> >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) >> -- >> 2.25.1 >> ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning 2021-06-21 15:49 ` Christian König (?) @ 2021-06-21 15:53 ` Daniel Vetter -1 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 15:53 UTC (permalink / raw) To: Christian König; +Cc: Nouveau Dev, amd-gfx list, dri-devel On Mon, Jun 21, 2021 at 5:49 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Am 21.06.21 um 16:54 schrieb Daniel Vetter: > > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > >> We actually need to wait for the moving fence after pinning > >> the BO to make sure that the pin is completed. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> CC: stable@kernel.org > >> --- > >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > >> index 347488685f74..591738545eba 100644 > >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > >> if (ret) > >> return -EINVAL; > >> > >> - return 0; > >> + if (nvbo->bo.moving) { > > Don't we need to hold the dma_resv to read this? We can grab a reference > > and then unlock, but I think just unlocked wait can go boom pretty easily > > (since we don't hold a reference or lock so someone else can jump in and > > free the moving fence). > > The moving fence is only modified while the BO is moved and since we > have just successfully pinned it.... Yeah ... so probably correct, but really tricky. Just wrapping a ttm_bo_reserve/unreserve around the code you add should be enough and get the job done? > But in general I agree that it would be better to avoid this. I just > didn't wanted to open a bigger can of worms by changing nouveau so much. Yeah, but I'm kinda thinking of some helpers to wait for the move fence (so that later on we can switch from having the exclusive fence to the move fence do that, maybe). And then locking checks in there would be nice. Also avoids the case of explaining why lockless here is fine, but lockless wait for the exclusive fence in e.g. a dynami dma-buf importer is very much not fine at all. Just all around less trouble. -Daniel > > Christian. > > > -Daniel > > > >> + ret = dma_fence_wait(nvbo->bo.moving, true); > >> + if (ret) > >> + nouveau_bo_unpin(nvbo); > >> + } > >> + > >> + return ret; > >> } > >> > >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > >> -- > >> 2.25.1 > >> > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 15:53 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 15:53 UTC (permalink / raw) To: Christian König; +Cc: Nouveau Dev, amd-gfx list, dri-devel On Mon, Jun 21, 2021 at 5:49 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Am 21.06.21 um 16:54 schrieb Daniel Vetter: > > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > >> We actually need to wait for the moving fence after pinning > >> the BO to make sure that the pin is completed. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> CC: stable@kernel.org > >> --- > >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > >> index 347488685f74..591738545eba 100644 > >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > >> if (ret) > >> return -EINVAL; > >> > >> - return 0; > >> + if (nvbo->bo.moving) { > > Don't we need to hold the dma_resv to read this? We can grab a reference > > and then unlock, but I think just unlocked wait can go boom pretty easily > > (since we don't hold a reference or lock so someone else can jump in and > > free the moving fence). > > The moving fence is only modified while the BO is moved and since we > have just successfully pinned it.... Yeah ... so probably correct, but really tricky. Just wrapping a ttm_bo_reserve/unreserve around the code you add should be enough and get the job done? > But in general I agree that it would be better to avoid this. I just > didn't wanted to open a bigger can of worms by changing nouveau so much. Yeah, but I'm kinda thinking of some helpers to wait for the move fence (so that later on we can switch from having the exclusive fence to the move fence do that, maybe). And then locking checks in there would be nice. Also avoids the case of explaining why lockless here is fine, but lockless wait for the exclusive fence in e.g. a dynami dma-buf importer is very much not fine at all. Just all around less trouble. -Daniel > > Christian. > > > -Daniel > > > >> + ret = dma_fence_wait(nvbo->bo.moving, true); > >> + if (ret) > >> + nouveau_bo_unpin(nvbo); > >> + } > >> + > >> + return ret; > >> } > >> > >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > >> -- > >> 2.25.1 > >> > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-21 15:53 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-21 15:53 UTC (permalink / raw) To: Christian König; +Cc: Nouveau Dev, amd-gfx list, dri-devel On Mon, Jun 21, 2021 at 5:49 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Am 21.06.21 um 16:54 schrieb Daniel Vetter: > > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > >> We actually need to wait for the moving fence after pinning > >> the BO to make sure that the pin is completed. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> CC: stable@kernel.org > >> --- > >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > >> index 347488685f74..591738545eba 100644 > >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > >> if (ret) > >> return -EINVAL; > >> > >> - return 0; > >> + if (nvbo->bo.moving) { > > Don't we need to hold the dma_resv to read this? We can grab a reference > > and then unlock, but I think just unlocked wait can go boom pretty easily > > (since we don't hold a reference or lock so someone else can jump in and > > free the moving fence). > > The moving fence is only modified while the BO is moved and since we > have just successfully pinned it.... Yeah ... so probably correct, but really tricky. Just wrapping a ttm_bo_reserve/unreserve around the code you add should be enough and get the job done? > But in general I agree that it would be better to avoid this. I just > didn't wanted to open a bigger can of worms by changing nouveau so much. Yeah, but I'm kinda thinking of some helpers to wait for the move fence (so that later on we can switch from having the exclusive fence to the move fence do that, maybe). And then locking checks in there would be nice. Also avoids the case of explaining why lockless here is fine, but lockless wait for the exclusive fence in e.g. a dynami dma-buf importer is very much not fine at all. Just all around less trouble. -Daniel > > Christian. > > > -Daniel > > > >> + ret = dma_fence_wait(nvbo->bo.moving, true); > >> + if (ret) > >> + nouveau_bo_unpin(nvbo); > >> + } > >> + > >> + return ret; > >> } > >> > >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > >> -- > >> 2.25.1 > >> > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning 2021-06-21 15:53 ` Daniel Vetter (?) @ 2021-06-22 9:20 ` Daniel Vetter -1 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-22 9:20 UTC (permalink / raw) To: Christian König; +Cc: Nouveau Dev, amd-gfx list, dri-devel On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Jun 21, 2021 at 5:49 PM Christian König > <ckoenig.leichtzumerken@gmail.com> wrote: > > > > Am 21.06.21 um 16:54 schrieb Daniel Vetter: > > > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > > >> We actually need to wait for the moving fence after pinning > > >> the BO to make sure that the pin is completed. > > >> > > >> Signed-off-by: Christian König <christian.koenig@amd.com> > > >> CC: stable@kernel.org > > >> --- > > >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> index 347488685f74..591738545eba 100644 > > >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > > >> if (ret) > > >> return -EINVAL; > > >> > > >> - return 0; > > >> + if (nvbo->bo.moving) { > > > Don't we need to hold the dma_resv to read this? We can grab a reference > > > and then unlock, but I think just unlocked wait can go boom pretty easily > > > (since we don't hold a reference or lock so someone else can jump in and > > > free the moving fence). > > > > The moving fence is only modified while the BO is moved and since we > > have just successfully pinned it.... > > Yeah ... so probably correct, but really tricky. Just wrapping a > ttm_bo_reserve/unreserve around the code you add should be enough and > get the job done? I think you distracted me a bit with the "it can't move", so yes there's a guarantee that no other fence can show up in ttm_bo->moving and confuse us. But it could get set to NULL because someone realized it signalled. We're not doing that systematically, but relying on fences never getting garbage-collected for correctness isn't great. Sot the ttm_bo_reserve/unreserve is definitely needed here around this bit of code. You don't need to merge it with the reserve/unreserve in the pin function though, it's just to protect against the use-after-free. -Daniel > > > But in general I agree that it would be better to avoid this. I just > > didn't wanted to open a bigger can of worms by changing nouveau so much. > > Yeah, but I'm kinda thinking of some helpers to wait for the move > fence (so that later on we can switch from having the exclusive fence > to the move fence do that, maybe). And then locking checks in there > would be nice. > > Also avoids the case of explaining why lockless here is fine, but > lockless wait for the exclusive fence in e.g. a dynami dma-buf > importer is very much not fine at all. Just all around less trouble. > -Daniel > > > > > Christian. > > > > > -Daniel > > > > > >> + ret = dma_fence_wait(nvbo->bo.moving, true); > > >> + if (ret) > > >> + nouveau_bo_unpin(nvbo); > > >> + } > > >> + > > >> + return ret; > > >> } > > >> > > >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > > >> -- > > >> 2.25.1 > > >> > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-22 9:20 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-22 9:20 UTC (permalink / raw) To: Christian König; +Cc: Nouveau Dev, amd-gfx list, dri-devel On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Jun 21, 2021 at 5:49 PM Christian König > <ckoenig.leichtzumerken@gmail.com> wrote: > > > > Am 21.06.21 um 16:54 schrieb Daniel Vetter: > > > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > > >> We actually need to wait for the moving fence after pinning > > >> the BO to make sure that the pin is completed. > > >> > > >> Signed-off-by: Christian König <christian.koenig@amd.com> > > >> CC: stable@kernel.org > > >> --- > > >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> index 347488685f74..591738545eba 100644 > > >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > > >> if (ret) > > >> return -EINVAL; > > >> > > >> - return 0; > > >> + if (nvbo->bo.moving) { > > > Don't we need to hold the dma_resv to read this? We can grab a reference > > > and then unlock, but I think just unlocked wait can go boom pretty easily > > > (since we don't hold a reference or lock so someone else can jump in and > > > free the moving fence). > > > > The moving fence is only modified while the BO is moved and since we > > have just successfully pinned it.... > > Yeah ... so probably correct, but really tricky. Just wrapping a > ttm_bo_reserve/unreserve around the code you add should be enough and > get the job done? I think you distracted me a bit with the "it can't move", so yes there's a guarantee that no other fence can show up in ttm_bo->moving and confuse us. But it could get set to NULL because someone realized it signalled. We're not doing that systematically, but relying on fences never getting garbage-collected for correctness isn't great. Sot the ttm_bo_reserve/unreserve is definitely needed here around this bit of code. You don't need to merge it with the reserve/unreserve in the pin function though, it's just to protect against the use-after-free. -Daniel > > > But in general I agree that it would be better to avoid this. I just > > didn't wanted to open a bigger can of worms by changing nouveau so much. > > Yeah, but I'm kinda thinking of some helpers to wait for the move > fence (so that later on we can switch from having the exclusive fence > to the move fence do that, maybe). And then locking checks in there > would be nice. > > Also avoids the case of explaining why lockless here is fine, but > lockless wait for the exclusive fence in e.g. a dynami dma-buf > importer is very much not fine at all. Just all around less trouble. > -Daniel > > > > > Christian. > > > > > -Daniel > > > > > >> + ret = dma_fence_wait(nvbo->bo.moving, true); > > >> + if (ret) > > >> + nouveau_bo_unpin(nvbo); > > >> + } > > >> + > > >> + return ret; > > >> } > > >> > > >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > > >> -- > > >> 2.25.1 > > >> > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-22 9:20 ` Daniel Vetter 0 siblings, 0 replies; 30+ messages in thread From: Daniel Vetter @ 2021-06-22 9:20 UTC (permalink / raw) To: Christian König; +Cc: Nouveau Dev, amd-gfx list, dri-devel On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Jun 21, 2021 at 5:49 PM Christian König > <ckoenig.leichtzumerken@gmail.com> wrote: > > > > Am 21.06.21 um 16:54 schrieb Daniel Vetter: > > > On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: > > >> We actually need to wait for the moving fence after pinning > > >> the BO to make sure that the pin is completed. > > >> > > >> Signed-off-by: Christian König <christian.koenig@amd.com> > > >> CC: stable@kernel.org > > >> --- > > >> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- > > >> 1 file changed, 7 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> index 347488685f74..591738545eba 100644 > > >> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > > >> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) > > >> if (ret) > > >> return -EINVAL; > > >> > > >> - return 0; > > >> + if (nvbo->bo.moving) { > > > Don't we need to hold the dma_resv to read this? We can grab a reference > > > and then unlock, but I think just unlocked wait can go boom pretty easily > > > (since we don't hold a reference or lock so someone else can jump in and > > > free the moving fence). > > > > The moving fence is only modified while the BO is moved and since we > > have just successfully pinned it.... > > Yeah ... so probably correct, but really tricky. Just wrapping a > ttm_bo_reserve/unreserve around the code you add should be enough and > get the job done? I think you distracted me a bit with the "it can't move", so yes there's a guarantee that no other fence can show up in ttm_bo->moving and confuse us. But it could get set to NULL because someone realized it signalled. We're not doing that systematically, but relying on fences never getting garbage-collected for correctness isn't great. Sot the ttm_bo_reserve/unreserve is definitely needed here around this bit of code. You don't need to merge it with the reserve/unreserve in the pin function though, it's just to protect against the use-after-free. -Daniel > > > But in general I agree that it would be better to avoid this. I just > > didn't wanted to open a bigger can of worms by changing nouveau so much. > > Yeah, but I'm kinda thinking of some helpers to wait for the move > fence (so that later on we can switch from having the exclusive fence > to the move fence do that, maybe). And then locking checks in there > would be nice. > > Also avoids the case of explaining why lockless here is fine, but > lockless wait for the exclusive fence in e.g. a dynami dma-buf > importer is very much not fine at all. Just all around less trouble. > -Daniel > > > > > Christian. > > > > > -Daniel > > > > > >> + ret = dma_fence_wait(nvbo->bo.moving, true); > > >> + if (ret) > > >> + nouveau_bo_unpin(nvbo); > > >> + } > > >> + > > >> + return ret; > > >> } > > >> > > >> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) > > >> -- > > >> 2.25.1 > > >> > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning 2021-06-22 9:20 ` Daniel Vetter (?) @ 2021-06-22 9:29 ` Christian König -1 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-22 9:29 UTC (permalink / raw) To: Daniel Vetter; +Cc: Nouveau Dev, amd-gfx list, dri-devel Am 22.06.21 um 11:20 schrieb Daniel Vetter: > On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel@ffwll.ch> wrote: >> On Mon, Jun 21, 2021 at 5:49 PM Christian König >> <ckoenig.leichtzumerken@gmail.com> wrote: >>> Am 21.06.21 um 16:54 schrieb Daniel Vetter: >>>> On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: >>>>> We actually need to wait for the moving fence after pinning >>>>> the BO to make sure that the pin is completed. >>>>> >>>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>>> CC: stable@kernel.org >>>>> --- >>>>> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- >>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> index 347488685f74..591738545eba 100644 >>>>> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) >>>>> if (ret) >>>>> return -EINVAL; >>>>> >>>>> - return 0; >>>>> + if (nvbo->bo.moving) { >>>> Don't we need to hold the dma_resv to read this? We can grab a reference >>>> and then unlock, but I think just unlocked wait can go boom pretty easily >>>> (since we don't hold a reference or lock so someone else can jump in and >>>> free the moving fence). >>> The moving fence is only modified while the BO is moved and since we >>> have just successfully pinned it.... >> Yeah ... so probably correct, but really tricky. Just wrapping a >> ttm_bo_reserve/unreserve around the code you add should be enough and >> get the job done? > I think you distracted me a bit with the "it can't move", so yes > there's a guarantee that no other fence can show up in ttm_bo->moving > and confuse us. But it could get set to NULL because someone realized > it signalled. We're not doing that systematically, but relying on > fences never getting garbage-collected for correctness isn't great. Yeah, that's what I essentially meant with it would be better in general to take the lock. > > Sot the ttm_bo_reserve/unreserve is definitely needed here around this > bit of code. You don't need to merge it with the reserve/unreserve in > the pin function though, it's just to protect against the > use-after-free. Ah, yes good point. That means I don't need to change the pin/unpin functions in nouveau at all. BTW: What do you think of making dma_fence_is_signaled() and dma_fence_wait_timeout() save to passing in NULL as fence? I think we have a lot of cases where we check "!fence || dma_fence_is_signaled(fence)" or similar. Christian. > -Daniel > >>> But in general I agree that it would be better to avoid this. I just >>> didn't wanted to open a bigger can of worms by changing nouveau so much. >> Yeah, but I'm kinda thinking of some helpers to wait for the move >> fence (so that later on we can switch from having the exclusive fence >> to the move fence do that, maybe). And then locking checks in there >> would be nice. >> >> Also avoids the case of explaining why lockless here is fine, but >> lockless wait for the exclusive fence in e.g. a dynami dma-buf >> importer is very much not fine at all. Just all around less trouble. >> -Daniel >> >>> Christian. >>> >>>> -Daniel >>>> >>>>> + ret = dma_fence_wait(nvbo->bo.moving, true); >>>>> + if (ret) >>>>> + nouveau_bo_unpin(nvbo); >>>>> + } >>>>> + >>>>> + return ret; >>>>> } >>>>> >>>>> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) >>>>> -- >>>>> 2.25.1 >>>>> >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > > _______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-22 9:29 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-22 9:29 UTC (permalink / raw) To: Daniel Vetter; +Cc: Nouveau Dev, amd-gfx list, dri-devel Am 22.06.21 um 11:20 schrieb Daniel Vetter: > On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel@ffwll.ch> wrote: >> On Mon, Jun 21, 2021 at 5:49 PM Christian König >> <ckoenig.leichtzumerken@gmail.com> wrote: >>> Am 21.06.21 um 16:54 schrieb Daniel Vetter: >>>> On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: >>>>> We actually need to wait for the moving fence after pinning >>>>> the BO to make sure that the pin is completed. >>>>> >>>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>>> CC: stable@kernel.org >>>>> --- >>>>> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- >>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> index 347488685f74..591738545eba 100644 >>>>> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) >>>>> if (ret) >>>>> return -EINVAL; >>>>> >>>>> - return 0; >>>>> + if (nvbo->bo.moving) { >>>> Don't we need to hold the dma_resv to read this? We can grab a reference >>>> and then unlock, but I think just unlocked wait can go boom pretty easily >>>> (since we don't hold a reference or lock so someone else can jump in and >>>> free the moving fence). >>> The moving fence is only modified while the BO is moved and since we >>> have just successfully pinned it.... >> Yeah ... so probably correct, but really tricky. Just wrapping a >> ttm_bo_reserve/unreserve around the code you add should be enough and >> get the job done? > I think you distracted me a bit with the "it can't move", so yes > there's a guarantee that no other fence can show up in ttm_bo->moving > and confuse us. But it could get set to NULL because someone realized > it signalled. We're not doing that systematically, but relying on > fences never getting garbage-collected for correctness isn't great. Yeah, that's what I essentially meant with it would be better in general to take the lock. > > Sot the ttm_bo_reserve/unreserve is definitely needed here around this > bit of code. You don't need to merge it with the reserve/unreserve in > the pin function though, it's just to protect against the > use-after-free. Ah, yes good point. That means I don't need to change the pin/unpin functions in nouveau at all. BTW: What do you think of making dma_fence_is_signaled() and dma_fence_wait_timeout() save to passing in NULL as fence? I think we have a lot of cases where we check "!fence || dma_fence_is_signaled(fence)" or similar. Christian. > -Daniel > >>> But in general I agree that it would be better to avoid this. I just >>> didn't wanted to open a bigger can of worms by changing nouveau so much. >> Yeah, but I'm kinda thinking of some helpers to wait for the move >> fence (so that later on we can switch from having the exclusive fence >> to the move fence do that, maybe). And then locking checks in there >> would be nice. >> >> Also avoids the case of explaining why lockless here is fine, but >> lockless wait for the exclusive fence in e.g. a dynami dma-buf >> importer is very much not fine at all. Just all around less trouble. >> -Daniel >> >>> Christian. >>> >>>> -Daniel >>>> >>>>> + ret = dma_fence_wait(nvbo->bo.moving, true); >>>>> + if (ret) >>>>> + nouveau_bo_unpin(nvbo); >>>>> + } >>>>> + >>>>> + return ret; >>>>> } >>>>> >>>>> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) >>>>> -- >>>>> 2.25.1 >>>>> >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/3] drm/nouveau: wait for moving fence after pinning @ 2021-06-22 9:29 ` Christian König 0 siblings, 0 replies; 30+ messages in thread From: Christian König @ 2021-06-22 9:29 UTC (permalink / raw) To: Daniel Vetter; +Cc: Nouveau Dev, amd-gfx list, dri-devel Am 22.06.21 um 11:20 schrieb Daniel Vetter: > On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel@ffwll.ch> wrote: >> On Mon, Jun 21, 2021 at 5:49 PM Christian König >> <ckoenig.leichtzumerken@gmail.com> wrote: >>> Am 21.06.21 um 16:54 schrieb Daniel Vetter: >>>> On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote: >>>>> We actually need to wait for the moving fence after pinning >>>>> the BO to make sure that the pin is completed. >>>>> >>>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>>> CC: stable@kernel.org >>>>> --- >>>>> drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++- >>>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> index 347488685f74..591738545eba 100644 >>>>> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c >>>>> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj) >>>>> if (ret) >>>>> return -EINVAL; >>>>> >>>>> - return 0; >>>>> + if (nvbo->bo.moving) { >>>> Don't we need to hold the dma_resv to read this? We can grab a reference >>>> and then unlock, but I think just unlocked wait can go boom pretty easily >>>> (since we don't hold a reference or lock so someone else can jump in and >>>> free the moving fence). >>> The moving fence is only modified while the BO is moved and since we >>> have just successfully pinned it.... >> Yeah ... so probably correct, but really tricky. Just wrapping a >> ttm_bo_reserve/unreserve around the code you add should be enough and >> get the job done? > I think you distracted me a bit with the "it can't move", so yes > there's a guarantee that no other fence can show up in ttm_bo->moving > and confuse us. But it could get set to NULL because someone realized > it signalled. We're not doing that systematically, but relying on > fences never getting garbage-collected for correctness isn't great. Yeah, that's what I essentially meant with it would be better in general to take the lock. > > Sot the ttm_bo_reserve/unreserve is definitely needed here around this > bit of code. You don't need to merge it with the reserve/unreserve in > the pin function though, it's just to protect against the > use-after-free. Ah, yes good point. That means I don't need to change the pin/unpin functions in nouveau at all. BTW: What do you think of making dma_fence_is_signaled() and dma_fence_wait_timeout() save to passing in NULL as fence? I think we have a lot of cases where we check "!fence || dma_fence_is_signaled(fence)" or similar. Christian. > -Daniel > >>> But in general I agree that it would be better to avoid this. I just >>> didn't wanted to open a bigger can of worms by changing nouveau so much. >> Yeah, but I'm kinda thinking of some helpers to wait for the move >> fence (so that later on we can switch from having the exclusive fence >> to the move fence do that, maybe). And then locking checks in there >> would be nice. >> >> Also avoids the case of explaining why lockless here is fine, but >> lockless wait for the exclusive fence in e.g. a dynami dma-buf >> importer is very much not fine at all. Just all around less trouble. >> -Daniel >> >>> Christian. >>> >>>> -Daniel >>>> >>>>> + ret = dma_fence_wait(nvbo->bo.moving, true); >>>>> + if (ret) >>>>> + nouveau_bo_unpin(nvbo); >>>>> + } >>>>> + >>>>> + return ret; >>>>> } >>>>> >>>>> void nouveau_gem_prime_unpin(struct drm_gem_object *obj) >>>>> -- >>>>> 2.25.1 >>>>> >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > > ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2021-06-22 9:29 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-21 13:03 [Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning Christian König 2021-06-21 13:03 ` Christian König 2021-06-21 13:03 ` Christian König 2021-06-21 13:03 ` [Nouveau] [PATCH 2/3] drm/radeon: " Christian König 2021-06-21 13:03 ` Christian König 2021-06-21 13:03 ` Christian König 2021-06-21 14:55 ` [Nouveau] " Daniel Vetter 2021-06-21 14:55 ` Daniel Vetter 2021-06-21 14:55 ` Daniel Vetter 2021-06-21 13:03 ` [Nouveau] [PATCH 3/3] drm/amdgpu: " Christian König 2021-06-21 13:03 ` Christian König 2021-06-21 13:03 ` Christian König 2021-06-21 14:56 ` [Nouveau] " Daniel Vetter 2021-06-21 14:56 ` Daniel Vetter 2021-06-21 14:56 ` Daniel Vetter 2021-06-21 14:54 ` [Nouveau] [PATCH 1/3] drm/nouveau: " Daniel Vetter 2021-06-21 14:54 ` Daniel Vetter 2021-06-21 14:54 ` Daniel Vetter 2021-06-21 15:49 ` [Nouveau] " Christian König 2021-06-21 15:49 ` Christian König 2021-06-21 15:49 ` Christian König 2021-06-21 15:53 ` [Nouveau] " Daniel Vetter 2021-06-21 15:53 ` Daniel Vetter 2021-06-21 15:53 ` Daniel Vetter 2021-06-22 9:20 ` [Nouveau] " Daniel Vetter 2021-06-22 9:20 ` Daniel Vetter 2021-06-22 9:20 ` Daniel Vetter 2021-06-22 9:29 ` [Nouveau] " Christian König 2021-06-22 9:29 ` Christian König 2021-06-22 9:29 ` Christian König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.