[PATCH 9/10] udlfb: explicit dependencies and warnings

[PATCH 9/10] udlfb: explicit dependencies and warnings

[Bernie Thompson]

Specify Kconfig dependencies, and include warnings for building as a module

udlfb is dependent on FB_DEFERRED_IO, FB_SYS_*, and FB_MODE_HELPERS

Because many kernels do not include defio (which cannot be built
as a module), yet users want to be able to build udlfb as a module later,
udlfb has ifdefs and these dependency warnings to help udlfb build with or
without certain dependencies, but also print warnings for any lost function.

Even though this kind of flexibility isn't common, we've gotten feedback
from a significant portion of users that they were frustrated without it.

Signed-off-by: Bernie Thompson <bernie@plugable.com>
---
 drivers/staging/udlfb/Kconfig |   14 ++++++++++----
 drivers/staging/udlfb/udlfb.c |   14 ++++++++++++++
 2 files changed, 24 insertions(+), 4 deletions(-)

Index: linux-next/drivers/staging/udlfb/Kconfig
=================================--- linux-next.orig/drivers/staging/udlfb/Kconfig	2010-02-14 20:21:30.000000000 -0800
+++ linux-next/drivers/staging/udlfb/Kconfig	2010-02-14 20:27:02.000000000 -0800
@@ -1,8 +1,14 @@
 config FB_UDL
 	tristate "Displaylink USB Framebuffer support"
 	depends on FB && USB
+	select FB_MODE_HELPERS
+	select FB_SYS_FILLRECT
+	select FB_SYS_COPYAREA
+	select FB_SYS_IMAGEBLIT
+	select FB_SYS_FOPS
+	select FB_DEFERRED_IO
 	---help---
-	  This is an experimental driver for DisplayLink USB devices
-	  that provides a framebuffer device.  A normal framebuffer can
-	  be used with this driver, or xorg can be run on the device
-	  using it.
+	  This is a kernel framebuffer driver for DisplayLink USB devices.
+	  Supports fbdev clients like xf86-video-fbdev, kdrive, fbi, and
+	  mplayer -vo fbdev. Supports all USB 2.0 era DisplayLink devices.
+	  To compile as a module, choose M here: the module name is udlfb.
Index: linux-next/drivers/staging/udlfb/udlfb.c
=================================--- linux-next.orig/drivers/staging/udlfb/udlfb.c	2010-02-14 20:27:01.000000000 -0800
+++ linux-next/drivers/staging/udlfb/udlfb.c	2010-02-14 20:27:02.000000000 -0800
@@ -56,6 +56,20 @@ static struct usb_device_id id_table[]  };
 MODULE_DEVICE_TABLE(usb, id_table);
 
+#ifndef CONFIG_FB_DEFERRED_IO
+#warning message "kernel FB_DEFFERRED_IO option to support generic fbdev apps"
+#endif
+
+#ifndef CONFIG_FB_SYS_IMAGEBLIT
+#ifndef CONFIG_FB_SYS_IMAGEBLIT_MODULE
+#warning message "FB_SYS_* in kernel or module option to support fb console"
+#endif
+#endif
+
+#ifndef CONFIG_FB_MODE_HELPERS
+#warning message "kernel FB_MODE_HELPERS required. Expect build break"
+#endif
+
 /* dlfb keeps a list of urbs for efficient bulk transfers */
 static void dlfb_urb_completion(struct urb *urb);
 static struct urb *dlfb_get_urb(struct dlfb_data *dev);

Re: [PATCH 9/10] udlfb: explicit dependencies and warnings

[Greg KH]

On Mon, Feb 15, 2010 at 06:46:35AM -0800, Bernie Thompson wrote:
> Specify Kconfig dependencies, and include warnings for building as a module
> 
> udlfb is dependent on FB_DEFERRED_IO, FB_SYS_*, and FB_MODE_HELPERS
> 
> Because many kernels do not include defio (which cannot be built
> as a module), yet users want to be able to build udlfb as a module later,
> udlfb has ifdefs and these dependency warnings to help udlfb build with or
> without certain dependencies, but also print warnings for any lost function.
> 
> Even though this kind of flexibility isn't common, we've gotten feedback
> from a significant portion of users that they were frustrated without it.

This patch adds the warning to the build:
	drivers/staging/udlfb/udlfb.c:65:2: warning: #warning message "FB_SYS_* in kernel or module option to support fb console"

Any ideas?

thanks,

greg k-h

Re: [PATCH 9/10] udlfb: explicit dependencies and warnings

[Bernie Thompson]

Hi Greg,

On Thu, Feb 18, 2010 at 8:01 AM, Greg KH <greg@kroah.com> wrote:
> This patch adds the warning to the build:
>        drivers/staging/udlfb/udlfb.c:65:2: warning: #warning message "FB_SYS_* in kernel or module option to support fb console"

The intent is you'd get that warning when:

* You're building udlfb as a module outside of full kernel rebuild
* You don't have a dependency you need (for full functionality)

If you were doing a full kernel build, Kconfig (as of this patch)
knows the dependencies as should pull them in.

In this case, your kernel headers say you don't have
CONFIG_FB_SYS_IMAGEBLIT (which is part of the "virtual framebuffer
driver" ops set of BLIT, FILL, etc. which has a lot of (IMHO
incorrectly strong) warnings around it in Kconfig not to enable, since
several drivers are now dependent.

That dependency is needed for framebuffer consoles (fbcon) to work
properly in udlfb with the current implementation. This post gives a
little background on the different types of framebuffer clients:
http://plugable.com/2010/01/30/linux-kernel-framebuffer-rendering-apis/

So you either need to recompile your kernel to get this dependency, or
live without framebuffer console functionality (which many people
don't care about -- but some do a lot).

Failing compile completely generated a strong response from some users
-- they didn't want to recompile their whole kernel to get a single
module, if the dependency was for functionality they didn't care
about.

Keeping the #ifdefs but getting rid of the #warnings would mean
silently loosing functionality, which would throw people off later.

So two questions:
* Is there evidence the #warning is getting triggered in a case where
it shouldn't (you actually have the dependency)?
* Does the wording of the #warning need to be better - any suggestions
for future patches?

Someone could also get rid of the dependency on these functions,
(udlfb didn't used to have them), but the old implementation was
actually incorrect -- it didn't handle all required elements of the
interface, e.g. XOR ops -- where the current/new implementation is
100% correct.

Best wishes,
Bernie

Re: [PATCH 9/10] udlfb: explicit dependencies and warnings

[Greg KH]

On Thu, Feb 18, 2010 at 02:49:34PM -0800, Bernie Thompson wrote:
> Hi Greg,
> 
> On Thu, Feb 18, 2010 at 8:01 AM, Greg KH <greg@kroah.com> wrote:
> > This patch adds the warning to the build:
> >        drivers/staging/udlfb/udlfb.c:65:2: warning: #warning message "FB_SYS_* in kernel or module option to support fb console"
> 
> The intent is you'd get that warning when:
> 
> * You're building udlfb as a module outside of full kernel rebuild
> * You don't have a dependency you need (for full functionality)
> 
> If you were doing a full kernel build, Kconfig (as of this patch)
> knows the dependencies as should pull them in.
> 
> In this case, your kernel headers say you don't have
> CONFIG_FB_SYS_IMAGEBLIT (which is part of the "virtual framebuffer
> driver" ops set of BLIT, FILL, etc. which has a lot of (IMHO
> incorrectly strong) warnings around it in Kconfig not to enable, since
> several drivers are now dependent.
> 
> That dependency is needed for framebuffer consoles (fbcon) to work
> properly in udlfb with the current implementation. This post gives a
> little background on the different types of framebuffer clients:
> http://plugable.com/2010/01/30/linux-kernel-framebuffer-rendering-apis/
> 
> So you either need to recompile your kernel to get this dependency, or
> live without framebuffer console functionality (which many people
> don't care about -- but some do a lot).
> 
> Failing compile completely generated a strong response from some users
> -- they didn't want to recompile their whole kernel to get a single
> module, if the dependency was for functionality they didn't care
> about.
> 
> Keeping the #ifdefs but getting rid of the #warnings would mean
> silently loosing functionality, which would throw people off later.
> 
> So two questions:
> * Is there evidence the #warning is getting triggered in a case where
> it shouldn't (you actually have the dependency)?
> * Does the wording of the #warning need to be better - any suggestions
> for future patches?

I must have done something wrong, built only the drivers/staging/udlfb/
directory or something, with a full kernel build I no longer see this
issue.

Very sorry for the noise, and thanks for the full response.

greg k-h