[PATCH 1/2] drm/i915: Block enabling FBC until flips have been completed

[PATCH 1/2] drm/i915: Block enabling FBC until flips have been completed

[Maarten Lankhorst]

There is a small race window in which FBC can be enabled after
pre_plane_update is called, but before the page flip has been
queued or completed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++----------------------
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6f08ab310118..328d4312c438 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -512,6 +512,7 @@ struct intel_fbc {
 
 	bool enabled;
 	bool active;
+	bool flip_pending;
 
 	bool underrun_detected;
 	struct work_struct underrun_work;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b431b6733cc1..9f9ea0b5452f 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -924,13 +924,6 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc,
 						32 * fbc->threshold) * 8;
 }
 
-static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
-				       struct intel_fbc_reg_params *params2)
-{
-	/* We can use this since intel_fbc_get_reg_params() does a memset. */
-	return memcmp(params1, params2, sizeof(*params1)) == 0;
-}
-
 void intel_fbc_pre_update(struct intel_crtc *crtc,
 			  struct intel_crtc_state *crtc_state,
 			  struct intel_plane_state *plane_state)
@@ -953,6 +946,7 @@ void intel_fbc_pre_update(struct intel_crtc *crtc,
 		goto unlock;
 
 	intel_fbc_update_state_cache(crtc, crtc_state, plane_state);
+	fbc->flip_pending = true;
 
 deactivate:
 	intel_fbc_deactivate(dev_priv, reason);
@@ -988,13 +982,15 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct intel_fbc_reg_params old_params;
 
 	WARN_ON(!mutex_is_locked(&fbc->lock));
 
 	if (!fbc->enabled || fbc->crtc != crtc)
 		return;
 
+	fbc->flip_pending = false;
+	WARN_ON(fbc->active);
+
 	if (!i915_modparams.enable_fbc) {
 		intel_fbc_deactivate(dev_priv, "disabled at runtime per module param");
 		__intel_fbc_disable(dev_priv);
@@ -1002,25 +998,16 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
 		return;
 	}
 
-	if (!intel_fbc_can_activate(crtc)) {
-		WARN_ON(fbc->active);
-		return;
-	}
-
-	old_params = fbc->params;
 	intel_fbc_get_reg_params(crtc, &fbc->params);
 
-	/* If the scanout has not changed, don't modify the FBC settings.
-	 * Note that we make the fundamental assumption that the fb->obj
-	 * cannot be unpinned (and have its GTT offset and fence revoked)
-	 * without first being decoupled from the scanout and FBC disabled.
-	 */
-	if (fbc->active &&
-	    intel_fbc_reg_params_equal(&old_params, &fbc->params))
+	if (!intel_fbc_can_activate(crtc))
 		return;
 
-	intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)");
-	intel_fbc_schedule_activation(crtc);
+	if (!fbc->busy_bits) {
+		intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)");
+		intel_fbc_schedule_activation(crtc);
+	} else
+		intel_fbc_deactivate(dev_priv, "frontbuffer write");
 }
 
 void intel_fbc_post_update(struct intel_crtc *crtc)
@@ -1085,7 +1072,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 	    (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc))) {
 		if (fbc->active)
 			intel_fbc_recompress(dev_priv);
-		else
+		else if (!fbc->flip_pending)
 			__intel_fbc_post_update(fbc->crtc);
 	}
 
-- 
2.18.0

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

[PATCH 2/2] drm/i915: Remove delayed FBC activation.

[Maarten Lankhorst]

The only time we should start FBC is when we have waited a vblank
after the atomic update. We've already forced a vblank wait by doing
wait_for_flip_done before intel_post_plane_update(), so we don't need
to wait a second time before enabling.

Removing the worker simplifies the code and removes possible race
conditions, like happening in 103167.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  5 --
 drivers/gpu/drm/i915/i915_drv.h     |  6 --
 drivers/gpu/drm/i915/intel_fbc.c    | 96 +----------------------------
 3 files changed, 1 insertion(+), 106 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c400f42a54ec..48a57c0636bf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 	else
 		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
 
-	if (fbc->work.scheduled)
-		seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
-			   fbc->work.scheduled_vblank,
-			   drm_crtc_vblank_count(&fbc->crtc->base));
-
 	if (intel_fbc_is_active(dev_priv)) {
 		u32 mask;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 328d4312c438..9645dcb30454 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -580,12 +580,6 @@ struct intel_fbc {
 		unsigned int gen9_wa_cfb_stride;
 	} params;
 
-	struct intel_fbc_work {
-		bool scheduled;
-		u64 scheduled_vblank;
-		struct work_struct work;
-	} work;
-
 	const char *no_fbc_reason;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 9f9ea0b5452f..01d1d2088f04 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private *dev_priv)
 	return dev_priv->fbc.active;
 }
 
-static void intel_fbc_work_fn(struct work_struct *__work)
-{
-	struct drm_i915_private *dev_priv =
-		container_of(__work, struct drm_i915_private, fbc.work.work);
-	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct intel_fbc_work *work = &fbc->work;
-	struct intel_crtc *crtc = fbc->crtc;
-	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[crtc->pipe];
-
-	if (drm_crtc_vblank_get(&crtc->base)) {
-		/* CRTC is now off, leave FBC deactivated */
-		mutex_lock(&fbc->lock);
-		work->scheduled = false;
-		mutex_unlock(&fbc->lock);
-		return;
-	}
-
-retry:
-	/* Delay the actual enabling to let pageflipping cease and the
-	 * display to settle before starting the compression. Note that
-	 * this delay also serves a second purpose: it allows for a
-	 * vblank to pass after disabling the FBC before we attempt
-	 * to modify the control registers.
-	 *
-	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
-	 *
-	 * It is also worth mentioning that since work->scheduled_vblank can be
-	 * updated multiple times by the other threads, hitting the timeout is
-	 * not an error condition. We'll just end up hitting the "goto retry"
-	 * case below.
-	 */
-	wait_event_timeout(vblank->queue,
-		drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank,
-		msecs_to_jiffies(50));
-
-	mutex_lock(&fbc->lock);
-
-	/* Were we cancelled? */
-	if (!work->scheduled)
-		goto out;
-
-	/* Were we delayed again while this function was sleeping? */
-	if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
-		mutex_unlock(&fbc->lock);
-		goto retry;
-	}
-
-	intel_fbc_hw_activate(dev_priv);
-
-	work->scheduled = false;
-
-out:
-	mutex_unlock(&fbc->lock);
-	drm_crtc_vblank_put(&crtc->base);
-}
-
-static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	struct intel_fbc *fbc = &dev_priv->fbc;
-	struct intel_fbc_work *work = &fbc->work;
-
-	WARN_ON(!mutex_is_locked(&fbc->lock));
-	if (WARN_ON(!fbc->enabled))
-		return;
-
-	if (drm_crtc_vblank_get(&crtc->base)) {
-		DRM_ERROR("vblank not available for FBC on pipe %c\n",
-			  pipe_name(crtc->pipe));
-		return;
-	}
-
-	/* It is useless to call intel_fbc_cancel_work() or cancel_work() in
-	 * this function since we're not releasing fbc.lock, so it won't have an
-	 * opportunity to grab it to discover that it was cancelled. So we just
-	 * update the expected jiffy count. */
-	work->scheduled = true;
-	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
-	drm_crtc_vblank_put(&crtc->base);
-
-	schedule_work(&work->work);
-}
-
 static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
 				 const char *reason)
 {
@@ -489,11 +406,6 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
 
 	WARN_ON(!mutex_is_locked(&fbc->lock));
 
-	/* Calling cancel_work() here won't help due to the fact that the work
-	 * function grabs fbc->lock. Just set scheduled to false so the work
-	 * function can know it was cancelled. */
-	fbc->work.scheduled = false;
-
 	if (fbc->active)
 		intel_fbc_hw_deactivate(dev_priv);
 
@@ -1005,7 +917,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
 
 	if (!fbc->busy_bits) {
 		intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)");
-		intel_fbc_schedule_activation(crtc);
+		intel_fbc_hw_activate(dev_priv);
 	} else
 		intel_fbc_deactivate(dev_priv, "frontbuffer write");
 }
@@ -1212,8 +1124,6 @@ void intel_fbc_disable(struct intel_crtc *crtc)
 	if (fbc->crtc == crtc)
 		__intel_fbc_disable(dev_priv);
 	mutex_unlock(&fbc->lock);
-
-	cancel_work_sync(&fbc->work.work);
 }
 
 /**
@@ -1235,8 +1145,6 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
 		__intel_fbc_disable(dev_priv);
 	}
 	mutex_unlock(&fbc->lock);
-
-	cancel_work_sync(&fbc->work.work);
 }
 
 static void intel_fbc_underrun_work_fn(struct work_struct *work)
@@ -1387,12 +1295,10 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_fbc *fbc = &dev_priv->fbc;
 
-	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
 	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
 	mutex_init(&fbc->lock);
 	fbc->enabled = false;
 	fbc->active = false;
-	fbc->work.scheduled = false;
 
 	if (need_fbc_vtd_wa(dev_priv))
 		mkwrite_device_info(dev_priv)->has_fbc = false;
-- 
2.18.0

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Block enabling FBC until flips have been completed

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Block enabling FBC until flips have been completed
URL   : https://patchwork.freedesktop.org/series/45349/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
155e8eaf0d4b drm/i915: Block enabling FBC until flips have been completed
-:92: CHECK:BRACES: braces {} should be used on all arms of this statement
#92: FILE: drivers/gpu/drm/i915/intel_fbc.c:1006:
+	if (!fbc->busy_bits) {
[...]
+	} else
[...]

-:95: CHECK:BRACES: Unbalanced braces around else statement
#95: FILE: drivers/gpu/drm/i915/intel_fbc.c:1009:
+	} else

total: 0 errors, 0 warnings, 2 checks, 82 lines checked
ad2a933a32b8 drm/i915: Remove delayed FBC activation.

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

✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Block enabling FBC until flips have been completed

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Block enabling FBC until flips have been completed
URL   : https://patchwork.freedesktop.org/series/45349/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915: Block enabling FBC until flips have been completed
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3680:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3681:16: warning: expression using sizeof(void)

Commit: drm/i915: Remove delayed FBC activation.
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3681:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3675:16: warning: expression using sizeof(void)

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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Block enabling FBC until flips have been completed

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Block enabling FBC until flips have been completed
URL   : https://patchwork.freedesktop.org/series/45349/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4375 -> Patchwork_9414 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9414 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9414, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45349/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9414:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-skl-6600u:       PASS -> DMESG-WARN

    igt@gem_sync@basic-each:
      fi-bsw-n3050:       PASS -> DMESG-FAIL

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-c:
      fi-bsw-n3050:       PASS -> FAIL

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bsw-n3050:       PASS -> DMESG-WARN

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        SKIP -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-bsw-n3050:       PASS -> SKIP +12

    
== Known issues ==

  Here are the changes found in Patchwork_9414 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-skl-6700k2:      PASS -> INCOMPLETE (fdo#105524, k.org#199541, fdo#104108)

    
    ==== Possible fixes ====

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105524 https://bugs.freedesktop.org/show_bug.cgi?id=105524
  k.org#199541 https://bugzilla.kernel.org/show_bug.cgi?id=199541


== Participating hosts (44 -> 39) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4375 -> Patchwork_9414

  CI_DRM_4375: 24b1806d544318f5a365c1ef715dc467940af59c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9414: ad2a933a32b8e42d95a7cbc4c3366f0009024729 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ad2a933a32b8 drm/i915: Remove delayed FBC activation.
155e8eaf0d4b drm/i915: Block enabling FBC until flips have been completed

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9414/issues.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 [1/2] drm/i915: Block enabling FBC until flips have been completed

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Block enabling FBC until flips have been completed
URL   : https://patchwork.freedesktop.org/series/45349/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4380 -> Patchwork_9425 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45349/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9425 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@gem_basic@bad-close:
      fi-skl-6770hq:      DMESG-WARN (fdo#105541) -> PASS

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


== Participating hosts (44 -> 39) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4380 -> Patchwork_9425

  CI_DRM_4380: e69a5560b14d9c7377744267ffd9963f06734f38 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9425: 7a7381fc1a43aff6a2a1c930b51ba2bfde5ed153 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

7a7381fc1a43 drm/i915: Remove delayed FBC activation.
3f1f970c9d86 drm/i915: Block enabling FBC until flips have been completed

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9425/issues.html
_______________________________________________
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 [1/2] drm/i915: Block enabling FBC until flips have been completed

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Block enabling FBC until flips have been completed
URL   : https://patchwork.freedesktop.org/series/45349/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4380_full -> Patchwork_9425_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9425_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9425_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9425_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-vebox:
      shard-kbl:          PASS -> SKIP +1

    igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
      shard-snb:          PASS -> SKIP +4

    
== Known issues ==

  Here are the changes found in Patchwork_9425_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_gtt:
      shard-apl:          PASS -> FAIL (fdo#105347)

    igt@gem_exec_big:
      shard-hsw:          PASS -> INCOMPLETE (fdo#103540)

    igt@gem_softpin@noreloc-s3:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_flip@modeset-vs-vblank-race:
      shard-glk:          PASS -> FAIL (fdo#103060)

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          DMESG-FAIL (fdo#106947, fdo#106560) -> PASS

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          FAIL (fdo#105454, fdo#106509) -> PASS

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-glk:          FAIL (fdo#100368) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
      shard-snb:          FAIL (fdo#103167, fdo#104724) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-mmap-wc:
      shard-glk:          FAIL (fdo#103167, fdo#104724) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347
  fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
  fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4380 -> Patchwork_9425

  CI_DRM_4380: e69a5560b14d9c7377744267ffd9963f06734f38 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4530: 0e98bf69f146eb72fe3a7c3b19a049b5786f0ca3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9425: 7a7381fc1a43aff6a2a1c930b51ba2bfde5ed153 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH 2/2] drm/i915: Remove delayed FBC activation.

[Ville Syrjälä]

On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
> The only time we should start FBC is when we have waited a vblank
> after the atomic update.

What about front buffer tracking? Is this going to leave FBC permanently
disabled unless there are flips/plane updates?

I think there are a few cases we need to consider:
1. plane update which doesn't need fbc disable
2. plane update which needs to disable fbc but can re-enable it after the flip
   has happended (eg. need to reallocate the cfb due to plane size/format change)
3. plane update which needs to disable fbc and can't re-enable it (eg. the new
   plane state no longer fbc compatible)
4. front buffer invalidate + flush

HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
post-flip vblank wait. Case 3 would ideally let us move FBC to another
plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
after the flush has happened.

> We've already forced a vblank wait by doing
> wait_for_flip_done before intel_post_plane_update(), so we don't need
> to wait a second time before enabling.
> 
> Removing the worker simplifies the code and removes possible race
> conditions, like happening in 103167.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  5 --
>  drivers/gpu/drm/i915/i915_drv.h     |  6 --
>  drivers/gpu/drm/i915/intel_fbc.c    | 96 +----------------------------
>  3 files changed, 1 insertion(+), 106 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c400f42a54ec..48a57c0636bf 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>  	else
>  		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
>  
> -	if (fbc->work.scheduled)
> -		seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
> -			   fbc->work.scheduled_vblank,
> -			   drm_crtc_vblank_count(&fbc->crtc->base));
> -
>  	if (intel_fbc_is_active(dev_priv)) {
>  		u32 mask;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 328d4312c438..9645dcb30454 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -580,12 +580,6 @@ struct intel_fbc {
>  		unsigned int gen9_wa_cfb_stride;
>  	} params;
>  
> -	struct intel_fbc_work {
> -		bool scheduled;
> -		u64 scheduled_vblank;
> -		struct work_struct work;
> -	} work;
> -
>  	const char *no_fbc_reason;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 9f9ea0b5452f..01d1d2088f04 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private *dev_priv)
>  	return dev_priv->fbc.active;
>  }
>  
> -static void intel_fbc_work_fn(struct work_struct *__work)
> -{
> -	struct drm_i915_private *dev_priv =
> -		container_of(__work, struct drm_i915_private, fbc.work.work);
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct intel_fbc_work *work = &fbc->work;
> -	struct intel_crtc *crtc = fbc->crtc;
> -	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[crtc->pipe];
> -
> -	if (drm_crtc_vblank_get(&crtc->base)) {
> -		/* CRTC is now off, leave FBC deactivated */
> -		mutex_lock(&fbc->lock);
> -		work->scheduled = false;
> -		mutex_unlock(&fbc->lock);
> -		return;
> -	}
> -
> -retry:
> -	/* Delay the actual enabling to let pageflipping cease and the
> -	 * display to settle before starting the compression. Note that
> -	 * this delay also serves a second purpose: it allows for a
> -	 * vblank to pass after disabling the FBC before we attempt
> -	 * to modify the control registers.
> -	 *
> -	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> -	 *
> -	 * It is also worth mentioning that since work->scheduled_vblank can be
> -	 * updated multiple times by the other threads, hitting the timeout is
> -	 * not an error condition. We'll just end up hitting the "goto retry"
> -	 * case below.
> -	 */
> -	wait_event_timeout(vblank->queue,
> -		drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank,
> -		msecs_to_jiffies(50));
> -
> -	mutex_lock(&fbc->lock);
> -
> -	/* Were we cancelled? */
> -	if (!work->scheduled)
> -		goto out;
> -
> -	/* Were we delayed again while this function was sleeping? */
> -	if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
> -		mutex_unlock(&fbc->lock);
> -		goto retry;
> -	}
> -
> -	intel_fbc_hw_activate(dev_priv);
> -
> -	work->scheduled = false;
> -
> -out:
> -	mutex_unlock(&fbc->lock);
> -	drm_crtc_vblank_put(&crtc->base);
> -}
> -
> -static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct intel_fbc_work *work = &fbc->work;
> -
> -	WARN_ON(!mutex_is_locked(&fbc->lock));
> -	if (WARN_ON(!fbc->enabled))
> -		return;
> -
> -	if (drm_crtc_vblank_get(&crtc->base)) {
> -		DRM_ERROR("vblank not available for FBC on pipe %c\n",
> -			  pipe_name(crtc->pipe));
> -		return;
> -	}
> -
> -	/* It is useless to call intel_fbc_cancel_work() or cancel_work() in
> -	 * this function since we're not releasing fbc.lock, so it won't have an
> -	 * opportunity to grab it to discover that it was cancelled. So we just
> -	 * update the expected jiffy count. */
> -	work->scheduled = true;
> -	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
> -	drm_crtc_vblank_put(&crtc->base);
> -
> -	schedule_work(&work->work);
> -}
> -
>  static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
>  				 const char *reason)
>  {
> @@ -489,11 +406,6 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
>  
>  	WARN_ON(!mutex_is_locked(&fbc->lock));
>  
> -	/* Calling cancel_work() here won't help due to the fact that the work
> -	 * function grabs fbc->lock. Just set scheduled to false so the work
> -	 * function can know it was cancelled. */
> -	fbc->work.scheduled = false;
> -
>  	if (fbc->active)
>  		intel_fbc_hw_deactivate(dev_priv);
>  
> @@ -1005,7 +917,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
>  
>  	if (!fbc->busy_bits) {
>  		intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)");
> -		intel_fbc_schedule_activation(crtc);
> +		intel_fbc_hw_activate(dev_priv);
>  	} else
>  		intel_fbc_deactivate(dev_priv, "frontbuffer write");
>  }
> @@ -1212,8 +1124,6 @@ void intel_fbc_disable(struct intel_crtc *crtc)
>  	if (fbc->crtc == crtc)
>  		__intel_fbc_disable(dev_priv);
>  	mutex_unlock(&fbc->lock);
> -
> -	cancel_work_sync(&fbc->work.work);
>  }
>  
>  /**
> @@ -1235,8 +1145,6 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
>  		__intel_fbc_disable(dev_priv);
>  	}
>  	mutex_unlock(&fbc->lock);
> -
> -	cancel_work_sync(&fbc->work.work);
>  }
>  
>  static void intel_fbc_underrun_work_fn(struct work_struct *work)
> @@ -1387,12 +1295,10 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_fbc *fbc = &dev_priv->fbc;
>  
> -	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
>  	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
>  	mutex_init(&fbc->lock);
>  	fbc->enabled = false;
>  	fbc->active = false;
> -	fbc->work.scheduled = false;
>  
>  	if (need_fbc_vtd_wa(dev_priv))
>  		mkwrite_device_info(dev_priv)->has_fbc = false;
> -- 
> 2.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/2] drm/i915: Remove delayed FBC activation.

[Maarten Lankhorst]

Op 26-06-18 om 19:59 schreef Ville Syrjälä:
> On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
>> The only time we should start FBC is when we have waited a vblank
>> after the atomic update.
> What about front buffer tracking? Is this going to leave FBC permanently
> disabled unless there are flips/plane updates?
No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page flip,
we will not enable FBC in intel_fbc_flush(), but in that case we would enable it from
intel_fbc_post_update().

Or the other way around, intel_fbc_post_update won't enable FBC if the fb is dirty, but
intel_fbc_flush() afterwards will.
> I think there are a few cases we need to consider:
> 1. plane update which doesn't need fbc disable
> 2. plane update which needs to disable fbc but can re-enable it after the flip
>    has happended (eg. need to reallocate the cfb due to plane size/format change)
> 3. plane update which needs to disable fbc and can't re-enable it (eg. the new
>    plane state no longer fbc compatible)
> 4. front buffer invalidate + flush
>
> HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
> post-flip vblank wait. Case 3 would ideally let us move FBC to another
> plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
> after the flush has happened.
I don't see how this code will break any case. :)

~Maarten

>> We've already forced a vblank wait by doing
>> wait_for_flip_done before intel_post_plane_update(), so we don't need
>> to wait a second time before enabling.
>>
>> Removing the worker simplifies the code and removes possible race
>> conditions, like happening in 103167.
>>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c |  5 --
>>  drivers/gpu/drm/i915/i915_drv.h     |  6 --
>>  drivers/gpu/drm/i915/intel_fbc.c    | 96 +----------------------------
>>  3 files changed, 1 insertion(+), 106 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index c400f42a54ec..48a57c0636bf 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>>  	else
>>  		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
>>  
>> -	if (fbc->work.scheduled)
>> -		seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
>> -			   fbc->work.scheduled_vblank,
>> -			   drm_crtc_vblank_count(&fbc->crtc->base));
>> -
>>  	if (intel_fbc_is_active(dev_priv)) {
>>  		u32 mask;
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 328d4312c438..9645dcb30454 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -580,12 +580,6 @@ struct intel_fbc {
>>  		unsigned int gen9_wa_cfb_stride;
>>  	} params;
>>  
>> -	struct intel_fbc_work {
>> -		bool scheduled;
>> -		u64 scheduled_vblank;
>> -		struct work_struct work;
>> -	} work;
>> -
>>  	const char *no_fbc_reason;
>>  };
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index 9f9ea0b5452f..01d1d2088f04 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private *dev_priv)
>>  	return dev_priv->fbc.active;
>>  }
>>  
>> -static void intel_fbc_work_fn(struct work_struct *__work)
>> -{
>> -	struct drm_i915_private *dev_priv =
>> -		container_of(__work, struct drm_i915_private, fbc.work.work);
>> -	struct intel_fbc *fbc = &dev_priv->fbc;
>> -	struct intel_fbc_work *work = &fbc->work;
>> -	struct intel_crtc *crtc = fbc->crtc;
>> -	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[crtc->pipe];
>> -
>> -	if (drm_crtc_vblank_get(&crtc->base)) {
>> -		/* CRTC is now off, leave FBC deactivated */
>> -		mutex_lock(&fbc->lock);
>> -		work->scheduled = false;
>> -		mutex_unlock(&fbc->lock);
>> -		return;
>> -	}
>> -
>> -retry:
>> -	/* Delay the actual enabling to let pageflipping cease and the
>> -	 * display to settle before starting the compression. Note that
>> -	 * this delay also serves a second purpose: it allows for a
>> -	 * vblank to pass after disabling the FBC before we attempt
>> -	 * to modify the control registers.
>> -	 *
>> -	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
>> -	 *
>> -	 * It is also worth mentioning that since work->scheduled_vblank can be
>> -	 * updated multiple times by the other threads, hitting the timeout is
>> -	 * not an error condition. We'll just end up hitting the "goto retry"
>> -	 * case below.
>> -	 */
>> -	wait_event_timeout(vblank->queue,
>> -		drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank,
>> -		msecs_to_jiffies(50));
>> -
>> -	mutex_lock(&fbc->lock);
>> -
>> -	/* Were we cancelled? */
>> -	if (!work->scheduled)
>> -		goto out;
>> -
>> -	/* Were we delayed again while this function was sleeping? */
>> -	if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
>> -		mutex_unlock(&fbc->lock);
>> -		goto retry;
>> -	}
>> -
>> -	intel_fbc_hw_activate(dev_priv);
>> -
>> -	work->scheduled = false;
>> -
>> -out:
>> -	mutex_unlock(&fbc->lock);
>> -	drm_crtc_vblank_put(&crtc->base);
>> -}
>> -
>> -static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
>> -{
>> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> -	struct intel_fbc *fbc = &dev_priv->fbc;
>> -	struct intel_fbc_work *work = &fbc->work;
>> -
>> -	WARN_ON(!mutex_is_locked(&fbc->lock));
>> -	if (WARN_ON(!fbc->enabled))
>> -		return;
>> -
>> -	if (drm_crtc_vblank_get(&crtc->base)) {
>> -		DRM_ERROR("vblank not available for FBC on pipe %c\n",
>> -			  pipe_name(crtc->pipe));
>> -		return;
>> -	}
>> -
>> -	/* It is useless to call intel_fbc_cancel_work() or cancel_work() in
>> -	 * this function since we're not releasing fbc.lock, so it won't have an
>> -	 * opportunity to grab it to discover that it was cancelled. So we just
>> -	 * update the expected jiffy count. */
>> -	work->scheduled = true;
>> -	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
>> -	drm_crtc_vblank_put(&crtc->base);
>> -
>> -	schedule_work(&work->work);
>> -}
>> -
>>  static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
>>  				 const char *reason)
>>  {
>> @@ -489,11 +406,6 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
>>  
>>  	WARN_ON(!mutex_is_locked(&fbc->lock));
>>  
>> -	/* Calling cancel_work() here won't help due to the fact that the work
>> -	 * function grabs fbc->lock. Just set scheduled to false so the work
>> -	 * function can know it was cancelled. */
>> -	fbc->work.scheduled = false;
>> -
>>  	if (fbc->active)
>>  		intel_fbc_hw_deactivate(dev_priv);
>>  
>> @@ -1005,7 +917,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
>>  
>>  	if (!fbc->busy_bits) {
>>  		intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)");
>> -		intel_fbc_schedule_activation(crtc);
>> +		intel_fbc_hw_activate(dev_priv);
>>  	} else
>>  		intel_fbc_deactivate(dev_priv, "frontbuffer write");
>>  }
>> @@ -1212,8 +1124,6 @@ void intel_fbc_disable(struct intel_crtc *crtc)
>>  	if (fbc->crtc == crtc)
>>  		__intel_fbc_disable(dev_priv);
>>  	mutex_unlock(&fbc->lock);
>> -
>> -	cancel_work_sync(&fbc->work.work);
>>  }
>>  
>>  /**
>> @@ -1235,8 +1145,6 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
>>  		__intel_fbc_disable(dev_priv);
>>  	}
>>  	mutex_unlock(&fbc->lock);
>> -
>> -	cancel_work_sync(&fbc->work.work);
>>  }
>>  
>>  static void intel_fbc_underrun_work_fn(struct work_struct *work)
>> @@ -1387,12 +1295,10 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>>  {
>>  	struct intel_fbc *fbc = &dev_priv->fbc;
>>  
>> -	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
>>  	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
>>  	mutex_init(&fbc->lock);
>>  	fbc->enabled = false;
>>  	fbc->active = false;
>> -	fbc->work.scheduled = false;
>>  
>>  	if (need_fbc_vtd_wa(dev_priv))
>>  		mkwrite_device_info(dev_priv)->has_fbc = false;
>> -- 
>> 2.18.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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

Re: [PATCH 2/2] drm/i915: Remove delayed FBC activation.

[Ville Syrjälä]

On Wed, Jun 27, 2018 at 01:45:04PM +0200, Maarten Lankhorst wrote:
> Op 26-06-18 om 19:59 schreef Ville Syrjälä:
> > On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
> >> The only time we should start FBC is when we have waited a vblank
> >> after the atomic update.
> > What about front buffer tracking? Is this going to leave FBC permanently
> > disabled unless there are flips/plane updates?
> No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page flip,
> we will not enable FBC in intel_fbc_flush(), but in that case we would enable it from
> intel_fbc_post_update().

I guess what I haven't quite figured out is why the pre_plane_update()
even leaves fbc logically enabled. I would think we should just disable
fbc there if anything important is going to change, and then re-enable
at post_plane_update() after reallocating the cfb.

> 
> Or the other way around, intel_fbc_post_update won't enable FBC if the fb is dirty, but
> intel_fbc_flush() afterwards will.
> > I think there are a few cases we need to consider:
> > 1. plane update which doesn't need fbc disable
> > 2. plane update which needs to disable fbc but can re-enable it after the flip
> >    has happended (eg. need to reallocate the cfb due to plane size/format change)
> > 3. plane update which needs to disable fbc and can't re-enable it (eg. the new
> >    plane state no longer fbc compatible)
> > 4. front buffer invalidate + flush
> >
> > HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
> > post-flip vblank wait. Case 3 would ideally let us move FBC to another
> > plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
> > after the flush has happened.
> I don't see how this code will break any case. :)

OK, so I guess you just remove the delayed activation at
flush/post_plane_update. So now it'll enable it immediately.

Hmm, and if we just get a flush without the invalidate then we're going
to just use the nuke msg register anyway. So I suppose this shouldn't
cause much additional fbc on<->off ping pong.

Actually, are we now effecitively forcing a synchronous vblank wait
in pre_plane_update for every frame via the hw_deactivate() polling
for fbc to stop? AFAICS with the re-enable now being immediate we're
going to have to disable fbc every single time. I think the correct
fix for that would involve a) not disabling fbc around flips when the
hw flip nuke will suffice, and b) convert the "wait for fbc to disable"
thing into a post_plane_update time assert (and verify whether the hw
is actually happy with disabling both fbc and the plane at the same
time).

> 
> ~Maarten
> 
> >> We've already forced a vblank wait by doing
> >> wait_for_flip_done before intel_post_plane_update(), so we don't need
> >> to wait a second time before enabling.
> >>
> >> Removing the worker simplifies the code and removes possible race
> >> conditions, like happening in 103167.
> >>
> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_debugfs.c |  5 --
> >>  drivers/gpu/drm/i915/i915_drv.h     |  6 --
> >>  drivers/gpu/drm/i915/intel_fbc.c    | 96 +----------------------------
> >>  3 files changed, 1 insertion(+), 106 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index c400f42a54ec..48a57c0636bf 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -1659,11 +1659,6 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
> >>  	else
> >>  		seq_printf(m, "FBC disabled: %s\n", fbc->no_fbc_reason);
> >>  
> >> -	if (fbc->work.scheduled)
> >> -		seq_printf(m, "FBC worker scheduled on vblank %llu, now %llu\n",
> >> -			   fbc->work.scheduled_vblank,
> >> -			   drm_crtc_vblank_count(&fbc->crtc->base));
> >> -
> >>  	if (intel_fbc_is_active(dev_priv)) {
> >>  		u32 mask;
> >>  
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 328d4312c438..9645dcb30454 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -580,12 +580,6 @@ struct intel_fbc {
> >>  		unsigned int gen9_wa_cfb_stride;
> >>  	} params;
> >>  
> >> -	struct intel_fbc_work {
> >> -		bool scheduled;
> >> -		u64 scheduled_vblank;
> >> -		struct work_struct work;
> >> -	} work;
> >> -
> >>  	const char *no_fbc_reason;
> >>  };
> >>  
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index 9f9ea0b5452f..01d1d2088f04 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -399,89 +399,6 @@ bool intel_fbc_is_active(struct drm_i915_private *dev_priv)
> >>  	return dev_priv->fbc.active;
> >>  }
> >>  
> >> -static void intel_fbc_work_fn(struct work_struct *__work)
> >> -{
> >> -	struct drm_i915_private *dev_priv =
> >> -		container_of(__work, struct drm_i915_private, fbc.work.work);
> >> -	struct intel_fbc *fbc = &dev_priv->fbc;
> >> -	struct intel_fbc_work *work = &fbc->work;
> >> -	struct intel_crtc *crtc = fbc->crtc;
> >> -	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[crtc->pipe];
> >> -
> >> -	if (drm_crtc_vblank_get(&crtc->base)) {
> >> -		/* CRTC is now off, leave FBC deactivated */
> >> -		mutex_lock(&fbc->lock);
> >> -		work->scheduled = false;
> >> -		mutex_unlock(&fbc->lock);
> >> -		return;
> >> -	}
> >> -
> >> -retry:
> >> -	/* Delay the actual enabling to let pageflipping cease and the
> >> -	 * display to settle before starting the compression. Note that
> >> -	 * this delay also serves a second purpose: it allows for a
> >> -	 * vblank to pass after disabling the FBC before we attempt
> >> -	 * to modify the control registers.
> >> -	 *
> >> -	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> >> -	 *
> >> -	 * It is also worth mentioning that since work->scheduled_vblank can be
> >> -	 * updated multiple times by the other threads, hitting the timeout is
> >> -	 * not an error condition. We'll just end up hitting the "goto retry"
> >> -	 * case below.
> >> -	 */
> >> -	wait_event_timeout(vblank->queue,
> >> -		drm_crtc_vblank_count(&crtc->base) != work->scheduled_vblank,
> >> -		msecs_to_jiffies(50));
> >> -
> >> -	mutex_lock(&fbc->lock);
> >> -
> >> -	/* Were we cancelled? */
> >> -	if (!work->scheduled)
> >> -		goto out;
> >> -
> >> -	/* Were we delayed again while this function was sleeping? */
> >> -	if (drm_crtc_vblank_count(&crtc->base) == work->scheduled_vblank) {
> >> -		mutex_unlock(&fbc->lock);
> >> -		goto retry;
> >> -	}
> >> -
> >> -	intel_fbc_hw_activate(dev_priv);
> >> -
> >> -	work->scheduled = false;
> >> -
> >> -out:
> >> -	mutex_unlock(&fbc->lock);
> >> -	drm_crtc_vblank_put(&crtc->base);
> >> -}
> >> -
> >> -static void intel_fbc_schedule_activation(struct intel_crtc *crtc)
> >> -{
> >> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> -	struct intel_fbc *fbc = &dev_priv->fbc;
> >> -	struct intel_fbc_work *work = &fbc->work;
> >> -
> >> -	WARN_ON(!mutex_is_locked(&fbc->lock));
> >> -	if (WARN_ON(!fbc->enabled))
> >> -		return;
> >> -
> >> -	if (drm_crtc_vblank_get(&crtc->base)) {
> >> -		DRM_ERROR("vblank not available for FBC on pipe %c\n",
> >> -			  pipe_name(crtc->pipe));
> >> -		return;
> >> -	}
> >> -
> >> -	/* It is useless to call intel_fbc_cancel_work() or cancel_work() in
> >> -	 * this function since we're not releasing fbc.lock, so it won't have an
> >> -	 * opportunity to grab it to discover that it was cancelled. So we just
> >> -	 * update the expected jiffy count. */
> >> -	work->scheduled = true;
> >> -	work->scheduled_vblank = drm_crtc_vblank_count(&crtc->base);
> >> -	drm_crtc_vblank_put(&crtc->base);
> >> -
> >> -	schedule_work(&work->work);
> >> -}
> >> -
> >>  static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
> >>  				 const char *reason)
> >>  {
> >> @@ -489,11 +406,6 @@ static void intel_fbc_deactivate(struct drm_i915_private *dev_priv,
> >>  
> >>  	WARN_ON(!mutex_is_locked(&fbc->lock));
> >>  
> >> -	/* Calling cancel_work() here won't help due to the fact that the work
> >> -	 * function grabs fbc->lock. Just set scheduled to false so the work
> >> -	 * function can know it was cancelled. */
> >> -	fbc->work.scheduled = false;
> >> -
> >>  	if (fbc->active)
> >>  		intel_fbc_hw_deactivate(dev_priv);
> >>  
> >> @@ -1005,7 +917,7 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
> >>  
> >>  	if (!fbc->busy_bits) {
> >>  		intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)");
> >> -		intel_fbc_schedule_activation(crtc);
> >> +		intel_fbc_hw_activate(dev_priv);
> >>  	} else
> >>  		intel_fbc_deactivate(dev_priv, "frontbuffer write");
> >>  }
> >> @@ -1212,8 +1124,6 @@ void intel_fbc_disable(struct intel_crtc *crtc)
> >>  	if (fbc->crtc == crtc)
> >>  		__intel_fbc_disable(dev_priv);
> >>  	mutex_unlock(&fbc->lock);
> >> -
> >> -	cancel_work_sync(&fbc->work.work);
> >>  }
> >>  
> >>  /**
> >> @@ -1235,8 +1145,6 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
> >>  		__intel_fbc_disable(dev_priv);
> >>  	}
> >>  	mutex_unlock(&fbc->lock);
> >> -
> >> -	cancel_work_sync(&fbc->work.work);
> >>  }
> >>  
> >>  static void intel_fbc_underrun_work_fn(struct work_struct *work)
> >> @@ -1387,12 +1295,10 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> >>  {
> >>  	struct intel_fbc *fbc = &dev_priv->fbc;
> >>  
> >> -	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
> >>  	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
> >>  	mutex_init(&fbc->lock);
> >>  	fbc->enabled = false;
> >>  	fbc->active = false;
> >> -	fbc->work.scheduled = false;
> >>  
> >>  	if (need_fbc_vtd_wa(dev_priv))
> >>  		mkwrite_device_info(dev_priv)->has_fbc = false;
> >> -- 
> >> 2.18.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

Re: [PATCH 2/2] drm/i915: Remove delayed FBC activation.

[Maarten Lankhorst]

Op 27-06-18 om 16:14 schreef Ville Syrjälä:
> On Wed, Jun 27, 2018 at 01:45:04PM +0200, Maarten Lankhorst wrote:
>> Op 26-06-18 om 19:59 schreef Ville Syrjälä:
>>> On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
>>>> The only time we should start FBC is when we have waited a vblank
>>>> after the atomic update.
>>> What about front buffer tracking? Is this going to leave FBC permanently
>>> disabled unless there are flips/plane updates?
>> No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page flip,
>> we will not enable FBC in intel_fbc_flush(), but in that case we would enable it from
>> intel_fbc_post_update().
> I guess what I haven't quite figured out is why the pre_plane_update()
> even leaves fbc logically enabled. I would think we should just disable
> fbc there if anything important is going to change, and then re-enable
> at post_plane_update() after reallocating the cfb.
Indeed.
>> Or the other way around, intel_fbc_post_update won't enable FBC if the fb is dirty, but
>> intel_fbc_flush() afterwards will.
>>> I think there are a few cases we need to consider:
>>> 1. plane update which doesn't need fbc disable
>>> 2. plane update which needs to disable fbc but can re-enable it after the flip
>>>    has happended (eg. need to reallocate the cfb due to plane size/format change)
>>> 3. plane update which needs to disable fbc and can't re-enable it (eg. the new
>>>    plane state no longer fbc compatible)
>>> 4. front buffer invalidate + flush
>>>
>>> HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
>>> post-flip vblank wait. Case 3 would ideally let us move FBC to another
>>> plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
>>> after the flush has happened.
>> I don't see how this code will break any case. :)
> OK, so I guess you just remove the delayed activation at
> flush/post_plane_update. So now it'll enable it immediately.
>
> Hmm, and if we just get a flush without the invalidate then we're going
> to just use the nuke msg register anyway. So I suppose this shouldn't
> cause much additional fbc on<->off ping pong.
>
> Actually, are we now effecitively forcing a synchronous vblank wait
> in pre_plane_update for every frame via the hw_deactivate() polling
> for fbc to stop? AFAICS with the re-enable now being immediate we're
> going to have to disable fbc every single time. I think the correct
> fix for that would involve a) not disabling fbc around flips when the
> hw flip nuke will suffice, and b) convert the "wait for fbc to disable"
> thing into a post_plane_update time assert (and verify whether the hw
> is actually happy with disabling both fbc and the plane at the same
> time).
Could we not block in hw_deactivate then? But only <gen5 is affected, not sure how much we still care about i8xx fbc since it's disabled by default.

I think doing it in pre_plane_update is fine? FBC is only enabled on bdw or later. It won't stall there.

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

Re: [PATCH 2/2] drm/i915: Remove delayed FBC activation.

[Ville Syrjälä]

On Thu, Jun 28, 2018 at 04:10:09PM +0200, Maarten Lankhorst wrote:
> Op 27-06-18 om 16:14 schreef Ville Syrjälä:
> > On Wed, Jun 27, 2018 at 01:45:04PM +0200, Maarten Lankhorst wrote:
> >> Op 26-06-18 om 19:59 schreef Ville Syrjälä:
> >>> On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
> >>>> The only time we should start FBC is when we have waited a vblank
> >>>> after the atomic update.
> >>> What about front buffer tracking? Is this going to leave FBC permanently
> >>> disabled unless there are flips/plane updates?
> >> No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page flip,
> >> we will not enable FBC in intel_fbc_flush(), but in that case we would enable it from
> >> intel_fbc_post_update().
> > I guess what I haven't quite figured out is why the pre_plane_update()
> > even leaves fbc logically enabled. I would think we should just disable
> > fbc there if anything important is going to change, and then re-enable
> > at post_plane_update() after reallocating the cfb.
> Indeed.
> >> Or the other way around, intel_fbc_post_update won't enable FBC if the fb is dirty, but
> >> intel_fbc_flush() afterwards will.
> >>> I think there are a few cases we need to consider:
> >>> 1. plane update which doesn't need fbc disable
> >>> 2. plane update which needs to disable fbc but can re-enable it after the flip
> >>>    has happended (eg. need to reallocate the cfb due to plane size/format change)
> >>> 3. plane update which needs to disable fbc and can't re-enable it (eg. the new
> >>>    plane state no longer fbc compatible)
> >>> 4. front buffer invalidate + flush
> >>>
> >>> HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
> >>> post-flip vblank wait. Case 3 would ideally let us move FBC to another
> >>> plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
> >>> after the flush has happened.
> >> I don't see how this code will break any case. :)
> > OK, so I guess you just remove the delayed activation at
> > flush/post_plane_update. So now it'll enable it immediately.
> >
> > Hmm, and if we just get a flush without the invalidate then we're going
> > to just use the nuke msg register anyway. So I suppose this shouldn't
> > cause much additional fbc on<->off ping pong.
> >
> > Actually, are we now effecitively forcing a synchronous vblank wait
> > in pre_plane_update for every frame via the hw_deactivate() polling
> > for fbc to stop? AFAICS with the re-enable now being immediate we're
> > going to have to disable fbc every single time. I think the correct
> > fix for that would involve a) not disabling fbc around flips when the
> > hw flip nuke will suffice, and b) convert the "wait for fbc to disable"
> > thing into a post_plane_update time assert (and verify whether the hw
> > is actually happy with disabling both fbc and the plane at the same
> > time).
> Could we not block in hw_deactivate then? But only <gen5 is affected, not sure how much we still care about i8xx fbc since it's disabled by default.

Hmm. I thought everyone waits there. Apparently I'm mistaken. Never mind
then. We should probably remove the poll from the fbc1 code too then,
and convert it to a post_plane_update assert. But we can defer to that
to when we fix the other remaining issues in that code.

Still a bit of redundant work to deactive+re-activate, but whatever.

OK, I think this series should be fine as is:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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