Re: [PATCH 05/24] media: v4l2-dev: convert VFL_TYPE_* into an enum

[Andrey Utkin]

On Mon, Oct 09, 2017 at 07:19:11AM -0300, Mauro Carvalho Chehab wrote:
> Using enums makes easier to document, as it can use kernel-doc
> markups. It also allows cross-referencing, with increases the
> kAPI readability.
> 


All changes look legit.

But I'd expect cx88_querycap() return type change and such to be in
separate commit.

> diff --git a/drivers/media/pci/cx88/cx88-blackbird.c 
> b/drivers/media/pci/cx88/cx88-blackbird.c
> index e3101f04941c..0e0952e60795 100644
> --- a/drivers/media/pci/cx88/cx88-blackbird.c
> +++ b/drivers/media/pci/cx88/cx88-blackbird.c
> @@ -805,8 +805,7 @@ static int vidioc_querycap(struct file *file, void  *priv,
>  
>   strcpy(cap->driver, "cx88_blackbird");
>   sprintf(cap->bus_info, "PCI:%s", pci_name(dev->pci));
> - cx88_querycap(file, core, cap);
> - return 0;
> + return cx88_querycap(file, core, cap);
>  }
>  
>  static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
> diff --git a/drivers/media/pci/cx88/cx88-video.c 
> b/drivers/media/pci/cx88/cx88-video.c
> index 7d25ecd4404b..9be682cdb644 100644
> --- a/drivers/media/pci/cx88/cx88-video.c
> +++ b/drivers/media/pci/cx88/cx88-video.c
> @@ -806,8 +806,8 @@ static int vidioc_s_fmt_vid_cap(struct file *file, void 
> *priv,
>   return 0;
>  }
>  
> -void cx88_querycap(struct file *file, struct cx88_core *core,
> -struct v4l2_capability *cap)
> +int cx88_querycap(struct file *file, struct cx88_core *core,
> +   struct v4l2_capability *cap)
>  {
>   struct video_device *vdev = video_devdata(file);
>  

Re: [PATCH] [media] solo6x10: export hardware GPIO pins 8:31 to gpiolib interface

[Andrey Utkin]

Hi Anton,

Nothing serious, just some purist nitpicking below.

On Wed, Aug 02, 2017 at 06:17:02PM +0400, Anton Sviridenko wrote:
> 24 GPIO pins from 32 available on solo6x10 chips are exported
> to gpiolib. First 8 GPIOs are reserved for internal use on capture card
> boards, GPIOs in range 8:15 are configured as outputs to control relays,
> remaining 16:31 are configured as inputs to read sensor states.
> Now with this patch userspace DVR software can switch relays and read
> sensor states when GPIO extension cards are attached to Softlogic solo6x10
> based video capture cards.
> 
> Signed-off-by: Anton Sviridenko 
> ---
>  drivers/media/pci/solo6x10/solo6x10-gpio.c | 97 
> ++
>  drivers/media/pci/solo6x10/solo6x10.h  |  5 ++
>  2 files changed, 102 insertions(+)
> 
> diff --git a/drivers/media/pci/solo6x10/solo6x10-gpio.c 
> b/drivers/media/pci/solo6x10/solo6x10-gpio.c
> index 6d3b4a36bc11..3d0d1aa2f6a8 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-gpio.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-gpio.c
> @@ -57,6 +57,9 @@ static void solo_gpio_mode(struct solo_dev *solo_dev,
>   ret |= 1 << port;
>   }
>  
> + /* Enable GPIO[31:16] */
> + ret |= 0x;
> +
>   solo_reg_write(solo_dev, SOLO_GPIO_CONFIG_1, ret);
>  }
>  
> @@ -90,16 +93,110 @@ static void solo_gpio_config(struct solo_dev *solo_dev)
>  

>   /* Initially set relay status to 0 */

Do you mean that relay is initially disabled?
Maybe a rewording would make it clearer.

>   solo_gpio_clear(solo_dev, 0xff00);
> +

> + /* Set input pins direction */

IMHO "Configure pins 16:31 as inputs" would be easier to read.

> + solo_gpio_mode(solo_dev, 0x, 0);
> +}
> +
> +#ifdef CONFIG_GPIOLIB

> +/* Pins 0-7 are not exported, because it seems from code above they are

Comment opening ("/*") is usually left on empty line without text.
The style you have here is discouraged.

Also, the comment reveals that you are not sure ("it seems from the code
above..."). Please research and/or ask somebody to gain confidence, or
state your lack of confidence in cover letter, but keep uncertainity out
of the codebase. That's abstract advice, sorry I can't now check the
actual issue to resolve this uncertainity.

Also, is there a chance that somebody, e.g. you, may want to edit or at
last to check in runtime how first 8 pins are configured, e.g. in
debugging purposes? This is an argument for using offset 0 for physical
pin 0.

> + * used for internal purposes. So offset 0 corresponds to pin 8, therefore
> + * offsets 0-7 are relay GPIOs, 8-23 - input GPIOs.
> + */
> +static int solo_gpiochip_get_direction(struct gpio_chip *chip,
> +unsigned int offset)
> +{
> + int ret, mode;
> + struct solo_dev *solo_dev = gpiochip_get_data(chip);
> +
> + if (offset < 8) {
> + ret = solo_reg_read(solo_dev, SOLO_GPIO_CONFIG_0);

> + mode = 3 & (ret >> ((offset + 8) * 2));

Mask of 0x3 is used, but result other than 0 and 1 yields "return -1". I
haven't checked the spec, this just looks suspicious. Is the code
correct?

Maybe some comment on meaning of "mode" value would help.

> + } else {
> + ret = solo_reg_read(solo_dev, SOLO_GPIO_CONFIG_1);
> + mode =  1 & (ret >> (offset - 8));
> + }
> +
> + if (!mode)
> + return 1;
> + else if (mode == 1)
> + return 0;
> +
> + return -1;
>  }
>  
> +static int solo_gpiochip_direction_input(struct gpio_chip *chip,
> +  unsigned int offset)
> +{
> + return -1;
> +}
> +
> +static int solo_gpiochip_direction_output(struct gpio_chip *chip,
> +   unsigned int offset, int value)
> +{
> + return -1;
> +}
> +

> +static int solo_gpiochip_get(struct gpio_chip *chip,
> + unsigned int offset)

line continuation is misaligned

> +{
> + int ret;
> + struct solo_dev *solo_dev = gpiochip_get_data(chip);
> +
> + ret = solo_reg_read(solo_dev, SOLO_GPIO_DATA_IN);
> +
> + return 1 & (ret >> (offset + 8));
> +}
> +

> +static void solo_gpiochip_set(struct gpio_chip *chip,
> + unsigned int offset, int value)

line continuation is misaligned

> +{
> + struct solo_dev *solo_dev = gpiochip_get_data(chip);
> +
> + if (value)
> + solo_gpio_set(solo_dev, 1 << (offset + 8));
> + else
> + solo_gpio_clear(solo_dev, 1 << (offset + 8));
> +}
> +#endif
> +
>  int solo_gpio_init(struct solo_dev *solo_dev)
>  {
> + int ret;
> +
>   solo_gpio_config(solo_dev);
> +#ifdef CONFIG_GPIOLIB

> + solo_dev->gpio_dev.label = SOLO6X10_NAME"_gpio";

my eyeballs as well as 'checkpatch.pl --strict' say:

Concatenated strings should use spaces between elements

> diff --git 

Re: [PATCH] [media] solo6x10: make const array saa7128_regs_ntsc static

[Andrey Utkin]

On Mon, Jul 10, 2017 at 07:51:03PM +0100, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Don't populate const array saa7128_regs_ntsc on the stack but insteaed make
> it static.  Makes the object code smaller and saves nearly 840 bytes
> 
> Before:
>text  data bss dec hex filename
>9218   360   09578256a solo6x10-tw28.o
> 
> After:
>text  data bss dec hex filename
>8237   504   087412225 solo6x10-tw28.o
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Acked-by: Andrey Utkin <andrey_ut...@fastmail.com>

Re: [PATCH] [media] solo6x10: fix detection of TW2864B chips

[Andrey Utkin]

On Sat, Jul 01, 2017 at 03:26:01PM +0400, Anton Sviridenko wrote:
> This patch enables support for non-Bluecherry labeled solo6110
> based PCI cards which have 3 x TW2864B chips and one TW2865.
> These cards are displayed by lspci -nn as
> 
> "Softlogic Co., Ltd. SOLO6110 H.264 Video encoder/decoder [9413:6110]"
> 
> Bluecherry cards have 4 x TW2864A. According to datasheet register 0xFF
> of TW2864B chips contains value 0x6A or 0x6B depending on revision 
> which being shifted 3 bits right gives value 0x0d.
> Existing version of solo6x10 fails on these cards with
> 
> [276582.344942] solo6x10 :07:00.0: Probing Softlogic 6110
> [276582.402151] solo6x10 :07:00.0: Could not initialize any techwell chips
> [276582.402781] solo6x10: probe of :07:00.0 failed with error -22
> 
> Signed-off-by: Anton Sviridenko <an...@corp.bluecherry.net>

Acked-by: Andrey Utkin <andrey_ut...@fastmail.com>

I have looked into same case long time ago, just haven't managed to
conclude.

[PATCH 2/2] MAINTAINERS: solo6x10: update Andrey Utkin email

[Andrey Utkin]

Updating my personal email address in solo6x10.

Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 026af206660b..885badfa4fa7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11966,7 +11966,7 @@ SOFTLOGIC 6x10 MPEG CODEC
 M: Bluecherry Maintainers <maintain...@bluecherrydvr.com>
 M: Anton Sviridenko <an...@corp.bluecherry.net>
 M: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
-M: Andrey Utkin <andrey.krieger.ut...@gmail.com>
+M: Andrey Utkin <andrey_ut...@fastmail.com>
 M: Ismael Luceno <ism...@iodev.co.uk>
 L: linux-media@vger.kernel.org
 S: Supported
-- 
2.13.0

[PATCH 1/2] MAINTAINERS: solo6x10, tw5864: add Anton Sviridenko

[Andrey Utkin]

Anton Sviridenko is now in charge of drivers in Bluecherry.

Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 053c3bdd1fe5..026af206660b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11964,6 +11964,7 @@ F:  drivers/leds/leds-net48xx.c
 
 SOFTLOGIC 6x10 MPEG CODEC
 M: Bluecherry Maintainers <maintain...@bluecherrydvr.com>
+M: Anton Sviridenko <an...@corp.bluecherry.net>
 M: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
 M: Andrey Utkin <andrey.krieger.ut...@gmail.com>
 M: Ismael Luceno <ism...@iodev.co.uk>
@@ -12911,6 +12912,7 @@ F:  Documentation/media/v4l-drivers/tm6000*
 
 TW5864 VIDEO4LINUX DRIVER
 M: Bluecherry Maintainers <maintain...@bluecherrydvr.com>
+M: Anton Sviridenko <an...@corp.bluecherry.net>
 M: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
 M: Andrey Utkin <andrey_ut...@fastmail.com>
 L: linux-media@vger.kernel.org
-- 
2.13.0

Re: [PATCH 1/4] [media] tw5864, fc0011: better handle WARN_ON()

[Andrey Utkin]

On Thu, May 18, 2017 at 11:06:43AM -0300, Mauro Carvalho Chehab wrote:
> As such macro will check if the expression is true, it may fall through, as
> warned:

> On both cases, it means an error, so, let's return an error
> code, to make gcc happy.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/pci/tw5864/tw5864-video.c | 1 +
>  drivers/media/tuners/fc0011.c   | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
> b/drivers/media/pci/tw5864/tw5864-video.c
> index 2a044be729da..e7bd2b8484e3 100644
> --- a/drivers/media/pci/tw5864/tw5864-video.c
> +++ b/drivers/media/pci/tw5864/tw5864-video.c
> @@ -545,6 +545,7 @@ static int tw5864_fmt_vid_cap(struct file *file, void 
> *priv,
>   switch (input->std) {
>   default:
>   WARN_ON_ONCE(1);
> + return -EINVAL;
>   case STD_NTSC:
>   f->fmt.pix.height = 480;
>   break;

Hi Mauro,

Thanks for the patch.

I actually meant it to fall through, but I agree this is not how it
should be.
I'm fine with this patch. Unfortunately I don't possess tw5864 hardware
now. CCing Anton Sviridenko whom I've handed it (I guess he's on
Bluecherry Maintainers groupmail as well).

Anton, just in case, could you please ensure the driver with this patch
works well in runtime?

Re: [PATCH] [media] solo6x10: release vb2 buffers in solo_stop_streaming()

[Andrey Utkin]

Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
Signed-off-by: Andrey Utkin <andrey_ut...@fastmail.com>

Please welcome Anton who is now in charge of solo6x10 and tw5864 support
and development in Bluecherry company, I have sent out to him the
hardware samples I possessed. (We will prepare the patch updating
MAINTAINERS file soon.)

If anybody has any outstanding complains, concerns or tasks regarding
solo6x10 and tw5864 drivers, I think now is good occasion to let us know
about it.

Re: [PATCH] [media] tveeprom: get rid of unused arg on tveeprom_hauppauge_analog()

[Andrey Utkin]

On Fri, Mar 03, 2017 at 07:33:43AM -0300, Mauro Carvalho Chehab wrote:
> tveeprom_hauppauge_analog() used to need the I2C adapter in
> order to print debug messages. As it now uses pr_foo() facilities,
> this is not needed anymore.
> 
> So, get rid of it.

Hi  Mauro,

Thanks for CCing :)

> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c 
> b/drivers/media/usb/em28xx/em28xx-cards.c
> index 5f90d0899a45..5f80a1b2fb8c 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -2974,8 +2974,7 @@ static void em28xx_card_setup(struct em28xx *dev)
>  #endif
>   /* Call first TVeeprom */
>  
> - dev->i2c_client[dev->def_i2c_bus].addr = 0xa0 >> 1;
> - tveeprom_hauppauge_analog(>i2c_client[dev->def_i2c_bus], 
> , dev->eedata);
> + tveeprom_hauppauge_analog(, dev->eedata);

Are we sure no other code relies on
dev->i2c_client[dev->def_i2c_bus].addr being set here?
I am completely not familiar with this driver, that's why I'm asking.

Re: [PATCH] [media] tw5864: use dev_warn instead of WARN to shut up warning

[Andrey Utkin]

On Tue, Feb 28, 2017 at 10:14:37PM +0100, Arnd Bergmann wrote:
> tw5864_frameinterval_get() only initializes its output when it successfully
> identifies the video standard in tw5864_input. We get a warning here because
> gcc can't always track the state if initialized warnings across a WARN()
> macro, and thinks it might get used incorrectly in tw5864_s_parm:
> 
> media/pci/tw5864/tw5864-video.c: In function 'tw5864_s_parm':
> media/pci/tw5864/tw5864-video.c:816:38: error: 'time_base.numerator' may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]
> media/pci/tw5864/tw5864-video.c:819:31: error: 'time_base.denominator' may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> Using dev_warn() instead of WARN() avoids the __branch_check__() in
> unlikely and lets the compiler see that the initialization is correct.
> 
> Signed-off-by: Arnd Bergmann <a...@arndb.de>

Thanks for the patch.
Acked-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>

See the note below.

> ---
>  drivers/media/pci/tw5864/tw5864-video.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
> b/drivers/media/pci/tw5864/tw5864-video.c
> index 9421216bb942..4d9994a11c22 100644
> --- a/drivers/media/pci/tw5864/tw5864-video.c
> +++ b/drivers/media/pci/tw5864/tw5864-video.c
> @@ -717,6 +717,8 @@ static void tw5864_frame_interval_set(struct tw5864_input 
> *input)
>  static int tw5864_frameinterval_get(struct tw5864_input *input,
>   struct v4l2_fract *frameinterval)
>  {
> + struct tw5864_dev *dev = input->root;
> +
>   switch (input->std) {
>   case STD_NTSC:
>   frameinterval->numerator = 1001;
> @@ -728,8 +730,8 @@ static int tw5864_frameinterval_get(struct tw5864_input 
> *input,
>   frameinterval->denominator = 25;
>   break;
>   default:
> - WARN(1, "tw5864_frameinterval_get requested for unknown std 
> %d\n",
> -  input->std);
> + dev_warn(>pci->dev, "tw5864_frameinterval_get requested 
> for unknown std %d\n",
> +  input->std);
>   return -EINVAL;
>   }

Looks good, though, arguably, it could fit 80 columns better if you put
the string literal to separate line.

Re: [PATCH] [media] tw5864: handle unknown video std gracefully

[Andrey Utkin]

On Tue, Feb 28, 2017 at 09:20:53AM +0100, Arnd Bergmann wrote:
> On Tue, Feb 28, 2017 at 2:08 AM, Andrey Utkin
> <andrey.ut...@corp.bluecherry.net> wrote:
> > Retcode checking takes place everywhere, but currently it overwrites
> > supplied structs with potentially-uninitialized values. To make it
> > cleaner, it should be (e.g. tw5864_g_parm())
> >
> > ret = tw5864_frameinterval_get(input, >timeperframe);
> > if (ret)
> > return ret;
> > cp->timeperframe.numerator *= input->frame_interval;
> > cp->capturemode = 0;
> > cp->readbuffers = 2;
> > return 0;
> >
> > and not
> >
> > ret = tw5864_frameinterval_get(input, >timeperframe);
> > cp->timeperframe.numerator *= input->frame_interval;
> > cp->capturemode = 0;
> > cp->readbuffers = 2;
> > return ret;
> >
> > That would resolve your concerns of uninitialized values propagation
> > without writing bogus values 1/1 in case of failure. I think I'd
> > personally prefer a called function to leave my data structs intact when
> > it fails.
> 
> That seems reasonable, I can try to come up with a new version that
> incorporates this change, but I haven't been able to avoid the warning
> without either removing the WARN() or adding an initialization.

I don't mind dropping WARN().

Thanks for your elaborate reply.

Re: [PATCH] [media] tw5864: handle unknown video std gracefully

[Andrey Utkin]

Hi Arnd,

Thanks for sending this patch.

On Mon, Feb 27, 2017 at 09:32:34PM +0100, Arnd Bergmann wrote:
> tw5864_frameinterval_get() only initializes its output when it successfully
> identifies the video standard in tw5864_input. We get a warning here because
> gcc can't always track the state if initialized warnings across a WARN()
> macro, and thinks it might get used incorrectly in tw5864_s_parm:
> 
> media/pci/tw5864/tw5864-video.c: In function 'tw5864_s_parm':
> media/pci/tw5864/tw5864-video.c:816:38: error: 'time_base.numerator' may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]
> media/pci/tw5864/tw5864-video.c:819:31: error: 'time_base.denominator' may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]

