Patchwork [7/9] drm/i915: adjust has_pending_fb_unpin to atomic

login
register
mail settings
Submitter Daniel Vetter
Date July 19, 2017, 12:55 p.m.
Message ID <20170719125502.25696-8-daniel.vetter@ffwll.ch>
Download mbox | patch
Permalink /patch/9851969/
State New
Headers show

Comments

Daniel Vetter - July 19, 2017, 12:55 p.m.
A bit an oversight - the current code did nothing, since only
legacy flips used the unpin_work_count and assorted logic.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)
Chris Wilson - July 19, 2017, 1:06 p.m.
Quoting Daniel Vetter (2017-07-19 13:55:00)
> A bit an oversight - the current code did nothing, since only
> legacy flips used the unpin_work_count and assorted logic.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

There's a fence deadlock testcase for this, kms_flip/fence?
-Chris
Daniel Vetter - July 19, 2017, 1:15 p.m.
On Wed, Jul 19, 2017 at 02:06:43PM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-07-19 13:55:00)
> > A bit an oversight - the current code did nothing, since only
> > legacy flips used the unpin_work_count and assorted logic.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> There's a fence deadlock testcase for this, kms_flip/fence?

Crunching through them, but since I tend to test my stuff all mixed into
one pile I've hit a bug in a different patch series of mine. Given that
we've run without this for a while, not sure it's that critical really.
-Daniel
Chris Wilson - July 19, 2017, 2:08 p.m.
Quoting Daniel Vetter (2017-07-19 14:15:44)
> On Wed, Jul 19, 2017 at 02:06:43PM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2017-07-19 13:55:00)
> > > A bit an oversight - the current code did nothing, since only
> > > legacy flips used the unpin_work_count and assorted logic.
> > > 
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > There's a fence deadlock testcase for this, kms_flip/fence?
> 
> Crunching through them, but since I tend to test my stuff all mixed into
> one pile I've hit a bug in a different patch series of mine. Given that
> we've run without this for a while, not sure it's that critical really.

It's only going to affect gen2 (realistically using 14 fences in a gen3
batch is unlikely) if we can have 3 fb in the pipeline as userspace
assumes 2 are reserved for the flip. Or at least if we may race while fb
holds 3 fences due to the wq.

EDEADLK is not something I've seen outside of buggy code, but it is
certainly possible and this patch should fix it.
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 02620f31374b..343883214113 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4140,21 +4140,22 @@  static void ironlake_fdi_disable(struct drm_crtc *crtc)
 
 bool intel_has_pending_fb_unpin(struct drm_i915_private *dev_priv)
 {
-	struct intel_crtc *crtc;
-
-	/* Note that we don't need to be called with mode_config.lock here
-	 * as our list of CRTC objects is static for the lifetime of the
-	 * device and so cannot disappear as we iterate. Similarly, we can
-	 * happily treat the predicates as racy, atomic checks as userspace
-	 * cannot claim and pin a new fb without at least acquring the
-	 * struct_mutex and so serialising with us.
-	 */
-	for_each_intel_crtc(&dev_priv->drm, crtc) {
-		if (atomic_read(&crtc->unpin_work_count) == 0)
+	struct drm_crtc *crtc;
+	bool cleanup_done;
+
+	drm_for_each_crtc(crtc, &dev_priv->drm) {
+		struct drm_crtc_commit *commit;
+		spin_lock(&crtc->commit_lock);
+		commit = list_first_entry_or_null(&crtc->commit_list,
+						  struct drm_crtc_commit, commit_entry);
+		cleanup_done = commit ?
+			try_wait_for_completion(&commit->cleanup_done) : true;
+		spin_unlock(&crtc->commit_lock);
+
+		if (cleanup_done)
 			continue;
 
-		if (crtc->flip_work)
-			intel_wait_for_vblank(dev_priv, crtc->pipe);
+		drm_crtc_wait_one_vblank(crtc);
 
 		return true;
 	}