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

[Jani Nikula <jani.nikula@linux.intel.com>]

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