[PATCH 0/3] Enable HSW FBC

[PATCH 0/3] Enable HSW FBC

[Rodrigo Vivi]

Hi all, this small series enable Frame Buffer Compression at HSW.

I decided to create a new function haswell_enable_fbc to avoid getting old
function messed with many IS_HASWELL checks.

Also I decided to split the 2 needed workarounds in separated pathces to be
easy to revert at any time if needed.

On my local tests using a HSW ULT machine I could save around 0.1/0.2W.
Up to 0.3W. And apparently stable enough.

As always, reviews, comments, bikeshedings, tests, etc are welcome.

Thanks in advance,
Rodrigo.

Rodrigo Vivi (3):
  drm/i915: Enable FBC at Haswell.
  drm/i915: HSW FBC WaFbcAsynchFlipDisableFbcQueue
  drm/i915: HSW FBC WaFbcDisableDpfcClockGating

 drivers/gpu/drm/i915/i915_drv.c |  1 +
 drivers/gpu/drm/i915/i915_reg.h | 16 +++++++++++++
 drivers/gpu/drm/i915/intel_pm.c | 50 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 66 insertions(+), 1 deletion(-)

-- 
1.8.1.4

[PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

This patch introduce Frame Buffer Compression (FBC) support for HSW.
It adds a new function haswell_enable_fbc to avoid getting
ironlake_enable_fbc messed with many IS_HASWELL checks.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  1 +
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0cfc778..88fd6fb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -291,6 +291,7 @@ static const struct intel_device_info intel_haswell_m_info = {
 	GEN7_FEATURES,
 	.is_haswell = 1,
 	.is_mobile = 1,
+	.has_fbc = 1,
 };
 
 static const struct pci_device_id pciidlist[] = {		/* aka */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5e91fbb..cb8e213 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -849,6 +849,12 @@
 #define   SNB_CPU_FENCE_ENABLE	(1<<29)
 #define DPFC_CPU_FENCE_OFFSET	0x100104
 
+/* Framebuffer compression for Haswell */
+#define HSW_FBC_RT_BASE			0x7020
+#define   HSW_FBC_RT_BASE_ADDR_SHIFT	12
+
+#define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
+#define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)
 
 /*
  * GPIO regs
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 27f94cd..94e1c3a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
 	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
+static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_framebuffer *fb = crtc->fb;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA : DPFC_CTL_PLANEB;
+	unsigned long stall_watermark = 200;
+	u32 dpfc_ctl;
+
+	dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
+	dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
+	dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
+	dpfc_ctl |= HSW_DPFC_CTL_DISABLE_SLB_INIT;
+	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
+		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
+		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
+	I915_WRITE(HSW_FBC_RT_BASE,
+		   obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
+		   ILK_FBC_RT_VALID);
+	/* enable it... */
+	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
+
+	if (obj->fence_reg != I915_FENCE_REG_NONE) {
+		I915_WRITE(SNB_DPFC_CTL_SA,
+			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
+		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
+	} else
+		I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
+
+	sandybridge_blit_fbc_update(dev);
+
+	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
+}
+
 bool intel_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4158,7 +4195,12 @@ void intel_init_pm(struct drm_device *dev)
 	if (I915_HAS_FBC(dev)) {
 		if (HAS_PCH_SPLIT(dev)) {
 			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
-			dev_priv->display.enable_fbc = ironlake_enable_fbc;
+			if (IS_HASWELL(dev))
+				dev_priv->display.enable_fbc =
+					haswell_enable_fbc;
+			else
+				dev_priv->display.enable_fbc =
+					ironlake_enable_fbc;
 			dev_priv->display.disable_fbc = ironlake_disable_fbc;
 		} else if (IS_GM45(dev)) {
 			dev_priv->display.fbc_enabled = g4x_fbc_enabled;
-- 
1.8.1.4

[PATCH 2/3] drm/i915: HSW FBC WaFbcAsynchFlipDisableFbcQueue

[Rodrigo Vivi]

Display register 420B0h bit 22 must be set to 1b for the entire time that
Frame Buffer Compression is enabled.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 7 +++++++
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cb8e213..2340bc2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -856,6 +856,13 @@
 #define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
 #define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)
 
+#define _HSW_PIPE_SLICE_CHICKEN_1_A	0x420B0
+#define _HSW_PIPE_SLICE_CHICKEN_1_B	0x420B4
+#define   HSW_BYPASS_FBC_QUEUE		(1<<22)
+#define HSW_PIPE_SLICE_CHICKEN_1(pipe) _PIPE(pipe, + \
+					     _HSW_PIPE_SLICE_CHICKEN_1_A, + \
+					     _HSW_PIPE_SLICE_CHICKEN_1_B)
+
 /*
  * GPIO regs
  */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 94e1c3a..0628a84 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -278,6 +278,10 @@ static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	/* enable it... */
 	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
 
+	/* WaFbcAsynchFlipDisableFbcQueue */
+	I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
+		   HSW_BYPASS_FBC_QUEUE);
+
 	if (obj->fence_reg != I915_FENCE_REG_NONE) {
 		I915_WRITE(SNB_DPFC_CTL_SA,
 			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
-- 
1.8.1.4

[PATCH 3/3] drm/i915: HSW FBC WaFbcDisableDpfcClockGating

[Rodrigo Vivi]

Display register 46500h bit 23 must be set to 1b for the entire time that
Frame Buffer Compression is enabled.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 3 +++
 drivers/gpu/drm/i915/intel_pm.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2340bc2..2ef0292 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -863,6 +863,9 @@
 					     _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 0628a84..f2ce541 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -281,6 +281,8 @@ static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	/* WaFbcAsynchFlipDisableFbcQueue */
 	I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
 		   HSW_BYPASS_FBC_QUEUE);
+	/* WaFbcDisableDpfcClockGating */
+	I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, HSW_DPFC_GATING_DISABLE);
 
 	if (obj->fence_reg != I915_FENCE_REG_NONE) {
 		I915_WRITE(SNB_DPFC_CTL_SA,
-- 
1.8.1.4

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Ville Syrjälä]

On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:
> This patch introduce Frame Buffer Compression (FBC) support for HSW.
> It adds a new function haswell_enable_fbc to avoid getting
> ironlake_enable_fbc messed with many IS_HASWELL checks.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |  1 +
>  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0cfc778..88fd6fb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -291,6 +291,7 @@ static const struct intel_device_info intel_haswell_m_info = {
>  	GEN7_FEATURES,
>  	.is_haswell = 1,
>  	.is_mobile = 1,
> +	.has_fbc = 1,
>  };
>  
>  static const struct pci_device_id pciidlist[] = {		/* aka */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5e91fbb..cb8e213 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -849,6 +849,12 @@
>  #define   SNB_CPU_FENCE_ENABLE	(1<<29)
>  #define DPFC_CPU_FENCE_OFFSET	0x100104
>  
> +/* Framebuffer compression for Haswell */
> +#define HSW_FBC_RT_BASE			0x7020
> +#define   HSW_FBC_RT_BASE_ADDR_SHIFT	12
> +
> +#define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
> +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)
>  
>  /*
>   * GPIO regs
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 27f94cd..94e1c3a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
>  	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
>  }
>  
> +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->fb;
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA : DPFC_CTL_PLANEB;
> +	unsigned long stall_watermark = 200;
> +	u32 dpfc_ctl;
> +
> +	dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> +	dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);

Accroding to BSpec FBC is always tied to plane A. Bit 30 is MBZ.

Maybe fix up plane C FBC support for IVB while you're poking at the
general direction?

> +	dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);

The CPU fence field must be written with 0. SNB/IVB could do with the
same fix.

> +	dpfc_ctl |= HSW_DPFC_CTL_DISABLE_SLB_INIT;
> +	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
> +		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
> +		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
> +	I915_WRITE(HSW_FBC_RT_BASE,
> +		   obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
> +		   ILK_FBC_RT_VALID);
> +	/* enable it... */
> +	I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> +
> +	if (obj->fence_reg != I915_FENCE_REG_NONE) {
> +		I915_WRITE(SNB_DPFC_CTL_SA,
> +			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> +		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> +	} else
> +		I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
> +
> +	sandybridge_blit_fbc_update(dev);
> +
> +	DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
> +}
> +
>  bool intel_fbc_enabled(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4158,7 +4195,12 @@ void intel_init_pm(struct drm_device *dev)
>  	if (I915_HAS_FBC(dev)) {
>  		if (HAS_PCH_SPLIT(dev)) {
>  			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
> -			dev_priv->display.enable_fbc = ironlake_enable_fbc;
> +			if (IS_HASWELL(dev))
> +				dev_priv->display.enable_fbc =
> +					haswell_enable_fbc;
> +			else
> +				dev_priv->display.enable_fbc =
> +					ironlake_enable_fbc;
>  			dev_priv->display.disable_fbc = ironlake_disable_fbc;
>  		} else if (IS_GM45(dev)) {
>  			dev_priv->display.fbc_enabled = g4x_fbc_enabled;
> -- 
> 1.8.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: HSW FBC WaFbcDisableDpfcClockGating

[Ville Syrjälä]

On Mon, Apr 08, 2013 at 06:49:44PM -0300, Rodrigo Vivi wrote:
> Display register 46500h bit 23 must be set to 1b for the entire time that
> Frame Buffer Compression is enabled.

So should we enable it again after FBC is disabled to avoid wasting
power?

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 3 +++
>  drivers/gpu/drm/i915/intel_pm.c | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2340bc2..2ef0292 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -863,6 +863,9 @@
>  					     _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 0628a84..f2ce541 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -281,6 +281,8 @@ static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
>  	/* WaFbcAsynchFlipDisableFbcQueue */
>  	I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
>  		   HSW_BYPASS_FBC_QUEUE);
> +	/* WaFbcDisableDpfcClockGating */
> +	I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, HSW_DPFC_GATING_DISABLE);
>  
>  	if (obj->fence_reg != I915_FENCE_REG_NONE) {
>  		I915_WRITE(SNB_DPFC_CTL_SA,
> -- 
> 1.8.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: HSW FBC WaFbcDisableDpfcClockGating

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 2328 bytes --]

On Tue, Apr 9, 2013 at 5:37 AM, Ville Syrjälä <ville.syrjala@linux.intel.com
> wrote:

> On Mon, Apr 08, 2013 at 06:49:44PM -0300, Rodrigo Vivi wrote:
> > Display register 46500h bit 23 must be set to 1b for the entire time that
> > Frame Buffer Compression is enabled.
>
> So should we enable it again after FBC is disabled to avoid wasting
> power?
>

I didn't take the opposite direction because it wasn't explicit.
I tested to enabled it again after FBC is disabled but I didn't see any
difference.
So, you suggestion is to enable back anyway?


>
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 3 +++
> >  drivers/gpu/drm/i915/intel_pm.c | 2 ++
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index 2340bc2..2ef0292 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -863,6 +863,9 @@
> >                                            _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 0628a84..f2ce541 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -281,6 +281,8 @@ static void haswell_enable_fbc(struct drm_crtc
> *crtc, unsigned long interval)
> >       /* WaFbcAsynchFlipDisableFbcQueue */
> >       I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
> >                  HSW_BYPASS_FBC_QUEUE);
> > +     /* WaFbcDisableDpfcClockGating */
> > +     I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, HSW_DPFC_GATING_DISABLE);
> >
> >       if (obj->fence_reg != I915_FENCE_REG_NONE) {
> >               I915_WRITE(SNB_DPFC_CTL_SA,
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 3478 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 5956 bytes --]

On Tue, Apr 9, 2013 at 5:35 AM, Ville Syrjälä <ville.syrjala@linux.intel.com
> wrote:

> On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:
> > This patch introduce Frame Buffer Compression (FBC) support for HSW.
> > It adds a new function haswell_enable_fbc to avoid getting
> > ironlake_enable_fbc messed with many IS_HASWELL checks.
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
> >  drivers/gpu/drm/i915/intel_pm.c | 44
> ++++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> > index 0cfc778..88fd6fb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -291,6 +291,7 @@ static const struct intel_device_info
> intel_haswell_m_info = {
> >       GEN7_FEATURES,
> >       .is_haswell = 1,
> >       .is_mobile = 1,
> > +     .has_fbc = 1,
> >  };
> >
> >  static const struct pci_device_id pciidlist[] = {            /* aka */
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index 5e91fbb..cb8e213 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -849,6 +849,12 @@
> >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)
> >  #define DPFC_CPU_FENCE_OFFSET        0x100104
> >
> > +/* Framebuffer compression for Haswell */
> > +#define HSW_FBC_RT_BASE                      0x7020
> > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12
> > +
> > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)
> > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)
> >
> >  /*
> >   * GPIO regs
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> > index 27f94cd..94e1c3a 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct drm_device
> *dev)
> >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> >  }
> >
> > +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long
> interval)
> > +{
> > +     struct drm_device *dev = crtc->dev;
> > +     struct drm_i915_private *dev_priv = dev->dev_private;
> > +     struct drm_framebuffer *fb = crtc->fb;
> > +     struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +     int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA :
> DPFC_CTL_PLANEB;
> > +     unsigned long stall_watermark = 200;
> > +     u32 dpfc_ctl;
> > +
> > +     dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> > +     dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
>
> Accroding to BSpec FBC is always tied to plane A. Bit 30 is MBZ.
>

Yeah, you are right. I'm going to add a verification at  begining like:
     if (intel_crtc->plane != PLANE_A) {
    dev_priv->no_fbc_reason = FBC_BAD_PLANE;
    return;
}


> Maybe fix up plane C FBC support for IVB while you're poking at the
> general direction?
>

Actually I wasn't trying general directions since I splited it out. It was
just bad copy and paste actually.


>
> > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
>
> The CPU fence field must be written with 0. SNB/IVB could do with the
> same fix.
>

Where did you get this restriction for HSW? Should we write 0 or not touch
by removing lis line?


Thanks for all your comments!


>
> > +     dpfc_ctl |= HSW_DPFC_CTL_DISABLE_SLB_INIT;
> > +     I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
> > +                (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
> > +                (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
> > +     I915_WRITE(HSW_FBC_RT_BASE,
> > +                obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
> > +                ILK_FBC_RT_VALID);
> > +     /* enable it... */
> > +     I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> > +
> > +     if (obj->fence_reg != I915_FENCE_REG_NONE) {
> > +             I915_WRITE(SNB_DPFC_CTL_SA,
> > +                        SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> > +             I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> > +     } else
> > +             I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
> > +
> > +     sandybridge_blit_fbc_update(dev);
> > +
> > +     DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
> > +}
> > +
> >  bool intel_fbc_enabled(struct drm_device *dev)
> >  {
> >       struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -4158,7 +4195,12 @@ void intel_init_pm(struct drm_device *dev)
> >       if (I915_HAS_FBC(dev)) {
> >               if (HAS_PCH_SPLIT(dev)) {
> >                       dev_priv->display.fbc_enabled =
> ironlake_fbc_enabled;
> > -                     dev_priv->display.enable_fbc = ironlake_enable_fbc;
> > +                     if (IS_HASWELL(dev))
> > +                             dev_priv->display.enable_fbc =
> > +                                     haswell_enable_fbc;
> > +                     else
> > +                             dev_priv->display.enable_fbc =
> > +                                     ironlake_enable_fbc;
> >                       dev_priv->display.disable_fbc =
> ironlake_disable_fbc;
> >               } else if (IS_GM45(dev)) {
> >                       dev_priv->display.fbc_enabled = g4x_fbc_enabled;
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 8325 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 3/3] drm/i915: HSW FBC WaFbcDisableDpfcClockGating

[Ville Syrjälä]

On Tue, Apr 09, 2013 at 03:05:10PM -0300, Rodrigo Vivi wrote:
> On Tue, Apr 9, 2013 at 5:37 AM, Ville Syrjälä <ville.syrjala@linux.intel.com
> > wrote:
> 
> > On Mon, Apr 08, 2013 at 06:49:44PM -0300, Rodrigo Vivi wrote:
> > > Display register 46500h bit 23 must be set to 1b for the entire time that
> > > Frame Buffer Compression is enabled.
> >
> > So should we enable it again after FBC is disabled to avoid wasting
> > power?
> >
> 
> I didn't take the opposite direction because it wasn't explicit.
> I tested to enabled it again after FBC is disabled but I didn't see any
> difference.
> So, you suggestion is to enable back anyway?

Perhaps. The idea was that it might save some tiny amount of power.

> 
> >
> > >
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h | 3 +++
> > >  drivers/gpu/drm/i915/intel_pm.c | 2 ++
> > >  2 files changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 2340bc2..2ef0292 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -863,6 +863,9 @@
> > >                                            _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 0628a84..f2ce541 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -281,6 +281,8 @@ static void haswell_enable_fbc(struct drm_crtc
> > *crtc, unsigned long interval)
> > >       /* WaFbcAsynchFlipDisableFbcQueue */
> > >       I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
> > >                  HSW_BYPASS_FBC_QUEUE);
> > > +     /* WaFbcDisableDpfcClockGating */
> > > +     I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, HSW_DPFC_GATING_DISABLE);
> > >
> > >       if (obj->fence_reg != I915_FENCE_REG_NONE) {
> > >               I915_WRITE(SNB_DPFC_CTL_SA,
> > > --
> > > 1.8.1.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Ville Syrjälä]

On Tue, Apr 09, 2013 at 03:13:10PM -0300, Rodrigo Vivi wrote:
> On Tue, Apr 9, 2013 at 5:35 AM, Ville Syrjälä <ville.syrjala@linux.intel.com
> > wrote:
> 
> > On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:
> > > This patch introduce Frame Buffer Compression (FBC) support for HSW.
> > > It adds a new function haswell_enable_fbc to avoid getting
> > > ironlake_enable_fbc messed with many IS_HASWELL checks.
> > >
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c |  1 +
> > >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
> > >  drivers/gpu/drm/i915/intel_pm.c | 44
> > ++++++++++++++++++++++++++++++++++++++++-
> > >  3 files changed, 50 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > > index 0cfc778..88fd6fb 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -291,6 +291,7 @@ static const struct intel_device_info
> > intel_haswell_m_info = {
> > >       GEN7_FEATURES,
> > >       .is_haswell = 1,
> > >       .is_mobile = 1,
> > > +     .has_fbc = 1,
> > >  };
> > >
> > >  static const struct pci_device_id pciidlist[] = {            /* aka */
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 5e91fbb..cb8e213 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -849,6 +849,12 @@
> > >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)
> > >  #define DPFC_CPU_FENCE_OFFSET        0x100104
> > >
> > > +/* Framebuffer compression for Haswell */
> > > +#define HSW_FBC_RT_BASE                      0x7020
> > > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12
> > > +
> > > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)
> > > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)
> > >
> > >  /*
> > >   * GPIO regs
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 27f94cd..94e1c3a 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct drm_device
> > *dev)
> > >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> > >  }
> > >
> > > +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long
> > interval)
> > > +{
> > > +     struct drm_device *dev = crtc->dev;
> > > +     struct drm_i915_private *dev_priv = dev->dev_private;
> > > +     struct drm_framebuffer *fb = crtc->fb;
> > > +     struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +     int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA :
> > DPFC_CTL_PLANEB;
> > > +     unsigned long stall_watermark = 200;
> > > +     u32 dpfc_ctl;
> > > +
> > > +     dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> > > +     dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
> >
> > Accroding to BSpec FBC is always tied to plane A. Bit 30 is MBZ.
> >
> 
> Yeah, you are right. I'm going to add a verification at  begining like:
>      if (intel_crtc->plane != PLANE_A) {
>     dev_priv->no_fbc_reason = FBC_BAD_PLANE;
>     return;
> }
> 
> 
> > Maybe fix up plane C FBC support for IVB while you're poking at the
> > general direction?
> >
> 
> Actually I wasn't trying general directions since I splited it out. It was
> just bad copy and paste actually.

I'm not a fan of copy pasting code and letting the old code paths rot.

> >
> > > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
> >
> > The CPU fence field must be written with 0. SNB/IVB could do with the
> > same fix.
> >
> 
> Where did you get this restriction for HSW?

BSpec.

> Should we write 0 or not touch
> by removing lis line?

Not sure. The spec doesn't say whether the "CPU fence enable" bit still
has some meaning or not. I guess the safe approach would be to set the
fence to 0 and only set the fence enable bit when we actually have a
fence.

BTW the whole dpfc_ctl handing seems to a bit broken. You're doing it
via RMW, but you're not clearing the fields that you modify. So you
could be just ORing bits to whatever garbage the register had before.

> 
> 
> Thanks for all your comments!
> 
> 
> >
> > > +     dpfc_ctl |= HSW_DPFC_CTL_DISABLE_SLB_INIT;
> > > +     I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
> > > +                (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
> > > +                (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
> > > +     I915_WRITE(HSW_FBC_RT_BASE,
> > > +                obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
> > > +                ILK_FBC_RT_VALID);
> > > +     /* enable it... */
> > > +     I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> > > +
> > > +     if (obj->fence_reg != I915_FENCE_REG_NONE) {
> > > +             I915_WRITE(SNB_DPFC_CTL_SA,
> > > +                        SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> > > +             I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> > > +     } else
> > > +             I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
> > > +
> > > +     sandybridge_blit_fbc_update(dev);
> > > +
> > > +     DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
> > > +}
> > > +
> > >  bool intel_fbc_enabled(struct drm_device *dev)
> > >  {
> > >       struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -4158,7 +4195,12 @@ void intel_init_pm(struct drm_device *dev)
> > >       if (I915_HAS_FBC(dev)) {
> > >               if (HAS_PCH_SPLIT(dev)) {
> > >                       dev_priv->display.fbc_enabled =
> > ironlake_fbc_enabled;
> > > -                     dev_priv->display.enable_fbc = ironlake_enable_fbc;
> > > +                     if (IS_HASWELL(dev))
> > > +                             dev_priv->display.enable_fbc =
> > > +                                     haswell_enable_fbc;
> > > +                     else
> > > +                             dev_priv->display.enable_fbc =
> > > +                                     ironlake_enable_fbc;
> > >                       dev_priv->display.disable_fbc =
> > ironlake_disable_fbc;
> > >               } else if (IS_GM45(dev)) {
> > >                       dev_priv->display.fbc_enabled = g4x_fbc_enabled;
> > > --
> > > 1.8.1.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Daniel Vetter]

On Wed, Apr 10, 2013 at 10:18 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> > > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
>> >
>> > The CPU fence field must be written with 0. SNB/IVB could do with the
>> > same fix.
>> >
>>
>> Where did you get this restriction for HSW?
>
> BSpec.
>
>> Should we write 0 or not touch
>> by removing lis line?
>
> Not sure. The spec doesn't say whether the "CPU fence enable" bit still
> has some meaning or not. I guess the safe approach would be to set the
> fence to 0 and only set the fence enable bit when we actually have a
> fence.
>
> BTW the whole dpfc_ctl handing seems to a bit broken. You're doing it
> via RMW, but you're not clearing the fields that you modify. So you
> could be just ORing bits to whatever garbage the register had before.

Ok, I guess it's time for me to write down the fbc master plan here ;-)

So imo psr and fbc fit into the exact same category: Both safe a bit
of power if the display contents doesn't change. And for both we need
to know when contents has changed so that we can either disable it or
through some hw magic update screen contents. So imo we can (and
should) wire up all the psr magic into the same
intel_enable/disable_fbc functions.

Now on platforms thus far the power savings part of fbc worked usually
rather well, real ugliness kicked in when trying to enable the hw
magic to detect display contents changes. The hw supplies three pieces
for that:
- cpu fence to detect writes from the cpu through the gtt
- blt fbc range - each time the blt touches anything in there fbc gets a note
- render fbc range - same
- pageflips send a signal to fbc/psr to update screen

Those above three things are usually riddled with workarounds, tend to
hang the box (or as in the case of the gen6 blitter) totally kill
throughput when enabled. The only thing which seems to work is the
pageflip stuff.

So my proposal is that before we enable anything like fbc or psr by
default, we can all this hw magic. Completely.

And then we replace it with some nice software tracking. The main idea
is that at least on modern desktop, screen updates always happen with
pageflips. That means for X only one active crtc since X still doesn't
have per-crtc pixmaps, but meh.

Now for the pageflipping compositor model we don't need any of the
fancy (and usually broken) hw magic to detect updates to the currently
scanning out framebuffer - the compositor always renders to the
backbuffer.

So the idea is now to enable fbc/psr when pageflipping and disable it
as soon as we detect a frontbuffer rendering event. Thanks to the
current gem api we can do this pretty cheaply:
- CPU writes to the frontbuffer call sw_finish_ioctl. I need to still
check whether that only applies to cpu maps or also gtt maps.
- GTT writes could easily be caught by unmapping userspace ptes (we
have the infrastructure for that already), but I hope we can avoid
that.
- Currently we have some fragile code in our execbuf handler to detect
render of the gpu. I think we can rip this all out and instead detect
frontbuffer gpu rendering in the busy ioctl - that has been
established as the canonical interface for userspace to ask the kernel
to flush the frontbuffer. Aside: This semantics was not designed, but
we've realized that this is what it is eventually ;-)

Now once we have done the Big Rip and restricted fbc/psr enabling to
pageflips and disabling to these few places there's one thing left: We
need proper locking. My proposal is to add a new fbc_mutex which
protects all the special fbc state. That way we can avoid the current
ugly locking hell between kms and gem: Since due to modeset/dpms
changes we need to update fbc state, we enable it in the kms pageflip
code if possible, but gem events ditacte when we need to disable it
again. But if the fbc state is protected by its own mutex which needs
within the kms/gem locks we should be fine.

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

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Chris Wilson]

On Wed, Apr 10, 2013 at 10:52:20AM +0200, Daniel Vetter wrote:
> So the idea is now to enable fbc/psr when pageflipping and disable it
> as soon as we detect a frontbuffer rendering event. Thanks to the
> current gem api we can do this pretty cheaply:
> - CPU writes to the frontbuffer call sw_finish_ioctl. I need to still
> check whether that only applies to cpu maps or also gtt maps.
> - GTT writes could easily be caught by unmapping userspace ptes (we
> have the infrastructure for that already), but I hope we can avoid
> that.
> - Currently we have some fragile code in our execbuf handler to detect
> render of the gpu. I think we can rip this all out and instead detect
> frontbuffer gpu rendering in the busy ioctl - that has been
> established as the canonical interface for userspace to ask the kernel
> to flush the frontbuffer. Aside: This semantics was not designed, but
> we've realized that this is what it is eventually ;-)

To add my wishlist item:
- a global property / param indicating the availabilty of FBC
- a crtc property indicating the status of FBC
That way X can avoid front buffer rendering when it would incur
penalties and is likely to result in corruption (I'm sure the next
generation of FBC will work, or maybe the next).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 7908 bytes --]

On Wed, Apr 10, 2013 at 5:18 AM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> On Tue, Apr 09, 2013 at 03:13:10PM -0300, Rodrigo Vivi wrote:
> > On Tue, Apr 9, 2013 at 5:35 AM, Ville Syrjälä <
> ville.syrjala@linux.intel.com
> > > wrote:
> >
> > > On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:
> > > > This patch introduce Frame Buffer Compression (FBC) support for HSW.
> > > > It adds a new function haswell_enable_fbc to avoid getting
> > > > ironlake_enable_fbc messed with many IS_HASWELL checks.
> > > >
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c |  1 +
> > > >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
> > > >  drivers/gpu/drm/i915/intel_pm.c | 44
> > > ++++++++++++++++++++++++++++++++++++++++-
> > > >  3 files changed, 50 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 0cfc778..88fd6fb 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -291,6 +291,7 @@ static const struct intel_device_info
> > > intel_haswell_m_info = {
> > > >       GEN7_FEATURES,
> > > >       .is_haswell = 1,
> > > >       .is_mobile = 1,
> > > > +     .has_fbc = 1,
> > > >  };
> > > >
> > > >  static const struct pci_device_id pciidlist[] = {            /* aka
> */
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 5e91fbb..cb8e213 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -849,6 +849,12 @@
> > > >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)
> > > >  #define DPFC_CPU_FENCE_OFFSET        0x100104
> > > >
> > > > +/* Framebuffer compression for Haswell */
> > > > +#define HSW_FBC_RT_BASE                      0x7020
> > > > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12
> > > > +
> > > > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)
> > > > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)
> > > >
> > > >  /*
> > > >   * GPIO regs
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 27f94cd..94e1c3a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct
> drm_device
> > > *dev)
> > > >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> > > >  }
> > > >
> > > > +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long
> > > interval)
> > > > +{
> > > > +     struct drm_device *dev = crtc->dev;
> > > > +     struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +     struct drm_framebuffer *fb = crtc->fb;
> > > > +     struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > > > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > > > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +     int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA :
> > > DPFC_CTL_PLANEB;
> > > > +     unsigned long stall_watermark = 200;
> > > > +     u32 dpfc_ctl;
> > > > +
> > > > +     dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> > > > +     dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
> > >
> > > Accroding to BSpec FBC is always tied to plane A. Bit 30 is MBZ.
> > >
> >
> > Yeah, you are right. I'm going to add a verification at  begining like:
> >      if (intel_crtc->plane != PLANE_A) {
> >     dev_priv->no_fbc_reason = FBC_BAD_PLANE;
> >     return;
> > }
> >
> >
> > > Maybe fix up plane C FBC support for IVB while you're poking at the
> > > general direction?
> > >
> >
> > Actually I wasn't trying general directions since I splited it out. It
> was
> > just bad copy and paste actually.
>
> I'm not a fan of copy pasting code and letting the old code paths rot.
>
> > >
> > > > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
> > >
> > > The CPU fence field must be written with 0. SNB/IVB could do with the
> > > same fix.
> > >
> >
> > Where did you get this restriction for HSW?
>
> BSpec.
>

Are you talking about bit 28 of 43208h DevHSW?
I couldn't find this restriction anywhere.
Besides that, setting it to 0 made me experience bugs like missing some
small screen updates. I mean, when it is 0 I missed many "****" when typing
my login password.
When it is set FBC works fine.


>
> > Should we write 0 or not touch
> > by removing lis line?
>
> Not sure. The spec doesn't say whether the "CPU fence enable" bit still
> has some meaning or not. I guess the safe approach would be to set the
> fence to 0 and only set the fence enable bit when we actually have a
> fence.
>
> BTW the whole dpfc_ctl handing seems to a bit broken. You're doing it
> via RMW, but you're not clearing the fields that you modify. So you
> could be just ORing bits to whatever garbage the register had before.
>

Although I don't see how a garbage could end up there I completely agree
with you. So I'm going to just set the register directly without using this
temp variable.


>
> >
> >
> > Thanks for all your comments!
> >
> >
> > >
> > > > +     dpfc_ctl |= HSW_DPFC_CTL_DISABLE_SLB_INIT;
> > > > +     I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
> > > > +                (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
> > > > +                (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
> > > > +     I915_WRITE(HSW_FBC_RT_BASE,
> > > > +                obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
> > > > +                ILK_FBC_RT_VALID);
> > > > +     /* enable it... */
> > > > +     I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> > > > +
> > > > +     if (obj->fence_reg != I915_FENCE_REG_NONE) {
> > > > +             I915_WRITE(SNB_DPFC_CTL_SA,
> > > > +                        SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> > > > +             I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> > > > +     } else
> > > > +             I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
> > > > +
> > > > +     sandybridge_blit_fbc_update(dev);
> > > > +
> > > > +     DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);
> > > > +}
> > > > +
> > > >  bool intel_fbc_enabled(struct drm_device *dev)
> > > >  {
> > > >       struct drm_i915_private *dev_priv = dev->dev_private;
> > > > @@ -4158,7 +4195,12 @@ void intel_init_pm(struct drm_device *dev)
> > > >       if (I915_HAS_FBC(dev)) {
> > > >               if (HAS_PCH_SPLIT(dev)) {
> > > >                       dev_priv->display.fbc_enabled =
> > > ironlake_fbc_enabled;
> > > > -                     dev_priv->display.enable_fbc =
> ironlake_enable_fbc;
> > > > +                     if (IS_HASWELL(dev))
> > > > +                             dev_priv->display.enable_fbc =
> > > > +                                     haswell_enable_fbc;
> > > > +                     else
> > > > +                             dev_priv->display.enable_fbc =
> > > > +                                     ironlake_enable_fbc;
> > > >                       dev_priv->display.disable_fbc =
> > > ironlake_disable_fbc;
> > > >               } else if (IS_GM45(dev)) {
> > > >                       dev_priv->display.fbc_enabled =
> g4x_fbc_enabled;
> > > > --
> > > > 1.8.1.4
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > >
> >
> >
> >
> > --
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
>
> --
> Ville Syrjälä
> Intel OTC
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 10880 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

This patch introduce Frame Buffer Compression (FBC) support for HSW.
It adds a new function haswell_enable_fbc to avoid getting
ironlake_enable_fbc messed with many IS_HASWELL checks.

v2: Fixes from Ville.
     	*  Fix Plane. FBC is tied to primary plane A in HSW
    	*  Fix DPFC initial write to avoid let trash on the register.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  1 +
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_pm.c | 46 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0cfc778..88fd6fb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -291,6 +291,7 @@ static const struct intel_device_info intel_haswell_m_info = {
 	GEN7_FEATURES,
 	.is_haswell = 1,
 	.is_mobile = 1,
+	.has_fbc = 1,
 };
 
 static const struct pci_device_id pciidlist[] = {		/* aka */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5e91fbb..cb8e213 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -849,6 +849,12 @@
 #define   SNB_CPU_FENCE_ENABLE	(1<<29)
 #define DPFC_CPU_FENCE_OFFSET	0x100104
 
+/* Framebuffer compression for Haswell */
+#define HSW_FBC_RT_BASE			0x7020
+#define   HSW_FBC_RT_BASE_ADDR_SHIFT	12
+
+#define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
+#define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)
 
 /*
  * GPIO regs
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 27f94cd..f3b35c4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -253,6 +253,45 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
 	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
+static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_framebuffer *fb = crtc->fb;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	unsigned long stall_watermark = 200;
+
+	/* FBC is tied to primary plane A in HSW */
+	if (intel_crtc->plane != PLANE_A) {
+	    dev_priv->no_fbc_reason = FBC_BAD_PLANE;
+	    return;
+	}
+
+	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
+		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
+		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
+	I915_WRITE(HSW_FBC_RT_BASE,
+		   obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
+		   ILK_FBC_RT_VALID);
+
+	I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
+		   (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg) |
+		   HSW_DPFC_CTL_DISABLE_SLB_INIT);
+
+	if (obj->fence_reg != I915_FENCE_REG_NONE) {
+		I915_WRITE(SNB_DPFC_CTL_SA,
+			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
+		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
+	} else
+		I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
+
+	sandybridge_blit_fbc_update(dev);
+
+	DRM_DEBUG_KMS("enabled fbc on plane A\n");
+}
+
 bool intel_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4158,7 +4197,12 @@ void intel_init_pm(struct drm_device *dev)
 	if (I915_HAS_FBC(dev)) {
 		if (HAS_PCH_SPLIT(dev)) {
 			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
-			dev_priv->display.enable_fbc = ironlake_enable_fbc;
+			if (IS_HASWELL(dev))
+				dev_priv->display.enable_fbc =
+					haswell_enable_fbc;
+			else
+				dev_priv->display.enable_fbc =
+					ironlake_enable_fbc;
 			dev_priv->display.disable_fbc = ironlake_disable_fbc;
 		} else if (IS_GM45(dev)) {
 			dev_priv->display.fbc_enabled = g4x_fbc_enabled;
-- 
1.8.1.4

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

[PATCH] drm/i915: HSW FBC WaFbcDisableDpfcClockGating

[Rodrigo Vivi]

Display register 46500h bit 23 must be set to 1b for the entire time that
Frame Buffer Compression is enabled.

v2: Ville suggested to enable it back when disabling fbc to avoid wasting
    power.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 3 +++
 drivers/gpu/drm/i915/intel_pm.c | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2340bc2..2ef0292 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -863,6 +863,9 @@
 					     _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 25dd054..5914d75 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -242,6 +242,11 @@ 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 */
+		    I915_WRITE(HSW_CLKGATE_DISABLE_PART_1,
+			       ~HSW_DPFC_GATING_DISABLE);
+
 		DRM_DEBUG_KMS("disabled FBC\n");
 	}
 }
@@ -283,6 +288,8 @@ static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 	/* WaFbcAsynchFlipDisableFbcQueue */
 	I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc->pipe),
 		   HSW_BYPASS_FBC_QUEUE);
+	/* WaFbcDisableDpfcClockGating */
+	I915_WRITE(HSW_CLKGATE_DISABLE_PART_1, HSW_DPFC_GATING_DISABLE);
 
 	if (obj->fence_reg != I915_FENCE_REG_NONE) {
 		I915_WRITE(SNB_DPFC_CTL_SA,
-- 
1.8.1.4

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

Re: [PATCH] drm/i915: Enable FBC at Haswell.

[Chris Wilson]

On Mon, Apr 15, 2013 at 08:56:28PM -0300, Rodrigo Vivi wrote:
> This patch introduce Frame Buffer Compression (FBC) support for HSW.
> It adds a new function haswell_enable_fbc to avoid getting
> ironlake_enable_fbc messed with many IS_HASWELL checks.
> 
> v2: Fixes from Ville.
>      	*  Fix Plane. FBC is tied to primary plane A in HSW
>     	*  Fix DPFC initial write to avoid let trash on the register.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |  1 +
>  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c | 46 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0cfc778..88fd6fb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -291,6 +291,7 @@ static const struct intel_device_info intel_haswell_m_info = {
>  	GEN7_FEATURES,
>  	.is_haswell = 1,
>  	.is_mobile = 1,
> +	.has_fbc = 1,
>  };
>  
>  static const struct pci_device_id pciidlist[] = {		/* aka */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5e91fbb..cb8e213 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -849,6 +849,12 @@
>  #define   SNB_CPU_FENCE_ENABLE	(1<<29)
>  #define DPFC_CPU_FENCE_OFFSET	0x100104
>  
> +/* Framebuffer compression for Haswell */
> +#define HSW_FBC_RT_BASE			0x7020
> +#define   HSW_FBC_RT_BASE_ADDR_SHIFT	12
> +
> +#define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
> +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)
>  
>  /*
>   * GPIO regs
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 27f94cd..f3b35c4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -253,6 +253,45 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
>  	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
>  }
>  
> +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->fb;
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	unsigned long stall_watermark = 200;
> +
> +	/* FBC is tied to primary plane A in HSW */
> +	if (intel_crtc->plane != PLANE_A) {
> +	    dev_priv->no_fbc_reason = FBC_BAD_PLANE;
> +	    return;
> +	}
Already checked in intel_update_fbc()

Otherwise looks fine.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Ville Syrjälä]

On Mon, Apr 15, 2013 at 06:14:46PM -0300, Rodrigo Vivi wrote:
> On Wed, Apr 10, 2013 at 5:18 AM, Ville Syrjälä <
> ville.syrjala@linux.intel.com> wrote:
> 
> > On Tue, Apr 09, 2013 at 03:13:10PM -0300, Rodrigo Vivi wrote:
> > > On Tue, Apr 9, 2013 at 5:35 AM, Ville Syrjälä <
> > ville.syrjala@linux.intel.com
> > > > wrote:
> > >
> > > > On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:
> > > > > This patch introduce Frame Buffer Compression (FBC) support for HSW.
> > > > > It adds a new function haswell_enable_fbc to avoid getting
> > > > > ironlake_enable_fbc messed with many IS_HASWELL checks.
> > > > >
> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.c |  1 +
> > > > >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
> > > > >  drivers/gpu/drm/i915/intel_pm.c | 44
> > > > ++++++++++++++++++++++++++++++++++++++++-
> > > > >  3 files changed, 50 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > > index 0cfc778..88fd6fb 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > @@ -291,6 +291,7 @@ static const struct intel_device_info
> > > > intel_haswell_m_info = {
> > > > >       GEN7_FEATURES,
> > > > >       .is_haswell = 1,
> > > > >       .is_mobile = 1,
> > > > > +     .has_fbc = 1,
> > > > >  };
> > > > >
> > > > >  static const struct pci_device_id pciidlist[] = {            /* aka
> > */
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 5e91fbb..cb8e213 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -849,6 +849,12 @@
> > > > >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)
> > > > >  #define DPFC_CPU_FENCE_OFFSET        0x100104
> > > > >
> > > > > +/* Framebuffer compression for Haswell */
> > > > > +#define HSW_FBC_RT_BASE                      0x7020
> > > > > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12
> > > > > +
> > > > > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)
> > > > > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)
> > > > >
> > > > >  /*
> > > > >   * GPIO regs
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > index 27f94cd..94e1c3a 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct
> > drm_device
> > > > *dev)
> > > > >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> > > > >  }
> > > > >
> > > > > +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long
> > > > interval)
> > > > > +{
> > > > > +     struct drm_device *dev = crtc->dev;
> > > > > +     struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > +     struct drm_framebuffer *fb = crtc->fb;
> > > > > +     struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > > > > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > > > > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > > +     int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA :
> > > > DPFC_CTL_PLANEB;
> > > > > +     unsigned long stall_watermark = 200;
> > > > > +     u32 dpfc_ctl;
> > > > > +
> > > > > +     dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> > > > > +     dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
> > > >
> > > > Accroding to BSpec FBC is always tied to plane A. Bit 30 is MBZ.
> > > >
> > >
> > > Yeah, you are right. I'm going to add a verification at  begining like:
> > >      if (intel_crtc->plane != PLANE_A) {
> > >     dev_priv->no_fbc_reason = FBC_BAD_PLANE;
> > >     return;
> > > }
> > >
> > >
> > > > Maybe fix up plane C FBC support for IVB while you're poking at the
> > > > general direction?
> > > >
> > >
> > > Actually I wasn't trying general directions since I splited it out. It
> > was
> > > just bad copy and paste actually.
> >
> > I'm not a fan of copy pasting code and letting the old code paths rot.
> >
> > > >
> > > > > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
> > > >
> > > > The CPU fence field must be written with 0. SNB/IVB could do with the
> > > > same fix.
> > > >
> > >
> > > Where did you get this restriction for HSW?
> >
> > BSpec.
> >
> 
> Are you talking about bit 28 of 43208h DevHSW?

Bits 0:3 of the same register.

> I couldn't find this restriction anywhere.
> Besides that, setting it to 0 made me experience bugs like missing some
> small screen updates. I mean, when it is 0 I missed many "****" when typing
> my login password.
> When it is set FBC works fine.

This is what BSpec is telling us to program:

FBC_CTL
 28 = ?
 0:3 = 0

DPFC_CONTROL_SA
 29 = 1
 0:4 = fence number

So the FBC_CTL bit 28 is not clear. BSpec doesn't say whether it should
be 0 or 1. Did you try both values for that bit?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 5766 bytes --]

Yeah, this makes sense. Yes, I tested many times here, with 0 at bit 28 I
always got that bug with missing updates, In the way it is it always worked
fine.


On Tue, Apr 16, 2013 at 7:28 AM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> On Mon, Apr 15, 2013 at 06:14:46PM -0300, Rodrigo Vivi wrote:
> > On Wed, Apr 10, 2013 at 5:18 AM, Ville Syrjälä <
> > ville.syrjala@linux.intel.com> wrote:
> >
> > > On Tue, Apr 09, 2013 at 03:13:10PM -0300, Rodrigo Vivi wrote:
> > > > On Tue, Apr 9, 2013 at 5:35 AM, Ville Syrjälä <
> > > ville.syrjala@linux.intel.com
> > > > > wrote:
> > > >
> > > > > On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:
> > > > > > This patch introduce Frame Buffer Compression (FBC) support for
> HSW.
> > > > > > It adds a new function haswell_enable_fbc to avoid getting
> > > > > > ironlake_enable_fbc messed with many IS_HASWELL checks.
> > > > > >
> > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_drv.c |  1 +
> > > > > >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
> > > > > >  drivers/gpu/drm/i915/intel_pm.c | 44
> > > > > ++++++++++++++++++++++++++++++++++++++++-
> > > > > >  3 files changed, 50 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > index 0cfc778..88fd6fb 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > @@ -291,6 +291,7 @@ static const struct intel_device_info
> > > > > intel_haswell_m_info = {
> > > > > >       GEN7_FEATURES,
> > > > > >       .is_haswell = 1,
> > > > > >       .is_mobile = 1,
> > > > > > +     .has_fbc = 1,
> > > > > >  };
> > > > > >
> > > > > >  static const struct pci_device_id pciidlist[] = {            /*
> aka
> > > */
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 5e91fbb..cb8e213 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -849,6 +849,12 @@
> > > > > >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)
> > > > > >  #define DPFC_CPU_FENCE_OFFSET        0x100104
> > > > > >
> > > > > > +/* Framebuffer compression for Haswell */
> > > > > > +#define HSW_FBC_RT_BASE                      0x7020
> > > > > > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12
> > > > > > +
> > > > > > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)
> > > > > > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)
> > > > > >
> > > > > >  /*
> > > > > >   * GPIO regs
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index 27f94cd..94e1c3a 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct
> > > drm_device
> > > > > *dev)
> > > > > >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> > > > > >  }
> > > > > >
> > > > > > +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned
> long
> > > > > interval)
> > > > > > +{
> > > > > > +     struct drm_device *dev = crtc->dev;
> > > > > > +     struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > +     struct drm_framebuffer *fb = crtc->fb;
> > > > > > +     struct intel_framebuffer *intel_fb =
> to_intel_framebuffer(fb);
> > > > > > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > > > > > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > > > +     int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA :
> > > > > DPFC_CTL_PLANEB;
> > > > > > +     unsigned long stall_watermark = 200;
> > > > > > +     u32 dpfc_ctl;
> > > > > > +
> > > > > > +     dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> > > > > > +     dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
> > > > >
> > > > > Accroding to BSpec FBC is always tied to plane A. Bit 30 is MBZ.
> > > > >
> > > >
> > > > Yeah, you are right. I'm going to add a verification at  begining
> like:
> > > >      if (intel_crtc->plane != PLANE_A) {
> > > >     dev_priv->no_fbc_reason = FBC_BAD_PLANE;
> > > >     return;
> > > > }
> > > >
> > > >
> > > > > Maybe fix up plane C FBC support for IVB while you're poking at the
> > > > > general direction?
> > > > >
> > > >
> > > > Actually I wasn't trying general directions since I splited it out.
> It
> > > was
> > > > just bad copy and paste actually.
> > >
> > > I'm not a fan of copy pasting code and letting the old code paths rot.
> > >
> > > > >
> > > > > > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
> > > > >
> > > > > The CPU fence field must be written with 0. SNB/IVB could do with
> the
> > > > > same fix.
> > > > >
> > > >
> > > > Where did you get this restriction for HSW?
> > >
> > > BSpec.
> > >
> >
> > Are you talking about bit 28 of 43208h DevHSW?
>
> Bits 0:3 of the same register.
>
> > I couldn't find this restriction anywhere.
> > Besides that, setting it to 0 made me experience bugs like missing some
> > small screen updates. I mean, when it is 0 I missed many "****" when
> typing
> > my login password.
> > When it is set FBC works fine.
>
> This is what BSpec is telling us to program:
>
> FBC_CTL
>  28 = ?
>  0:3 = 0
>
> DPFC_CONTROL_SA
>  29 = 1
>  0:4 = fence number
>
> So the FBC_CTL bit 28 is not clear. BSpec doesn't say whether it should
> be 0 or 1. Did you try both values for that bit?
>
> --
> Ville Syrjälä
> Intel OTC
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 8231 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

This patch introduce Frame Buffer Compression (FBC) support for HSW.
It adds a new function haswell_enable_fbc to avoid getting
ironlake_enable_fbc messed with many IS_HASWELL checks.

v2: Fixes from Ville.
     	*  Fix Plane. FBC is tied to primary plane A in HSW
    	*  Fix DPFC initial write to avoid let trash on the register.

v3: Checking for bad plane on intel_update_fbc() as Chris suggested.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  1 +
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_pm.c | 43 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0cfc778..88fd6fb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -291,6 +291,7 @@ static const struct intel_device_info intel_haswell_m_info = {
 	GEN7_FEATURES,
 	.is_haswell = 1,
 	.is_mobile = 1,
+	.has_fbc = 1,
 };
 
 static const struct pci_device_id pciidlist[] = {		/* aka */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5e91fbb..cb8e213 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -849,6 +849,12 @@
 #define   SNB_CPU_FENCE_ENABLE	(1<<29)
 #define DPFC_CPU_FENCE_OFFSET	0x100104
 
+/* Framebuffer compression for Haswell */
+#define HSW_FBC_RT_BASE			0x7020
+#define   HSW_FBC_RT_BASE_ADDR_SHIFT	12
+
+#define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
+#define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)
 
 /*
  * GPIO regs
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 27f94cd..a88ae3c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -253,6 +253,39 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
 	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
+static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_framebuffer *fb = crtc->fb;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	unsigned long stall_watermark = 200;
+
+	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
+		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
+		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
+	I915_WRITE(HSW_FBC_RT_BASE,
+		   obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
+		   ILK_FBC_RT_VALID);
+
+	I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
+		   (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg) |
+		   HSW_DPFC_CTL_DISABLE_SLB_INIT);
+
+	if (obj->fence_reg != I915_FENCE_REG_NONE) {
+		I915_WRITE(SNB_DPFC_CTL_SA,
+			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
+		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
+	} else
+		I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
+
+	sandybridge_blit_fbc_update(dev);
+
+	DRM_DEBUG_KMS("enabled fbc on plane A\n");
+}
+
 bool intel_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -460,7 +493,8 @@ void intel_update_fbc(struct drm_device *dev)
 		dev_priv->no_fbc_reason = FBC_MODE_TOO_LARGE;
 		goto out_disable;
 	}
-	if ((IS_I915GM(dev) || IS_I945GM(dev)) && intel_crtc->plane != 0) {
+	if ((IS_I915GM(dev) || IS_I945GM(dev) || IS_HASWELL(dev))
+	    && intel_crtc->plane != 0) {
 		DRM_DEBUG_KMS("plane not 0, disabling compression\n");
 		dev_priv->no_fbc_reason = FBC_BAD_PLANE;
 		goto out_disable;
@@ -4158,7 +4192,12 @@ void intel_init_pm(struct drm_device *dev)
 	if (I915_HAS_FBC(dev)) {
 		if (HAS_PCH_SPLIT(dev)) {
 			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
-			dev_priv->display.enable_fbc = ironlake_enable_fbc;
+			if (IS_HASWELL(dev))
+				dev_priv->display.enable_fbc =
+					haswell_enable_fbc;
+			else
+				dev_priv->display.enable_fbc =
+					ironlake_enable_fbc;
 			dev_priv->display.disable_fbc = ironlake_disable_fbc;
 		} else if (IS_GM45(dev)) {
 			dev_priv->display.fbc_enabled = g4x_fbc_enabled;
-- 
1.8.1.4

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

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Ville Syrjälä]

On Tue, Apr 16, 2013 at 10:23:17AM -0300, Rodrigo Vivi wrote:
> Yeah, this makes sense. Yes, I tested many times here, with 0 at bit 28 I
> always got that bug with missing updates, In the way it is it always worked
> fine.

So did you actually test with this config?

FBC_CTL
28 = 1
0:3 = 0

DPFC_CONTROL_SA
29 = 1
0:4 = fence number

Oh, and for this test you should make sure fence reg != 0, so that
we can find out for sure whether the FBC_CTL bits 0:3 have some real
effect.

> On Tue, Apr 16, 2013 at 7:28 AM, Ville Syrjälä <
> ville.syrjala@linux.intel.com> wrote:
> 
> > On Mon, Apr 15, 2013 at 06:14:46PM -0300, Rodrigo Vivi wrote:
> > > On Wed, Apr 10, 2013 at 5:18 AM, Ville Syrjälä <
> > > ville.syrjala@linux.intel.com> wrote:
> > >
> > > > On Tue, Apr 09, 2013 at 03:13:10PM -0300, Rodrigo Vivi wrote:
> > > > > On Tue, Apr 9, 2013 at 5:35 AM, Ville Syrjälä <
> > > > ville.syrjala@linux.intel.com
> > > > > > wrote:
> > > > >
> > > > > > On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:
> > > > > > > This patch introduce Frame Buffer Compression (FBC) support for
> > HSW.
> > > > > > > It adds a new function haswell_enable_fbc to avoid getting
> > > > > > > ironlake_enable_fbc messed with many IS_HASWELL checks.
> > > > > > >
> > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/i915_drv.c |  1 +
> > > > > > >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
> > > > > > >  drivers/gpu/drm/i915/intel_pm.c | 44
> > > > > > ++++++++++++++++++++++++++++++++++++++++-
> > > > > > >  3 files changed, 50 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > index 0cfc778..88fd6fb 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > @@ -291,6 +291,7 @@ static const struct intel_device_info
> > > > > > intel_haswell_m_info = {
> > > > > > >       GEN7_FEATURES,
> > > > > > >       .is_haswell = 1,
> > > > > > >       .is_mobile = 1,
> > > > > > > +     .has_fbc = 1,
> > > > > > >  };
> > > > > > >
> > > > > > >  static const struct pci_device_id pciidlist[] = {            /*
> > aka
> > > > */
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > index 5e91fbb..cb8e213 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > @@ -849,6 +849,12 @@
> > > > > > >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)
> > > > > > >  #define DPFC_CPU_FENCE_OFFSET        0x100104
> > > > > > >
> > > > > > > +/* Framebuffer compression for Haswell */
> > > > > > > +#define HSW_FBC_RT_BASE                      0x7020
> > > > > > > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12
> > > > > > > +
> > > > > > > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)
> > > > > > > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)
> > > > > > >
> > > > > > >  /*
> > > > > > >   * GPIO regs
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > index 27f94cd..94e1c3a 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct
> > > > drm_device
> > > > > > *dev)
> > > > > > >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> > > > > > >  }
> > > > > > >
> > > > > > > +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned
> > long
> > > > > > interval)
> > > > > > > +{
> > > > > > > +     struct drm_device *dev = crtc->dev;
> > > > > > > +     struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > > +     struct drm_framebuffer *fb = crtc->fb;
> > > > > > > +     struct intel_framebuffer *intel_fb =
> > to_intel_framebuffer(fb);
> > > > > > > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > > > > > > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > > > > +     int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA :
> > > > > > DPFC_CTL_PLANEB;
> > > > > > > +     unsigned long stall_watermark = 200;
> > > > > > > +     u32 dpfc_ctl;
> > > > > > > +
> > > > > > > +     dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> > > > > > > +     dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
> > > > > >
> > > > > > Accroding to BSpec FBC is always tied to plane A. Bit 30 is MBZ.
> > > > > >
> > > > >
> > > > > Yeah, you are right. I'm going to add a verification at  begining
> > like:
> > > > >      if (intel_crtc->plane != PLANE_A) {
> > > > >     dev_priv->no_fbc_reason = FBC_BAD_PLANE;
> > > > >     return;
> > > > > }
> > > > >
> > > > >
> > > > > > Maybe fix up plane C FBC support for IVB while you're poking at the
> > > > > > general direction?
> > > > > >
> > > > >
> > > > > Actually I wasn't trying general directions since I splited it out.
> > It
> > > > was
> > > > > just bad copy and paste actually.
> > > >
> > > > I'm not a fan of copy pasting code and letting the old code paths rot.
> > > >
> > > > > >
> > > > > > > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
> > > > > >
> > > > > > The CPU fence field must be written with 0. SNB/IVB could do with
> > the
> > > > > > same fix.
> > > > > >
> > > > >
> > > > > Where did you get this restriction for HSW?
> > > >
> > > > BSpec.
> > > >
> > >
> > > Are you talking about bit 28 of 43208h DevHSW?
> >
> > Bits 0:3 of the same register.
> >
> > > I couldn't find this restriction anywhere.
> > > Besides that, setting it to 0 made me experience bugs like missing some
> > > small screen updates. I mean, when it is 0 I missed many "****" when
> > typing
> > > my login password.
> > > When it is set FBC works fine.
> >
> > This is what BSpec is telling us to program:
> >
> > FBC_CTL
> >  28 = ?
> >  0:3 = 0
> >
> > DPFC_CONTROL_SA
> >  29 = 1
> >  0:4 = fence number
> >
> > So the FBC_CTL bit 28 is not clear. BSpec doesn't say whether it should
> > be 0 or 1. Did you try both values for that bit?
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/3] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 7157 bytes --]

uhm, got your point... I'm getting exactly this values, because fence
number is 0 here,
so it is a coincidence and I should remove  obj->fence_reg of FBC_CTL set,
right?



On Tue, Apr 16, 2013 at 10:37 AM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> On Tue, Apr 16, 2013 at 10:23:17AM -0300, Rodrigo Vivi wrote:
> > Yeah, this makes sense. Yes, I tested many times here, with 0 at bit 28 I
> > always got that bug with missing updates, In the way it is it always
> worked
> > fine.
>
> So did you actually test with this config?
>
> FBC_CTL
> 28 = 1
> 0:3 = 0
>
> DPFC_CONTROL_SA
> 29 = 1
> 0:4 = fence number
>
> Oh, and for this test you should make sure fence reg != 0, so that
> we can find out for sure whether the FBC_CTL bits 0:3 have some real
> effect.
>
> > On Tue, Apr 16, 2013 at 7:28 AM, Ville Syrjälä <
> > ville.syrjala@linux.intel.com> wrote:
> >
> > > On Mon, Apr 15, 2013 at 06:14:46PM -0300, Rodrigo Vivi wrote:
> > > > On Wed, Apr 10, 2013 at 5:18 AM, Ville Syrjälä <
> > > > ville.syrjala@linux.intel.com> wrote:
> > > >
> > > > > On Tue, Apr 09, 2013 at 03:13:10PM -0300, Rodrigo Vivi wrote:
> > > > > > On Tue, Apr 9, 2013 at 5:35 AM, Ville Syrjälä <
> > > > > ville.syrjala@linux.intel.com
> > > > > > > wrote:
> > > > > >
> > > > > > > On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:
> > > > > > > > This patch introduce Frame Buffer Compression (FBC) support
> for
> > > HSW.
> > > > > > > > It adds a new function haswell_enable_fbc to avoid getting
> > > > > > > > ironlake_enable_fbc messed with many IS_HASWELL checks.
> > > > > > > >
> > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/i915_drv.c |  1 +
> > > > > > > >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
> > > > > > > >  drivers/gpu/drm/i915/intel_pm.c | 44
> > > > > > > ++++++++++++++++++++++++++++++++++++++++-
> > > > > > > >  3 files changed, 50 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > > index 0cfc778..88fd6fb 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > > > > @@ -291,6 +291,7 @@ static const struct intel_device_info
> > > > > > > intel_haswell_m_info = {
> > > > > > > >       GEN7_FEATURES,
> > > > > > > >       .is_haswell = 1,
> > > > > > > >       .is_mobile = 1,
> > > > > > > > +     .has_fbc = 1,
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  static const struct pci_device_id pciidlist[] = {
>  /*
> > > aka
> > > > > */
> > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > index 5e91fbb..cb8e213 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > @@ -849,6 +849,12 @@
> > > > > > > >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)
> > > > > > > >  #define DPFC_CPU_FENCE_OFFSET        0x100104
> > > > > > > >
> > > > > > > > +/* Framebuffer compression for Haswell */
> > > > > > > > +#define HSW_FBC_RT_BASE                      0x7020
> > > > > > > > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12
> > > > > > > > +
> > > > > > > > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)
> > > > > > > > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)
> > > > > > > >
> > > > > > > >  /*
> > > > > > > >   * GPIO regs
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > > index 27f94cd..94e1c3a 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > > @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct
> > > > > drm_device
> > > > > > > *dev)
> > > > > > > >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static void haswell_enable_fbc(struct drm_crtc *crtc,
> unsigned
> > > long
> > > > > > > interval)
> > > > > > > > +{
> > > > > > > > +     struct drm_device *dev = crtc->dev;
> > > > > > > > +     struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > > > +     struct drm_framebuffer *fb = crtc->fb;
> > > > > > > > +     struct intel_framebuffer *intel_fb =
> > > to_intel_framebuffer(fb);
> > > > > > > > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > > > > > > > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > > > > > +     int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA :
> > > > > > > DPFC_CTL_PLANEB;
> > > > > > > > +     unsigned long stall_watermark = 200;
> > > > > > > > +     u32 dpfc_ctl;
> > > > > > > > +
> > > > > > > > +     dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> > > > > > > > +     dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);
> > > > > > >
> > > > > > > Accroding to BSpec FBC is always tied to plane A. Bit 30 is
> MBZ.
> > > > > > >
> > > > > >
> > > > > > Yeah, you are right. I'm going to add a verification at  begining
> > > like:
> > > > > >      if (intel_crtc->plane != PLANE_A) {
> > > > > >     dev_priv->no_fbc_reason = FBC_BAD_PLANE;
> > > > > >     return;
> > > > > > }
> > > > > >
> > > > > >
> > > > > > > Maybe fix up plane C FBC support for IVB while you're poking
> at the
> > > > > > > general direction?
> > > > > > >
> > > > > >
> > > > > > Actually I wasn't trying general directions since I splited it
> out.
> > > It
> > > > > was
> > > > > > just bad copy and paste actually.
> > > > >
> > > > > I'm not a fan of copy pasting code and letting the old code paths
> rot.
> > > > >
> > > > > > >
> > > > > > > > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);
> > > > > > >
> > > > > > > The CPU fence field must be written with 0. SNB/IVB could do
> with
> > > the
> > > > > > > same fix.
> > > > > > >
> > > > > >
> > > > > > Where did you get this restriction for HSW?
> > > > >
> > > > > BSpec.
> > > > >
> > > >
> > > > Are you talking about bit 28 of 43208h DevHSW?
> > >
> > > Bits 0:3 of the same register.
> > >
> > > > I couldn't find this restriction anywhere.
> > > > Besides that, setting it to 0 made me experience bugs like missing
> some
> > > > small screen updates. I mean, when it is 0 I missed many "****" when
> > > typing
> > > > my login password.
> > > > When it is set FBC works fine.
> > >
> > > This is what BSpec is telling us to program:
> > >
> > > FBC_CTL
> > >  28 = ?
> > >  0:3 = 0
> > >
> > > DPFC_CONTROL_SA
> > >  29 = 1
> > >  0:4 = fence number
> > >
> > > So the FBC_CTL bit 28 is not clear. BSpec doesn't say whether it should
> > > be 0 or 1. Did you try both values for that bit?
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > >
> >
> >
> >
> > --
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
>
> --
> Ville Syrjälä
> Intel OTC
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 10763 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

This patch introduce Frame Buffer Compression (FBC) support for HSW.
It adds a new function haswell_enable_fbc to avoid getting
ironlake_enable_fbc messed with many IS_HASWELL checks.

v2: Fixes from Ville.
     	*  Fix Plane. FBC is tied to primary plane A in HSW
    	*  Fix DPFC initial write to avoid let trash on the register.

v3: Checking for bad plane on intel_update_fbc() as Chris suggested.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  1 +
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_pm.c | 43 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9ebe895..ba0935d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -314,6 +314,7 @@ static const struct intel_device_info intel_haswell_m_info = {
 	GEN7_FEATURES,
 	.is_haswell = 1,
 	.is_mobile = 1,
+	.has_fbc = 1,
 };
 
 static const struct pci_device_id pciidlist[] = {		/* aka */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 077d40f..b9725ba 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -857,6 +857,12 @@
 #define   SNB_CPU_FENCE_ENABLE	(1<<29)
 #define DPFC_CPU_FENCE_OFFSET	0x100104
 