I think behaviour of tw5864_frameinterval_get() is ok.
I don't see how WARN() could affect gcc state tracking. There's "return
-EINVAL" right after WARN() which lets caller handle the failure case
gracefully. Maybe I just don't see how confusing WARN() can be for gcc
in this situation, but it's not as confusing as BUG() would be, right?

I see the reason of that warning is 

 - time_base being not initialized in tw5864_s_parm()
 - gcc being too dumb to recognize that we have checked the retcode in
   tw5864_s_parm() and proceed only when we are sure we have correctly
   initialized time_base.

Is that you compiling with manually added -Werror=maybe-uninitialized or
is that default compilation flags? I don't remember encountering that
and I doubt a lot of kernel code compiles without warnings with such
flag.

Also, which GCC version are you using?

> This particular use happens to be ok, but we do copy the uninitialized
> output of tw5864_frameinterval_get() into other memory without checking
> the return code, interestingly without getting a warning here.

Retcode checking takes place everywhere, but currently it overwrites
supplied structs with potentially-uninitialized values. To make it
cleaner, it should be (e.g. tw5864_g_parm())

ret = tw5864_frameinterval_get(input, >timeperframe);
if (ret)
return ret;
cp->timeperframe.numerator *= input->frame_interval;
cp->capturemode = 0;
cp->readbuffers = 2;
return 0;

and not

ret = tw5864_frameinterval_get(input, >timeperframe);
cp->timeperframe.numerator *= input->frame_interval;
cp->capturemode = 0;
cp->readbuffers = 2;
return ret;

That would resolve your concerns of uninitialized values propagation
without writing bogus values 1/1 in case of failure. I think I'd
personally prefer a called function to leave my data structs intact when
it fails.

> 
> This initializes the output to 1/1s for the case, to make sure we do
> get an intialization that doesn't cause a division-by-zero exception
> in case we end up using this uninitialized data later.

Personally I won't object against such patch, but I find it a bit too
much "defensive" for kernel coding taste.

Making sure somebody who doesn't check return codes don't get a crash is
traditionally not considered a valid concern AFAIK.

Please let me know what you think about this.

Re: [PATCH 4/6] [media] tw5864: improve subscribe event handling

[Andrey Utkin]

Hi Gustavo,

Thanks for the patches, and sorry for delay.

Acked-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>

Re: [PATCH 3/6] [media] solo6x10: improve subscribe event handling

[Andrey Utkin]

Hi Gustavo,

Thanks for the patches, and sorry for delay.

Acked-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>

Re: [PATCH] solo6x10: use designated initializers

[Andrey Utkin]

On Fri, Jan 06, 2017 at 01:21:10PM -0800, Kees Cook wrote:
> > Since `ops` is static, what about this?
> > For the variant given below, you have my signoff.
> >
> >> --- a/drivers/media/pci/solo6x10/solo6x10-g723.c
> >> +++ b/drivers/media/pci/solo6x10/solo6x10-g723.c
> >> @@ -350,7 +350,7 @@ static int solo_snd_pcm_init(struct solo_dev *solo_dev)
> >>
> >>  int solo_g723_init(struct solo_dev *solo_dev)
> >>  {
> >> - static struct snd_device_ops ops = { NULL };
> >> + static struct snd_device_ops ops;
> 
> Ah! Yes, thanks. That works fine too. :) Can this be const as well?

No, it can't be const, it's used as parameter for snd_device_new() which
takes "struct snd_device_ops *".
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] solo6x10: use designated initializers

[Andrey Utkin]

On Fri, Dec 16, 2016 at 05:05:36PM -0800, Kees Cook wrote:
> Prepare to mark sensitive kernel structures for randomization by making
> sure they're using designated initializers. These were identified during
> allyesconfig builds of x86, arm, and arm64, with most initializer fixes
> extracted from grsecurity.

Ok I've reviewed all the patchset, googled a bit and now I see what's
going on.

> 
> Signed-off-by: Kees Cook 
> ---
>  drivers/media/pci/solo6x10/solo6x10-g723.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/pci/solo6x10/solo6x10-g723.c 
> b/drivers/media/pci/solo6x10/solo6x10-g723.c
> index 6a35107aca25..36e93540bb49 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-g723.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-g723.c
> @@ -350,7 +350,7 @@ static int solo_snd_pcm_init(struct solo_dev *solo_dev)
>  
>  int solo_g723_init(struct solo_dev *solo_dev)
>  {
> - static struct snd_device_ops ops = { NULL };
> + static struct snd_device_ops ops = { };

I'm not that keen on syntax subtleties, but...
 * Empty initializer is not quite "designated" as I can judge.
 * From brief googling I see that empty initializer is not valid in
   some C standards.

Since `ops` is static, what about this?
For the variant given below, you have my signoff.

> --- a/drivers/media/pci/solo6x10/solo6x10-g723.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-g723.c
> @@ -350,7 +350,7 @@ static int solo_snd_pcm_init(struct solo_dev *solo_dev)
>  
>  int solo_g723_init(struct solo_dev *solo_dev)
>  {
> - static struct snd_device_ops ops = { NULL };
> + static struct snd_device_ops ops;
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [media] cx88: make checkpatch.pl happy

[Andrey Utkin]

Thanks for your hard work at beautification of this driver :)
>From reviewing the diff over v1, it looks good.

Also thanks for deep explanations you gave me for my comments.

On Sat, Nov 19, 2016 at 07:27:30PM -0200, Mauro Carvalho Chehab wrote:
> 
> Suggested-by: Andrey Utkin <andrey_ut...@fastmail.com>
> Fixes: 65bc2fe86e66 ("[media] cx88: convert it to use pr_foo() macros")
> Fixes: 7b61ba8ff838 ("[media] cx88: make checkpatch happier")
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> ---

Reviewed-by: Andrey Utkin <andrey_ut...@fastmail.com>

> --- a/drivers/media/pci/cx88/cx88-input.c
> +++ b/drivers/media/pci/cx88/cx88-input.c
> @@ -62,11 +62,15 @@ static int ir_debug;
>  module_param(ir_debug, int, 0644);   /* debug level [IR] */
>  MODULE_PARM_DESC(ir_debug, "enable debug messages [IR]");
>  
> -#define ir_dprintk(fmt, arg...)  if (ir_debug) \
> - printk(KERN_DEBUG "%s IR: " fmt, ir->core->name, ##arg)
> +#define ir_dprintk(fmt, arg...)  do {
> \
> + if (ir_debug)   \
> + printk(KERN_DEBUG "%s IR: " fmt, ir->core->name, ##arg);\
> +} while (0)

Oh ok, so when the patch is applied, the backslash doesn't stand out, it
just looks this way in the diff.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] cx88: make checkpatch.pl happy

[Andrey Utkin]

This is from checkpatch run on cx88 source files with "-f", not your
patch files, right? I guess it might produce less changes if run on
patches.

On Sat, Nov 19, 2016 at 10:14:05AM -0200, Mauro Carvalho Chehab wrote:
> Usually, I don't like fixing coding style issues on non-staging
> drivers, as it could be a mess pretty easy, and could become like
> a snow ball. That's the case of recent changes on two changesets:
> they disalign some statements.

In my understanding, commits dedicated to style fixes on non-staging are
discouraged because they clutter git log and "git blame" view. But new
commits are encouraged to be style-perfect.

And in case of discussed alignment breakage, I expected that you make
this your fixup (the current patch) really a git-ish fixup and just
merge it into 09/35 patch. As I see it's published in media tree master
already and you are not going to force-push there; maybe a bit of
latency in pushing patches to media tree after publishing them for
review would prevent this sort of inconvenience.

P. S. (Running away from rotten tomatoes) another way to avoid such
painful alignment issues is to legalize "one-more-tab" indentation for
trailing parts of lines, alignment on opening brace is brittle IMHO.


> --- a/drivers/media/pci/cx88/cx88-blackbird.c
> +++ b/drivers/media/pci/cx88/cx88-blackbird.c

> @@ -1061,7 +1092,8 @@ static int cx8802_blackbird_advise_acquire(struct 
> cx8802_driver *drv)
>  
>   switch (core->boardnr) {
>   case CX88_BOARD_HAUPPAUGE_HVR1300:
> - /* By default, core setup will leave the cx22702 out of reset, 
> on the bus.
> + /* By default, core setup will leave the cx22702 out of reset,
> +  * on the bus.
>* We left the hardware on power up with the cx22702 active.
>* We're being given access to re-arrange the GPIOs.
>* Take the bus off the cx22702 and put the cx23416 on it.

Let first line with "/*" be empty :)

> --- a/drivers/media/pci/cx88/cx88-core.c
> +++ b/drivers/media/pci/cx88/cx88-core.c

> @@ -102,28 +104,29 @@ static __le32 *cx88_risc_field(__le32 *rp, struct 
> scatterlist *sglist,
>   sol = RISC_SOL | RISC_IRQ1 | RISC_CNT_INC;
>   else
>   sol = RISC_SOL;
> - if (bpl <= sg_dma_len(sg)-offset) {
> + if (bpl <= sg_dma_len(sg) - offset) {
>   /* fits into current chunk */
> - *(rp++) = cpu_to_le32(RISC_WRITE|sol|RISC_EOL|bpl);
> - *(rp++) = cpu_to_le32(sg_dma_address(sg)+offset);
> + *(rp++) = cpu_to_le32(RISC_WRITE | sol |
> +   RISC_EOL | bpl);
> + *(rp++) = cpu_to_le32(sg_dma_address(sg) + offset);
>   offset += bpl;
>   } else {
>   /* scanline needs to be split */
>   todo = bpl;
> - *(rp++) = cpu_to_le32(RISC_WRITE|sol|
> - (sg_dma_len(sg)-offset));
> - *(rp++) = cpu_to_le32(sg_dma_address(sg)+offset);
> - todo -= (sg_dma_len(sg)-offset);
> + *(rp++) = cpu_to_le32(RISC_WRITE | sol |
> + (sg_dma_len(sg) - offset));

This is strange, but checkpatch --strict is really happy on this,
however there is a misalignment in added lines. Going to look into this
later.

> --- a/drivers/media/pci/cx88/cx88-input.c
> +++ b/drivers/media/pci/cx88/cx88-input.c
> @@ -62,11 +62,15 @@ static int ir_debug;
>  module_param(ir_debug, int, 0644);   /* debug level [IR] */
>  MODULE_PARM_DESC(ir_debug, "enable debug messages [IR]");
>  
> -#define ir_dprintk(fmt, arg...)  if (ir_debug) \
> - printk(KERN_DEBUG "%s IR: " fmt, ir->core->name, ##arg)
> +#define ir_dprintk(fmt, arg...)  do {
> \

Backslash stands out.

> --- a/drivers/media/pci/cx88/cx88-reg.h
> +++ b/drivers/media/pci/cx88/cx88-reg.h
> @@ -1,32 +1,32 @@
>  /*
> -
> -cx88x-hw.h - CX2388x register offsets
> -
> -Copyright (C) 1996,97,98 Ralph Metzler (r...@thp.uni-koeln.de)
> -   2001 Michael Eskin
> -   2002 Yurij Sysoev 
> -   2003 Gerd Knorr 
> -
> -This program is free software; you can redistribute it and/or modify
> -it under the terms of the GNU General Public License as published by
> -the Free Software Foundation; either version 2 of the License, or
> -(at your option) any later version.
> -
> -This program is distributed in the hope that it will be useful,
> -but WITHOUT ANY WARRANTY; without even the implied warranty of
> -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> -GNU General Public License for more details.
> -
> -You should have received a copy of the GNU General Public 

Re: [PATCH 08/35] [media] cx88: convert it to use pr_foo() macros

[Andrey Utkin]

On Wed, Nov 16, 2016 at 02:42:40PM -0200, Mauro Carvalho Chehab wrote:
> From: Mauro Carvalho Chehab <mche...@osg.samsung.com>
> 
> Instead of calling printk() directly, use pr_foo()
> macros, as suggested at the Kernel's coding style.
> 
> Please notice that a conversion to dev_foo() is not trivial,
> as several parts on this driver uses pr_cont().

Haven't followed closely the current discussion about line continuation,
so commenting on logical part is not up to me, at last I don't see
anything weird. So I will be an alignment-proofreading monkey :)

> 
> Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Reviewed-by: Andrey Utkin <andrey_ut...@fastmail.com>

> --- a/drivers/media/pci/cx88/cx88-cards.c
> +++ b/drivers/media/pci/cx88/cx88-cards.c

> @@ -3646,8 +3626,8 @@ static int cx88_pci_quirks(const char *name, struct 
> pci_dev *pci)
>   pci_write_config_byte(pci, CX88X_DEVCTRL, value);
>   }
>   if (UNSET != lat) {
> - printk(KERN_INFO "%s: setting pci latency timer to %d\n",
> -name, latency);
> + pr_info("setting pci latency timer to %d\n",
> + latency);

Can fit single line.
This wasn't handled by checkpatch in next patch, so manual fix would be
nice.

> --- a/drivers/media/pci/cx88/cx88-core.c
> +++ b/drivers/media/pci/cx88/cx88-core.c

> @@ -60,10 +61,15 @@ static unsigned int nocomb;
>  module_param(nocomb,int,0644);
>  MODULE_PARM_DESC(nocomb,"disable comb filter");
>  
> -#define dprintk(level,fmt, arg...)   do {\
> - if (cx88_core_debug >= level)   \
> - printk(KERN_DEBUG "%s: " fmt, core->name , ## arg); \
> - } while(0)
> +#define dprintk0(fmt, arg...)\
> + printk(KERN_DEBUG pr_fmt("%s: core:" fmt),  \
> + __func__, ##arg)\
> +

Could fit single line

> @@ -399,12 +405,12 @@ static int cx88_risc_decode(u32 risc)
>   };
>   int i;
>  
> - printk(KERN_DEBUG "0x%08x [ %s", risc,
> + dprintk0("0x%08x [ %s", risc,
>  instr[risc >> 28] ? instr[risc >> 28] : "INVALID");

Alignment got broken here and in quite some similar places :(
And checkpatch hasn't gone after it. What if you run it with --strict
--fix-inplace to make it check brace alignment and fix it at once?

And then make sure to run it again because it seems to fix one error at
a time.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/35] [media] cx88: make checkpatch happier

[Andrey Utkin]

On Wed, Nov 16, 2016 at 02:42:41PM -0200, Mauro Carvalho Chehab wrote:
> This driver is old, and have lots of checkpatch violations.
> As we're touching a lot on this driver due to the printk
> conversions, let's run checkpatch --fix on it, in order to
> solve some of those issues. Also, let's remove the FSF
> address and use the usual coding style for the initial comments.

Good idea to give checkpatch a run.
Good job by checkpatch, really powerful tool.

Have proofread, no weirdness except for few places where vertical
"table-alike" alignment across lines got broken.

> 
> Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Reviewed-by: Andrey Utkin <andrey_ut...@fastmail.com>

> --- a/drivers/media/pci/cx88/cx88-cards.c
> +++ b/drivers/media/pci/cx88/cx88-cards.c

> @@ -2911,33 +2906,33 @@ static const struct {
>   int  fm;
>   const char *name;
>  } gdi_tuner[] = {
> - [ 0x01 ] = { .id   = UNSET,
> + [0x01] = { .id   = UNSET,
>.name = "NTSC_M" },

Alignment got broken

> --- a/drivers/media/pci/cx88/cx88-vbi.c
> +++ b/drivers/media/pci/cx88/cx88-vbi.c

> @@ -57,9 +57,9 @@ static int cx8800_start_vbi_dma(struct cx8800_dev*dev,
>   cx88_sram_channel_setup(dev->core, _sram_channels[SRAM_CH24],
>   VBI_LINE_LENGTH, buf->risc.dma);
>  
> - cx_write(MO_VBOS_CONTROL, ( (1 << 18) |  // comb filter delay fixup
> + cx_write(MO_VBOS_CONTROL, ((1 << 18) |  // comb filter delay fixup

Alignment got broken.

> --- a/drivers/media/pci/cx88/cx88.h
> +++ b/drivers/media/pci/cx88/cx88.h

> @@ -385,8 +381,8 @@ struct cx88_core {
>   /* state info */
>   struct task_struct *kthread;
>   v4l2_std_idtvnorm;
> - unsigned   width, height;
> - unsigned   field;
> + unsigned int width, height;
> + unsigned int field;

Alignment got broken

> @@ -591,23 +587,23 @@ struct cx8802_dev {
>  /* --- */
>  
>  #define cx_read(reg) readl(core->lmmio + ((reg)>>2))
> -#define cx_write(reg,value)  writel((value), core->lmmio + ((reg)>>2))
> -#define cx_writeb(reg,value) writeb((value), core->bmmio + (reg))
> +#define cx_write(reg, value)  writel((value), core->lmmio + ((reg)>>2))
> +#define cx_writeb(reg, value) writeb((value), core->bmmio + (reg))

Alignment got broken

>  
> -#define cx_andor(reg,mask,value) \
> +#define cx_andor(reg, mask, value) \
>writel((readl(core->lmmio+((reg)>>2)) & ~(mask)) |\
>((value) & (mask)), core->lmmio+((reg)>>2))
> -#define cx_set(reg,bit)  cx_andor((reg),(bit),(bit))
> -#define cx_clear(reg,bit)cx_andor((reg),(bit),0)
> +#define cx_set(reg, bit)  cx_andor((reg), (bit), (bit))
> +#define cx_clear(reg, bit)cx_andor((reg), (bit), 0)

Alignment got broken
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [media] au0828-video: Use kcalloc() in au0828_init_isoc()

[Andrey Utkin]

On Mon, Oct 24, 2016 at 10:11:15PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 24 Oct 2016 23:28:44 +0100
> Andrey Utkin <andrey_ut...@fastmail.com> escreveu:
> 
> > On Mon, Oct 24, 2016 at 10:59:24PM +0200, SF Markus Elfring wrote:
> > > From: Markus Elfring <elfr...@users.sourceforge.net>
> > > Date: Mon, 24 Oct 2016 22:08:47 +0200
> > > 
> > > * Multiplications for the size determination of memory allocations
> > >   indicated that array data structures should be processed.
> > >   Thus use the corresponding function "kcalloc".
> > > 
> > >   This issue was detected by using the Coccinelle software.
> > > 
> > > * Replace the specification of data types by pointer dereferences
> > >   to make the corresponding size determination a bit safer according to
> > >   the Linux coding style convention.
> > > 
> > > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
> > > ---
> > >  drivers/media/usb/au0828/au0828-video.c | 11 +++
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/usb/au0828/au0828-video.c 
> > > b/drivers/media/usb/au0828/au0828-video.c
> > > index 85dd9a8..85b13c1 100644
> > > --- a/drivers/media/usb/au0828/au0828-video.c
> > > +++ b/drivers/media/usb/au0828/au0828-video.c
> > > @@ -221,15 +221,18 @@ static int au0828_init_isoc(struct au0828_dev *dev, 
> > > int max_packets,
> > >  
> > >   dev->isoc_ctl.isoc_copy = isoc_copy;
> > >   dev->isoc_ctl.num_bufs = num_bufs;
> > > -  
> > 
> > > - dev->isoc_ctl.urb = kzalloc(sizeof(void *)*num_bufs,  GFP_KERNEL);
> > > + dev->isoc_ctl.urb = kcalloc(num_bufs,
> > > + sizeof(*dev->isoc_ctl.urb),
> > > + GFP_KERNEL);  
> > 
> > What about this (for both hunks)?
> > 
> > -   dev->isoc_ctl.urb = kzalloc(sizeof(void *)*num_bufs,  GFP_KERNEL);
> > +   dev->isoc_ctl.urb =
> > +   kcalloc(num_bufs, sizeof(*dev->isoc_ctl.urb), GFP_KERNEL);

Now i see that also this should suit style better than original variant:

dev->isoc_ctl.urb = kcalloc(num_bufs, sizeof(*dev->isoc_ctl.urb),
GFP_KERNEL);

That's what vim with github.com/vivien/vim-linux-coding-style plugin
proposes.

> That's worse :)

I was about to send long emotional noobish bikeshedding rant arguing
with this point, but restrained from that keeping in mind that I want to
proceed contributing to the codebase successfully :) I'll keep my coding
style preferences for myself for a while.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] [media] au0828-video: Use kcalloc() in au0828_init_isoc()

[Andrey Utkin]

On Mon, Oct 24, 2016 at 10:59:24PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Mon, 24 Oct 2016 22:08:47 +0200
> 
> * Multiplications for the size determination of memory allocations
>   indicated that array data structures should be processed.
>   Thus use the corresponding function "kcalloc".
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Replace the specification of data types by pointer dereferences
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/media/usb/au0828/au0828-video.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/au0828/au0828-video.c 
> b/drivers/media/usb/au0828/au0828-video.c
> index 85dd9a8..85b13c1 100644
> --- a/drivers/media/usb/au0828/au0828-video.c
> +++ b/drivers/media/usb/au0828/au0828-video.c
> @@ -221,15 +221,18 @@ static int au0828_init_isoc(struct au0828_dev *dev, int 
> max_packets,
>  
>   dev->isoc_ctl.isoc_copy = isoc_copy;
>   dev->isoc_ctl.num_bufs = num_bufs;
> -

> - dev->isoc_ctl.urb = kzalloc(sizeof(void *)*num_bufs,  GFP_KERNEL);
> + dev->isoc_ctl.urb = kcalloc(num_bufs,
> + sizeof(*dev->isoc_ctl.urb),
> + GFP_KERNEL);

What about this (for both hunks)?

-   dev->isoc_ctl.urb = kzalloc(sizeof(void *)*num_bufs,  GFP_KERNEL);
+   dev->isoc_ctl.urb =
+   kcalloc(num_bufs, sizeof(*dev->isoc_ctl.urb), GFP_KERNEL);
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Andrey Utkin]

On Mon, Oct 24, 2016 at 05:32:33PM -0200, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Sep 2016 07:21:44 +0200
> khal...@piap.pl (Krzysztof Hałasa) escreveu:
> 
> > Andrey Utkin <andrey_ut...@fastmail.com> writes:
> > 
> > > Lockup happens only on 6010. In provided log you can see that 6110
> > > passes just fine right before 6010. Also if 6010 PCI ID is removed from
> > > solo6x10 driver's devices list, the freeze doesn't happen.  
> > 
> > Probably explains why I don't see lockups :-)
> > 
> > I will have a look.
> 
> Any news on this? Should the patch be applied or not? If not, are there
> any other patch to fix this regression?

Actual patch is

Subject: [PATCH v2] media: solo6x10: fix lockup by avoiding delayed register 
write
Message-Id: <20161022153436.12076-1-andrey.ut...@corp.bluecherry.net>
Date: Sat, 22 Oct 2016 16:34:36 +0100
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] media: solo6x10: fix lockup by avoiding delayed register write

[Andrey Utkin]

On Mon, Oct 24, 2016 at 06:57:25AM +0200, Krzysztof Hałasa wrote:
> Andrey Utkin <andrey.ut...@corp.bluecherry.net> writes:
> 
> > --- a/drivers/media/pci/solo6x10/solo6x10.h
> > +++ b/drivers/media/pci/solo6x10/solo6x10.h
> > @@ -284,7 +284,10 @@ static inline u32 solo_reg_read(struct solo_dev 
> > *solo_dev, int reg)
> >  static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
> >   u32 data)
> >  {
> > +   u16 val;
> > +
> > writel(data, solo_dev->reg_base + reg);
> > +   pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
> >  }
> >  
> >  static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
> 
> This is ok for now. I hope I will find some to refine this, so not all
> register writes are done with the penalty - eventually.

I'm afraid it'd be hard if you don't have a hardware sample which hangs.
I could in theory provide you with SSH access to my dev machine, but
currently I'm in another country so managing this is hard, too.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] [media] mb86a20s: always initialize a return value

[Andrey Utkin]

On Sun, Oct 23, 2016 at 07:09:10PM +0200, Nicolas Iooss wrote:
> Hello,
> 
> I sent the following patch (available on
> https://patchwork.kernel.org/patch/9325035/) a few weeks ago and got no
> feedback even though the bug it fixes seems to still exist in
> linux-next. Did I do something wrong? Should I consider this patch to be
> rejected?

No, it's extremely unlikely that bug fixing patches get silently
rejected by being ignored. Most probably the time of your submission is
unfortunate, being at the time when submissions are not merged.
I am in same situation with a couple of patches. I asked Mauro yesterday
on IRC and he replied:

I don't handle submissions during 3 weeks, during the merge window
one week before, and the two weeks after that
except when it is a bug that would affect the merge window
(end  of quote)

So unless you make it clear about which "release branches" are affected,
your submission is to be delayed - possibly up to 6 weeks or so.

I was suggested to add tags Fixes: buggy-commit-id and
Cc: sta...@vger.kernel.org
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: em28xx WinTV dualHD in Raspbian

[Andrey Utkin]

On Sat, Oct 22, 2016 at 07:36:13PM +0200, ps0...@yahoo.de wrote:
> Hopefully some driver expert can get the second tuner working, that would be
> awesome

What's the problem? I don't see it described in this discussion thread.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] media: solo6x10: fix lockup by avoiding delayed register write

[Andrey Utkin]

This fixes a lockup at device probing which happens on some solo6010
hardware samples. This is a regression introduced by commit e1ceb25a1569
("[media] SOLO6x10: remove unneeded register locking and barriers")

The observed lockup happens in solo_set_motion_threshold() called from
solo_motion_config().

This extra "flushing" is not fundamentally needed for every write, but
apparently the code in driver assumes such behaviour at last in some
places.

Actual fix was proposed by Hans Verkuil.

Fixes: e1ceb25a1569 ("[media] SOLO6x10: remove unneeded register locking and 
barriers")
Cc: sta...@vger.kernel.org
Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
This is a second submission, the first one was
"[PATCH] [media] solo6x10: avoid delayed register write" from 22 Sep 2016,
with same content.

Dear maintainers - please take this at last into v4.9 if possible.

Changes since v1:
 - changed subject to show that this fixes a lockup
 - added Cc: stable
 - added Fixes: tag

 drivers/media/pci/solo6x10/solo6x10.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/pci/solo6x10/solo6x10.h 
b/drivers/media/pci/solo6x10/solo6x10.h
index 5bd4987..3f8da5e 100644
--- a/drivers/media/pci/solo6x10/solo6x10.h
+++ b/drivers/media/pci/solo6x10/solo6x10.h
@@ -284,7 +284,10 @@ static inline u32 solo_reg_read(struct solo_dev *solo_dev, 
int reg)
 static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
  u32 data)
 {
+   u16 val;
+
writel(data, solo_dev->reg_base + reg);
+   pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
 }
 
 static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] solo6x10: avoid delayed register write

[Andrey Utkin]

On Thu, Sep 22, 2016 at 03:03:31AM +0300, Andrey Utkin wrote:
> This fixes a lockup at device probing which happens on some solo6010
> hardware samples. This is a regression introduced by commit e1ceb25a1569
> ("[media] SOLO6x10: remove unneeded register locking and barriers")
> 
> The observed lockup happens in solo_set_motion_threshold() called from
> solo_motion_config().
> 
> This extra "flushing" is not fundamentally needed for every write, but
> apparently the code in driver assumes such behaviour at last in some
> places.
> 
> Actual fix was proposed by Hans Verkuil.
> 
> Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
> ---
>  drivers/media/pci/solo6x10/solo6x10.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/pci/solo6x10/solo6x10.h 
> b/drivers/media/pci/solo6x10/solo6x10.h
> index 5bd4987..3f8da5e 100644
> --- a/drivers/media/pci/solo6x10/solo6x10.h
> +++ b/drivers/media/pci/solo6x10/solo6x10.h
> @@ -284,7 +284,10 @@ static inline u32 solo_reg_read(struct solo_dev 
> *solo_dev, int reg)
>  static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
> u32 data)
>  {
> + u16 val;
> +
>   writel(data, solo_dev->reg_base + reg);
> + pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
>  }
>  
>  static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
> -- 
> 2.9.2
> 

Mauro, Hans,
Please pick this up. This has been around for a month, I expected it
would get to v4.9-rc1 easily.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 54/57] [media] platform: don't break long lines

[Andrey Utkin]

On Mon, Oct 17, 2016 at 09:44:19PM +0300, Laurent Pinchart wrote:
> Hi Andrey,
> 
> On Monday 17 Oct 2016 20:39:45 Andrey Utkin wrote:
> > Maybe the remaining manual work may be outsourced to seekers of janitor
> > tasks?
> 
> I'm fine with that, but we should rework the original patches, not merge 
> fixes 
> on top of them.

I haven't meant that :) What I had in mind was a singular patchset with
combination of automated and manual work, just as you say, I just didn't
state it clearly.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] tw5864: crop picture width to 704

[Andrey Utkin]

On Thu, Sep 22, 2016 at 03:04:20AM +0300, Andrey Utkin wrote:
> Previously, width of 720 was used, but it gives 16-pixel wide black bar
> at right side of encoded picture.
> 
> Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
> ---
>  drivers/media/pci/tw5864/tw5864-reg.h   |  8 
>  drivers/media/pci/tw5864/tw5864-video.c | 13 +++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/tw5864/tw5864-reg.h 
> b/drivers/media/pci/tw5864/tw5864-reg.h
> index 92a1b07..30ac142 100644
> --- a/drivers/media/pci/tw5864/tw5864-reg.h
> +++ b/drivers/media/pci/tw5864/tw5864-reg.h
> @@ -1879,6 +1879,14 @@
>  #define TW5864_INDIR_IN_PIC_HEIGHT(channel) (0x201 + 4 * channel)
>  #define TW5864_INDIR_OUT_PIC_WIDTH(channel) (0x202 + 4 * channel)
>  #define TW5864_INDIR_OUT_PIC_HEIGHT(channel) (0x203 + 4 * channel)
> +
> +/* Some registers skipped */
> +
> +#define TW5864_INDIR_CROP_ETC 0x260
> +/* Define controls in register TW5864_INDIR_CROP_ETC */
> +/* Enable cropping from 720 to 704 */
> +#define TW5864_INDIR_CROP_ETC_CROP_EN 0x4
> +
>  /*
>   * Interrupt status register from the front-end. Write "1" to each bit to 
> clear
>   * the interrupt
> diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
> b/drivers/media/pci/tw5864/tw5864-video.c
> index ff94e6c..3c8c302 100644
> --- a/drivers/media/pci/tw5864/tw5864-video.c
> +++ b/drivers/media/pci/tw5864/tw5864-video.c
> @@ -590,6 +590,15 @@ static int tw5864_enable_input(struct tw5864_input 
> *input)
>   tw_indir_writeb(TW5864_INDIR_OUT_PIC_WIDTH(nr), input->width / 4);
>   tw_indir_writeb(TW5864_INDIR_OUT_PIC_HEIGHT(nr), input->height / 4);
>  
> + /*
> +  * Crop width from 720 to 704.
> +  * Above register settings need value 720 involved.
> +  */
> + input->width = 704;
> + tw_indir_writeb(TW5864_INDIR_CROP_ETC,
> + tw_indir_readb(TW5864_INDIR_CROP_ETC) |
> + TW5864_INDIR_CROP_ETC_CROP_EN);
> +
>   tw_writel(TW5864_DSP_PIC_MAX_MB,
> ((input->width / 16) << 8) | (input->height / 16));
>  
> @@ -792,7 +801,7 @@ static int tw5864_fmt_vid_cap(struct file *file, void 
> *priv,
>  {
>   struct tw5864_input *input = video_drvdata(file);
>  
> - f->fmt.pix.width = 720;
> + f->fmt.pix.width = 704;
>   switch (input->std) {
>   default:
>   WARN_ON_ONCE(1);
> @@ -998,7 +1007,7 @@ static int tw5864_enum_framesizes(struct file *file, 
> void *priv,
>   return -EINVAL;
>  
>   fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> - fsize->discrete.width = 720;
> + fsize->discrete.width = 704;
>   fsize->discrete.height = input->std == STD_NTSC ? 480 : 576;
>  
>   return 0;
> -- 
> 2.9.2
> 

Mauro, Hans,
Please pick this up. This has been around for a month, I expected it
would get to v4.9-rc1 easily.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 54/57] [media] platform: don't break long lines

[Andrey Utkin]

On Mon, Oct 17, 2016 at 04:45:06PM +0300, Laurent Pinchart wrote:
> If you really want to perform such a change, let's not make lines 
> unnecessarily long either. You should add a line break after the first and 
> second argument:
> 
>   dprintk(ctx->dev,
>   "%s data will not fit into plane(%lu < %lu)\n",
>   __func__, vb2_plane_size(vb, 0),
>   (long)q_data->sizeimage);
> 
> And everything will fit in 80 columns.

Same happens in other places, e.g. the hunk for
cx8802_unregister_driver() in another patch in this series, and not just
one time (looked just patches where I was a direct recipient).
There is a printing function call with previously split string literal,
followed by several arguments. This unnecessarily long function call is
now a single line.
Maybe the remaining manual work may be outsourced to seekers of janitor
tasks?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1] media: saa7146: Fix for "[BUG] process stuck when closing saa7146 [dvb_ttpci]"

[Andrey Utkin]

Release queued DMA buffers when ending streaming, so that
videobuf_waiton() doesn't block forever.

As reported, this fixes avoids occasional lockup of process reading from
video device, which manifests in such log:

INFO: task ffmpeg:9864 blocked for more than 120 seconds.
  Tainted: P   O4.6.7 #3
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
ffmpeg  D 880177cc7b00 0  9864  1 0x
 880177cc7b00 0202 0202 8180b4c0
 88019d79e4c0 81064050 880177cc7ae0 880177cc8000
 880177cc7b18 8801fd41d648 8802307acca0 8802307acc70
Call Trace:
 [] ? preempt_count_add+0x89/0xab
 [] schedule+0x86/0x9e
 [] ? schedule+0x86/0x9e
 [] videobuf_waiton+0x131/0x15e [videobuf_core]
 [] ? wait_woken+0x6d/0x6d
 [] saa7146_dma_free+0x39/0x5b [saa7146_vv]
 [] buffer_release+0x2a/0x3e [saa7146_vv]
 [] videobuf_vm_close+0xd8/0x103 [videobuf_dma_sg]
 [] remove_vma+0x25/0x4d
 [] exit_mmap+0xce/0xf7
 [] mmput+0x4e/0xe2
 [] do_exit+0x372/0x920
 [] do_group_exit+0x3c/0x98
 [] get_signal+0x4e8/0x56e
 [] ? task_dead_fair+0xd/0xf
 [] do_signal+0x23/0x521
 [] ? _raw_spin_unlock_irqrestore+0x13/0x25
 [] ? hrtimer_try_to_cancel+0xd7/0x104
 [] ? ktime_get+0x4c/0xa1
 [] ? update_rmtp+0x46/0x5b
 [] ? hrtimer_nanosleep+0xe4/0x10e
 [] ? hrtimer_init+0xeb/0xeb
 [] exit_to_usermode_loop+0x4f/0x93
 [] syscall_return_slowpath+0x3b/0x46
 [] entry_SYSCALL_64_fastpath+0x8d/0x8f

Reported-by: Philipp Matthias Hahn <pmhahn+vi...@pmhahn.de>
Tested-by: Philipp Matthias Hahn <pmhahn+vi...@pmhahn.de>
Signed-off-by: Andrey Utkin <andrey_ut...@fastmail.com>
---

Dear maintainers,
Please check whether the used buffer status (DONE) is correct for this
situation. I am in doubt, shouldn't it be VIDEOBUF_ERROR? However, ERROR
wasn't tested by Philipp and I guess it may indicate some undesired failure
status to application.

[PATCH v1] prefix is used to distinguish it from old letter with subj
"[PATCH] ..." which was a quick reply to Philipp's request and wasn't signed
off and otherwise properly formatted for submission.

 drivers/media/common/saa7146/saa7146_video.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/common/saa7146/saa7146_video.c 
b/drivers/media/common/saa7146/saa7146_video.c
index ea2f3bf..e034bcf 100644
--- a/drivers/media/common/saa7146/saa7146_video.c
+++ b/drivers/media/common/saa7146/saa7146_video.c
@@ -390,6 +390,7 @@ static int video_end(struct saa7146_fh *fh, struct file 
*file)
 {
struct saa7146_dev *dev = fh->dev;
struct saa7146_vv *vv = dev->vv_data;
+   struct saa7146_dmaqueue *q = >video_dmaq;
struct saa7146_format *fmt = NULL;
unsigned long flags;
unsigned int resource;
@@ -428,6 +429,9 @@ static int video_end(struct saa7146_fh *fh, struct file 
*file)
/* shut down all used video dma transfers */
saa7146_write(dev, MC1, dmas);
 
+   if (q->curr)
+   saa7146_buffer_finish(dev, q, VIDEOBUF_DONE);
+
spin_unlock_irqrestore(>slock, flags);
 
vv->video_fh = NULL;
-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] : Removing warnings caught by checkpatch.pl

[Andrey Utkin]

On Sun, Oct 02, 2016 at 02:30:45AM +0530, Harman Kalra wrote:
>  static int iss_video_queue_setup(struct vb2_queue *vq,
> -  unsigned int *count, unsigned int *num_planes,
> -  unsigned int sizes[], struct device 
> *alloc_devs[])
> + unsigned int *count, unsigned int *num_planes,
> + unsigned int sizes[], struct device *alloc_devs[])

2 spaces + 3 tabs -> 2 spaces + 2 tabs? Am I seeing this correctly?
Both ways are against CodingStyle.

> - /* Try the get selection operation first and fallback to get format if 
> not
> -  * implemented.
> + /* Try the get selection operation first and
> +  * fallback to get format if not implemented.
>*/

This comment style is discouraged so this is at lease not perfect.
Doesn't checkpatch complain with this change? If it doesn't, could you
please also check with --strict, as long as you're working on style.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Andrey Utkin]

On Tue, Sep 27, 2016 at 01:33:49PM +0200, Krzysztof Hałasa wrote:
> Thanks. I can see you have quite a set of video devices there.
> I will see what I can do with this.

Yeah, I have got also 4-chip tw5864 board here :)
Bluecherry decided to switch to it because they are available for retail
purchase, unlike solo* which must be ordered in large batch. It was huge
reverse-engineering effort to make it work, though, and there are still
issues with H.264 encoding functionality, and audio functionality is not
done yet.

> BTW does the lookup occur on SOLO6010, 6110, or both?

Lockup happens only on 6010. In provided log you can see that 6110
passes just fine right before 6010. Also if 6010 PCI ID is removed from
solo6x10 driver's devices list, the freeze doesn't happen.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Andrey Utkin]

On Tue, Sep 27, 2016 at 07:27:53AM +0200, Krzysztof Hałasa wrote:
> Andrey Utkin <andrey_ut...@fastmail.com> writes:
> 
> >> Does (only) adding the
> >> 
> >>pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
> >> 
> >> in solo_reg_write() help?
> >
> > Yes.
> > I have posted a patch with this change few days ago, I thought you have
> > noticed it.
> 
> Well, I think you haven't sent me a copy. Anyway, it would be great to
> determine where exactly writes need a flush. Adding it everywhere is a
> bit suboptimal, one would think.

Oh, I'm terribly sorry, I really meant to send you a copy.
Actual posting is:
lkml.kernel.org/r/20160922000331.4193-1-andrey.ut...@corp.bluecherry.net

> Can you share some details about the machine you are experiencing the
> problems on? CPU, chipset? I'd try to see if I can recreate the problem.

See solo.txt.gz attached.

> Alternatively, you could investigate yourself - at first you could put
> pci_read_config_word() at the end of subroutines (including return
> statements) using solo_reg_write(). And in that solo_p2m_dma_desc(),
> before wait_for_completion_timeout(). Then eliminate them using some
> sort of binary search to see which ones are required.

Sorry, but I've got no time for this long-lasting debug session right
now, and except for this issue, users enjoy their mainline kernel
driver. So I'd just fix that in mainline kernels as quickly as possible.
Now I'm even considering submitting that to longterm 4.4 branch.


solo.txt.gz
Description: Binary data

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Andrey Utkin]

On Mon, Sep 26, 2016 at 07:38:05AM +0200, Krzysztof Hałasa wrote:
> Andrey Utkin <andrey_ut...@fastmail.com> writes:
> 
> > On Thu, Sep 22, 2016 at 10:51:37AM +0200, Krzysztof Hałasa wrote:
> >> I wonder if the following fixes the problem (completely untested).
> >
> > I have given this a run, and it still hangs.
> 
> Does (only) adding the
> 
>   pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
> 
> in solo_reg_write() help?

Yes.
I have posted a patch with this change few days ago, I thought you have
noticed it.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: TW2866 i2c driver and solo6x10

[Andrey Utkin]

So is the actual machine x86 or ARM?
Do the tw28xx chips behind the bus (i2c as you said) show up in any way
at all? Is something like i2c controller available? Or it's ARM and we
should tell kernel how to "find" the i2c line by feeding correct
devicetree to it?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Andrey Utkin]

On Thu, Sep 22, 2016 at 10:51:37AM +0200, Krzysztof Hałasa wrote:
> I wonder if the following fixes the problem (completely untested).

I have given this a run, and it still hangs.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] tw5864: crop picture width to 704

[Andrey Utkin]

Previously, width of 720 was used, but it gives 16-pixel wide black bar
at right side of encoded picture.

Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
 drivers/media/pci/tw5864/tw5864-reg.h   |  8 
 drivers/media/pci/tw5864/tw5864-video.c | 13 +++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/tw5864/tw5864-reg.h 
b/drivers/media/pci/tw5864/tw5864-reg.h
index 92a1b07..30ac142 100644
--- a/drivers/media/pci/tw5864/tw5864-reg.h
+++ b/drivers/media/pci/tw5864/tw5864-reg.h
@@ -1879,6 +1879,14 @@
 #define TW5864_INDIR_IN_PIC_HEIGHT(channel) (0x201 + 4 * channel)
 #define TW5864_INDIR_OUT_PIC_WIDTH(channel) (0x202 + 4 * channel)
 #define TW5864_INDIR_OUT_PIC_HEIGHT(channel) (0x203 + 4 * channel)
+
+/* Some registers skipped */
+
+#define TW5864_INDIR_CROP_ETC 0x260
+/* Define controls in register TW5864_INDIR_CROP_ETC */
+/* Enable cropping from 720 to 704 */
+#define TW5864_INDIR_CROP_ETC_CROP_EN 0x4
+
 /*
  * Interrupt status register from the front-end. Write "1" to each bit to clear
  * the interrupt
diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
b/drivers/media/pci/tw5864/tw5864-video.c
index ff94e6c..3c8c302 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -590,6 +590,15 @@ static int tw5864_enable_input(struct tw5864_input *input)
tw_indir_writeb(TW5864_INDIR_OUT_PIC_WIDTH(nr), input->width / 4);
tw_indir_writeb(TW5864_INDIR_OUT_PIC_HEIGHT(nr), input->height / 4);
 
+   /*
+* Crop width from 720 to 704.
+* Above register settings need value 720 involved.
+*/
+   input->width = 704;
+   tw_indir_writeb(TW5864_INDIR_CROP_ETC,
+   tw_indir_readb(TW5864_INDIR_CROP_ETC) |
+   TW5864_INDIR_CROP_ETC_CROP_EN);
+
tw_writel(TW5864_DSP_PIC_MAX_MB,
  ((input->width / 16) << 8) | (input->height / 16));
 
@@ -792,7 +801,7 @@ static int tw5864_fmt_vid_cap(struct file *file, void *priv,
 {
struct tw5864_input *input = video_drvdata(file);
 
-   f->fmt.pix.width = 720;
+   f->fmt.pix.width = 704;
switch (input->std) {
default:
WARN_ON_ONCE(1);
@@ -998,7 +1007,7 @@ static int tw5864_enum_framesizes(struct file *file, void 
*priv,
return -EINVAL;
 
fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
-   fsize->discrete.width = 720;
+   fsize->discrete.width = 704;
fsize->discrete.height = input->std == STD_NTSC ? 480 : 576;
 
return 0;
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] solo6x10: avoid delayed register write

[Andrey Utkin]

This fixes a lockup at device probing which happens on some solo6010
hardware samples. This is a regression introduced by commit e1ceb25a1569
("[media] SOLO6x10: remove unneeded register locking and barriers")

The observed lockup happens in solo_set_motion_threshold() called from
solo_motion_config().

This extra "flushing" is not fundamentally needed for every write, but
apparently the code in driver assumes such behaviour at last in some
places.

Actual fix was proposed by Hans Verkuil.

Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
 drivers/media/pci/solo6x10/solo6x10.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/pci/solo6x10/solo6x10.h 
b/drivers/media/pci/solo6x10/solo6x10.h
index 5bd4987..3f8da5e 100644
--- a/drivers/media/pci/solo6x10/solo6x10.h
+++ b/drivers/media/pci/solo6x10/solo6x10.h
@@ -284,7 +284,10 @@ static inline u32 solo_reg_read(struct solo_dev *solo_dev, 
int reg)
 static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
  u32 data)
 {
+   u16 val;
+
writel(data, solo_dev->reg_base + reg);
+   pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
 }
 
 static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Andrey Utkin]

On Wed, Sep 21, 2016 at 03:16:57PM +0200, Krzysztof Hałasa wrote:
> Hans Verkuil  writes:
> 
> > That was probably the reason for the pci_read_config_word in the reg_write
> > code. Try putting that back (and just that).
> 
> Yes. I guess a single pci_read_config_word() would suffice.
> 
> Though it would obviously be much better to identify the place in the
> driver which needs to have the write buffers flushed, and add a read()
> just there.
> 
> The interrupt handler maybe (e.g. just before the return IRQ_HANDLED)?
> 
> OTOH this may be some sort of timing problem, I mean the faster code may
> put too much stress on the SOLO chip.
> 
> Doesn't happen here so I can't test the cure.

It happens in solo_disp_init at uploading default motion thresholds
array.

I've got a prints trace with solo6010-fix-lockup branch
https://github.com/bluecherrydvr/linux/tree/solo6010-fix-lockup/drivers/media/pci/solo6x10
the trace itself in jpg:
https://decent.im:5281/upload/3793f393-e285-4514-83dd-bf08d1c8b4a2/e7ad898b-515b-4522-86a9-553daaeb0860.jpg

Indeed, targeted fixing would be more reasonable than making register
r/w routines follow blocking fashion. But the driver is already complete
and was known to be working, and I seems all places in code assume the
blocking fashion of reg r/w, and changing that assumption may lead to
covert bugs anywhere else, not just at probing, which may be hard to
nail down.

For now, I'll try setting pci_read_config_word() back instead of full
revert. Does it need to be just in reg_write? No need for it in
reg_read, right?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

tw5864 - call to hardware owners

[Andrey Utkin]

Hi all,

I would love to hear from anybody who owns any sample of PCIE board with
TW5864 chip.

It is possible to buy from here
http://www.provideo.com.tw/Products.htm?link=web/DVR%20Card_Hardward.htm
I guess there are more companies selling boards with it.

So there is a driver, "tw5864", submitted and accepted upstream,
currently in linux-next, I guess it will get into Linux v4.9.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/media/pci/tw5864

Then there is a pile of sources from vendor, obfuscated and bloated, not
very human-readable, but that is what our painful development started
with:
http://lizard.bluecherry.net/~autkin/tw5864/TW-3XX_Linux.rar

TW5864 datasheet from manufacturer:
http://lizard.bluecherry.net/~autkin/tw5864/tw5864b1-ds.pdf

Recently a developer from another company contacted us, reporting that
our driver doesn't work well on samples they had, and sharing quite
different driver sources from their vendor, which work fine. Those
sources were also obfuscated, but much less, and they have successfully
deobfuscated by them. Link to the code:
https://github.com/bluecherrydvr/linux/tree/master/drivers/media/pci/Isil5864

This second driver is interesting because on some samples it really
works well, despite the upstreamed driver gives worse picture. I cannot
work on that productively because my hardware sample is not affected.
That's why some communication with other owners would be useful.

Oh and of course Intersil (current owner of Techwell labs) technical
support is useless, just stating that development team was dismissed
several years ago.

(By the way, if anybody is aware of different PCI Express boards with
both analog input decoders and video compression encoders, except
solo6x10, which are easy to buy and to run on Linux, please let us
know).
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: TW2866 i2c driver and solo6x10

[Andrey Utkin]

I'm going to have quite constrained time for participation in this
driver development, but still I think this is perspective project which
is in line with trend of exposing internal details of complex media
hardware for configuration by V4L2 framework. Also tw286x are used in
both tw5864 and solo6x10 boards, and in both cases it could be
controlled better from userspace.
I think first thing to do is expose tw286x chips as i2c- (or more
precisely SMBus-) controllable devices. I have accomplished that in some
way for tw5864, and hopefully I'll manage that for solo6x10.
But beyond that, I currently don't know.
A senior mentor would be very appreciated :)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Potential fix for "[BUG] process stuck when closing saa7146 [dvb_ttpci]"

[Andrey Utkin]

Hi Philipp,
Please try this patch. It is purely speculative as I don't have the hardware,
but I hope my approach is right.

---
 drivers/media/common/saa7146/saa7146_video.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/common/saa7146/saa7146_video.c 
b/drivers/media/common/saa7146/saa7146_video.c
index ea2f3bf..93c64f0 100644
--- a/drivers/media/common/saa7146/saa7146_video.c
+++ b/drivers/media/common/saa7146/saa7146_video.c
@@ -390,6 +390,7 @@ static int video_end(struct saa7146_fh *fh, struct file 
*file)
 {
struct saa7146_dev *dev = fh->dev;
struct saa7146_vv *vv = dev->vv_data;
+   struct saa7146_dmaqueue *q = >video_dmaq;
struct saa7146_format *fmt = NULL;
unsigned long flags;
unsigned int resource;
@@ -428,6 +429,9 @@ static int video_end(struct saa7146_fh *fh, struct file 
*file)
/* shut down all used video dma transfers */
saa7146_write(dev, MC1, dmas);
 
+   if(q->curr)
+   saa7146_buffer_finish(dev, q, VIDEOBUF_DONE);
+
spin_unlock_irqrestore(>slock, flags);
 
vv->video_fh = NULL;
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Andrey Utkin]

On Thu, Sep 15, 2016 at 03:15:53PM +0200, Hans Verkuil wrote:
> It could be related to the fact that a PCI write may be delayed unless
> it is followed by a read (see also the comments in 
> drivers/media/pci/ivtv/ivtv-driver.h).

Thanks for explanation!

> That was probably the reason for the pci_read_config_word in the reg_write
> code. Try putting that back (and just that).

In this case reg_write becomes not atomic, thus spinlock would be
required again here, right?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Andrey Utkin]

Hi Krzysztof,

Me and one more solo6010 board user experience machine lockup when
solo6x10 module is loaded on kernel series starting with 4.3 (despite
solo6110 board probes just fine on all kernels). That is, 3.16, 3.18,
4.1 and 4.2 are tested and fine, and 4.3, 4.4, and others up to current
linux-next are bad.
So regression slipped in between 4.2 and 4.3. The diff between
stable/linux-4.2.y and ...-4.3.y (which were tested) is not large, and
my suspect fell on ripoff of register writing procedures complexity,
which was introduced in e1ceb25a (see below). Reversion of that fixes
lockup.  However, if, on top of reversion of e1ceb25a, i drop barrier
stuff and pci_read_config... (see
https://github.com/bluecherrydvr/linux/commit/d59aaf3), leaving the
spinlock stuff, it locks up again.  This is a matter in which I'm not
quite qualified, so I have no idea what that code copes with and why
this workaround works for solo6010.  For now I think I'll tell the
customer to use kernel with e1ceb25a reverted, but for upstream fix, I'm
interested in more in-depth investigation. I'll be able to provide dmesg
logs a bit later.

The breaking commit is quoted below.

commit e1ceb25a1569ce5b61b9c496dd32d038ba8cb936
Author: Krzysztof Hałasa 
Date:   Mon Jun 8 10:42:24 2015 -0300

[media] SOLO6x10: remove unneeded register locking and barriers

readl() and writel() are atomic, we don't need the spin lock.
Also, flushing posted write buffer isn't required. Especially on read :-)

Signed-off-by: Krzysztof Ha?asa 
Signed-off-by: Hans Verkuil 
Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/pci/solo6x10/solo6x10-core.c 
b/drivers/media/pci/solo6x10/solo6x10-core.c
index 84627e6..9c948b1 100644
--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -483,7 +483,6 @@ static int solo_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *id)
 
solo_dev->type = id->driver_data;
solo_dev->pdev = pdev;
-   spin_lock_init(_dev->reg_io_lock);
ret = v4l2_device_register(>dev, _dev->v4l2_dev);
if (ret)
goto fail_probe;
diff --git a/drivers/media/pci/solo6x10/solo6x10.h 
b/drivers/media/pci/solo6x10/solo6x10.h
index 1ca54b0..27423d7 100644
--- a/drivers/media/pci/solo6x10/solo6x10.h
+++ b/drivers/media/pci/solo6x10/solo6x10.h
@@ -199,7 +199,6 @@ struct solo_dev {
int nr_ext;
u32 irq_mask;
u32 motion_mask;
-   spinlock_t  reg_io_lock;
struct v4l2_device  v4l2_dev;
 
/* tw28xx accounting */
@@ -281,36 +280,13 @@ struct solo_dev {
 
 static inline u32 solo_reg_read(struct solo_dev *solo_dev, int reg)
 {
-   unsigned long flags;
-   u32 ret;
-   u16 val;
-
-   spin_lock_irqsave(_dev->reg_io_lock, flags);
-
-   ret = readl(solo_dev->reg_base + reg);
-   rmb();
-   pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
-   rmb();
-
-   spin_unlock_irqrestore(_dev->reg_io_lock, flags);
-
-   return ret;
+   return readl(solo_dev->reg_base + reg);
 }
 
 static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
  u32 data)
 {
-   unsigned long flags;
-   u16 val;
-
-   spin_lock_irqsave(_dev->reg_io_lock, flags);
-
writel(data, solo_dev->reg_base + reg);
-   wmb();
-   pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
-   rmb();
-
-   spin_unlock_irqrestore(_dev->reg_io_lock, flags);
 }
 
 static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] [media] tw5864 constify some structures

[Andrey Utkin]

On Tue, Sep 13, 2016 at 02:02:36AM +0300, Andrey Utkin wrote:
> tw5864 is a recently submitted driver and it is currently present only
> in media tree.

