[PATCH] drm/Kconfig: favor n for DRM_I915_PRELIMINARY_HW_SUPPORT

[PATCH] drm/Kconfig: favor n for DRM_I915_PRELIMINARY_HW_SUPPORT

[Ben Widawsky]

We generally don't want people or distros to use this option unless they
know what they're doing. I missed the initial conversation but it's
likely a way for people who have a built-in i915.ko and have no other
way to change the behavior.

As such:
	Set default to n
	Display message for what users should select (N)
	and while there, a small whitespace fix.

Cc Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/Kconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 62a06c7..ad4e369 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -171,12 +171,15 @@ config DRM_I915_KMS
 config DRM_I915_PRELIMINARY_HW_SUPPORT
 	bool "Enable preliminary support for prerelease Intel hardware by default"
 	depends on DRM_I915
+	default n
 	help
 	  Choose this option if you have prerelease Intel hardware and want the
-	  i915 driver to support it by default.  You can enable such support at
+	  i915 driver to support it by default. You can enable such support at
 	  runtime with the module option i915.preliminary_hw_support=1; this
 	  option changes the default for that module option.
 
+	  If in doubt, say "N".
+
 config DRM_MGA
 	tristate "Matrox g200/g400"
 	depends on DRM && PCI
-- 
1.8.3.4

Re: [PATCH] drm/Kconfig: favor n for DRM_I915_PRELIMINARY_HW_SUPPORT

[Damien Lespiau]

On Mon, Aug 19, 2013 at 04:20:40PM -0700, Ben Widawsky wrote:
> We generally don't want people or distros to use this option unless they
> know what they're doing. I missed the initial conversation but it's
> likely a way for people who have a built-in i915.ko and have no other
> way to change the behavior.
 
It's to be able to have a config file to generate kernels supporting
preliminary hardware, without having to always add the command line
parameter or carry a patch or changing the bootloader configuration.

> As such:
> 	Set default to n
> 	Display message for what users should select (N)
> 	and while there, a small whitespace fix.
> 
> Cc Josh Triplett <josh@joshtriplett.org>

Missing the ':' so you didn't end up Ccing Josh.

> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/Kconfig | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 62a06c7..ad4e369 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -171,12 +171,15 @@ config DRM_I915_KMS
>  config DRM_I915_PRELIMINARY_HW_SUPPORT
>  	bool "Enable preliminary support for prerelease Intel hardware by default"
>  	depends on DRM_I915
> +	default n

