staging/easycap cleanups + fix

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Monday, June 13, 2011 3:18 PM
To: Greg Kroah-Hartman
Cc: Winkler, Tomas; Mike Thomas; [email protected]; linux- [email protected]; Kirill Smelkov
Subject: [PATCH 1/5] staging/easycap: Kill leftover build options in readme

The following options were removed from in-tree driver, but were left in README:

- EASYCAP_IS_VIDEODEV_CLIENT was removed in cb81fa07 (staging/easycap:
kill EASYCAP_IS_VIDEODEV_CLIENT compilation conditional);

- EASYCAP_NEEDS_V4L2_DEVICE_H/EASYCAP_NEEDS_V4L2_FOPS were removed in
30516058 (staging/easycap: kill EASYCAP_NEEDS_V4L2_DEVICE_H and EASYCAP_NEEDS_V4L2_FOPS);

- EASYCAP_NEEDS_UNLOCKED_IOCTL was removed in f2b3c685 (staging/easycap:
kill EASYCAP_NEEDS_UNLOCKED_IOCTL).

Remove them.

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

---
 drivers/staging/easycap/README |   22 ----------------------
 1 files changed, 0 insertions(+), 22 deletions(-)

(quoted diff) +0 -0

b/drivers/staging/easycap/README index 6b5ac0d..16b8264 100644

drivers/staging/easycap/README +0 -0

@@ -27,28 +27,6 @@ BUILD OPTIONS AND DEPENDENCIES  Unless

EASYCAP_DEBUG is defined during compilation it will not be possible to select a debug level at the time of module installation.

-If the parameter EASYCAP_IS_VIDEODEV_CLIENT is undefined during compilation -the built module is entirely independent of the videodev module, and when -the EasyCAP is physically plugged into a USB port the special files
-/dev/easycap0 and /dev/easysnd1 are created as video and sound sources - respectively.

drivers/staging/easycap/README +0 -2

-
-If the parameter EASYCAP_IS_VIDEODEV_CLIENT is defined during

compilation -the built easycap module is configured to register with the videodev module, -in which case the special files created when the EasyCAP is plugged in are

drivers/staging/easycap/README +0 -3

-/dev/video0 and /dev/easysnd0.
-
-During in-tree builds the following should should be defined whenever the -

parameter EASYCAP_IS_VIDEODEV_CLIENT is defined:

drivers/staging/easycap/README +0 -6

-
-EASYCAP_NEEDS_V4L2_DEVICE_H
-EASYCAP_NEEDS_V4L2_FOPS
-EASYCAP_NEEDS_UNLOCKED_IOCTL
-
-If the build is performed out-of-tree against older kernels the parameters -

to be defined depend on the kernel version in a way which will not be - discussed here.
-

KNOWN RUNTIME ISSUES
--------------------
--
1.7.6.rc1

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Monday, June 13, 2011 3:18 PM
To: Greg Kroah-Hartman
Cc: Winkler, Tomas; Mike Thomas; [email protected]; linux- [email protected]; Kirill Smelkov
Subject: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

Both bars=0 and bars=1 were described as meaning to display bars on signal lost. Actually bars=1 means "display bars", but bars=0 means display raw source as is (usually black screen).

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

---
 drivers/staging/easycap/README |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/drivers/staging/easycap/README index 16b8264..cd55eea 100644

drivers/staging/easycap/README +1 -2

@@ -60,7 +60,7 @@ Three module parameters are defined:
 
 debug      the easycap module is configured at diagnostic level n (0 to 9)
 gain       audio gain level n (0 to 31, default is 16)
-bars       0 =>  testcard bars when incoming video signal is lost
+bars       0 =>  no testcard bars when incoming video signal is lost
            1 =>  testcard bars when incoming video signal is lost (default)
 
 
--

1.7.6.rc1

Both bars=0 and bars=1 were described as meaning to display bars on signal lost. Actually bars=1 means "display bars", but bars=0 means display raw source as is (usually black screen).

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

---
 drivers/staging/easycap/README |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/easycap/README +1 -1

