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

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

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?

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;

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.