+/* Framebuffer compression for Haswell */
+#define HSW_FBC_RT_BASE			0x7020
+#define   HSW_FBC_RT_BASE_ADDR_SHIFT	12
+
+#define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
+#define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)
 
 /*
  * GPIO regs
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f747cb0..21885eb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -253,6 +253,39 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
 	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
+static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_framebuffer *fb = crtc->fb;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	unsigned long stall_watermark = 200;
+
+	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
+		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
+		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
+	I915_WRITE(HSW_FBC_RT_BASE,
+		   obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
+		   ILK_FBC_RT_VALID);
+
+	I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
+		   (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg) |
+		   HSW_DPFC_CTL_DISABLE_SLB_INIT);
+
+	if (obj->fence_reg != I915_FENCE_REG_NONE) {
+		I915_WRITE(SNB_DPFC_CTL_SA,
+			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
+		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
+	} else
+		I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
+
+	sandybridge_blit_fbc_update(dev);
+
+	DRM_DEBUG_KMS("enabled fbc on plane A\n");
+}
+
 bool intel_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -460,7 +493,8 @@ void intel_update_fbc(struct drm_device *dev)
 		dev_priv->no_fbc_reason = FBC_MODE_TOO_LARGE;
 		goto out_disable;
 	}
-	if ((IS_I915GM(dev) || IS_I945GM(dev)) && intel_crtc->plane != 0) {
+	if ((IS_I915GM(dev) || IS_I945GM(dev) || IS_HASWELL(dev))
+	    && intel_crtc->plane != 0) {
 		DRM_DEBUG_KMS("plane not 0, disabling compression\n");
 		dev_priv->no_fbc_reason = FBC_BAD_PLANE;
 		goto out_disable;
@@ -4180,7 +4214,12 @@ void intel_init_pm(struct drm_device *dev)
 	if (I915_HAS_FBC(dev)) {
 		if (HAS_PCH_SPLIT(dev)) {
 			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
-			dev_priv->display.enable_fbc = ironlake_enable_fbc;
+			if (IS_HASWELL(dev))
+				dev_priv->display.enable_fbc =
+					haswell_enable_fbc;
+			else
+				dev_priv->display.enable_fbc =
+					ironlake_enable_fbc;
 			dev_priv->display.disable_fbc = ironlake_disable_fbc;
 		} else if (IS_GM45(dev)) {
 			dev_priv->display.fbc_enabled = g4x_fbc_enabled;
-- 
1.8.1.4

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

[PATCH] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

This patch introduce Frame Buffer Compression (FBC) support for HSW.
It adds a new function haswell_enable_fbc to avoid getting
ironlake_enable_fbc messed with many IS_HASWELL checks.

v2: Fixes from Ville.
     	*  Fix Plane. FBC is tied to primary plane A in HSW
    	*  Fix DPFC initial write to avoid let trash on the register.

v3: Checking for bad plane on intel_update_fbc() as Chris suggested.

v4: Ville pointed out that according to BSpec FBC_CTL bits 0:3 must be 0.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.c |  1 +
 drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
 drivers/gpu/drm/i915/intel_pm.c | 42 +++++++++++++++++++++++++++++++++++++++--
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9ebe895..ba0935d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -314,6 +314,7 @@ static const struct intel_device_info intel_haswell_m_info = {
 	GEN7_FEATURES,
 	.is_haswell = 1,
 	.is_mobile = 1,
+	.has_fbc = 1,
 };
 
 static const struct pci_device_id pciidlist[] = {		/* aka */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 077d40f..b9725ba 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -857,6 +857,12 @@
 #define   SNB_CPU_FENCE_ENABLE	(1<<29)
 #define DPFC_CPU_FENCE_OFFSET	0x100104
 
+/* Framebuffer compression for Haswell */
+#define HSW_FBC_RT_BASE			0x7020
+#define   HSW_FBC_RT_BASE_ADDR_SHIFT	12
+
+#define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
+#define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)
 
 /*
  * GPIO regs
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f747cb0..7b20fc5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -253,6 +253,38 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
 	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
 }
 
+static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_framebuffer *fb = crtc->fb;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	unsigned long stall_watermark = 200;
+
+	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
+		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
+		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
+	I915_WRITE(HSW_FBC_RT_BASE,
+		   obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
+		   ILK_FBC_RT_VALID);
+
+	I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
+		   HSW_DPFC_CTL_FENCE_EN | HSW_DPFC_CTL_DISABLE_SLB_INIT);
+
+	if (obj->fence_reg != I915_FENCE_REG_NONE) {
+		I915_WRITE(SNB_DPFC_CTL_SA,
+			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
+		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
+	} else
+		I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
+
+	sandybridge_blit_fbc_update(dev);
+
+	DRM_DEBUG_KMS("enabled fbc on plane A\n");
+}
+
 bool intel_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -460,7 +492,8 @@ void intel_update_fbc(struct drm_device *dev)
 		dev_priv->no_fbc_reason = FBC_MODE_TOO_LARGE;
 		goto out_disable;
 	}
-	if ((IS_I915GM(dev) || IS_I945GM(dev)) && intel_crtc->plane != 0) {
+	if ((IS_I915GM(dev) || IS_I945GM(dev) || IS_HASWELL(dev))
+	    && intel_crtc->plane != 0) {
 		DRM_DEBUG_KMS("plane not 0, disabling compression\n");
 		dev_priv->no_fbc_reason = FBC_BAD_PLANE;
 		goto out_disable;
@@ -4180,7 +4213,12 @@ void intel_init_pm(struct drm_device *dev)
 	if (I915_HAS_FBC(dev)) {
 		if (HAS_PCH_SPLIT(dev)) {
 			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
-			dev_priv->display.enable_fbc = ironlake_enable_fbc;
+			if (IS_HASWELL(dev))
+				dev_priv->display.enable_fbc =
+					haswell_enable_fbc;
+			else
+				dev_priv->display.enable_fbc =
+					ironlake_enable_fbc;
 			dev_priv->display.disable_fbc = ironlake_disable_fbc;
 		} else if (IS_GM45(dev)) {
 			dev_priv->display.fbc_enabled = g4x_fbc_enabled;
-- 
1.8.1.4

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

Re: [PATCH] drm/i915: Enable FBC at Haswell.

[Ville Syrjälä]

On Tue, Apr 16, 2013 at 01:33:44PM -0300, Rodrigo Vivi wrote:
> This patch introduce Frame Buffer Compression (FBC) support for HSW.
> It adds a new function haswell_enable_fbc to avoid getting
> ironlake_enable_fbc messed with many IS_HASWELL checks.
> 
> v2: Fixes from Ville.
>      	*  Fix Plane. FBC is tied to primary plane A in HSW
>     	*  Fix DPFC initial write to avoid let trash on the register.
> 
> v3: Checking for bad plane on intel_update_fbc() as Chris suggested.
> 
> v4: Ville pointed out that according to BSpec FBC_CTL bits 0:3 must be 0.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |  1 +
>  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c | 42 +++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9ebe895..ba0935d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -314,6 +314,7 @@ static const struct intel_device_info intel_haswell_m_info = {
>  	GEN7_FEATURES,
>  	.is_haswell = 1,
>  	.is_mobile = 1,
> +	.has_fbc = 1,
>  };
>  
>  static const struct pci_device_id pciidlist[] = {		/* aka */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 077d40f..b9725ba 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -857,6 +857,12 @@
>  #define   SNB_CPU_FENCE_ENABLE	(1<<29)
>  #define DPFC_CPU_FENCE_OFFSET	0x100104
>  
> +/* Framebuffer compression for Haswell */
> +#define HSW_FBC_RT_BASE			0x7020
> +#define   HSW_FBC_RT_BASE_ADDR_SHIFT	12
> +
> +#define   HSW_DPFC_CTL_FENCE_EN		(1<<28)
> +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT	(1<<15)

