[PATCH 0/4] Enable FBC on SKL, v3

[PATCH 0/4] Enable FBC on SKL, v3

[Paulo Zanoni]

Now with the suggestion from Chris instead of the old workaround. We don't need
new DDX patches anymore, but now we need new IGT patches.

Chris Wilson (1):
  drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps

Paulo Zanoni (3):
  drm/i915/fbc: update busy_bits even for GTT and flip flushes
  drm/i915/fbc: sanitize i915.enable_fbc during FBC init
  drm/i915/fbc: enable FBC on gen 9+ too

 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/i915_gem.c  | 14 +++++++++++---
 drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++-----------
 3 files changed, 28 insertions(+), 14 deletions(-)

-- 
2.8.0.rc3

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

[PATCH 1/4] drm/i915/fbc: update busy_bits even for GTT and flip flushes

[Paulo Zanoni]

We ignore ORIGIN_GTT because the hardware tracking can recognize GTT
writes and take care of them. We also ignore ORIGIN_FLIP because we
deal with flips without relying on the frontbuffer tracking
infrastructure. On the other hand, a flush is a flush and means we're
good to go, so we need to update busy_bits in order to reflect that,
even if we're not going to do anything else about it.

How to reproduce the bug fixed by this patch:
 - boot SKL up to the desktop environment
 - stop the display manager
 - run any of the igt/kms_frontbuffer_tracking/*fbc*onoff* subtests
 - the tests will fail

The steps above will create the right conditions for us to lose track
of busy_bits. If you, for example, run the full set of FBC tests, the
onoff subtests will succeed.

Also notice that the "bug" is that we'll just keep FBC disabled on
cases where it could be enabled, so it's not something the users can
perceive, it just affects power consumption numbers on properly
configured machines.

Testcase: igt/kms_frontbuffer_tracking/*fbc*onoff* (see above)
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index d5a7cfe..fc3c094 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -997,13 +997,13 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 	if (!fbc_supported(dev_priv))
 		return;
 
-	if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
-		return;
-
 	mutex_lock(&fbc->lock);
 
 	fbc->busy_bits &= ~frontbuffer_bits;
 
+	if (origin == ORIGIN_GTT || origin == ORIGIN_FLIP)
+		goto out;
+
 	if (!fbc->busy_bits && fbc->enabled &&
 	    (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc))) {
 		if (fbc->active)
@@ -1012,6 +1012,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
 			__intel_fbc_post_update(fbc->crtc);
 	}
 
+out:
 	mutex_unlock(&fbc->lock);
 }
 
-- 
2.8.0.rc3

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

[PATCH 2/4] drm/i915/fbc: sanitize i915.enable_fbc during FBC init

[Paulo Zanoni]

The DDX driver changes its behavior depending on the value it reads
from i915.enable_fbc, so sanitize the value in order to allow it to
know what's going on. It uses this in order to choose the defaults for
the TearFree option. Before this patch, it will read -1 and always
assume that FBC is disabled, so it won't force TearFree.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index fc3c094..3d84ce3 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -824,21 +824,14 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	bool enable_by_default = IS_HASWELL(dev_priv) ||
-				 IS_BROADWELL(dev_priv);
 
 	if (intel_vgpu_active(dev_priv->dev)) {
 		fbc->no_fbc_reason = "VGPU is active";
 		return false;
 	}
 
-	if (i915.enable_fbc < 0 && !enable_by_default) {
-		fbc->no_fbc_reason = "disabled per chip default";
-		return false;
-	}
-
 	if (!i915.enable_fbc) {
-		fbc->no_fbc_reason = "disabled per module param";
+		fbc->no_fbc_reason = "disabled per module param or by default";
 		return false;
 	}
 
@@ -1240,6 +1233,16 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	fbc->active = false;
 	fbc->work.scheduled = false;
 
+	/* The DDX driver changes its behavior depending on the value it reads
+	 * from i915.enable_fbc, so sanitize the value in order to allow it to
+	 * know what's going on. */
+	if (i915.enable_fbc < 0) {
+		i915.enable_fbc = IS_HASWELL(dev_priv) ||
+				  IS_BROADWELL(dev_priv);
+		DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",
+			      i915.enable_fbc);
+	}
+
 	if (!HAS_FBC(dev_priv)) {
 		fbc->no_fbc_reason = "unsupported by this chipset";
 		return;
-- 
2.8.0.rc3

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

Re: [PATCH 2/4] drm/i915/fbc: sanitize i915.enable_fbc during FBC init

[Chris Wilson]

On Mon, Apr 04, 2016 at 06:17:16PM -0300, Paulo Zanoni wrote:
> The DDX driver changes its behavior depending on the value it reads
> from i915.enable_fbc, so sanitize the value in order to allow it to
> know what's going on. It uses this in order to choose the defaults for
> the TearFree option. Before this patch, it will read -1 and always
> assume that FBC is disabled, so it won't force TearFree.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index fc3c094..3d84ce3 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -824,21 +824,14 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  	struct intel_fbc *fbc = &dev_priv->fbc;
> -	bool enable_by_default = IS_HASWELL(dev_priv) ||
> -				 IS_BROADWELL(dev_priv);
>  
>  	if (intel_vgpu_active(dev_priv->dev)) {
>  		fbc->no_fbc_reason = "VGPU is active";
>  		return false;
>  	}
>  
> -	if (i915.enable_fbc < 0 && !enable_by_default) {
> -		fbc->no_fbc_reason = "disabled per chip default";
> -		return false;
> -	}
> -
>  	if (!i915.enable_fbc) {
> -		fbc->no_fbc_reason = "disabled per module param";
> +		fbc->no_fbc_reason = "disabled per module param or by default";
>  		return false;
>  	}
>  
> @@ -1240,6 +1233,16 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  	fbc->active = false;
>  	fbc->work.scheduled = false;
>  
> +	/* The DDX driver changes its behavior depending on the value it reads
> +	 * from i915.enable_fbc, so sanitize the value in order to allow it to
> +	 * know what's going on. */
> +	if (i915.enable_fbc < 0) {
> +		i915.enable_fbc = IS_HASWELL(dev_priv) ||
> +				  IS_BROADWELL(dev_priv);
> +		DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",
> +			      i915.enable_fbc);
> +	}

Standard practice would be to call intel_sanitize_fbc().

i915.enable_fbc = intel_sanitize_fbc(dev_priv);
if (!i915.enable_fbc))
>  		fbc->no_fbc_reason = "unsupported by this chipset";
>  		return;
}
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915/fbc: sanitize i915.enable_fbc during FBC init

[Paulo Zanoni]

The DDX driver changes its behavior depending on the value it reads
from i915.enable_fbc, so sanitize the value in order to allow it to
know what's going on. It uses this in order to choose the defaults for
the TearFree option. Before this patch, it would read -1 and always
assume that FBC was disabled, so it wouldn't force TearFree.

v2: Extract intel_sanitize_fbc_option() (Chris).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index fc3c094..c3dffba 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -824,21 +824,14 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	bool enable_by_default = IS_HASWELL(dev_priv) ||
-				 IS_BROADWELL(dev_priv);
 
 	if (intel_vgpu_active(dev_priv->dev)) {
 		fbc->no_fbc_reason = "VGPU is active";
 		return false;
 	}
 
-	if (i915.enable_fbc < 0 && !enable_by_default) {
-		fbc->no_fbc_reason = "disabled per chip default";
-		return false;
-	}
-
 	if (!i915.enable_fbc) {
-		fbc->no_fbc_reason = "disabled per module param";
+		fbc->no_fbc_reason = "disabled per module param or by default";
 		return false;
 	}
 
@@ -1223,6 +1216,26 @@ void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
 			dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
 }
 