Oops, have just fetched linux-next updates, tw5864 is already in next.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] [media] tw5864: constify vb2_ops structure

[Andrey Utkin]

Inspired by "[media] pci: constify vb2_ops structures" patch
from Julia Lawall (dated 9 Sep 2016).

Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
 drivers/media/pci/tw5864/tw5864-video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
b/drivers/media/pci/tw5864/tw5864-video.c
index 6c1685a..7401b64 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -465,7 +465,7 @@ static void tw5864_stop_streaming(struct vb2_queue *q)
spin_unlock_irqrestore(>slock, flags);
 }
 
-static struct vb2_ops tw5864_video_qops = {
+static const struct vb2_ops tw5864_video_qops = {
.queue_setup = tw5864_queue_setup,
.buf_queue = tw5864_buf_queue,
.start_streaming = tw5864_start_streaming,
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] [media] tw5864: constify struct video_device template

[Andrey Utkin]

tw5864_video_template is used for filling of actual video_device
structures. It is copied by value, and is not used for anything else.

Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
 drivers/media/pci/tw5864/tw5864-video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/tw5864/tw5864-video.c 
b/drivers/media/pci/tw5864/tw5864-video.c
index 7401b64..652a059 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -912,7 +912,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
 #endif
 };
 
-static struct video_device tw5864_video_template = {
+static const struct video_device tw5864_video_template = {
.name = "tw5864_video",
.fops = _fops,
.ioctl_ops = _ioctl_ops,
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] [media] tw5864 constify some structures

[Andrey Utkin]

tw5864 is a recently submitted driver and it is currently present only
in media tree.

Recent patches submitted by Julia Lawall urged me to make similar
changes in this driver.

Andrey Utkin (2):
  [media] tw5864: constify vb2_ops structure
  [media] tw5864: constify struct video_device template

 drivers/media/pci/tw5864/tw5864-video.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo error on second capture from USB device, leading to V4L2_BUF_FLAG_ERROR

[Andrey Utkin]

On Sat, Sep 10, 2016 at 01:21:10PM +0300, Oliver Collyer wrote:
> 
> >> I have written a patch for FFmpeg that deals with the problem for both
> >> devices so it’s not really an issue for me anymore, but I’m not sure
> >> if the patch will get accepted in their master git as it’s a little
> >> messy.
> > 
> > Please post this patch here!
> 
> Here you go, Andrey. This patch basically makes it throw away corrupted 
> buffers and then also the first 8 buffers after the last corrupted buffer.

Thanks a lot for sharing.

> It’s not sufficient just to throw away the corrupted buffers as I have 
> noticed that the first few legitimate buffers appear at slightly irregular 
> time intervals leading to FFmpeg spewing out a bunch of warnings for the 
> duration of the capture. In my tests around 3 buffers have to be ignored but 
> I’ve fixed it at 8 to be on the safe side. It’s a bit ugly though, to be 
> honest, I don’t know how the number of buffers that need to be ignored would 
> depend on the framerate, video size etc, but it works for my 1080i test.
> 
> With this patch, you get some warnings at the start, for both devices, as it 
> encounters (and recovers from) the corrupted buffers but after that the 
> captures work just fine.
> 
> 
> diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c
> old mode 100644
> new mode 100755
> index ddf331d..7b4a826
> --- a/libavdevice/v4l2.c
> +++ b/libavdevice/v4l2.c
> @@ -79,6 +79,7 @@ struct video_data {
>  
>  int buffers;
>  volatile int buffers_queued;
> +int buffers_ignore;
>  void **buf_start;
>  unsigned int *buf_len;
>  char *standard;
> @@ -519,7 +520,9 @@ static int mmap_read_frame(AVFormatContext *ctx, AVPacket 
> *pkt)
>  av_log(ctx, AV_LOG_WARNING,
> "Dequeued v4l2 buffer contains corrupted data (%d bytes).\n",
> buf.bytesused);
> -buf.bytesused = 0;
> +s->buffers_ignore = 8;
> +enqueue_buffer(s, );
> +return FFERROR_REDO;
>  } else
>  #endif
>  {
> @@ -529,14 +532,28 @@ static int mmap_read_frame(AVFormatContext *ctx, 
> AVPacket *pkt)
>  s->frame_size = buf.bytesused;
>  
>  if (s->frame_size > 0 && buf.bytesused != s->frame_size) {
> -av_log(ctx, AV_LOG_ERROR,
> +av_log(ctx, AV_LOG_WARNING,
> "Dequeued v4l2 buffer contains %d bytes, but %d were 
> expected. Flags: 0x%08X.\n",
> buf.bytesused, s->frame_size, buf.flags);
> +s->buffers_ignore = 8;
>  enqueue_buffer(s, );
> -return AVERROR_INVALIDDATA;
> +return FFERROR_REDO;
>  }
>  }

These two chunks look like legit resilience measure, and maybe could be
even added to upstream ffmpeg, maybe for non-default mode.

>  
> +
> +/* if we just encounted some corrupted buffers then we ignore the next 
> few
> + * legitimate buffers because they can arrive at irregular intervals, 
> causing
> + * the timestamps of the input and output streams to be out-of-sync and 
> FFmpeg
> + * to continually emit warnings. */
> +if (s->buffers_ignore) {
> +av_log(ctx, AV_LOG_WARNING,
> +   "Ignoring dequeued v4l2 buffer due to earlier corruption.\n");
> +s->buffers_ignore --;
> +enqueue_buffer(s, );
> +return FFERROR_REDO;
> +}

Not clear exactly happens here so that such workaround is needed...


Congratulations, you've ended up with a workaround which works for you,
for such a mysterious issue :)

I still don't know what exactly causes this error condition on original
layer (I suppose that's some "panic" in the peripheral device), but I
guess that due to rarity of this condition, V4L2 code developers (in
both kernel and ffmpeg) just haven't had an opportunity to debug such
situations and handled this error condition formally, without experience
of running into it, and knowledge why it happens and how it could be
handled in most resilient way. (Maybe this should NOT be handled in
resilient way in theory, but still works for your case.) So you had to
pave your own way here.

Maybe comments from senior V4L2 developers shed more lights on this.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo error on second capture from USB device, leading to V4L2_BUF_FLAG_ERROR

[Andrey Utkin]

On Sat, Sep 10, 2016 at 10:37:08AM +0300, Oliver Collyer wrote:
> Ok, so I’ve provided all these logs requested by Andrey in an earlier
> message but I’m unsubscribing from this list now.

Fine. You'll be able to post here, as well as receive replies for this
thread, without subscription.

> I have written a patch for FFmpeg that deals with the problem for both
> devices so it’s not really an issue for me anymore, but I’m not sure
> if the patch will get accepted in their master git as it’s a little
> messy.

Please post this patch here!
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] pci: constify vb2_ops structures

[Andrey Utkin]

On Fri, Sep 09, 2016 at 10:31:30PM +0800, Julia Lawall wrote:
> Will this soon reach linux-next?

No idea. Indeed it's simpler if you leave your patch as is, and then
later we patch this new driver separately.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] v4l2-ctl: Fix unneeded dot in "no hsync lock"

[Andrey Utkin]

From: Andrey Utkin <andrey_ut...@fastmail.com>

Signed-off-by: Andrey Utkin <andrey_ut...@fastmail.com>
---
 utils/v4l2-ctl/v4l2-ctl-io.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/v4l2-ctl/v4l2-ctl-io.cpp b/utils/v4l2-ctl/v4l2-ctl-io.cpp
index e778569..30798ea 100644
--- a/utils/v4l2-ctl/v4l2-ctl-io.cpp
+++ b/utils/v4l2-ctl/v4l2-ctl-io.cpp
@@ -53,7 +53,7 @@ static const flag_def in_status_def[] = {
{ V4L2_IN_ST_NO_COLOR,"no color" },
{ V4L2_IN_ST_HFLIP,   "hflip" },
{ V4L2_IN_ST_VFLIP,   "vflip" },
-   { V4L2_IN_ST_NO_H_LOCK,   "no hsync lock." },
+   { V4L2_IN_ST_NO_H_LOCK,   "no hsync lock" },
{ V4L2_IN_ST_COLOR_KILL,  "color kill" },
{ V4L2_IN_ST_NO_SYNC, "no sync lock" },
{ V4L2_IN_ST_NO_EQU,  "no equalizer lock" },
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] pci: constify vb2_ops structures

[Andrey Utkin]

On Fri, Sep 09, 2016 at 01:59:18AM +0200, Julia Lawall wrote:
> Check for vb2_ops structures that are only stored in the ops field of a
> vb2_queue structure.  That field is declared const, so vb2_ops structures
> that have this property can be declared as const also.

> Signed-off-by: Julia Lawall <julia.law...@lip6.fr>
> 
> ---
>  drivers/media/pci/cx23885/cx23885-417.c|2 +-
>  drivers/media/pci/cx23885/cx23885-dvb.c|2 +-
>  drivers/media/pci/cx23885/cx23885-video.c  |2 +-
>  drivers/media/pci/cx25821/cx25821-video.c  |2 +-
>  drivers/media/pci/cx88/cx88-blackbird.c|2 +-
>  drivers/media/pci/cx88/cx88-dvb.c  |2 +-
>  drivers/media/pci/cx88/cx88-video.c|2 +-
>  drivers/media/pci/netup_unidvb/netup_unidvb_core.c |2 +-
>  drivers/media/pci/saa7134/saa7134-empress.c|2 +-
>  drivers/media/pci/saa7134/saa7134-video.c  |2 +-
>  drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c |2 +-
>  drivers/media/pci/tw68/tw68-video.c|2 +-
>  drivers/media/pci/tw686x/tw686x-video.c|2 +-
>  13 files changed, 13 insertions(+), 13 deletions(-)

Applies and compiles cleanly on media tree.
But please regenerate this patch on git://linuxtv.org/media_tree.git -
it has a new driver by me, drivers/media/pci/tw5864 , which is affected.

Otherwise

Acked-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>

