[PATCH] drm/amd/display: wait for fence without holding reservation lock

[PATCH] drm/amd/display: wait for fence without holding reservation lock

[Christian König]

Don't block others while waiting for the fences to finish, concurrent
submission is perfectly valid in this case and holding the lock can
prevent killed applications from terminating.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 697af51d403a..9426e701a7b6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5129,23 +5129,26 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			continue;
 		}
 
+		abo = gem_to_amdgpu_bo(fb->obj[0]);
+
+		/* Wait for all fences on this FB */
+		r = reservation_object_wait_timeout_rcu(abo->tbo.resv, true,
+							false,
+							MAX_SCHEDULE_TIMEOUT);
+		WARN_ON(r < 0);
+
 		/*
 		 * TODO This might fail and hence better not used, wait
 		 * explicitly on fences instead
 		 * and in general should be called for
 		 * blocking commit to as per framework helpers
 		 */
-		abo = gem_to_amdgpu_bo(fb->obj[0]);
 		r = amdgpu_bo_reserve(abo, true);
 		if (unlikely(r != 0)) {
 			DRM_ERROR("failed to reserve buffer before flip\n");
 			WARN_ON(1);
 		}
 
-		/* Wait for all fences on this FB */
-		WARN_ON(reservation_object_wait_timeout_rcu(abo->tbo.resv, true, false,
-									    MAX_SCHEDULE_TIMEOUT) < 0);
-
 		amdgpu_bo_get_tiling_flags(abo, &tiling_flags);
 
 		amdgpu_bo_unreserve(abo);
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: wait for fence without holding reservation lock

[Kazlauskas, Nicholas]

On 4/2/19 10:28 AM, Christian König wrote:
> Don't block others while waiting for the fences to finish, concurrent
> submission is perfectly valid in this case and holding the lock can
> prevent killed applications from terminating.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

I do recall having the discussion about this a while back with you and 
Andrey.

I highly doubt that the flags would ever really change here (if they're 
even capable) between the wait and the lock so I think this is fine.

I think there's a bigger problem with that and how they could 
potentially change between atomic check and atomic commit tail anyway 
(and doing something bad after we think it's passed validation).

But this is mostly a theoretical concern I think rather than a practical 
one.

Thanks.

Nicholas Kazlauskas


> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 697af51d403a..9426e701a7b6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5129,23 +5129,26 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   			continue;
>   		}
>   
> +		abo = gem_to_amdgpu_bo(fb->obj[0]);
> +
> +		/* Wait for all fences on this FB */
> +		r = reservation_object_wait_timeout_rcu(abo->tbo.resv, true,
> +							false,
> +							MAX_SCHEDULE_TIMEOUT);
> +		WARN_ON(r < 0);
> +
>   		/*
>   		 * TODO This might fail and hence better not used, wait
>   		 * explicitly on fences instead
>   		 * and in general should be called for
>   		 * blocking commit to as per framework helpers
>   		 */
> -		abo = gem_to_amdgpu_bo(fb->obj[0]);
>   		r = amdgpu_bo_reserve(abo, true);
>   		if (unlikely(r != 0)) {
>   			DRM_ERROR("failed to reserve buffer before flip\n");
>   			WARN_ON(1);
>   		}
>   
> -		/* Wait for all fences on this FB */
> -		WARN_ON(reservation_object_wait_timeout_rcu(abo->tbo.resv, true, false,
> -									    MAX_SCHEDULE_TIMEOUT) < 0);
> -
>   		amdgpu_bo_get_tiling_flags(abo, &tiling_flags);
>   
>   		amdgpu_bo_unreserve(abo);
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: wait for fence without holding reservation lock

[Kazlauskas, Nicholas]

On 4/2/19 10:41 AM, Kazlauskas, Nicholas wrote:
> On 4/2/19 10:28 AM, Christian König wrote:
>> Don't block others while waiting for the fences to finish, concurrent
>> submission is perfectly valid in this case and holding the lock can
>> prevent killed applications from terminating.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> 
> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> 
> I do recall having the discussion about this a while back with you and
> Andrey.
> 
> I highly doubt that the flags would ever really change here (if they're
> even capable) between the wait and the lock so I think this is fine.
> 
> I think there's a bigger problem with that and how they could
> potentially change between atomic check and atomic commit tail anyway
> (and doing something bad after we think it's passed validation).
> 
> But this is mostly a theoretical concern I think rather than a practical
> one.
> 
> Thanks.
> 
> Nicholas Kazlauskas

...and waiting for the fence isn't really analogous to ensuring that 
they can't change in the first place I suppose.

This looks fine.

Nicholas Kazlauskas


> 
> 
>> ---
>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 ++++++++-----
>>    1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 697af51d403a..9426e701a7b6 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -5129,23 +5129,26 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>    			continue;
>>    		}
>>    
>> +		abo = gem_to_amdgpu_bo(fb->obj[0]);
>> +
>> +		/* Wait for all fences on this FB */
>> +		r = reservation_object_wait_timeout_rcu(abo->tbo.resv, true,
>> +							false,
>> +							MAX_SCHEDULE_TIMEOUT);
>> +		WARN_ON(r < 0);
>> +
>>    		/*
>>    		 * TODO This might fail and hence better not used, wait
>>    		 * explicitly on fences instead
>>    		 * and in general should be called for
>>    		 * blocking commit to as per framework helpers
>>    		 */
>> -		abo = gem_to_amdgpu_bo(fb->obj[0]);
>>    		r = amdgpu_bo_reserve(abo, true);
>>    		if (unlikely(r != 0)) {
>>    			DRM_ERROR("failed to reserve buffer before flip\n");
>>    			WARN_ON(1);
>>    		}
>>    
>> -		/* Wait for all fences on this FB */
>> -		WARN_ON(reservation_object_wait_timeout_rcu(abo->tbo.resv, true, false,
>> -									    MAX_SCHEDULE_TIMEOUT) < 0);
>> -
>>    		amdgpu_bo_get_tiling_flags(abo, &tiling_flags);
>>    
>>    		amdgpu_bo_unreserve(abo);
>>
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx