[PATCH] drm/i915: Add a module option for disabling use of stolen memory

[PATCH] drm/i915: Add a module option for disabling use of stolen memory

[Chris Wilson]

One of the first steps in triaging memory corruption on module load is
to disable stolen. (It helps reduce the impact of the BIOS clobbering
our memory since we allocate our ringbuffers and initial objects from
stolen, if they are clobbered we get an immediate hang and garbage on
screen.) Rather than requesting bug reporters patch their kernel to test
the theory, allow us to disable stolen through a module option.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++
 drivers/gpu/drm/i915/i915_params.c     | 6 ++++++
 drivers/gpu/drm/i915/i915_params.h     | 1 +
 3 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index e9cd82290408..bb8567b1d6a4 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -411,6 +411,11 @@ int i915_gem_init_stolen(struct drm_device *dev)
 	}
 #endif
 
+	if (!i915.enable_stolen) {
+		DRM_INFO("Disabling use of stolen memory at user request.\n");
+		return 0;
+	}
+
 	if (ggtt->stolen_size == 0)
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 573e78723fc5..511528199aec 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -60,6 +60,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
 	.enable_dpcd_backlight = false,
+	.enable_stolen = true,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -119,6 +120,11 @@ MODULE_PARM_DESC(enable_hangcheck,
 	"WARNING: Disabling this can cause system wide hangs. "
 	"(default: true)");
 
+module_param_named(enable_stolen, i915.enable_stolen, bool, 0400);
+MODULE_PARM_DESC(enable_stolen,
+	"Enable use of memory reserved (stolen) by the BIOS for graphics. "
+	"(default: true)");
+
 module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
 MODULE_PARM_DESC(enable_ppgtt,
 	"Override PPGTT usage. "
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 1323261a0cdd..f126decae848 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -63,6 +63,7 @@ struct i915_params {
 	bool nuclear_pageflip;
 	bool enable_dp_mst;
 	bool enable_dpcd_backlight;
+	bool enable_stolen;
 };
 
 extern struct i915_params i915 __read_mostly;
-- 
2.8.1

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

✗ Ro.CI.BAT: failure for drm/i915: Add a module option for disabling use of stolen memory

[Patchwork]

== Series Details ==

Series: drm/i915: Add a module option for disabling use of stolen memory
URL   : https://patchwork.freedesktop.org/series/8579/
State : failure

== Summary ==

Series 8579v1 drm/i915: Add a module option for disabling use of stolen memory
http://patchwork.freedesktop.org/api/1.0/series/8579/revisions/1/mbox

Test drv_module_reload_basic:
                pass       -> SKIP       (ro-snb-i7-2620M)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                fail       -> PASS       (ro-byt-n2820)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                dmesg-warn -> PASS       (ro-hsw-i3-4010u)
                pass       -> SKIP       (fi-skl-i5-6260u)
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                skip       -> PASS       (fi-skl-i5-6260u)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> FAIL       (ro-byt-n2820)
        Subgroup suspend-read-crc-pipe-a:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-bdw-i7-5557u  total:213  pass:201  dwarn:0   dfail:0   fail:0   skip:12 
fi-skl-i5-6260u  total:213  pass:200  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-i7-6700k  total:213  pass:188  dwarn:0   dfail:0   fail:0   skip:25 
fi-snb-i7-2600   total:213  pass:174  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:213  pass:197  dwarn:2   dfail:0   fail:0   skip:14 
ro-bdw-i7-5557U  total:213  pass:197  dwarn:1   dfail:0   fail:0   skip:15 
ro-bdw-i7-5600u  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-bsw-n3050     total:213  pass:172  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:213  pass:173  dwarn:0   dfail:0   fail:3   skip:37 
ro-hsw-i3-4010u  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:213  pass:190  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:213  pass:150  dwarn:0   dfail:0   fail:1   skip:62 
ro-ilk1-i5-650   total:208  pass:150  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:213  pass:181  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:213  pass:185  dwarn:0   dfail:0   fail:0   skip:28 
ro-skl3-i5-6260u total:213  pass:201  dwarn:1   dfail:0   fail:0   skip:11 
ro-snb-i7-2620M  total:213  pass:173  dwarn:0   dfail:0   fail:1   skip:39 
fi-hsw-i7-4770k failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1166/

94fd582 drm-intel-nightly: 2016y-06m-10d-16h-42m-36s UTC integration manifest
c178552 drm/i915: Add a module option for disabling use of stolen memory

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

Re: [PATCH] drm/i915: Add a module option for disabling use of stolen memory

[Jani Nikula]

On Sat, 11 Jun 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> One of the first steps in triaging memory corruption on module load is
> to disable stolen. (It helps reduce the impact of the BIOS clobbering
> our memory since we allocate our ringbuffers and initial objects from
> stolen, if they are clobbered we get an immediate hang and garbage on
> screen.) Rather than requesting bug reporters patch their kernel to test
> the theory, allow us to disable stolen through a module option.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++
>  drivers/gpu/drm/i915/i915_params.c     | 6 ++++++
>  drivers/gpu/drm/i915/i915_params.h     | 1 +
>  3 files changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index e9cd82290408..bb8567b1d6a4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -411,6 +411,11 @@ int i915_gem_init_stolen(struct drm_device *dev)
>  	}
>  #endif
>  
> +	if (!i915.enable_stolen) {
> +		DRM_INFO("Disabling use of stolen memory at user request.\n");
> +		return 0;
> +	}
> +
>  	if (ggtt->stolen_size == 0)
>  		return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 573e78723fc5..511528199aec 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -60,6 +60,7 @@ struct i915_params i915 __read_mostly = {
>  	.enable_dp_mst = true,
>  	.inject_load_failure = 0,
>  	.enable_dpcd_backlight = false,
> +	.enable_stolen = true,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -119,6 +120,11 @@ MODULE_PARM_DESC(enable_hangcheck,
>  	"WARNING: Disabling this can cause system wide hangs. "
>  	"(default: true)");
>  
> +module_param_named(enable_stolen, i915.enable_stolen, bool, 0400);

_unsafe, to make it clear that this is for debugging?

BR,
Jani.

> +MODULE_PARM_DESC(enable_stolen,
> +	"Enable use of memory reserved (stolen) by the BIOS for graphics. "
> +	"(default: true)");
> +
>  module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
>  MODULE_PARM_DESC(enable_ppgtt,
>  	"Override PPGTT usage. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 1323261a0cdd..f126decae848 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -63,6 +63,7 @@ struct i915_params {
>  	bool nuclear_pageflip;
>  	bool enable_dp_mst;
>  	bool enable_dpcd_backlight;
> +	bool enable_stolen;
>  };
>  
>  extern struct i915_params i915 __read_mostly;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add a module option for disabling use of stolen memory

[Chris Wilson]

On Sat, Jun 11, 2016 at 06:22:00PM +0300, Jani Nikula wrote:
> On Sat, 11 Jun 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > One of the first steps in triaging memory corruption on module load is
> > to disable stolen. (It helps reduce the impact of the BIOS clobbering
> > our memory since we allocate our ringbuffers and initial objects from
> > stolen, if they are clobbered we get an immediate hang and garbage on
> > screen.) Rather than requesting bug reporters patch their kernel to test
> > the theory, allow us to disable stolen through a module option.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++
> >  drivers/gpu/drm/i915/i915_params.c     | 6 ++++++
> >  drivers/gpu/drm/i915/i915_params.h     | 1 +
> >  3 files changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index e9cd82290408..bb8567b1d6a4 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -411,6 +411,11 @@ int i915_gem_init_stolen(struct drm_device *dev)
> >  	}
> >  #endif
> >  
> > +	if (!i915.enable_stolen) {
> > +		DRM_INFO("Disabling use of stolen memory at user request.\n");
> > +		return 0;
> > +	}
> > +
> >  	if (ggtt->stolen_size == 0)
> >  		return 0;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index 573e78723fc5..511528199aec 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -60,6 +60,7 @@ struct i915_params i915 __read_mostly = {
> >  	.enable_dp_mst = true,
> >  	.inject_load_failure = 0,
> >  	.enable_dpcd_backlight = false,
> > +	.enable_stolen = true,
> >  };
> >  
> >  module_param_named(modeset, i915.modeset, int, 0400);
> > @@ -119,6 +120,11 @@ MODULE_PARM_DESC(enable_hangcheck,
> >  	"WARNING: Disabling this can cause system wide hangs. "
> >  	"(default: true)");
> >  
> > +module_param_named(enable_stolen, i915.enable_stolen, bool, 0400);
> 
> _unsafe, to make it clear that this is for debugging?

There shouldn't be a negative consequence, some secondary effects but
nothing that I would consider unsafe. It would be the same as tainting
the kernel because of a BIOS setting.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add a module option for disabling use of stolen memory

[Jani Nikula]

On Sat, 11 Jun 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, Jun 11, 2016 at 06:22:00PM +0300, Jani Nikula wrote:
>> On Sat, 11 Jun 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > One of the first steps in triaging memory corruption on module load is
>> > to disable stolen. (It helps reduce the impact of the BIOS clobbering
>> > our memory since we allocate our ringbuffers and initial objects from
>> > stolen, if they are clobbered we get an immediate hang and garbage on
>> > screen.) Rather than requesting bug reporters patch their kernel to test
>> > the theory, allow us to disable stolen through a module option.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++
>> >  drivers/gpu/drm/i915/i915_params.c     | 6 ++++++
>> >  drivers/gpu/drm/i915/i915_params.h     | 1 +
>> >  3 files changed, 12 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> > index e9cd82290408..bb8567b1d6a4 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> > @@ -411,6 +411,11 @@ int i915_gem_init_stolen(struct drm_device *dev)
>> >  	}
>> >  #endif
>> >  
>> > +	if (!i915.enable_stolen) {
>> > +		DRM_INFO("Disabling use of stolen memory at user request.\n");
>> > +		return 0;
>> > +	}
>> > +
>> >  	if (ggtt->stolen_size == 0)
>> >  		return 0;
>> >  
>> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> > index 573e78723fc5..511528199aec 100644
>> > --- a/drivers/gpu/drm/i915/i915_params.c
>> > +++ b/drivers/gpu/drm/i915/i915_params.c
>> > @@ -60,6 +60,7 @@ struct i915_params i915 __read_mostly = {
>> >  	.enable_dp_mst = true,
>> >  	.inject_load_failure = 0,
>> >  	.enable_dpcd_backlight = false,
>> > +	.enable_stolen = true,
>> >  };
>> >  
>> >  module_param_named(modeset, i915.modeset, int, 0400);
>> > @@ -119,6 +120,11 @@ MODULE_PARM_DESC(enable_hangcheck,
>> >  	"WARNING: Disabling this can cause system wide hangs. "
>> >  	"(default: true)");
>> >  
>> > +module_param_named(enable_stolen, i915.enable_stolen, bool, 0400);
>> 
>> _unsafe, to make it clear that this is for debugging?
>
> There shouldn't be a negative consequence, some secondary effects but
> nothing that I would consider unsafe. It would be the same as tainting
> the kernel because of a BIOS setting.

It's not so much about it being unsafe or not, really, but a statement
that this is not ABI and people shouldn't rely on it. If someone works
around an issue in their system by toggling this, removing the option
becomes a regression.

If it were as convenient to add debugfs flags, and you could set and
have them available at module reload time, that's what we'd do. But
instead we have these debug-but-ABI-flags just because it's the
convenient thing to do.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add a module option for disabling use of stolen memory

[Lukas Wunner]

On Sat, Jun 11, 2016 at 09:21:11AM +0100, Chris Wilson wrote:
> One of the first steps in triaging memory corruption on module load is
> to disable stolen. (It helps reduce the impact of the BIOS clobbering
> our memory since we allocate our ringbuffers and initial objects from
> stolen, if they are clobbered we get an immediate hang and garbage on
> screen.) Rather than requesting bug reporters patch their kernel to test
> the theory, allow us to disable stolen through a module option.

Just a quick drive-by observation, intelfb_alloc() calls
i915_gem_object_create_stolen() and if I follow the call chain
in that function it seems to me that dev_priv->mm.stolen_base == 0
isn't checked anywhere, so this looks like something that might break
with i915.enable_stolen=0. But probably that's what you meant with
"secondary effects".

Best regards,

Lukas

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++
>  drivers/gpu/drm/i915/i915_params.c     | 6 ++++++
>  drivers/gpu/drm/i915/i915_params.h     | 1 +
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index e9cd82290408..bb8567b1d6a4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -411,6 +411,11 @@ int i915_gem_init_stolen(struct drm_device *dev)
>  	}
>  #endif
>  
> +	if (!i915.enable_stolen) {
> +		DRM_INFO("Disabling use of stolen memory at user request.\n");
> +		return 0;
> +	}
> +
>  	if (ggtt->stolen_size == 0)
>  		return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 573e78723fc5..511528199aec 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -60,6 +60,7 @@ struct i915_params i915 __read_mostly = {
>  	.enable_dp_mst = true,
>  	.inject_load_failure = 0,
>  	.enable_dpcd_backlight = false,
> +	.enable_stolen = true,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -119,6 +120,11 @@ MODULE_PARM_DESC(enable_hangcheck,
>  	"WARNING: Disabling this can cause system wide hangs. "
>  	"(default: true)");
>  
> +module_param_named(enable_stolen, i915.enable_stolen, bool, 0400);
> +MODULE_PARM_DESC(enable_stolen,
> +	"Enable use of memory reserved (stolen) by the BIOS for graphics. "
> +	"(default: true)");
> +
>  module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
>  MODULE_PARM_DESC(enable_ppgtt,
>  	"Override PPGTT usage. "
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 1323261a0cdd..f126decae848 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -63,6 +63,7 @@ struct i915_params {
>  	bool nuclear_pageflip;
>  	bool enable_dp_mst;
>  	bool enable_dpcd_backlight;
> +	bool enable_stolen;
>  };
>  
>  extern struct i915_params i915 __read_mostly;
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx