Re: [PATCH 2.6.9-rc1-mm1] Disable colour conversion in the CPiA Video Camera driver

Re: [PATCH 2.6.9-rc1-mm1] Disable colour conversion in the CPiA Video Camera driver

[Gerd Knorr]

> Given that colour conversion is not allowed in kernel space, this patch
> disables it in the CPiA driver. The routines implementing the conversions
> can be removed at all by the maintainers of the driver; however, this
> patch is a good starting point and makes someone happy.

Yes, colorspace conversion shouldn't be done by the kernel but by the
applications.  I don't like the idea to just disable them through:

First: there should be a reasonable warning time for the current users.
Some printk message telling them they are using a depricated feature.
Maybe even a insmod option to enable/disable it, with the default being
software conversion disabled.

Second: IMHO it would be a very good idea to port the driver to the v4l2
API before ripping the in-kernel colorspace conversion support.  v4l2
provides a sane API to get a list of supported color formats, whereas
with v4l1 it is dirty trial-and-error + guesswork for the applications.

While thinking about it: due to the v4l1 trial-and-error mess a printk
likely generates alot of false positives, so a insmod option
(+rate-limited printk) is likely the better way to figure how much
userspace software breaks.  Maybe it isn't that much as other drivers
don't offer in-kernel conversion in the first place, so apps already
have to deal with that ...

  Gerd

-- 
return -ENOSIG;

Re: [PATCH 2.6.9-rc1-mm1] Disable colour conversion in the CPiA Video Camera driver

[Luca Risolia]

On Mon, 30 Aug 2004 15:32:05 +0200
Gerd Knorr <kraxel@bytesex.org> wrote:

> First: there should be a reasonable warning time for the current users.
> Some printk message telling them they are using a depricated feature.
> Maybe even a insmod option to enable/disable it, with the default being
> software conversion disabled.
> 
> Second: IMHO it would be a very good idea to port the driver to the v4l2
> API before ripping the in-kernel colorspace conversion support.  v4l2
> provides a sane API to get a list of supported color formats, whereas
> with v4l1 it is dirty trial-and-error + guesswork for the applications.

I don't see why porting the driver to the V4L2 API has to precede the
software conversion removal I was talking about. I think that the
negotiation of the color formats is not the point here...

For many years no one has ever cancelled deprecated code yet both
users and developers know that software conversion should be made
in userspace. It is worth to state that those who have preached this rule
and have always, for instance, been opposed to optional decoders
in kernel space, have never lifted a finger to correct already existing code. 
Now, I do not expect that these people wake up and even provide a V4L2 driver.

In addition, aside from software conversion, it is enough to take
a quick look at the code to understand that it is no longer maintained.
Having seen the recent occurrences, non-maintained drivers should be
altogether removed.

Nevertheless, there is below a new patch that adds a parameter to the
module and warnings for users, as suggested by you. Within a few months,
if this situation has not changed, I will propose a new patch that
completely removes the driver. At this later date, I hope not to find
objections and excuses that delay this problem.

Rules are made for everyone.

Luca


Signed-off-by: Luca Risolia <luca.risolia@studio.unibo.it>

--- devel-2.6.8/drivers/media/video/cpia.c.orig	2004-08-29 11:28:14.000000000 +0200
+++ devel-2.6.8/drivers/media/video/cpia.c	2004-08-30 17:14:36.000000000 +0200
@@ -29,6 +29,7 @@
 #include <linux/config.h>
 
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/vmalloc.h>
@@ -62,6 +63,15 @@ MODULE_LICENSE("GPL");
 MODULE_SUPPORTED_DEVICE("video");
 #endif
 
+static unsigned short colorspace_conv = 0;
+module_param(colorspace_conv, ushort, 0444);
+MODULE_PARM_DESC(colorspace_conv,
+                 "\n<n> Colorspace conversion:"
+                 "\n0 = disable"
+                 "\n1 = enable"
+                 "\nDefault value is 0"
+                 "\n");
+
 #define ABOUT "V4L-Driver for Vision CPiA based cameras"
 
 #ifndef VID_HARDWARE_CPIA
@@ -1428,14 +1438,22 @@ static void __exit proc_cpia_destroy(voi
 /* supported frame palettes and depths */
 static inline int valid_mode(u16 palette, u16 depth)
 {
-	return (palette == VIDEO_PALETTE_GREY && depth == 8) ||
-	       (palette == VIDEO_PALETTE_RGB555 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_RGB565 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_RGB24 && depth == 24) ||
-	       (palette == VIDEO_PALETTE_RGB32 && depth == 32) ||
-	       (palette == VIDEO_PALETTE_YUV422 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_YUYV && depth == 16) ||
-	       (palette == VIDEO_PALETTE_UYVY && depth == 16);
+	if ((palette == VIDEO_PALETTE_YUV422 && depth == 16) ||
+	    (palette == VIDEO_PALETTE_YUYV && depth == 16))
+		return 1;
+
+	if (colorspace_conv)
+		return (palette == VIDEO_PALETTE_GREY && depth == 8) ||
+		       (palette == VIDEO_PALETTE_RGB555 && depth == 16) ||
+		       (palette == VIDEO_PALETTE_RGB565 && depth == 16) ||
+		       (palette == VIDEO_PALETTE_RGB24 && depth == 24) ||
+		       (palette == VIDEO_PALETTE_RGB32 && depth == 32) ||
+		       (palette == VIDEO_PALETTE_UYVY && depth == 16);
+	else 
+		LOG("Palette %u not available by default. Set colorspace_conv "
+		    "module parameter to 1 to enable it", (unsigned)(palette));
+
+	return 0;
 }
 
 static int match_videosize( int width, int height )

Re: [PATCH 2.6.9-rc1-mm1] Disable colour conversion in the CPiA Video Camera driver

[Gerd Knorr]

On Mon, Aug 30, 2004 at 08:31:17PM +0200, Luca Risolia wrote:
> On Mon, 30 Aug 2004 15:32:05 +0200
> Gerd Knorr <kraxel@bytesex.org> wrote:
> 
> > First: there should be a reasonable warning time for the current users.
> > Some printk message telling them they are using a depricated feature.
> > Maybe even a insmod option to enable/disable it, with the default being
> > software conversion disabled.
> > 
> > Second: IMHO it would be a very good idea to port the driver to the v4l2
> > API before ripping the in-kernel colorspace conversion support.  v4l2
> > provides a sane API to get a list of supported color formats, whereas
> > with v4l1 it is dirty trial-and-error + guesswork for the applications.
> 
> I don't see why porting the driver to the V4L2 API has to precede the
> software conversion removal I was talking about. I think that the
> negotiation of the color formats is not the point here...

It's a suggestion, not a requirement.  The point simply is that due to
the lack of a sane negotiation of the color formats in the v4l1 API
you'll get warning printk's even if there is no real problem.  xawtv for
example doesn't depend on RGB formats being available, but will try to
use them (which then generates a warning printk), and failing that
fallback to yuv and software conversion.  With v4l2 you don't get those
bogous and confusing warnings as the app can simply ask the driver for a
list of supported formats instead of depending on trial-and-error games.

> For many years no one has ever cancelled deprecated code yet both
> users and developers know that software conversion should be made
> in userspace.

That isn't true.  I can remember that at least one usb webcam driver
stopped working with xawtv because in-kernel software conversion was
dropped and xawtv had no support the specific color format used by
that webcam at that time.

> It is worth to state that those who have preached this rule and have
> always, for instance, been opposed to optional decoders in kernel
> space, have never lifted a finger to correct already existing code.
> Now, I do not expect that these people wake up and even provide a V4L2
> driver.
> 
> In addition, aside from software conversion, it is enough to take a
> quick look at the code to understand that it is no longer maintained.

Oh.  That sounds like you don't even remotely intend to care about that
driver?  Why do you mail patches for it then?

> Having seen the recent occurrences, non-maintained drivers should be
> altogether removed.

What is the point in removing a working driver?  If a driver is broken
and nobody cares fixing it -- fine, rip it, user base likely is close to
zero anyway then.  But that isn't true for cpia as far I know.

> Nevertheless, there is below a new patch that adds a parameter to the
> module and warnings for users, as suggested by you.

> +		LOG("Palette %u not available by default. Set colorspace_conv "
> +		    "module parameter to 1 to enable it", (unsigned)(palette));

The message should be rate limited as suggested in the previous message
(because otherwise the log may be flooded with bogous warnings due to
the v4l1 API issues mentioned above).  Maybe a single message at insmod
time is even better.

In any case the message should make clear the intention of this, i.e.
that it is planned to drop in-kernel conversion altogether by -- say --
sept 2005 (should be enougth warning time), that is disabled by default
now to catch problem cases, and that the users should fix the apps in
case they don't work without conversion reenabled via insmod option.

  Gerd

-- 
return -ENOSIG;

Re: [PATCH 2.6.9-rc1-mm1] Disable colour conversion in the CPiA Video Camera driver

[Alan Cox]

On Tue, Aug 31, 2004 at 07:52:36PM +0200, Gerd Knorr wrote:
> the lack of a sane negotiation of the color formats in the v4l1 API
> you'll get warning printk's even if there is no real problem.  xawtv for

The warning printks are bugs

> example doesn't depend on RGB formats being available, but will try to
> use them (which then generates a warning printk), and failing that

The printk is the bug here. The v4l1 design isnt very good either on this
matter but thats a seperate screwup 8)

> That isn't true.  I can remember that at least one usb webcam driver
> stopped working with xawtv because in-kernel software conversion was
> dropped and xawtv had no support the specific color format used by
> that webcam at that time.

Generally neither party will fix such problems until its forced. So a little
pain ends up neccessary

> In any case the message should make clear the intention of this, i.e.
> that it is planned to drop in-kernel conversion altogether by -- say --
> sept 2005 (should be enougth warning time), that is disabled by default

We've been saying its not allowed since 2000 or so, if nobody listens
whats the point of saying 2005

Re: [PATCH 2.6.9-rc1-mm1] Disable colour conversion in the CPiA Video Camera driver

[Luca Risolia]

On Tue, 31 Aug 2004 19:52:36 +0200
Gerd Knorr <kraxel@bytesex.org> wrote:

> The message should be rate limited as suggested in the previous message
> (because otherwise the log may be flooded with bogous warnings due to
> the v4l1 API issues mentioned above).  Maybe a single message at insmod
> time is even better.
> 
> In any case the message should make clear the intention of this, i.e.
> that it is planned to drop in-kernel conversion altogether by -- say --
> sept 2005 (should be enougth warning time), that is disabled by default
> now to catch problem cases, and that the users should fix the apps in
> case they don't work without conversion reenabled via insmod option.

Okay, please apply.

Signed-off-by: Luca Risolia <luca.risolia@studio.unibo.it>

--- devel-2.6.8/drivers/media/video/cpia.c.orig	2004-08-29 11:28:14.000000000 +0200
+++ devel-2.6.8/drivers/media/video/cpia.c	2004-09-01 07:54:13.000000000 +0200
@@ -29,6 +29,7 @@
 #include <linux/config.h>
 
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/init.h>
 #include <linux/fs.h>
 #include <linux/vmalloc.h>
@@ -62,6 +63,15 @@ MODULE_LICENSE("GPL");
 MODULE_SUPPORTED_DEVICE("video");
 #endif
 
+static unsigned short colorspace_conv = 0;
+module_param(colorspace_conv, ushort, 0444);
+MODULE_PARM_DESC(colorspace_conv,
+                 "\n<n> Colorspace conversion:"
+                 "\n0 = disable"
+                 "\n1 = enable"
+                 "\nDefault value is 0"
+                 "\n");
+
 #define ABOUT "V4L-Driver for Vision CPiA based cameras"
 
 #ifndef VID_HARDWARE_CPIA
