diff mbox

drm/atomic: Make atomic helper track newly assigned planes correctly, v2.

Message ID 20171017052047.8983-1-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maarten Lankhorst Oct. 17, 2017, 5:20 a.m. UTC
Commit 669c9215afea ("drm/atomic: Make async plane update checks work as
intended, v2.") forced planes to always be tracked, but forgot to
explicitly get the crtc commit from the new crtc when available.

This broke plane commit tracking, and caused kms_atomic_transitions
to randomly fail with -EBUSY.

Changes since v1:
- Prefer new_crtc_state->crtc above old_crtc_state->crtc.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102671
Testcase: kms_atomic_transitions
---
 drivers/gpu/drm/drm_atomic_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Vetter Oct. 17, 2017, 12:11 p.m. UTC | #1
On Tue, Oct 17, 2017 at 07:20:47AM +0200, Maarten Lankhorst wrote:
> Commit 669c9215afea ("drm/atomic: Make async plane update checks work as
> intended, v2.") forced planes to always be tracked, but forgot to
> explicitly get the crtc commit from the new crtc when available.
> 
> This broke plane commit tracking, and caused kms_atomic_transitions
> to randomly fail with -EBUSY.
> 
> Changes since v1:
> - Prefer new_crtc_state->crtc above old_crtc_state->crtc.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
> Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102671
> Testcase: kms_atomic_transitions

Do we need to make this testcase more effective at hitting this?
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d59441f1dcd4..d0c2b266289e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1804,7 +1804,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
>  			return -EBUSY;
>  
> -		commit = crtc_or_fake_commit(state, old_plane_state->crtc);
> +		commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);

Maybe line-break and don't use the ?: gcc-ism. Either way

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  		if (!commit)
>  			return -ENOMEM;
>  
> -- 
> 2.14.1
>
Maarten Lankhorst Oct. 17, 2017, 12:35 p.m. UTC | #2
Op 17-10-17 om 14:11 schreef Daniel Vetter:
> On Tue, Oct 17, 2017 at 07:20:47AM +0200, Maarten Lankhorst wrote:
>> Commit 669c9215afea ("drm/atomic: Make async plane update checks work as
>> intended, v2.") forced planes to always be tracked, but forgot to
>> explicitly get the crtc commit from the new crtc when available.
>>
>> This broke plane commit tracking, and caused kms_atomic_transitions
>> to randomly fail with -EBUSY.
>>
>> Changes since v1:
>> - Prefer new_crtc_state->crtc above old_crtc_state->crtc.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
>> Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102671
>> Testcase: kms_atomic_transitions
> Do we need to make this testcase more effective at hitting this?
Not necessarily, the test already runs through all plane combinations and some of the transitions will likely hit it..

Test is still broken until IGT is fixed.
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index d59441f1dcd4..d0c2b266289e 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1804,7 +1804,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>>  		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
>>  			return -EBUSY;
>>  
>> -		commit = crtc_or_fake_commit(state, old_plane_state->crtc);
>> +		commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);
> Maybe line-break and don't use the ?: gcc-ism. Either way
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I like gcc-isms when it makes things easier. We use it in other places inside drm, and i915 anyway. :)

Thanks,
Pushed.
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d59441f1dcd4..d0c2b266289e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1804,7 +1804,7 @@  int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
 			return -EBUSY;
 
-		commit = crtc_or_fake_commit(state, old_plane_state->crtc);
+		commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);
 		if (!commit)
 			return -ENOMEM;