+/*
+ * The DDX driver changes its behavior depending on the value it reads from
+ * i915.enable_fbc, so sanitize it by translating the default value into either
+ * 0 or 1 in order to allow it to know what's going on.
+ *
+ * Notice that this is done at driver initialization and we still allow user
+ * space to change the value during runtime without sanitizing it again. IGT
+ * relies on being able to change i915.enable_fbc at runtime.
+ */
+static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
+{
+	if (i915.enable_fbc >= 0)
+		return !!i915.enable_fbc;
+
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+		return 1;
+
+	return 0;
+}
+
 /**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
@@ -1240,6 +1253,9 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	fbc->active = false;
 	fbc->work.scheduled = false;
 
+	i915.enable_fbc = intel_sanitize_fbc_option(dev_priv);
+	DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n", i915.enable_fbc);
+
 	if (!HAS_FBC(dev_priv)) {
 		fbc->no_fbc_reason = "unsupported by this chipset";
 		return;
-- 
2.8.0.rc3

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

Re: [PATCH 2/4] drm/i915/fbc: sanitize i915.enable_fbc during FBC init

[Daniel Vetter]

On Wed, Apr 13, 2016 at 04:01:09PM -0300, Paulo Zanoni wrote:
> The DDX driver changes its behavior depending on the value it reads
> from i915.enable_fbc, so sanitize the value in order to allow it to
> know what's going on. It uses this in order to choose the defaults for
> the TearFree option. Before this patch, it would read -1 and always
> assume that FBC was disabled, so it wouldn't force TearFree.
> 
> v2: Extract intel_sanitize_fbc_option() (Chris).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index fc3c094..c3dffba 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -824,21 +824,14 @@ static bool intel_fbc_can_choose(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  	struct intel_fbc *fbc = &dev_priv->fbc;
> -	bool enable_by_default = IS_HASWELL(dev_priv) ||
> -				 IS_BROADWELL(dev_priv);
>  
>  	if (intel_vgpu_active(dev_priv->dev)) {
>  		fbc->no_fbc_reason = "VGPU is active";
>  		return false;
>  	}
>  
> -	if (i915.enable_fbc < 0 && !enable_by_default) {
> -		fbc->no_fbc_reason = "disabled per chip default";
> -		return false;
> -	}
> -
>  	if (!i915.enable_fbc) {
> -		fbc->no_fbc_reason = "disabled per module param";
> +		fbc->no_fbc_reason = "disabled per module param or by default";
>  		return false;
>  	}
>  
> @@ -1223,6 +1216,26 @@ void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
>  			dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
>  }
>  
> +/*
> + * The DDX driver changes its behavior depending on the value it reads from
> + * i915.enable_fbc, so sanitize it by translating the default value into either
> + * 0 or 1 in order to allow it to know what's going on.
> + *
> + * Notice that this is done at driver initialization and we still allow user
> + * space to change the value during runtime without sanitizing it again. IGT
> + * relies on being able to change i915.enable_fbc at runtime.
> + */
> +static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
> +{
> +	if (i915.enable_fbc >= 0)
> +		return !!i915.enable_fbc;
> +
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> +		return 1;
> +
> +	return 0;
> +}
> +
>  /**
>   * intel_fbc_init - Initialize FBC
>   * @dev_priv: the i915 device
> @@ -1240,6 +1253,9 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  	fbc->active = false;
>  	fbc->work.scheduled = false;
>  
> +	i915.enable_fbc = intel_sanitize_fbc_option(dev_priv);
> +	DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n", i915.enable_fbc);
> +
>  	if (!HAS_FBC(dev_priv)) {
>  		fbc->no_fbc_reason = "unsupported by this chipset";
>  		return;
> -- 
> 2.8.0.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 3/4] drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps

[Paulo Zanoni]

From: Chris Wilson <chris@chris-wilson.co.uk>

... instead of the previous ORIGIN_GTT. This should actually
invalidate FBC once something is written on the frontbuffer using WC
mmaps. The problem with ORIGIN_GTT is that the automatic hardware
tracking is not able to detect the WC writes as it can detect the GTT
writes.

This should help fix the SKL bug where nothing happens when you type
your username/password on lightdm.

This patch was originally pasted on an email by Chris and converted to
an actual git patch by Paulo.

Cc: <stable@vger.kernel.org> # 4.6
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dd18772..17b42a8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2157,6 +2157,7 @@ struct drm_i915_gem_object {
 	unsigned int cache_dirty:1;
 
 	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
+	unsigned int has_wc_mmap:1;
 
 	unsigned int pin_display;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 40f90c7..bfafa07 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1608,6 +1608,13 @@ static struct intel_rps_client *to_rps_client(struct drm_file *file)
 	return &fpriv->rps;
 }
 
+static enum fb_op_origin
+write_origin(struct drm_i915_gem_object *obj, unsigned domain)
+{
+	return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
+	       ORIGIN_GTT : ORIGIN_CPU;
+}
+
 /**
  * Called when user space prepares to use an object with the CPU, either
  * through the mmap ioctl's mapping or a GTT mapping.
@@ -1661,9 +1668,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
 
 	if (write_domain != 0)
-		intel_fb_obj_invalidate(obj,
-					write_domain == I915_GEM_DOMAIN_GTT ?
-					ORIGIN_GTT : ORIGIN_CPU);
+		intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
 
 unref:
 	drm_gem_object_unreference(&obj->base);
@@ -1761,6 +1766,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		else
 			addr = -ENOMEM;
 		up_write(&mm->mmap_sem);
+
+		/* This may race, but that's ok, it only gets set */
+		to_intel_bo(obj)->has_wc_mmap = true;
 	}
 	drm_gem_object_unreference_unlocked(obj);
 	if (IS_ERR((void *)addr))
-- 
2.8.0.rc3

Re: [Intel-gfx] [PATCH 3/4] drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps

[Daniel Vetter]

On Mon, Apr 04, 2016 at 06:17:17PM -0300, Paulo Zanoni wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> ... instead of the previous ORIGIN_GTT. This should actually
> invalidate FBC once something is written on the frontbuffer using WC
> mmaps. The problem with ORIGIN_GTT is that the automatic hardware
> tracking is not able to detect the WC writes as it can detect the GTT
> writes.
> 
> This should help fix the SKL bug where nothing happens when you type
> your username/password on lightdm.
> 
> This patch was originally pasted on an email by Chris and converted to
> an actual git patch by Paulo.
> 
> Cc: <stable@vger.kernel.org> # 4.6
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

This is From: Chris but lacks his sob. Can't merge as-is, pls ask him for
his sob on irc.

> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dd18772..17b42a8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2157,6 +2157,7 @@ struct drm_i915_gem_object {
>  	unsigned int cache_dirty:1;
>  
>  	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> +	unsigned int has_wc_mmap:1;
>  
>  	unsigned int pin_display;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 40f90c7..bfafa07 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1608,6 +1608,13 @@ static struct intel_rps_client *to_rps_client(struct drm_file *file)
>  	return &fpriv->rps;
>  }
>  
> +static enum fb_op_origin
> +write_origin(struct drm_i915_gem_object *obj, unsigned domain)
> +{
> +	return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
> +	       ORIGIN_GTT : ORIGIN_CPU;
> +}
> +
>  /**
>   * Called when user space prepares to use an object with the CPU, either
>   * through the mmap ioctl's mapping or a GTT mapping.
> @@ -1661,9 +1668,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>  		ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
>  
>  	if (write_domain != 0)
> -		intel_fb_obj_invalidate(obj,
> -					write_domain == I915_GEM_DOMAIN_GTT ?
> -					ORIGIN_GTT : ORIGIN_CPU);
> +		intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
>  
>  unref:
>  	drm_gem_object_unreference(&obj->base);
> @@ -1761,6 +1766,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>  		else
>  			addr = -ENOMEM;
>  		up_write(&mm->mmap_sem);
> +
> +		/* This may race, but that's ok, it only gets set */

This comment doesn't mesh with the

	unsigned int has_wc_mmap:1;

above. If you depend upon atomicity of writes at the hw level, your struct
member _must_ be a full machine word. Which in practice means

	/*
	 * IMPORTANT: We have unlocked access to this, this must be a
	 * stand-alone bool.
	 */
	bool has_wc_mmap;

You can only fold together different bitfields into one if they're _all_
protected by the same lock. gcc needs to generate read-modify-write cycles
to write them, which means if any of them are accessed through a separate
lock or lock-lessly, there's a real race.

With that fixed (and assuming the ABI fix has still Chris' ack/sob):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		to_intel_bo(obj)->has_wc_mmap = true;
>  	}
>  	drm_gem_object_unreference_unlocked(obj);
>  	if (IS_ERR((void *)addr))
> -- 
> 2.8.0.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 3/4] drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps

[Chris Wilson]

On Mon, Apr 25, 2016 at 10:15:11AM +0200, Daniel Vetter wrote:
> On Mon, Apr 04, 2016 at 06:17:17PM -0300, Paulo Zanoni wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > ... instead of the previous ORIGIN_GTT. This should actually
> > invalidate FBC once something is written on the frontbuffer using WC
> > mmaps. The problem with ORIGIN_GTT is that the automatic hardware
> > tracking is not able to detect the WC writes as it can detect the GTT
> > writes.
> > 
> > This should help fix the SKL bug where nothing happens when you type
> > your username/password on lightdm.
> > 
> > This patch was originally pasted on an email by Chris and converted to
> > an actual git patch by Paulo.
> > 
> > Cc: <stable@vger.kernel.org> # 4.6
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
> This is From: Chris but lacks his sob. Can't merge as-is, pls ask him for
> his sob on irc.
> 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> >  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index dd18772..17b42a8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2157,6 +2157,7 @@ struct drm_i915_gem_object {
> >  	unsigned int cache_dirty:1;
> >  
> >  	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> > +	unsigned int has_wc_mmap:1;
> >  
> >  	unsigned int pin_display;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 40f90c7..bfafa07 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1608,6 +1608,13 @@ static struct intel_rps_client *to_rps_client(struct drm_file *file)
> >  	return &fpriv->rps;
> >  }
> >  
> > +static enum fb_op_origin
> > +write_origin(struct drm_i915_gem_object *obj, unsigned domain)
> > +{
> > +	return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
> > +	       ORIGIN_GTT : ORIGIN_CPU;
> > +}
> > +
> >  /**
> >   * Called when user space prepares to use an object with the CPU, either
> >   * through the mmap ioctl's mapping or a GTT mapping.
> > @@ -1661,9 +1668,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> >  		ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
> >  
> >  	if (write_domain != 0)
> > -		intel_fb_obj_invalidate(obj,
> > -					write_domain == I915_GEM_DOMAIN_GTT ?
> > -					ORIGIN_GTT : ORIGIN_CPU);
> > +		intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
> >  
> >  unref:
> >  	drm_gem_object_unreference(&obj->base);
> > @@ -1761,6 +1766,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
> >  		else
> >  			addr = -ENOMEM;
> >  		up_write(&mm->mmap_sem);
> > +
> > +		/* This may race, but that's ok, it only gets set */
> 
> This comment doesn't mesh with the
> 
> 	unsigned int has_wc_mmap:1;
> 
> above. If you depend upon atomicity of writes at the hw level, your struct
> member _must_ be a full machine word. Which in practice means
> 
> 	/*
> 	 * IMPORTANT: We have unlocked access to this, this must be a
> 	 * stand-alone bool.
> 	 */
> 	bool has_wc_mmap;
> 
> You can only fold together different bitfields into one if they're _all_
> protected by the same lock. gcc needs to generate read-modify-write cycles
> to write them, which means if any of them are accessed through a separate
> lock or lock-lessly, there's a real race.
> 
> With that fixed (and assuming the ABI fix has still Chris' ack/sob):

Yup, we can do better and use WRITE_ONCE() as well. That would make GCC
complain about it being bogus.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 3/4] drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps

[Daniel Vetter]

On Mon, Apr 25, 2016 at 09:20:05AM +0100, Chris Wilson wrote:
> On Mon, Apr 25, 2016 at 10:15:11AM +0200, Daniel Vetter wrote:
> > On Mon, Apr 04, 2016 at 06:17:17PM -0300, Paulo Zanoni wrote:
> > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > ... instead of the previous ORIGIN_GTT. This should actually
> > > invalidate FBC once something is written on the frontbuffer using WC
> > > mmaps. The problem with ORIGIN_GTT is that the automatic hardware
> > > tracking is not able to detect the WC writes as it can detect the GTT
> > > writes.
> > > 
> > > This should help fix the SKL bug where nothing happens when you type
> > > your username/password on lightdm.
> > > 
> > > This patch was originally pasted on an email by Chris and converted to
> > > an actual git patch by Paulo.
> > > 
> > > Cc: <stable@vger.kernel.org> # 4.6
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > This is From: Chris but lacks his sob. Can't merge as-is, pls ask him for
> > his sob on irc.
> > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> > >  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
> > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index dd18772..17b42a8 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2157,6 +2157,7 @@ struct drm_i915_gem_object {
> > >  	unsigned int cache_dirty:1;
> > >  
> > >  	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> > > +	unsigned int has_wc_mmap:1;
> > >  
> > >  	unsigned int pin_display;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 40f90c7..bfafa07 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -1608,6 +1608,13 @@ static struct intel_rps_client *to_rps_client(struct drm_file *file)
> > >  	return &fpriv->rps;
> > >  }
> > >  
> > > +static enum fb_op_origin
> > > +write_origin(struct drm_i915_gem_object *obj, unsigned domain)
> > > +{
> > > +	return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
> > > +	       ORIGIN_GTT : ORIGIN_CPU;
> > > +}
> > > +
> > >  /**
> > >   * Called when user space prepares to use an object with the CPU, either
> > >   * through the mmap ioctl's mapping or a GTT mapping.
> > > @@ -1661,9 +1668,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> > >  		ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
> > >  
> > >  	if (write_domain != 0)
> > > -		intel_fb_obj_invalidate(obj,
> > > -					write_domain == I915_GEM_DOMAIN_GTT ?
> > > -					ORIGIN_GTT : ORIGIN_CPU);
> > > +		intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
> > >  
> > >  unref:
> > >  	drm_gem_object_unreference(&obj->base);
> > > @@ -1761,6 +1766,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
> > >  		else
> > >  			addr = -ENOMEM;
> > >  		up_write(&mm->mmap_sem);
> > > +
> > > +		/* This may race, but that's ok, it only gets set */
> > 
> > This comment doesn't mesh with the
> > 
> > 	unsigned int has_wc_mmap:1;
> > 
> > above. If you depend upon atomicity of writes at the hw level, your struct
> > member _must_ be a full machine word. Which in practice means
> > 
> > 	/*
> > 	 * IMPORTANT: We have unlocked access to this, this must be a
> > 	 * stand-alone bool.
> > 	 */
> > 	bool has_wc_mmap;
> > 
> > You can only fold together different bitfields into one if they're _all_
> > protected by the same lock. gcc needs to generate read-modify-write cycles
> > to write them, which means if any of them are accessed through a separate
> > lock or lock-lessly, there's a real race.
> > 
> > With that fixed (and assuming the ABI fix has still Chris' ack/sob):
> 
> Yup, we can do better and use WRITE_ONCE() as well. That would make GCC
> complain about it being bogus.

+1 for WRITE_ONCE(). Great suggestion, totally forgot about it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 3/4] drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps

[Paulo Zanoni]

From: Chris Wilson <chris@chris-wilson.co.uk>

... instead of the previous ORIGIN_GTT. This should actually
invalidate FBC once something is written on the frontbuffer using WC
mmaps. The problem with ORIGIN_GTT is that the automatic hardware
tracking is not able to detect the WC writes as it can detect the GTT
writes.

This should help fix the SKL bug where nothing happens when you type
your username/password on lightdm.

This patch was originally pasted on an email by Chris and converted to
an actual git patch by Paulo.

v2 (From Paulo):
 - Make it a full variable instead of a bit-field (Daniel)
 - Use WRITE_ONCE (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  6 ++++++
 drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

Chris, can you give your Signed-off-by on this patch, please?

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 53d9e3f..20a676d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2204,6 +2204,12 @@ struct drm_i915_gem_object {
 
 	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
 
+	/*
+	 * IMPORTANT: We have unlocked access to this, this must be a
+	 * stand-alone bool.
+	 */
+	unsigned int has_wc_mmap;
+
 	unsigned int pin_display;
 
 	struct sg_table *pages;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index eae8d7a..98389e4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1603,6 +1603,13 @@ static struct intel_rps_client *to_rps_client(struct drm_file *file)
 	return &fpriv->rps;
 }
 
+static enum fb_op_origin
+write_origin(struct drm_i915_gem_object *obj, unsigned domain)
+{
+	return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
+	       ORIGIN_GTT : ORIGIN_CPU;
+}
+
 /**
  * Called when user space prepares to use an object with the CPU, either
  * through the mmap ioctl's mapping or a GTT mapping.
@@ -1659,9 +1666,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
 
 	if (write_domain != 0)
-		intel_fb_obj_invalidate(obj,
-					write_domain == I915_GEM_DOMAIN_GTT ?
-					ORIGIN_GTT : ORIGIN_CPU);
+		intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
 
 unref:
 	drm_gem_object_unreference(&obj->base);
@@ -1768,6 +1773,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		else
 			addr = -ENOMEM;
 		up_write(&mm->mmap_sem);
+
+		/* This may race, but that's ok, it only gets set */
+		WRITE_ONCE(to_intel_bo(obj)->has_wc_mmap, true);
 	}
 	drm_gem_object_unreference_unlocked(obj);
 	if (IS_ERR((void *)addr))
-- 
2.5.5

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

[PATCH 3/4] drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps

[Paulo Zanoni]

From: Chris Wilson <chris@chris-wilson.co.uk>

... instead of the previous ORIGIN_GTT. This should actually
invalidate FBC once something is written on the frontbuffer using WC
mmaps. The problem with ORIGIN_GTT is that the automatic hardware
tracking is not able to detect the WC writes as it can detect the GTT
writes.

This should help fix the SKL bug where nothing happens when you type
your username/password on lightdm.

This patch was originally pasted on an email by Chris and converted to
an actual git patch by Paulo.

v2 (from Paulo):
 - Make it a full variable instead of a bit-field (Daniel)
 - Use WRITE_ONCE (Chris)
v3 (from Paulo):
 - Remove huge comment since now we have WRITE_ONCE (Chris)
 - Remove uneeded new line (Chris)
 - Add Chris' Signed-off-by, authorized via IRC

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9fa9698..4399ea4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2206,6 +2206,7 @@ struct drm_i915_gem_object {
 
 	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
 
+	unsigned int has_wc_mmap;
 	unsigned int pin_display;
 
 	struct sg_table *pages;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 21d0dea..6b1b2fb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1809,6 +1809,13 @@ static struct intel_rps_client *to_rps_client(struct drm_file *file)
 	return &fpriv->rps;
 }
 
+static enum fb_op_origin
+write_origin(struct drm_i915_gem_object *obj, unsigned domain)
+{
+	return domain == I915_GEM_DOMAIN_GTT && !obj->has_wc_mmap ?
+	       ORIGIN_GTT : ORIGIN_CPU;
+}
+
 /**
  * Called when user space prepares to use an object with the CPU, either
  * through the mmap ioctl's mapping or a GTT mapping.
@@ -1865,9 +1872,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0);
 
 	if (write_domain != 0)
-		intel_fb_obj_invalidate(obj,
-					write_domain == I915_GEM_DOMAIN_GTT ?
-					ORIGIN_GTT : ORIGIN_CPU);
+		intel_fb_obj_invalidate(obj, write_origin(obj, write_domain));
 
 unref:
 	drm_gem_object_unreference(&obj->base);
@@ -1974,6 +1979,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		else
 			addr = -ENOMEM;
 		up_write(&mm->mmap_sem);
+
+		/* This may race, but that's ok, it only gets set */
+		WRITE_ONCE(to_intel_bo(obj)->has_wc_mmap, true);
 	}
 	drm_gem_object_unreference_unlocked(obj);
 	if (IS_ERR((void *)addr))
-- 
2.5.5

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

[PATCH 4/4] drm/i915/fbc: enable FBC on gen 9+ too

[Paulo Zanoni]

Now that we're more protected against user space doing frontbuffer
mmap rendering, the last - how many times did I say this before? -
SKL problem seems to be solved. So let's give it a try.

And since the other newer platforms are still under preliminary
support, let's enable FBC on them too to make sure we fix FBC on them.
Also, it seems KBL is passing the tests.

If you reached this commit through git bisect or if you just want more
information about FBC, please see:
    commit a98ee79317b4091cafb502b4ffdbbbe1335e298c
    Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
    Date:   Tue Feb 16 18:47:21 2016 -0200
        drm/i915/fbc: enable FBC by default on HSW and BDW

v2: Enable not only on SKL, but for everything new (Daniel).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 3d84ce3..548d00c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1238,7 +1238,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	 * know what's going on. */
 	if (i915.enable_fbc < 0) {
 		i915.enable_fbc = IS_HASWELL(dev_priv) ||
-				  IS_BROADWELL(dev_priv);
+				  IS_BROADWELL(dev_priv) ||
+				  INTEL_INFO(dev_priv)->gen >= 9;
 		DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",
 			      i915.enable_fbc);
 	}
-- 
2.8.0.rc3

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

[PATCH 4/4] drm/i915/fbc: enable FBC on gen 9+ too

[Paulo Zanoni]

Now that we're more protected against user space doing frontbuffer
mmap rendering, the last - how many times did I say this before? -
SKL problem seems to be solved. So let's give it a try.

And since the other newer platforms are still under preliminary
support, let's enable FBC on them too to make sure we fix FBC on them.
Also, it seems KBL is passing the tests.

If you reached this commit through git bisect or if you just want more
information about FBC, please see:
    commit a98ee79317b4091cafb502b4ffdbbbe1335e298c
    Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
    Date:   Tue Feb 16 18:47:21 2016 -0200
        drm/i915/fbc: enable FBC by default on HSW and BDW

v2: Enable not only on SKL, but for everything new (Daniel).
v3: Rebase after the intel_sanitize_fbc_option() change.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index c3dffba..1043482 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1230,7 +1230,8 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
 	if (i915.enable_fbc >= 0)
 		return !!i915.enable_fbc;
 
-	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv) ||
+	    INTEL_GEN(dev_priv) >= 9)
 		return 1;
 
 	return 0;
-- 
2.8.0.rc3

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

Re: [PATCH 4/4] drm/i915/fbc: enable FBC on gen 9+ too

[Daniel Vetter]

On Wed, Apr 13, 2016 at 04:01:37PM -0300, Paulo Zanoni wrote:
> Now that we're more protected against user space doing frontbuffer
> mmap rendering, the last - how many times did I say this before? -
> SKL problem seems to be solved. So let's give it a try.
> 
> And since the other newer platforms are still under preliminary
> support, let's enable FBC on them too to make sure we fix FBC on them.
> Also, it seems KBL is passing the tests.
> 
> If you reached this commit through git bisect or if you just want more
> information about FBC, please see:
>     commit a98ee79317b4091cafb502b4ffdbbbe1335e298c
>     Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>     Date:   Tue Feb 16 18:47:21 2016 -0200
>         drm/i915/fbc: enable FBC by default on HSW and BDW
> 
> v2: Enable not only on SKL, but for everything new (Daniel).
> v3: Rebase after the intel_sanitize_fbc_option() change.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

\o/ let's pop the champange!

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index c3dffba..1043482 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1230,7 +1230,8 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
>  	if (i915.enable_fbc >= 0)
>  		return !!i915.enable_fbc;
>  
> -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv) ||
> +	    INTEL_GEN(dev_priv) >= 9)
>  		return 1;
>  
>  	return 0;
> -- 
> 2.8.0.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Chris Wilson]

Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are Enabled

"Display flickering may occur when both FBC (Frame Buffer Compression)
and VT - d (Intel® Virtualization Technology for Directed I/O) are enabled
and in use by the display controller."

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_fbc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index fddba1eed5ad..e47785467220 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1230,6 +1230,18 @@ void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
 			dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
 }
 
+static bool need_vtd_wa(struct drm_i915_private *dev_priv)
+{
+#ifdef CONFIG_INTEL_IOMMU
+	if (!intel_iommu_gfx_mapped)
+		return false;
+
+	if (INTEL_GEN(dev_priv) == 9)
+		return true;
+#endif
+	return false;
+}
+
 /**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
@@ -1247,6 +1259,12 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	fbc->active = false;
 	fbc->work.scheduled = false;
 
+	if (need_vtd_wa(dev_priv)) {
+		struct intel_device_info *info =
+			(struct intel_device_info *)&dev_priv->info;
+		info->has_fbc = false;
+	}
+
 	if (!HAS_FBC(dev_priv)) {
 		fbc->no_fbc_reason = "unsupported by this chipset";
 		return;
-- 
2.8.1

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

Re: [PATCH] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Zanoni, Paulo R]

Em Sex, 2016-06-17 às 17:39 +0100, Chris Wilson escreveu:
> Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are
> Enabled
> 
> "Display flickering may occur when both FBC (Frame Buffer
> Compression)
> and VT - d (Intel® Virtualization Technology for Directed I/O) are
> enabled
> and in use by the display controller."

Ouch...

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index fddba1eed5ad..e47785467220 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1230,6 +1230,18 @@ void intel_fbc_init_pipe_state(struct
> drm_i915_private *dev_priv)
>  			dev_priv->fbc.visible_pipes_mask |= (1 <<
> crtc->pipe);
>  }
>  
> +static bool need_vtd_wa(struct drm_i915_private *dev_priv)

Notice we have a function with the exact same name in intel_display.c.

> +{
> +#ifdef CONFIG_INTEL_IOMMU
> +	if (!intel_iommu_gfx_mapped)
> +		return false;
> +
> +	if (INTEL_GEN(dev_priv) == 9)
> +		return true;
> +#endif
> +	return false;
> +}
> +
>  /**
>   * intel_fbc_init - Initialize FBC
>   * @dev_priv: the i915 device
> @@ -1247,6 +1259,12 @@ void intel_fbc_init(struct drm_i915_private
> *dev_priv)
>  	fbc->active = false;
>  	fbc->work.scheduled = false;
>  
> +	if (need_vtd_wa(dev_priv)) {
> +		struct intel_device_info *info =
> +			(struct intel_device_info *)&dev_priv->info;
> +		info->has_fbc = false;
> +	}

I just find this piece above a little not-so-beautiful and possibly
confusing for people debugging failures and/or IGT. Alternatives:

1 - Move the check to intel_fbc_can_choose(), so we can give a nice
no_fbc_reason. Problem: this would not be as efficient as what you
wrote.

2 - Move the check to intel_sanitize_fbc_option(), which is reviewed
but not merged yet. Problem: same as 1.

3 - Patch fbc_supported() and make the line below call fbc_supported()
instead of HAS_FBC(). Problem: we call it many times.

4 - Create dev_priv->fbc.is_supported (or some other meaningful name),
set it at init time, and make fbc_supported() use it (or just replace
fbc_supported() calls with the variable check).

All solutions above would probably require some adjustments to debugfs
and/or IGT (which relies on debugfs), but at least they wouldn't
surprise users or IGT runners with "why does it say SKL is not
supported by FBC?".

> +
>  	if (!HAS_FBC(dev_priv)) {
>  		fbc->no_fbc_reason = "unsupported by this chipset";
>  		return;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[chris]

On Fri, Jun 17, 2016 at 07:02:57PM +0000, Zanoni, Paulo R wrote:
> Em Sex, 2016-06-17 às 17:39 +0100, Chris Wilson escreveu:
> > Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are
> > Enabled
> > 
> > "Display flickering may occur when both FBC (Frame Buffer
> > Compression)
> > and VT - d (Intel® Virtualization Technology for Directed I/O) are
> > enabled
> > and in use by the display controller."
> 
> Ouch...
> 
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_fbc.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > b/drivers/gpu/drm/i915/intel_fbc.c
> > index fddba1eed5ad..e47785467220 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -1230,6 +1230,18 @@ void intel_fbc_init_pipe_state(struct
> > drm_i915_private *dev_priv)
> >  			dev_priv->fbc.visible_pipes_mask |= (1 <<
> > crtc->pipe);
> >  }
> >  
> > +static bool need_vtd_wa(struct drm_i915_private *dev_priv)
> 
> Notice we have a function with the exact same name in intel_display.c.

It's amazing how often we need to write w/a for vtd. I wasn't feeling
imaginative and the uniformity may be useful?

> > +{
> > +#ifdef CONFIG_INTEL_IOMMU
> > +	if (!intel_iommu_gfx_mapped)
> > +		return false;
> > +
> > +	if (INTEL_GEN(dev_priv) == 9)
> > +		return true;
> > +#endif
> > +	return false;
> > +}
> > +
> >  /**
> >   * intel_fbc_init - Initialize FBC
> >   * @dev_priv: the i915 device
> > @@ -1247,6 +1259,12 @@ void intel_fbc_init(struct drm_i915_private
> > *dev_priv)
> >  	fbc->active = false;
> >  	fbc->work.scheduled = false;
> >  
> > +	if (need_vtd_wa(dev_priv)) {
> > +		struct intel_device_info *info =
> > +			(struct intel_device_info *)&dev_priv->info;
> > +		info->has_fbc = false;
> > +	}
> 
> I just find this piece above a little not-so-beautiful and possibly
> confusing for people debugging failures and/or IGT. Alternatives:

Honestly, I thought you were going to suggest something more like:

#define mkwrite_intel_info(ptr) ((struct intel_device_info *)&(ptr)->info)

if (need_vta_wa(dev_priv))
	mkwrite_intel_info(dev_priv)->has_fbc = false;

We have a few other places that require such a beast.

> 1 - Move the check to intel_fbc_can_choose(), so we can give a nice
> no_fbc_reason. Problem: this would not be as efficient as what you
> wrote.
> 
> 2 - Move the check to intel_sanitize_fbc_option(), which is reviewed
> but not merged yet. Problem: same as 1.
> 
> 3 - Patch fbc_supported() and make the line below call fbc_supported()
> instead of HAS_FBC(). Problem: we call it many times.
> 
> 4 - Create dev_priv->fbc.is_supported (or some other meaningful name),
> set it at init time, and make fbc_supported() use it (or just replace
> fbc_supported() calls with the variable check).
> 
> All solutions above would probably require some adjustments to debugfs
> and/or IGT (which relies on debugfs), but at least they wouldn't
> surprise users or IGT runners with "why does it say SKL is not
> supported by FBC?".

Or, we use DRM_INFO() and explain the quirk, which is what I should have
done (since that is the mo for applying quirks).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Ville Syrjälä]

On Fri, Jun 17, 2016 at 05:39:51PM +0100, Chris Wilson wrote:
> Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are Enabled
> 
> "Display flickering may occur when both FBC (Frame Buffer Compression)
> and VT - d (Intel® Virtualization Technology for Directed I/O) are enabled
> and in use by the display controller."

WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index fddba1eed5ad..e47785467220 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1230,6 +1230,18 @@ void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
>  			dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
>  }
>  
> +static bool need_vtd_wa(struct drm_i915_private *dev_priv)
> +{
> +#ifdef CONFIG_INTEL_IOMMU
> +	if (!intel_iommu_gfx_mapped)
> +		return false;
> +
> +	if (INTEL_GEN(dev_priv) == 9)
> +		return true;
> +#endif
> +	return false;
> +}
> +
>  /**
>   * intel_fbc_init - Initialize FBC
>   * @dev_priv: the i915 device
> @@ -1247,6 +1259,12 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  	fbc->active = false;
>  	fbc->work.scheduled = false;
>  
> +	if (need_vtd_wa(dev_priv)) {
> +		struct intel_device_info *info =
> +			(struct intel_device_info *)&dev_priv->info;
> +		info->has_fbc = false;
> +	}
> +
>  	if (!HAS_FBC(dev_priv)) {
>  		fbc->no_fbc_reason = "unsupported by this chipset";
>  		return;
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH v2] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Chris Wilson]

Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are Enabled

"Display flickering may occur when both FBC (Frame Buffer Compression)
and VT - d (Intel® Virtualization Technology for Directed I/O) are enabled
and in use by the display controller."

Ville found the w/a name in the database:
WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl

v2: Log when the quirk is applied.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_fbc.c | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index afe1ff67557c..02b888684de8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2577,6 +2577,8 @@ struct drm_i915_cmd_table {
 #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
 #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
 
+#define mkwrite_intel_info(p) ((struct intel_device_info *)INTEL_INFO(p))
+
 #define REVID_FOREVER		0xff
 #define INTEL_REVID(p)	(__I915__(p)->drm.pdev->revision)
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index fddba1eed5ad..b3c67021771b 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1230,6 +1230,18 @@ void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
 			dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
 }
 
+static bool need_fbc_wa(struct drm_i915_private *dev_priv)
+{
+#ifdef CONFIG_INTEL_IOMMU
+	/* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl */
+	if (intel_iommu_gfx_mapped && IS_SKYLAKE(dev_priv)) {
+		DRM_INFO("Disabling framebuffer compression (FBC) to prevent screen flicker with VT-d enabled\n");
+		return true;
+	}
+#endif
+	return false;
+}
+
 /**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
@@ -1247,6 +1259,9 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	fbc->active = false;
 	fbc->work.scheduled = false;
 
+	if (need_fbc_wa(dev_priv))
+		mkwrite_intel_info(dev_priv)->has_fbc = false;
+
 	if (!HAS_FBC(dev_priv)) {
 		fbc->no_fbc_reason = "unsupported by this chipset";
 		return;
-- 
2.8.1

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

[PATCH v3] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Chris Wilson]

Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are Enabled

"Display flickering may occur when both FBC (Frame Buffer Compression)
and VT - d (Intel® Virtualization Technology for Directed I/O) are enabled
and in use by the display controller."

Ville found the w/a name in the database:
WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl

v2: Log when the quirk is applied.
v3: Ensure i915.enable_fbc is false when !HAS_FBC()

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 89298d3ad94b..f1e9fd07d441 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2586,6 +2586,8 @@ struct drm_i915_cmd_table {
 #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
 #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
 
+#define mkwrite_intel_info(p) ((struct intel_device_info *)INTEL_INFO(p))
+
 #define REVID_FOREVER		0xff
 #define INTEL_REVID(p)	(__I915__(p)->drm.pdev->revision)
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index ac26aa8be9d0..fd5865d80bec 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1238,12 +1238,28 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
 	if (i915.enable_fbc >= 0)
 		return !!i915.enable_fbc;
 
+	if (!HAS_FBC(dev_priv))
+		return 0;
+
 	if (IS_BROADWELL(dev_priv))
 		return 1;
 
 	return 0;
 }
 
+static bool need_fbc_wa(struct drm_i915_private *dev_priv)
+{
+#ifdef CONFIG_INTEL_IOMMU
+	/* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl */
+	if (intel_iommu_gfx_mapped && IS_SKYLAKE(dev_priv)) {
+		DRM_INFO("Disabling framebuffer compression (FBC) to prevent screen flicker with VT-d enabled\n");
+		return true;
+	}
+#endif
+
+	return false;
+}
+
 /**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
@@ -1261,6 +1277,9 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	fbc->active = false;
 	fbc->work.scheduled = false;
 
+	if (need_fbc_wa(dev_priv))
+		mkwrite_intel_info(dev_priv)->has_fbc = false;
+
 	i915.enable_fbc = intel_sanitize_fbc_option(dev_priv);
 	DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n", i915.enable_fbc);
 
-- 
2.8.1

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

Re: [PATCH v3] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Daniel Vetter]

On Tue, Jun 21, 2016 at 08:25:27AM +0100, Chris Wilson wrote:
> Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are Enabled
> 
> "Display flickering may occur when both FBC (Frame Buffer Compression)
> and VT - d (Intel® Virtualization Technology for Directed I/O) are enabled
> and in use by the display controller."
> 
> Ville found the w/a name in the database:
> WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl
> 
> v2: Log when the quirk is applied.
> v3: Ensure i915.enable_fbc is false when !HAS_FBC()
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Do we know whether this helps on other machines too? I can imagine that
the additional lookup latency just plain wreaks havoc everywhere ...
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 89298d3ad94b..f1e9fd07d441 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2586,6 +2586,8 @@ struct drm_i915_cmd_table {
>  #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
>  #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>  
> +#define mkwrite_intel_info(p) ((struct intel_device_info *)INTEL_INFO(p))
> +
>  #define REVID_FOREVER		0xff
>  #define INTEL_REVID(p)	(__I915__(p)->drm.pdev->revision)
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index ac26aa8be9d0..fd5865d80bec 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1238,12 +1238,28 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
>  	if (i915.enable_fbc >= 0)
>  		return !!i915.enable_fbc;
>  
> +	if (!HAS_FBC(dev_priv))
> +		return 0;
> +
>  	if (IS_BROADWELL(dev_priv))
>  		return 1;
>  
>  	return 0;
>  }
>  
> +static bool need_fbc_wa(struct drm_i915_private *dev_priv)
> +{
> +#ifdef CONFIG_INTEL_IOMMU
> +	/* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl */
> +	if (intel_iommu_gfx_mapped && IS_SKYLAKE(dev_priv)) {
> +		DRM_INFO("Disabling framebuffer compression (FBC) to prevent screen flicker with VT-d enabled\n");
> +		return true;
> +	}
> +#endif
> +
> +	return false;
> +}
> +
>  /**
>   * intel_fbc_init - Initialize FBC
>   * @dev_priv: the i915 device
> @@ -1261,6 +1277,9 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
>  	fbc->active = false;
>  	fbc->work.scheduled = false;
>  
> +	if (need_fbc_wa(dev_priv))
> +		mkwrite_intel_info(dev_priv)->has_fbc = false;
> +
>  	i915.enable_fbc = intel_sanitize_fbc_option(dev_priv);
>  	DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n", i915.enable_fbc);
>  
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v3] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Chris Wilson]

On Tue, Jun 21, 2016 at 03:31:25PM +0200, Daniel Vetter wrote:
> On Tue, Jun 21, 2016 at 08:25:27AM +0100, Chris Wilson wrote:
> > Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are Enabled
> > 
> > "Display flickering may occur when both FBC (Frame Buffer Compression)
> > and VT - d (Intel® Virtualization Technology for Directed I/O) are enabled
> > and in use by the display controller."
> > 
> > Ville found the w/a name in the database:
> > WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl
> > 
> > v2: Log when the quirk is applied.
> > v3: Ensure i915.enable_fbc is false when !HAS_FBC()
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Do we know whether this helps on other machines too? I can imagine that
> the additional lookup latency just plain wreaks havoc everywhere ...

Yeah, they do tend to play into fifo trouble. Could find anything
though. But I think we have enough evidence to suggest acking this w/a
and moving forward if we find any others...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Zanoni, Paulo R]

Em Qua, 2016-06-22 às 21:34 +0100, Chris Wilson escreveu:
> On Tue, Jun 21, 2016 at 03:31:25PM +0200, Daniel Vetter wrote:
> > 
> > On Tue, Jun 21, 2016 at 08:25:27AM +0100, Chris Wilson wrote:
> > > 
> > > Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC
> > > Are Enabled
> > > 
> > > "Display flickering may occur when both FBC (Frame Buffer
> > > Compression)
> > > and VT - d (Intel® Virtualization Technology for Directed I/O)
> > > are enabled
> > > and in use by the display controller."
> > > 
> > > Ville found the w/a name in the database:
> > > WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl
> > > 
> > > v2: Log when the quirk is applied.
> > > v3: Ensure i915.enable_fbc is false when !HAS_FBC()
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Do we know whether this helps on other machines too? I can imagine
> > that
> > the additional lookup latency just plain wreaks havoc everywhere
> > ...
> Yeah, they do tend to play into fifo trouble. Could find anything
> though. But I think we have enough evidence to suggest acking this
> w/a
> and moving forward if we find any others...

From the HSD, it looks like this problem would lead to an underrun, so
the "disable FBC if we ever get an underrun" would at least help
diminish the damage in case the WA is actually valid for the previous
gens.

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

Re: [PATCH v3] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Zanoni, Paulo R]

Em Ter, 2016-06-21 às 08:25 +0100, Chris Wilson escreveu:
> Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are
> Enabled
> 
> "Display flickering may occur when both FBC (Frame Buffer
> Compression)
> and VT - d (Intel® Virtualization Technology for Directed I/O) are
> enabled
> and in use by the display controller."
> 
> Ville found the w/a name in the database:
> WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl
> 
> v2: Log when the quirk is applied.
> v3: Ensure i915.enable_fbc is false when !HAS_FBC()
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 89298d3ad94b..f1e9fd07d441 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2586,6 +2586,8 @@ struct drm_i915_cmd_table {
>  #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
>  #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>  
> +#define mkwrite_intel_info(p) ((struct intel_device_info
> *)INTEL_INFO(p))

So why don't we un-const struct intel_device_info?

> +
>  #define REVID_FOREVER		0xff
>  #define INTEL_REVID(p)	(__I915__(p)->drm.pdev->revision)
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index ac26aa8be9d0..fd5865d80bec 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -1238,12 +1238,28 @@ static int intel_sanitize_fbc_option(struct
> drm_i915_private *dev_priv)
>  	if (i915.enable_fbc >= 0)
>  		return !!i915.enable_fbc;
>  
> +	if (!HAS_FBC(dev_priv))
> +		return 0;
> +
>  	if (IS_BROADWELL(dev_priv))
>  		return 1;
>  
>  	return 0;
>  }
>  
> +static bool need_fbc_wa(struct drm_i915_private *dev_priv)
> +{
> +#ifdef CONFIG_INTEL_IOMMU
> +	/* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl */
> +	if (intel_iommu_gfx_mapped 

The doc says: "Workaround: Disable FBC when VT-d superpage is used.".
My iommu-fu is not strong enough for me to be 100% sure that the
variable above corresponds to that. I'll just have to take your word
here.


> && IS_SKYLAKE(dev_priv)) {

The WA also applies to BXT, and pre-prod KBL, so I suppose at least a
check for BXT would be good.

> +		DRM_INFO("Disabling framebuffer compression (FBC) to
> prevent screen flicker with VT-d enabled\n");
> +		return true;
> +	}
> +#endif
> +
> +	return false;
> +}
> +
>  /**
>   * intel_fbc_init - Initialize FBC
>   * @dev_priv: the i915 device
> @@ -1261,6 +1277,9 @@ void intel_fbc_init(struct drm_i915_private
> *dev_priv)
>  	fbc->active = false;
>  	fbc->work.scheduled = false;
>  
> +	if (need_fbc_wa(dev_priv))
> +		mkwrite_intel_info(dev_priv)->has_fbc = false;
> +
>  	i915.enable_fbc = intel_sanitize_fbc_option(dev_priv);
>  	DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n",
> i915.enable_fbc);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Jani Nikula]

On Thu, 23 Jun 2016, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> wrote:
> Em Ter, 2016-06-21 às 08:25 +0100, Chris Wilson escreveu:
>> Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are
>> Enabled
>> 
>> "Display flickering may occur when both FBC (Frame Buffer
>> Compression)
>> and VT - d (Intel® Virtualization Technology for Directed I/O) are
>> enabled
>> and in use by the display controller."
>> 
>> Ville found the w/a name in the database:
>> WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl
>> 
>> v2: Log when the quirk is applied.
>> v3: Ensure i915.enable_fbc is false when !HAS_FBC()
>> 
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>>  drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++++++++++++++
>>  2 files changed, 21 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 89298d3ad94b..f1e9fd07d441 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2586,6 +2586,8 @@ struct drm_i915_cmd_table {
>>  #define INTEL_GEN(p)	(INTEL_INFO(p)->gen)
>>  #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
>>  
>> +#define mkwrite_intel_info(p) ((struct intel_device_info
>> *)INTEL_INFO(p))
>
> So why don't we un-const struct intel_device_info?

Yeah, this is more than a little ugly. Either it should stay const and
we shouldn't modify it (except in intel_device_info_runtime_init(), and
even that is a bit ugly), or we make it non-const.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH igt 1/3] kms_frontbuffer_tracking: prefer the BLT drawing method

[Paulo Zanoni]

A recent Kernel fix changed the way GTT and WC mmaps behave during
frontbuffer drawing. This, added with the fact that GTT mmaps are
special cases for PSR, suggests that maybe we should move to BLT
drawing in places where we can, in order to simplify things a little
bit.

v2: New commit message.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index b4fbbc5..c6d6bc0 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1130,7 +1130,7 @@ static void fill_fb_region(struct fb_region *region, enum color ecolor)
 {
 	uint32_t color = pick_color(region->fb, ecolor);
 
-	igt_draw_rect_fb(drm.fd, NULL, NULL, region->fb, IGT_DRAW_MMAP_CPU,
+	igt_draw_rect_fb(drm.fd, drm.bufmgr, NULL, region->fb, IGT_DRAW_BLT,
 			 region->x, region->y, region->w, region->h,
 			 color);
 }
@@ -3565,7 +3565,7 @@ int main(int argc, char *argv[])
 		if (t.pipes != PIPE_SINGLE ||
 		    t.screen != SCREEN_PRIM ||
 		    t.plane != PLANE_PRI ||
-		    t.method != IGT_DRAW_MMAP_CPU)
+		    t.method != IGT_DRAW_BLT)
 			continue;
 		igt_subtest_f("%s-%s-scaledprimary",
 			      feature_str(t.feature),
@@ -3578,7 +3578,7 @@ int main(int argc, char *argv[])
 		    t.screen != SCREEN_PRIM ||
 		    t.plane != PLANE_PRI ||
 		    t.fbs != FBS_INDIVIDUAL ||
-		    t.method != IGT_DRAW_MMAP_CPU)
+		    t.method != IGT_DRAW_BLT)
 			continue;
 
 		igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature))
-- 
2.8.0.rc3

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

[PATCH igt 2/3] kms_frontbuffer_tracking: recreate the FBs at every subtest

[Paulo Zanoni]

This patch was originally written because of a workaround we were
planning to merge. Later we improved the workaround so it wouldn't
need this patch anymore. Then later we gave up on the workaround, but
decided to go with a plan that would cause GTT mmap writes to
invalidate FBC in case the frontbuffer ever had a WC mmap. So now we
need the patch again because we don't want a subtest that involves an
WC mmap to change the behavior of other unrelated subtests that use
GTT mmaps.

Even if we don't go with the current planned Kernel patch, merging
this should be worth in order to avoid future related problems.

v2: New commit message.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index c6d6bc0..f37de6d 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -711,12 +711,31 @@ static void create_shared_fb(enum pixel_format format)
 		  PLANE_PRI, &s->big);
 }
 
+static void destroy_fbs(enum pixel_format format)
+{
+	struct screen_fbs *s = &fbs[format];
+
+	if (!s->initialized)
+		return;
+
+	if (scnd_mode_params.connector_id) {
+		igt_remove_fb(drm.fd, &s->scnd_pri);
+		igt_remove_fb(drm.fd, &s->scnd_cur);
+		igt_remove_fb(drm.fd, &s->scnd_spr);
+	}
+	igt_remove_fb(drm.fd, &s->prim_pri);
+	igt_remove_fb(drm.fd, &s->prim_cur);
+	igt_remove_fb(drm.fd, &s->prim_spr);
+	igt_remove_fb(drm.fd, &s->offscreen);
+	igt_remove_fb(drm.fd, &s->big);
+}
+
 static void create_fbs(enum pixel_format format)
 {
 	struct screen_fbs *s = &fbs[format];
 
 	if (s->initialized)
-		return;
+		destroy_fbs(format);
 
 	s->initialized = true;
 
@@ -747,25 +766,6 @@ static void create_fbs(enum pixel_format format)
 		  LOCAL_I915_FORMAT_MOD_X_TILED, PLANE_SPR, &s->scnd_spr);
 }
 
-static void destroy_fbs(enum pixel_format format)
-{
-	struct screen_fbs *s = &fbs[format];
-
-	if (!s->initialized)
-		return;
-
-	if (scnd_mode_params.connector_id) {
-		igt_remove_fb(drm.fd, &s->scnd_pri);
-		igt_remove_fb(drm.fd, &s->scnd_cur);
-		igt_remove_fb(drm.fd, &s->scnd_spr);
-	}
-	igt_remove_fb(drm.fd, &s->prim_pri);
-	igt_remove_fb(drm.fd, &s->prim_cur);
-	igt_remove_fb(drm.fd, &s->prim_spr);
-	igt_remove_fb(drm.fd, &s->offscreen);
-	igt_remove_fb(drm.fd, &s->big);
-}
-
 static bool set_mode_for_params(struct modeset_params *params)
 {
 	int rc;
-- 
2.8.0.rc3

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

[PATCH igt 3/3] kms_frontbuffer_tracking: properly handle mixing GTT and WC mmaps

[Paulo Zanoni]

The new Kernel behavior is that whenever a buffer has ever been WC
mmapped, the GTT mmaps will be treated as CPU operations. Because of
this, if we don't issue the dirty_fb IOCTL after doing frontbuffer
rendering with the GTT mmaps, FBC will remain disabled. Luckily, the
only subtest that does this sort of mix is the multidraw subtest.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index f37de6d..89e6ea8 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -2079,6 +2079,7 @@ static void multidraw_subtest(const struct test_mode *t)
 	struct modeset_params *params = pick_params(t);
 	struct fb_region *target;
 	enum igt_draw_method m1, m2, used_method;
+	bool wc_used = false;
 
 	switch (t->plane) {
 	case PLANE_PRI:
@@ -2108,6 +2109,17 @@ static void multidraw_subtest(const struct test_mode *t)
 					igt_draw_get_method_name(used_method));
 
 				draw_rect(pattern, target, used_method, r);
+
+				if (used_method == IGT_DRAW_MMAP_WC)
+					wc_used = true;
+
+				if (used_method == IGT_DRAW_MMAP_GTT &&
+				    wc_used) {
+					struct rect rect =
+						pattern->get_rect(target, r);
+					fb_dirty_ioctl(target, &rect);
+				}
+
 				update_wanted_crc(t,
 						  &pattern->crcs[t->format][r]);
 
-- 
2.8.0.rc3

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

Re: [PATCH 0/4] Enable FBC on SKL, v3

[Thulasimani, Sivakumar]

dont want to hijack thread but wanted to point out a possible regression 
in the
previous patches of this series.

intel_fbc_can_choose: returns true for gen 4/5/6/7. (possible bug)

so intel_crtc_state->enable_fbc = true; will be executed for first crtc
everytime intel_fbc_choose_crtc is called. Although there is check to
handle fbc already enabled, it may fail when we fbc is disabled and
we are working on non supported panel.

regards,
Sivakumar

On 4/5/2016 2:47 AM, Paulo Zanoni wrote:
> Now with the suggestion from Chris instead of the old workaround. We don't need
> new DDX patches anymore, but now we need new IGT patches.
>
> Chris Wilson (1):
>    drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps
>
> Paulo Zanoni (3):
>    drm/i915/fbc: update busy_bits even for GTT and flip flushes
>    drm/i915/fbc: sanitize i915.enable_fbc during FBC init
>    drm/i915/fbc: enable FBC on gen 9+ too
>
>   drivers/gpu/drm/i915/i915_drv.h  |  1 +
>   drivers/gpu/drm/i915/i915_gem.c  | 14 +++++++++++---
>   drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++-----------
>   3 files changed, 28 insertions(+), 14 deletions(-)
>

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

Re: [PATCH 0/4] Enable FBC on SKL, v3

[Zanoni, Paulo R]

Em Qua, 2016-04-06 às 10:36 +0530, Thulasimani, Sivakumar escreveu:
> dont want to hijack thread but wanted to point out a possible
> regression 
> in the
> previous patches of this series.
> 
> intel_fbc_can_choose: returns true for gen 4/5/6/7. (possible bug)

How? It will check for i915.enable_fbc, which will have been sanitized
to zero on these platforms. Aren't you explicitly enabling FBC on these
platforms by using i915.enable_fbc=1?

> 
> so intel_crtc_state->enable_fbc = true; will be executed for first
> crtc
> everytime intel_fbc_choose_crtc is called. Although there is check to
> handle fbc already enabled, it may fail when we fbc is disabled and
> we are working on non supported panel.
> 
> regards,
> Sivakumar
> 
> On 4/5/2016 2:47 AM, Paulo Zanoni wrote:
> > 
> > Now with the suggestion from Chris instead of the old workaround.
> > We don't need
> > new DDX patches anymore, but now we need new IGT patches.
> > 
> > Chris Wilson (1):
> >    drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC
> > mmaps
> > 
> > Paulo Zanoni (3):
> >    drm/i915/fbc: update busy_bits even for GTT and flip flushes
> >    drm/i915/fbc: sanitize i915.enable_fbc during FBC init
> >    drm/i915/fbc: enable FBC on gen 9+ too
> > 
> >   drivers/gpu/drm/i915/i915_drv.h  |  1 +
> >   drivers/gpu/drm/i915/i915_gem.c  | 14 +++++++++++---
> >   drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++-----------
> >   3 files changed, 28 insertions(+), 14 deletions(-)
> > 
> _______________________________________________
> 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 0/4] Enable FBC on SKL, v3

[Thulasimani, Sivakumar]

On 4/6/2016 7:24 PM, Zanoni, Paulo R wrote:
> Em Qua, 2016-04-06 às 10:36 +0530, Thulasimani, Sivakumar escreveu:
>> dont want to hijack thread but wanted to point out a possible
>> regression
>> in the
>> previous patches of this series.
>>
>> intel_fbc_can_choose: returns true for gen 4/5/6/7. (possible bug)
> How? It will check for i915.enable_fbc, which will have been sanitized
> to zero on these platforms. Aren't you explicitly enabling FBC on these
> platforms by using i915.enable_fbc=1?
nope, found as part of code walkthrough hence mentioned above that it is
"possible regression" :) .

Two points
 > found that this is not a regression per say as it was there before 
your change too
 > Agree that FBC code wont reach intel_fbc_can_choose due to platform 
checks
   but without proper comments wont someone assume that the rest of code 
should
work properly especially when we have checks for non haswell/bdw 
platforms in
the code flow.

Also found another possible bug in multiple_pipes_ok.
i assume it is supposed to return true when we can enable FBC
after considering multiple pipes are enabled or not.
but it returns false in case of single display in gen3 thus ending up
doing opposite of multiple pipe requirement.

i understand all these are on very old platforms which dont have
FBC enabled at all so fixing them should be lower priority.
but sharing them here so that at-least it is documented somewhere.

regards,
Sivakumar
>> so intel_crtc_state->enable_fbc = true; will be executed for first
>> crtc
>> everytime intel_fbc_choose_crtc is called. Although there is check to
>> handle fbc already enabled, it may fail when we fbc is disabled and
>> we are working on non supported panel.
>>
>> regards,
>> Sivakumar
>>
>> On 4/5/2016 2:47 AM, Paulo Zanoni wrote:
>>> Now with the suggestion from Chris instead of the old workaround.
>>> We don't need
>>> new DDX patches anymore, but now we need new IGT patches.
>>>
>>> Chris Wilson (1):
>>>     drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC
>>> mmaps
>>>
>>> Paulo Zanoni (3):
>>>     drm/i915/fbc: update busy_bits even for GTT and flip flushes
>>>     drm/i915/fbc: sanitize i915.enable_fbc during FBC init
>>>     drm/i915/fbc: enable FBC on gen 9+ too
>>>
>>>    drivers/gpu/drm/i915/i915_drv.h  |  1 +
>>>    drivers/gpu/drm/i915/i915_gem.c  | 14 +++++++++++---
>>>    drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++-----------
>>>    3 files changed, 28 insertions(+), 14 deletions(-)
>>>
>> _______________________________________________
>> 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

✓ Fi.CI.BAT: success for Enable FBC on SKL (rev4)

[Patchwork]

== Series Details ==

Series: Enable FBC on SKL (rev4)
URL   : https://patchwork.freedesktop.org/series/4722/
State : success

== Summary ==

Series 4722v4 Enable FBC on SKL
http://patchwork.freedesktop.org/api/1.0/series/4722/revisions/4/mbox/

Test gem_busy:
        Subgroup basic-blt:
                skip       -> PASS       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (snb-x220t)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (snb-dellxps)

bdw-nuci7        total:203  pass:191  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:203  pass:180  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:202  pass:163  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:202  pass:164  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:203  pass:179  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:203  pass:184  dwarn:0   dfail:0   fail:0   skip:19 
ilk-hp8440p      total:203  pass:135  dwarn:0   dfail:0   fail:0   skip:68 
ivb-t430s        total:203  pass:175  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:203  pass:178  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:203  pass:192  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:203  pass:165  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:203  pass:165  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_1899/

c0a9d3a8bcf7f049a391a601578054d96708667e drm-intel-nightly: 2016y-04m-14d-12h-22m-13s UTC integration manifest
9ce4821 drm/i915/fbc: enable FBC on gen 9+ too
0662155 drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps
d26f985 drm/i915/fbc: sanitize i915.enable_fbc during FBC init
7f0588e drm/i915/fbc: update busy_bits even for GTT and flip flushes

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

✓ Ro.CI.BAT: success for Enable FBC on SKL (rev5)

[Patchwork]

== Series Details ==

Series: Enable FBC on SKL (rev5)
URL   : https://patchwork.freedesktop.org/series/4722/
State : success

== Summary ==

Series 4722v5 Enable FBC on SKL
http://patchwork.freedesktop.org/api/1.0/series/4722/revisions/5/mbox

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-bdw-i7-5600u)

ro-bdw-i5-5250u  total:213  pass:197  dwarn:2   dfail:0   fail:0   skip:14 
ro-bdw-i7-5557U  total:213  pass:197  dwarn:1   dfail:0   fail:0   skip:15 
ro-bdw-i7-5600u  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-bsw-n3050     total:213  pass:172  dwarn:0   dfail:0   fail:2   skip:39 
ro-hsw-i3-4010u  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:213  pass:150  dwarn:0   dfail:0   fail:1   skip:62 
ro-ilk1-i5-650   total:208  pass:150  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:213  pass:181  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-snb-i7-2620M  total:213  pass:174  dwarn:0   dfail:0   fail:1   skip:38 
fi-hsw-i7-4770k failed to connect after reboot
ro-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1153/

b373842 drm-intel-nightly: 2016y-06m-09d-16h-49m-09s UTC integration manifest
a0a9e44 drm/i915/fbc: enable FBC on gen 9+ too
4fe03eb drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps
8670248 drm/i915/fbc: sanitize i915.enable_fbc during FBC init
9eb1895 drm/i915/fbc: update busy_bits even for GTT and flip flushes

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

✗ Ro.CI.BAT: failure for Enable FBC on SKL (rev6)

[Patchwork]

== Series Details ==

Series: Enable FBC on SKL (rev6)
URL   : https://patchwork.freedesktop.org/series/4722/
State : failure

== Summary ==

Applying: drm/i915/fbc: update busy_bits even for GTT and flip flushes
Applying: drm/i915/fbc: sanitize i915.enable_fbc during FBC init
fatal: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_fbc.c).
error: could not build fake ancestor
Patch failed at 0002 drm/i915/fbc: sanitize i915.enable_fbc during FBC init
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

✗ Ro.CI.BAT: failure for Enable FBC on SKL (rev8)

[Patchwork]

== Series Details ==

Series: Enable FBC on SKL (rev8)
URL   : https://patchwork.freedesktop.org/series/4722/
State : failure

== Summary ==

Applying: drm/i915/fbc: update busy_bits even for GTT and flip flushes
Applying: drm/i915/fbc: sanitize i915.enable_fbc during FBC init
fatal: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_fbc.c).
error: could not build fake ancestor
Patch failed at 0002 drm/i915/fbc: sanitize i915.enable_fbc during FBC init
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

✗ Ro.CI.BAT: failure for Enable FBC on SKL (rev9)

[Patchwork]

== Series Details ==

Series: Enable FBC on SKL (rev9)
URL   : https://patchwork.freedesktop.org/series/4722/
State : failure

== Summary ==

Applying: drm/i915/fbc: update busy_bits even for GTT and flip flushes
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_fbc.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915/fbc: sanitize i915.enable_fbc during FBC init
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_fbc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_fbc.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_fbc.c
error: Failed to merge in the changes.
Patch failed at 0002 drm/i915/fbc: sanitize i915.enable_fbc during FBC init
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH v2] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Ville Syrjälä]

On Thu, Aug 04, 2016 at 08:03:09AM +0100, Chris Wilson wrote:
> On Thu, Aug 04, 2016 at 09:57:11AM +0300, Ville Syrjälä wrote:
> > On Wed, Aug 03, 2016 at 05:16:42PM +0100, Chris Wilson wrote:
> > > Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are Enabled
> > > 
> > > "Display flickering may occur when both FBC (Frame Buffer Compression)
> > > and VT - d (Intel® Virtualization Technology for Directed I/O) are enabled
> > > and in use by the display controller."
> > > 
> > > Ville found the w/a name in the database:
> > > WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl
> > > 
> > > v2: Log when the quirk is applied.
> > > v3: Ensure i915.enable_fbc is false when !HAS_FBC()
> > > v4: Fix function name after rebase
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_fbc.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > > index 8147eb9e8475..b673c7c54a46 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > @@ -1229,12 +1229,28 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
> > >  	if (i915.enable_fbc >= 0)
> > >  		return !!i915.enable_fbc;
> > >  
> > > +	if (!HAS_FBC(dev_priv))
> > > +		return 0;
> > > +
> > >  	if (IS_BROADWELL(dev_priv))
> > >  		return 1;
> > >  
> > >  	return 0;
> > >  }
> > >  
> > > +static bool need_fbc_wa(struct drm_i915_private *dev_priv)
> > 
> > need_fbc_vtd_wa() perhaps?
> 
> Right now, yes. I left it open just in case - but that's most likely a
> false hope.
> 
> > > +{
> > > +#ifdef CONFIG_INTEL_IOMMU
> > > +	/* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl */
> > > +	if (intel_iommu_gfx_mapped && IS_SKYLAKE(dev_priv)) {
> > 
> > BXT needs this as well AFAICS.
> 
> Will IS_GEN9() suit? +skl,bxt in the description.

Looks like KBL doesn't need this w/a.

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

[PATCH v3] drm/i915/fbc: FBC causes display flicker when VT-d is enabled on Skylake

[Chris Wilson]

Erratum SKL075: Display Flicker May Occur When Both VT-d And FBC Are Enabled

"Display flickering may occur when both FBC (Frame Buffer Compression)
and VT - d (Intel® Virtualization Technology for Directed I/O) are enabled
and in use by the display controller."

Ville found the w/a name in the database:
WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl,bxt and also dug out that it
affects Broxton.

v2: Log when the quirk is applied.
v3: Ensure i915.enable_fbc is false when !HAS_FBC()
v4: Fix function name after rebase
v5: Add Broxton to the workaround

Note for backporting to stable, we need to add
	((struct intel_device_info *)INTEL_INFO(ptr))

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_fbc.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 8147eb9e8475..d4be07615aa9 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -1229,12 +1229,29 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *dev_priv)
 	if (i915.enable_fbc >= 0)
 		return !!i915.enable_fbc;
 
+	if (!HAS_FBC(dev_priv))
+		return 0;
+
 	if (IS_BROADWELL(dev_priv))
 		return 1;
 
 	return 0;
 }
 
+static bool need_fbc_vtd_wa(struct drm_i915_private *dev_priv)
+{
+#ifdef CONFIG_INTEL_IOMMU
+	/* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl,bxt */
+	if (intel_iommu_gfx_mapped &&
+	    (IS_SKYLAKE(dev_priv) || IS_BROXTON(dev_priv))) {
+		DRM_INFO("Disabling framebuffer compression (FBC) to prevent screen flicker with VT-d enabled\n");
+		return true;
+	}
+#endif
+
+	return false;
+}
+
 /**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
@@ -1252,6 +1269,9 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 	fbc->active = false;
 	fbc->work.scheduled = false;
 
+	if (need_fbc_vtd_wa(dev_priv))
+		mkwrite_device_info(dev_priv)->has_fbc = false;
+
 	i915.enable_fbc = intel_sanitize_fbc_option(dev_priv);
 	DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n", i915.enable_fbc);
 
-- 
2.8.1