| Message ID | 1376954440-26434-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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>
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
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
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(-)