index 16b8264..cd55eea 100644
@@ -60,7 +60,7 @@ Three module parameters are defined:
 
 debug      the easycap module is configured at diagnostic level n (0 to 9)
 gain       audio gain level n (0 to 31, default is 16)
-bars       0 =>  testcard bars when incoming video signal is lost
+bars       0 =>  no testcard bars when incoming video signal is lost
            1 =>  testcard bars when incoming video signal is lost (default)
 
 

Signature · 6 lines

1.7.6.rc1

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Quoted 3 lines · Kirill Smelkov

Both bars=0 and bars=1 were described as meaning to display bars on signal lost. Actually bars=1 means "display bars", but bars=0 means display raw source as is (usually black screen).

Your description up here is much better than the one in the README. Maybe just include yours and delete the original.

Quoted 7 lines · Dan Carpenter

Quoted 3 lines · Kirill Smelkov

Both bars=0 and bars=1 were described as meaning to display bars on signal lost. Actually bars=1 means "display bars", but bars=0 means display raw source as is (usually black screen).

Your description up here is much better than the one in the README. Maybe just include yours and delete the original.

Thanks for head ups. Something like this:

Quoted 6 lines · Kirill Smelkov

Quoted 5 lines · Dan Carpenter

Quoted 3 lines · Kirill Smelkov

Both bars=0 and bars=1 were described as meaning to display bars on signal lost. Actually bars=1 means "display bars", but bars=0 means display raw source as is (usually black screen).

Quoted 5 lines

Quoted 2 lines

Your description up here is much better than the one in the README. Maybe just include yours and delete the original.

Thanks for head ups. Something like this:

I like it. :)

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Wednesday, June 29, 2011 11:35 AM
To: Dan Carpenter; Winkler, Tomas
Cc: Greg Kroah-Hartman; [email protected]; Mike Thomas; linux- [email protected]
Subject: Re: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

Quoted 10 lines · Dan Carpenter

Quoted 9 lines · Kirill Smelkov

Quoted 7 lines · Dan Carpenter

Quoted 3 lines · Kirill Smelkov

Both bars=0 and bars=1 were described as meaning to display bars on signal lost. Actually bars=1 means "display bars", but bars=0 means display raw source as is (usually black screen).

Your description up here is much better than the one in the README. Maybe just include yours and delete the original.

Quoted 4 lines

Quoted 2 lines

Thanks for head ups. Something like this:

I like it. :)

Thanks. So I assume this version is better to go in. Tomas, may I add your ACK to this one, or maybe there is some issue with it?

Thanks,
Kirill

P.S. By the way, how should we proceed to get this merged into staging tree? Will Greg just pick it up or should I repost the whole series?

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Monday, June 13, 2011 3:19 PM
To: Greg Kroah-Hartman
Cc: Winkler, Tomas; Mike Thomas; [email protected]; linux- [email protected]; Kirill Smelkov
Subject: [PATCH 3/5] staging/easycap: There is no need to support V4L1 ioctls

Because V4L1 was completly removed from the kernel in 2.6.38. See e.g.

08af245d ([media] V4L: remove V4L1 compatibility mode).

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

---
 drivers/staging/easycap/README |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/staging/easycap/README index cd55eea..08f649d 100644

drivers/staging/easycap/README +0 -0

@@ -106,8 +106,6 @@ hardware, but as yet it has actually been tested on

only a few of these.
I have been unable to test and calibrate the S-video input myself because I
do not possess any equipment with S-video output.

drivers/staging/easycap/README +0 -3

-This driver does not understand the V4L1 IOCTL commands.
-
 
 UDEV RULES
 ----------
--

1.7.6.rc1

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Monday, June 13, 2011 3:19 PM
To: Greg Kroah-Hartman
Cc: Winkler, Tomas; Mike Thomas; [email protected]; linux- [email protected]; Kirill Smelkov
Subject: [PATCH 4/5] staging/easycap: PAGE_SIZE is always defined

I'm not 100% sure, only 99.99% that PAGE_SIZE is always defined in Linux. So there is no need to check for it.

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

---
 drivers/staging/easycap/easycap.h |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

drivers/staging/easycap/easycap.h +0 -5

index 1f94e23..5d21620 100644
@@ -85,10 +85,6 @@
 #include <linux/videodev2.h>
 #include <linux/soundcard.h>
 
-#ifndef PAGE_SIZE
-#error "PAGE_SIZE not defined"
-#endif /* PAGE_SIZE */
-
 /*---------------------------------------------------------------------------*/
 /*  VENDOR, PRODUCT:  Syntek Semiconductor Co., Ltd
  *
--

1.7.6.rc1

P.S. why at all we use those mask/shifts? Why not use bitfields?

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Monday, June 13, 2011 3:19 PM
To: Greg Kroah-Hartman
Cc: Winkler, Tomas; Mike Thomas; [email protected]; linux- [email protected]; Kirill Smelkov
Subject: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

As described above fillin_formats()

"""
/*
* THE 16-BIT easycap_format.mask HAS MEANING:
* (least significant) BIT 0: 0 => PAL, 25 FPS; 1 => NTSC, 30 FPS
* BITS 2-4: RESERVED FOR DIFFERENTIATING STANDARDS
* BITS 5-7: NUMBER OF BYTES PER PIXEL
* BIT 8: 0 => NATIVE BYTE ORDER; 1 => SWAPPED
* BITS 9-10: RESERVED FOR OTHER BYTE PERMUTATIONS
* BIT 11: 0 => UNDECIMATED; 1 => DECIMATED
* BIT 12: 0 => OFFER FRAMES; 1 => OFFER FIELDS
* BIT 13: 0 => FULL FRAMERATE; 1 => REDUCED
* (most significant) BITS 14-15: RESERVED FOR OTHER FIELD/FRAME
OPTIONS
* IT FOLLOWS THAT:
* bytesperpixel IS ((0x00E0 & easycap_format.mask) >> 5)
* byteswaporder IS true IF (0 != (0x0100 & easycap_format.mask))
*
* decimatepixel IS true IF (0 != (0x0800 & easycap_format.mask))
*
* offerfields IS true IF (0 != (0x1000 & easycap_format.mask))
*/
"""

bytes-per-pixel is stored in bits 5-7 of calculated mask.

But when calculating bytes-per-line we were extracting wrong value instead of bytes-per-pixel, which was usually 2 times bigger -- e.g. for PAL YUV 422 I was getting ((mask3 & 0x00F0) >> 4) = 4 bytes instead of 2.

The error here is that even in comments there is a line saying

* bytesperpixel IS ((0x00E0 & easycap_format.mask) >> 5)

but we were using

                                    ((0x00F0 & easycap_format.mask) >> 4)

With 2 times bigger bytesperpixel and automatically bytesperline, the video was shown halfheight'ed, which is understandable if we look at video- memory layout:

    <------- bytesperline -------->
    <- real bpl ->

    x0----------y0  x1-----------y1
    x2----------y2  x3-----------y3

    xn----------yn  xn-----------yn
    <garbage>

for each line, we should display width pixels, then move to next line with bytesperline, and oops, if bytesperline = 2*real-bytesperlin, we'll skip one line and move to next-next line, and so only half lines will be shown.

Initially I've debugged the problem with my video application[1], but I've checked that after this patch both rawv (mine app) and tvtime work correctly.

[1] http://repo.or.cz/w/rawv.git

P.S. why at all we use those mask/shifts? Why not use bitfields?

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

---
 drivers/staging/easycap/easycap_settings.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/easycap/easycap_settings.c +1 -1

index 898559d..70f59b1 100644
@@ -567,7 +567,7 @@ int fillin_formats(void)
 				default:
 					return -3;
 				}
-				bytesperline = width * ((mask3 & 0x00F0) >>
 4);
+				bytesperline = width * ((mask3 & 0x00E0) >>
 5);
 				sizeimage =  bytesperline * height;

for (m = 0; m < INTERLACE_MANY; m++) {
--
1.7.6.rc1

Quoted 69 lines

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Monday, June 13, 2011 3:19 PM
To: Greg Kroah-Hartman
Cc: Winkler, Tomas; Mike Thomas; [email protected]; linux- [email protected]; Kirill Smelkov
Subject: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

As described above fillin_formats()

"""
/*
* THE 16-BIT easycap_format.mask HAS MEANING:
* (least significant) BIT 0: 0 => PAL, 25 FPS; 1 => NTSC, 30 FPS
* BITS 2-4: RESERVED FOR DIFFERENTIATING STANDARDS
* BITS 5-7: NUMBER OF BYTES PER PIXEL
* BIT 8: 0 => NATIVE BYTE ORDER; 1 => SWAPPED
* BITS 9-10: RESERVED FOR OTHER BYTE PERMUTATIONS
* BIT 11: 0 => UNDECIMATED; 1 => DECIMATED
* BIT 12: 0 => OFFER FRAMES; 1 => OFFER FIELDS
* BIT 13: 0 => FULL FRAMERATE; 1 => REDUCED
* (most significant) BITS 14-15: RESERVED FOR OTHER FIELD/FRAME
OPTIONS
* IT FOLLOWS THAT:
* bytesperpixel IS ((0x00E0 & easycap_format.mask) >> 5)
* byteswaporder IS true IF (0 != (0x0100 & easycap_format.mask))
*
* decimatepixel IS true IF (0 != (0x0800 & easycap_format.mask))
*
* offerfields IS true IF (0 != (0x1000 & easycap_format.mask))
*/
"""

bytes-per-pixel is stored in bits 5-7 of calculated mask.

But when calculating bytes-per-line we were extracting wrong value instead of bytes-per-pixel, which was usually 2 times bigger -- e.g. for PAL YUV 422 I was getting ((mask3 & 0x00F0) >> 4) = 4 bytes instead of 2.

The error here is that even in comments there is a line saying

* bytesperpixel IS ((0x00E0 & easycap_format.mask) >> 5)

but we were using

                                    ((0x00F0 & easycap_format.mask) >> 4)

With 2 times bigger bytesperpixel and automatically bytesperline, the video was shown halfheight'ed, which is understandable if we look at video- memory layout:

    <------- bytesperline -------->
    <- real bpl ->

    x0----------y0  x1-----------y1
    x2----------y2  x3-----------y3

    xn----------yn  xn-----------yn
    <garbage>

for each line, we should display width pixels, then move to next line with bytesperline, and oops, if bytesperline = 2*real-bytesperlin, we'll skip one line and move to next-next line, and so only half lines will be shown.

Initially I've debugged the problem with my video application[1], but I've checked that after this patch both rawv (mine app) and tvtime work correctly.

Quoted 3 lines

[1] http://repo.or.cz/w/rawv.git

P.S. why at all we use those mask/shifts? Why not use bitfields?

First of all I have feeling this code history beyond this driver.
Second there possible issues with bit fields related to endianity and efficiency. I'm not sure it applies also in this driver

context and I agree it looks ugly here , yet I personally usually avoid using bit fields.

Quoted 4 lines

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

Acked-by Tomas Winkler <[email protected]>

It warms my heart to see a beautiful commit message like that, complete with diagrams. :)

Quoted 2 lines

P.S. why at all we use those mask/shifts? Why not use bitfields?

Because this is crap code from the staging dir.

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Tuesday, June 14, 2011 7:37 PM
To: Dan Carpenter; Winkler, Tomas
Cc: Greg Kroah-Hartman; [email protected]; Mike Thomas; linux- [email protected]
Subject: Re: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

Quoted 35 lines · Tomas

Quoted 35 lines

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Monday, June 13, 2011 3:19 PM
To: Greg Kroah-Hartman
Cc: Winkler, Tomas; Mike Thomas; [email protected]; linux- [email protected]; Kirill Smelkov
Subject: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

As described above fillin_formats()

"""
/*
* THE 16-BIT easycap_format.mask HAS MEANING:
* (least significant) BIT 0: 0 => PAL, 25 FPS; 1 => NTSC, 30 FPS
* BITS 2-4: RESERVED FOR DIFFERENTIATING STANDARDS
* BITS 5-7: NUMBER OF BYTES PER PIXEL
* BIT 8: 0 => NATIVE BYTE ORDER; 1 => SWAPPED
* BITS 9-10: RESERVED FOR OTHER BYTE PERMUTATIONS
* BIT 11: 0 => UNDECIMATED; 1 => DECIMATED
* BIT 12: 0 => OFFER FRAMES; 1 => OFFER FIELDS
* BIT 13: 0 => FULL FRAMERATE; 1 => REDUCED
* (most significant) BITS 14-15: RESERVED FOR OTHER FIELD/FRAME
OPTIONS
* IT FOLLOWS THAT:
* bytesperpixel IS ((0x00E0 & easycap_format.mask) >> 5)
* byteswaporder IS true IF (0 != (0x0100 & easycap_format.mask))
*
* decimatepixel IS true IF (0 != (0x0800 & easycap_format.mask))
*
* offerfields IS true IF (0 != (0x1000 & easycap_format.mask))
*/
"""

Quoted 5 lines

Quoted 5 lines

bytes-per-pixel is stored in bits 5-7 of calculated mask.

But when calculating bytes-per-line we were extracting wrong value instead of bytes-per-pixel, which was usually 2 times bigger -- e.g. for PAL YUV 422 I was getting ((mask3 & 0x00F0) >> 4) = 4 bytes instead of

2.

Quoted 19 lines

Quoted 19 lines

The error here is that even in comments there is a line saying

* bytesperpixel IS ((0x00E0 & easycap_format.mask) >> 5)

but we were using

                                    ((0x00F0 & easycap_format.mask)

Quoted 1 line

Quoted 1 line

4)

With 2 times bigger bytesperpixel and automatically bytesperline, the video was shown halfheight'ed, which is understandable if we look at video- memory layout:

    <------- bytesperline -------->
    <- real bpl ->

    x0----------y0  x1-----------y1
    x2----------y2  x3-----------y3

Quoted 6 lines

Quoted 6 lines

    xn----------yn  xn-----------yn
    <garbage>

for each line, we should display width pixels, then move to next line with bytesperline, and oops, if bytesperline = 2*real-bytesperlin, we'll skip one line and move to next-next line, and so

only half lines will be shown.

Quoted 5 lines

Quoted 5 lines

Initially I've debugged the problem with my video application[1], but I've checked that after this patch both rawv (mine app) and tvtime work correctly.

Quoted 7 lines

Quoted 3 lines

[1] http://repo.or.cz/w/rawv.git

P.S. why at all we use those mask/shifts? Why not use bitfields?

First of all I have feeling this code history beyond this driver. Second there possible issues with bit fields related to endianity and efficiency. I'm not sure it applies also in this driver

I'd agree about endianity, but if I see it correctly, in this driver easycap_format.mask is used only as internal driver flags not writtent to hardware registers. So endianity should be out of scope here.

As to efficiency, in essence the compiler generates almost the same shifts and masks for accessing bitfields as a human would do, so there should be no difference, especially when we are not talking about hot-path code.

And also, if endianity matters, there is always possibility to do something like this (from drivers/atm/fore200e.h):

    /* bitfields endian games */

    #if defined(__LITTLE_ENDIAN_BITFIELD)
    #define BITFIELD2(b1, b2)                    b1; b2;
    #define BITFIELD3(b1, b2, b3)                b1; b2; b3;
    #define BITFIELD4(b1, b2, b3, b4)            b1; b2; b3; b4;
    #define BITFIELD5(b1, b2, b3, b4, b5)        b1; b2; b3; b4; b5;
    #define BITFIELD6(b1, b2, b3, b4, b5, b6)    b1; b2; b3; b4; b5; b6;
    #elif defined(__BIG_ENDIAN_BITFIELD)
    #define BITFIELD2(b1, b2)                                    b2; b1;
    #define BITFIELD3(b1, b2, b3)                            b3; b2; b1;
    #define BITFIELD4(b1, b2, b3, b4)                    b4; b3; b2; b1;
    #define BITFIELD5(b1, b2, b3, b4, b5)            b5; b4; b3; b2; b1;
    #define BITFIELD6(b1, b2, b3, b4, b5, b6)    b6; b5; b4; b3; b2; b1;
    #else
    #error unknown bitfield endianess
    #endif


    /* ATM cell header (minus HEC byte) */

    typedef struct atm_header {
        BITFIELD5(
            u32 clp :  1,    /* cell loss priority         */
            u32 plt :  3,    /* payload type               */
            u32 vci : 16,    /* virtual channel identifier */
            u32 vpi :  8,    /* virtual path identifier    */
            u32 gfc :  4     /* generic flow control       */
       )
    } atm_header_t;

to me, it's the sanest approach to C bitfields and endianity. Only we'd better have those BITFIELDX in include/linux/bitfield.h

Quoted 2 lines

context and I agree it looks ugly here , yet I personally usually avoid using

bit fields.

Quoted 2 lines

Quoted 2 lines

Quoted 6 lines

Quoted 3 lines

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

Acked-by Tomas Winkler <[email protected]>

Did you try running it? How come this bug was undiscovered for so long?

Quoted 134 lines

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Tuesday, June 14, 2011 7:37 PM
To: Dan Carpenter; Winkler, Tomas
Cc: Greg Kroah-Hartman; [email protected]; Mike Thomas; linux- [email protected]
Subject: Re: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

Quoted 35 lines · Tomas

Quoted 35 lines

-----Original Message-----
From: Kirill Smelkov [mailto:[email protected]]
Sent: Monday, June 13, 2011 3:19 PM
To: Greg Kroah-Hartman
Cc: Winkler, Tomas; Mike Thomas; [email protected]; linux- [email protected]; Kirill Smelkov
Subject: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

As described above fillin_formats()

"""
/*
* THE 16-BIT easycap_format.mask HAS MEANING:
* (least significant) BIT 0: 0 => PAL, 25 FPS; 1 => NTSC, 30 FPS
* BITS 2-4: RESERVED FOR DIFFERENTIATING STANDARDS
* BITS 5-7: NUMBER OF BYTES PER PIXEL
* BIT 8: 0 => NATIVE BYTE ORDER; 1 => SWAPPED
* BITS 9-10: RESERVED FOR OTHER BYTE PERMUTATIONS
* BIT 11: 0 => UNDECIMATED; 1 => DECIMATED
* BIT 12: 0 => OFFER FRAMES; 1 => OFFER FIELDS
* BIT 13: 0 => FULL FRAMERATE; 1 => REDUCED
* (most significant) BITS 14-15: RESERVED FOR OTHER FIELD/FRAME
OPTIONS
* IT FOLLOWS THAT:
* bytesperpixel IS ((0x00E0 & easycap_format.mask) >> 5)
* byteswaporder IS true IF (0 != (0x0100 & easycap_format.mask))
*
* decimatepixel IS true IF (0 != (0x0800 & easycap_format.mask))
*
* offerfields IS true IF (0 != (0x1000 & easycap_format.mask))
*/
"""

Quoted 5 lines

Quoted 5 lines

bytes-per-pixel is stored in bits 5-7 of calculated mask.

But when calculating bytes-per-line we were extracting wrong value instead of bytes-per-pixel, which was usually 2 times bigger -- e.g. for PAL YUV 422 I was getting ((mask3 & 0x00F0) >> 4) = 4 bytes instead of

2.

Quoted 19 lines

Quoted 19 lines

The error here is that even in comments there is a line saying

* bytesperpixel IS ((0x00E0 & easycap_format.mask) >> 5)

but we were using

                                    ((0x00F0 & easycap_format.mask)

Quoted 1 line

Quoted 1 line

4)

With 2 times bigger bytesperpixel and automatically bytesperline, the video was shown halfheight'ed, which is understandable if we look at video- memory layout:

    <------- bytesperline -------->
    <- real bpl ->

    x0----------y0  x1-----------y1
    x2----------y2  x3-----------y3

Quoted 6 lines

Quoted 6 lines

    xn----------yn  xn-----------yn
    <garbage>

for each line, we should display width pixels, then move to next line with bytesperline, and oops, if bytesperline = 2*real-bytesperlin, we'll skip one line and move to next-next line, and so

only half lines will be shown.

Quoted 5 lines

Quoted 5 lines

Initially I've debugged the problem with my video application[1], but I've checked that after this patch both rawv (mine app) and tvtime work correctly.

Quoted 7 lines

Quoted 3 lines

[1] http://repo.or.cz/w/rawv.git

P.S. why at all we use those mask/shifts? Why not use bitfields?

First of all I have feeling this code history beyond this driver. Second there possible issues with bit fields related to endianity and efficiency. I'm not sure it applies also in this driver

I'd agree about endianity, but if I see it correctly, in this driver easycap_format.mask is used only as internal driver flags not writtent to hardware registers. So endianity should be out of scope here.

As to efficiency, in essence the compiler generates almost the same shifts and masks for accessing bitfields as a human would do, so there should be no difference, especially when we are not talking about hot-path code.

And also, if endianity matters, there is always possibility to do something like this (from drivers/atm/fore200e.h):

    /* bitfields endian games */

    #if defined(__LITTLE_ENDIAN_BITFIELD)
    #define BITFIELD2(b1, b2)                    b1; b2;
    #define BITFIELD3(b1, b2, b3)                b1; b2; b3;
    #define BITFIELD4(b1, b2, b3, b4)            b1; b2; b3; b4;
    #define BITFIELD5(b1, b2, b3, b4, b5)        b1; b2; b3; b4; b5;
    #define BITFIELD6(b1, b2, b3, b4, b5, b6)    b1; b2; b3; b4; b5; b6;
    #elif defined(__BIG_ENDIAN_BITFIELD)
    #define BITFIELD2(b1, b2)                                    b2; b1;
    #define BITFIELD3(b1, b2, b3)                            b3; b2; b1;
    #define BITFIELD4(b1, b2, b3, b4)                    b4; b3; b2; b1;
    #define BITFIELD5(b1, b2, b3, b4, b5)            b5; b4; b3; b2; b1;
    #define BITFIELD6(b1, b2, b3, b4, b5, b6)    b6; b5; b4; b3; b2; b1;
    #else
    #error unknown bitfield endianess
    #endif


    /* ATM cell header (minus HEC byte) */

    typedef struct atm_header {
        BITFIELD5(
            u32 clp :  1,    /* cell loss priority         */
            u32 plt :  3,    /* payload type               */
            u32 vci : 16,    /* virtual channel identifier */
            u32 vpi :  8,    /* virtual path identifier    */
            u32 gfc :  4     /* generic flow control       */
       )
    } atm_header_t;

Quoted 3 lines

to me, it's the sanest approach to C bitfields and endianity. Only we'd better have those BITFIELDX in include/linux/bitfield.h

Still I don't like these macro templates either but that's probably personal taste only I wouldn't NACK it.

Quoted 9 lines

Quoted 1 line

context and I agree it looks ugly here , yet I personally usually avoid using

bit fields.

Quoted 7 lines

Quoted 5 lines

Cc Tomas Winkler <[email protected]>

Cc Mike Thomas <[email protected]>

Signed-off-by Kirill Smelkov <[email protected]>

Quoted 3 lines

Quoted 1 line

Acked-by Tomas Winkler <[email protected]>

Did you try running it? How come this bug was undiscovered for so long?

There is a lot of cleanups to be done in this area. So far I've cleaned up mostly the basic code styling and next it would be to rewrite the USB handling and then to fit this somehow to new linux-media framework including video quality fixing

Currently I'm more busy with my real life so it is going slower :)

I'm really glad that you are also interested in fixing this driver. It quite affordable HW and does its work.