[PATCH 0/5] staging/easycap cleanups + fix

[PATCH 0/5] staging/easycap cleanups + fix

[Kirill Smelkov]

Hello up there,

Recently I've bought Mobiledata's easycap, and while trying to make it work
under Linux, and studying the driver, have found some places to clean up and 1
bug related to bytesperline calculation.


Please apply and thanks,
Kirill


Kirill Smelkov (5):
  staging/easycap: Kill leftover build options in readme
  staging/easycap: Fix thinko in "bars" module option description
  staging/easycap: There is no need to support V4L1 ioctls
  staging/easycap: PAGE_SIZE is always defined
  staging/easycap: Fix bytesperline calculation

 drivers/staging/easycap/README             |   26 +-------------------------
 drivers/staging/easycap/easycap.h          |    4 ----
 drivers/staging/easycap/easycap_settings.c |    2 +-
 3 files changed, 2 insertions(+), 30 deletions(-)

-- 
1.7.6.rc1

[PATCH 1/5] staging/easycap: Kill leftover build options in readme

[Kirill Smelkov]

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 <tomas.winkler@intel.com>
Cc: Mike Thomas <rmthomas@sciolus.org>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/staging/easycap/README |   22 ----------------------
 1 files changed, 0 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/easycap/README b/drivers/staging/easycap/README
index 6b5ac0d..16b8264 100644
--- a/drivers/staging/easycap/README
+++ b/drivers/staging/easycap/README
@@ -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.
-
-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
-/dev/video0 and /dev/easysnd0.
-
-During in-tree builds the following should should be defined whenever the
-parameter EASYCAP_IS_VIDEODEV_CLIENT is defined:
-
-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

RE: [PATCH 1/5] staging/easycap: Kill leftover build options in readme

[Winkler, Tomas]

> -----Original Message-----
> From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> Sent: Monday, June 13, 2011 3:18 PM
> To: Greg Kroah-Hartman
> Cc: Winkler, Tomas; Mike Thomas; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; 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 <tomas.winkler@intel.com>
> Cc: Mike Thomas <rmthomas@sciolus.org>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
Acked-by: Tomas Winkler <tomas.winkler@intel.com>

> ---
>  drivers/staging/easycap/README |   22 ----------------------
>  1 files changed, 0 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/staging/easycap/README
> b/drivers/staging/easycap/README index 6b5ac0d..16b8264 100644
> --- a/drivers/staging/easycap/README
> +++ b/drivers/staging/easycap/README
> @@ -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.
> -
> -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
> -/dev/video0 and /dev/easysnd0.
> -
> -During in-tree builds the following should should be defined whenever the -
> parameter EASYCAP_IS_VIDEODEV_CLIENT is defined:
> -
> -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

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

[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).

Cc: Tomas Winkler <tomas.winkler@intel.com>
Cc: Mike Thomas <rmthomas@sciolus.org>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/staging/easycap/README |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/easycap/README b/drivers/staging/easycap/README
index 16b8264..cd55eea 100644
--- a/drivers/staging/easycap/README
+++ b/drivers/staging/easycap/README
@@ -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

RE: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

[Winkler, Tomas]

> -----Original Message-----
> From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> Sent: Monday, June 13, 2011 3:18 PM
> To: Greg Kroah-Hartman
> Cc: Winkler, Tomas; Mike Thomas; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; 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 <tomas.winkler@intel.com>
> Cc: Mike Thomas <rmthomas@sciolus.org>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>


Acked-by: Tomas Winkler <tomas.winkler@intel.com>

> ---
>  drivers/staging/easycap/README |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/easycap/README
> b/drivers/staging/easycap/README index 16b8264..cd55eea 100644
> --- a/drivers/staging/easycap/README
> +++ b/drivers/staging/easycap/README
> @@ -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

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

[Dan Carpenter]

On Mon, Jun 13, 2011 at 04:18:29PM +0400, Kirill Smelkov wrote:
> 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.

regards,
dan carpenter

> 
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Cc: Mike Thomas <rmthomas@sciolus.org>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  drivers/staging/easycap/README |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/easycap/README b/drivers/staging/easycap/README
> index 16b8264..cd55eea 100644
> --- a/drivers/staging/easycap/README
> +++ b/drivers/staging/easycap/README
> @@ -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
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

[Kirill Smelkov]

On Mon, Jun 13, 2011 at 03:27:17PM +0300, Dan Carpenter wrote:
> On Mon, Jun 13, 2011 at 04:18:29PM +0400, Kirill Smelkov wrote:
> > 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:

---- 8< ----
From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Mon, 13 Jun 2011 15:09:31 +0400
Subject: [PATCH v2] 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).

Instead of changing bars=0 to "_no_ testcard bars ..." as suggested by
Dan Carpenter reword the whole bars description for clarity.

Cc: Tomas Winkler <tomas.winkler@intel.com>
Cc: Mike Thomas <rmthomas@sciolus.org>
Cc: Dan Carpenter <error27@gmail.com>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/staging/easycap/README |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/easycap/README b/drivers/staging/easycap/README
index 16b8264..dadceb9 100644
--- a/drivers/staging/easycap/README
+++ b/drivers/staging/easycap/README
@@ -60,8 +60,8 @@ 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
-           1 =>  testcard bars when incoming video signal is lost (default)
+bars       whether to display testcard bars when incoming video signal is lost
+           0 => no, 1 => yes (default)
 
 
 SUPPORTED TV STANDARDS AND RESOLUTIONS
-- 
1.7.6.rc1

Re: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

[Dan Carpenter]

On Tue, Jun 14, 2011 at 08:46:49PM +0400, Kirill Smelkov wrote:
> On Mon, Jun 13, 2011 at 03:27:17PM +0300, Dan Carpenter wrote:
> > On Mon, Jun 13, 2011 at 04:18:29PM +0400, Kirill Smelkov wrote:
> > > 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:
>

I like it.  :)

regards,
dan carpenter

Re: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

[Kirill Smelkov]

On Tue, Jun 14, 2011 at 08:16:19PM +0300, Dan Carpenter wrote:
> On Tue, Jun 14, 2011 at 08:46:49PM +0400, Kirill Smelkov wrote:
> > On Mon, Jun 13, 2011 at 03:27:17PM +0300, Dan Carpenter wrote:
> > > On Mon, Jun 13, 2011 at 04:18:29PM +0400, Kirill Smelkov wrote:
> > > > 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:
> >
> 
> 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?

RE: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option description

[Winkler, Tomas]

> -----Original Message-----
> From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> Sent: Wednesday, June 29, 2011 11:35 AM
> To: Dan Carpenter; Winkler, Tomas
> Cc: Greg Kroah-Hartman; devel@driverdev.osuosl.org; Mike Thomas; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/5] staging/easycap: Fix thinko in "bars" module option
> description
> 
> On Tue, Jun 14, 2011 at 08:16:19PM +0300, Dan Carpenter wrote:
> > On Tue, Jun 14, 2011 at 08:46:49PM +0400, Kirill Smelkov wrote:
> > > On Mon, Jun 13, 2011 at 03:27:17PM +0300, Dan Carpenter wrote:
> > > > On Mon, Jun 13, 2011 at 04:18:29PM +0400, Kirill Smelkov wrote:
> > > > > 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:
> > >
> >
> > 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?

Sure, ACK

> 
> 
> 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?

Thanks
Tomas

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 3/5] staging/easycap: There is no need to support V4L1 ioctls

[Kirill Smelkov]

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 <tomas.winkler@intel.com>
Cc: Mike Thomas <rmthomas@sciolus.org>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/staging/easycap/README |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/easycap/README b/drivers/staging/easycap/README
index cd55eea..08f649d 100644
--- a/drivers/staging/easycap/README
+++ b/drivers/staging/easycap/README
@@ -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.
 
-This driver does not understand the V4L1 IOCTL commands.
-
 
 UDEV RULES
 ----------
-- 
1.7.6.rc1

RE: [PATCH 3/5] staging/easycap: There is no need to support V4L1 ioctls

[Winkler, Tomas]

> -----Original Message-----
> From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> Sent: Monday, June 13, 2011 3:19 PM
> To: Greg Kroah-Hartman
> Cc: Winkler, Tomas; Mike Thomas; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; 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 <tomas.winkler@intel.com>
> Cc: Mike Thomas <rmthomas@sciolus.org>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
Acked-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/staging/easycap/README |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/easycap/README
> b/drivers/staging/easycap/README index cd55eea..08f649d 100644
> --- a/drivers/staging/easycap/README
> +++ b/drivers/staging/easycap/README
> @@ -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.
> 
> -This driver does not understand the V4L1 IOCTL commands.
> -
> 
>  UDEV RULES
>  ----------
> --
> 1.7.6.rc1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 4/5] staging/easycap: PAGE_SIZE is always defined

[Kirill Smelkov]

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 <tomas.winkler@intel.com>
Cc: Mike Thomas <rmthomas@sciolus.org>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/staging/easycap/easycap.h |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/easycap/easycap.h b/drivers/staging/easycap/easycap.h
index 1f94e23..5d21620 100644
--- a/drivers/staging/easycap/easycap.h
+++ b/drivers/staging/easycap/easycap.h
@@ -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

RE: [PATCH 4/5] staging/easycap: PAGE_SIZE is always defined

[Winkler, Tomas]

> -----Original Message-----
> From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> Sent: Monday, June 13, 2011 3:19 PM
> To: Greg Kroah-Hartman
> Cc: Winkler, Tomas; Mike Thomas; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; 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 <tomas.winkler@intel.com>
> Cc: Mike Thomas <rmthomas@sciolus.org>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
Acked-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/staging/easycap/easycap.h |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/easycap/easycap.h
> b/drivers/staging/easycap/easycap.h
> index 1f94e23..5d21620 100644
> --- a/drivers/staging/easycap/easycap.h
> +++ b/drivers/staging/easycap/easycap.h
> @@ -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

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 5/5] staging/easycap: Fix bytesperline calculation

[Kirill Smelkov]

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 <tomas.winkler@intel.com>
Cc: Mike Thomas <rmthomas@sciolus.org>
Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
---
 drivers/staging/easycap/easycap_settings.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/easycap/easycap_settings.c b/drivers/staging/easycap/easycap_settings.c
index 898559d..70f59b1 100644
--- a/drivers/staging/easycap/easycap_settings.c
+++ b/drivers/staging/easycap/easycap_settings.c
@@ -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

Re: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

[Dan Carpenter]

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

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

Because this is crap code from the staging dir.

regards,
dan carpenter

Re: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

[Kirill Smelkov]

On Mon, Jun 13, 2011 at 04:23:45PM +0300, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> > Sent: Monday, June 13, 2011 3:19 PM
> > To: Greg Kroah-Hartman
> > Cc: Winkler, Tomas; Mike Thomas; devel@driverdev.osuosl.org; linux-
> > kernel@vger.kernel.org; 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?
> 
> 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


> context and I agree it looks ugly here , yet I personally usually avoid using bit fields. 
> 
> > 
> > Cc: Tomas Winkler <tomas.winkler@intel.com>
> > Cc: Mike Thomas <rmthomas@sciolus.org>
> > Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> 
> 
> Acked-by:  Tomas Winkler <tomas.winkler@intel.com>

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



On Mon, Jun 13, 2011 at 03:36:59PM +0300, Dan Carpenter wrote:
> It warms my heart to see a beautiful commit message like that,
> complete with diagrams.  :)

Thanks. It's a warm fuzzy feeling to receive feedback of this kind :)
thanks again.

> > 
> > P.S. why at all we use those mask/shifts? Why not use bitfields?
> 
> Because this is crap code from the staging dir.

I see, thanks.
Kirill

RE: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

[Winkler, Tomas]

> -----Original Message-----
> From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> Sent: Tuesday, June 14, 2011 7:37 PM
> To: Dan Carpenter; Winkler, Tomas
> Cc: Greg Kroah-Hartman; devel@driverdev.osuosl.org; Mike Thomas; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 5/5] staging/easycap: Fix bytesperline calculation
> 
> On Mon, Jun 13, 2011 at 04:23:45PM +0300, Winkler, Tomas wrote:
> >
> >
> > > -----Original Message-----
> > > From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> > > Sent: Monday, June 13, 2011 3:19 PM
> > > To: Greg Kroah-Hartman
> > > Cc: Winkler, Tomas; Mike Thomas; devel@driverdev.osuosl.org; linux-
> > > kernel@vger.kernel.org; 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?
> >
> > 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
> 

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

> 
> > context and I agree it looks ugly here , yet I personally usually avoid using
> bit fields.
> >
> > >
> > > Cc: Tomas Winkler <tomas.winkler@intel.com>
> > > Cc: Mike Thomas <rmthomas@sciolus.org>
> > > Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> >
> >
> > Acked-by:  Tomas Winkler <tomas.winkler@intel.com>
> 
> 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.

Thanks
Tomas
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

[Kirill Smelkov]

On Wed, Jun 15, 2011 at 12:01:57AM +0300, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> > Sent: Tuesday, June 14, 2011 7:37 PM
> > To: Dan Carpenter; Winkler, Tomas
> > Cc: Greg Kroah-Hartman; devel@driverdev.osuosl.org; Mike Thomas; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 5/5] staging/easycap: Fix bytesperline calculation
> > 
> > On Mon, Jun 13, 2011 at 04:23:45PM +0300, Winkler, Tomas wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> > > > Sent: Monday, June 13, 2011 3:19 PM
> > > > To: Greg Kroah-Hartman
> > > > Cc: Winkler, Tomas; Mike Thomas; devel@driverdev.osuosl.org; linux-
> > > > kernel@vger.kernel.org; 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?
> > >
> > > 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
> > 
> 
> Still I don't like these macro templates either but that's probably personal taste only I wouldn't  NACK it.

Yes, we'd better have something like
__attribute__(le_bitfiels/be_bitfields), but up to date this macro is
less ugliest one.


Though I'm not engaging to refactor bitfilds in the kernel now.


> > > context and I agree it looks ugly here , yet I personally usually avoid using
> > bit fields.
> > >
> > > >
> > > > Cc: Tomas Winkler <tomas.winkler@intel.com>
> > > > Cc: Mike Thomas <rmthomas@sciolus.org>
> > > > Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> > >
> > >
> > > Acked-by:  Tomas Winkler <tomas.winkler@intel.com>
> > 
> > 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

And thanks a lot for doing this.

With bytesperline fix I've made my easycap work, but the video was
coming with visaully seen jitter and because of very tight schedule to
get my particular task done, I've also tried em28xx based card which
worked without jitter.


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

Same here and s/life/work/ :)

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

Yes, easycap was 2 times cheaper than em28xx based card. I still have
both of them and interested in getting easycap into shape, because
besides everything else, physically easycap dongle is significantly
smaller and compact.


Thanks again for your work on easycap, Tomas and Mike,

Kirill

RE: [PATCH 5/5] staging/easycap: Fix bytesperline calculation

[Winkler, Tomas]

> -----Original Message-----
> From: Kirill Smelkov [mailto:kirr@mns.spb.ru]
> Sent: Monday, June 13, 2011 3:19 PM
> To: Greg Kroah-Hartman
> Cc: Winkler, Tomas; Mike Thomas; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; 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?

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. 

> 
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Cc: Mike Thomas <rmthomas@sciolus.org>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>


Acked-by:  Tomas Winkler <tomas.winkler@intel.com>

> ---
>  drivers/staging/easycap/easycap_settings.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/easycap/easycap_settings.c
> b/drivers/staging/easycap/easycap_settings.c
> index 898559d..70f59b1 100644
> --- a/drivers/staging/easycap/easycap_settings.c
> +++ b/drivers/staging/easycap/easycap_settings.c
> @@ -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

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.