[PATCH 1/2] drm/i915: add fbc enable flag, but disable by default

[PATCH 1/2] drm/i915: add fbc enable flag, but disable by default

[Jesse Barnes]

FBC has too many corner cases that we don't currently deal with, so
disable it by default so we can enable more important features like RC6,
which conflicts in some configurations.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    3 +++
 drivers/gpu/drm/i915/i915_drv.c      |    5 ++++-
 drivers/gpu/drm/i915/i915_drv.h      |    2 ++
 drivers/gpu/drm/i915/intel_display.c |    5 +++++
 4 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 52d2306..f3240ee 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1059,6 +1059,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 		case FBC_MULTIPLE_PIPES:
 			seq_printf(m, "multiple pipes are enabled");
 			break;
+		case FBC_MANUAL_DISABLE:
+			seq_printf(m, "manually disabled");
+			break;
 		default:
 			seq_printf(m, "unknown reason");
 		}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f72f9ff..34a4496 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -55,6 +55,9 @@ module_param_named(semaphores, i915_semaphores, int, 0600);
 unsigned int i915_enable_rc6 = 0;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 
+unsigned int i915_enable_fbc = 0;
+module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
+
 unsigned int i915_lvds_downclock = 0;
 module_param_named(lvds_downclock, i915_lvds_downclock, int, 0400);
 
@@ -169,7 +172,7 @@ static const struct intel_device_info intel_ironlake_d_info = {
 static const struct intel_device_info intel_ironlake_m_info = {
 	.gen = 5, .is_mobile = 1,
 	.need_gfx_hws = 1, .has_hotplug = 1,
-	.has_fbc = 0, /* disabled due to buggy hardware */
+	.has_fbc = 1,
 	.has_bsd_ring = 1,
 };
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 354e733..196af79 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -249,6 +249,7 @@ enum no_fbc_reason {
 	FBC_BAD_PLANE, /* fbc not supported on plane */
 	FBC_NOT_TILED, /* buffer not tiled */
 	FBC_MULTIPLE_PIPES, /* more than one pipe active */
+	FBC_MANUAL_DISABLE,
 };
 
 enum intel_pch {
@@ -988,6 +989,7 @@ extern unsigned int i915_lvds_downclock;
 extern unsigned int i915_panel_use_ssc;
 extern int i915_vbt_sdvo_panel_type;
 extern unsigned int i915_enable_rc6;
+extern unsigned int i915_enable_fbc;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f552f59..bd99216 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1731,6 +1731,11 @@ static void intel_update_fbc(struct drm_device *dev)
 	intel_fb = to_intel_framebuffer(fb);
 	obj = intel_fb->obj;
 
+	if (!i915_enable_fbc) {
+		DRM_DEBUG_KMS("fbc disabled manually\n");
+		dev_priv->no_fbc_reason = FBC_MANUAL_DISABLE;
+		goto out_disable;
+	}
 	if (intel_fb->obj->base.size > dev_priv->cfb_size) {
 		DRM_DEBUG_KMS("framebuffer too large, disabling "
 			      "compression\n");
-- 
1.7.4.1

[PATCH 2/2] drm/i915: enable rc6 by default

[Jesse Barnes]

With FBC disabled by default, it should be safe to enable RC6.  So let's
give it a try.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 34a4496..84bacc2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -52,7 +52,7 @@ module_param_named(powersave, i915_powersave, int, 0600);
 unsigned int i915_semaphores = 1;
 module_param_named(semaphores, i915_semaphores, int, 0600);
 
-unsigned int i915_enable_rc6 = 0;
+unsigned int i915_enable_rc6 = 1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 
 unsigned int i915_enable_fbc = 0;
-- 
1.7.4.1

Re: [PATCH 1/2] drm/i915: add fbc enable flag, but disable by default

[Ben Widawsky]

On Thu, May 05, 2011 at 02:42:25PM -0700, Jesse Barnes wrote:
> FBC has too many corner cases that we don't currently deal with, so
> disable it by default so we can enable more important features like RC6,
> which conflicts in some configurations.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |    3 +++
>  drivers/gpu/drm/i915/i915_drv.c      |    5 ++++-
>  drivers/gpu/drm/i915/i915_drv.h      |    2 ++
>  drivers/gpu/drm/i915/intel_display.c |    5 +++++
>  4 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 52d2306..f3240ee 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1059,6 +1059,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
>  		case FBC_MULTIPLE_PIPES:
>  			seq_printf(m, "multiple pipes are enabled");
>  			break;
> +		case FBC_MANUAL_DISABLE:
> +			seq_printf(m, "manually disabled");
> +			break;
>  		default:
>  			seq_printf(m, "unknown reason");
>  		}
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f72f9ff..34a4496 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -55,6 +55,9 @@ module_param_named(semaphores, i915_semaphores, int, 0600);
>  unsigned int i915_enable_rc6 = 0;
>  module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
>  
> +unsigned int i915_enable_fbc = 0;
> +module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
> +
>  unsigned int i915_lvds_downclock = 0;
>  module_param_named(lvds_downclock, i915_lvds_downclock, int, 0400);
>  
> @@ -169,7 +172,7 @@ static const struct intel_device_info intel_ironlake_d_info = {
>  static const struct intel_device_info intel_ironlake_m_info = {
>  	.gen = 5, .is_mobile = 1,
>  	.need_gfx_hws = 1, .has_hotplug = 1,
> -	.has_fbc = 0, /* disabled due to buggy hardware */
> +	.has_fbc = 1,
>  	.has_bsd_ring = 1,
>  };
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 354e733..196af79 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -249,6 +249,7 @@ enum no_fbc_reason {
>  	FBC_BAD_PLANE, /* fbc not supported on plane */
>  	FBC_NOT_TILED, /* buffer not tiled */
>  	FBC_MULTIPLE_PIPES, /* more than one pipe active */
> +	FBC_MANUAL_DISABLE,
>  };
>  
>  enum intel_pch {
> @@ -988,6 +989,7 @@ extern unsigned int i915_lvds_downclock;
>  extern unsigned int i915_panel_use_ssc;
>  extern int i915_vbt_sdvo_panel_type;
>  extern unsigned int i915_enable_rc6;
> +extern unsigned int i915_enable_fbc;
>  
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
>  extern int i915_resume(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f552f59..bd99216 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1731,6 +1731,11 @@ static void intel_update_fbc(struct drm_device *dev)
>  	intel_fb = to_intel_framebuffer(fb);
>  	obj = intel_fb->obj;
>  
> +	if (!i915_enable_fbc) {
> +		DRM_DEBUG_KMS("fbc disabled manually\n");
> +		dev_priv->no_fbc_reason = FBC_MANUAL_DISABLE;
> +		goto out_disable;
> +	}
>  	if (intel_fb->obj->base.size > dev_priv->cfb_size) {
>  		DRM_DEBUG_KMS("framebuffer too large, disabling "
>  			      "compression\n");

bikeshed: I don't know that manual disable is the correct term.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=31742

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

Re: [PATCH 1/2] drm/i915: add fbc enable flag, but disable by default

[Adam Jackson]

On 5/5/11 5:42 PM, Jesse Barnes wrote:
> FBC has too many corner cases that we don't currently deal with, so
> disable it by default so we can enable more important features like RC6,
> which conflicts in some configurations.
>
> Signed-off-by: Jesse Barnes<jbarnes@virtuousgeek.org>

It's a bit weird to ...

> +unsigned int i915_enable_fbc = 0;
> +module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
> +

... disable fbc by default ...

> +	if (!i915_enable_fbc) {
> +		DRM_DEBUG_KMS("fbc disabled manually\n");
> +		dev_priv->no_fbc_reason = FBC_MANUAL_DISABLE;
> +		goto out_disable;
> +	}

... and then claim the user is to blame.

- ajax

Re: [PATCH 2/2] drm/i915: enable rc6 by default

[Ben Widawsky]

On Thu, May 05, 2011 at 02:42:26PM -0700, Jesse Barnes wrote:
> With FBC disabled by default, it should be safe to enable RC6.  So let's
> give it a try.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 34a4496..84bacc2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -52,7 +52,7 @@ module_param_named(powersave, i915_powersave, int, 0600);
>  unsigned int i915_semaphores = 1;
>  module_param_named(semaphores, i915_semaphores, int, 0600);
>  
> -unsigned int i915_enable_rc6 = 0;
> +unsigned int i915_enable_rc6 = 1;
>  module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
>  
>  unsigned int i915_enable_fbc = 0;

I've seen this patch before :). Multiple times actually.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Re: [PATCH 1/2] drm/i915: add fbc enable flag, but disable by default

[Jesse Barnes]

On Thu, 05 May 2011 18:07:43 -0400
Adam Jackson <ajax@redhat.com> wrote:

> On 5/5/11 5:42 PM, Jesse Barnes wrote:
> > FBC has too many corner cases that we don't currently deal with, so
> > disable it by default so we can enable more important features like RC6,
> > which conflicts in some configurations.
> >
> > Signed-off-by: Jesse Barnes<jbarnes@virtuousgeek.org>
> 
> It's a bit weird to ...
> 
> > +unsigned int i915_enable_fbc = 0;
> > +module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
> > +
> 
> ... disable fbc by default ...
> 
> > +	if (!i915_enable_fbc) {
> > +		DRM_DEBUG_KMS("fbc disabled manually\n");
> > +		dev_priv->no_fbc_reason = FBC_MANUAL_DISABLE;
> > +		goto out_disable;
> > +	}
> 
> ... and then claim the user is to blame.

Yeah suppose I could use something like "fbc disabled per module
param".  We just have no way of knowing whether the user passed
i915_enable_fbc = 0 or the default was taken.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: add fbc enable flag, but disable by default

[Jesse Barnes]

On Thu, 05 May 2011 18:07:43 -0400
Adam Jackson <ajax@redhat.com> wrote:

> On 5/5/11 5:42 PM, Jesse Barnes wrote:
> > FBC has too many corner cases that we don't currently deal with, so
> > disable it by default so we can enable more important features like RC6,
> > which conflicts in some configurations.
> >
> > Signed-off-by: Jesse Barnes<jbarnes@virtuousgeek.org>
> 
> It's a bit weird to ...
> 
> > +unsigned int i915_enable_fbc = 0;
> > +module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
> > +
> 
> ... disable fbc by default ...
> 
> > +	if (!i915_enable_fbc) {
> > +		DRM_DEBUG_KMS("fbc disabled manually\n");
> > +		dev_priv->no_fbc_reason = FBC_MANUAL_DISABLE;
> > +		goto out_disable;
> > +	}
> 
> ... and then claim the user is to blame.

Updated patch below.

-- 
Jesse Barnes, Intel Open Source Technology Center

>From da2e71822765c1c80838c17fb63e967419274879 Mon Sep 17 00:00:00 2001
From: Jesse Barnes <jbarnes@virtuousgeek.org>
Date: Thu, 5 May 2011 14:39:26 -0700
Subject: [PATCH] drm/i915: add fbc enable flag, but disable by default

FBC has too many corner cases that we don't currently deal with, so
disable it by default so we can enable more important features like RC6,
which conflicts in some configurations.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |    3 +++
 drivers/gpu/drm/i915/i915_drv.c      |    5 ++++-
 drivers/gpu/drm/i915/i915_drv.h      |    2 ++
 drivers/gpu/drm/i915/intel_display.c |    5 +++++
 4 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 52d2306..1e2e87f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1059,6 +1059,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
 		case FBC_MULTIPLE_PIPES:
 			seq_printf(m, "multiple pipes are enabled");
 			break;
+		case FBC_MODULE_PARAM:
+			seq_printf(m, "disabled per module param (default off)");
+			break;
 		default:
 			seq_printf(m, "unknown reason");
 		}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b2a55d6..26bbb2f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -55,6 +55,9 @@ module_param_named(semaphores, i915_semaphores, int, 0600);
 unsigned int i915_enable_rc6 = 0;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 
+unsigned int i915_enable_fbc = 0;
+module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
+
 unsigned int i915_lvds_downclock = 0;
 module_param_named(lvds_downclock, i915_lvds_downclock, int, 0400);
 
@@ -173,7 +176,7 @@ static const struct intel_device_info intel_ironlake_d_info = {
 static const struct intel_device_info intel_ironlake_m_info = {
 	.gen = 5, .is_mobile = 1,
 	.need_gfx_hws = 1, .has_hotplug = 1,
-	.has_fbc = 0, /* disabled due to buggy hardware */
+	.has_fbc = 1,
 	.has_bsd_ring = 1,
 };
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 02dea46..87e0622 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -251,6 +251,7 @@ enum no_fbc_reason {
 	FBC_BAD_PLANE, /* fbc not supported on plane */
 	FBC_NOT_TILED, /* buffer not tiled */
 	FBC_MULTIPLE_PIPES, /* more than one pipe active */
+	FBC_MODULE_PARAM,
 };
 
 enum intel_pch {
@@ -995,6 +996,7 @@ extern unsigned int i915_lvds_downclock;
 extern unsigned int i915_panel_use_ssc;
 extern int i915_vbt_sdvo_panel_type;
 extern unsigned int i915_enable_rc6;
+extern unsigned int i915_enable_fbc;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6608f4c..3ef0c02 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1731,6 +1731,11 @@ static void intel_update_fbc(struct drm_device *dev)
 	intel_fb = to_intel_framebuffer(fb);
 	obj = intel_fb->obj;
 
+	if (!i915_enable_fbc) {
+		DRM_DEBUG_KMS("fbc disabled per module param (default off)\n");
+		dev_priv->no_fbc_reason = FBC_MODULE_PARAM;
+		goto out_disable;
+	}
 	if (intel_fb->obj->base.size > dev_priv->cfb_size) {
 		DRM_DEBUG_KMS("framebuffer too large, disabling "
 			      "compression\n");
-- 
1.7.4.1