[PATCH 1/2] drm/i915: add module parameter compiler hints

[PATCH 1/2] drm/i915: add module parameter compiler hints

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   24 ++++++++++++------------
 drivers/gpu/drm/i915/i915_drv.h |   20 ++++++++++----------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 649278f..f5ef954 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -37,40 +37,40 @@
 #include <linux/console.h>
 #include "drm_crtc_helper.h"
 
-static int i915_modeset = -1;
+static int i915_modeset __read_mostly = -1;
 module_param_named(modeset, i915_modeset, int, 0400);
 
-unsigned int i915_fbpercrtc = 0;
+unsigned int i915_fbpercrtc __always_unused = 0;
 module_param_named(fbpercrtc, i915_fbpercrtc, int, 0400);
 
-int i915_panel_ignore_lid = 0;
+int i915_panel_ignore_lid __read_mostly = 0;
 module_param_named(panel_ignore_lid, i915_panel_ignore_lid, int, 0600);
 
-unsigned int i915_powersave = 1;
+unsigned int i915_powersave __read_mostly = 1;
 module_param_named(powersave, i915_powersave, int, 0600);
 
-unsigned int i915_semaphores = 0;
+unsigned int i915_semaphores __read_mostly = 0;
 module_param_named(semaphores, i915_semaphores, int, 0600);
 
-unsigned int i915_enable_rc6 = 1;
+unsigned int i915_enable_rc6 __read_mostly = 1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 
-unsigned int i915_enable_fbc = 0;
+unsigned int i915_enable_fbc __read_mostly = 0;
 module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
 
-unsigned int i915_lvds_downclock = 0;
+unsigned int i915_lvds_downclock __read_mostly = 0;
 module_param_named(lvds_downclock, i915_lvds_downclock, int, 0400);
 
-unsigned int i915_panel_use_ssc = 1;
+unsigned int i915_panel_use_ssc __read_mostly = 1;
 module_param_named(lvds_use_ssc, i915_panel_use_ssc, int, 0600);
 
-int i915_vbt_sdvo_panel_type = -1;
+int i915_vbt_sdvo_panel_type __read_mostly = -1;
 module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
 
-static bool i915_try_reset = true;
+static bool i915_try_reset __read_mostly = true;
 module_param_named(reset, i915_try_reset, bool, 0600);
 
-bool i915_enable_hangcheck = true;
+bool i915_enable_hangcheck __read_mostly = true;
 module_param_named(enable_hangcheck, i915_enable_hangcheck, bool, 0644);
 
 static struct drm_driver driver;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4958ce0..7f253b3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -985,16 +985,16 @@ struct drm_i915_file_private {
 
 extern struct drm_ioctl_desc i915_ioctls[];
 extern int i915_max_ioctl;
-extern unsigned int i915_fbpercrtc;
-extern int i915_panel_ignore_lid;
-extern unsigned int i915_powersave;
-extern unsigned int i915_semaphores;
-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 bool i915_enable_hangcheck;
+extern unsigned int i915_fbpercrtc __always_unused;
+extern int i915_panel_ignore_lid __read_mostly;
+extern unsigned int i915_powersave __read_mostly;
+extern unsigned int i915_semaphores __read_mostly;
+extern unsigned int i915_lvds_downclock __read_mostly;
+extern unsigned int i915_panel_use_ssc __read_mostly;
+extern int i915_vbt_sdvo_panel_type __read_mostly;
+extern unsigned int i915_enable_rc6 __read_mostly;
+extern unsigned int i915_enable_fbc __read_mostly;
+extern bool i915_enable_hangcheck __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
-- 
1.7.6

[PATCH 2/2] drm/i915: provide module parameter description

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f5ef954..feac0f9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -39,39 +39,62 @@
 
 static int i915_modeset __read_mostly = -1;
 module_param_named(modeset, i915_modeset, int, 0400);
+MODULE_PARM_DESC(modeset, "Use kernel modesetting (KMS) "
+			  "(0=use DRM_I915_KMS from .config, "
+			  "1=on, "
+			  "-1=force vga console preference (default))");
 
 unsigned int i915_fbpercrtc __always_unused = 0;
 module_param_named(fbpercrtc, i915_fbpercrtc, int, 0400);
 
 int i915_panel_ignore_lid __read_mostly = 0;
 module_param_named(panel_ignore_lid, i915_panel_ignore_lid, int, 0600);
+MODULE_PARM_DESC(panel_ignore_lid, "Override lid status "
+				   "(0=autodetect (default), 1=lid open, -1=lid closed)");
 
 unsigned int i915_powersave __read_mostly = 1;
 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;
 module_param_named(semaphores, i915_semaphores, int, 0600);
+MODULE_PARM_DESC(semaphores, "Use semaphores for inter-ring synchronization "
+			     "(default: false)");
 
 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)");
 
 unsigned int i915_enable_fbc __read_mostly = 0;
 module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
+MODULE_PARM_DESC(i915_enable_fbc, "Enable frame buffer compression for power savings "
+				  "(default: false)");
 
 unsigned int i915_lvds_downclock __read_mostly = 0;
 module_param_named(lvds_downclock, i915_lvds_downclock, int, 0400);
+MODULE_PARM_DESC(lvds_downclock, "Use panel (LVDS/eDP) downclocking for "
+				 "power savings (default: false)");
 
 unsigned int i915_panel_use_ssc __read_mostly = 1;
 module_param_named(lvds_use_ssc, i915_panel_use_ssc, int, 0600);
+MODULE_PARM_DESC(lvds_use_ssc, "Use Spread Spectrum Clock with panels "
+			       "(LVDS/eDP) (default: true)");
 
 int i915_vbt_sdvo_panel_type __read_mostly = -1;
 module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
+MODULE_PARM_DESC(vbt_sdvo_panel_type, "Override selection of SDVO panel mode "
+				      "in the VBT (default: auto)");
 
 static bool i915_try_reset __read_mostly = true;
 module_param_named(reset, i915_try_reset, bool, 0600);
+MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
 
 bool i915_enable_hangcheck __read_mostly = true;
 module_param_named(enable_hangcheck, i915_enable_hangcheck, bool, 0644);
+MODULE_PARM_DESC(enable_hangcheck, "Periodically check GPU activity for "
+				   "detecting hangs (default: true)");
 
 static struct drm_driver driver;
 extern int intel_agp_enabled;
-- 
1.7.6

Re: [PATCH 1/2] drm/i915: add module parameter compiler hints

[Keith Packard]

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

On Wed,  6 Jul 2011 15:14:52 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:


> -static int i915_modeset = -1;
> +static int i915_modeset __read_mostly = -1;

What effect does this have? Performance? Code size? More warnings?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 1/2] drm/i915: add module parameter compiler hints

[Ben Widawsky]

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

On Wed, Jul 06, 2011 at 04:03:14PM -0700, Keith Packard wrote:
> On Wed,  6 Jul 2011 15:14:52 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> 
> > -static int i915_modeset = -1;
> > +static int i915_modeset __read_mostly = -1;
> 
> What effect does this have? Performance? Code size? More warnings?

__read_mostly attempts to put variables in a cachelines to make it more
SMP friendly. I'm not sure if there is a performance impact as a result,
but made this patch based on feedback from Chris. I've seen no
regressions on Nexuiz.

The __always_unused I don't think has any real use other than
documentation. Potentially GCC could do something useful with it, but I
don't think it's enabled anywhere important.

Ben