(however this time I'm writing from another email.)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] pci: constify snd_pcm_ops structures

[Andrey Utkin]

Thanks for looking into this.
I have tested that it compiles and passes checks (C=2) cleanly after 
this patch.

Acked-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>

While we're at it, what about constification of
*-core.c:static struct pci_driver *_pci_driver = {
*-video.c:static struct vb2_ops *_video_qops = {
*-video.c:static struct video_device *_video_template = {

in drivers/media/pci/? Also there are even more similar entries not
constified yet in drivers/media/, however I may be underestimating
complexity of making semantic rules for catching that all.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo error on second capture from USB device, leading to V4L2_BUF_FLAG_ERROR

[Andrey Utkin]

On Tue, Sep 06, 2016 at 01:51:51PM +0300, Oliver Collyer wrote:
> So today I installed Ubuntu 16.04 on another PC (this one a high spec machine 
> with a Rampage V Extreme motherboard) and I reproduced exactly the same 
> errors and trace.
> 
> Rebooting the same PC back into Windows 10 and using the same USB 3.0 port, I 
> had no problems capturing using FFmpeg via DirectShow. I could start and stop 
> the capture repeatedly without any warnings or errors appearing in FFmpeg 
> (built from the same source).
> 
> If the hardware is misbehaving, on both these capture devices, then DS must 
> be handling it better than V4L2. Or there is simply an obscure bug in V4L2 
> which only manifests itself with certain devices.
> 
> Would providing ssh access to the machine be of interest to anyone who wants 
> to debug this?

I am curious to tinker with this, just not sure about free time for it.
Please go through the following instruction, and then we'll see if ssh
is going to help to debug this.

Also I think it is worth to CC actual manufacturers. There are addresses
for technical support of both devices in public on maker websites.
Please CC them when replying with new logs, to let them catch up.

So, I am still not certain what confuses the device, i.e. where the
faulty usage pattern comes from: ffmpeg or driver. So I'd like you to
check the difference with various userspace applications which involve
streaming from device.

For each of your two devices, alone (not two at same time), do this:

For each command from this list:
"v4l2-compliance -s -d /dev/video0",
"ffmpeg -f v4l2 -i /dev/video0 -vcodec rawvideo -f null -y /dev/null",
""
(feel free to add more, maybe mplayer invocation or such)

dmesg -C
plug in the device
modprobe uvcvideo module
run the command twice or more in row
save uncut commands output (with command lines) to separate file
rmmod uvcvideo
unplug the device
save "dmesg" output to separate file


Done.

I guess this test makes sense, or am I missing something you've already
told us?

If you go making a script for this, make sure to notice if rmmod fails
for any reason, etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo error on second capture from USB device, leading to V4L2_BUF_FLAG_ERROR

[Andrey Utkin]

On Mon, Sep 05, 2016 at 10:43:49PM +0300, Oliver Collyer wrote:
> I do not have any knowledge of uvcvideo and the associated classes apart from 
> the studying I’ve done the past day or two, but it seems likely that error 
> -71 and the later setting of V4L2_BUF_FLAG_ERROR are linked. Also, the fact 
> it only happens in captures after the first one suggests something isn’t 
> being cleared down or released properly in uvcvideo/v4l2-core at the end of 
> the first capture.
> 
> Let me know what I need to do next to further narrow it down.

Have tried to reproduce this (with kernel 4.6.0 and fresh build of
ffmpeg) with uvcvideo-driven laptop webcam, and it doesn't happen to me.
Also -EPROTO in uvcvideo comes from low-level USB stuff, see
drivers/media/usb/uvc/uvc_status.c:127:

case -EPROTO:   /* Device is disconnected (reported by some
 * host controller). */

So it seems like hardware misbehaves. To further clairify situation, I
have such question: do the devices work in other operation systems on
the same machine?

Reviewing your original email mentioning that two different devices
reproduce same problem, which is apparently related to disconnection in
the middle of USB communication, I came to me that the connected device
may be underpowered. So,
 - try plugging your devices through reliable _active_ USB hub,
 - use the most reliable cables you can get,
 - plug into USB 3.0 port if available - it should provide more power
   than 1.0 and 2.0.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] [media] tw5864-core: remove double irq lock code

[Andrey Utkin]

Just received a notification from patchwork that my alternative patch on
this was rejected as not applicable.

Thanks to Hans Verkuil - he has applied a reversion of Mauro's wrong
patch and also applied my patch in his repo,
git://linuxtv.org/hverkuil/media_tree.git (branch for-v4.9c).

As I see the wrong patch is in git://linuxtv.org/media_tree.git master
branch, and is considered "merged", but I would love to see that wrong
patch (and reversion of it) dropped before it gets into linux-next (I
don't know whether media tree has a tradition to not reset branches).
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: uvcvideo error on second capture from USB device, leading to V4L2_BUF_FLAG_ERROR

[Andrey Utkin]

Hi!
Seems like weird error in V4L subsystem or in uvcvideo driver, in the
most standard usage scenario.
Please retry with kernel and FFmpeg as new as possible, best if compiled
from latest upstream sources.
For kernel please try release 4.7.2 or even linux-next
(git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git), for
FFmpeg please make a git clone from git://source.ffmpeg.org/ffmpeg.git
and there do "./configure && make" and run obtained "ffmpeg" binary.

Please CC me when you come back with your results.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] [media] tw5864-core: remove double irq lock code

[Andrey Utkin]

Hi Mauro,
Have you received my letter which is cited below?
I also tried to get in touch with you on #linuxtv.

On Thu, Aug 25, 2016 at 05:46:06PM +0300, Andrey Utkin wrote:
> For some reason (maybe "unlisted recipients"?), my reply didn't get
> through to maillists. Resending my reply.
> 
> On Wed, Aug 24, 2016, at 19:30, Mauro Carvalho Chehab wrote:
> > As warned by smatch:
> > drivers/media/pci/tw5864/tw5864-core.c:160 tw5864_h264_isr() error: 
> > double lock 'irqsave:flags'
> > drivers/media/pci/tw5864/tw5864-core.c:174 tw5864_h264_isr() error: 
> > double unlock 'irqsave:flags'
> > 
> > Remove the IRQ duplicated lock.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
> > ---
> >  drivers/media/pci/tw5864/tw5864-core.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/media/pci/tw5864/tw5864-core.c
> > b/drivers/media/pci/tw5864/tw5864-core.c
> > index 440cd7bb8d04..e3d884e963c0 100644
> > --- a/drivers/media/pci/tw5864/tw5864-core.c
> > +++ b/drivers/media/pci/tw5864/tw5864-core.c
> > @@ -157,12 +157,10 @@ static void tw5864_h264_isr(struct tw5864_dev *dev)
> >  
> > cur_frame = next_frame;
> >  
> > -   spin_lock_irqsave(>slock, flags);
> > input->frame_seqno++;
> > input->frame_gop_seqno++;
> > if (input->frame_gop_seqno >= input->gop)
> > input->frame_gop_seqno = 0;
> > -   spin_unlock_irqrestore(>slock, flags);
> > } else {
> > dev_err(>pci->dev,
> > "Skipped frame on input %d because all buffers busy\n",
> 
> 
> Thank you very much for catching this issue, but NACK on the patch.
> 
> These two lock operations are on different spinlocks. One for device,
> another
> one for input (a subordinate entity of device). What is superfluous here
> is
> second _irqsave. Also "flags" variable reuse is wrong. So what would be
> right,
> in my opinion, is the following (going to submit this patch):
> 
> diff --git a/drivers/media/pci/tw5864/tw5864-core.c
> b/drivers/media/pci/tw5864/tw5864-core.c
> index 440cd7b..1d43b96 100644
> --- a/drivers/media/pci/tw5864/tw5864-core.c
> +++ b/drivers/media/pci/tw5864/tw5864-core.c
> @@ -157,12 +157,12 @@ static void tw5864_h264_isr(struct tw5864_dev
> *dev)
>  
>   cur_frame = next_frame;
>  
> -   spin_lock_irqsave(>slock, flags);
> +   spin_lock(>slock);
>   input->frame_seqno++;
>   input->frame_gop_seqno++;
>   if (input->frame_gop_seqno >= input->gop)
>   input->frame_gop_seqno = 0;
> -   spin_unlock_irqrestore(>slock, flags);
> +   spin_unlock(>slock);
>   } else {
>   dev_err(>pci->dev,
>   "Skipped frame on input %d because all buffers
>   busy\n",
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] [media] tw5864-core: remove double irq lock code

[Andrey Utkin]

For some reason (maybe "unlisted recipients"?), my reply didn't get
through to maillists. Resending my reply.

On Wed, Aug 24, 2016, at 19:30, Mauro Carvalho Chehab wrote:
> As warned by smatch:
>   drivers/media/pci/tw5864/tw5864-core.c:160 tw5864_h264_isr() error: 
> double lock 'irqsave:flags'
>   drivers/media/pci/tw5864/tw5864-core.c:174 tw5864_h264_isr() error: 
> double unlock 'irqsave:flags'
> 
> Remove the IRQ duplicated lock.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/pci/tw5864/tw5864-core.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/pci/tw5864/tw5864-core.c
> b/drivers/media/pci/tw5864/tw5864-core.c
> index 440cd7bb8d04..e3d884e963c0 100644
> --- a/drivers/media/pci/tw5864/tw5864-core.c
> +++ b/drivers/media/pci/tw5864/tw5864-core.c
> @@ -157,12 +157,10 @@ static void tw5864_h264_isr(struct tw5864_dev *dev)
>  
>   cur_frame = next_frame;
>  
> -   spin_lock_irqsave(>slock, flags);
>   input->frame_seqno++;
>   input->frame_gop_seqno++;
>   if (input->frame_gop_seqno >= input->gop)
>   input->frame_gop_seqno = 0;
> -   spin_unlock_irqrestore(>slock, flags);
>   } else {
>   dev_err(>pci->dev,
>   "Skipped frame on input %d because all buffers busy\n",


Thank you very much for catching this issue, but NACK on the patch.

These two lock operations are on different spinlocks. One for device,
another
one for input (a subordinate entity of device). What is superfluous here
is
second _irqsave. Also "flags" variable reuse is wrong. So what would be
right,
in my opinion, is the following (going to submit this patch):

diff --git a/drivers/media/pci/tw5864/tw5864-core.c
b/drivers/media/pci/tw5864/tw5864-core.c
index 440cd7b..1d43b96 100644
--- a/drivers/media/pci/tw5864/tw5864-core.c
+++ b/drivers/media/pci/tw5864/tw5864-core.c
@@ -157,12 +157,12 @@ static void tw5864_h264_isr(struct tw5864_dev
*dev)
 
cur_frame = next_frame;
 
-   spin_lock_irqsave(>slock, flags);
+   spin_lock(>slock);
input->frame_seqno++;
input->frame_gop_seqno++;
if (input->frame_gop_seqno >= input->gop)
input->frame_gop_seqno = 0;
-   spin_unlock_irqrestore(>slock, flags);
+   spin_unlock(>slock);
} else {
dev_err(>pci->dev,
"Skipped frame on input %d because all buffers
busy\n",
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [media] tw5864-core: remove excessive irqsave

[Andrey Utkin]

As warned by smatch:
drivers/media/pci/tw5864/tw5864-core.c:160 tw5864_h264_isr() error: 
double lock 'irqsave:flags'
drivers/media/pci/tw5864/tw5864-core.c:174 tw5864_h264_isr() error: 
double unlock 'irqsave:flags'

Two different spinlocks are obtained, so having two calls is correct,
but second irqsave is superfluous, and using same "flags" variable is
just wrong.

Reported-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
 drivers/media/pci/tw5864/tw5864-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/tw5864/tw5864-core.c 
b/drivers/media/pci/tw5864/tw5864-core.c
index 440cd7b..1d43b96 100644
--- a/drivers/media/pci/tw5864/tw5864-core.c
+++ b/drivers/media/pci/tw5864/tw5864-core.c
@@ -157,12 +157,12 @@ static void tw5864_h264_isr(struct tw5864_dev *dev)
 
cur_frame = next_frame;
 
-   spin_lock_irqsave(>slock, flags);
+   spin_lock(>slock);
input->frame_seqno++;
input->frame_gop_seqno++;
if (input->frame_gop_seqno >= input->gop)
input->frame_gop_seqno = 0;
-   spin_unlock_irqrestore(>slock, flags);
+   spin_unlock(>slock);
} else {
dev_err(>pci->dev,
"Skipped frame on input %d because all buffers busy\n",
-- 
2.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] tw5864: remove two unused vars

[Andrey Utkin]

On Wed, Aug 24, 2016 at 01:30:40PM -0300, Mauro Carvalho Chehab wrote:
> Remove those two vars that aren't used, as reported by smatch:

Acked-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>

Sorry for missing this.
Thanks a lot.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux support for current StarTech analog video capture device (SAA711xx)

[Andrey Utkin]

On Fri, Aug 19, 2016 at 01:52:23PM +, Steve Preston wrote:
> Hello everyone,
> 
> Sorry for the slow response.  Several people have offered to help and this is 
> greatly appreciated. I'm not the occultation person actually doing the dev 
> work on our Linux project.

You should CC your Linux people in this thread so that you don't need to
relay messages back and forth manually.

> I have forwarded the suggestions from this thread on the list and we let you 
> know as we make progress.
> 
> We have two models of the StarTech in use: SVID2USB2 and SVID2USB23.  The 
> "23" version is the only version currently listed on StarTech's website.  It 
> is available via Amazon in the USA but I'm not sure about other countries.

OK, make sure to try what Hans has suggested, and to describe in details
what you have tried so far and what is your state - i.e. what happens,
what is logged to dmesg, etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Linux support for current StarTech analog video capture device (SAA711xx)

[Andrey Utkin]

On Tue, Aug 16, 2016 at 09:29:49PM +, Steve Preston wrote:
> I realize this is a long shot but I was directed to this mailing list as one 
> possibility  . 
>  
> I work with a group of amateur astronomers who use analog video cameras to 
> record occultations ( www.occulations.org ).  Several observers have been 
> using the StarTech SVID2USB2 class of analog capture devices (USB dongle) 
> under Windows.  The StarTech devices are one of the few such devices which 
> are readily available today. These StarTech devices seemed to be based on the 
> empia 28xx + SAA71xx chipset devices which have some support in the linux 
> kernel.  Unfortunately, we are having trouble with the StarTech devices in 
> Linux.  Does anyone on this list know of anyone in the linxtv.org (or 
> related) community that might be willing to help us modify a current driver 
> to enable the StarTech device(s)?  Or, do you know of anyone who currently 
> works with analog video capture hardware in linux who might be willing to 
> provide other ideas?

Please try what Hans has proposed. Then come back to here and post what
is your current state.
Please keep me in CC when you post. I would like to help.
Where can one buy online the exact device you're talking about?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: TW2866 i2c driver and solo6x10

[Andrey Utkin]

On Wed, Aug 03, 2016 at 03:26:39PM -0500, Marty Plummer wrote:
> An understanable sentement, but its not just about me (though I actually own 
> two
> of the same dvr's). There is a large number, if reports are to be believed, of
> more or less identical dvr systems with the same security holes and hard-coded
> credentials, with no vendor firmware updates. In addition, I am also in 
> possesion
> of the chip vendor sdk with full source code for the kernel and modules and a
> host of pertenant datasheets. My end goal is something along the line of 
> openwrt
> or lede for these systems in the interest of the community at large.
> 
> I'm not averse to the concept of hard work, and while I'm very experienced 
> when
> it comes to kernel hacking I think I can manage well enough with relevant 
> source
> and examples, of which there is a large amount.
> 
> One concern I have is dealing with devicetree, but I think I can manage.
> 

That's intriguing. I'm interested to know what this model is and where
it can be bought, and also to see all the sdk. BTW have you got sdk from
manufacturer after asking for it, or otherwise? Are manufacturers keen
to answer technical questions?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: TW2866 i2c driver and solo6x10

[Andrey Utkin]

On Wed, Jul 27, 2016 at 11:51:22PM -0500, Marty Plummer wrote:
> I have one of those rebranded chinese security dvrs, the ones with all the 
> gaping
> security holes. I'd like to fix that up and setup a good rtsp server on it, 
> but
> first comes low-level stuff, drivers and such. I've been squinting at the pcb 
> and
> ID'ing chips for a bit now, and I've figured most of them out. Looks like the 
> actual
> video processing is done on 4 tw2866 chips, though the kernel module has 
> symbols
> referring to tw2865. I've seen another driver in the kernel tree, the 
> bluecherry
> solo6x10, but that's on the pci bus. as far as I can figure, the dvr uses i2c 
> for
> them. So, what I'm wondering is would it be feasible to factor out some of 
> the solo
> functionality into a generic tw2865 driver and be able to pull from that for 
> an i2c
> kernel module? I'd really hate to have to rewrite the whole thing, duplicated 
> code
> and overworking are generally a bad idea, even if I do have a datasheet for 
> the chip
> in question


Hi Matt,

Bluecherry LLC software developer here (barely knowing about tw28xx stuff).

If I was you, I'd restrain from such project unless I had a bulk of such
hardware, not just single unit. This is because things are hard to get
right without tinkering step by step. Also somebody would still need to
do fair amount of coding. And unless you are already qualified to do it
alone, you'd need many cycles of asking questions and providing lots of
details of your actual system. If you are interested in merging your
results upstream, then there's even more efforts to put.
As a developer affiliated with Bluecherry LLC, I will do my best to help
you, but I am mere mortal with all sorts of constraints - knowledge,
time, etc. But I or Bluecherry won't just make everything for you, even
if you send us a sample of hardware (which would be a good start for a
volunteer willing to take that challenge).
So feel free to post here your specific questions if you go for it.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] [media] pci: Add tw5864 driver - fixed few style nits, going to resubmit soon

[Andrey Utkin]

Found and fixed few very minor coding style nits, will resubmit in few days,
now still waiting for comments to v4.

https://github.com/bluecherrydvr/linux/commits/tw5864

commit 31f7c98a144cb3fb8a94662f002d9b6142d1f390
Author: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
Date:   Wed Jul 13 05:00:28 2016 +0300

Fix checkpatch --strict issue

 CHECK: Alignment should match open parenthesis
 #3599: FILE: drivers/media/pci/tw5864/tw5864-video.c:539:
 +static int tw5864_fmt_vid_cap(struct file *file, void *priv,
 +   struct v4l2_format *f)

commit 11a09a1048af597ecf374507b08c809eed91b86d
Author: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
Date:   Wed Jul 13 04:59:34 2016 +0300

Fix checkpatch --strict issue

 CHECK: Please don't use multiple blank lines
 #3244: FILE: drivers/media/pci/tw5864/tw5864-video.c:184:

commit 861b2ba8593db7abe89291a4ba85976519783f4a
Author: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
Date:   Wed Jul 13 04:58:37 2016 +0300

Fix checkpatch --strict issue

 CHECK: No space is necessary after a cast
 #3053: FILE: drivers/media/pci/tw5864/tw5864-util.c:36:
 +   return (u8) tw_readl(TW5864_IND_DATA);
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] [media] pci: Add tw5864 driver

[Andrey Utkin]

On Mon, Jul 11, 2016 at 09:40:53AM -0700, Joe Perches wrote:
> Each of these blocks will start with the dev_ prefix
> and the subsequent lines will not have the same prefix

Yes. I have checked how it looks before submitting, but I didn't see
this as a problem. I don't mind changing that (anyway I have found few
micro-issues with checkpatch --strict and would like to resubmit), but
would like to hear some second opinion.

> It also might be better to issue something like a single
> line dev_warn referring to the driver code and just leave
> this comment in the driver sources.
> 
> Something like:
> 
>   dev_warn(_dev->dev,
>   "This driver has known defects in video quality\n");

Things get complicated if you consider mainstream distros and their
years-behind kernels. The simplest way to preserve correspondence
between state of driver and such notice is to contain the notice in the
compiled driver. I hope the state of affairs will change to better
someday :)
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Add tw5864 driver

[Andrey Utkin]

Thanks for review Hans!

On Mon, Jul 11, 2016 at 07:58:38AM +0200, Hans Verkuil wrote:
> > +" v4l2-ctl --device $dev --set-ctrl=video_gop_size=1; done\n"
> 
> Replace $dev by /dev/videoX
> 
> Wouldn't it make more sense to default to this? And show the warning only if
> P-frames are enabled?

I believe it's better to leave P-frames on by default. All-I-frames
stream has huge bitrate. And the pixels artifacts is not very strong,
it's 0 - 10 bad pixels on picture at same time in our dev environment,
and probably up to 50 bad pixels max in other environments I know of.

> > +   dma_sync_single_for_cpu(>pci->dev, cur_frame->vlc.dma_addr,
> > +   H264_VLC_BUF_SIZE, DMA_FROM_DEVICE);
> > +   dma_sync_single_for_cpu(>pci->dev, cur_frame->mv.dma_addr,
> > +   H264_MV_BUF_SIZE, DMA_FROM_DEVICE);
> 
> This is almost certainly the wrong place. This should probably happen in the
> tasklet. The tasklet runs after the isr, so by the time the tasklet runs
> you've already called dma_sync_single_for_device.

Thanks, moved to tasklet subroutine tw5864_handle_frame().

I didn't seem to me like dma_sync_single_for_* can take long time or be
otherwise bad to be done from interrupt context.

> > +static int tw5864_querycap(struct file *file, void *priv,
> > +  struct v4l2_capability *cap)
> > +{
> > +   struct tw5864_input *input = video_drvdata(file);
> > +
> > +   strcpy(cap->driver, "tw5864");
> > +   snprintf(cap->card, sizeof(cap->card), "TW5864 Encoder %d",
> > +input->nr);
> > +   sprintf(cap->bus_info, "PCI:%s", pci_name(input->root->pci));
> > +   cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_READWRITE |
> > +   V4L2_CAP_STREAMING;
> > +   cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> 
> This line can be dropped, the core will fill in the capabilities field for 
> you.

No, removing this line causes v4l2-compliance failures and also ffmpeg fails to
play the device.

Required ioctls:
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL

Allow for multiple opens:
test second video open: OK
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] media: solo6x10: increase FRAME_BUF_SIZE

[Andrey Utkin]

In practice, devices sometimes return frames larger than current buffer
size, leading to failure in solo_send_desc().
It is not clear which minimal increase in buffer size would be enough,
so this patch doubles it, this should be safely assumed as sufficient.

Signed-off-by: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
---
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
index 8b1cde5..3991643 100644
--- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
@@ -33,7 +33,7 @@
 #include "solo6x10-jpeg.h"
 
 #define MIN_VID_BUFFERS2
-#define FRAME_BUF_SIZE (196 * 1024)
+#define FRAME_BUF_SIZE (400 * 1024)
 #define MP4_QS 16
 #define DMA_ALIGN  4096
 
-- 
2.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Add tw5864 driver

[Andrey Utkin]

Hi Hans,

Thanks for great help.
I believe the issues highlighted by your are rectified by now.

One chunk of your proposed changes seems to be wrong.

Also I have one non-technical change I want to introduce to this driver, see it
in the bottom of this letter ("Also, I decided to document known video quality
issues in a printed warning...").

On Fri, Jul 01, 2016 at 03:35:40PM +0200, Hans Verkuil wrote:
> On 06/10/2016 12:11 AM, Andrey Utkin wrote:
> > +   cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> 
> This line can be dropped: the v4l2 core will do this automatically.

This seems not so: dropping it resulted in new compliance fails:

Required ioctls:
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL

Allow for multiple opens:
test second video open: OK
fail: v4l2-compliance.cpp(550): dcaps & ~caps
test VIDIOC_QUERYCAP: FAIL


I am running latest v4l-utils from git.
This particular fail happens on kernels built from next-20160707 and
next-20160609.

BTW next-20160707 makes my dev machine to hang after few minutes of uptime,
regardless of my module being loaded, so for now I am testing driver on
next-20160609.
This (running old linux-next) causes such new fail with latest v4l-utils:

fail: v4l2-test-buffers.cpp(293): g_flags() & V4L2_BUF_FLAG_DONE

which is understandable because of recent commit to v4l-utils flipping expected
behaviour in this regard:

commit 7d784c6894b10cdf5ec025c2cd7c320320f5f658
Author: Hans Verkuil <hans.verk...@cisco.com>
Date:   Fri Jul 8 23:10:34 2016 +0200

v4l2-compliance: fix a check for the DONE flag

This was always set by vb2 drivers due to a bug. It is now cleared
again after that bug was fixed, but the test should now be inverted.

Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>

diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp 
b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index fb14170..dc82918 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -290,7 +290,7 @@ int buffer::check(unsigned type, unsigned memory, unsigned 
index,
fail_on_test(g_bytesused(p) > g_length(p));
}
fail_on_test(!g_timestamp().tv_sec && !g_timestamp().tv_usec);
-   fail_on_test(!(g_flags() & V4L2_BUF_FLAG_DONE));
+   fail_on_test(g_flags() & V4L2_BUF_FLAG_DONE);
fail_on_test((int)g_sequence() < seq.last_seq + 1);
if (v4l_type_is_video(g_type())) {
fail_on_test(g_field() == V4L2_FIELD_ALTERNATE);

So please expect this fail in v4l2-compliance logs of my new submission.



Also, I decided to document known video quality issues in a printed warning; I
like how it looks now both in code and in dmesg, but checkpatch.pl doesn't like
it. See commit at
https://github.com/bluecherrydvr/linux/commit/83395b6c5e1e5ceb642c9a04a28db5fc22566c87

The message is split in pieces because otherwise it gets truncated.

I'd like some approval or suggestion for rework on this.

It looks like this in dmesg:

[ 5101.182151] tw5864 :06:07.0: BEWARE OF KNOWN ISSUES WITH VIDEO QUALITY

   This driver was developed by Bluecherry LLC by deducing 
behaviour of original manufacturer's driver, from both source code and 
execution traces.
   It is known that there are some artifacts on output video with 
this driver:
- on all known hardware samples: random pixels of wrong color 
(mostly white, red or blue) appearing and disappearing on sequences of P-frames;
- on some hardware samples (known with H.264 core version 
e006:2800): total madness on P-frames: blocks of wrong luminance; blocks of 
wrong colors "creeping" across the picture.
   There is a workaround for both issues: avoid P-frames by setting 
GOP size to 1. To do that, run such command on device files created by this 
driver:

   for dev in /dev/video*; do v4l2-ctl --device $dev 
--set-ctrl=video_gop_size=1; done

[ 5101.357312] systemd-journald[219]: Compressed data object 850 -> 636 using XZ
[ 5101.471071] tw5864 :06:07.0: These issues are not decoding errors; all 
produced H.264 streams are decoded properly. Streams without P-frames don't 
have these artifacts so it's not analog-to-digital conversion issues nor 
internal memory errors; we conclude it's internal H.264 encoder issues.
   We cannot even check the original driver's behaviour because it 
has never worked properly at all in our development environment. So these 
issues may be actually related to firmware or hardware. However it may be that 
there's just some more register settings missing in the driver which would 
please the hardware.
   Manufacturer didn't help much on our inquiries, but feel free t

Re: [PATCH v2 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Andrey Utkin]

On Mon, Jun 27, 2016 at 11:12:42AM +0200, Hans Verkuil wrote:
> Andrey,
> 
> Since you are the original author, can you give me your Signed-off-by line?

No, as increasing buffer size by few kilobytes doesn't change anything. I've
increased it from 200 to 204, then found new occurances of the issue,
then increased it again and again by few kilobytes. Then I got that this
is not a (nice) solution, and have never came back to this. Maybe
doubling current buffer size would make users forget about this, but I'm
not sure maintainers would be glad with such patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Add tw5864 driver

[Andrey Utkin]

Update: added local change
 - to require fewer DMA buffers;
 - to require fewer vb2_queue buffers;
 - don't require vb2_queue buffers to be DMA-capable.
https://github.com/bluecherrydvr/linux/commit/410a86b08d230ff2a401ac9f5be3b30f8b29f30d
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

How should I use kernel-defined i2c structs in this driver

[Andrey Utkin]

Could anybody please give a hint - which kernel-defined i2c objects, and how
many of them, I need to define and use to substitute these driver-defined
functions i2c_read(), i2c_write() ?
https://github.com/bluecherrydvr/linux/blob/release/tw5864/1.16/drivers/media/pci/tw5864/tw5864-config.c
In a word, there's 4 chips with different addresses, to which this code
communicates via main chip's dedicated registers.
Do i need a single i2c_adapter or several?
Do i need i2c_client entities?
where should I put what is named "devid" here?

Thanks in advance.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Andrey Utkin]

On Wed, May 04, 2016 at 01:21:20PM -0300, Ismael Luceno wrote:
> From: Andrey Utkin <andrey.ut...@corp.bluecherry.net>
> 
> Such frame size is met in practice. Also report oversized frames.
> 
> [ismael: Reworked warning and commit message]
> 
> Signed-off-by: Ismael Luceno <ism...@iodev.co.uk>

I object against merging the first part.

> ---
>  drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c 
> b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> index 67a14c4..f98017b 100644
> --- a/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> +++ b/drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c
> @@ -33,7 +33,7 @@
>  #include "solo6x10-jpeg.h"
>  
>  #define MIN_VID_BUFFERS  2
> -#define FRAME_BUF_SIZE   (196 * 1024)
> +#define FRAME_BUF_SIZE   (200 * 1024)

Please don't push this.
It doesn't matter whether there are 196 or 200 KiB because there happen
bigger frames.
I don't remember details so I cannot point to all time max frame size.
AFAIK this issue appeared on one particular customer installation. I
don't monitor it closely right now. I think I have compiled custom
package for that setup with FRAME_BUF_SIZE increased much more (maybe
10x?).
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Andrey Utkin]

On Wed, May 4, 2016 at 5:04 PM, Hans Verkuil  wrote:
> BTW, looking at the MAINTAINERS file I see two email addresses for Andrey,
> neither of which is the fastmail.com address this email came from.

Now I'm replying from corporate email.

> Andrey, it might be a good idea to post such fixes to the mailinglist sooner,
> both to prevent situations like this and to keep the diffs between mainline
> and your internal code as small as possible.

In a word - we would do what is possible to achieve that, but there's
little time
and little incentive for that.
The codebases have already diverged a lot, having unique sets of runtime bugs.
And this exact issue alone is not resolved yet in a good way and is
not actually critical.
Merging would require a lot of working time. And it is complicated by
the fact that
there's not going to be any new manufacturing orders (the minimal order quantity
is too high for Bluecherry), and that we have picked tw5864 as
reachable for retail orders.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] solo6x10: Set FRAME_BUF_SIZE to 200KB

[Andrey Utkin]

On Sat, Apr 30, 2016 at 12:17:08AM -0300, Ismael Luceno wrote:
> Such frame size is met in practice. Also report oversized frames.
> 
> Based on patches by Andrey Utkin <andrey.ut...@corp.bluecherry.net>.

If it is based on my patches([1] [2]), then why you claim authorship and
why you don't let me know (at last CCing me)?

Do you know that 200 KiB is not the limit, just as previous value? I
haven't researched subj deep enough to figure out proven good value for
new buffer size.

It's both laughable and infuriating for me to spectate your behaviour of
"stealth driver developer".
You have added yourself back to driver maintainers in MAINTAINERS file
after your quit without letting us know.
You are not affiliated with Bluecherry for two years, and you are not
informed about how the driver is working in production on customers
setups. So you are not aware what are real issues with it. BTW do you
still have a sample of actual hardware? Yeah, I agree that this can be
argument against Bluecherry and lack of openness in its bug tracking.
But you are also not open and not collaborating.

The point of my accusation to you is that you seem to be just gaining
"kernel developer" score for nobody's (except your CV's) benefit.
Development and maintenance is what Hans Verkuil, Krzysztof Halasa and
others do to this driver, but not this.

Sorry to be harsh.

[1] 
https://github.com/bluecherrydvr/solo6x10/commit/5cd985087362e2e524b3e44504eea791ae7cda7e
[2] 
https://github.com/bluecherrydvr/solo6x10/commit/3b437f79c40438eb09bb2d5dbcfe67dbc94648ed
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Add tw5864 driver - cover letter

[Andrey Utkin]

From: Andrey Utkin <andrey.ut...@corp.bluecherry.net>

This is a driver for multimedia devices based on Techwell/Intersil TW5864 chip.

It is basically written from scratch. There was an awful reference driver for
2.6 kernel, which is nearly million lines of code and requires half a dozen
special userspace libraries, and still doesn't quite work. So currently
submitted driver is a product of reverse-engineering and heuristics. tw68
driver was used as code skeleton.

The device advertises many capabilities, but this version of driver only
supports H.264 encoding of captured video channels.

There is one known issue, which reproduces on two of five setups of which I
know: P-frames are distorted, but I-frames are fine. Changing quality and
framerate settings does not affect this. Currently such workaround is used:

v4l2-ctl -d /dev/video$n -c video_gop_size=1

GOP size is set to 1, so that every output frame is I-frame. 
We are regularly contacting manufacturer regarding such issues, but
unfortunately they can do little to help us.


Andrey Utkin (1):
  Add tw5864 driver

 MAINTAINERS  |7 +
 drivers/staging/media/Kconfig|2 +
 drivers/staging/media/Makefile   |1 +
 drivers/staging/media/tw5864/Kconfig |   11 +
 drivers/staging/media/tw5864/Makefile|3 +
 drivers/staging/media/tw5864/tw5864-bs.h |  154 ++
 drivers/staging/media/tw5864/tw5864-config.c |  359 +
 drivers/staging/media/tw5864/tw5864-core.c   |  453 ++
 drivers/staging/media/tw5864/tw5864-h264.c   |  183 +++
 drivers/staging/media/tw5864/tw5864-reg.h| 2200 ++
 drivers/staging/media/tw5864/tw5864-tables.h |  237 +++
 drivers/staging/media/tw5864/tw5864-video.c  | 1364 
 drivers/staging/media/tw5864/tw5864.h|  280 
 include/linux/pci_ids.h  |1 +
 14 files changed, 5255 insertions(+)
 create mode 100644 drivers/staging/media/tw5864/Kconfig
 create mode 100644 drivers/staging/media/tw5864/Makefile
 create mode 100644 drivers/staging/media/tw5864/tw5864-bs.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-config.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-core.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-h264.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-reg.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-tables.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-video.c
 create mode 100644 drivers/staging/media/tw5864/tw5864.h

-- 
2.7.1.380.g0fea050.dirty

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Add tw5864 driver - cover letter

[Andrey Utkin]

This is a driver for multimedia devices based on Techwell/Intersil TW5864 chip.

It is basically written from scratch. There was an awful reference driver for
2.6 kernel, which is nearly million lines of code and requires half a dozen
special userspace libraries, and still doesn't quite work. So currently
submitted driver is a product of reverse-engineering and heuristics. tw68
driver was used as code skeleton.

The device advertises many capabilities, but this version of driver only
supports H.264 encoding of captured video channels.

There is one known issue, which reproduces on two of five setups of which I
know: P-frames are distorted, but I-frames are fine. Changing quality and
framerate settings does not affect this. Currently such workaround is used:

v4l2-ctl -d /dev/video$n -c video_gop_size=1

GOP size is set to 1, so that every output frame is I-frame. 
We are regularly contacting manufacturer regarding such issues, but
unfortunately they can do little to help us.


Andrey Utkin (1):
  Add tw5864 driver

 MAINTAINERS  |7 +
 drivers/staging/media/Kconfig|2 +
 drivers/staging/media/Makefile   |1 +
 drivers/staging/media/tw5864/Kconfig |   11 +
 drivers/staging/media/tw5864/Makefile|3 +
 drivers/staging/media/tw5864/tw5864-bs.h |  154 ++
 drivers/staging/media/tw5864/tw5864-config.c |  359 +
 drivers/staging/media/tw5864/tw5864-core.c   |  453 ++
 drivers/staging/media/tw5864/tw5864-h264.c   |  183 +++
 drivers/staging/media/tw5864/tw5864-reg.h| 2200 ++
 drivers/staging/media/tw5864/tw5864-tables.h |  237 +++
 drivers/staging/media/tw5864/tw5864-video.c  | 1364 
 drivers/staging/media/tw5864/tw5864.h|  280 
 include/linux/pci_ids.h  |1 +
 14 files changed, 5255 insertions(+)
 create mode 100644 drivers/staging/media/tw5864/Kconfig
 create mode 100644 drivers/staging/media/tw5864/Makefile
 create mode 100644 drivers/staging/media/tw5864/tw5864-bs.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-config.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-core.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-h264.c
 create mode 100644 drivers/staging/media/tw5864/tw5864-reg.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-tables.h
 create mode 100644 drivers/staging/media/tw5864/tw5864-video.c
 create mode 100644 drivers/staging/media/tw5864/tw5864.h

-- 
2.7.1.380.g0fea050.dirty

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

On Fri, 11 Mar 2016 10:59:02 +0100
Hans Verkuil  wrote:
> While userspace may specify FIELD_ANY when setting a format, the
> driver should always map that to a specific field setting and should
> never return FIELD_ANY back to userspace.
> 
> In this case, the 'field' field of the v4l2_buffer struct has
> FIELD_ANY which means it is not set correctly (or at all) in the
> driver.
> 
> It's a common mistake, which is why v4l2-compliance tests for it :-)

Thanks for great guidance Hans, finally I have solved all issues.

You can review latest state at tw5864 branch, also you can review
changelog of v4l2-compliance fixing at tags tw5864_pre_1.11,
tw5864_pre_1.10 of https://github.com/bluecherrydvr/linux .

I will make a final internal review before submission, and try
to submit the driver for inclusion.

Everybody is appreciated to make any comments even before submission,
the actual code to review is at tw5864 branch.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

On Fri, 11 Mar 2016 09:00:18 +0100
Hans Verkuil  wrote:
> The reason is likely to be the tw5864_queue_setup function which has
> not been updated to handle CREATE_BUFS support correctly. It should
> look like this:
> 
> static int tw5864_queue_setup(struct vb2_queue *q,
> unsigned int *num_buffers,
> unsigned int *num_planes, unsigned int
> sizes[], void *alloc_ctxs[])
> {
>   struct tw5864_input *dev = vb2_get_drv_priv(q);
> 
>   if (q->num_buffers + *num_buffers < 12)
>   *num_buffers = 12 - q->num_buffers;
> 
>   alloc_ctxs[0] = dev->alloc_ctx;
>   if (*num_planes)
>   return sizes[0] < H264_VLC_BUF_SIZE ? -EINVAL : 0;
> 
>   sizes[0] = H264_VLC_BUF_SIZE;
>   *num_planes = 1;
> 
>   return 0;
> }

Thanks for suggestion, but now the failure looks this way:

Streaming ioctls:
test read/write: OK
fail: v4l2-test-buffers.cpp(297): g_field() == V4L2_FIELD_ANY
fail: v4l2-test-buffers.cpp(703): buf.check(q, last_seq)
fail: v4l2-test-buffers.cpp(976): captureBufs(node, q, m2m_q, 
frame_count, false)
test MMAP: FAIL

Will check that later. If you have any suggestions, I would be very
grateful.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

Hi Hans!

Some improvements took place on the driver, including cleaner
v4l2-compliance tests passing. But there's a single test failure I
don't understand.

In the code of v4l2-compliance, it seems like an API
call CREATE_BUFS is supposed to fail with EINVAL. But in case of my
driver, which simply uses vb2_ioctl_create_bufs(), the call returns 0.

Please review the below report.

The report is produced by fresh v4l-utils from git:
v4l-utils-1.10.0-59-g1388c0a

The actual code which was tested is at tag release/tw5864/1.10 of
https://github.com/bluecherrydvr/linux.git , see
drivers/staging/media/tw5864 . If we sort this out, I am considering
resubmit the driver for merging upstream.

There's also another issue with v4l2-compliance. At some moment the
driver receives S_PARM command with timeperframe 0/0, by which reason I
added special handling, but I guess there's something out of my
knowledge which caused this and which should be fixed.


 # v4l2-compliance -d /dev/video6 -s
Driver Info:
Driver name   : tw5864
Card type : TW5864 Encoder 2
Bus info  : PCI::06:05.0
Driver version: 4.5.0
Capabilities  : 0x8521
Video Capture
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps   : 0x0521
Video Capture
Read/Write
Streaming
Extended Pix Format

Compliance test for device /dev/video6 (not using libv4l2):

Required ioctls:
test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK

Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK

Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 11 Private Controls: 0

Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK (Not Supported)

Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK

Test input 0:

Streaming ioctls:
test read/write: OK
fail: v4l2-test-buffers.cpp(959): ret != EINVAL
test MMAP: FAIL
test USERPTR: OK (Not Supported)
test DMABUF: Cannot test, specify --expbuf-device


Total: 45, Succeeded: 44, Failed: 1, Warnings: 0
[ERR]
14:14:15root@tw ~
 # gdb v4l2-compliance 
GNU gdb (Ubuntu 7.10-1ubuntu3) 7.10
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
 This is free software: you are free
to change and redistribute it. There is NO WARRANTY, to the extent
permitted by law.  Type "show copying" and "show warranty" for details.
This GDB was configured as "i686-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:

Re: [RFC PATCH v0] Add tw5864 driver

[Andrey Utkin]

On Sun, Jan 3, 2016 at 5:47 AM, Joe Perches <j...@perches.com> wrote:
> several of these have unnecessary parentheses

Thanks, fixed.

> Maybe use bool a bit more

Thanks, fixed.

> or maybe just use fls

Thanks, fls() fit greatly, rewritten the function with compatibility testing.

>> +static inline int bs_size_ue(unsigned int val)
>> +{
>> + int i_size = 0;
>> + static const int i_size0_254[255] = {
>
> Same sort of thing

Dropped this procedure because it is not used.
Thanks.

>> diff --git a/drivers/staging/media/tw5864/tw5864-config.c 
>> b/drivers/staging/media/tw5864/tw5864-config.c
> []
>> +u8 tw_indir_readb(struct tw5864_dev *dev, u16 addr)
>> +{
>> + int timeout = 3;
>
> misleading name, retries would be more proper,
> or maybe use real timed loops.

Thanks, renamed to "retries".

> This seems a little repetitive.

Thanks, reworked.

> u16?

Thanks, fixed.

> odd indentation

Indeed. For some mysterious reason, vim + syntastic insists on this way. Fixed.

>> +#ifdef DEBUG
>> + dev_dbg(>root->pci->dev,
>> + "input %d, frame md stats: min %u, max %u, avg %u, cells above 
>> threshold: %u\n",
>> + input->input_number, min, max, sum / md_cells,
>> + cnt_above_thresh);
>> +#endif
>
> unnecessary #ifdef

Not quite. This debug printout works with variables which are declared
in another "#ifdef DEBUG" clause. And it turns out that dev_dbg is
compiled not only if DEBUG is declared, so when I remove this ifdef, I
get "undefined variable" errors. It seems it is compiled if this
condition is met:

#if (defined DEBUG) || (defined CONFIG_DYNAMIC_DEBUG)

so I can wrap my stats variables into this statement instead. But such
change is not equivalent - I guess CONFIG_DYNAMIC_DEBUG is common to
be enabled, so debug stats will be always calculated, even when module
is not under debug. Except if I use DEFINE_DYNAMIC_DEBUG_METADATA etc.
in my code. Please let me know if this can be sorted out in cleaner
way.



On Sun, Jan 3, 2016 at 7:38 AM, Leon Romanovsky <l...@leon.nu> wrote:
> On Sun, Jan 03, 2016 at 03:41:42AM +0200, Andrey Utkin wrote:
> 
>> +/*
>> + *  TW5864 driver - Exp-Golomb code functions
>> + *
>> + *  Copyright (C) 2015 Bluecherry, LLC <maintain...@bluecherrydvr.com>
>> + *  Copyright (C) 2015 Andrey Utkin <andrey.ut...@corp.bluecherry.net>
>
> I doubt that you have contract with your employer which permits you to
> claim copyright on the work/product.

Thank you for commenting.
I have previously asked my employer to review copyright statment, and
he told this is fine.
Now I have requrested him again with reference to your comment.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: On Lindent shortcomings and massive style fixing

[Andrey Utkin]

On Tue, Dec 29, 2015 at 9:32 AM, Mauro Carvalho Chehab
 wrote:
> IMHO, there are two problems by letting indent breaking long
> lines:
>
> 1) indent would break strings on printks. This is something that we don't
> want to break strings on multiple lines in the Kernel;

Yeah, GNU indent does its work badly (although I believe it could be
taught to respect long literals), this makes me want to have a better
tool for clean "relayouting" C code.
I believe that'd require at last a proper syntax parser. With such
tool, it would be straightforward to rewrite source code automatically
to please any demanding reviewer of code style, except for issues of
higher level of thought (like naming or nesting limitations).

> 2) It doesn't actually solve the problem of having too complex loops,
> with is why the 80 columns warning is meant to warn. Worse than that,
> if a piece of code is inside more than 4 or 5 indentation levels, the
> resulting code of using indent for 80-cols line break is a total crap.

Then I'd propose to enforce nesting limitation explicitly, because
Documentation/CodingStyle appreciates low nesting, just doesn't give
exact numbers.
It's better this way, because the programmer could pay attention to N
places of excessive nesting and fix it manually, and then carelessly
reformat NNN places of "80 chars" issues automatically, comparing to
reviewing all NNN places, to figure out if there's excessive nesting,
or not.
(CCed checkpatch.pl maintainers.)

> That's said, on a quick look at the driver, it seems that the 80-cols
> violations are mostly (if not all) on the comments, like:
>
> int i_poc_lsb = (frame_seqno_in_gop << 1); /* why multiplied by two? 
> TODO try without multiplication */
>
> and
>
> #define TW5864_UNDEF_REG_0x0224 0x0224  /* Undeclared in spec (or not yet 
> added to tw5864-reg.h) but used */
> #define TW5864_UNDEF_REG_0x4014 0x4014  /* Undeclared in spec (or not yet 
> added to tw5864-reg.h) but used */
> #define TW5864_UNDEF_REG_0xA800 0xA800  /* Undeclared in spec (or not yet 
> added to tw5864-reg.h) but used */
>
> Btw, the content of tw5864-reg-undefined.h is weird... Why not just
> add the stuff there at tw5864-reg.h and remove the comments for all
> defines there?

tw5864-reg-undefined.h will be edited for sure (maybe dropped), of
course it won't stay as it is now.
It was generated by script during reverse-engineering that bastard
chip from hell.

> Also, Lindent already did some crappy 80-cols like breaks, like:
>
> static int pci_i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn, u8 
> *buf,
>u32 count)
>
> (count is misaligned with the open parenthesis)

I just added "static " after indenting.
Actually, Documentation/CodingStyle says nothing about alignment of
function declaration tail on open parenthesis.
Also I'd like to mention that I hate such alignment, because it
requires intermixing of tabs and spaces. I am not aware if this is K
thing or not. If it is, then please don't kill me.

Thanks for kind replies, gentlemen.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Lindent shortcomings and massive style fixing

[Andrey Utkin]

After some iterations of checkpatch.pl, on a new developed driver
(tw5864), now I have the following:

 $ grep 'WARNING\|ERROR' /src/checkpatch.tw5864 | sort | uniq -c
 31 ERROR: do not use C99 // comments
147 WARNING: Block comments use a trailing */ on a separate line
144 WARNING: Block comments use * on subsequent lines
435 WARNING: line over 80 characters

At this point, Lindent was already used, and checkpatch.pl warnings
introduced by Lindent itself were fixed. Usage of "indent
--linux-style" (which behaves differently BTW) doesn't help anymore,
too.

Could anybody please advise how to sort out these issues
automatically, because they look like perfectly solvable in automated
fashion. Of course manual work would result in more niceness, but I am
not eager to go through hundreds of place of code just to fix "over 80
characters" issues now.

First one ("do not use C99 // comments") looks easy with regexps, but
the other are not.

Is there any known improvements or successors for Lindent? Or could we
get indent/Lindent improved if we collect some money for this task?

If anybody wants to look at actual code, here it is:
https://github.com/bluecherrydvr/linux.git , branch tw5864_stable,
drivers/staging/media/tw5864

Current output of "checkpatch.pl -f" for all source files in the
driver is here:
https://gist.github.com/andrey-utkin/12295148475e34ef948b

Thanks in advance.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

H264 headers generation for driver

[Andrey Utkin]

This is a new chapter of tw5864 video grabber & encoder driver
development drama.
Last state of code is here (tw5864 branch, drivers/staging/media/tw5864):
https://github.com/bluecherrydvr/linux/tree/tw5864/drivers/staging/media/tw5864

Currently I use a third-side LGPL library for H.264 headers generation
- SPS, PPS and slice headers (because device doesn't generate them).
It is included as a git submodule "h264bitstream". It is used from
tw5864-h264.c .
Of course we want our driver to get to upstream repository when it
matures enough, that's why we want to ask for advice regarding this.
I see that there is no similar case in upstream kernel repo - no
submodules and no libraries for H264 bitstreams.
Device datasheet
(http://lizard.bluecherry.net/~autkin/tw5864/tw5864b1-ds.pdf , page
47) shows that there's almost no variety of modes, so minimally an
implementation of bitstream writing functions ue() and se() will
suffice.
I guess that one acceptable way is to pre-generate all headers for all
needed cases and ship them inlined; for correctness checking purpose,
it is possible to ship also a script or additional source code file
which is able to generate same headers.
Please advise.

Thanks in advance for any kind reply.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Help to debug h264 headers (or video) generation in kernel driver

[Andrey Utkin]

I'm working on Linux kernel driver for multimedia grabber and H264
encoder based on Techwell/Intersil 5864 chip. Of course it will be
open and pushed into upstream kernel.

The device produces H264 encoded data, but it lacks any headers. The
reference driver generates headers and glues frames together in code,
but I failed both reverse-engineering and porting that code. The code
of reference driver is overwhelmingly complicated, and I have no
knowledge how H264 bitstream is formed, that's why my attempts failed.

I have reached a limited success with both setting up the hardware
encoder and forming the headers. You can see the samples of produced
video here:
http://lizard.bluecherry.net/~autkin/tw5864_tiled_video/ntsc_cif.h264
http://lizard.bluecherry.net/~autkin/tw5864_tiled_video/ntsc_d1.h264
http://lizard.bluecherry.net/~autkin/tw5864_tiled_video/pal_cif.h264
http://lizard.bluecherry.net/~autkin/tw5864_tiled_video/pal_d1.h264

Currently it produces "tiled" picture. The width of tiles on these
sample videos is 256 pixels.
I am not sure whether the issue is in hardware settings, or incorrect
headers which lead to partial failure of decoding.
Here is ffplay log for playing back such video:
https://gist.github.com/krieger-od/5269c762a36bcb52b93a . Obviously
some enhancements to headers generation are needed.

Playing with hardware mirroring flags proves that the raw frame gets
caught fully and correctly. But for h264 encoding, it gets garbled.
Also please notice that frame order is incorrect, the motion goes back
and forth.

You can see our latest code for this development at
https://github.com/bluecherrydvr/linux/tree/tw5864/drivers/staging/media/tw5864
(branch tw5864, subdirectory drivers/staging/media/tw5864).

Any input is extremely appreciated.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [x264-devel] Help to debug h264 headers (or video) generation in kernel driver

[Andrey Utkin]

On Sat, Sep 26, 2015 at 10:44 AM, Max Lapshin  wrote:
> Do you get some packets from this device or it is a bytestream?

Oh hi a linux.org.ru buddy :)
I get headerless binary data portions of known length, one portion for
each encoded video frame. NAL headers generation is done on driver
level in reference software, and I have no choice but to follow same
approach.
You can check most of the info about this project from my earlier
request for help: https://lkml.org/lkml/2015/6/2/761

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw5864 driver development, help needed

[Andrey Utkin]

An update: see there:
http://ffmpeg.org/pipermail/ffmpeg-devel/2015-July/175796.html


-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw5864 driver development, help needed

[Andrey Utkin]

An update with a request for help.

Asking for help with h264 headers generation. Both copying headers
from similar videos and porting headers generation code from reference
driver don't work for me (ref driver is weird and very complicated, so
porting involved importing of lots of code, but still in my case the
generated header differs from the one produced by reference driver,
and resulting files are not decodable).
Wireshark seems not able to dissect raw h264 files, so that I could
compare headers bitfield-wise.
I have dumped 64 first encoded frames as they appear from hardware, so
that you could look at it.
This is archive: http://lizard.bluecherry.net/~autkin/vlc.tar.gz and
this is contents list of archive
http://lizard.bluecherry.net/~autkin/vlc.contents

Only one camera is attached to this multi-channel video grabber and
encoder, and i don't know which chip has it (4, 5, 6 or 7).
The produced frames should have the same settings as this file:
http://lizard.bluecherry.net/~autkin/test_main.h264
This is PAL D1, 720x576.
Thanks in advance for any help. At last, the ability to look at
visualization of those h264 streams would be great.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw5864 driver development, help needed

[Andrey Utkin]

On Fri, Jul 3, 2015 at 5:23 PM, Andrey Utkin
andrey.ut...@corp.bluecherry.net wrote:
 Up... we are moving much slower than we expected, desperately needing help.

 Running reference driver with Ubuntu 9 (with kernel 2.6.28.10) with
 16-port card shows that the
 reference driver fails to work with it correctly. Also that driver is
 not complete, it requires your userland counterpart for usable
 operation, which is far from being acceptable in production.

 Currently what stops us with our driver is that H264 encoding done
 interrupt doesn't repeat, and CRC checksums mismatch for the first
 (and last) time this interrupt happens.
 We do our best to mimic what the reference driver does, but we might
 miss some point.

 I suspect that my initialization of video inputs or board clock
 configuration is insufficient or inconsistent with what device needs.

 Our work in progress is located in
 https://github.com/krieger-od/linux, directory
 drivers/staging/media/tw5864

 This is another request for expert help.
 The time is very important for us now.

 Thanks in advance and sorry for distraction.

Thanks for all who contacted us! Now we know we can count on you.
Just a small update so that you know the status of the project (and so
that you won't proceed looking into the last described issue, if this
is the case). We have interrupts repeating, and CRC checksums
matching. Now we are working on formatting of h264 stream (the frames
are returned without any headers, and reference driver has header
generation deeply and tightly bound to other code, so this will take
some time.
The latest state of this work in progress is in
github.com/krieger-od/linux.git , branch brutal.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Subjective maturity of tw6869, cx25821, bluecherry/softlogic drivers

[Andrey Utkin]

On Fri, Jul 3, 2015 at 1:46 PM, Krzysztof Hałasa khal...@piap.pl wrote:
 Also, TW686x are (mini) PCIe while SOLO6110 (and earlier SOLO6010 which
 produces MPEG4 part 2 instead of H.264) are (mini) PCI.

solo6110 is PCI-E, not PCI.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tw5864 driver development, help needed

[Andrey Utkin]

On Wed, Jun 3, 2015 at 1:03 AM, Andrey Utkin
andrey.ut...@corp.bluecherry.net wrote:
 Hi! I am working on making a Linux driver for TW5864-based videoaudio
 capture and encoding PCI boards. The driver is to be submitted for
 inclusion to Linux upstream.
 The following two links are links to boards available for buying:
 http://www.provideo.com.tw/web/DVR%20Card_TW-310.htm
 http://www.provideo.com.tw/web/DVR%20Card_TW-320.htm
 We possess one 8-port board and we try to make it play.

 http://whdd.org/tw5864/TW-3XX_Linux.rar - this is reference driver
 code. Overwhelmingly complicated IMO.
 http://whdd.org/tw5864/tw5864b1-ds.pdf - Datasheet.
 http://whdd.org/tw5864/TW5864_datasheet_0.6d.pdf - Another datasheet.
 These two differ in some minor points.
 https://github.com/krieger-od/linux - my work in progress on this, in
 drivers/staging/media/tw5864 directory. Derived from
 drivers/media/pci/tw68 (which is raw video capture card), defined
 reasonable part of registers, now trying to make device produce video
 capture and encoding interrupts, but cannot get any interrupts except
 GPIO and timer ones. This is currently the critical blocking issue in
 development.
 I hope that somebody experienced with similar boards would have
 quesswork on how to proceed.
 My work-on-progress code is dirty, so if you would agree to check that
 only if it will be cleaned up, please let me know.

 I am willing to pay for productive help.

 --
 Bluecherry developer.


Up... we are moving much slower than we expected, desperately needing help.

Running reference driver with Ubuntu 9 (with kernel 2.6.28.10) with
16-port card shows that the
reference driver fails to work with it correctly. Also that driver is
not complete, it requires your userland counterpart for usable
operation, which is far from being acceptable in production.

Currently what stops us with our driver is that H264 encoding done
interrupt doesn't repeat, and CRC checksums mismatch for the first
(and last) time this interrupt happens.
We do our best to mimic what the reference driver does, but we might
miss some point.

I suspect that my initialization of video inputs or board clock
configuration is insufficient or inconsistent with what device needs.

Our work in progress is located in
https://github.com/krieger-od/linux, directory
drivers/staging/media/tw5864

This is another request for expert help.
The time is very important for us now.

Thanks in advance and sorry for distraction.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

tw5864 driver development, help needed

[Andrey Utkin]

Hi! I am working on making a Linux driver for TW5864-based videoaudio
capture and encoding PCI boards. The driver is to be submitted for
inclusion to Linux upstream.
The following two links are links to boards available for buying:
http://www.provideo.com.tw/web/DVR%20Card_TW-310.htm
http://www.provideo.com.tw/web/DVR%20Card_TW-320.htm
We possess one 8-port board and we try to make it play.

http://whdd.org/tw5864/TW-3XX_Linux.rar - this is reference driver
code. Overwhelmingly complicated IMO.
http://whdd.org/tw5864/tw5864b1-ds.pdf - Datasheet.
http://whdd.org/tw5864/TW5864_datasheet_0.6d.pdf - Another datasheet.
These two differ in some minor points.
https://github.com/krieger-od/linux - my work in progress on this, in
drivers/staging/media/tw5864 directory. Derived from
drivers/media/pci/tw68 (which is raw video capture card), defined
reasonable part of registers, now trying to make device produce video
capture and encoding interrupts, but cannot get any interrupts except
GPIO and timer ones. This is currently the critical blocking issue in
development.
I hope that somebody experienced with similar boards would have
quesswork on how to proceed.
My work-on-progress code is dirty, so if you would agree to check that
only if it will be cleaned up, please let me know.

I am willing to pay for productive help.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: On register r/w macros/procedures of drivers/media/pci

[Andrey Utkin]

On Mon, Apr 20, 2015 at 3:45 PM, Krzysztof Hałasa khal...@piap.pl wrote:
 Andrey Utkin andrey.ut...@corp.bluecherry.net writes:

 Please check first digit. I mean _5_864, in your post there's 6869.

 Ok, I just thought it may be a typo. I can now see 5864 is a H.264
 encoder, while 686x are simpler frame-grabbers only.

 Sorry for the noise.

Nothing to excuse for!
Thanks for the info anyway!

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: On register r/w macros/procedures of drivers/media/pci

[Andrey Utkin]

On Mon, Apr 20, 2015 at 2:18 PM, Krzysztof Hałasa khal...@piap.pl wrote:
 Andrey Utkin andrey.ut...@corp.bluecherry.net writes:

 I am starting a work on driver for techwell tw5864 media grabberencoder.

 If this is tw6864 then I have a driver mostly completed.
 Actually I'm using tw6869 but I think this is very similar (4 channels
 instead of 8 and PCI instead of PCIe). I have 6864s but haven't yet
 tried using them for trivial reasons.

 This is BTW completely different chip (family) than the old tw68x.

Please check first digit. I mean _5_864, in your post there's 6869.
Please specify.
Very interested.anyway. Thanks.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On register r/w macros/procedures of drivers/media/pci

[Andrey Utkin]

I am starting a work on driver for techwell tw5864 media grabberencoder.
I am basing on tw68 driver (mentorship, advising and testing by board
owners are appreciated). And here (and in some other
drivers/media/pci/ drivers) I see what confuses me:

tw68-core.c:
dev-lmmio = ioremap(pci_resource_start(pci_dev, 0),
 pci_resource_len(pci_dev, 0));
dev-bmmio = (__u8 __iomem *)dev-lmmio;

tw68.h:
#define tw_readl(reg)   readl(dev-lmmio + ((reg)  2))
#define tw_readb(reg)   readb(dev-bmmio + (reg))
#define tw_writel(reg, value)   writel((value), dev-lmmio + ((reg)  2))
#define tw_writeb(reg, value)   writeb((value), dev-bmmio + (reg))

I am mostly userland developer and I wouldn't expect bmmio pointer to
contain value numerically different from lmmio after such simple
casting.
But still, if this is correct, then how should I implement
tw_{read,write}w to operate on 2 bytes (a word)? Similarly, it would
look like this:
#define tw_readl(reg)   readl(dev-lmmio + ((reg)  1))
That looks odd.

In contrary, in drivers/media/pci/dm1105, we see no explicit shifting
of register address. It uses {in,out}{b,w,l} macros, which seem to
turn out the same {read,write}{b,w,l} (with some reservations):
http://lxr.free-electrons.com/source/include/asm-generic/io.h#L354

dm1105.c:#define dm_io_mem(reg) ((unsigned long)(dev-io_mem[reg]))
dm1105.c:#define dm_readb(reg)  inb(dm_io_mem(reg))
dm1105.c:#define dm_writeb(reg, value)  outb((value), (dm_io_mem(reg)))
dm1105.c:#define dm_readw(reg)  inw(dm_io_mem(reg))
dm1105.c:#define dm_writew(reg, value)  outw((value), (dm_io_mem(reg)))
dm1105.c:#define dm_readl(reg)  inl(dm_io_mem(reg))
dm1105.c:#define dm_writel(reg, value)  outl((value), (dm_io_mem(reg)))

This looks like contradiction to me (shifting register address vs. no
shifting), so that one practice may be even just wrong and broken
(which is hard to believe due to active maintenance of all drivers).
I highly appreciate your help me in determining the best practice to
follow in this new driver.
Thanks.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: On register r/w macros/procedures of drivers/media/pci

[Andrey Utkin]

On Sun, Apr 19, 2015 at 11:28 AM, Hans Verkuil hverk...@xs4all.nl wrote:
 Check the types of llmio and bbmio:

 u32 __iomem *lmmio;
 u8  __iomem *bmmio;

 So the values of the pointers are the same, but the types are not.

 So 'lmmio + 1' == 'bmmio + sizeof(u32)' == 'bbmio + 4'.

 Since all the registers are defined as byte offsets relative to the start
 of the memory map you cannot just do 'lmmio + reg' since that would be a
 factor 4 off. Instead you have to divide by 4 to get it back in line.

 Frankly, I don't think lmmio is necessary at all since readl/writel don't
 need a u32 pointer at all since they use void pointers. I never noticed
 that when I cleaned up the tw68 driver. Using 'void __iomem *mmio' instead
 of lmmio/bmmio and dropping the shifts in the tw_ macros would work just
 as well.


 Hope this helps,

Oh, indeed, I have forgot this basic thing of pointer arithmetics.
Thanks a lot for elaboration and the proposed solution.

-- 
Bluecherry developer.
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html