Re: [PATCH] drm, i915: select framebuffer support automatically

[Dave Airlie]

On Fri, Feb 6, 2009 at 2:56 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
>
> * Eric Anholt <eric@xxxxxxxxxx> wrote:
>
>> On Wed, 2009-02-04 at 19:56 +0100, Ingo Molnar wrote:
>> > * Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>> >
>> > > On Wed, 4 Feb 2009, Norbert Preining wrote:
>> > > >
>> > > > The problem is that if you have a configuration under 2.6.28 without
>> > > > CONFIG_FB and just call make oldconfig, or even make config and don't
>> > > > know that you loose the DRM. And I was using make oldconfig (there is a
>> > > > graphical config?? ;-))
>> > >
>> > > Sure. It's inconvenient, no question about that. I asked the i915 people
>> > > to look into not requiring CONFIG_FB, and I hope they will, but my point
>> > > is that I don't think we can consider "small one-time inconvenience" to be
>> > > a "regression".
>> >
>> > if you mean that as a general principle, there's four very real downsides in
>> > my opinion.
>> >
>> > Firstly, we could have done better (and still can do better), via various
>> > easy and non-intrusive measures:
>> >
>> >    - We could add a runtime warning:
>> >
>> >       for example a WARN_ONCE("please enable CONFIG_DRM_I915 and CONFIG_FB")
>> >       that there's no DRM because CONFIG_FB is not selected and oldconfig
>> >       loses the I915 setting silently - placed in a key DRM ioctl, would
>> >       have gone a long way addressing the issue. Testers do notice kernel
>> >       warnings that pop up when their X gets slow. (This approach might also
>> >       have the added bonus of warning folks who enable the wrong driver for
>> >       the hardware.)

We can't actually do this, if you don't have a DRM driver for the hw,
you don't get any device node or ioctl
to play with. X just loads the module, if it doesn't exist X
complains, nothing we could do about it in the kernel
without adding some wierdass fake drm devices.

>> >
>> >    - Or we could add a more thoughtful Kconfig migration:
>> >
>> >       Rename DRM_I915 to DRM_I915_FB [which it really is now], and keep
>> >       DRM_I915 as a non-interactive migration helper: if set, it
>> >       auto-selects both FB and DRM_I915_FB.
>> >
>> >       While CONFIG_FB is an interactive Kconfig option so a select can be
>> >       dangerous to a correct dependency tree, it seems safe to do in this
>> >       specific case because it seems to be a rather leaf entry with no
>> >       dependencies.

This is the correct answer, the only other option was to allow i915 to
build without modesetting
if FB isn't selected but Intel didn't want to go down that road.

>>
>> I tried select FB.  It's the right thing to do.  It doesn't work.  I
>> posted to the mailing list two weeks ago about the insane dependency
>> chain that kbuild comes up with and fails on when we do this, and got
>> silence.
>
> I mean the patch below.
>
> I have tested it here it works fine and has no dependency problems, nor any
> build breakages on 32-bit or 64-bit x86. When you tried this you probably
> ran into the FB_I810 and FB_INTEL complication - this patch solves that too.
>
> What do you think?
>
>        Ingo
>
> --------------->
> From ca835567dcb7c513a26a4396d86f12848e62de8d Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@xxxxxxx>
> Date: Thu, 5 Feb 2009 16:03:34 +0100
> Subject: [PATCH] drm, i915: select framebuffer support automatically
>
> Migration helper.
>
> The i915 driver recently added a 'depends on FB' rule to its
> Kconfig entry - which silently turns off DRM_I915 if someone
> has a working config but no CONFIG_FB selected, and upgrades
> to the latest upstream kernel.
>
> Norbert Preining reported this problem:
>
>   Bug-Entry   : http://bugzilla.kernel.org/show_bug.cgi?id=12599
>   Subject     : dri /dev node disappeared with 2.6.29-rc1
>
> So change it to "select FB", which auto-selects framebuffer
> support. This way the driver keeps working, regardless of
> whether FB was enabled before or not.
>
> Kconfig select's of interactive options can be problematic to
> dependencies and can cause build breakages - but in this case
> it's safe because it's a leaf entry with no dependencies of its
> own.
>
> ( There is some minor circular dependency fallout as FB_I810
>  and FB_INTEL also used 'depends on FB' constructs - update
>  those to "select FB" too. )
>
> Reported-by: Norbert Preining <preining@xxxxxxxx>
> Signed-off-by: Ingo Molnar <mingo@xxxxxxx>

This looks like the best option to me so I'll suck this patch into my tree.

Thanks Ingo for solving the wierdass depends chain.

Dave.

> ---
>  drivers/gpu/drm/Kconfig |    2 +-
>  drivers/video/Kconfig   |    6 ++++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 5130b72..4be3acb 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -70,7 +70,7 @@ config DRM_I915
>        select FB_CFB_FILLRECT
>        select FB_CFB_COPYAREA
>        select FB_CFB_IMAGEBLIT
> -       depends on FB
> +       select FB
>        tristate "i915 driver"
>        help
>          Choose this option if you have a system that has Intel 830M, 845G,
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index f026770..bf0af66 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -1054,9 +1054,10 @@ config FB_RIVA_BACKLIGHT
>
>  config FB_I810
>        tristate "Intel 810/815 support (EXPERIMENTAL)"
> -       depends on FB && EXPERIMENTAL && PCI && X86_32
> +       depends on EXPERIMENTAL && PCI && X86_32
>        select AGP
>        select AGP_INTEL
> +       select FB
>        select FB_MODE_HELPERS
>        select FB_CFB_FILLRECT
>        select FB_CFB_COPYAREA
> @@ -1119,7 +1120,8 @@ config FB_CARILLO_RANCH
>
>  config FB_INTEL
>        tristate "Intel 830M/845G/852GM/855GM/865G/915G/945G/945GM/965G/965GM support (EXPERIMENTAL)"
> -       depends on FB && EXPERIMENTAL && PCI && X86
> +       depends on EXPERIMENTAL && PCI && X86
> +       select FB
>        select AGP
>        select AGP_INTEL
>        select FB_MODE_HELPERS
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/