[-- Attachment #1.2: Type: application/pgp-signature, Size: 490 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 1/2] drm/i915: add module parameter compiler hints

[Keith Packard]

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

On Wed, 6 Jul 2011 16:16:01 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
Non-text part: multipart/signed

> I've seen no regressions on Nexuiz.

'this doesn't seem to hurt any' is hardly a strong recommendation...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 1/2] drm/i915: add module parameter compiler hints

[Ted Phelps]

Ben Widawsky writes:
> On Wed, Jul 06, 2011 at 04:03:14PM -0700, Keith Packard wrote:
> > On Wed,  6 Jul 2011 15:14:52 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> >
> > > -static int i915_modeset =3D -1;
> > > +static int i915_modeset __read_mostly =3D -1;
> >
> > What effect does this have? Performance? Code size? More warnings?
> 
> __read_mostly attempts to put variables in a cachelines to make it more
> SMP friendly. I'm not sure if there is a performance impact as a result,
> but made this patch based on feedback from Chris. I've seen no
> regressions on Nexuiz.

A link to an informative LKML discussion on this topic is here:

    http://lkml.org/lkml/2007/12/17/43

-Ted

Re: [PATCH 1/2] drm/i915: add module parameter compiler hints

[Chris Wilson]

On Wed,  6 Jul 2011 15:14:52 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:

And programmer hints too. They serve as useful documentation on the
expected usage of the variable as well. But maybe that's just me...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: provide module parameter description

[Chris Wilson]

On Wed,  6 Jul 2011 15:14:53 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f5ef954..feac0f9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -39,39 +39,62 @@
>  
>  static int i915_modeset __read_mostly = -1;
>  module_param_named(modeset, i915_modeset, int, 0400);
> +MODULE_PARM_DESC(modeset, "Use kernel modesetting (KMS) "
> +			  "(0=use DRM_I915_KMS from .config, "
> +			  "1=on, "
> +			  "-1=force vga console preference (default))");

I find this alignment slightly odd, and it is disadvantageous later where
it forces a string across two lines that could just be one. Can you try
aligning the start of the description with the bracket on a new line?
i.e. set cino=:0,(0
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: provide module parameter description

[Chris Wilson]

On Wed,  6 Jul 2011 15:14:53 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
>  bool i915_enable_hangcheck __read_mostly = true;
>  module_param_named(enable_hangcheck, i915_enable_hangcheck, bool, 0644);
> +MODULE_PARM_DESC(enable_hangcheck, "Periodically check GPU activity for "
> +				   "detecting hangs (default: true)");

I think we need to stress here that disabling the hangcheck can cause the
system to hang (X, VT switching, etc) following an GPU hang. The
assumption that the hangcheck will prevail and break any waits (and
eventually release the struct_mutex) is deeply ingrained into the code.

The alternate scenario is to wait without the mutex, which then requires
revalidating all state after a wait, effectively we would have to restart
the operation from the being. And introduces unfairness (potential
starvation) into command/resource allocations...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: add module parameter compiler hints

[Jesse Barnes]

On Wed, 06 Jul 2011 17:37:16 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Wed, 6 Jul 2011 16:16:01 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Non-text part: multipart/signed
> 
> > I've seen no regressions on Nexuiz.
> 
> 'this doesn't seem to hurt any' is hardly a strong recommendation...

If anything, using __read_mostly appropriately should *increase*
performance, not decrease it.  But I think we'd have to measure real
hard to notice anything at all for this particular set of changes
(though __read_mostly is definitely the right thing to do).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: add module parameter compiler hints

[Keith Packard]

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

On Wed,  6 Jul 2011 15:14:52 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

This patch series doesn't apply to -next anymore; care to clean it up so
I can merge it in?

(too many -fixes applied for 3.0, I fear)

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 2/2] drm/i915: provide module parameter description

[Chris Wilson]

On Wed, 13 Jul 2011 14:38:18 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/2] drm/i915: provide module parameter description

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ef5cf03..ce045a8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -39,39 +39,68 @@
 
 static int i915_modeset __read_mostly = -1;
 module_param_named(modeset, i915_modeset, int, 0400);
+MODULE_PARM_DESC(modeset,
+		"Use kernel modesetting [KMS] (0=DRM_I915_KMS from .config, "
+		"1=on, -1=force vga console preference [default])");
 
 unsigned int i915_fbpercrtc __always_unused = 0;
 module_param_named(fbpercrtc, i915_fbpercrtc, int, 0400);
 
 int i915_panel_ignore_lid __read_mostly = 0;
 module_param_named(panel_ignore_lid, i915_panel_ignore_lid, int, 0600);
+MODULE_PARM_DESC(panel_ignore_lid,
+		"Override lid status (0=autodetect [default], 1=lid open, "
+		"-1=lid closed)");
 
 unsigned int i915_powersave __read_mostly = 1;
 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;
 module_param_named(semaphores, i915_semaphores, int, 0600);
+MODULE_PARM_DESC(semaphores,
+		"Use semaphores for inter-ring sync (default: false)");
 
 unsigned int i915_enable_rc6 __read_mostly = 0;
 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)");
 
 unsigned int i915_enable_fbc __read_mostly = 1;
 module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
+MODULE_PARM_DESC(i915_enable_fbc,
+		"Enable frame buffer compression for power savings "
+		"(default: false)");
 
 unsigned int i915_lvds_downclock __read_mostly = 0;
 module_param_named(lvds_downclock, i915_lvds_downclock, int, 0400);
+MODULE_PARM_DESC(lvds_downclock,
+		"Use panel (LVDS/eDP) downclocking for power savings "
+		"(default: false)");
 
 unsigned int i915_panel_use_ssc __read_mostly = 1;
 module_param_named(lvds_use_ssc, i915_panel_use_ssc, int, 0600);
+MODULE_PARM_DESC(lvds_use_ssc,
+		"Use Spread Spectrum Clock with panels [LVDS/eDP] "
+		"(default: true)");
 
 int i915_vbt_sdvo_panel_type __read_mostly = -1;
 module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
+MODULE_PARM_DESC(vbt_sdvo_panel_type,
+		"Override selection of SDVO panel mode in the VBT "
+		"(default: auto)");
 
 static bool i915_try_reset __read_mostly = true;
 module_param_named(reset, i915_try_reset, bool, 0600);
+MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
 
 bool i915_enable_hangcheck __read_mostly = true;
 module_param_named(enable_hangcheck, i915_enable_hangcheck, bool, 0644);
+MODULE_PARM_DESC(enable_hangcheck,
+		"Periodically check GPU activity for detecting hangs. "
+		"WARNING: Disabling this can cause system wide hangs. "
+		"(default: true)");
 
 static struct drm_driver driver;
 extern int intel_agp_enabled;
-- 
1.7.6