[PATCH 1/4] drm/i915: Remove WaFbcDisableDpfcClockGating on IVB

[PATCH 1/4] drm/i915: Remove WaFbcDisableDpfcClockGating on IVB

[Ben Widawsky]

Production IVB does not need it. I confirmed this with Art.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d4dd543..33ad028 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -254,12 +254,6 @@ static void ironlake_disable_fbc(struct drm_device *dev)
 		dpfc_ctl &= ~DPFC_CTL_EN;
 		I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
 
-		if (IS_IVYBRIDGE(dev))
-			/* WaFbcDisableDpfcClockGating:ivb */
-			I915_WRITE(ILK_DSPCLK_GATE_D,
-				   I915_READ(ILK_DSPCLK_GATE_D) &
-				   ~ILK_DPFCUNIT_CLOCK_GATE_DISABLE);
-
 		if (IS_HASWELL(dev))
 			/* WaFbcDisableDpfcClockGating:hsw */
 			I915_WRITE(HSW_CLKGATE_DISABLE_PART_1,
@@ -295,10 +289,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	if (IS_IVYBRIDGE(dev)) {
 		/* WaFbcAsynchFlipDisableFbcQueue:ivb */
 		I915_WRITE(ILK_DISPLAY_CHICKEN1, ILK_FBCQ_DIS);
-		/* WaFbcDisableDpfcClockGating:ivb */
-		I915_WRITE(ILK_DSPCLK_GATE_D,
-			   I915_READ(ILK_DSPCLK_GATE_D) |
-			   ILK_DPFCUNIT_CLOCK_GATE_DISABLE);
 	} else {
 		/* WaFbcAsynchFlipDisableFbcQueue:hsw */
 		I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
-- 
1.8.4.1

[PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Ben Widawsky]

Production HSW does not need it. I confirmed this with Art.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_reg.h |  3 ---
 drivers/gpu/drm/i915/intel_pm.c | 10 ----------
 2 files changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6c98238..6799d53 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1110,9 +1110,6 @@
 					     _HSW_PIPE_SLICE_CHICKEN_1_A, + \
 					     _HSW_PIPE_SLICE_CHICKEN_1_B)
 
-#define HSW_CLKGATE_DISABLE_PART_1	0x46500
-#define   HSW_DPFC_GATING_DISABLE	(1<<23)
-
 /*
  * GPIO regs
  */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 33ad028..686699c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -254,12 +254,6 @@ static void ironlake_disable_fbc(struct drm_device *dev)
 		dpfc_ctl &= ~DPFC_CTL_EN;
 		I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
 
-		if (IS_HASWELL(dev))
-			/* WaFbcDisableDpfcClockGating:hsw */
-			I915_WRITE(HSW_CLKGATE_DISABLE_PART_1,
-				   I915_READ(HSW_CLKGATE_DISABLE_PART_1) &
-				   ~HSW_DPFC_GATING_DISABLE);
-
 		DRM_DEBUG_KMS("disabled FBC\n");
 	}
 }
@@ -293,10 +287,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 		/* WaFbcAsynchFlipDisableFbcQueue:hsw */
 		I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
 			   HSW_BYPASS_FBC_QUEUE);
-		/* WaFbcDisableDpfcClockGating:hsw */
-		I915_WRITE(HSW_CLKGATE_DISABLE_PART_1,
-			   I915_READ(HSW_CLKGATE_DISABLE_PART_1) |
-			   HSW_DPFC_GATING_DISABLE);
 	}
 
 	I915_WRITE(SNB_DPFC_CTL_SA,
-- 
1.8.4.1

[PATCH 3/4] drm/i915: WaFbcDisableDpfcrClockGating only with fbc

[Ben Widawsky]

We were turning this on for ILK regardless of whether or not we use FBC.
We can save the slightest amount of power if we don't disable it when
not using FBC.

The workaround should be bit 8 for ILK. Notice it is 1 bit difference
from SNB. This is actually DPFCR unit as we've defined it.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 686699c..bbcf100 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -238,6 +238,11 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
 		sandybridge_blit_fbc_update(dev);
+	} else {
+		/* WaFbcDisableDpfcClockGating:ilk */
+		I915_WRITE(ILK_DSPCLK_GATE_D,
+			   I915_READ(ILK_DSPCLK_GATE_D) |
+			   ILK_DPFCRUNIT_CLOCK_GATE_DISABLE);
 	}
 
 	DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
@@ -254,6 +259,12 @@ static void ironlake_disable_fbc(struct drm_device *dev)
 		dpfc_ctl &= ~DPFC_CTL_EN;
 		I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
 
+		if (IS_GEN5(dev))
+			/* WaFbcDisableDpfcClockGating:ilk */
+			I915_WRITE(ILK_DSPCLK_GATE_D,
+				   I915_READ(ILK_DSPCLK_GATE_D) &
+				   ~ILK_DPFCRUNIT_CLOCK_GATE_DISABLE);
+
 		DRM_DEBUG_KMS("disabled FBC\n");
 	}
 }
@@ -4932,9 +4943,9 @@ static void ironlake_init_clock_gating(struct drm_device *dev)
 
 	/*
 	 * Required for FBC
-	 * WaFbcDisableDpfcClockGating:ilk
+	 * WaFbcDisableDpfcClockGating:snb
 	 */
-	dspclk_gate |= ILK_DPFCRUNIT_CLOCK_GATE_DISABLE |
+	dspclk_gate |=
 		   ILK_DPFCUNIT_CLOCK_GATE_DISABLE |
 		   ILK_DPFDUNIT_CLOCK_GATE_ENABLE;
 
-- 
1.8.4.1

[PATCH 4/4] drm/i915: WaFbcDisableDpfcClockGating only with fbc

[Ben Widawsky]

We were turning this on for SNB regardless of whether or not we use FBC.
We can save the slightest amount of power if we don't disable it when
not using FBC.

