[PATCH 1/3] drm/i915: support for per-device semaphores settings

[PATCH 1/3] drm/i915: support for per-device semaphores settings

[Eugeni Dodonov]

This allows to enable semaphores by default on devices which support them.

By default, let's enable them on IVB only for now. When DMAR issues on SNB
will be solved, we can enable them on SNB as well.

For IVB, it should fix many hangs and issues.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42696
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=40564
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=41353
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38862
Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |    6 ++++--
 drivers/gpu/drm/i915/i915_drv.h            |    3 +++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 +++++-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index cc531bb..355f1ab 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -58,10 +58,10 @@ module_param_named(powersave, i915_powersave, int, 0600);
 MODULE_PARM_DESC(powersave,
 		"Enable powersavings, fbc, downclocking, etc. (default: true)");
 
-unsigned int i915_semaphores __read_mostly = 0;
+unsigned int i915_semaphores __read_mostly = -1;
 module_param_named(semaphores, i915_semaphores, int, 0600);
 MODULE_PARM_DESC(semaphores,
-		"Use semaphores for inter-ring sync (default: false)");
+		"Use semaphores for inter-ring sync (default: -1 (use per-chip defaults))");
 
 unsigned int i915_enable_rc6 __read_mostly = 0;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
@@ -229,6 +229,7 @@ static const struct intel_device_info intel_ivybridge_d_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
+	.enable_semaphores = 1,
 };
 
 static const struct intel_device_info intel_ivybridge_m_info = {
@@ -237,6 +238,7 @@ static const struct intel_device_info intel_ivybridge_m_info = {
 	.has_fbc = 0,	/* FBC is not enabled on Ivybridge mobile yet */
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
+	.enable_semaphores = 1,
 };
 
 static const struct pci_device_id pciidlist[] = {		/* aka */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 06a37f4..0e819d2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -247,6 +247,7 @@ struct intel_device_info {
 	u8 supports_tv:1;
 	u8 has_bsd_ring:1;
 	u8 has_blt_ring:1;
+	u8 enable_semaphores:1;
 };
 
 enum no_fbc_reason {
@@ -990,6 +991,8 @@ struct drm_i915_file_private {
 #define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
 #define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
 
+#define ENABLE_SEMAPHORES(dev)            (INTEL_INFO(dev)->enable_semaphores)
+
 #include "i915_trace.h"
 
 extern struct drm_ioctl_desc i915_ioctls[];
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3693e83..094ff4c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -753,12 +753,16 @@ i915_gem_execbuffer_sync_rings(struct drm_i915_gem_object *obj,
 	struct intel_ring_buffer *from = obj->ring;
 	u32 seqno;
 	int ret, idx;
+	int enable_semaphores = 0;
 
 	if (from == NULL || to == from)
 		return 0;
 
+	if (i915_semaphores < 0 && ENABLE_SEMAPHORES(obj->base.dev))
+		enable_semaphores = 1;
+
 	/* XXX gpu semaphores are implicated in various hard hangs on SNB */
-	if (INTEL_INFO(obj->base.dev)->gen < 6 || !i915_semaphores)
+	if (INTEL_INFO(obj->base.dev)->gen < 6 || !enable_semaphores)
 		return i915_gem_object_wait_rendering(obj);
 
 	idx = intel_ring_sync_index(from, to);
-- 
1.7.7.3

[PATCH 2/3] drm/i915: allow semaphores on SNB if DMAR is disabled

[Eugeni Dodonov]

Semaphores cause issues when DMAR is enabled. So if we are set to per-chip
default, and we are on SNB, we can enable semaphores as long as SMAR is
disabled.

Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |    2 ++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 +++-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 355f1ab..565725c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -214,6 +214,7 @@ static const struct intel_device_info intel_sandybridge_d_info = {
 	.need_gfx_hws = 1, .has_hotplug = 1,
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
+	.enable_semaphores = 1,
 };
 
 static const struct intel_device_info intel_sandybridge_m_info = {
@@ -222,6 +223,7 @@ static const struct intel_device_info intel_sandybridge_m_info = {
 	.has_fbc = 1,
 	.has_bsd_ring = 1,
 	.has_blt_ring = 1,
+	.enable_semaphores = 1,
 };
 
 static const struct intel_device_info intel_ivybridge_d_info = {
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 094ff4c..0510735 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -32,6 +32,7 @@
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include "intel_drv.h"
+#include <linux/intel-iommu.h>
 
 struct change_domains {
 	uint32_t invalidate_domains;
@@ -758,7 +759,8 @@ i915_gem_execbuffer_sync_rings(struct drm_i915_gem_object *obj,
 	if (from == NULL || to == from)
 		return 0;
 
-	if (i915_semaphores < 0 && ENABLE_SEMAPHORES(obj->base.dev))
+	/* Only enable semaphores if DMAR is disabled */
+	if (i915_semaphores < 0 && ENABLE_SEMAPHORES(obj->base.dev) && dmar_disabled)
 		enable_semaphores = 1;
 
 	/* XXX gpu semaphores are implicated in various hard hangs on SNB */
-- 
1.7.7.3

Re: [PATCH 2/3] drm/i915: allow semaphores on SNB if DMAR is disabled

[Ben Widawsky]

On Wed, Nov 16, 2011 at 10:17:54PM -0200, Eugeni Dodonov wrote:
> Semaphores cause issues when DMAR is enabled. So if we are set to per-chip
> default, and we are on SNB, we can enable semaphores as long as SMAR is
> disabled.
> 
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

See my response to patch 3...

[PATCH 3/3] drm/i915: re-enable rc6 by default when GMAR is disabled

[Eugeni Dodonov]

Most of the rc6-related hangs and major issues were addressed for the past
months.

Let's re-enable it by default to provide a more wider testing, and catch
the remaining problems.

According to tests, enablement of rc6 results in up to +50% improvements
in power usage and battery life, so it certainly would be a nice feature
to have enabled by default.

Also, most of the rc6-related issues seem to came from GMAR, so we only
enable it as long as GMAR is disabled.

CC: Keith Packard <keithp@keithp.com>
CC: Daniel Vetter <daniel.vetter@ffwll.ch>
CC: Jesse Barnes <jbarnes@virtuousgeek.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38567
Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |    2 +-
 drivers/gpu/drm/i915/intel_display.c |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 565725c..337a568 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -63,7 +63,7 @@ module_param_named(semaphores, i915_semaphores, int, 0600);
 MODULE_PARM_DESC(semaphores,
 		"Use semaphores for inter-ring sync (default: -1 (use per-chip defaults))");
 
-unsigned int i915_enable_rc6 __read_mostly = 0;
+unsigned int i915_enable_rc6 __read_mostly = 1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 MODULE_PARM_DESC(i915_enable_rc6,
 		"Enable power-saving render C-state 6 (default: true)");
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 981b1f1..5dd0878 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -31,6 +31,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/vgaarb.h>
+#include <linux/intel-iommu.h>
 #include <drm/drm_edid.h>
 #include "drmP.h"
 #include "intel_drv.h"
@@ -8746,7 +8747,7 @@ void intel_modeset_init(struct drm_device *dev)
 
 void intel_modeset_gem_init(struct drm_device *dev)
 {
-	if (IS_IRONLAKE_M(dev))
+	if (IS_IRONLAKE_M(dev) && dmar_disabled)
 		ironlake_enable_rc6(dev);
 
 	intel_setup_overlay(dev);
-- 
1.7.7.3

Re: [PATCH 3/3] drm/i915: re-enable rc6 by default when GMAR is disabled

[Ben Widawsky]

On Wed, Nov 16, 2011 at 10:17:55PM -0200, Eugeni Dodonov wrote:
> Most of the rc6-related hangs and major issues were addressed for the past
> months.
> 
> Let's re-enable it by default to provide a more wider testing, and catch
> the remaining problems.
> 
> According to tests, enablement of rc6 results in up to +50% improvements
> in power usage and battery life, so it certainly would be a nice feature
> to have enabled by default.
> 
> Also, most of the rc6-related issues seem to came from GMAR, so we only
> enable it as long as GMAR is disabled.
> 
> CC: Keith Packard <keithp@keithp.com>
> CC: Daniel Vetter <daniel.vetter@ffwll.ch>
> CC: Jesse Barnes <jbarnes@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38567
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |    2 +-
>  drivers/gpu/drm/i915/intel_display.c |    3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 565725c..337a568 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -63,7 +63,7 @@ module_param_named(semaphores, i915_semaphores, int, 0600);
>  MODULE_PARM_DESC(semaphores,
>  		"Use semaphores for inter-ring sync (default: -1 (use per-chip defaults))");
>  
> -unsigned int i915_enable_rc6 __read_mostly = 0;
> +unsigned int i915_enable_rc6 __read_mostly = 1;
>  module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
>  MODULE_PARM_DESC(i915_enable_rc6,
>  		"Enable power-saving render C-state 6 (default: true)");
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 981b1f1..5dd0878 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -31,6 +31,7 @@
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
>  #include <linux/vgaarb.h>
> +#include <linux/intel-iommu.h>
>  #include <drm/drm_edid.h>
>  #include "drmP.h"
>  #include "intel_drv.h"
> @@ -8746,7 +8747,7 @@ void intel_modeset_init(struct drm_device *dev)
>  
>  void intel_modeset_gem_init(struct drm_device *dev)
>  {
> -	if (IS_IRONLAKE_M(dev))
> +	if (IS_IRONLAKE_M(dev) && dmar_disabled)
>  		ironlake_enable_rc6(dev);
>  
>  	intel_setup_overlay(dev);
> -- 

This is not sufficient. You need to know that DMAR is compiled in, and
is actually being used.

The variable you want is: !intel_iommu_gfx_mapped

I think I saw Keith say he was sending this patch out on IRC.

Ben

Re: [PATCH 3/3] drm/i915: re-enable rc6 by default when GMAR is disabled

[Eugeni Dodonov]

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

On Wed, Nov 16, 2011 at 22:56, Ben Widawsky <ben@bwidawsk.net> wrote:

> This is not sufficient. You need to know that DMAR is compiled in, and
> is actually being used.
>
> The variable you want is: !intel_iommu_gfx_mapped
>
> I think I saw Keith say he was sending this patch out on IRC.
>

Thanks, makes sense!

I'll wait for Keith patch then to avoid duplicating work.

On a side note, would it make sense to add the .enable_rc6 bits to
i915_drv.c, in a similar way I did for semaphores?

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 870 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: re-enable rc6 by default when GMAR is disabled

[Ben Widawsky]

On Wed, Nov 16, 2011 at 11:02:36PM -0200, Eugeni Dodonov wrote:
> On Wed, Nov 16, 2011 at 22:56, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > This is not sufficient. You need to know that DMAR is compiled in, and
> > is actually being used.
> >
> > The variable you want is: !intel_iommu_gfx_mapped
> >
> > I think I saw Keith say he was sending this patch out on IRC.
> >
> 
> Thanks, makes sense!
> 
> I'll wait for Keith patch then to avoid duplicating work.
> 
> On a side note, would it make sense to add the .enable_rc6 bits to
> i915_drv.c, in a similar way I did for semaphores?

I'm pretty sure we used to have that. In general, I dislike the idea of
device specific parameters until we know exactly why something doesn't
work. It makes it too tempting to just something off instead of
debugging it.
In your patch series I would have changed the check to be
if (GEN7)
	do semaphore stuff
For the first patch, and then for the second patch
if (>= GEN6)
	do semaphore stuff

Instead of changing the module parameter.

But that's just me, and others tend to disagree with me more than most.

Only bikeshedding because you asked :-).

Ben

Re: [PATCH 3/3] drm/i915: re-enable rc6 by default when GMAR is disabled

[Ben Widawsky]

On Wed, 16 Nov 2011 22:11:06 -0800
keith.packard@intel.com wrote:

> On Wed, 16 Nov 2011 16:56:16 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > The variable you want is: !intel_iommu_gfx_mapped
> 
> From what Daniel Vetter said:
> 
> > Last time I've played around with all the combinations, only
> > intel_iommu=off was good enough to prevent the hang.
> > intel_iommu=igd_off still hangs (which is what the current value
> > exported by the dmar code dopends on, iirc). If I remember things
> > correctly, intel_iommu=off also reliably works around issues for all
> > reporters (both semaphores and rc6).
> 
> we need to use dmar_disabled instead of intel_iommu_gfx_mapped. Other
> places in the code use:
> 
>         if (no_iommu || dmar_disabled)
> 

As long as he meant igfx_off, then okay... igd_off didn't immediately register
to me earlier. Sorry for the confusion.

Re: [PATCH 1/3] drm/i915: support for per-device semaphores settings

[Daniel Vetter]

On Thu, Nov 17, 2011 at 01:17, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> This allows to enable semaphores by default on devices which support them.
>
> By default, let's enable them on IVB only for now. When DMAR issues on SNB
> will be solved, we can enable them on SNB as well.
>
> For IVB, it should fix many hangs and issues.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42696
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=40564
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=41353
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38862
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Minor bikeshed comment: I prefer an if chain instead of overloading
intel_info - currently intel_info contains information about the hw
and work-arounds are applied in the code. This way they also stick out
more. I'll try to test this as soon as I get back home.
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch