All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915: extract intel_init_fbc()
@ 2014-09-19 19:04 Paulo Zanoni
  2014-09-19 19:04 ` [PATCH 2/2] drm/i915: add SW tracking to FBC enabling Paulo Zanoni
  2014-09-19 19:36 ` [PATCH 1/2] drm/i915: extract intel_init_fbc() Rodrigo Vivi
  0 siblings, 2 replies; 5+ messages in thread
From: Paulo Zanoni @ 2014-09-19 19:04 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because I plan to expand it a little bit.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1ec3c8f..2ca9fdb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7358,33 +7358,39 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
 	pm_runtime_disable(device);
 }
 
+static void intel_init_fbc(struct drm_i915_private *dev_priv)
+{
+	if (!HAS_FBC(dev_priv))
+		return;
+
+	if (INTEL_INFO(dev_priv)->gen >= 7) {
+		dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
+		dev_priv->display.enable_fbc = gen7_enable_fbc;
+		dev_priv->display.disable_fbc = ironlake_disable_fbc;
+	} else if (INTEL_INFO(dev_priv)->gen >= 5) {
+		dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
+		dev_priv->display.enable_fbc = ironlake_enable_fbc;
+		dev_priv->display.disable_fbc = ironlake_disable_fbc;
+	} else if (IS_GM45(dev_priv)) {
+		dev_priv->display.fbc_enabled = g4x_fbc_enabled;
+		dev_priv->display.enable_fbc = g4x_enable_fbc;
+		dev_priv->display.disable_fbc = g4x_disable_fbc;
+	} else {
+		dev_priv->display.fbc_enabled = i8xx_fbc_enabled;
+		dev_priv->display.enable_fbc = i8xx_enable_fbc;
+		dev_priv->display.disable_fbc = i8xx_disable_fbc;
+
+		/* This value was pulled out of someone's hat */
+		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
+	}
+}
+
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (HAS_FBC(dev)) {
-		if (INTEL_INFO(dev)->gen >= 7) {
-			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
-			dev_priv->display.enable_fbc = gen7_enable_fbc;
-			dev_priv->display.disable_fbc = ironlake_disable_fbc;
-		} else if (INTEL_INFO(dev)->gen >= 5) {
-			dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
-			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;
-			dev_priv->display.enable_fbc = g4x_enable_fbc;
-			dev_priv->display.disable_fbc = g4x_disable_fbc;
-		} else {
-			dev_priv->display.fbc_enabled = i8xx_fbc_enabled;
-			dev_priv->display.enable_fbc = i8xx_enable_fbc;
-			dev_priv->display.disable_fbc = i8xx_disable_fbc;
-
-			/* This value was pulled out of someone's hat */
-			I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
-		}
-	}
+	intel_init_fbc(dev_priv);
 
 	/* For cxsr */
 	if (IS_PINEVIEW(dev))
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 2/2] drm/i915: add SW tracking to FBC enabling
  2014-09-19 19:04 [PATCH 1/2] drm/i915: extract intel_init_fbc() Paulo Zanoni