The workaround should be bit 9 for SNB.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bbcf100..4ebbe65 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -237,6 +237,12 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 		I915_WRITE(SNB_DPFC_CTL_SA,
 			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
 		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
+
+		/* WaFbcDisableDpfcClockGating:snb */
+		I915_WRITE(ILK_DSPCLK_GATE_D,
+			   I915_READ(ILK_DSPCLK_GATE_D) |
+			   ILK_DPFCUNIT_CLOCK_GATE_DISABLE);
+
 		sandybridge_blit_fbc_update(dev);
 	} else {
 		/* WaFbcDisableDpfcClockGating:ilk */
@@ -259,7 +265,12 @@ static void ironlake_disable_fbc(struct drm_device *dev)
 		dpfc_ctl &= ~DPFC_CTL_EN;
 		I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
 
-		if (IS_GEN5(dev))
+		if (IS_GEN6(dev))
+			/* WaFbcDisableDpfcClockGating:snb */
+			I915_WRITE(ILK_DSPCLK_GATE_D,
+				   I915_READ(ILK_DSPCLK_GATE_D) &
+				   ~ILK_DPFCUNIT_CLOCK_GATE_DISABLE);
+		else if (IS_GEN5(dev))
 			/* WaFbcDisableDpfcClockGating:ilk */
 			I915_WRITE(ILK_DSPCLK_GATE_D,
 				   I915_READ(ILK_DSPCLK_GATE_D) &
@@ -4939,15 +4950,9 @@ static void g4x_disable_trickle_feed(struct drm_device *dev)
 static void ironlake_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;
-
-	/*
-	 * Required for FBC
-	 * WaFbcDisableDpfcClockGating:snb
-	 */
-	dspclk_gate |=
-		   ILK_DPFCUNIT_CLOCK_GATE_DISABLE |
-		   ILK_DPFDUNIT_CLOCK_GATE_ENABLE;
+	uint32_t dspclk_gate =
+		ILK_VRHUNIT_CLOCK_GATE_DISABLE |
+		ILK_DPFDUNIT_CLOCK_GATE_ENABLE;
 
 	I915_WRITE(PCH_3DCGDIS0,
 		   MARIUNIT_CLOCK_GATE_DISABLE |
-- 
1.8.4.1

Re: [PATCH 3/4] drm/i915: WaFbcDisableDpfcrClockGating only with fbc

[Paulo Zanoni]

2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> We were turning this on for ILK regardless of whether or not we use FBC.
> We can save the slightest amount of power if we don't disable it when
> not using FBC.

Finally someone did what I requested months ago:
http://lists.freedesktop.org/archives/intel-gfx/2013-June/028906.html
:)


>
> The workaround should be bit 8 for ILK. Notice it is 1 bit difference
> from SNB. This is actually DPFCR unit as we've defined it.

Ok, so we have bits 8 and 9. Judging by the register names, I would
say bit 9 is WaFbcDisableDpfcClockGating (without 'r') and bit 8 is
the ironlake-only WaFbcDisableDpfcrClockGating. Is that right?


>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 686699c..bbcf100 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -238,6 +238,11 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
>                            SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>                 I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
>                 sandybridge_blit_fbc_update(dev);
> +       } else {
> +               /* WaFbcDisableDpfcClockGating:ilk */

If you agree with me on the question above, then the WA name here is
missing an 'r' char.


> +               I915_WRITE(ILK_DSPCLK_GATE_D,
> +                          I915_READ(ILK_DSPCLK_GATE_D) |
> +                          ILK_DPFCRUNIT_CLOCK_GATE_DISABLE);
>         }
>
>         DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
> @@ -254,6 +259,12 @@ static void ironlake_disable_fbc(struct drm_device *dev)
>                 dpfc_ctl &= ~DPFC_CTL_EN;
>                 I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
>
> +               if (IS_GEN5(dev))
> +                       /* WaFbcDisableDpfcClockGating:ilk */

Same here.


> +                       I915_WRITE(ILK_DSPCLK_GATE_D,
> +                                  I915_READ(ILK_DSPCLK_GATE_D) &
> +                                  ~ILK_DPFCRUNIT_CLOCK_GATE_DISABLE);
> +
>                 DRM_DEBUG_KMS("disabled FBC\n");
>         }
>  }
> @@ -4932,9 +4943,9 @@ static void ironlake_init_clock_gating(struct drm_device *dev)
>
>         /*
>          * Required for FBC
> -        * WaFbcDisableDpfcClockGating:ilk
> +        * WaFbcDisableDpfcClockGating:snb

The ":snb" part is certainly wrong since this function doesn't run on SNB.

The actual code (excluding comments) looks correct.


>          */
> -       dspclk_gate |= ILK_DPFCRUNIT_CLOCK_GATE_DISABLE |
> +       dspclk_gate |=
>                    ILK_DPFCUNIT_CLOCK_GATE_DISABLE |
>                    ILK_DPFDUNIT_CLOCK_GATE_ENABLE;
>
> --
> 1.8.4.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/4] drm/i915: WaFbcDisableDpfcClockGating only with fbc

[Paulo Zanoni]

2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> We were turning this on for SNB regardless of whether or not we use FBC.
> We can save the slightest amount of power if we don't disable it when
> not using FBC.
>
> The workaround should be bit 9 for SNB.

First, see comment in patch 3. So you're removing the WA on ILK and
applying it for SNB-only. Since the spec doesn't say "SNB only", I
guess we need this WA on both gens.

>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bbcf100..4ebbe65 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -237,6 +237,12 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
>                 I915_WRITE(SNB_DPFC_CTL_SA,
>                            SNB_CPU_FENCE_ENABLE | obj->fence_reg);
>                 I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> +
> +               /* WaFbcDisableDpfcClockGating:snb */
> +               I915_WRITE(ILK_DSPCLK_GATE_D,
> +                          I915_READ(ILK_DSPCLK_GATE_D) |
> +                          ILK_DPFCUNIT_CLOCK_GATE_DISABLE);
> +
>                 sandybridge_blit_fbc_update(dev);
>         } else {
>                 /* WaFbcDisableDpfcClockGating:ilk */
> @@ -259,7 +265,12 @@ static void ironlake_disable_fbc(struct drm_device *dev)
>                 dpfc_ctl &= ~DPFC_CTL_EN;
>                 I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
>
> -               if (IS_GEN5(dev))
> +               if (IS_GEN6(dev))
> +                       /* WaFbcDisableDpfcClockGating:snb */
> +                       I915_WRITE(ILK_DSPCLK_GATE_D,
> +                                  I915_READ(ILK_DSPCLK_GATE_D) &
> +                                  ~ILK_DPFCUNIT_CLOCK_GATE_DISABLE);
> +               else if (IS_GEN5(dev))
>                         /* WaFbcDisableDpfcClockGating:ilk */
>                         I915_WRITE(ILK_DSPCLK_GATE_D,
>                                    I915_READ(ILK_DSPCLK_GATE_D) &
> @@ -4939,15 +4950,9 @@ static void g4x_disable_trickle_feed(struct drm_device *dev)
>  static void ironlake_init_clock_gating(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;
> -
> -       /*
> -        * Required for FBC
> -        * WaFbcDisableDpfcClockGating:snb
> -        */
> -       dspclk_gate |=
> -                  ILK_DPFCUNIT_CLOCK_GATE_DISABLE |
> -                  ILK_DPFDUNIT_CLOCK_GATE_ENABLE;
> +       uint32_t dspclk_gate =
> +               ILK_VRHUNIT_CLOCK_GATE_DISABLE |
> +               ILK_DPFDUNIT_CLOCK_GATE_ENABLE;
>
>         I915_WRITE(PCH_3DCGDIS0,
>                    MARIUNIT_CLOCK_GATE_DISABLE |
> --
> 1.8.4.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/4] drm/i915: Remove WaFbcDisableDpfcClockGating on IVB

[Paulo Zanoni]

2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> Production IVB does not need it. I confirmed this with Art.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d4dd543..33ad028 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -254,12 +254,6 @@ static void ironlake_disable_fbc(struct drm_device *dev)
>                 dpfc_ctl &= ~DPFC_CTL_EN;
>                 I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
>
> -               if (IS_IVYBRIDGE(dev))
> -                       /* WaFbcDisableDpfcClockGating:ivb */
> -                       I915_WRITE(ILK_DSPCLK_GATE_D,
> -                                  I915_READ(ILK_DSPCLK_GATE_D) &
> -                                  ~ILK_DPFCUNIT_CLOCK_GATE_DISABLE);
> -
>                 if (IS_HASWELL(dev))
>                         /* WaFbcDisableDpfcClockGating:hsw */
>                         I915_WRITE(HSW_CLKGATE_DISABLE_PART_1,
> @@ -295,10 +289,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
>         if (IS_IVYBRIDGE(dev)) {
>                 /* WaFbcAsynchFlipDisableFbcQueue:ivb */
>                 I915_WRITE(ILK_DISPLAY_CHICKEN1, ILK_FBCQ_DIS);
> -               /* WaFbcDisableDpfcClockGating:ivb */
> -               I915_WRITE(ILK_DSPCLK_GATE_D,
> -                          I915_READ(ILK_DSPCLK_GATE_D) |
> -                          ILK_DPFCUNIT_CLOCK_GATE_DISABLE);
>         } else {
>                 /* WaFbcAsynchFlipDisableFbcQueue:hsw */
>                 I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
> --
> 1.8.4.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Paulo Zanoni]

2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> Production HSW does not need it. I confirmed this with Art.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

I just hope these things don't start uncovering bugs :)

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_reg.h |  3 ---
>  drivers/gpu/drm/i915/intel_pm.c | 10 ----------
>  2 files changed, 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6c98238..6799d53 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1110,9 +1110,6 @@
>                                              _HSW_PIPE_SLICE_CHICKEN_1_A, + \
>                                              _HSW_PIPE_SLICE_CHICKEN_1_B)
>
> -#define HSW_CLKGATE_DISABLE_PART_1     0x46500
> -#define   HSW_DPFC_GATING_DISABLE      (1<<23)
> -
>  /*
>   * GPIO regs
>   */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 33ad028..686699c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -254,12 +254,6 @@ static void ironlake_disable_fbc(struct drm_device *dev)
>                 dpfc_ctl &= ~DPFC_CTL_EN;
>                 I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
>
> -               if (IS_HASWELL(dev))
> -                       /* WaFbcDisableDpfcClockGating:hsw */
> -                       I915_WRITE(HSW_CLKGATE_DISABLE_PART_1,
> -                                  I915_READ(HSW_CLKGATE_DISABLE_PART_1) &
> -                                  ~HSW_DPFC_GATING_DISABLE);
> -
>                 DRM_DEBUG_KMS("disabled FBC\n");
>         }
>  }
> @@ -293,10 +287,6 @@ static void gen7_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
>                 /* WaFbcAsynchFlipDisableFbcQueue:hsw */
>                 I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
>                            HSW_BYPASS_FBC_QUEUE);
> -               /* WaFbcDisableDpfcClockGating:hsw */
> -               I915_WRITE(HSW_CLKGATE_DISABLE_PART_1,
> -                          I915_READ(HSW_CLKGATE_DISABLE_PART_1) |
> -                          HSW_DPFC_GATING_DISABLE);
>         }
>
>         I915_WRITE(SNB_DPFC_CTL_SA,
> --
> 1.8.4.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Daniel Vetter]

On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote:
> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> > Production HSW does not need it. I confirmed this with Art.
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> I just hope these things don't start uncovering bugs :)
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged the first 2 patches of this series. Not sure what to do with the
other two, since fbc is essentially disabled on pre-hsw. And no one seems
to really work on it :( So I only see minimal reasons to frob with it ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Paulo Zanoni]

2013/10/27 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote:
>> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
>> > Production HSW does not need it. I confirmed this with Art.
>> >
>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>>
>> I just hope these things don't start uncovering bugs :)
>>
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Merged the first 2 patches of this series. Not sure what to do with the
> other two, since fbc is essentially disabled on pre-hsw. And no one seems
> to really work on it :( So I only see minimal reasons to frob with it ...

IMHO what you said is another reason to actually merge the other two
patches, since they make FBC-only WAs be applied only on FBC (e.g.,
probably never).

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Ville Syrjälä]

On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote:
> 2013/10/27 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote:
> >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> >> > Production HSW does not need it. I confirmed this with Art.
> >> >
> >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >>
> >> I just hope these things don't start uncovering bugs :)
> >>
> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Merged the first 2 patches of this series. Not sure what to do with the
> > other two, since fbc is essentially disabled on pre-hsw. And no one seems
> > to really work on it :( So I only see minimal reasons to frob with it ...
> 
> IMHO what you said is another reason to actually merge the other two
> patches, since they make FBC-only WAs be applied only on FBC (e.g.,
> probably never).

Another reason would be keeping the codepaths at least somewhat similar.
Could make it a bit easier to fix things later. If it would be me who
gets to fix the FBC mess at some point, I'd try to fix it for all gens
for sure.

At some point I posted a patch to attempt a quick FBC fix for SNB:
"[PATCH] drm/i915: Attempt to fix FBC render tracking with hardware contexts"

In theory that could make FBC work equally well for SNB as it works for
IVB+. And I must confess that I have FBC enabled on my IVB ultrabook
currently since it appears to save a rather significant amount of power.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Daniel Vetter]

On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote:
> 2013/10/27 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote:
> >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> >> > Production HSW does not need it. I confirmed this with Art.
> >> >
> >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >>
> >> I just hope these things don't start uncovering bugs :)
> >>
> >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Merged the first 2 patches of this series. Not sure what to do with the
> > other two, since fbc is essentially disabled on pre-hsw. And no one seems
> > to really work on it :( So I only see minimal reasons to frob with it ...
> 
> IMHO what you said is another reason to actually merge the other two
> patches, since they make FBC-only WAs be applied only on FBC (e.g.,
> probably never).

Oh, that was just a comment on top since the last 2 patches in this series
are blocked on review comments anyway. I'll pull them in once that's
settled. But maybe it spurs someone to work a bit on fbc ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Ben Widawsky]

On Mon, Oct 28, 2013 at 03:05:12PM +0200, Ville Syrjälä wrote:
> On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote:
> > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>:
> > > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote:
> > >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> > >> > Production HSW does not need it. I confirmed this with Art.
> > >> >
> > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > >>
> > >> I just hope these things don't start uncovering bugs :)
> > >>
> > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >
> > > Merged the first 2 patches of this series. Not sure what to do with the
> > > other two, since fbc is essentially disabled on pre-hsw. And no one seems
> > > to really work on it :( So I only see minimal reasons to frob with it ...
> > 
> > IMHO what you said is another reason to actually merge the other two
> > patches, since they make FBC-only WAs be applied only on FBC (e.g.,
> > probably never).
> 
> Another reason would be keeping the codepaths at least somewhat similar.
> Could make it a bit easier to fix things later. If it would be me who
> gets to fix the FBC mess at some point, I'd try to fix it for all gens
> for sure.
> 
> At some point I posted a patch to attempt a quick FBC fix for SNB:
> "[PATCH] drm/i915: Attempt to fix FBC render tracking with hardware contexts"
> 
> In theory that could make FBC work equally well for SNB as it works for
> IVB+. And I must confess that I have FBC enabled on my IVB ultrabook
> currently since it appears to save a rather significant amount of power.
> 
> -- 
> Ville Syrjälä
> Intel OTC


I just looked at your patch, and I should probably comment there, but
it's 5 months old :D. Did you actually observe a fix of something with
that patch? I feel like the way in which we enable/disable tracking, it
shouldn't make a difference.

FWIW: I have two other patches I didn't post yet for SNB. Unfortunately,
I only know one person with failing fbc on SNB, and given his
description, I am not certain it will fix anything.

http://cgit.freedesktop.org/~bwidawsk/drm-intel/log/?h=fbc_fixes

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: WaFbcDisableDpfcrClockGating only with fbc

[Ben Widawsky]

On Fri, Oct 25, 2013 at 03:14:50PM -0200, Paulo Zanoni wrote:
> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> > We were turning this on for ILK regardless of whether or not we use FBC.
> > We can save the slightest amount of power if we don't disable it when
> > not using FBC.
> 
> Finally someone did what I requested months ago:
> http://lists.freedesktop.org/archives/intel-gfx/2013-June/028906.html
> :)
> 
> 
> >
> > The workaround should be bit 8 for ILK. Notice it is 1 bit difference
> > from SNB. This is actually DPFCR unit as we've defined it.
> 
> Ok, so we have bits 8 and 9. Judging by the register names, I would
> say bit 9 is WaFbcDisableDpfcClockGating (without 'r') and bit 8 is
> the ironlake-only WaFbcDisableDpfcrClockGating. Is that right?

Actually from what I remember in the database (not looking currently),
the workaround names are the same, but maybe my eyes missed the 'R'. I
differentiated it with the 'R' so that anyone who was reviewing it
didn't go insane.

> 
> 
> >
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 686699c..bbcf100 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -238,6 +238,11 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> >                            SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> >                 I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> >                 sandybridge_blit_fbc_update(dev);
> > +       } else {
> > +               /* WaFbcDisableDpfcClockGating:ilk */
> 
> If you agree with me on the question above, then the WA name here is
> missing an 'r' char.

I guess I have to go back to the database now to check. I don't have the
bookmark anywhere on my machines here.

> 
> 
> > +               I915_WRITE(ILK_DSPCLK_GATE_D,
> > +                          I915_READ(ILK_DSPCLK_GATE_D) |
> > +                          ILK_DPFCRUNIT_CLOCK_GATE_DISABLE);
> >         }
> >
> >         DRM_DEBUG_KMS("enabled fbc on plane %c\n", plane_name(intel_crtc->plane));
> > @@ -254,6 +259,12 @@ static void ironlake_disable_fbc(struct drm_device *dev)
> >                 dpfc_ctl &= ~DPFC_CTL_EN;
> >                 I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl);
> >
> > +               if (IS_GEN5(dev))
> > +                       /* WaFbcDisableDpfcClockGating:ilk */
> 
> Same here.
> 
> 
> > +                       I915_WRITE(ILK_DSPCLK_GATE_D,
> > +                                  I915_READ(ILK_DSPCLK_GATE_D) &
> > +                                  ~ILK_DPFCRUNIT_CLOCK_GATE_DISABLE);
> > +
> >                 DRM_DEBUG_KMS("disabled FBC\n");
> >         }
> >  }
> > @@ -4932,9 +4943,9 @@ static void ironlake_init_clock_gating(struct drm_device *dev)
> >
> >         /*
> >          * Required for FBC
> > -        * WaFbcDisableDpfcClockGating:ilk
> > +        * WaFbcDisableDpfcClockGating:snb
> 
> The ":snb" part is certainly wrong since this function doesn't run on SNB.
> 
> The actual code (excluding comments) looks correct.
> 

Oops, thanks. The diff should just remove the line completely.

> 
> >          */
> > -       dspclk_gate |= ILK_DPFCRUNIT_CLOCK_GATE_DISABLE |
> > +       dspclk_gate |=
> >                    ILK_DPFCUNIT_CLOCK_GATE_DISABLE |
> >                    ILK_DPFDUNIT_CLOCK_GATE_ENABLE;
> >
> > --
> > 1.8.4.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: WaFbcDisableDpfcClockGating only with fbc

[Ben Widawsky]

On Fri, Oct 25, 2013 at 03:24:12PM -0200, Paulo Zanoni wrote:
> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> > We were turning this on for SNB regardless of whether or not we use FBC.
> > We can save the slightest amount of power if we don't disable it when
> > not using FBC.
> >
> > The workaround should be bit 9 for SNB.
> 
> First, see comment in patch 3. So you're removing the WA on ILK and
> applying it for SNB-only. Since the spec doesn't say "SNB only", I
> guess we need this WA on both gens.

This is a question for Art, since he confirmed this privately IIRC.

Art does ILK need bit 8 and 9 set in 0x42020?

[snip]

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Ville Syrjälä]

On Mon, Oct 28, 2013 at 09:48:55AM -0700, Ben Widawsky wrote:
> On Mon, Oct 28, 2013 at 03:05:12PM +0200, Ville Syrjälä wrote:
> > On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote:
> > > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>:
> > > > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote:
> > > >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> > > >> > Production HSW does not need it. I confirmed this with Art.
> > > >> >
> > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > >>
> > > >> I just hope these things don't start uncovering bugs :)
> > > >>
> > > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >
> > > > Merged the first 2 patches of this series. Not sure what to do with the
> > > > other two, since fbc is essentially disabled on pre-hsw. And no one seems
> > > > to really work on it :( So I only see minimal reasons to frob with it ...
> > > 
> > > IMHO what you said is another reason to actually merge the other two
> > > patches, since they make FBC-only WAs be applied only on FBC (e.g.,
> > > probably never).
> > 
> > Another reason would be keeping the codepaths at least somewhat similar.
> > Could make it a bit easier to fix things later. If it would be me who
> > gets to fix the FBC mess at some point, I'd try to fix it for all gens
> > for sure.
> > 
> > At some point I posted a patch to attempt a quick FBC fix for SNB:
> > "[PATCH] drm/i915: Attempt to fix FBC render tracking with hardware contexts"
> > 
> > In theory that could make FBC work equally well for SNB as it works for
> > IVB+. And I must confess that I have FBC enabled on my IVB ultrabook
> > currently since it appears to save a rather significant amount of power.
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> 
> I just looked at your patch, and I should probably comment there, but
> it's 5 months old :D. Did you actually observe a fix of something with
> that patch? I feel like the way in which we enable/disable tracking, it
> shouldn't make a difference.

I've never even tried to enable FBC on SNB ;)

But it should easy to trick it into doing the wrong thing.

1) switch to context A
2) page flip to buf 0 
   -> FBC RT address will point to buf 0
3) switch to context B
4) page flip to buf 1
   -> FBC RT address will point to buf 1
5) switch to context A
   -> FBC RT addres will be restored to point to buf 0
6) render into buf 1 and observe that FBC doesn't invalidate
   the compressed data

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/4] drm/i915: Remove WaFbcDisableDpfcClockGating on HSW

[Ben Widawsky]

On Mon, Oct 28, 2013 at 07:43:30PM +0200, Ville Syrjälä wrote:
> On Mon, Oct 28, 2013 at 09:48:55AM -0700, Ben Widawsky wrote:
> > On Mon, Oct 28, 2013 at 03:05:12PM +0200, Ville Syrjälä wrote:
> > > On Mon, Oct 28, 2013 at 10:22:31AM -0200, Paulo Zanoni wrote:
> > > > 2013/10/27 Daniel Vetter <daniel@ffwll.ch>:
> > > > > On Fri, Oct 25, 2013 at 03:27:50PM -0200, Paulo Zanoni wrote:
> > > > >> 2013/10/24 Ben Widawsky <benjamin.widawsky@intel.com>:
> > > > >> > Production HSW does not need it. I confirmed this with Art.
> > > > >> >
> > > > >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > >>
> > > > >> I just hope these things don't start uncovering bugs :)
> > > > >>
> > > > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > >
> > > > > Merged the first 2 patches of this series. Not sure what to do with the
> > > > > other two, since fbc is essentially disabled on pre-hsw. And no one seems
> > > > > to really work on it :( So I only see minimal reasons to frob with it ...
> > > > 
> > > > IMHO what you said is another reason to actually merge the other two
> > > > patches, since they make FBC-only WAs be applied only on FBC (e.g.,
> > > > probably never).
> > > 
> > > Another reason would be keeping the codepaths at least somewhat similar.
> > > Could make it a bit easier to fix things later. If it would be me who
> > > gets to fix the FBC mess at some point, I'd try to fix it for all gens
> > > for sure.
> > > 
> > > At some point I posted a patch to attempt a quick FBC fix for SNB:
> > > "[PATCH] drm/i915: Attempt to fix FBC render tracking with hardware contexts"
> > > 
> > > In theory that could make FBC work equally well for SNB as it works for
> > > IVB+. And I must confess that I have FBC enabled on my IVB ultrabook
> > > currently since it appears to save a rather significant amount of power.
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > 
> > I just looked at your patch, and I should probably comment there, but
> > it's 5 months old :D. Did you actually observe a fix of something with
> > that patch? I feel like the way in which we enable/disable tracking, it
> > shouldn't make a difference.
> 
> I've never even tried to enable FBC on SNB ;)
> 
> But it should easy to trick it into doing the wrong thing.
> 
> 1) switch to context A
> 2) page flip to buf 0 
>    -> FBC RT address will point to buf 0
> 3) switch to context B
> 4) page flip to buf 1
>    -> FBC RT address will point to buf 1
> 5) switch to context A
>    -> FBC RT addres will be restored to point to buf 0
> 6) render into buf 1 and observe that FBC doesn't invalidate
>    the compressed data
> 
> -- 
> Ville Syrjälä
> Intel OTC

I thought this is fine because we set the enable bit concurrently with
the base address, and disable when not being used. I really don't know
the code well enough though.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx