[PATCH 1/2] drm/atomic-helper: Complete fake_commit->flip_done potentially earlier

[PATCH 1/2] drm/atomic-helper: Complete fake_commit->flip_done potentially earlier

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Consider the following scenario:
1. nonblocking enable crtc
2. wait for the event
3. nonblocking disable crtc

On i915 this can lead to a spurious -EBUSY from step 3 on
account of non-enabled planes getting the fake_commit in step 1
and we don't complete the fake_commit-> flip_done until
drm_atomic_helper_commit_hw_done() which can happen a long
time after the flip event was sent out.

This will become somewhat easy to hit on SKL+ once we start
to add all the planes for the crtc to every modeset commit
for the purposes of forcing a watermark register programming
[1].

To make the race a little less pronounced let's complete
fake_commit->flip_done after drm_atomic_helper_wait_for_flip_done().
For the single crtc case this should make the race quite
theoretical, assuming drm_atomic_helper_wait_for_flip_done()
actually has to wait for the real commit flip_done. In case
the real commit flip_done gets completed singificantly before
drm_atomic_helper_wait_for_flip_done(), or we are dealing with
multiple crtcs whose vblanks don't line up nicely the race still
exists.

[1] https://patchwork.freedesktop.org/patch/262670/

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 080de2e5be2d ("drm/atomic: Check for busy planes/connectors before setting the commit")
Testcase: igt/kms_cursor_legacy/*nonblocking-modeset-vs-cursor-atomic
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fa95f9974f6d..47398662b895 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1460,6 +1460,9 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
 			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
 				  crtc->base.id, crtc->name);
 	}
+
+	if (old_state->fake_commit)
+		complete_all(&old_state->fake_commit->flip_done);
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done);
 
-- 
2.18.1

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

[PATCH 2/2] drm/atomic-helper: WARN if fake_commit->hw_done is not completed as expected

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

For real commits we WARN if ->hw_done hasn't been completed by the time
drm_atomic_helper_commit_cleanup_done() is called. Let's do the same for
the fake commit.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 47398662b895..bc9fc9665614 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2220,8 +2220,10 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
 		spin_unlock(&crtc->commit_lock);
 	}
 
-	if (old_state->fake_commit)
+	if (old_state->fake_commit) {
 		complete_all(&old_state->fake_commit->cleanup_done);
+		WARN_ON(!try_wait_for_completion(&old_state->fake_commit->hw_done));
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
 
-- 
2.18.1

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

Re: [PATCH 1/2] drm/atomic-helper: Complete fake_commit->flip_done potentially earlier

[Maarten Lankhorst]

Op 22-11-18 om 15:34 schreef Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Consider the following scenario:
> 1. nonblocking enable crtc
> 2. wait for the event
> 3. nonblocking disable crtc
>
> On i915 this can lead to a spurious -EBUSY from step 3 on
> account of non-enabled planes getting the fake_commit in step 1
> and we don't complete the fake_commit-> flip_done until
> drm_atomic_helper_commit_hw_done() which can happen a long
> time after the flip event was sent out.
>
> This will become somewhat easy to hit on SKL+ once we start
> to add all the planes for the crtc to every modeset commit
> for the purposes of forcing a watermark register programming
> [1].
>
> To make the race a little less pronounced let's complete
> fake_commit->flip_done after drm_atomic_helper_wait_for_flip_done().
> For the single crtc case this should make the race quite
> theoretical, assuming drm_atomic_helper_wait_for_flip_done()
> actually has to wait for the real commit flip_done. In case
> the real commit flip_done gets completed singificantly before
> drm_atomic_helper_wait_for_flip_done(), or we are dealing with
> multiple crtcs whose vblanks don't line up nicely the race still
> exists.
>
> [1] https://patchwork.freedesktop.org/patch/262670/
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 080de2e5be2d ("drm/atomic: Check for busy planes/connectors before setting the commit")
> Testcase: igt/kms_cursor_legacy/*nonblocking-modeset-vs-cursor-atomic
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fa95f9974f6d..47398662b895 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1460,6 +1460,9 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>  			DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
>  				  crtc->base.id, crtc->name);
>  	}
> +
> +	if (old_state->fake_commit)
> +		complete_all(&old_state->fake_commit->flip_done);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done);
>  

Yeah as long as we don't enable hw_done sooner, this should be fine.

For both patches:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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