@@ -1428,14 +1438,19 @@ static void __exit proc_cpia_destroy(voi
 /* supported frame palettes and depths */
 static inline int valid_mode(u16 palette, u16 depth)
 {
-	return (palette == VIDEO_PALETTE_GREY && depth == 8) ||
-	       (palette == VIDEO_PALETTE_RGB555 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_RGB565 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_RGB24 && depth == 24) ||
-	       (palette == VIDEO_PALETTE_RGB32 && depth == 32) ||
-	       (palette == VIDEO_PALETTE_YUV422 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_YUYV && depth == 16) ||
-	       (palette == VIDEO_PALETTE_UYVY && depth == 16);
+	if ((palette == VIDEO_PALETTE_YUV422 && depth == 16) ||
+	    (palette == VIDEO_PALETTE_YUYV && depth == 16))
+		return 1;
+
+	if (colorspace_conv)
+		return (palette == VIDEO_PALETTE_GREY && depth == 8) ||
+		       (palette == VIDEO_PALETTE_RGB555 && depth == 16) ||
+		       (palette == VIDEO_PALETTE_RGB565 && depth == 16) ||
+		       (palette == VIDEO_PALETTE_RGB24 && depth == 24) ||
+		       (palette == VIDEO_PALETTE_RGB32 && depth == 32) ||
+		       (palette == VIDEO_PALETTE_UYVY && depth == 16);
+
+	return 0;
 }
 
 static int match_videosize( int width, int height )
@@ -4040,6 +4055,13 @@ static int __init cpia_init(void)
 {
 	printk(KERN_INFO "%s v%d.%d.%d\n", ABOUT,
 	       CPIA_MAJ_VER, CPIA_MIN_VER, CPIA_PATCH_VER);
+
+	printk(KERN_WARNING "Since in-kernel colorspace conversion is not "
+	       "allowed, it is disabled by default now. Users should fix the "
+	       "applications in case they don't work without conversion "
+	       "reenabled by setting the 'colorspace_conv' module "
+	       "parameter to 1");
+
 #ifdef CONFIG_PROC_FS
 	proc_cpia_create();
 #endif

Re: [PATCH 2.6.9-rc1-mm1] Disable colour conversion in the CPiA Video Camera driver

[Bill Davidsen]

Gerd Knorr wrote:
>>Given that colour conversion is not allowed in kernel space, this patch
>>disables it in the CPiA driver. The routines implementing the conversions
>>can be removed at all by the maintainers of the driver; however, this
>>patch is a good starting point and makes someone happy.
> 
> 
> Yes, colorspace conversion shouldn't be done by the kernel but by the
> applications.  I don't like the idea to just disable them through:
> 
> First: there should be a reasonable warning time for the current users.
> Some printk message telling them they are using a depricated feature.
> Maybe even a insmod option to enable/disable it, with the default being
> software conversion disabled.
> 
> Second: IMHO it would be a very good idea to port the driver to the v4l2
> API before ripping the in-kernel colorspace conversion support.  v4l2
> provides a sane API to get a list of supported color formats, whereas
> with v4l1 it is dirty trial-and-error + guesswork for the applications.

The side effect, a list of supported color formats, is a good one. 
Perhaps someone will suggest some better way to accomplish this than is 
currently the case.

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

[PATCH 2.6.9-rc1-mm1] Disable colour conversion in the CPiA Video Camera driver

[Luca Risolia]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

Given that colour conversion is not allowed in kernel space, this patch
disables it in the CPiA driver. The routines implementing the conversions
can be removed at all by the maintainers of the driver; however, this
patch is a good starting point and makes someone happy.

I have already submitted this patch to both the V4L mailing list
and the V4L maintainer two months ago, but it has been ignored for
some unknown reasons, so here it is again.

Please apply.

Signed-off-by: Luca Risolia <luca.risolia@studio.unibo.it>

--- devel-2.6.8/drivers/media/video/cpia.c.orig	2004-08-29 11:28:14.000000000 +0200
+++ devel-2.6.8/drivers/media/video/cpia.c	2004-08-29 11:29:55.000000000 +0200
@@ -1428,14 +1428,8 @@ static void __exit proc_cpia_destroy(voi
 /* supported frame palettes and depths */
 static inline int valid_mode(u16 palette, u16 depth)
 {
-	return (palette == VIDEO_PALETTE_GREY && depth == 8) ||
-	       (palette == VIDEO_PALETTE_RGB555 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_RGB565 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_RGB24 && depth == 24) ||
-	       (palette == VIDEO_PALETTE_RGB32 && depth == 32) ||
-	       (palette == VIDEO_PALETTE_YUV422 && depth == 16) ||
-	       (palette == VIDEO_PALETTE_YUYV && depth == 16) ||
-	       (palette == VIDEO_PALETTE_UYVY && depth == 16);
+	return (palette == VIDEO_PALETTE_YUV422 && depth == 16) ||
+	       (palette == VIDEO_PALETTE_YUYV && depth == 16);
 }
 
 static int match_videosize( int width, int height )

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]