That's the default, you don't have to make it explicit (but of course it
doesn't hurt). make oldconfig with this new option gives:

  Enable preliminary support for prerelease Intel hardware by default
  (DRM_I915_PRELIMINARY_HW_SUPPORT) [N/y/?] (NEW)

>  	help
>  	  Choose this option if you have prerelease Intel hardware and want the
> -	  i915 driver to support it by default.  You can enable such support at
> +	  i915 driver to support it by default. You can enable such support at

The Kconfig files have both styles, 2 spaces or 1 space after the '.'.

>  	  runtime with the module option i915.preliminary_hw_support=1; this
>  	  option changes the default for that module option.
>  
> +	  If in doubt, say "N".
> +
>  config DRM_MGA
>  	tristate "Matrox g200/g400"
>  	depends on DRM && PCI
> -- 
> 1.8.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/Kconfig: favor n for DRM_I915_PRELIMINARY_HW_SUPPORT

[Ben Widawsky]

On Tue, Aug 20, 2013 at 01:08:22AM +0100, Damien Lespiau wrote:
> On Mon, Aug 19, 2013 at 04:20:40PM -0700, Ben Widawsky wrote:
> > We generally don't want people or distros to use this option unless they
> > know what they're doing. I missed the initial conversation but it's
> > likely a way for people who have a built-in i915.ko and have no other
> > way to change the behavior.
>  
> It's to be able to have a config file to generate kernels supporting
> preliminary hardware, without having to always add the command line
> parameter or carry a patch or changing the bootloader configuration.
> 

Changing default module parameter options is not something we should be
solving with a CONFIG option. It is solvable with modeprobe.conf. The
unsolvable problem is for building in, which is the reason I agree this
patch is necessary at all. (The other being systems which don't have, or
have an ancient version of modprobe, but I don't believe that is the
target audience). Given the recent kernels allow us to break module
parameter "ABI" would be another (albeit less defensible) reason not to
use a CONFIG option. All this is, "whatever" though, since the patch is
already merged.

> > As such:
> > 	Set default to n
> > 	Display message for what users should select (N)
> > 	and while there, a small whitespace fix.
> > 
> > Cc Josh Triplett <josh@joshtriplett.org>
> 
> Missing the ':' so you didn't end up Ccing Josh.

Thanks a lot for catching that. CC'd now.

> 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/Kconfig | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 62a06c7..ad4e369 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -171,12 +171,15 @@ config DRM_I915_KMS
> >  config DRM_I915_PRELIMINARY_HW_SUPPORT
> >  	bool "Enable preliminary support for prerelease Intel hardware by default"
> >  	depends on DRM_I915
> > +	default n
> 
> That's the default, you don't have to make it explicit (but of course it
> doesn't hurt). make oldconfig with this new option gives:
> 
>   Enable preliminary support for prerelease Intel hardware by default
>   (DRM_I915_PRELIMINARY_HW_SUPPORT) [N/y/?] (NEW)
> 

Right. We can drop it, I don't really care.

> >  	help
> >  	  Choose this option if you have prerelease Intel hardware and want the
> > -	  i915 driver to support it by default.  You can enable such support at
> > +	  i915 driver to support it by default. You can enable such support at
> 
> The Kconfig files have both styles, 2 spaces or 1 space after the '.'.

I learned something new here. We can drop this too then.

> 
> >  	  runtime with the module option i915.preliminary_hw_support=1; this
> >  	  option changes the default for that module option.
> >  
> > +	  If in doubt, say "N".
> > +

Heh. You didn't comment on the one part I really wanted to add ;-). The
rest of the patch doesn't matter much to me.

> >  config DRM_MGA
> >  	tristate "Matrox g200/g400"
> >  	depends on DRM && PCI
-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/Kconfig: favor n for DRM_I915_PRELIMINARY_HW_SUPPORT

[Josh Triplett]

On Tue, Aug 20, 2013 at 01:08:22AM +0100, Damien Lespiau wrote:
> On Mon, Aug 19, 2013 at 04:20:40PM -0700, Ben Widawsky wrote:
> > We generally don't want people or distros to use this option unless they
> > know what they're doing. I missed the initial conversation but it's
> > likely a way for people who have a built-in i915.ko and have no other
> > way to change the behavior.
>  
> It's to be able to have a config file to generate kernels supporting
> preliminary hardware, without having to always add the command line
> parameter or carry a patch or changing the bootloader configuration.

Exactly.

> > As such:
> > 	Set default to n
> > 	Display message for what users should select (N)
> > 	and while there, a small whitespace fix.
> > 
> > Cc Josh Triplett <josh@joshtriplett.org>
> 
> Missing the ':' so you didn't end up Ccing Josh.

Thanks for fixing that. :)

> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/Kconfig | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 62a06c7..ad4e369 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -171,12 +171,15 @@ config DRM_I915_KMS
> >  config DRM_I915_PRELIMINARY_HW_SUPPORT
> >  	bool "Enable preliminary support for prerelease Intel hardware by default"
> >  	depends on DRM_I915
> > +	default n
> 
> That's the default, you don't have to make it explicit (but of course it
> doesn't hurt). make oldconfig with this new option gives:
> 
>   Enable preliminary support for prerelease Intel hardware by default
>   (DRM_I915_PRELIMINARY_HW_SUPPORT) [N/y/?] (NEW)

Right.  I definitely would *not* have set this to default y, but default
n is the default default, so it doesn't need stating.

- Josh Triplett

Re: [PATCH] drm/Kconfig: favor n for DRM_I915_PRELIMINARY_HW_SUPPORT

[Josh Triplett]

On Mon, Aug 19, 2013 at 05:25:34PM -0700, Ben Widawsky wrote:
> On Tue, Aug 20, 2013 at 01:08:22AM +0100, Damien Lespiau wrote:
> > On Mon, Aug 19, 2013 at 04:20:40PM -0700, Ben Widawsky wrote:
> > >  	  runtime with the module option i915.preliminary_hw_support=1; this
> > >  	  option changes the default for that module option.
> > >  
> > > +	  If in doubt, say "N".
> > > +
> 
> Heh. You didn't comment on the one part I really wanted to add ;-). The
> rest of the patch doesn't matter much to me.

I'm fine with adding the "If in doubt", yes. :)

- Josh Triplett

Re: [PATCH] drm/Kconfig: favor n for DRM_I915_PRELIMINARY_HW_SUPPORT

[Damien Lespiau]

On Mon, Aug 19, 2013 at 05:25:34PM -0700, Ben Widawsky wrote:
> > >  	  runtime with the module option i915.preliminary_hw_support=1; this
> > >  	  option changes the default for that module option.
> > >  
> > > +	  If in doubt, say "N".
> > > +
> 
> Heh. You didn't comment on the one part I really wanted to add ;-). The
> rest of the patch doesn't matter much to me.

A patch with that hunk has my Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>


-- 
Damien

Re: [PATCH] drm/Kconfig: favor n for DRM_I915_PRELIMINARY_HW_SUPPORT

[Daniel Vetter]

On Tue, Aug 20, 2013 at 1:26 PM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> On Mon, Aug 19, 2013 at 05:25:34PM -0700, Ben Widawsky wrote:
>> > >     runtime with the module option i915.preliminary_hw_support=1; this
>> > >     option changes the default for that module option.
>> > >
>> > > +   If in doubt, say "N".
>> > > +
>>
>> Heh. You didn't comment on the one part I really wanted to add ;-). The
>> rest of the patch doesn't matter much to me.

Added.

> A patch with that hunk has my Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

I already have an r-b tag from you on that patch ... ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch