[2/2] drm/amd/display: Remove wait for hw/flip done in atomic check

Submitted by Kazlauskas, Nicholas on Nov. 22, 2018, 5:34 p.m.

Details

Message ID 20181122173437.13538-2-nicholas.kazlauskas@amd.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Kazlauskas, Nicholas Nov. 22, 2018, 5:34 p.m.
[Why]
Atomic check can't truly be non-blocking if amdgpu_dm is waiting for
hw_done and flip_done in atomic check. This introduces waits when
any previous non-blocking commits queued work on a worker thread and
a new atomic commit attempts to do another full update/modeset.

[How]
Drop the waits and move the global lock acqusition into atomic check.

This is fine to do since commit tail waits for these dependencies
before calling amdgpu_dm_atomic_commit_tail.

This is only safe as long as DC never queries anything from within
current_state when doing validation in atomic_check. This is the
case as of writing, but any future uses of dc->current_state from
within atomic_check should be considered incorrect.

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------
 1 file changed, 6 insertions(+), 52 deletions(-)

Patch hide | download patch | download mbox

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 3ae438d9849f..fe21bb86b66a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5078,57 +5078,6 @@  void dm_restore_drm_connector_state(struct drm_device *dev,
 		dm_force_atomic_commit(&aconnector->base);
 }
 
-/*
- * Grabs all modesetting locks to serialize against any blocking commits,
- * Waits for completion of all non blocking commits.
- */
-static int do_aquire_global_lock(struct drm_device *dev,
-				 struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_commit *commit;
-	long ret;
-
-	/*
-	 * Adding all modeset locks to aquire_ctx will
-	 * ensure that when the framework release it the
-	 * extra locks we are locking here will get released to
-	 */
-	ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
-	if (ret)
-		return ret;
-
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		spin_lock(&crtc->commit_lock);
-		commit = list_first_entry_or_null(&crtc->commit_list,
-				struct drm_crtc_commit, commit_entry);
-		if (commit)
-			drm_crtc_commit_get(commit);
-		spin_unlock(&crtc->commit_lock);
-
-		if (!commit)
-			continue;
-
-		/*
-		 * Make sure all pending HW programming completed and
-		 * page flips done
-		 */
-		ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);
-
-		if (ret > 0)
-			ret = wait_for_completion_interruptible_timeout(
-					&commit->flip_done, 10*HZ);
-
-		if (ret == 0)
-			DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "
-				  "timed out\n", crtc->base.id, crtc->name);
-
-		drm_crtc_commit_put(commit);
-	}
-
-	return ret < 0 ? ret : 0;
-}
-
 void set_freesync_on_stream(struct amdgpu_display_manager *dm,
 			    struct dm_crtc_state *new_crtc_state,
 			    struct dm_connector_state *new_con_state,
@@ -5793,7 +5742,12 @@  static int amdgpu_dm_atomic_check(struct drm_device *dev,
 		if (ret)
 			goto fail;
 
-		ret = do_aquire_global_lock(dev, state);
+		/*
+		 * This should be replaced with finer locking on the
+		 * on the appropriate resources when possible.
+		 * For now it's safer to grab everything.
+		 */
+		ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
 		if (ret)
 			goto fail;
 

Comments

On 11/22/2018 12:34 PM, Nicholas Kazlauskas wrote:
> [Why]

> Atomic check can't truly be non-blocking if amdgpu_dm is waiting for

> hw_done and flip_done in atomic check. This introduces waits when

> any previous non-blocking commits queued work on a worker thread and

> a new atomic commit attempts to do another full update/modeset.

>

> [How]

> Drop the waits and move the global lock acqusition into atomic check.

>

> This is fine to do since commit tail waits for these dependencies

> before calling amdgpu_dm_atomic_commit_tail.


Note that drm_atomic_helper_wait_for_dependencies waits only on all 
preceeding commits that touch the same CRTC
while our custom do_aquire_global_lock wait on ALL previous commits in 
the system - even on other CRTCS. As far as I can
remember that was important because we have global dc_state context 
which must be protected for access/modifications while
looks like DRM assumes unrelated CRTCs can be modified concurrently.
Try to verify that latest display code will not be broken by this 
relaxation of synchronization.

Andrey

>

> This is only safe as long as DC never queries anything from within

> current_state when doing validation in atomic_check. This is the

> case as of writing, but any future uses of dc->current_state from

> within atomic_check should be considered incorrect.

>

> Cc: Harry Wentland <harry.wentland@amd.com>

> Cc: Leo Li <sunpeng.li@amd.com>

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

> ---

>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------

>   1 file changed, 6 insertions(+), 52 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 3ae438d9849f..fe21bb86b66a 100644

> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

> @@ -5078,57 +5078,6 @@ void dm_restore_drm_connector_state(struct drm_device *dev,

>   		dm_force_atomic_commit(&aconnector->base);

>   }

>   

> -/*

> - * Grabs all modesetting locks to serialize against any blocking commits,

> - * Waits for completion of all non blocking commits.

> - */

> -static int do_aquire_global_lock(struct drm_device *dev,

> -				 struct drm_atomic_state *state)

> -{

> -	struct drm_crtc *crtc;

> -	struct drm_crtc_commit *commit;

> -	long ret;

> -

> -	/*

> -	 * Adding all modeset locks to aquire_ctx will

> -	 * ensure that when the framework release it the

> -	 * extra locks we are locking here will get released to

> -	 */

> -	ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);

> -	if (ret)

> -		return ret;

> -

> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {

> -		spin_lock(&crtc->commit_lock);

> -		commit = list_first_entry_or_null(&crtc->commit_list,

> -				struct drm_crtc_commit, commit_entry);

> -		if (commit)

> -			drm_crtc_commit_get(commit);

> -		spin_unlock(&crtc->commit_lock);

> -

> -		if (!commit)

> -			continue;

> -

> -		/*

> -		 * Make sure all pending HW programming completed and

> -		 * page flips done

> -		 */

> -		ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);

> -

> -		if (ret > 0)

> -			ret = wait_for_completion_interruptible_timeout(

> -					&commit->flip_done, 10*HZ);

> -

> -		if (ret == 0)

> -			DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "

> -				  "timed out\n", crtc->base.id, crtc->name);

> -

> -		drm_crtc_commit_put(commit);

> -	}

> -

> -	return ret < 0 ? ret : 0;

> -}

> -

>   void set_freesync_on_stream(struct amdgpu_display_manager *dm,

>   			    struct dm_crtc_state *new_crtc_state,

>   			    struct dm_connector_state *new_con_state,

> @@ -5793,7 +5742,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,

>   		if (ret)

>   			goto fail;

>   

> -		ret = do_aquire_global_lock(dev, state);

> +		/*

> +		 * This should be replaced with finer locking on the

> +		 * on the appropriate resources when possible.

> +		 * For now it's safer to grab everything.

> +		 */

> +		ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);

>   		if (ret)

>   			goto fail;

>
On 11/22/18 2:39 PM, Grodzovsky, Andrey wrote:
> 

> 

> On 11/22/2018 12:34 PM, Nicholas Kazlauskas wrote:

>> [Why]

>> Atomic check can't truly be non-blocking if amdgpu_dm is waiting for

>> hw_done and flip_done in atomic check. This introduces waits when

>> any previous non-blocking commits queued work on a worker thread and

>> a new atomic commit attempts to do another full update/modeset.

>>

>> [How]

>> Drop the waits and move the global lock acqusition into atomic check.

>>

>> This is fine to do since commit tail waits for these dependencies

>> before calling amdgpu_dm_atomic_commit_tail.

> 

> Note that drm_atomic_helper_wait_for_dependencies waits only on all

> preceeding commits that touch the same CRTC

> while our custom do_aquire_global_lock wait on ALL previous commits in

> the system - even on other CRTCS. As far as I can

> remember that was important because we have global dc_state context

> which must be protected for access/modifications while

> looks like DRM assumes unrelated CRTCs can be modified concurrently.

> Try to verify that latest display code will not be broken by this

> relaxation of synchronization.

> 

> Andrey


This is a good point.

What we should really be doing instead here is locking anything that can 
modify dc->current_state from within atomic commit tail. Serializing 
commits into a queue could work too.

Either should fix non-blocking support.

Nicholas Kazlauskas

> 

>>

>> This is only safe as long as DC never queries anything from within

>> current_state when doing validation in atomic_check. This is the

>> case as of writing, but any future uses of dc->current_state from

>> within atomic_check should be considered incorrect.

>>

>> Cc: Harry Wentland <harry.wentland@amd.com>

>> Cc: Leo Li <sunpeng.li@amd.com>

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

>> ---

>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------

>>    1 file changed, 6 insertions(+), 52 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 3ae438d9849f..fe21bb86b66a 100644

>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

>> @@ -5078,57 +5078,6 @@ void dm_restore_drm_connector_state(struct drm_device *dev,

>>    		dm_force_atomic_commit(&aconnector->base);

>>    }

>>    

>> -/*

>> - * Grabs all modesetting locks to serialize against any blocking commits,

>> - * Waits for completion of all non blocking commits.

>> - */

>> -static int do_aquire_global_lock(struct drm_device *dev,

>> -				 struct drm_atomic_state *state)

>> -{

>> -	struct drm_crtc *crtc;

>> -	struct drm_crtc_commit *commit;

>> -	long ret;

>> -

>> -	/*

>> -	 * Adding all modeset locks to aquire_ctx will

>> -	 * ensure that when the framework release it the

>> -	 * extra locks we are locking here will get released to

>> -	 */

>> -	ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);

>> -	if (ret)

>> -		return ret;

>> -

>> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {

>> -		spin_lock(&crtc->commit_lock);

>> -		commit = list_first_entry_or_null(&crtc->commit_list,

>> -				struct drm_crtc_commit, commit_entry);

>> -		if (commit)

>> -			drm_crtc_commit_get(commit);

>> -		spin_unlock(&crtc->commit_lock);

>> -

>> -		if (!commit)

>> -			continue;

>> -

>> -		/*

>> -		 * Make sure all pending HW programming completed and

>> -		 * page flips done

>> -		 */

>> -		ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);

>> -

>> -		if (ret > 0)

>> -			ret = wait_for_completion_interruptible_timeout(

>> -					&commit->flip_done, 10*HZ);

>> -

>> -		if (ret == 0)

>> -			DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "

>> -				  "timed out\n", crtc->base.id, crtc->name);

>> -

>> -		drm_crtc_commit_put(commit);

>> -	}

>> -

>> -	return ret < 0 ? ret : 0;

>> -}

>> -

>>    void set_freesync_on_stream(struct amdgpu_display_manager *dm,

>>    			    struct dm_crtc_state *new_crtc_state,

>>    			    struct dm_connector_state *new_con_state,

>> @@ -5793,7 +5742,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,

>>    		if (ret)

>>    			goto fail;

>>    

>> -		ret = do_aquire_global_lock(dev, state);

>> +		/*

>> +		 * This should be replaced with finer locking on the

>> +		 * on the appropriate resources when possible.

>> +		 * For now it's safer to grab everything.

>> +		 */

>> +		ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);

>>    		if (ret)

>>    			goto fail;

>>    

> 

> _______________________________________________

> amd-gfx mailing list

> amd-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

>
On 11/22/2018 02:43 PM, Kazlauskas, Nicholas wrote:
> On 11/22/18 2:39 PM, Grodzovsky, Andrey wrote:

>>

>> On 11/22/2018 12:34 PM, Nicholas Kazlauskas wrote:

>>> [Why]

>>> Atomic check can't truly be non-blocking if amdgpu_dm is waiting for

>>> hw_done and flip_done in atomic check. This introduces waits when

>>> any previous non-blocking commits queued work on a worker thread and

>>> a new atomic commit attempts to do another full update/modeset.

>>>

>>> [How]

>>> Drop the waits and move the global lock acqusition into atomic check.

>>>

>>> This is fine to do since commit tail waits for these dependencies

>>> before calling amdgpu_dm_atomic_commit_tail.

>> Note that drm_atomic_helper_wait_for_dependencies waits only on all

>> preceeding commits that touch the same CRTC

>> while our custom do_aquire_global_lock wait on ALL previous commits in

>> the system - even on other CRTCS. As far as I can

>> remember that was important because we have global dc_state context

>> which must be protected for access/modifications while

>> looks like DRM assumes unrelated CRTCs can be modified concurrently.

>> Try to verify that latest display code will not be broken by this

>> relaxation of synchronization.

>>

>> Andrey

> This is a good point.

>

> What we should really be doing instead here is locking anything that can

> modify dc->current_state from within atomic commit tail. Serializing

> commits into a queue could work too.


This is a possibility by then you have to duplicate 
drm_atomic_helper_commit with only once change which
is to substitute system_unbound_wq with you costume WQ. Alternatively 
propose a changeĀ  to update drm_atomic_helper_commit
to allow WQs as parameter.

Andrey

>

> Either should fix non-blocking support.

>

> Nicholas Kazlauskas

>

>>> This is only safe as long as DC never queries anything from within

>>> current_state when doing validation in atomic_check. This is the

>>> case as of writing, but any future uses of dc->current_state from

>>> within atomic_check should be considered incorrect.

>>>

>>> Cc: Harry Wentland <harry.wentland@amd.com>

>>> Cc: Leo Li <sunpeng.li@amd.com>

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

>>> ---

>>>     .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------

>>>     1 file changed, 6 insertions(+), 52 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 3ae438d9849f..fe21bb86b66a 100644

>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

>>> @@ -5078,57 +5078,6 @@ void dm_restore_drm_connector_state(struct drm_device *dev,

>>>     		dm_force_atomic_commit(&aconnector->base);

>>>     }

>>>     

>>> -/*

>>> - * Grabs all modesetting locks to serialize against any blocking commits,

>>> - * Waits for completion of all non blocking commits.

>>> - */

>>> -static int do_aquire_global_lock(struct drm_device *dev,

>>> -				 struct drm_atomic_state *state)

>>> -{

>>> -	struct drm_crtc *crtc;

>>> -	struct drm_crtc_commit *commit;

>>> -	long ret;

>>> -

>>> -	/*

>>> -	 * Adding all modeset locks to aquire_ctx will

>>> -	 * ensure that when the framework release it the

>>> -	 * extra locks we are locking here will get released to

>>> -	 */

>>> -	ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);

>>> -	if (ret)

>>> -		return ret;

>>> -

>>> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {

>>> -		spin_lock(&crtc->commit_lock);

>>> -		commit = list_first_entry_or_null(&crtc->commit_list,

>>> -				struct drm_crtc_commit, commit_entry);

>>> -		if (commit)

>>> -			drm_crtc_commit_get(commit);

>>> -		spin_unlock(&crtc->commit_lock);

>>> -

>>> -		if (!commit)

>>> -			continue;

>>> -

>>> -		/*

>>> -		 * Make sure all pending HW programming completed and

>>> -		 * page flips done

>>> -		 */

>>> -		ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);

>>> -

>>> -		if (ret > 0)

>>> -			ret = wait_for_completion_interruptible_timeout(

>>> -					&commit->flip_done, 10*HZ);

>>> -

>>> -		if (ret == 0)

>>> -			DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "

>>> -				  "timed out\n", crtc->base.id, crtc->name);

>>> -

>>> -		drm_crtc_commit_put(commit);

>>> -	}

>>> -

>>> -	return ret < 0 ? ret : 0;

>>> -}

>>> -

>>>     void set_freesync_on_stream(struct amdgpu_display_manager *dm,

>>>     			    struct dm_crtc_state *new_crtc_state,

>>>     			    struct dm_connector_state *new_con_state,

>>> @@ -5793,7 +5742,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,

>>>     		if (ret)

>>>     			goto fail;

>>>     

>>> -		ret = do_aquire_global_lock(dev, state);

>>> +		/*

>>> +		 * This should be replaced with finer locking on the

>>> +		 * on the appropriate resources when possible.

>>> +		 * For now it's safer to grab everything.

>>> +		 */

>>> +		ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);

>>>     		if (ret)

>>>     			goto fail;

>>>     

>> _______________________________________________

>> 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