Nouveau - Jun 2021 - [PATCH 1/3] drm/nouveau: wait for moving fence after pinning

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 at amd.com>
> CC: stable at 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

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 at amd.com>
>> CC: stable at 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
>>