@ 2014-09-19 19:04 ` Paulo Zanoni
  2014-09-19 19:44   ` Rodrigo Vivi
  2014-09-19 19:36 ` [PATCH 1/2] drm/i915: extract intel_init_fbc() Rodrigo Vivi
  1 sibling, 1 reply; 5+ messages in thread
From: Paulo Zanoni @ 2014-09-19 19:04 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Currently, calling intel_fbc_enabled() will trigger a register read.
And we call it a lot of times, even when FBC is disabled, so saving a
few cycles would be a good thing.

Another reason for this patch is because we currently call
intel_fbc_enabled() while the HW is runtime suspended, so the read
makes no sense and triggers a WARN. This happens even if FBC is
disabled by default. Of course one could argue that we just shouldn't
be calling intel_fbc_enabled() while the driver is runtime suspended,
and I agree that's a good argument, but I still think that the reason
explained in the first paragraph already justifies the patch.
This problem can easily be reproduced with many subtests of
igt/pm_rpm, and it is a regression introduced by:

    commit c5ad011d7d256ecbe173324029e992817194d2b0
    Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
    Date:   Mon Aug 4 03:51:38 2014 -0700
        drm/i915: FBC flush nuke for BDW

Testcase: igt/pm_rpm/cursor (and others)
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  4 ++++
 drivers/gpu/drm/i915/intel_pm.c | 31 ++++++++++++++++++++-----------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5fce16c..999bd57 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -662,6 +662,10 @@ struct i915_fbc {
 
 	bool false_color;
 
+	/* Tracks whether the HW is actually enabled, not whether the feature is
+	 * possible. */
+	bool enabled;
+
 	struct intel_fbc_work {
 		struct delayed_work work;
 		struct drm_crtc *crtc;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2ca9fdb..6b41620 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -71,6 +71,8 @@ static void i8xx_disable_fbc(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 fbc_ctl;
 
+	dev_priv->fbc.enabled = false;
+
 	/* Disable compression */
 	fbc_ctl = I915_READ(FBC_CONTROL);
 	if ((fbc_ctl & FBC_CTL_EN) == 0)
@@ -99,6 +101,8 @@ static void i8xx_enable_fbc(struct drm_crtc *crtc)
 	int i;
 	u32 fbc_ctl;
 
+	dev_priv->fbc.enabled = true;
+
 	cfb_pitch = dev_priv->fbc.size / FBC_LL_SIZE;
 	if (fb->pitches[0] < cfb_pitch)
 		cfb_pitch = fb->pitches[0];
@@ -153,6 +157,8 @@ static void g4x_enable_fbc(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
 
+	dev_priv->fbc.enabled = true;
+
 	dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane) | DPFC_SR_EN;
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
 		dpfc_ctl |= DPFC_CTL_LIMIT_2X;
@@ -173,6 +179,8 @@ static void g4x_disable_fbc(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 dpfc_ctl;
 
+	dev_priv->fbc.enabled = false;
+
 	/* Disable compression */
 	dpfc_ctl = I915_READ(DPFC_CONTROL);
 	if (dpfc_ctl & DPFC_CTL_EN) {
@@ -224,6 +232,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
 
+	dev_priv->fbc.enabled = true;
+
 	dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
 		dev_priv->fbc.threshold++;
@@ -264,6 +274,8 @@ static void ironlake_disable_fbc(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 dpfc_ctl;
 
+	dev_priv->fbc.enabled = false;
+
 	/* Disable compression */
 	dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
 	if (dpfc_ctl & DPFC_CTL_EN) {
@@ -290,6 +302,8 @@ static void gen7_enable_fbc(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 dpfc_ctl;
 
+	dev_priv->fbc.enabled = true;
+
 	dpfc_ctl = IVB_DPFC_CTL_PLANE(intel_crtc->plane);
 	if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
 		dev_priv->fbc.threshold++;
@@ -339,16 +353,7 @@ bool intel_fbc_enabled(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	/* If it wasn't never enabled by kernel parameter or platform default
-	 * we can avoid reading registers so many times in vain
-	 */
-	if (!i915.enable_fbc)
-		return false;
-
-	if (!dev_priv->display.fbc_enabled)
-		return false;
-
-	return dev_priv->display.fbc_enabled(dev);
+	return dev_priv->fbc.enabled;
 }
 
 void gen8_fbc_sw_flush(struct drm_device *dev, u32 value)
@@ -7360,8 +7365,10 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
 
 static void intel_init_fbc(struct drm_i915_private *dev_priv)
 {
-	if (!HAS_FBC(dev_priv))
+	if (!HAS_FBC(dev_priv)) {
+		dev_priv->fbc.enabled = false;
 		return;
+	}
 
 	if (INTEL_INFO(dev_priv)->gen >= 7) {
 		dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
@@ -7383,6 +7390,8 @@ static void intel_init_fbc(struct drm_i915_private *dev_priv)
 		/* This value was pulled out of someone's hat */
 		I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
 	}
+
+	dev_priv->fbc.enabled = dev_priv->display.fbc_enabled(dev_priv->dev);
 }
 
 /* Set up chip specific power management-related functions */
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] drm/i915: extract intel_init_fbc()
  2014-09-19 19:04 [PATCH 1/2] drm/i915: extract intel_init_fbc() Paulo Zanoni
  2014-09-19 19:04 ` [PATCH 2/2] drm/i915: add SW tracking to FBC enabling Paulo Zanoni
@ 2014-09-19 19:36 ` Rodrigo Vivi
  1 sibling, 0 replies; 5+ messages in thread
From: Rodrigo Vivi @ 2014-09-19 19:36 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni


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

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Fri, Sep 19, 2014 at 12:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Because I plan to expand it a little bit.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 50
> +++++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 1ec3c8f..2ca9fdb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7358,33 +7358,39 @@ void intel_fini_runtime_pm(struct drm_i915_private
> *dev_priv)
>         pm_runtime_disable(device);
>  }
>
> +static void intel_init_fbc(struct drm_i915_private *dev_priv)
> +{
> +       if (!HAS_FBC(dev_priv))
> +               return;
> +
> +       if (INTEL_INFO(dev_priv)->gen >= 7) {
> +               dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
> +               dev_priv->display.enable_fbc = gen7_enable_fbc;
> +               dev_priv->display.disable_fbc = ironlake_disable_fbc;
> +       } else if (INTEL_INFO(dev_priv)->gen >= 5) {
> +               dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
> +               dev_priv->display.enable_fbc = ironlake_enable_fbc;
> +               dev_priv->display.disable_fbc = ironlake_disable_fbc;
> +       } else if (IS_GM45(dev_priv)) {
> +               dev_priv->display.fbc_enabled = g4x_fbc_enabled;
> +               dev_priv->display.enable_fbc = g4x_enable_fbc;
> +               dev_priv->display.disable_fbc = g4x_disable_fbc;
> +       } else {
> +               dev_priv->display.fbc_enabled = i8xx_fbc_enabled;
> +               dev_priv->display.enable_fbc = i8xx_enable_fbc;
> +               dev_priv->display.disable_fbc = i8xx_disable_fbc;
> +
> +               /* This value was pulled out of someone's hat */
> +               I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
> +       }
> +}
> +
>  /* Set up chip specific power management-related functions */
>  void intel_init_pm(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> -       if (HAS_FBC(dev)) {
> -               if (INTEL_INFO(dev)->gen >= 7) {
> -                       dev_priv->display.fbc_enabled =
> ironlake_fbc_enabled;
> -                       dev_priv->display.enable_fbc = gen7_enable_fbc;
> -                       dev_priv->display.disable_fbc =
> ironlake_disable_fbc;
> -               } else if (INTEL_INFO(dev)->gen >= 5) {
> -                       dev_priv->display.fbc_enabled =
> ironlake_fbc_enabled;
> -                       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;
> -                       dev_priv->display.enable_fbc = g4x_enable_fbc;
> -                       dev_priv->display.disable_fbc = g4x_disable_fbc;
> -               } else {
> -                       dev_priv->display.fbc_enabled = i8xx_fbc_enabled;
> -                       dev_priv->display.enable_fbc = i8xx_enable_fbc;
> -                       dev_priv->display.disable_fbc = i8xx_disable_fbc;
> -
> -                       /* This value was pulled out of someone's hat */
> -                       I915_WRITE(FBC_CONTROL, 500 <<
> FBC_CTL_INTERVAL_SHIFT);
> -               }
> -       }
> +       intel_init_fbc(dev_priv);
>
>         /* For cxsr */
>         if (IS_PINEVIEW(dev))
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



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

[-- Attachment #1.2: Type: text/html, Size: 5394 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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm/i915: add SW tracking to FBC enabling
  2014-09-19 19:04 ` [PATCH 2/2] drm/i915: add SW tracking to FBC enabling Paulo Zanoni
@ 2014-09-19 19:44   ` Rodrigo Vivi
  2014-09-23  8:29     ` Daniel Vetter
  0 siblings, 1 reply; 5+ messages in thread
From: Rodrigo Vivi @ 2014-09-19 19:44 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni, Rodrigo Vivi


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

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

But I think it would be good now to change fbc_status interface on debugfs
to show the current bit state as well.


On Fri, Sep 19, 2014 at 12:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Currently, calling intel_fbc_enabled() will trigger a register read.
> And we call it a lot of times, even when FBC is disabled, so saving a
> few cycles would be a good thing.
>
> Another reason for this patch is because we currently call
> intel_fbc_enabled() while the HW is runtime suspended, so the read
> makes no sense and triggers a WARN. This happens even if FBC is
> disabled by default. Of course one could argue that we just shouldn't
> be calling intel_fbc_enabled() while the driver is runtime suspended,
> and I agree that's a good argument, but I still think that the reason
> explained in the first paragraph already justifies the patch.
> This problem can easily be reproduced with many subtests of
> igt/pm_rpm, and it is a regression introduced by:
>
>     commit c5ad011d7d256ecbe173324029e992817194d2b0
>     Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
>     Date:   Mon Aug 4 03:51:38 2014 -0700
>         drm/i915: FBC flush nuke for BDW
>
> Testcase: igt/pm_rpm/cursor (and others)
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  4 ++++
>  drivers/gpu/drm/i915/intel_pm.c | 31 ++++++++++++++++++++-----------
>  2 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 5fce16c..999bd57 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -662,6 +662,10 @@ struct i915_fbc {
>
>         bool false_color;
>
> +       /* Tracks whether the HW is actually enabled, not whether the
> feature is
> +        * possible. */
> +       bool enabled;
> +
>         struct intel_fbc_work {
>                 struct delayed_work work;
>                 struct drm_crtc *crtc;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 2ca9fdb..6b41620 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -71,6 +71,8 @@ static void i8xx_disable_fbc(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 fbc_ctl;
>
> +       dev_priv->fbc.enabled = false;
> +
>         /* Disable compression */
>         fbc_ctl = I915_READ(FBC_CONTROL);
>         if ((fbc_ctl & FBC_CTL_EN) == 0)
> @@ -99,6 +101,8 @@ static void i8xx_enable_fbc(struct drm_crtc *crtc)
>         int i;
>         u32 fbc_ctl;
>
> +       dev_priv->fbc.enabled = true;
> +
>         cfb_pitch = dev_priv->fbc.size / FBC_LL_SIZE;
>         if (fb->pitches[0] < cfb_pitch)
>                 cfb_pitch = fb->pitches[0];
> @@ -153,6 +157,8 @@ static void g4x_enable_fbc(struct drm_crtc *crtc)
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>         u32 dpfc_ctl;
>
> +       dev_priv->fbc.enabled = true;
> +
>         dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane) | DPFC_SR_EN;
>         if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
>                 dpfc_ctl |= DPFC_CTL_LIMIT_2X;
> @@ -173,6 +179,8 @@ static void g4x_disable_fbc(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 dpfc_ctl;
>
> +       dev_priv->fbc.enabled = false;
> +
>         /* Disable compression */
>         dpfc_ctl = I915_READ(DPFC_CONTROL);
>         if (dpfc_ctl & DPFC_CTL_EN) {
> @@ -224,6 +232,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc)
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>         u32 dpfc_ctl;
>
> +       dev_priv->fbc.enabled = true;
> +
>         dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
>         if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
>                 dev_priv->fbc.threshold++;
> @@ -264,6 +274,8 @@ static void ironlake_disable_fbc(struct drm_device
> *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 dpfc_ctl;
>
> +       dev_priv->fbc.enabled = false;
> +
>         /* Disable compression */
>         dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
>         if (dpfc_ctl & DPFC_CTL_EN) {
> @@ -290,6 +302,8 @@ static void gen7_enable_fbc(struct drm_crtc *crtc)
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>         u32 dpfc_ctl;
>
> +       dev_priv->fbc.enabled = true;
> +
>         dpfc_ctl = IVB_DPFC_CTL_PLANE(intel_crtc->plane);
>         if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
>                 dev_priv->fbc.threshold++;
> @@ -339,16 +353,7 @@ bool intel_fbc_enabled(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
> -       /* If it wasn't never enabled by kernel parameter or platform
> default
> -        * we can avoid reading registers so many times in vain
> -        */
> -       if (!i915.enable_fbc)
> -               return false;
> -
> -       if (!dev_priv->display.fbc_enabled)
> -               return false;
> -
> -       return dev_priv->display.fbc_enabled(dev);
> +       return dev_priv->fbc.enabled;
>  }
>
>  void gen8_fbc_sw_flush(struct drm_device *dev, u32 value)
> @@ -7360,8 +7365,10 @@ void intel_fini_runtime_pm(struct drm_i915_private
> *dev_priv)
>
>  static void intel_init_fbc(struct drm_i915_private *dev_priv)
>  {
> -       if (!HAS_FBC(dev_priv))
> +       if (!HAS_FBC(dev_priv)) {
> +               dev_priv->fbc.enabled = false;
>                 return;
> +       }
>
>         if (INTEL_INFO(dev_priv)->gen >= 7) {
>                 dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
> @@ -7383,6 +7390,8 @@ static void intel_init_fbc(struct drm_i915_private
> *dev_priv)
>                 /* This value was pulled out of someone's hat */
>                 I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
>         }
> +
> +       dev_priv->fbc.enabled =
> dev_priv->display.fbc_enabled(dev_priv->dev);
>  }
>
>  /* Set up chip specific power management-related functions */
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



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

[-- Attachment #1.2: Type: text/html, Size: 8291 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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm/i915: add SW tracking to FBC enabling
  2014-09-19 19:44   ` Rodrigo Vivi
@ 2014-09-23  8:29     ` Daniel Vetter
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2014-09-23  8:29 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: Rodrigo Vivi, intel-gfx, Paulo Zanoni

On Fri, Sep 19, 2014 at 12:44:08PM -0700, Rodrigo Vivi wrote:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Both patches merged to dinq.

> But I think it would be good now to change fbc_status interface on debugfs
> to show the current bit state as well.

Seconded. Also some locking for fbc would be awesome ;-)

Cheers, Daniel
> 
> 
> On Fri, Sep 19, 2014 at 12:04 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Currently, calling intel_fbc_enabled() will trigger a register read.
> > And we call it a lot of times, even when FBC is disabled, so saving a
> > few cycles would be a good thing.
> >
> > Another reason for this patch is because we currently call
> > intel_fbc_enabled() while the HW is runtime suspended, so the read
> > makes no sense and triggers a WARN. This happens even if FBC is
> > disabled by default. Of course one could argue that we just shouldn't
> > be calling intel_fbc_enabled() while the driver is runtime suspended,
> > and I agree that's a good argument, but I still think that the reason
> > explained in the first paragraph already justifies the patch.
> > This problem can easily be reproduced with many subtests of
> > igt/pm_rpm, and it is a regression introduced by:
> >
> >     commit c5ad011d7d256ecbe173324029e992817194d2b0
> >     Author: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >     Date:   Mon Aug 4 03:51:38 2014 -0700
> >         drm/i915: FBC flush nuke for BDW
> >
> > Testcase: igt/pm_rpm/cursor (and others)
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  4 ++++
> >  drivers/gpu/drm/i915/intel_pm.c | 31 ++++++++++++++++++++-----------
> >  2 files changed, 24 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 5fce16c..999bd57 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -662,6 +662,10 @@ struct i915_fbc {
> >
> >         bool false_color;
> >
> > +       /* Tracks whether the HW is actually enabled, not whether the
> > feature is
> > +        * possible. */
> > +       bool enabled;
> > +
> >         struct intel_fbc_work {
> >                 struct delayed_work work;
> >                 struct drm_crtc *crtc;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 2ca9fdb..6b41620 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -71,6 +71,8 @@ static void i8xx_disable_fbc(struct drm_device *dev)
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         u32 fbc_ctl;
> >
> > +       dev_priv->fbc.enabled = false;
> > +
> >         /* Disable compression */
> >         fbc_ctl = I915_READ(FBC_CONTROL);
> >         if ((fbc_ctl & FBC_CTL_EN) == 0)
> > @@ -99,6 +101,8 @@ static void i8xx_enable_fbc(struct drm_crtc *crtc)
> >         int i;
> >         u32 fbc_ctl;
> >
> > +       dev_priv->fbc.enabled = true;
> > +
> >         cfb_pitch = dev_priv->fbc.size / FBC_LL_SIZE;
> >         if (fb->pitches[0] < cfb_pitch)
> >                 cfb_pitch = fb->pitches[0];
> > @@ -153,6 +157,8 @@ static void g4x_enable_fbc(struct drm_crtc *crtc)
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >         u32 dpfc_ctl;
> >
> > +       dev_priv->fbc.enabled = true;
> > +
> >         dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane) | DPFC_SR_EN;
> >         if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
> >                 dpfc_ctl |= DPFC_CTL_LIMIT_2X;
> > @@ -173,6 +179,8 @@ static void g4x_disable_fbc(struct drm_device *dev)
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         u32 dpfc_ctl;
> >
> > +       dev_priv->fbc.enabled = false;
> > +
> >         /* Disable compression */
> >         dpfc_ctl = I915_READ(DPFC_CONTROL);
> >         if (dpfc_ctl & DPFC_CTL_EN) {
> > @@ -224,6 +232,8 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc)
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >         u32 dpfc_ctl;
> >
> > +       dev_priv->fbc.enabled = true;
> > +
> >         dpfc_ctl = DPFC_CTL_PLANE(intel_crtc->plane);
> >         if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
> >                 dev_priv->fbc.threshold++;
> > @@ -264,6 +274,8 @@ static void ironlake_disable_fbc(struct drm_device
> > *dev)
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >         u32 dpfc_ctl;
> >
> > +       dev_priv->fbc.enabled = false;
> > +
> >         /* Disable compression */
> >         dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> >         if (dpfc_ctl & DPFC_CTL_EN) {
> > @@ -290,6 +302,8 @@ static void gen7_enable_fbc(struct drm_crtc *crtc)
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >         u32 dpfc_ctl;
> >
> > +       dev_priv->fbc.enabled = true;
> > +
> >         dpfc_ctl = IVB_DPFC_CTL_PLANE(intel_crtc->plane);
> >         if (drm_format_plane_cpp(fb->pixel_format, 0) == 2)
> >                 dev_priv->fbc.threshold++;
> > @@ -339,16 +353,7 @@ bool intel_fbc_enabled(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > -       /* If it wasn't never enabled by kernel parameter or platform
> > default
> > -        * we can avoid reading registers so many times in vain
> > -        */
> > -       if (!i915.enable_fbc)
> > -               return false;
> > -
> > -       if (!dev_priv->display.fbc_enabled)
> > -               return false;
> > -
> > -       return dev_priv->display.fbc_enabled(dev);
> > +       return dev_priv->fbc.enabled;
> >  }
> >
> >  void gen8_fbc_sw_flush(struct drm_device *dev, u32 value)
> > @@ -7360,8 +7365,10 @@ void intel_fini_runtime_pm(struct drm_i915_private
> > *dev_priv)
> >
> >  static void intel_init_fbc(struct drm_i915_private *dev_priv)
> >  {
> > -       if (!HAS_FBC(dev_priv))
> > +       if (!HAS_FBC(dev_priv)) {
> > +               dev_priv->fbc.enabled = false;
> >                 return;
> > +       }
> >
> >         if (INTEL_INFO(dev_priv)->gen >= 7) {
> >                 dev_priv->display.fbc_enabled = ironlake_fbc_enabled;
> > @@ -7383,6 +7390,8 @@ static void intel_init_fbc(struct drm_i915_private
> > *dev_priv)
> >                 /* This value was pulled out of someone's hat */
> >                 I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
> >         }
> > +
> > +       dev_priv->fbc.enabled =
> > dev_priv->display.fbc_enabled(dev_priv->dev);
> >  }
> >
> >  /* Set up chip specific power management-related functions */
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

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


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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-23  8:29 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-19 19:04 [PATCH 1/2] drm/i915: extract intel_init_fbc() Paulo Zanoni
2014-09-19 19:04 ` [PATCH 2/2] drm/i915: add SW tracking to FBC enabling Paulo Zanoni
2014-09-19 19:44   ` Rodrigo Vivi
2014-09-23  8:29     ` Daniel Vetter
2014-09-19 19:36 ` [PATCH 1/2] drm/i915: extract intel_init_fbc() Rodrigo Vivi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.