These registers/bits already exists on IVB. All the registers you set
seem to be in line with IVB BSpec, so I think you just need to to
s/haswell/gen7/ everywhere, and then we get FBC for IVB for free.

Well, the only real work left would be adding plane B and C FBC
support on IVB.

The new DPFC_CTL bits should also probably be grouped with the rest of
the ILK_DPFC_CONTROL bits, otherwise it's difficult to know which
register they belong to.

>  
>  /*
>   * GPIO regs
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f747cb0..7b20fc5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -253,6 +253,38 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
>  	return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
>  }
>  
> +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_framebuffer *fb = crtc->fb;
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	unsigned long stall_watermark = 200;
> +
> +	I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
> +		   (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
> +		   (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
> +	I915_WRITE(HSW_FBC_RT_BASE,
> +		   obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
> +		   ILK_FBC_RT_VALID);
> +
> +	I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
> +		   HSW_DPFC_CTL_FENCE_EN | HSW_DPFC_CTL_DISABLE_SLB_INIT);
> +
> +	if (obj->fence_reg != I915_FENCE_REG_NONE) {

Is there actually a case that we wouldn't have a fence? I think we
always reserve a fence for scanout even though we wouldn't really
need it on gen4+ (or for untiled on gen2-3). At least we don't have any
sanity checks for fence_reg in the ironlake_enable_fbc().

But if this can happen, then shouldn't we also clear the
"fence enable" bit from ILK_DPFC_CONTROL?

> +		I915_WRITE(SNB_DPFC_CTL_SA,
> +			   SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> +		I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> +	} else
> +		I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
> +
> +	sandybridge_blit_fbc_update(dev);
> +
> +	DRM_DEBUG_KMS("enabled fbc on plane A\n");
> +}
> +
>  bool intel_fbc_enabled(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -460,7 +492,8 @@ void intel_update_fbc(struct drm_device *dev)
>  		dev_priv->no_fbc_reason = FBC_MODE_TOO_LARGE;
>  		goto out_disable;
>  	}
> -	if ((IS_I915GM(dev) || IS_I945GM(dev)) && intel_crtc->plane != 0) {
> +	if ((IS_I915GM(dev) || IS_I945GM(dev) || IS_HASWELL(dev))
> +	    && intel_crtc->plane != 0) {
>  		DRM_DEBUG_KMS("plane not 0, disabling compression\n");
>  		dev_priv->no_fbc_reason = FBC_BAD_PLANE;
>  		goto out_disable;
> @@ -4180,7 +4213,12 @@ void intel_init_pm(struct drm_device *dev)
>  	if (I915_HAS_FBC(dev)) {
>  		if (HAS_PCH_SPLIT(dev)) {
>  			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
> -			dev_priv->display.enable_fbc = ironlake_enable_fbc;
> +			if (IS_HASWELL(dev))
> +				dev_priv->display.enable_fbc =
> +					haswell_enable_fbc;
> +			else
> +				dev_priv->display.enable_fbc =
> +					ironlake_enable_fbc;
>  			dev_priv->display.disable_fbc = ironlake_disable_fbc;
>  		} else if (IS_GM45(dev)) {
>  			dev_priv->display.fbc_enabled = g4x_fbc_enabled;
> -- 
> 1.8.1.4

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Enable FBC at Haswell.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 7080 bytes --]

On Tue, Apr 16, 2013 at 2:49 PM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> On Tue, Apr 16, 2013 at 01:33:44PM -0300, Rodrigo Vivi wrote:
> > This patch introduce Frame Buffer Compression (FBC) support for HSW.
> > It adds a new function haswell_enable_fbc to avoid getting
> > ironlake_enable_fbc messed with many IS_HASWELL checks.
> >
> > v2: Fixes from Ville.
> >       *  Fix Plane. FBC is tied to primary plane A in HSW
> >       *  Fix DPFC initial write to avoid let trash on the register.
> >
> > v3: Checking for bad plane on intel_update_fbc() as Chris suggested.
> >
> > v4: Ville pointed out that according to BSpec FBC_CTL bits 0:3 must be 0.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++
> >  drivers/gpu/drm/i915/intel_pm.c | 42
> +++++++++++++++++++++++++++++++++++++++--
> >  3 files changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> > index 9ebe895..ba0935d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -314,6 +314,7 @@ static const struct intel_device_info
> intel_haswell_m_info = {
> >       GEN7_FEATURES,
> >       .is_haswell = 1,
> >       .is_mobile = 1,
> > +     .has_fbc = 1,
> >  };
> >
> >  static const struct pci_device_id pciidlist[] = {            /* aka */
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index 077d40f..b9725ba 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -857,6 +857,12 @@
> >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)
> >  #define DPFC_CPU_FENCE_OFFSET        0x100104
> >
> > +/* Framebuffer compression for Haswell */
> > +#define HSW_FBC_RT_BASE                      0x7020
> > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12
> > +
> > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)
> > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)
>
> These registers/bits already exists on IVB. All the registers you set
> seem to be in line with IVB BSpec, so I think you just need to to
> s/haswell/gen7/ everywhere, and then we get FBC for IVB for free.
>
> Well, the only real work left would be adding plane B and C FBC
> support on IVB.
>

I implemented here the ivb version, tested and it didn't worked.
So I believe we can go on with HSW names and change it in the future if we
can really get fbc working on ivb.
What do you think?


>
> The new DPFC_CTL bits should also probably be grouped with the rest of
> the ILK_DPFC_CONTROL bits, otherwise it's difficult to know which
> register they belong to.
>

I just didn't do this because it was already organized by platforms. But it
makes sense. coming on v5.


>
> >
> >  /*
> >   * GPIO regs
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> > index f747cb0..7b20fc5 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -253,6 +253,38 @@ static bool ironlake_fbc_enabled(struct drm_device
> *dev)
> >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> >  }
> >
> > +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long
> interval)
> > +{
> > +     struct drm_device *dev = crtc->dev;
> > +     struct drm_i915_private *dev_priv = dev->dev_private;
> > +     struct drm_framebuffer *fb = crtc->fb;
> > +     struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +     unsigned long stall_watermark = 200;
> > +
> > +     I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
> > +                (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
> > +                (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
> > +     I915_WRITE(HSW_FBC_RT_BASE,
> > +                obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |
> > +                ILK_FBC_RT_VALID);
> > +
> > +     I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIMIT_1X |
> > +                HSW_DPFC_CTL_FENCE_EN | HSW_DPFC_CTL_DISABLE_SLB_INIT);
> > +
> > +     if (obj->fence_reg != I915_FENCE_REG_NONE) {
>
> Is there actually a case that we wouldn't have a fence? I think we
> always reserve a fence for scanout even though we wouldn't really
> need it on gen4+ (or for untiled on gen2-3). At least we don't have any
> sanity checks for fence_reg in the ironlake_enable_fbc().
>

Yeah, you are right. I don't see any case without the fence. So I'm going
to remove this check and else block in v5.


>
> But if this can happen, then shouldn't we also clear the
> "fence enable" bit from ILK_DPFC_CONTROL?


> > +             I915_WRITE(SNB_DPFC_CTL_SA,
> > +                        SNB_CPU_FENCE_ENABLE | obj->fence_reg);
> > +             I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);
> > +     } else
> > +             I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);
> > +
> > +     sandybridge_blit_fbc_update(dev);
> > +
> > +     DRM_DEBUG_KMS("enabled fbc on plane A\n");
> > +}
> > +
> >  bool intel_fbc_enabled(struct drm_device *dev)
> >  {
> >       struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -460,7 +492,8 @@ void intel_update_fbc(struct drm_device *dev)
> >               dev_priv->no_fbc_reason = FBC_MODE_TOO_LARGE;
> >               goto out_disable;
> >       }
> > -     if ((IS_I915GM(dev) || IS_I945GM(dev)) && intel_crtc->plane != 0) {
> > +     if ((IS_I915GM(dev) || IS_I945GM(dev) || IS_HASWELL(dev))
> > +         && intel_crtc->plane != 0) {
> >               DRM_DEBUG_KMS("plane not 0, disabling compression\n");
> >               dev_priv->no_fbc_reason = FBC_BAD_PLANE;
> >               goto out_disable;
> > @@ -4180,7 +4213,12 @@ void intel_init_pm(struct drm_device *dev)
> >       if (I915_HAS_FBC(dev)) {
> >               if (HAS_PCH_SPLIT(dev)) {
> >                       dev_priv->display.fbc_enabled =
> ironlake_fbc_enabled;
> > -                     dev_priv->display.enable_fbc = ironlake_enable_fbc;
> > +                     if (IS_HASWELL(dev))
> > +                             dev_priv->display.enable_fbc =
> > +                                     haswell_enable_fbc;
> > +                     else
> > +                             dev_priv->display.enable_fbc =
> > +                                     ironlake_enable_fbc;
> >                       dev_priv->display.disable_fbc =
> ironlake_disable_fbc;
> >               } else if (IS_GM45(dev)) {
> >                       dev_priv->display.fbc_enabled = g4x_fbc_enabled;
> > --
> > 1.8.1.4
>
> --
> Ville Syrjälä
> Intel OTC
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[-- Attachment #1.2: Type: text/html, Size: 9239 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Enable FBC at Haswell.

[Chris Wilson]

On Tue, Apr 16, 2013 at 01:33:44PM -0300, Rodrigo Vivi wrote:
> This patch introduce Frame Buffer Compression (FBC) support for HSW.
> It adds a new function haswell_enable_fbc to avoid getting
> ironlake_enable_fbc messed with many IS_HASWELL checks.
> 
> v2: Fixes from Ville.
>      	*  Fix Plane. FBC is tied to primary plane A in HSW
>     	*  Fix DPFC initial write to avoid let trash on the register.
> 
> v3: Checking for bad plane on intel_update_fbc() as Chris suggested.
> 
> v4: Ville pointed out that according to BSpec FBC_CTL bits 0:3 must be 0.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

I'm failing in sanity checking FBC on HSW due to lack of the appropriate
hardware. I still believe we need a cooperative userspace to make best
use of FBC, avoid known limitations and apply required workarounds.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre