[PATCH 1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[PATCH 1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[Maarten Lankhorst]

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.

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d59441f1dcd4..b64c8f5bc940 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, old_plane_state->crtc ?: new_plane_state->crtc);
 		if (!commit)
 			return -ENOMEM;
 
-- 
2.14.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/atomic: Check for busy planes/connectors before setting the commit

[Maarten Lankhorst]

We still want to fail with -EBUSY if a plane or connector is part of
a commit, even if it will be assigned to a new commit.

This closes a small hole left open where we should return -EBUSY.
It's not severe, because wait_for_dependencies and swap_state helpers
still block. But it should return -EBUSY and not stall.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 21a01abbe32a ("drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.")
---
 drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b64c8f5bc940..c2a7887deb25 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1775,16 +1775,16 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 	}
 
 	for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
-		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
-		if (new_conn_state->crtc)
-			continue;
-
 		/* Userspace is not allowed to get ahead of the previous
 		 * commit with nonblocking ones. */
 		if (nonblock && old_conn_state->commit &&
 		    !try_wait_for_completion(&old_conn_state->commit->flip_done))
 			return -EBUSY;
 
+		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
+		if (new_conn_state->crtc)
+			continue;
+
 		commit = crtc_or_fake_commit(state, old_conn_state->crtc);
 		if (!commit)
 			return -ENOMEM;
@@ -1793,17 +1793,16 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 	}
 
 	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
-		/*
-		 * Unlike connectors, always track planes explicitly for
-		 * async pageflip support.
-		 */
-
 		/* Userspace is not allowed to get ahead of the previous
 		 * commit with nonblocking ones. */
 		if (nonblock && old_plane_state->commit &&
 		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
 			return -EBUSY;
 
+		/*
+		 * Unlike connectors, always track planes explicitly for
+		 * async pageflip support.
+		 */
 		commit = crtc_or_fake_commit(state, old_plane_state->crtc ?: new_plane_state->crtc);
 		if (!commit)
 			return -ENOMEM;
-- 
2.14.1

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

Re: [PATCH 2/2] drm/atomic: Check for busy planes/connectors before setting the commit

[Ville Syrjälä]

On Mon, Oct 16, 2017 at 03:29:28PM +0200, Maarten Lankhorst wrote:
> We still want to fail with -EBUSY if a plane or connector is part of
> a commit, even if it will be assigned to a new commit.
> 
> This closes a small hole left open where we should return -EBUSY.
> It's not severe, because wait_for_dependencies and swap_state helpers
> still block. But it should return -EBUSY and not stall.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 21a01abbe32a ("drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.")

Looks sane to me
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index b64c8f5bc940..c2a7887deb25 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1775,16 +1775,16 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  	}
>  
>  	for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
> -		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> -		if (new_conn_state->crtc)
> -			continue;
> -
>  		/* Userspace is not allowed to get ahead of the previous
>  		 * commit with nonblocking ones. */
>  		if (nonblock && old_conn_state->commit &&
>  		    !try_wait_for_completion(&old_conn_state->commit->flip_done))
>  			return -EBUSY;
>  
> +		/* commit tracked through new_crtc_state->commit, no need to do it explicitly */
> +		if (new_conn_state->crtc)
> +			continue;
> +
>  		commit = crtc_or_fake_commit(state, old_conn_state->crtc);
>  		if (!commit)
>  			return -ENOMEM;
> @@ -1793,17 +1793,16 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>  	}
>  
>  	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> -		/*
> -		 * Unlike connectors, always track planes explicitly for
> -		 * async pageflip support.
> -		 */
> -
>  		/* Userspace is not allowed to get ahead of the previous
>  		 * commit with nonblocking ones. */
>  		if (nonblock && old_plane_state->commit &&
>  		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
>  			return -EBUSY;
>  
> +		/*
> +		 * Unlike connectors, always track planes explicitly for
> +		 * async pageflip support.
> +		 */
>  		commit = crtc_or_fake_commit(state, old_plane_state->crtc ?: new_plane_state->crtc);
>  		if (!commit)
>  			return -ENOMEM;
> -- 
> 2.14.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[Ville Syrjälä]

On Mon, Oct 16, 2017 at 03:29:27PM +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.
> 
> 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(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d59441f1dcd4..b64c8f5bc940 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, old_plane_state->crtc ?: new_plane_state->crtc);

Shouldn't old vs. new state be the other way around?

>  		if (!commit)
>  			return -ENOMEM;
>  
> -- 
> 2.14.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[Maarten Lankhorst]

Op 16-10-17 om 15:42 schreef Ville Syrjälä:
> On Mon, Oct 16, 2017 at 03:29:27PM +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.
>>
>> 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(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index d59441f1dcd4..b64c8f5bc940 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, old_plane_state->crtc ?: new_plane_state->crtc);
> Shouldn't old vs. new state be the other way around?
Hmm to be honest, could be. We don't allow crtc's to switch planes directly. So in practice it doesn't matter.
But if we ever did allow moving crtc's, it's up for debate what crtc we want to use here..

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[Ville Syrjälä]

On Mon, Oct 16, 2017 at 03:59:38PM +0200, Maarten Lankhorst wrote:
> Op 16-10-17 om 15:42 schreef Ville Syrjälä:
> > On Mon, Oct 16, 2017 at 03:29:27PM +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.
> >>
> >> 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(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index d59441f1dcd4..b64c8f5bc940 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, old_plane_state->crtc ?: new_plane_state->crtc);
> > Shouldn't old vs. new state be the other way around?
> Hmm to be honest, could be. We don't allow crtc's to switch planes directly. So in practice it doesn't matter.

Not sure where we actually prevent that. A quick trawl through the code
didn't reveal anything like that.

> But if we ever did allow moving crtc's, it's up for debate what crtc we want to use here..

new is the one it'd be hanging off at the end so that seems like the
right choice. It would also match what we do in i915 code.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[Maarten Lankhorst]

Op 16-10-17 om 16:48 schreef Ville Syrjälä:
> On Mon, Oct 16, 2017 at 03:59:38PM +0200, Maarten Lankhorst wrote:
>> Op 16-10-17 om 15:42 schreef Ville Syrjälä:
>>> On Mon, Oct 16, 2017 at 03:29:27PM +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.
>>>>
>>>> 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(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index d59441f1dcd4..b64c8f5bc940 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, old_plane_state->crtc ?: new_plane_state->crtc);
>>> Shouldn't old vs. new state be the other way around?
>> Hmm to be honest, could be. We don't allow crtc's to switch planes directly. So in practice it doesn't matter.
> Not sure where we actually prevent that. A quick trawl through the code
> didn't reveal anything like that.
plane_switching_crtc(), called from drm_atomic_check_only->drm_atomic_plane_check().

So I wouldn't worry. :)
>> But if we ever did allow moving crtc's, it's up for debate what crtc we want to use here..
> new is the one it'd be hanging off at the end so that seems like the
> right choice. It would also match what we do in i915 code.
Ok new one it is, with that changed do I have your r-b?


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

Re: [PATCH 1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[Ville Syrjälä]

On Mon, Oct 16, 2017 at 05:28:27PM +0200, Maarten Lankhorst wrote:
> Op 16-10-17 om 16:48 schreef Ville Syrjälä:
> > On Mon, Oct 16, 2017 at 03:59:38PM +0200, Maarten Lankhorst wrote:
> >> Op 16-10-17 om 15:42 schreef Ville Syrjälä:
> >>> On Mon, Oct 16, 2017 at 03:29:27PM +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.
> >>>>
> >>>> 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(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> index d59441f1dcd4..b64c8f5bc940 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, old_plane_state->crtc ?: new_plane_state->crtc);
> >>> Shouldn't old vs. new state be the other way around?
> >> Hmm to be honest, could be. We don't allow crtc's to switch planes directly. So in practice it doesn't matter.
> > Not sure where we actually prevent that. A quick trawl through the code
> > didn't reveal anything like that.
> plane_switching_crtc(), called from drm_atomic_check_only->drm_atomic_plane_check().

Ah, that's where it was hiding.

> 
> So I wouldn't worry. :)
> >> But if we ever did allow moving crtc's, it's up for debate what crtc we want to use here..
> > new is the one it'd be hanging off at the end so that seems like the
> > right choice. It would also match what we do in i915 code.
> Ok new one it is, with that changed do I have your r-b?
> 

Yes.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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

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

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

Re: [PATCH] drm/atomic: Make atomic helper track newly assigned planes correctly, v2.

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Make atomic helper track newly assigned planes correctly, v2.

[Maarten Lankhorst]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.
URL   : https://patchwork.freedesktop.org/series/32044/
State : success

== Summary ==

Series 32044v1 series starting with [1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.
https://patchwork.freedesktop.org/api/1.0/series/32044/revisions/1/mbox/

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:457s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:471s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:388s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:578s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:285s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:521s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:526s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:542s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:512s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:558s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:432s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:272s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:602s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:443s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:466s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:503s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:478s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:504s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:584s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:492s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:595s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:465s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:662s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:528s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:519s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:472s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:583s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:429s
fi-pnv-d510 failed to connect after reboot

d6b500e96a1b961f4b74ea2b70e1f2f8ce59e1d6 drm-tip: 2017y-10m-16d-13h-20m-58s UTC integration manifest
db35c16706c1 drm/atomic: Check for busy planes/connectors before setting the commit
d686e566e4a0 drm/atomic: Make atomic helper track newly assigned planes correctly.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6054/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for series starting with [1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.
URL   : https://patchwork.freedesktop.org/series/32044/
State : warning

== Summary ==

Test kms_universal_plane:
        Subgroup universal-plane-pipe-B-functional:
                pass       -> SKIP       (shard-hsw)
Test kms_flip:
        Subgroup dpms-vs-vblank-race-interruptible:
                fail       -> PASS       (shard-hsw) fdo#103060
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (shard-hsw) fdo#102614

fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614

shard-hsw        total:2553 pass:1440 dwarn:1   dfail:0   fail:8   skip:1104 time:9612s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6054/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with drm/atomic: Make atomic helper track newly assigned planes correctly, v2. (rev2)

[Patchwork]

== Series Details ==

Series: series starting with drm/atomic: Make atomic helper track newly assigned planes correctly, v2. (rev2)
URL   : https://patchwork.freedesktop.org/series/32044/
State : success

== Summary ==

Series 32044v2 series starting with drm/atomic: Make atomic helper track newly assigned planes correctly, v2.
https://patchwork.freedesktop.org/api/1.0/series/32044/revisions/2/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#101705

fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:447s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:380s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:512s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:263s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:493s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:493s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:488s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:480s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:416s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:249s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:583s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:424s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:432s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:492s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:463s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:573s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:474s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:581s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:545s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:448s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:644s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:518s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:453s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:561s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:416s
fi-bdw-5557u failed to connect after reboot
fi-kbl-7500u failed to connect after reboot

ba1af442e4884a1148422a7f92ae2f978cfb26a1 drm-tip: 2017y-10m-17d-00h-18m-03s UTC integration manifest
32eb5e151733 drm/atomic: Make atomic helper track newly assigned planes correctly, v2.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6062/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with drm/atomic: Make atomic helper track newly assigned planes correctly, v2. (rev2)

[Patchwork]

== Series Details ==

Series: series starting with drm/atomic: Make atomic helper track newly assigned planes correctly, v2. (rev2)
URL   : https://patchwork.freedesktop.org/series/32044/
State : success

== Summary ==

Test kms_flip:
        Subgroup blt-wf_vblank-vs-dpms-interruptible:
                pass       -> DMESG-WARN (shard-hsw) fdo#102614
Test prime_self_import:
        Subgroup export-vs-gem_close-race:
                pass       -> FAIL       (shard-hsw) fdo#102655
Test gem_sync:
        Subgroup basic-many-each:
                fail       -> PASS       (shard-hsw) fdo#100007

fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#102655 https://bugs.freedesktop.org/show_bug.cgi?id=102655
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

shard-hsw        total:2553 pass:1438 dwarn:1   dfail:0   fail:11  skip:1103 time:9246s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6062/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx