Re: [PATCH] media: em28xx: fix a regression with HVR-950

[Devin Heitmueller]

On Tue, Jun 26, 2018 at 4:09 PM, Brad Love  wrote:
> Hi Mauro,
>
> It turns out this patch breaks DualHD multiple tuner capability. When
> alt mode is set in start_streaming it immediately kills the other tuners
> stream. Essentially both tuners cannot be used together when this is
> applied. I unfortunately don't have a HVR-950 to try and fix the
> original regression better. Can you please take another look at this?
> Until this is sorted, DualHD are effectively broken.

Yeah, because the Empia device uses the same USB Interface for both
endpoints, you need to have a reference count and only change the
alternate when the first endpoint is put into use and then only reset
the alternate back to zero when neither endpoint is in use.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] au8522: remove duplicate code

[Devin Heitmueller]

Reviewed-by: Devin Heitmueller <dheitmuel...@kernellabs.com>

Devin

On Tue, May 22, 2018 at 1:09 PM, Gustavo A. R. Silva
<gust...@embeddedor.com> wrote:
> This code has been there for nine years now, and it has been
> working "good enough" since then [1].
>
> Remove duplicate code by getting rid of the if-else statement.
>
> [1] https://marc.info/?l=linux-kernel=152693550225081=2
>
> Cc: Devin Heitmueller <dheitmuel...@kernellabs.com>
> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
> ---
>  drivers/media/dvb-frontends/au8522_decoder.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/au8522_decoder.c 
> b/drivers/media/dvb-frontends/au8522_decoder.c
> index 343dc92..f285096 100644
> --- a/drivers/media/dvb-frontends/au8522_decoder.c
> +++ b/drivers/media/dvb-frontends/au8522_decoder.c
> @@ -280,14 +280,12 @@ static void setup_decoder_defaults(struct au8522_state 
> *state, bool is_svideo)
> AU8522_TOREGAAGC_REG0E5H_CVBS);
> au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS);
>
> -   if (is_svideo) {
> -   /* Despite what the table says, for the HVR-950q we still need
> -  to be in CVBS mode for the S-Video input (reason unknown). 
> */
> -   /* filter_coef_type = 3; */
> -   filter_coef_type = 5;
> -   } else {
> -   filter_coef_type = 5;
> -   }
> +   /*
> +* Despite what the table says, for the HVR-950q we still need
> +* to be in CVBS mode for the S-Video input (reason unknown).
> +*/
> +   /* filter_coef_type = 3; */
> +   filter_coef_type = 5;
>
> /* Load the Video Decoder Filter Coefficients */
> for (i = 0; i < NUM_FILTER_COEF; i++) {
> --
> 2.7.4
>



-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [media] duplicate code in media drivers

[Devin Heitmueller]

>> diff -u -p drivers/media/dvb-frontends/au8522_decoder.c 
>> /tmp/nothing/media/dvb-frontends/au8522_decoder.c
>> --- drivers/media/dvb-frontends/au8522_decoder.c
>> +++ /tmp/nothing/media/dvb-frontends/au8522_decoder.c
>> @@ -280,14 +280,9 @@ static void setup_decoder_defaults(struc
>> AU8522_TOREGAAGC_REG0E5H_CVBS);
>> au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS);
>>
>> -   if (is_svideo) {
>> /* Despite what the table says, for the HVR-950q we still 
>> need
>>to be in CVBS mode for the S-Video input (reason 
>> unknown). */
>> /* filter_coef_type = 3; */
>> -   filter_coef_type = 5;
>> -   } else {
>> -   filter_coef_type = 5;
>> -   }
>
> Better ask Devin about this (c/c).

This was a case where the implementation didn't match the datasheet,
and it wasn't clear why the filter coefficients weren't working
properly.  Essentially I should have labeled that as a TODO or FIXME
when I disabled the "right" value and forced it to always be five.  It
was also likely that the filter coefficients would need to differ if
taking video over the IF interface as opposed to CVBS/S-video, which
is why I didn't want to get rid of the logic entirely.  That said, the
only product I've ever seen with the tda18271 mated to the au8522 will
likely never be supported for analog video under Linux for unrelated
reasons.

That said, it's worked "good enough" since I wrote the code nine years
ago, so if somebody wants to submit a patch to either get rid of the
if() statement or mark it as a FIXME that will likely never actually
get fixed, I wouldn't have an objection to either.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH] cx88: Get rid of spurious call to cx8800_start_vbi_dma()

[Devin Heitmueller]

This was left over from the conversion to VB2, where the call was
getting invoked both in buffer_queue and start_streaming, which
was intermittently causing invalid opcodes on the VBI RISC queue.

This change effectively mirrors the exact same change Hans Verkuil
made in cx88-video.c in 389208e1173e097590856ed24a505551510f78d4.

Thanks to Daniel Glöckner for spotting the actual bug after I spent
several days trying to chase down the issue.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
Thanks-to: Daniel Glöckner <daniel...@gmx.net>
Cc: Hans Verkuil <hverk...@xs4all.nl>
---
 drivers/media/pci/cx88/cx88-vbi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/pci/cx88/cx88-vbi.c 
b/drivers/media/pci/cx88/cx88-vbi.c
index c637679..58489ea 100644
--- a/drivers/media/pci/cx88/cx88-vbi.c
+++ b/drivers/media/pci/cx88/cx88-vbi.c
@@ -178,7 +178,6 @@ static void buffer_queue(struct vb2_buffer *vb)
 
if (list_empty(>active)) {
list_add_tail(>list, >active);
-   cx8800_start_vbi_dma(dev, q, buf);
dprintk(2, "[%p/%d] vbi_queue - first active\n",
buf, buf->vb.vb2_buf.index);
 
-- 
1.9.1

Re: cx88 invalid video opcodes when VBI enabled

[Devin Heitmueller]

Hi Daniel,

My apologies for the delayed replies; been out of town for the last
couple of days.

On Fri, Apr 20, 2018 at 5:54 PM, Daniel Glöckner  wrote:
> for some reason I feel like buffer_queue in cx88-vbi.c should not be
> calling cx8800_start_vbi_dma as it is also called a few lines further
> down in start_streaming.
>
> Devin, can you check if it helps to remove that line and if VBI still
> works afterwards?

So I've commented out that line in buffer_queue, and so far haven't
been able to reproduce the issue, and it does look like VBI is working
as expected (captions are being rendered in VLC).  This doesn't
suggest I've done exhaustive testing by any means, but it's certainly
a good sign.

I've seen drivers in the past which start the main data pump when
buffer_queue() or buffer_prepare() is called (whether it be to start a
DMA engine in the case of PCI or start URB submission in the case of
USB devices).  However it's not clear that's required, in particular
with VB2 which will automatically call start_streaming() in the case
where read() is used.  If I had to guess, I suspect the origin of
starting DMA that early was probably oriented around users who wanted
to simply run "cat /dev/video0 > out.mpeg" without having to
explicitly issue a bunch of V4L ioctl() calls beforehand.

It's worth noting that we're also doing it in the buffer_queue()
routine for video and not just VBI.  Presumably we would want to drop
both cases.

Hans, you did the VB2 conversion and have obviously been through this
exercise with a number of other drivers.  Any thoughts on whether we
can drop the starting of DMA engine in buffer_queue()?

On a related note, a quick review of the start/stop logic for DMA in
that driver suggests the calls might not be properly balanced.  Looks
like portions of the core logic are also duplicated between
stop_streaming() and stop_video_dma() (which is only ever used if
CONFIG_PM is defined).  It feels like it could probably use some
review/cleanup, although I'm loathed to touch such a mature driver for
fear of breaking something subtle.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: cx88 invalid video opcodes when VBI enabled

[Devin Heitmueller]

Hello Daniel,

See below.

Devin

[   68.750805] cx88[0]: irq vid [0x18080] vbi_risc2* vbi_sync opc_err*
[   68.750805] cx88[0]/0: video risc op code error
[   68.750809] cx88[0]: video y / packed - dma channel status dump
[   68.750811] cx88[0]:   cmds: initial risc: 0x8aa98000
[   68.750813] cx88[0]:   cmds: cdt base: 0x00180440
[   68.750815] cx88[0]:   cmds: cdt size: 0x000c
[   68.750816] cx88[0]:   cmds: iq base : 0x00180400
[   68.750818] cx88[0]:   cmds: iq size : 0x0010
[   68.750820] cx88[0]:   cmds: risc pc : 0x8aa1f03c
[   68.750821] cx88[0]:   cmds: iq wr ptr   : 0x010d
[   68.750823] cx88[0]:   cmds: iq rd ptr   : 0x0101
[   68.750825] cx88[0]:   cmds: cdt current : 0x0478
[   68.750827] cx88[0]:   cmds: pci target  : 0x8a9ed800
[   68.750828] cx88[0]:   cmds: line / byte : 0x0278
[   68.750832] cx88[0]:   risc0: 0x80008000 [ sync resync count=0 ]
[   68.750835] cx88[0]:   risc1: 0x1c000280 [ write sol eol count=640 ]
[   68.750837] cx88[0]:   risc2: 0x8ab2 [ arg #1 ]
[   68.750840] cx88[0]:   risc3: 0x1c000280 [ write sol eol count=640 ]
[   68.750842] cx88[0]:   iq 0: 0x80008000 [ sync resync count=0 ]
[   68.750845] cx88[0]:   iq 1: 0x1c000280 [ write sol eol count=640 ]
[   68.750847] cx88[0]:   iq 2: 0x8ab2 [ arg #1 ]
[   68.750850] cx88[0]:   iq 3: 0x1c000280 [ write sol eol count=640 ]
[   68.750852] cx88[0]:   iq 4: 0x8ab20500 [ arg #1 ]
[   68.750854] cx88[0]:   iq 5: 0x1c000280 [ write sol eol count=640 ]
[   68.750856] cx88[0]:   iq 6: 0x8ab20a00 [ arg #1 ]
[   68.750859] cx88[0]:   iq 7: 0x1c000280 [ write sol eol count=640 ]
[   68.750861] cx88[0]:   iq 8: 0x8ab20f00 [ arg #1 ]
[   68.750864] cx88[0]:   iq 9: 0x1c000280 [ write sol eol count=640 ]
[   68.750866] cx88[0]:   iq a: 0x8ab21400 [ arg #1 ]
[   68.750868] cx88[0]:   iq b: 0x1c000280 [ write sol eol count=640 ]
[   68.750870] cx88[0]:   iq c: 0x8ab21900 [ arg #1 ]
[   68.750873] cx88[0]:   iq d: 0x1c000280 [ write sol eol count=640 ]
[   68.750875] cx88[0]:   iq e: 0x8a9ed580 [ arg #1 ]
[   68.750877] cx88[0]:   iq f: 0x7001 [ jump cnt0 count=0 ]
[   68.750879] cx88[0]:   iq 10: 0x00180c00 [ arg #1 ]
[   68.750880] cx88[0]: fifo: 0x00180c00 -> 0x183400
[   68.750881] cx88[0]: ctrl: 0x00180400 -> 0x180460
[   68.750882] cx88[0]:   ptr1_reg: 0x00181380
[   68.750884] cx88[0]:   ptr2_reg: 0x00180478
[   68.750885] cx88[0]:   cnt1_reg: 0x
[   68.750887] cx88[0]:   cnt2_reg: 0x
[   68.750887] cx88[0]: vbi - dma channel status dump
[   68.750889] cx88[0]:   cmds: initial risc: 0x8a9cd000
[   68.750891] cx88[0]:   cmds: cdt base: 0x00180620
[   68.750892] cx88[0]:   cmds: cdt size: 0x0004
[   68.750894] cx88[0]:   cmds: iq base : 0x001805e0
[   68.750896] cx88[0]:   cmds: iq size : 0x0010
[   68.750897] cx88[0]:   cmds: risc pc : 0x8a9cd008
[   68.750899] cx88[0]:   cmds: iq wr ptr   : 0x017f
[   68.750901] cx88[0]:   cmds: iq rd ptr   : 0x0011
[   68.750902] cx88[0]:   cmds: cdt current : 0x0638
[   68.750904] cx88[0]:   cmds: pci target  : 0x05d9d242
[   68.750906] cx88[0]:   cmds: line / byte : 0x0003
[   68.750910] cx88[0]:   risc0: 0x8aa98000 [ sync sol irq2 23 21 19
cnt0 resync count=0 ]
[   68.750913] cx88[0]:   risc1: 0x00180440 [ INVALID 20 19 count=1088 ]
[   68.750915] cx88[0]:   risc2: 0x000c [ INVALID count=12 ]
[   68.750918] cx88[0]:   risc3: 0x00180400 [ INVALID 20 19 count=1024 ]
[   68.750920] cx88[0]:   iq 0: 0x1c000800 [ write sol eol count=2048 ]
[   68.750922] cx88[0]:   iq 1: 0x8ab4f800 [ arg #1 ]
[   68.750925] cx88[0]:   iq 2: 0x1c000800 [ write sol eol count=2048 ]
[   68.750927] cx88[0]:   iq 3: 0x8aa02000 [ arg #1 ]
[   68.750930] cx88[0]:   iq 4: 0x1c000800 [ write sol eol count=2048 ]
[   68.750932] cx88[0]:   iq 5: 0x8ab43000 [ arg #1 ]
[   68.750934] cx88[0]:   iq 6: 0x7001 [ jump cnt0 count=0 ]
[   68.750936] cx88[0]:   iq 7: 0x8ab43800 [ arg #1 ]
[   68.750939] cx88[0]:   iq 8: 0x1c000800 [ write sol eol count=2048 ]
[   68.750940] cx88[0]:   iq 9: 0x8ab48000 [ arg #1 ]
[   68.750943] cx88[0]:   iq a: 0x80008200 [ sync resync count=512 ]
[   68.750946] cx88[0]:   iq b: 0x1c000800 [ write sol eol count=2048 ]
[   68.750948] cx88[0]:   iq c: 0x8ab4c800 [ arg #1 ]
[   68.750951] cx88[0]:   iq d: 0x1c000800 [ write sol eol count=2048 ]
[   68.750952] cx88[0]:   iq e: 0x8ab4d000 [ arg #1 ]
[   68.750955] cx88[0]:   iq f: 0x1c000800 [ write sol eol count=2048 ]
[   68.750957] cx88[0]:   iq 10: 0x00184400 [ arg #1 ]
[   68.750958] cx88[0]: fifo: 0x00184400 -> 0x185400
[   68.750958] cx88[0]: ctrl: 0x001805e0 -> 0x180640
[   68.750960] cx88[0]:   ptr1_reg: 0x00184400
[   68.750961] cx88[0]:   ptr2_reg: 0x00180628
[   68.750963] cx88[0]:   cnt1_reg: 0x
[   68.750964] cx88[0]:   cnt2_reg: 0x

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: cx88 invalid video opcodes when VBI enabled

[Devin Heitmueller]

Hi Daniel,

Thanks for the feedback.

On Tue, Apr 17, 2018 at 12:53 AM, Daniel Glöckner  wrote:

>> [   54.427224] cx88[0]: irq vid [0x10088] vbi_risc1* vbi_risc2* opc_err*
>> [   54.427232] cx88[0]/0: video risc op code error
>> [   54.427238] cx88[0]: video y / packed - dma channel status dump
>
> Since the video IRQ status register has vbi_risc2 set, which we never
> generate with our RISC programs, I assume it is the VBI RISC engine
> that is executing garbage. So the dump of the video y/packed RISC engine
> does not help us here. Can you add a call to cx88_sram_channel_dump for
> SRAM_CH24 next to the existing one in cx8800_vid_irq?

Doh, I actually already did that a few days ago but didn't save the
log (and in fact, it was the VBI RISC queue that had garbage on it).
I'll dig up the log later today (or just add the line back in and
recreate it).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

cx88 invalid video opcodes when VBI enabled

[Devin Heitmueller]

Hello,

I received a report of a case where cx88 video capture was failing on
start and the dmesg was reporting an invalid opcode on the video IRQ
handler.  I did a bisect, and it looks like it's a result of the
videobuf2 conversion done back in late 2014.  A few notes:

1.  It only seems to occur if both video and VBI are being used.  I
cannot reproduce the issue when doing video only.

2.  It seems like it's some sort of order-of-operations issue when
interacting with the video/vbi device nodes, since it only happens
with the stock VLC and not tvtime.  It could also be that VLC uses
libzvbi, which access the VBI device in mmap mode, whereas tvtime
still uses read() for VBI access.

3.  The problem is readily reproducible on a stock Ubuntu 14.04
system, as well as with 16.04, using the stock VLC that ships with the
distro.  I'm testing with the following command line:

vlc v4l:///dev/video0 :v4l2-vbi-dev=/dev/vbi0

Sometimes it doesn't happen on the first hit and you have to run it a
few times, but I've never seen it take more than 5 executions for the
failure to occur.

4.  The problem occurs even when I blacklist the cx88-alsa driver.
This is worth noting since cx88-alsa has a different issue that
results in dumping out the RISC engine state, and I wanted to both
ensure the two issues weren't confused for each other, nor that the
ALSA interaction could be impacting the order of operations for
interacting with the driver.

Any suggestions on the best way to debug this without having to learn
the intimate details of the RISC engine on the cx88?  From the state
of the RISC engine it looks like there is some issue with queuing the
opcodes/arguments (where in some cases arguments are interpreted as
opcodes), but this is certainly not my area of expertise.

Thanks,

Devin

[   54.427224] cx88[0]: irq vid [0x10088] vbi_risc1* vbi_risc2* opc_err*
[   54.427232] cx88[0]/0: video risc op code error
[   54.427238] cx88[0]: video y / packed - dma channel status dump
[   54.427241] cx88[0]:   cmds: initial risc: 0x87cdb000
[   54.427244] cx88[0]:   cmds: cdt base: 0x00180440
[   54.427247] cx88[0]:   cmds: cdt size: 0x000c
[   54.427249] cx88[0]:   cmds: iq base : 0x00180400
[   54.427251] cx88[0]:   cmds: iq size : 0x0010
[   54.427253] cx88[0]:   cmds: risc pc : 0x87cdb03c
[   54.427256] cx88[0]:   cmds: iq wr ptr   : 0x010e
[   54.427258] cx88[0]:   cmds: iq rd ptr   : 0x0102
[   54.427260] cx88[0]:   cmds: cdt current : 0x0458
[   54.427263] cx88[0]:   cmds: pci target  : 0x
[   54.427265] cx88[0]:   cmds: line / byte : 0x
[   54.427267] cx88[0]:   risc0: 0x80008000 [ sync resync count=0 ]
[   54.427271] cx88[0]:   risc1: 0x1c000280 [ write sol eol count=640 ]
[   54.427276] cx88[0]:   risc2: 0x87dc [ arg #1 ]
[   54.427279] cx88[0]:   risc3: 0x1c000280 [ write sol eol count=640 ]
[   54.427283] cx88[0]:   iq 0: 0x7000 [ jump count=0 ]
[   54.427286] cx88[0]:   iq 1: 0x80008000 [ arg #1 ]
[   54.427289] cx88[0]:   iq 2: 0x1c000280 [ write sol eol count=640 ]
[   54.427293] cx88[0]:   iq 3: 0x87dc [ arg #1 ]
[   54.427295] cx88[0]:   iq 4: 0x1c000280 [ write sol eol count=640 ]
[   54.427300] cx88[0]:   iq 5: 0x87dc0500 [ arg #1 ]
[   54.427302] cx88[0]:   iq 6: 0x1c000280 [ write sol eol count=640 ]
[   54.427306] cx88[0]:   iq 7: 0x87dc0a00 [ arg #1 ]
[   54.427309] cx88[0]:   iq 8: 0x1c000280 [ write sol eol count=640 ]
[   54.427313] cx88[0]:   iq 9: 0x87dc0f00 [ arg #1 ]
[   54.427315] cx88[0]:   iq a: 0x1c000280 [ write sol eol count=640 ]
[   54.427319] cx88[0]:   iq b: 0x87dc1400 [ arg #1 ]
[   54.427321] cx88[0]:   iq c: 0x1c000280 [ write sol eol count=640 ]
[   54.427326] cx88[0]:   iq d: 0x87dc1900 [ arg #1 ]
[   54.427328] cx88[0]:   iq e: 0xd201 [ writecr irq2 count=1 ]
[   54.427332] cx88[0]:   iq f: 0x0031c040 [ arg #1 ]
[   54.427334] cx88[0]:   iq 10: 0x00180c00 [ arg #2 ]
[   54.427337] cx88[0]:   iq 11: 0x6f60580c [ arg #3 ]
[   54.427339] cx88[0]: fifo: 0x00180c00 -> 0x183400
[   54.427340] cx88[0]: ctrl: 0x00180400 -> 0x180460
[   54.427342] cx88[0]:   ptr1_reg: 0x00180eb8
[   54.427344] cx88[0]:   ptr2_reg: 0x00180458
[   54.427347] cx88[0]:   cnt1_reg: 0x0007
[   54.427349] cx88[0]:   cnt2_reg: 0x


-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH RFC] media: em28xx: don't use coherent buffer for DMA transfers

[Devin Heitmueller]

On Tue, Feb 27, 2018 at 12:42 PM, Mauro Carvalho Chehab
 wrote:
> While coherent memory is cheap on x86, it has problems on
> arm. So, stop using it.
>
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>
> I wrote this patch in order to check if this would make things better
> for ISOCH transfers on Raspberry Pi3. It didn't. Yet, keep using
> coherent memory at USB drivers seem an overkill.
>
> So, I'm actually not sure if we should either go ahead and merge it
> or not.
>
> Comments? Tests?

For what it's worth, while I haven't tested this patch you're
proposing, I've been running what is essentially the same change in a
private tree for several years in order for the device to work better
with several TI Davinci SOC platforms.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: pinnacle 300i driver crashed after first device access

[Devin Heitmueller]

On Fri, Feb 23, 2018 at 2:37 PM, Devin Heitmueller
<dheitmuel...@kernellabs.com> wrote:
> On Fri, Feb 23, 2018 at 2:30 PM, Federico Allegretti
> <allegf...@gmail.com> wrote:
>> i noticed that my pinnacle 300i could accept full resolution settings:
>> v4l2-ctl --set-fmt-video=width=720,height=576
>>
>> only the first time the command is fired.
>>
>> after that, evey time i try to set that resolution with the same
>> command, i get instead only the half vertical resolution:
>> v4l2-ctl --get-fmt-video
>> Format Video Capture:
>> Width/Height  : 720/288
>> Pixel Format  : 'YU12'
>> Field : Bottom
>> Bytes per Line: 720
>> Size Image: 311040
>> Colorspace: SMPTE 170M
>> Transfer Function : Default
>> YCbCr/HSV Encoding: Default
>> Quantization  : Default
>> Flags :

Also, looks like the field format is set to bottom, which would
explain why you're only getting half the lines.  You probably need to
set the field type to interlaced in your --set-fmt-video arguments
(although I don't have the exact syntax right in front of me).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: pinnacle 300i driver crashed after first device access

[Devin Heitmueller]

On Fri, Feb 23, 2018 at 2:30 PM, Federico Allegretti
 wrote:
> i noticed that my pinnacle 300i could accept full resolution settings:
> v4l2-ctl --set-fmt-video=width=720,height=576
>
> only the first time the command is fired.
>
> after that, evey time i try to set that resolution with the same
> command, i get instead only the half vertical resolution:
> v4l2-ctl --get-fmt-video
> Format Video Capture:
> Width/Height  : 720/288
> Pixel Format  : 'YU12'
> Field : Bottom
> Bytes per Line: 720
> Size Image: 311040
> Colorspace: SMPTE 170M
> Transfer Function : Default
> YCbCr/HSV Encoding: Default
> Quantization  : Default
> Flags :

Did you set the video standard?  All sorts of bad things could happen
if you set the format to 720x576 but the standard is still set to
NTSC.

To get the standards supported, you can run:

v4l2-ctl --list-standards

And then set the standard with "v4l2-ctl -s".  Do this before setting
the format.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: Bug: Two device nodes created in /dev for a single UVC webcam

[Devin Heitmueller]

Hi Laurent,

On Mon, Feb 19, 2018 at 11:19 AM, Laurent Pinchart
 wrote:
> I've tested VLC (2.2.8) and haven't noticed any issue. If a program is
> directed to the metadata video node and tries to capture video from it it will
> obviously fail. That being said, software that work today should continue
> working, otherwise it's a regression, and we'll have to handle that.

Perhaps it shouldn't be a video node then (as we do with VBI devices).
Would something like /dev/videometadataX would be more appropriate?

People have for years operated under the expectation that /dev/videoX
nodes are video nodes.  If we're going to be creating things with that
name which aren't video nodes then that is going to cause considerable
confusion as well as messing up all sorts of existing applications
which operate under that expectation.

I know that some of the older PCI boards have always exposed a bunch
of video nodes for various things (i.e. raw video vs. mpeg, etc), but
because USB devices have traditionally been simpler they generally
expose only one node of each type (i.e. one /dev/videoX, /dev/vbiX
/dev/radioX).  I've already gotten an email from a customer who has a
ton of scripts which depend on this behavior, so please seriously
consider the implications of this design decision.

It's easy to brush this off as "all the existing applications will
eventually be updated", but you're talking about changing the basic
behavior of how these device nodes have been presented for over a
decade.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/1] vb2: core: Finish buffers at the end of the stream

[Devin Heitmueller]

Hi Sakari,

I tried this patch, and I no longer see the messages in dmesg output
when closing the V4L2 device node.

Tested-by: Devin Heitmueller <dheitmuel...@kernellabs.com>

Thanks,

Devin

On Fri, Feb 2, 2018 at 8:57 AM, Devin Heitmueller
<dheitmuel...@kernellabs.com> wrote:
> Hello Sakari,
>
> Thanks for proposing this patch.  I'll give it a try this weekend.
>
> Regards,
>
> Devin
>
> On Fri, Feb 2, 2018 at 5:08 AM, Sakari Ailus
> <sakari.ai...@linux.intel.com> wrote:
>> If buffers were prepared or queued and the buffers were released without
>> starting the queue, the finish mem op (corresponding to the prepare mem
>> op) was never called to the buffers.
>>
>> Before commit a136f59c0a1f there was no need to do this as in such a case
>> the prepare mem op had not been called yet. Address the problem by
>> explicitly calling finish mem op when the queue is stopped if the buffer
>> is in either prepared or queued state.
>>
>> Fixes: a136f59c0a1f ("[media] vb2: Move buffer cache synchronisation to 
>> prepare from queue")
>> Cc: sta...@vger.kernel.org # for v4.13 and up
>> Signed-off-by: Sakari Ailus <sakari.ai...@linux.intel.com>
>> ---
>> Hi Devin,
>>
>> Could you check whether this will resolve the problem you've found?
>>
>> Thanks.
>>
>>  drivers/media/common/videobuf2/videobuf2-core.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index f7109f827f6e..52a7c1d0a79a 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1696,6 +1696,15 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>> for (i = 0; i < q->num_buffers; ++i) {
>> struct vb2_buffer *vb = q->bufs[i];
>>
>> +   if (vb->state == VB2_BUF_STATE_PREPARED ||
>> +   vb->state == VB2_BUF_STATE_QUEUED) {
>> +   unsigned int plane;
>> +
>> +   for (plane = 0; plane < vb->num_planes; ++plane)
>> +   call_void_memop(vb, finish,
>> +   vb->planes[plane].mem_priv);
>> +   }
>> +
>> if (vb->state != VB2_BUF_STATE_DEQUEUED) {
>> vb->state = VB2_BUF_STATE_PREPARED;
>> call_void_vb_qop(vb, buf_finish, vb);
>> --
>> 2.11.0
>>
>
>
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com



-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/1] vb2: core: Finish buffers at the end of the stream

[Devin Heitmueller]

Hello Sakari,

Thanks for proposing this patch.  I'll give it a try this weekend.

Regards,

Devin

On Fri, Feb 2, 2018 at 5:08 AM, Sakari Ailus
 wrote:
> If buffers were prepared or queued and the buffers were released without
> starting the queue, the finish mem op (corresponding to the prepare mem
> op) was never called to the buffers.
>
> Before commit a136f59c0a1f there was no need to do this as in such a case
> the prepare mem op had not been called yet. Address the problem by
> explicitly calling finish mem op when the queue is stopped if the buffer
> is in either prepared or queued state.
>
> Fixes: a136f59c0a1f ("[media] vb2: Move buffer cache synchronisation to 
> prepare from queue")
> Cc: sta...@vger.kernel.org # for v4.13 and up
> Signed-off-by: Sakari Ailus 
> ---
> Hi Devin,
>
> Could you check whether this will resolve the problem you've found?
>
> Thanks.
>
>  drivers/media/common/videobuf2/videobuf2-core.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c 
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index f7109f827f6e..52a7c1d0a79a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1696,6 +1696,15 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> for (i = 0; i < q->num_buffers; ++i) {
> struct vb2_buffer *vb = q->bufs[i];
>
> +   if (vb->state == VB2_BUF_STATE_PREPARED ||
> +   vb->state == VB2_BUF_STATE_QUEUED) {
> +   unsigned int plane;
> +
> +   for (plane = 0; plane < vb->num_planes; ++plane)
> +   call_void_memop(vb, finish,
> +   vb->planes[plane].mem_priv);
> +   }
> +
> if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> vb->state = VB2_BUF_STATE_PREPARED;
> call_void_vb_qop(vb, buf_finish, vb);
> --
> 2.11.0
>



-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: Regression in VB2 alloc prepare/finish balancing with em28xx/au0828

[Devin Heitmueller]

Hi Sakari, Hans,

Do either of you have any thoughts on whether I'm actually leaking any
resources, or whether this is just a warning that doesn't have any
practical implication since I'm tearing down the videobuf2 queue?

I don't really care about the embedded use case - do you see any
reason where at least for my local tree I cannot simply revert this
patch until a real solution is found?

Cheers,

Devin

On Mon, Jan 29, 2018 at 8:44 PM, Devin Heitmueller
<dheitmuel...@kernellabs.com> wrote:
> Hello Sakari,
>
> Thanks for taking the time to investigate.  See comments inline.
>
> On Sun, Jan 28, 2018 at 5:23 PM, Sakari Ailus
> <sakari.ai...@linux.intel.com> wrote:
>> Hi Devin,
>>
>> On Sun, Jan 28, 2018 at 09:12:44AM -0500, Devin Heitmueller wrote:
>>> Hello all,
>>>
>>> I recently updated to the latest kernel, and I am seeing the following
>>> dumped to dmesg with both au0828 and em28xx based devices whenever I
>>> exit tvtime (my kernel is compiled with CONFIG_VIDEO_ADV_DEBUG=y by
>>> default):
>>
>> Thanks for reporting this. Would you be able to provide the full dmesg,
>> with VB2 debug parameter set to 2?
>
> Output can be found at https://pastebin.com/nXS7MTJH
>
>> I can't immediately see how you'd get this, well, without triggering a
>> kernel warning or two. The code is pretty complex though.
>
> If this is something I screwed up when I did the VB2 port for em28xx
> several years ago, point me in the right direction and I'll see what I
> can do.  However given we're seeing it with multiple drivers, this
> feels like some subtle issue inside videobuf2.
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com



-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: Regression in VB2 alloc prepare/finish balancing with em28xx/au0828

[Devin Heitmueller]

Hello Sakari,

Thanks for taking the time to investigate.  See comments inline.

On Sun, Jan 28, 2018 at 5:23 PM, Sakari Ailus
<sakari.ai...@linux.intel.com> wrote:
> Hi Devin,
>
> On Sun, Jan 28, 2018 at 09:12:44AM -0500, Devin Heitmueller wrote:
>> Hello all,
>>
>> I recently updated to the latest kernel, and I am seeing the following
>> dumped to dmesg with both au0828 and em28xx based devices whenever I
>> exit tvtime (my kernel is compiled with CONFIG_VIDEO_ADV_DEBUG=y by
>> default):
>
> Thanks for reporting this. Would you be able to provide the full dmesg,
> with VB2 debug parameter set to 2?

Output can be found at https://pastebin.com/nXS7MTJH

> I can't immediately see how you'd get this, well, without triggering a
> kernel warning or two. The code is pretty complex though.

If this is something I screwed up when I did the VB2 port for em28xx
several years ago, point me in the right direction and I'll see what I
can do.  However given we're seeing it with multiple drivers, this
feels like some subtle issue inside videobuf2.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Regression in VB2 alloc prepare/finish balancing with em28xx/au0828

[Devin Heitmueller]

Hello all,

I recently updated to the latest kernel, and I am seeing the following
dumped to dmesg with both au0828 and em28xx based devices whenever I
exit tvtime (my kernel is compiled with CONFIG_VIDEO_ADV_DEBUG=y by
default):

[  129.219666] vb2: counters for queue 88026463ac48:
[  129.219667] vb2: setup: 1 start_streaming: 2 stop_streaming: 2
[  129.219667] vb2: wait_prepare: 0 wait_finish: 0
[  129.219668] vb2:   counters for queue 88026463ac48, buffer 0: UNBALANCED!
[  129.219669] vb2: buf_init: 1 buf_cleanup: 1 buf_prepare: 14
buf_finish: 14
[  129.219669] vb2: buf_queue: 13 buf_done: 13
[  129.219673] vb2: alloc: 1 put: 1 prepare: 14 finish: 13 mmap: 1
[  129.219674] vb2: get_userptr: 0 put_userptr: 0
[  129.219674] vb2: attach_dmabuf: 0 detach_dmabuf: 0 map_dmabuf:
0 unmap_dmabuf: 0
[  129.219675] vb2: get_dmabuf: 0 num_users: 0 vaddr: 13 cookie: 0
[  129.219676] vb2:   counters for queue 88026463ac48, buffer 1: UNBALANCED!
[  129.219676] vb2: buf_init: 1 buf_cleanup: 1 buf_prepare: 13
buf_finish: 13
[  129.219677] vb2: buf_queue: 12 buf_done: 12
[  129.219678] vb2: alloc: 1 put: 1 prepare: 13 finish: 12 mmap: 1
[  129.219678] vb2: get_userptr: 0 put_userptr: 0
[  129.219679] vb2: attach_dmabuf: 0 detach_dmabuf: 0 map_dmabuf:
0 unmap_dmabuf: 0
[  129.219679] vb2: get_dmabuf: 0 num_users: 0 vaddr: 12 cookie: 0
[  129.219680] vb2:   counters for queue 88026463ac48, buffer 2: UNBALANCED!
[  129.219680] vb2: buf_init: 1 buf_cleanup: 1 buf_prepare: 13
buf_finish: 13
[  129.219681] vb2: buf_queue: 12 buf_done: 12
[  129.219682] vb2: alloc: 1 put: 1 prepare: 13 finish: 12 mmap: 1
[  129.219682] vb2: get_userptr: 0 put_userptr: 0
[  129.219683] vb2: attach_dmabuf: 0 detach_dmabuf: 0 map_dmabuf:
0 unmap_dmabuf: 0
[  129.219683] vb2: get_dmabuf: 0 num_users: 0 vaddr: 12 cookie: 0
[  129.219684] vb2:   counters for queue 88026463ac48, buffer 3: UNBALANCED!
[  129.219684] vb2: buf_init: 1 buf_cleanup: 1 buf_prepare: 13
buf_finish: 13
[  129.219685] vb2: buf_queue: 12 buf_done: 12
[  129.219686] vb2: alloc: 1 put: 1 prepare: 13 finish: 12 mmap: 1
[  129.219686] vb2: get_userptr: 0 put_userptr: 0
[  129.219687] vb2: attach_dmabuf: 0 detach_dmabuf: 0 map_dmabuf:
0 unmap_dmabuf: 0
[  129.219687] vb2: get_dmabuf: 0 num_users: 0 vaddr: 12 cookie: 0

The above suggests that the prepare/finish calls are unbalanced.  I
added a bit of debug and identified that it only occurs with the video
node; it's not happening with the VBI node (which when using tvtime
makes use of the read() emulation done by videobuf2).

I went through a git bisect, and came up with the following patch
which introduced the issue:

commit a136f59c0a1f1b09eb203951975e3fc5e8d3e722
Author: Sakari Ailus 
Date:   Wed May 31 11:17:26 2017 -0300

[media] vb2: Move buffer cache synchronisation to prepare from queue

The buffer cache should be synchronised in buffer preparation, not when
the buffer is queued to the device. Fix this.

Mmap buffers do not need cache synchronisation since they are always
coherent.

Signed-off-by: Sakari Ailus 
Acked-by: Hans Verkuil 
Signed-off-by: Mauro Carvalho Chehab 

It's not clear to me whether this is really a regression in videobuf2,
or if it's a change in behavior that exposes some improper handling in
em28xx/au0828 when the device node is closed.

To reproduce:

1.  compile kernel with CONFIG_VIDEO_ADV_DEBUG=y
2.  Plug in au0828 or em28xx device (reproduced with HVR-950 and HVR-950q)
3.  start tvtime
4.  exit tvtime

I don't claim to be videobuf2 expert, so any advice that could be
offered with regards to a fix (either in videobuf2 or in au0828) would
certainly be welcome.

Thanks in advance,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 2/9] em28xx: Bulk transfer implementation fix

[Devin Heitmueller]

Hi Brad,

My documents indicate that Register 0x5D and 0x5E are read-only, and
populated based on the eeprom programming.

On your device, what is the value of those registers prior to you changing them?

If you write to those registers, do they reflect the new values if you
read them back?

Does changing these values result in any change to the device's
endpoint configuration (which is typically statically defined when the
device is probed)?

What precisely is the behavior you were seeing prior to this patch?

Devin

On Thu, Jan 4, 2018 at 7:22 PM, Michael Ira Krufky  wrote:
> On Thu, Jan 4, 2018 at 7:04 PM, Brad Love  wrote:
>> Set appropriate bulk/ISOC transfer multiplier on capture start.
>> This sets ISOC transfer to 940 bytes (188 * 5)
>> This sets bulk transfer to 48128 bytes (188 * 256)
>>
>> The above values are maximum allowed according to Empia.
>>
>> Signed-off-by: Brad Love 
>
> :+1
>
> Reviewed-by: Michael Ira Krufky 
>
>> ---
>>  drivers/media/usb/em28xx/em28xx-core.c | 12 
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c 
>> b/drivers/media/usb/em28xx/em28xx-core.c
>> index ef38e56..67ed6a3 100644
>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>> @@ -638,6 +638,18 @@ int em28xx_capture_start(struct em28xx *dev, int start)
>> dev->chip_id == CHIP_ID_EM28174 ||
>> dev->chip_id == CHIP_ID_EM28178) {
>> /* The Transport Stream Enable Register moved in em2874 */
>> +   if (dev->dvb_xfer_bulk) {
>> +   /* Max Tx Size = 188 * 256 = 48128 - LCM(188,512) * 
>> 2 */
>> +   em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>> +   EM2874_R5D_TS1_PKT_SIZE :
>> +   EM2874_R5E_TS2_PKT_SIZE,
>> +   0xFF);
>> +   } else {
>> +   /* TS2 Maximum Transfer Size = 188 * 5 */
>> +   em28xx_write_reg(dev, (dev->ts == PRIMARY_TS) ?
>> +   EM2874_R5D_TS1_PKT_SIZE :
>> +   EM2874_R5E_TS2_PKT_SIZE, 0x05);
>> +   }
>> if (dev->ts == PRIMARY_TS)
>> rc = em28xx_write_reg_bits(dev,
>> EM2874_R5F_TS_ENABLE,
>> --
>> 2.7.4
>>



-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [bug report] drx: add initial drx-d driver

[Devin Heitmueller]

> I think I wrote the driver more than 10 years ago and somebody later 
> submitted it
> to the kernel.

I'm pretty sure that was me, or perhaps I was the first person to get
it to work with a device upstream - it was so long ago.

> I don't know if there is a anybody still maintaining this. Is it even used 
> anymore?
> I could write a patch but cannot test it (e.g. to see if it really always
> loops 1000 times ...)

Pretty sure the register was a status register that had bits set for
pending writes, and when the transfer was complete all the bits would
be zero (at which point it should stop polling the register and bail
out).

I would have to test it to be sure, but I'm 80% confident that in
normal operation that loop only does a few iterations.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: cx231xx IR remote control protocol

[Devin Heitmueller]

Hi Oleh,

On Mon, Oct 23, 2017 at 5:37 AM, Sean Young  wrote:
> Hi Oleh,
>
> On Sun, Oct 22, 2017 at 05:01:05PM +0300, Oleh Kravchenko wrote:
>> Hi,
>>
>> I'm trying add support remote control for tuners:
>> - EvroMedia Full Hybrid Full HD
>> - Astrometa T2hybrid
>>
>> But I'm stuck. Can anybody recognize this protocol?
>

Is there anything to indicate that there actually a separate IR
receiver on the board?  Most cx231xx devices have an MCEUSB compliant
IR RX/TX built into the chip, and thus they don't use a separate part.

Also, IIRC, address 0x30 is the I2C address of the onboard analog
frontend for the cx23102, and I suspect you're just reading the values
out of the AFE.  I suspect you've grabbed a dump from the initial
device plug-in, which would coincide with the initial register
programming of the AFE.

I suspect if you connect the device, let it device settle, *then*
start the I2C capture and hit a key on the remote, you'll see traffic
on a totally different endpoint which would be the IR traffic being
sent back over the IR interface/endpoint.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH v3 04/26] media: lirc_zilog: remove receiver

[Devin Heitmueller]

Hi Andy,

> 5. Rx and IR Learn both use the same external hardware.  Not
> coordinating Rx with Learn mode in the same driver, will prevent Learn
> operation from working.  That is, if Learn mode is ever implemented.
> (Once upon a time, I was planning on doing that.  But I have no time
> for that anymore.)

There's not really any infrastructure in Linux that maps to the
Zilog's "learning mode" functionality.  Usually I would just tell
users to do the learning under Windows and send me the resulting .ini
file (which we could then add to the database).

I had planned on getting rid of the database entirely and just
converting an MCE compatible pulse train to the blasting format
required by the Zilog firmware (using the awesome work you sent me
privately), but the fact of the matter is that nobody cares and MCEUSB
devices are $20 online.

> I'm glad someone remembers all this stuff.  I'm assuming you had more
> pain with this than I ever did.

This would be a safe assumption.  I probably put about a month's worth
of engineering into driver work for the Zilog, which seems
extraordinary given how simple something like an IR blaster/receiver
is supposed to be.  I guess that's the fun of proving out a new
hardware design as opposed to just making something work under Linux
that is already known to work under Windows.

> I never owned an HD-PVR.

I'm sure I have a spare or two if you really want one (not that you
have the time to muck with such things nowadays).  :-)

The HD-PVR was a bit of a weird case compared to devices like ivtv and
cx18 because it was technically multi-master (I2C commands came both
from the host and from the onboard SOC).  Hence you could have weird
cases where one would block the other at unexpected times.  I2C
commands to the Zilog would hold the bus which would delay the onboard
firmware from issuing commands to the video decoder (fun timing
issues).  There was also some weird edge case I don't recall the
details of that prompted them to add an I2C gate in later board
revisions.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH v3 04/26] media: lirc_zilog: remove receiver

[Devin Heitmueller]

> There's an ir_lock mutex in the driver to prevent simultaneous access to the 
> Rx and Tx functions of the z8.  Accessing Rx and Tx functions of the chip 
> together can cause it to do the wrong thing (sometimes hang?), IIRC.
>
> I'll see if I can dig up my old disassembly of the z8's firmware to see if 
> that interlock is strictly necessary.
>
> Yes I know ir-kbd-i2c is in mainline, but as I said, I had reasons for 
> avoiding it when using Tx operation of the chip.  It's been 7 years, and I'm 
> getting too old to remember the reasons. :P

Yeah, you definitely don't want to be issuing requests to the Rx and
Tx addresses at the same time.  Very bad things will happen.

Also, what is the polling interval for ir-kbd-i2c?  If it's too high
you can wedge the I2C bus (depending on the hardware design).
Likewise, many people disable IR RX entirely (which is trivial to do
with lirc_zilog through a modprobe optoin).  This is because early
versions of the HDPVR had issues where polling too often can interfere
with traffic that configures the component receiver chip.  This was
improved in later versions of the design, but many people found it was
just easiest to disable RX since they don't use it (i.e. they would
use the blaster for controlling their STB but use a separate IR
receiver).

Are you testing with video capture actively in use?  If you're testing
the IR interface without an active HD capture in progress you're
likely to miss some of these edge cases (in particular those which
would hang the encoder).

I'm not against the removal of duplicate code in general, but you have
to tread carefully because there are plenty of non-obvious edge cases
to consider.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] Support HVR-1200 analog video as a clone of HVR-1500. Tested, composite and s-video inputs.

[Devin Heitmueller]

On Sun, Sep 17, 2017 at 5:42 PM, Nigel Kettlewell
 wrote:
> I propose the following patch to support Hauppauge HVR-1200 analog video,
> nothing more than a clone of HVR-1500. Patch based on Linux 4.9 commit
> 69973b830859bc6529a7a0468ba0d80ee5117826
>
> I have tested composite and S-Video inputs.
>
> With the change, HVR-1200 devices have a /dev/video entry which is
> accessible in the normal way.
>
> Let me know if you need anything more.

I'm not confident the tuner config for this board is correct.  The
HVR-1200 is much closer to the HVR-1250 as opposed to the HVR-1500,
and IIRC it didn't have an xc3028.

I don't dispute that with the patch in question the composite/s-video
are probably working ok, but I wouldn't recommend accepting this patch
as-is until the tuner is verified for DVB-T and analog (ideally both).

Can you provide the output of dmesg on device load?  If it's filled
with a bunch of errors showing xc3028 firmware load failures, that
would be a smoking gun that it doesn't have the xc3028.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 08/12] Add USB quirk for HVR-950q to avoid intermittent device resets

[Devin Heitmueller]

> I've accepted the other patches in this patch series for the media subsystem,
> but this patch should go through the USB subsystem. Cc-ed linux-usb.
>
> Acked-by: Hans Verkuil 



I'm not sure who on the linux-usb mailing list would need to deal with
this, but would be great if we could get this merged.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 1/2] em28xx: allow setting the eeprom bus at cards struct

[Devin Heitmueller]

On Mon, May 1, 2017 at 10:11 AM, Frank Schäfer
 wrote:
>
> Am 01.05.2017 um 13:38 schrieb Mauro Carvalho Chehab:
>> Right now, all devices use bus 0 for eeprom. However, newer
>> versions of Terratec H6 use a different buffer for eeprom.
>>
>> So, add support to use a different I2C address for eeprom.
>
> Has this been tested ?
> Did you read my reply to the previous patch version ?:
> See http://www.spinics.net/lists/linux-media/msg114860.html
>
> I doubt it will work. At least not for the device from the thread in the
> Kodi-forum.

Based on what I know about the Empia 2874/2884 design, I would be
absolutely shocked if the eeprom was really on the second I2C bus.
The boot code in ROM requires the eeprom to be on bus 0 in order to
find the 8051 microcode to be executed.  This is a documented hardware
design requirement.

I have seen designs where the first bus is accessible through an I2C
gate on a demodulator on the second bus.  This creates a multi-master
situation and I have no idea why anyone would ever do this.  However
it does explain a situation where the EEPROM could be optionally
accessed via the second bus (if the I2C gate on the demod was open at
the time).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH] mxl111sf: Fix driver to use heap allocate buffers for USB messages

[Devin Heitmueller]

The recent changes in 4.9 to mandate USB buffers be heap allocated
broke this driver, which was allocating the buffers on the stack.
This resulted in the device failing at initialization.

Introduce dedicated send/receive buffers as part of the state
structure, and add a mutex to protect access to them.

Note: we also had to tweak the API to mxl111sf_ctrl_msg to pass
the pointer to the state struct rather than the device, since
we need it inside the function to access the buffers and the
mutex.  This patch adjusts the callers to match the API change.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
Reported-by: Doug Lung <dlu...@gmail.com>
Cc: Michael Ira Krufky <mkru...@linuxtv.org>
---
 drivers/media/usb/dvb-usb-v2/mxl111sf-i2c.c |  4 ++--
 drivers/media/usb/dvb-usb-v2/mxl111sf.c | 32 +
 drivers/media/usb/dvb-usb-v2/mxl111sf.h |  8 +++-
 3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf-i2c.c 
b/drivers/media/usb/dvb-usb-v2/mxl111sf-i2c.c
index ffb49c2..0eb33e04 100644
--- a/drivers/media/usb/dvb-usb-v2/mxl111sf-i2c.c
+++ b/drivers/media/usb/dvb-usb-v2/mxl111sf-i2c.c
@@ -316,7 +316,7 @@ static int mxl111sf_i2c_sw_xfer_msg(struct mxl111sf_state 
*state,
 static int mxl111sf_i2c_send_data(struct mxl111sf_state *state,
  u8 index, u8 *wdata)
 {
-   int ret = mxl111sf_ctrl_msg(state->d, wdata[0],
+   int ret = mxl111sf_ctrl_msg(state, wdata[0],
[1], 25, NULL, 0);
mxl_fail(ret);
 
@@ -326,7 +326,7 @@ static int mxl111sf_i2c_send_data(struct mxl111sf_state 
*state,
 static int mxl111sf_i2c_get_data(struct mxl111sf_state *state,
 u8 index, u8 *wdata, u8 *rdata)
 {
-   int ret = mxl111sf_ctrl_msg(state->d, wdata[0],
+   int ret = mxl111sf_ctrl_msg(state, wdata[0],
[1], 25, rdata, 24);
mxl_fail(ret);
 
diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf.c 
b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
index abf69d8..b0d5904 100644
--- a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
+++ b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
@@ -24,9 +24,6 @@
 #include "lgdt3305.h"
 #include "lg2160.h"
 
-/* Max transfer size done by I2C transfer functions */
-#define MAX_XFER_SIZE  64
-
 int dvb_usb_mxl111sf_debug;
 module_param_named(debug, dvb_usb_mxl111sf_debug, int, 0644);
 MODULE_PARM_DESC(debug, "set debugging level (1=info, 2=xfer, 4=i2c, 8=reg, 
16=adv (or-able)).");
@@ -55,27 +52,34 @@
 
 DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 
-int mxl111sf_ctrl_msg(struct dvb_usb_device *d,
+int mxl111sf_ctrl_msg(struct mxl111sf_state *state,
  u8 cmd, u8 *wbuf, int wlen, u8 *rbuf, int rlen)
 {
+   struct dvb_usb_device *d = state->d;
int wo = (rbuf == NULL || rlen == 0); /* write-only */
int ret;
-   u8 sndbuf[MAX_XFER_SIZE];
 
-   if (1 + wlen > sizeof(sndbuf)) {
+   if (1 + wlen > MXL_MAX_XFER_SIZE) {
pr_warn("%s: len=%d is too big!\n", __func__, wlen);
return -EOPNOTSUPP;
}
 
pr_debug("%s(wlen = %d, rlen = %d)\n", __func__, wlen, rlen);
 
-   memset(sndbuf, 0, 1+wlen);
+   mutex_lock(>msg_lock);
+   memset(state->sndbuf, 0, 1+wlen);
+   memset(state->rcvbuf, 0, rlen);
+
+   state->sndbuf[0] = cmd;
+   memcpy(>sndbuf[1], wbuf, wlen);
 
-   sndbuf[0] = cmd;
-   memcpy([1], wbuf, wlen);
+   ret = (wo) ? dvb_usbv2_generic_write(d, state->sndbuf, 1+wlen) :
+   dvb_usbv2_generic_rw(d, state->sndbuf, 1+wlen, state->rcvbuf,
+rlen);
+
+   memcpy(rbuf, state->rcvbuf, rlen);
+   mutex_unlock(>msg_lock);
 
-   ret = (wo) ? dvb_usbv2_generic_write(d, sndbuf, 1+wlen) :
-   dvb_usbv2_generic_rw(d, sndbuf, 1+wlen, rbuf, rlen);
mxl_fail(ret);
 
return ret;
@@ -91,7 +95,7 @@ int mxl111sf_read_reg(struct mxl111sf_state *state, u8 addr, 
u8 *data)
u8 buf[2];
int ret;
 
-   ret = mxl111sf_ctrl_msg(state->d, MXL_CMD_REG_READ, , 1, buf, 2);
+   ret = mxl111sf_ctrl_msg(state, MXL_CMD_REG_READ, , 1, buf, 2);
if (mxl_fail(ret)) {
mxl_debug("error reading reg: 0x%02x", addr);
goto fail;
@@ -117,7 +121,7 @@ int mxl111sf_write_reg(struct mxl111sf_state *state, u8 
addr, u8 data)
 
pr_debug("W: (0x%02x, 0x%02x)\n", addr, data);
 
-   ret = mxl111sf_ctrl_msg(state->d, MXL_CMD_REG_WRITE, buf, 2, NULL, 0);
+   ret = mxl111sf_ctrl_msg(state, MXL_CMD_REG_WRITE, buf, 2, NULL, 0);
if (mxl_fail(ret))
pr_err("error writing reg: 0x%02x, val: 0x%02x", addr, data);
return ret;
@@ -926,6 +930,8 @@ static int mxl111

Re: RFC: Power states and VIDIOC_STREAMON

[Devin Heitmueller]

>> I had always interpreted it such that the STREAMON call just
>> controlled whether the DMA engine was running, and thus you could do
>> anything else with the decoder before calling STREAMON other than
>> actually receiving video buffers.
>
> Indeed there's an ambiguity there, although I always read that
> the device's logic should keep accepting calls via both DVB
> and V4L2 APIs until V4L2 streaming ioctls are issued.

Yeah, the hybrid use case is clearly something that didn't exist back
then.  Most of the video decoders I've worked on (e.g. tvp5150,
saa7115) have the device running all the time and s_stream is only
used for output control (i.e. enable/disable the ITU-656 output).

> That's, btw, what happens on older drivers like cx88 and bttv.
>
> For example, on bttv, there's this logic:
>
> static int bttv_s_fmt_vid_cap(struct file *file, void *priv,
> struct v4l2_format *f)
> {
> int retval;
> const struct bttv_format *fmt;
> struct bttv_fh *fh = priv;
> struct bttv *btv = fh->btv;
> __s32 width, height;
> unsigned int width_mask, width_bias;
> enum v4l2_field field;
>
> retval = bttv_switch_type(fh, f->type);
> if (0 != retval)
> return retval;
>
> The logic there actually happens earlier, at VIDIOC_S_FMT. Although I
> guess all apps call it before streaming, the problem with the above is
> that the V4L2 API doesn't actually make it mandatory to call this ioctl
> before start streaming.
>
> I guess that the idea of doing that at STREAMON started when we
> discussed how to lock hybrid devices via MC. I guess it was suggested
> by Shuah, who looked on those issues and analyzed what apps used to do.

I've got a pile of changes which involve refactoring the power
management on the au8522, but I have never thoroughly reviewed where
Shuah ended up with the MC changes and thus it's likely they don't
take those into account.

>> My instinct would be to revert the patch in question since it breaks
>> ABI behavior which has been present for over a decade, but I suspect
>> such a patch would be rejected since it was Mauro himself who
>> introduced the change in behavior.
>
> It doesn't matter who committed a patch. If it is wrong, something
> should be done.
>
> However, in the specific case of a change like that, just reverting the
> patch right now would make it worse, as it will break the resource locks
> between FM, analog TV and digital TV, causing regressions.
>
> Locking it at tuner get status is a bad place, as I guess that would
> break locking between FM radio and analog TV, as both can read
> tuner status.

For what it's worth, FM radio isn't supported in the au0828/au8522
driver, so that doesn't need to be a consideration for this particular
driver unless you're suggesting some changes to the framework common
to all devices.

> Maybe one solution would be to lock the resources on either
> for VIDIOC_S_FMT, VIDIOC_STREAMON or read() (whatever comes first),
> but we need to check if this won't break switching between analog TV
> and FM.

I could argue that despite the PAL-M changes you made a couple of
years ago, the device is only ever really used with a single standard
(NTSC), and thus it's entirely possible that the user may never call
S_FMT given the default is to capture 720x480 YUY2 and the device is
already in that mode at initialization.  Also, IIRC the s_stream()
subdev callback automatically gets invoked when you do a read(), in
order to support both MMAP and READ modes.  I could suggest that it
should be locked when you call S_INPUT, but I'm pretty sure that's how
I had it to begin with.  Likewise even in that case you could still
hit the issue if the user is trying to use the default input of 0
(i.e. plug in stick, call S_FREQ, and then poll for lock).

No easy answers here, just trade-offs between how badly you want to
break an API that was created with no consideration for power
management or hybrid devices.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

[PATCH 03/12] au8522: rework setup of audio routing

[Devin Heitmueller]

The original code was based on my reverse engineering of an I2C trace
of the Windows driver.  Now that I know what the registers actually do,
restructure the code a bit, removing some unneeded register programming
and fixing the sequencing of operations.

This reduces the time it takes to change inputs from 1300ms down to
600ms (as measured by "time v4l2-ctl -i 0")

Note this does not address outstanding issues related to the management
of the module clocks and power control for the various blocks, which
will be done in a separate patch.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/dvb-frontends/au8522_decoder.c | 29 ++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/media/dvb-frontends/au8522_decoder.c 
b/drivers/media/dvb-frontends/au8522_decoder.c
index 7811717..281b5ac 100644
--- a/drivers/media/dvb-frontends/au8522_decoder.c
+++ b/drivers/media/dvb-frontends/au8522_decoder.c
@@ -418,28 +418,29 @@ static void set_audio_input(struct au8522_state *state)
lpfilter_coef[i].reg_val[0]);
}
 
-   /* Setup audio */
-   au8522_writereg(state, AU8522_AUDIO_VOLUME_L_REG0F2H, 0x00);
-   au8522_writereg(state, AU8522_AUDIO_VOLUME_R_REG0F3H, 0x00);
-   au8522_writereg(state, AU8522_AUDIO_VOLUME_REG0F4H, 0x00);
-   msleep(150);
-   au8522_writereg(state, AU8522_SYSTEM_MODULE_CONTROL_0_REG0A4H, 0x00);
-   msleep(10);
-   au8522_writereg(state, AU8522_SYSTEM_MODULE_CONTROL_0_REG0A4H,
-   AU8522_SYSTEM_MODULE_CONTROL_0_REG0A4H_CVBS);
-   msleep(50);
+   /* Set the volume */
au8522_writereg(state, AU8522_AUDIO_VOLUME_L_REG0F2H, 0x7F);
au8522_writereg(state, AU8522_AUDIO_VOLUME_R_REG0F3H, 0x7F);
au8522_writereg(state, AU8522_AUDIO_VOLUME_REG0F4H, 0xff);
-   msleep(80);
-   au8522_writereg(state, AU8522_AUDIO_VOLUME_L_REG0F2H, 0x7F);
-   au8522_writereg(state, AU8522_AUDIO_VOLUME_R_REG0F3H, 0x7F);
+
+   /* Not sure what this does */
au8522_writereg(state, AU8522_REG0F9H, AU8522_REG0F9H_AUDIO);
+
+   /* Setup the audio mode to stereo DBX */
au8522_writereg(state, AU8522_AUDIO_MODE_REG0F1H, 0x82);
msleep(70);
-   au8522_writereg(state, AU8522_SYSTEM_MODULE_CONTROL_1_REG0A5H, 0x09);
+
+   /* Start the audio processing module */
+   au8522_writereg(state, AU8522_SYSTEM_MODULE_CONTROL_0_REG0A4H, 0x9d);
+
+   /* Set the audio frequency to 48 KHz */
au8522_writereg(state, AU8522_AUDIOFREQ_REG606H, 0x03);
+
+   /* Set the I2S parameters (WS, LSB, mode, sample rate */
au8522_writereg(state, AU8522_I2S_CTRL_2_REG112H, 0xc2);
+
+   /* Enable the I2S output */
+   au8522_writereg(state, AU8522_SYSTEM_MODULE_CONTROL_1_REG0A5H, 0x09);
 }
 
 /* --- */
-- 
1.9.1

[PATCH 09/12] xc5000: Don't spin waiting for analog lock

[Devin Heitmueller]

The xc5000 driver should not be spinning waiting for an analog lock.
The ioctl() should be returning immediately and the application is
responsible for polling for lock status.

This behavior isn't very visible in cases where you tune to a valid
channel, since lock is usually achieved much faster than 400ms.
However it is highly visible where doing things like changing video
standards, which sends tuning request for a frequency that is
almost never going to have an actual channel on it.

Also fixup the return values to treat zero as success and an actual
error code on error (to be consistent with other functions).  Note
this change has no practical effect at this time as none of the
callers inspect the return value.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/tuners/xc5000.c | 26 ++
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index e823aaf..e144627 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -565,38 +565,16 @@ static int xc_get_totalgain(struct xc5000_priv *priv, u16 
*totalgain)
return xc5000_readreg(priv, XREG_TOTALGAIN, totalgain);
 }
 
-static u16 wait_for_lock(struct xc5000_priv *priv)
-{
-   u16 lock_state = 0;
-   int watch_dog_count = 40;
-
-   while ((lock_state == 0) && (watch_dog_count > 0)) {
-   xc_get_lock_status(priv, _state);
-   if (lock_state != 1) {
-   msleep(5);
-   watch_dog_count--;
-   }
-   }
-   return lock_state;
-}
-
 #define XC_TUNE_ANALOG  0
 #define XC_TUNE_DIGITAL 1
 static int xc_tune_channel(struct xc5000_priv *priv, u32 freq_hz, int mode)
 {
-   int found = 0;
-
dprintk(1, "%s(%u)\n", __func__, freq_hz);
 
if (xc_set_rf_frequency(priv, freq_hz) != 0)
-   return 0;
-
-   if (mode == XC_TUNE_ANALOG) {
-   if (wait_for_lock(priv) == 1)
-   found = 1;
-   }
+   return -EREMOTEIO;
 
-   return found;
+   return 0;
 }
 
 static int xc_set_xtal(struct dvb_frontend *fe)
-- 
1.9.1

[PATCH 11/12] Fix breakage in "make menuconfig" for media_build

[Devin Heitmueller]

The Kconfig format is strict enough where if the indentation isn't
correct then the "make menuconfig" will break.

Fix the indentation to match all the other entries.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/rc/Kconfig | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/Kconfig b/drivers/media/rc/Kconfig
index e422f3d..5e83b76 100644
--- a/drivers/media/rc/Kconfig
+++ b/drivers/media/rc/Kconfig
@@ -169,11 +169,11 @@ config IR_HIX5HD2
tristate "Hisilicon hix5hd2 IR remote control"
depends on RC_CORE
help
-Say Y here if you want to use hisilicon hix5hd2 remote control.
-To compile this driver as a module, choose M here: the module will be
-called ir-hix5hd2.
+  Say Y here if you want to use hisilicon hix5hd2 remote control.
+  To compile this driver as a module, choose M here: the module will be
+  called ir-hix5hd2.
 
-If you're not sure, select N here
+  If you're not sure, select N here
 
 config IR_IMON
tristate "SoundGraph iMON Receiver and Display"
-- 
1.9.1

[PATCH 10/12] au8522: Set the initial modulation

[Devin Heitmueller]

We need to set the initial modulation on driver setup, or else any
calls to GET_FRONTEND prior to the first SET_FRONTEND call will get
back garbage.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/dvb-frontends/au8522_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/dvb-frontends/au8522_common.c 
b/drivers/media/dvb-frontends/au8522_common.c
index cf4ac24..6722838 100644
--- a/drivers/media/dvb-frontends/au8522_common.c
+++ b/drivers/media/dvb-frontends/au8522_common.c
@@ -234,6 +234,7 @@ int au8522_init(struct dvb_frontend *fe)
   chip, so that when it gets powered back up it won't think
   that it is already tuned */
state->current_frequency = 0;
+   state->current_modulation = VSB_8;
 
au8522_writereg(state, 0xa4, 1 << 5);
 
-- 
1.9.1

[PATCH 02/12] au8522: don't touch i2c master registers on au8522

[Devin Heitmueller]

Some stray lines got inserted into the driver when I reverse engineered
the I2C traffic (at the time I didn't know what the registers did).

It turns up these registers muck with the onboard I2C master, which
we don't use since we instead use the I2C gate.  Remove the lines
which can actually interfere with the operation of the bus.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/dvb-frontends/au8522_decoder.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/dvb-frontends/au8522_decoder.c 
b/drivers/media/dvb-frontends/au8522_decoder.c
index 12a5c2c..7811717 100644
--- a/drivers/media/dvb-frontends/au8522_decoder.c
+++ b/drivers/media/dvb-frontends/au8522_decoder.c
@@ -422,8 +422,6 @@ static void set_audio_input(struct au8522_state *state)
au8522_writereg(state, AU8522_AUDIO_VOLUME_L_REG0F2H, 0x00);
au8522_writereg(state, AU8522_AUDIO_VOLUME_R_REG0F3H, 0x00);
au8522_writereg(state, AU8522_AUDIO_VOLUME_REG0F4H, 0x00);
-   au8522_writereg(state, AU8522_I2C_CONTROL_REG1_REG091H, 0x80);
-   au8522_writereg(state, AU8522_I2C_CONTROL_REG0_REG090H, 0x84);
msleep(150);
au8522_writereg(state, AU8522_SYSTEM_MODULE_CONTROL_0_REG0A4H, 0x00);
msleep(10);
-- 
1.9.1

[PATCH 08/12] Add USB quirk for HVR-950q to avoid intermittent device resets

[Devin Heitmueller]

The USB core and sysfs will attempt to enumerate certain parameters
which are unsupported by the au0828 - causing inconsistent behavior
and sometimes causing the chip to reset.  Avoid making these calls.

This problem manifested as intermittent cases where the au8522 would
be reset on analog video startup, in particular when starting up ALSA
audio streaming in parallel - the sysfs entries created by
snd-usb-audio on streaming startup would result in unsupported control
messages being sent during tuning which would put the chip into an
unknown state.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/usb/core/quirks.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 96b21b0..3116edf 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -223,6 +223,10 @@
/* Blackmagic Design UltraStudio SDI */
{ USB_DEVICE(0x1edb, 0xbd4f), .driver_info = USB_QUIRK_NO_LPM },
 
+   /* Hauppauge HVR-950q */
+   { USB_DEVICE(0x2040, 0x7200), .driver_info =
+   USB_QUIRK_CONFIG_INTF_STRINGS },
+
/* INTEL VALUE SSD */
{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
 
-- 
1.9.1

[PATCH 01/12] au8522: don't attempt to configure unsupported VBI slicer

[Devin Heitmueller]

Since we don't suppoort sliced VBI with the au0828/au8522, there is
no need to configure the au8522 VBI slicer, which because of the
coefficients requires a large amount of i2c traffic.

Remove the relevant code.  Note that this has no effect on raw VBI
support, which is currently the only supported way to access VBI on
this device.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/dvb-frontends/au8522_decoder.c | 38 
 1 file changed, 38 deletions(-)

diff --git a/drivers/media/dvb-frontends/au8522_decoder.c 
b/drivers/media/dvb-frontends/au8522_decoder.c
index a2e7713..12a5c2c 100644
--- a/drivers/media/dvb-frontends/au8522_decoder.c
+++ b/drivers/media/dvb-frontends/au8522_decoder.c
@@ -179,42 +179,6 @@ static inline struct au8522_state *to_state(struct 
v4l2_subdev *sd)
return container_of(sd, struct au8522_state, sd);
 }
 
-static void setup_vbi(struct au8522_state *state, int aud_input)
-{
-   int i;
-
-   /* These are set to zero regardless of what mode we're in */
-   au8522_writereg(state, AU8522_TVDEC_VBI_CTRL_H_REG017H, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_CTRL_L_REG018H, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_TOTAL_BITS_REG019H, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_TUNIT_H_REG01AH, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_TUNIT_L_REG01BH, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_THRESH1_REG01CH, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_FRAME_PAT2_REG01EH, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_FRAME_PAT1_REG01FH, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_FRAME_PAT0_REG020H, 0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_FRAME_MASK2_REG021H,
-   0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_FRAME_MASK1_REG022H,
-   0x00);
-   au8522_writereg(state, AU8522_TVDEC_VBI_USER_FRAME_MASK0_REG023H,
-   0x00);
-
-   /* Setup the VBI registers */
-   for (i = 0x30; i < 0x60; i++)
-   au8522_writereg(state, i, 0x40);
-
-   /* For some reason, every register is 0x40 except register 0x44
-  (confirmed via the HVR-950q USB capture) */
-   au8522_writereg(state, 0x44, 0x60);
-
-   /* Enable VBI (we always do this regardless of whether the user is
-  viewing closed caption info) */
-   au8522_writereg(state, AU8522_TVDEC_VBI_CTRL_H_REG017H,
-   AU8522_TVDEC_VBI_CTRL_H_REG017H_CCON);
-
-}
-
 static void setup_decoder_defaults(struct au8522_state *state, bool is_svideo)
 {
int i;
@@ -317,8 +281,6 @@ static void setup_decoder_defaults(struct au8522_state 
*state, bool is_svideo)
AU8522_TOREGAAGC_REG0E5H_CVBS);
au8522_writereg(state, AU8522_REG016H, AU8522_REG016H_CVBS);
 
-   setup_vbi(state, 0);
-
if (is_svideo) {
/* Despite what the table says, for the HVR-950q we still need
   to be in CVBS mode for the S-Video input (reason unknown). */
-- 
1.9.1

[PATCH 04/12] au8522: remove note about VBI not being implemented

[Devin Heitmueller]

I got this working a couple of years ago.  Remove it from the
list of known issues.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/dvb-frontends/au8522_decoder.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/au8522_decoder.c 
b/drivers/media/dvb-frontends/au8522_decoder.c
index 281b5ac..5e21640 100644
--- a/drivers/media/dvb-frontends/au8522_decoder.c
+++ b/drivers/media/dvb-frontends/au8522_decoder.c
@@ -17,7 +17,6 @@
 
 /* Developer notes:
  *
- * VBI support is not yet working
  * Enough is implemented here for CVBS and S-Video inputs, but the actual
  *  analog demodulator code isn't implemented (not needed for xc5000 since it
  *  has its own demodulator and outputs CVBS)
-- 
1.9.1

[PATCH 05/12] au8522: remove leading bit for register writes

[Devin Heitmueller]

The leading bit in register values is actually an indicator as to
whether to perform a read or write, so remove the bit from the
register values, since the au8522_writereg() is now responsible
for adding this bit automatically.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/dvb-frontends/au8522_dig.c | 200 +++
 1 file changed, 100 insertions(+), 100 deletions(-)

diff --git a/drivers/media/dvb-frontends/au8522_dig.c 
b/drivers/media/dvb-frontends/au8522_dig.c
index 7ed326e..d117ddb 100644
--- a/drivers/media/dvb-frontends/au8522_dig.c
+++ b/drivers/media/dvb-frontends/au8522_dig.c
@@ -271,9 +271,9 @@ static int au8522_set_if(struct dvb_frontend *fe, enum 
au8522_if_freq if_freq)
return -EINVAL;
}
dprintk("%s() %s MHz\n", __func__, ifmhz);
-   au8522_writereg(state, 0x80b5, r0b5);
-   au8522_writereg(state, 0x80b6, r0b6);
-   au8522_writereg(state, 0x80b7, r0b7);
+   au8522_writereg(state, 0x00b5, r0b5);
+   au8522_writereg(state, 0x00b6, r0b6);
+   au8522_writereg(state, 0x00b7, r0b7);
 
return 0;
 }
@@ -283,33 +283,33 @@ static int au8522_set_if(struct dvb_frontend *fe, enum 
au8522_if_freq if_freq)
u16 reg;
u16 data;
 } VSB_mod_tab[] = {
-   { 0x8090, 0x84 },
+   { 0x0090, 0x84 },
{ 0x4092, 0x11 },
{ 0x2005, 0x00 },
-   { 0x8091, 0x80 },
-   { 0x80a3, 0x0c },
-   { 0x80a4, 0xe8 },
-   { 0x8081, 0xc4 },
-   { 0x80a5, 0x40 },
-   { 0x80a7, 0x40 },
-   { 0x80a6, 0x67 },
-   { 0x8262, 0x20 },
-   { 0x821c, 0x30 },
-   { 0x80d8, 0x1a },
-   { 0x8227, 0xa0 },
-   { 0x8121, 0xff },
-   { 0x80a8, 0xf0 },
-   { 0x80a9, 0x05 },
-   { 0x80aa, 0x77 },
-   { 0x80ab, 0xf0 },
-   { 0x80ac, 0x05 },
-   { 0x80ad, 0x77 },
-   { 0x80ae, 0x41 },
-   { 0x80af, 0x66 },
-   { 0x821b, 0xcc },
-   { 0x821d, 0x80 },
-   { 0x80a4, 0xe8 },
-   { 0x8231, 0x13 },
+   { 0x0091, 0x80 },
+   { 0x00a3, 0x0c },
+   { 0x00a4, 0xe8 },
+   { 0x0081, 0xc4 },
+   { 0x00a5, 0x40 },
+   { 0x00a7, 0x40 },
+   { 0x00a6, 0x67 },
+   { 0x0262, 0x20 },
+   { 0x021c, 0x30 },
+   { 0x00d8, 0x1a },
+   { 0x0227, 0xa0 },
+   { 0x0121, 0xff },
+   { 0x00a8, 0xf0 },
+   { 0x00a9, 0x05 },
+   { 0x00aa, 0x77 },
+   { 0x00ab, 0xf0 },
+   { 0x00ac, 0x05 },
+   { 0x00ad, 0x77 },
+   { 0x00ae, 0x41 },
+   { 0x00af, 0x66 },
+   { 0x021b, 0xcc },
+   { 0x021d, 0x80 },
+   { 0x00a4, 0xe8 },
+   { 0x0231, 0x13 },
 };
 
 /* QAM64 Modulation table */
@@ -396,78 +396,78 @@ static int au8522_set_if(struct dvb_frontend *fe, enum 
au8522_if_freq if_freq)
u16 reg;
u16 data;
 } QAM256_mod_tab[] = {
-   { 0x80a3, 0x09 },
-   { 0x80a4, 0x00 },
-   { 0x8081, 0xc4 },
-   { 0x80a5, 0x40 },
-   { 0x80aa, 0x77 },
-   { 0x80ad, 0x77 },
-   { 0x80a6, 0x67 },
-   { 0x8262, 0x20 },
-   { 0x821c, 0x30 },
-   { 0x80b8, 0x3e },
-   { 0x80b9, 0xf0 },
-   { 0x80ba, 0x01 },
-   { 0x80bb, 0x18 },
-   { 0x80bc, 0x50 },
-   { 0x80bd, 0x00 },
-   { 0x80be, 0xea },
-   { 0x80bf, 0xef },
-   { 0x80c0, 0xfc },
-   { 0x80c1, 0xbd },
-   { 0x80c2, 0x1f },
-   { 0x80c3, 0xfc },
-   { 0x80c4, 0xdd },
-   { 0x80c5, 0xaf },
-   { 0x80c6, 0x00 },
-   { 0x80c7, 0x38 },
-   { 0x80c8, 0x30 },
-   { 0x80c9, 0x05 },
-   { 0x80ca, 0x4a },
-   { 0x80cb, 0xd0 },
-   { 0x80cc, 0x01 },
-   { 0x80cd, 0xd9 },
-   { 0x80ce, 0x6f },
-   { 0x80cf, 0xf9 },
-   { 0x80d0, 0x70 },
-   { 0x80d1, 0xdf },
-   { 0x80d2, 0xf7 },
-   { 0x80d3, 0xc2 },
-   { 0x80d4, 0xdf },
-   { 0x80d5, 0x02 },
-   { 0x80d6, 0x9a },
-   { 0x80d7, 0xd0 },
-   { 0x8250, 0x0d },
-   { 0x8251, 0xcd },
-   { 0x8252, 0xe0 },
-   { 0x8253, 0x05 },
-   { 0x8254, 0xa7 },
-   { 0x8255, 0xff },
-   { 0x8256, 0xed },
-   { 0x8257, 0x5b },
-   { 0x8258, 0xae },
-   { 0x8259, 0xe6 },
-   { 0x825a, 0x3d },
-   { 0x825b, 0x0f },
-   { 0x825c, 0x0d },
-   { 0x825d, 0xea },
-   { 0x825e, 0xf2 },
-   { 0x825f, 0x51 },
-   { 0x8260, 0xf5 },
-   { 0x8261, 0x06 },
-   { 0x821a, 0x00 },
-   { 0x8546, 0x40 },
-   { 0x8210, 0x26 },
-   { 0x8211, 0xf6 },
-   { 0x8212, 0x84 },
-   { 0x8213, 0x02 },
-   { 0x8502, 0x01 },
-   { 0x8121, 0x04 },
-   { 0x8122, 0x04 },
-   { 0x852e, 0x10 },
-   { 0x80a4, 0xca },
-   { 0x80a7, 0x40 },
-   { 0x8526, 0x01 },
+   { 0x00a3, 0x09 },
+   { 0x00a4, 0x00 },
+   { 0x0081, 0xc4 },
+   { 0x00a5, 0x40 },
+   { 0x00aa, 0x77 },
+   { 0x00ad, 0x77 },
+   { 0x00a6, 0x67 },
+   { 0x0262, 0x20 },
+   { 0x021c, 0x30 },
+ 

[PATCH 07/12] au8522: fix lock detection to be more reliable.

[Devin Heitmueller]

Only looking at the lock register causes the status to float
between locked and not locked when there is no signal.  So improve
the logic to also examine the state of the FSC PLL, which results
in the lock status being consistently reported.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/dvb-frontends/au8522_decoder.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/au8522_decoder.c 
b/drivers/media/dvb-frontends/au8522_decoder.c
index 5e21640..343dc92 100644
--- a/drivers/media/dvb-frontends/au8522_decoder.c
+++ b/drivers/media/dvb-frontends/au8522_decoder.c
@@ -623,10 +623,12 @@ static int au8522_g_tuner(struct v4l2_subdev *sd, struct 
v4l2_tuner *vt)
int val = 0;
struct au8522_state *state = to_state(sd);
u8 lock_status;
+   u8 pll_status;
 
/* Interrogate the decoder to see if we are getting a real signal */
lock_status = au8522_readreg(state, 0x00);
-   if (lock_status == 0xa2)
+   pll_status = au8522_readreg(state, 0x7e);
+   if ((lock_status == 0xa2) && (pll_status & 0x10))
vt->signal = 0x;
else
vt->signal = 0x00;
-- 
1.9.1

[PATCH 12/12] au0828: Add timer to restart TS stream if no data arrives on bulk endpoint

[Devin Heitmueller]

For reasons unclear, we intermittently see a case where the tune
is successful but the bulk stream fails to deliver any packets.

Add a timer to automatically stop/start the data pump if we
encounter such a case.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/usb/au0828/au0828-dvb.c | 30 ++
 drivers/media/usb/au0828/au0828.h |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/media/usb/au0828/au0828-dvb.c 
b/drivers/media/usb/au0828/au0828-dvb.c
index 7e0c9b7..34dc7e0 100644
--- a/drivers/media/usb/au0828/au0828-dvb.c
+++ b/drivers/media/usb/au0828/au0828-dvb.c
@@ -105,6 +105,15 @@
 
 static void au0828_restart_dvb_streaming(struct work_struct *work);
 
+static void au0828_bulk_timeout(unsigned long data)
+{
+   struct au0828_dev *dev = (struct au0828_dev *) data;
+
+   dprintk(1, "%s called\n", __func__);
+   dev->bulk_timeout_running = 0;
+   schedule_work(>restart_streaming);
+}
+
 /*---*/
 static void urb_completion(struct urb *purb)
 {
@@ -138,6 +147,13 @@ static void urb_completion(struct urb *purb)
ptr[0], purb->actual_length);
schedule_work(>restart_streaming);
return;
+   } else if (dev->bulk_timeout_running == 1) {
+   /* The URB handler has fired, so cancel timer which would
+* restart endpoint if we hadn't
+*/
+   dprintk(1, "%s cancelling bulk timeout\n", __func__);
+   dev->bulk_timeout_running = 0;
+   del_timer(>bulk_timeout);
}
 
/* Feed the transport payload into the kernel demux */
@@ -160,6 +176,11 @@ static int stop_urb_transfer(struct au0828_dev *dev)
if (!dev->urb_streaming)
return 0;
 
+   if (dev->bulk_timeout_running == 1) {
+   dev->bulk_timeout_running = 0;
+   del_timer(>bulk_timeout);
+   }
+
dev->urb_streaming = false;
for (i = 0; i < URB_COUNT; i++) {
if (dev->urbs[i]) {
@@ -232,6 +253,11 @@ static int start_urb_transfer(struct au0828_dev *dev)
}
 
dev->urb_streaming = true;
+
+   /* If we don't valid data within 1 second, restart stream */
+   mod_timer(>bulk_timeout, jiffies + (HZ));
+   dev->bulk_timeout_running = 1;
+
return 0;
 }
 
@@ -622,6 +648,10 @@ int au0828_dvb_register(struct au0828_dev *dev)
return ret;
}
 
+   dev->bulk_timeout.function = au0828_bulk_timeout;
+   dev->bulk_timeout.data = (unsigned long) dev;
+   init_timer(>bulk_timeout);
+
return 0;
 }
 
diff --git a/drivers/media/usb/au0828/au0828.h 
b/drivers/media/usb/au0828/au0828.h
index 88e5974..05e445f 100644
--- a/drivers/media/usb/au0828/au0828.h
+++ b/drivers/media/usb/au0828/au0828.h
@@ -195,6 +195,8 @@ struct au0828_dev {
/* Digital */
struct au0828_dvb   dvb;
struct work_struct  restart_streaming;
+   struct timer_list   bulk_timeout;
+   int bulk_timeout_running;
 
 #ifdef CONFIG_VIDEO_AU0828_V4L2
/* Analog */
-- 
1.9.1

[PATCH 00/12] minor cleanups for HVR-950q

[Devin Heitmueller]

This is a series of mostly minor cleanups/fixes for the HVR-950q
driver.  We'll get this stuff merged since it's non-controversial,
and then we can argue about the more invasive patches to follow.

Devin Heitmueller (12):
  au8522: don't attempt to configure unsupported VBI slicer
  au8522: don't touch i2c master registers on au8522
  au8522: rework setup of audio routing
  au8522: remove note about VBI not being implemented
  au8522: remove leading bit for register writes
  au8522 Remove 0x4 bit for register reads
  au8522: fix lock detection to be more reliable.
  Add USB quirk for HVR-950q to avoid intermittent device resets
  xc5000: Don't spin waiting for analog lock
  au8522: Set the initial modulation
  Fix breakage in "make menuconfig" for media_build
  au0828: Add timer to restart TS stream if no data arrives on bulk
endpoint

 drivers/media/dvb-frontends/au8522_common.c  |   1 +
 drivers/media/dvb-frontends/au8522_decoder.c |  74 +++--
 drivers/media/dvb-frontends/au8522_dig.c | 215 +--
 drivers/media/rc/Kconfig |   8 +-
 drivers/media/tuners/xc5000.c|  26 +---
 drivers/media/usb/au0828/au0828-dvb.c|  30 
 drivers/media/usb/au0828/au0828.h|   2 +
 drivers/usb/core/quirks.c|   4 +
 8 files changed, 168 insertions(+), 192 deletions(-)

-- 
1.9.1

[PATCH 06/12] au8522 Remove 0x4 bit for register reads

[Devin Heitmueller]

The second highest bit in the register value is an indicator to do
a register read, so remove it since now au8522_regread() inserts
the bit automatically.

Also remove a stray instance where we were actually trying to write
to the I2C status register, which was actually a read.

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/dvb-frontends/au8522_dig.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb-frontends/au8522_dig.c 
b/drivers/media/dvb-frontends/au8522_dig.c
index d117ddb..3f3635f 100644
--- a/drivers/media/dvb-frontends/au8522_dig.c
+++ b/drivers/media/dvb-frontends/au8522_dig.c
@@ -284,7 +284,6 @@ static int au8522_set_if(struct dvb_frontend *fe, enum 
au8522_if_freq if_freq)
u16 data;
 } VSB_mod_tab[] = {
{ 0x0090, 0x84 },
-   { 0x4092, 0x11 },
{ 0x2005, 0x00 },
{ 0x0091, 0x80 },
{ 0x00a3, 0x0c },
@@ -654,12 +653,12 @@ static int au8522_read_status(struct dvb_frontend *fe, 
enum fe_status *status)
 
if (state->current_modulation == VSB_8) {
dprintk("%s() Checking VSB_8\n", __func__);
-   reg = au8522_readreg(state, 0x4088);
+   reg = au8522_readreg(state, 0x0088);
if ((reg & 0x03) == 0x03)
*status |= FE_HAS_LOCK | FE_HAS_SYNC | FE_HAS_VITERBI;
} else {
dprintk("%s() Checking QAM\n", __func__);
-   reg = au8522_readreg(state, 0x4541);
+   reg = au8522_readreg(state, 0x0541);
if (reg & 0x80)
*status |= FE_HAS_VITERBI;
if (reg & 0x20)
@@ -745,17 +744,17 @@ static int au8522_read_snr(struct dvb_frontend *fe, u16 
*snr)
if (state->current_modulation == QAM_256)
ret = au8522_mse2snr_lookup(qam256_mse2snr_tab,
ARRAY_SIZE(qam256_mse2snr_tab),
-   au8522_readreg(state, 0x4522),
+   au8522_readreg(state, 0x0522),
snr);
else if (state->current_modulation == QAM_64)
ret = au8522_mse2snr_lookup(qam64_mse2snr_tab,
ARRAY_SIZE(qam64_mse2snr_tab),
-   au8522_readreg(state, 0x4522),
+   au8522_readreg(state, 0x0522),
snr);
else /* VSB_8 */
ret = au8522_mse2snr_lookup(vsb_mse2snr_tab,
ARRAY_SIZE(vsb_mse2snr_tab),
-   au8522_readreg(state, 0x4311),
+   au8522_readreg(state, 0x0311),
snr);
 
if (state->config.led_cfg)
@@ -804,9 +803,9 @@ static int au8522_read_ucblocks(struct dvb_frontend *fe, 
u32 *ucblocks)
struct au8522_state *state = fe->demodulator_priv;
 
if (state->current_modulation == VSB_8)
-   *ucblocks = au8522_readreg(state, 0x4087);
+   *ucblocks = au8522_readreg(state, 0x0087);
else
-   *ucblocks = au8522_readreg(state, 0x4543);
+   *ucblocks = au8522_readreg(state, 0x0543);
 
return 0;
 }
-- 
1.9.1

[PATCH] cx88: Fix regression in initial video standard setting

[Devin Heitmueller]

Setting initial standard at the top of cx8800_initdev would cause the
first call to cx88_set_tvnorm() to return without programming any
registers (leaving the driver saying it's set to NTSC but the hardware
isn't programmed).  Even worse, any subsequent attempt to explicitly
set it to NTSC-M will return success but actually fail to program the
underlying registers unless first changing the standard to something
other than NTSC-M.

Set the initial standard later in the process, and make sure the field
is zero at the beginning to ensure that the call always goes through.

This regression was introduced in the following commit:

commit ccd6f1d488e7 ("[media] cx88: move width, height and field to core
struct")

Author: Hans Verkuil <hans.verk...@cisco.com>
Date:   Sat Sep 20 09:23:44 2014 -0300

[media] cx88: move width, height and field to core struct

Signed-off-by: Devin Heitmueller <dheitmuel...@kernellabs.com>
---
 drivers/media/pci/cx88/cx88-cards.c | 9 -
 drivers/media/pci/cx88/cx88-video.c | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/cx88/cx88-cards.c 
b/drivers/media/pci/cx88/cx88-cards.c
index 73cc7a6..b7a8c8c 100644
--- a/drivers/media/pci/cx88/cx88-cards.c
+++ b/drivers/media/pci/cx88/cx88-cards.c
@@ -3681,7 +3681,14 @@ struct cx88_core *cx88_core_create(struct pci_dev *pci, 
int nr)
core->nr = nr;
sprintf(core->name, "cx88[%d]", core->nr);
 
-   core->tvnorm = V4L2_STD_NTSC_M;
+   /*
+* Note: Setting initial standard here would cause first call to
+* cx88_set_tvnorm() to return without programming any registers.  Leave
+* it blank for at this point and it will get set later in
+* cx8800_inittdev()
+*/
+   core->tvnorm  = 0;
+
core->width   = 320;
core->height  = 240;
core->field   = V4L2_FIELD_INTERLACED;
diff --git a/drivers/media/pci/cx88/cx88-video.c 
b/drivers/media/pci/cx88/cx88-video.c
index c7d4e87..3c529dd 100644
--- a/drivers/media/pci/cx88/cx88-video.c
+++ b/drivers/media/pci/cx88/cx88-video.c
@@ -1435,7 +1435,7 @@ static int cx8800_initdev(struct pci_dev *pci_dev,
 
/* initial device configuration */
mutex_lock(>lock);
-   cx88_set_tvnorm(core, core->tvnorm);
+   cx88_set_tvnorm(core, V4L2_STD_NTSC_M);
v4l2_ctrl_handler_setup(>video_hdl);
v4l2_ctrl_handler_setup(>audio_hdl);
cx88_video_mux(core, 0);
-- 
1.9.1

RFC: Power states and VIDIOC_STREAMON

[Devin Heitmueller]

Hello all,

I'm in the process of putting together a bunch of long-standing fixes
for HVR-950q driver, and I ran into a regression related to the way
the video decoder is being managed.  Before we dig into the details
here's the general question:

Should a user be able to interrogate video decoder properties after
doing a tune without having first called VIDIOC_STREAMON?

A long-standing use case is to be able to use a command-line tool like
v4l2-ctl to set the input, set the standard, issue a tuning request,
poll for a lock status, and once you see a signal lock then start
streaming.  This means that prior to starting streaming the tuner,
analog demodulator, and video decoder are all running even though
you're not actually receiving video buffers.

The problem comes down to these two patches:

https://git.linuxtv.org/media_tree.git/commit/drivers/media/dvb-frontends/au8522_decoder.c?id=38fe3510fa8fb5e75ee3b196e44a7b717d167e5d
https://git.linuxtv.org/media_tree.git/commit/drivers/media/dvb-frontends/au8522_decoder.c?id=d289cdf022c5bebf09c73097404aa9faf2211381

Prior to these patches, I would power up the IP blocks for the analog
demodulator and decoder when the video routing was setup (typically
when setting the input).  However as a result of these patches
powering up of those IP blocks is deferred until STREAMON is called.
Hence if you just set the input and issue a tuning request, and poll
for lock then you will never see the tuner in a locked state
regardless of the actual signal state.

I can appreciate the motivation behind Mauro's change in wanting to
constrain the window that the analog decoder is powered up to reduce
the risk of having it powered up at the same time as the digital
demodulator, but if it breaks long-standing ABI behavior then that
probably isn't going to work.

I did take a look at the the VIDIOC_STREAMON docs, which state that
the "Capture hardware is disabled and no buffers are filled" prior to
calling STREAMON:

https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/vidioc-streamon.html

However that language would suggest that even the tuner would be
powered down prior to calling STREAMON, which we know is almost never
the case.

I suspect the ambiguity lies in what is defined by "capture hardware":

The DMA engine or other mechanism of transferring completed video
buffers to userland?
The DMA engine and the video decoder?
The DMA engine, video decoder, and analog demodulator?
The DMA engine, video decoder, analog demodulator, and tuner?

I had always interpreted it such that the STREAMON call just
controlled whether the DMA engine was running, and thus you could do
anything else with the decoder before calling STREAMON other than
actually receiving video buffers.

My instinct would be to revert the patch in question since it breaks
ABI behavior which has been present for over a decade, but I suspect
such a patch would be rejected since it was Mauro himself who
introduced the change in behavior.

I look forward to hearing from the V4L2 maintainers with regards to
what the expected ABI behavior should be, at which point I can figure
out how to adjust the driver code to accommodate such behavior (and if
that means I cannot query for a signal lock prior to calling STREAMON,
going back and changing a bunch of userland code which expects such).

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFC] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1

[Devin Heitmueller]

>> For what it's worth, I doubt most of the em28xx designs have the
>> tvp5150 interrupt request line connected in any way.
>
> True. But, on embedded hardware, such line may be connected into the
> SoC. Actually, from the IGEPv3 expansion diagram:
>
> 
> https://www.isee.biz/support/downloads/item/igepv2-expansion-rc-schematics
>
> The INT line is connected to CAM_IRQ. That's connected to GPIO_154 pin
> at OMAP3.
>
> So, on a first glance, it seems possible to use it, instead of polling.

To be clear, I wasn't suggesting that the IRQ request line on the
tvp5150 couldn't be supported in general (for example, for those
embedded targets which have it wired up to a host processor).  I'm
just saying you shouldn't expect it to work on most (perhaps all)
em28xx designs which have the tvp5150.  In fact on some em28xx designs
the pin is used as a GPIO output tied to a mux to control input
selection.  Hence blindly enabling the interrupt request line by
default would do all sorts of bad things.

>> You would likely
>> have to poll the FIFO status register via I2C,
>
> Yes, I considered this option when I wrote the driver. It could work,
> although it would likely have some performance drawback, as the driver
> would need to poll it at least 60 times per second.
>
>> or use the feature to
>> embed the sliced data into as VANC data in the 656 output (as
>> described in sec 3.9 of the tvp5150am1 spec).
>
> True, but the bridge driver would need to handle such data.

Correct.

> I remember I looked on this when I wrote the driver, but I was
> unable to find a way for em28xx to parse (or forward) such
> data packets.

I'm pretty sure it's possible, but I haven't looked at the datasheets
in a number of years and don't recall the details.

Hardware VBI splicing is supported by a number of decoders but it's
rarely used on commodity PCs (the Conexant and NXP decoders support it
as well).  That said, I won't argue there might be some value on
really low end platforms.  All I would ask is that if you do introduce
any such functionality into the tvp5150 driver for some embedded
application that you please not break support for devices such as the
em28xx.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [RFC] [media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1

[Devin Heitmueller]

> Currently, the driver doesn't support (2), because, at the time
> I wrote the driver, I didn't find a way to read the interrupts generated
> by tvp5150 at em28xx[1], due to the lack of em28xx documentation,
> but adding support for it shoudn't be hard. I may eventually do it
> when I have some time to play with my ISEE hardware.

For what it's worth, I doubt most of the em28xx designs have the
tvp5150 interrupt request line connected in any way.  You would likely
have to poll the FIFO status register via I2C, or use the feature to
embed the sliced data into as VANC data in the 656 output (as
described in sec 3.9 of the tvp5150am1 spec).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: Problem: saa7113 (saa7115) vs. "conrad usb grabber usb-472"

[Devin Heitmueller]

> lsusb:
> Bus 003 Device 002: ID 0573:0400 Zoran Co. Personal Media Division (Nogatech) 
> D-Link V100

The Zoran usbvision driver has been a mess for years, and it's not
going to get better anytime soon.  It's a *really* old design and
there hasn't been any interest from any of the developers to work on
it.

In this particular case, you're probably way better off just throwing
it away and buying a new $25 capture device.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [xawtv3] Request: Support for FM RDS

[Devin Heitmueller]

Hi George,

The big problem is that almost none of the hardware tuners out there
which support FM have support for RDS.  You generally need an extra
chip, and very few devices have it (IIRC, none of the ones that are
supported in Linux have been available in retail for a number of
years).

Don't get me wrong - I would like to see RDS supported as well - but I
couldn't find a single tuner product shipping in retail that supports
it.

In short, it's a hardware limitation, not a problem with the
linux-media driver stack.

Devin

On Wed, Feb 15, 2017 at 11:07 AM, George Pojar  wrote:
> FM RDS would be a great feature in radio console application. It would
> be nice to see what the name of the song is that is playing (that is,
> if the station supports RDS).



-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH v2 0/6] Fix tvp5150 regression with em28xx

[Devin Heitmueller]

Hi Mauro,

> With that, I completed the tests on HVR-950. My tests covered:
> - S-Video, Composite, TV
> - 480i and 480p
> - Closed Captions (with HVR-350 - it seems that MediaMVP doesn't
>   produce NTSC CC).

FYI:  the MediaMVP HD can be configured to output NTSC CC over VBI.
If you want that functionality, I can dig out the script.  In fact
I've got an alternate GUI which just plays a clip on boot and lets you
select all the different resolutions/framerates available for
composte/component/HDMI (for both PAL and NTSC) just by hitting
buttons on the remote.  If you're interested, let me know and I'll dig
it up.  It's a great tool to have, especially when doing work with
HDMI where there are many more possible combinations to choose from.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] tvp5150: don't touch register TVP5150_CONF_SHARED_PIN if not needed

[Devin Heitmueller]

Hi Mauro, Laurent,

I tried out Mauro's latest patch (9:46am EST), and it appears to at
least partially address the issue, but still doesn't work.  In fact,
whereas before I was getting stable video with a chroma issue, with
the patch applied I'm now getting no video at all (i.e. tvtime is
completely blocked waiting for frames to arrive).

First off, a register dump does show that register 0x03 is now 0x6F,
so at least that part is working.  However, TVP5150_DATA_RATE_SEL,
(register 0x0D) is now being set to 0x40, whereas it needs to be set
to 0x47 to work properly.  Just to confirm, I started up tvtime and
fed the device the following command, at which point video started
rendering properly:

sudo v4l2-dbg --chip=subdev0 --set-register=0x0d 0x47

I'm not sitting in front of the datasheet right now so I cannot
suggest what the correct fix is, but at first glance it looks like the
first hunk of Mauro's patch isn't correct for em28xx devices.

Also worth noting for the moment I'm testing exclusively with
composite on the HVR-850.  Once we've got that working, I'll dig out
an s-video cable and make sure that is working too.

Cheers,

Devin


On Thu, Dec 8, 2016 at 10:22 AM, Laurent Pinchart
 wrote:
> On Thursday 08 Dec 2016 12:16:08 Mauro Carvalho Chehab wrote:
>> Em Thu, 08 Dec 2016 15:41:59 +0200 Laurent Pinchart escreveu:
>> > On Thursday 08 Dec 2016 11:38:34 Mauro Carvalho Chehab wrote:
>> > > changeset 460b6c0831cb ("[media] tvp5150: Add s_stream subdev operation
>> > > support") added a logic that overrides TVP5150_CONF_SHARED_PIN setting,
>> > > depending on the type of bus set via the .set_fmt() subdev callback.
>> > >
>> > > This is known to cause trobules on devices that don't use a V4L2
>> > > subdev devnode, and a fix for it was made by changeset 47de9bf8931e
>> > > ("[media] tvp5150: Fix breakage for serial usage"). Unfortunately,
>> > > such fix doesn't consider the case of progressive video inputs,
>> > > causing chroma decoding issues on such videos, as it overrides not
>> > > only the type of video output, but also other unrelated bits.
>> > >
>> > > So, instead of trying to guess, let's detect if the device is set
>> > > via Device Tree. If not, just ignore the bogus logic.
>> >
>> > If you add a big [HACK] tag to the subject line, sure. I thought this
>> > would have been an occasion to fix the problem correctly :-(
>>
>> No, this is not a hack.
>>
>> It is a patch that restores the driver behavior that used to be
>> before adding DT support to the driver. Whatever DT-based drivers
>> need, it *should not* change the behavior for devices that don't
>> use DT.
>
> 1. This has nothing to do with DT, but with the addition of the s_stream()
> operation.
>
> 2. When I added s_stream() support the em28xx driver did not call s_stream(1).
> That has been changed by
>
> commit 13d52fe40f1f7bbad49128e8ee6a2fe5e13dd18d
> Author: Mauro Carvalho Chehab 
> Date:   Tue Jan 26 06:59:39 2016 -0200
>
> [media] em28xx: fix implementation of s_stream
>
> On em28xx driver, s_stream subdev ops was not implemented
> properly. It was used only to disable stream, never enabling it.
> That was the root cause of the regression when we added support
> for s_stream on tvp5150 driver.
>
> With that, we can get rid of the changes on tvp5150 side,
> e. g. changeset 47de9bf8931e ('[media] tvp5150: Fix breakage for serial
> usage').
>
> Tested video output on em2820+tvp5150 on WinTV USB2 and
> video and/or vbi output on em288x+tvp5150 on HVR 950.
>
> Signed-off-by: Mauro Carvalho Chehab 
>
> 3. There's clearly a bug in the current implementation, and it needs to be
> fixed. That fact does not turn every attempt to address the bug into proper
> fixes by magic. Hacks remain hacks.
>
> 4. I'm working on a proper fix.
>
>> I agree with you that the patch is incomplete, as it doesn't
>> add any OF var that would allow DT to specify the values
>> to be used for TVP5150_CONF_SHARED_PIN and TVP5150_MISC_CTL,
>> and assumes that tvp5150, tvp5151 and tvp5150am1 will all use
>> the same values for TVP5150_MISC_CTL.
>>
>> In order to fix that, someone with a DT-based driver with tvp5150,
>> tvp5150am1 and/or tvp5151 would need to spend some time and test
>> the hardware with both interlaced and progressive video inputs.
>>
>> That's not me, as I don't have any hardware that meets such requirement.
>>
>> If someone ships me such hardware, I could work on it on my spare time.
>> Otherwise, then perhaps you could work on such patch - or we could ping
>> Javier on Monday and see if has time/interest to work on it (afaikt, he's
>> OOT the rest of this week).
>>
>> Anyway, with this patch applied, the one working on such fix won't need
>> to be concerned to cause new regressions on the non-DT drivers that use
>> this chip, with is, IMHO, a very good thing.
>>
>> Also, this patch is simple enough to be backported to 

Regression: tvp5150 refactoring breaks all em28xx devices

[Devin Heitmueller]

Hello Javier, Mauro, Laurent,

I hope all is well with you.  Mauro, Laurent:  you guys going to
ELC/Portland in February?

Looks like the refactoring done to tvp5150 in January 2016 for
s_stream() to support some embedded platform caused breakage in the
30+ em28xx products that also use the chip.

Problem confirmed on both the Startech SVIDUSB2 board Steve Preston
was nice enough to ship me (after adding a board profile), as well as
on my original HVR-950 which has worked fine since 2008.

The implementation tramples the TVP5150_MISC_CTL register, blowing
into it a hard-coded value based on one of two scenarios, neither of
which matches what is expected by em28xx devices.  At least in the
case of NTSC, this results in chroma cycling.  This was also reported
by Alexandre-Xavier Labonté-Lamoureux back in August, although in the
video below he's also having some other issue related to progressive
video because he's using an old gaming console as the source (i.e. pay
attention to the chroma effects in the top half of the video rather
than the fact that only the first field is being rendered).

https://youtu.be/WLlqJ7T3y4g

The s_stream implementation writes 0x09 or 0x0d into TVP5150_MISC_CTL
(overriding whatever was written by tvp5150_init_default and
tvp5150_selmux().  In fact, just as a test I was able to start up
video, see the corruption, and write the correct value back into the
register via v4l2-dbg in order to get it working again:

sudo v4l2-dbg --chip=subdev0 --set-register=0x03 0x6f

There's no easy fix for this without extending the driver to support
proper configuration of the output pin muxing, which it isn't clear to
me what the right approach is and I don't have the embedded hardware
platform that prompted the refactoring in order to do regression
testing anyway.

Feel free to take it upon yourselves to fix the regression you introduced.

Thanks,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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 7/8] v4l: Add signal lock status to source change events

[Devin Heitmueller]

> OK, but what can the application do with that event? If the glitch didn't
> affect the video, then it is pointless.
>
> If the lock is lost, then normally you loose video as well. If not, then
> applications are not interested in the event.

What about free running mode (where some decoders delivers blue or
black video with no signal present)?  In that case it might still be
useful to inform the application so it can show a message that says
something like "No Signal".

Devin


-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] v4l2 support for thermopile devices

[Devin Heitmueller]

Hi Matt,

> Need some input for the video pixel data types, which the device we
> are using (see datasheet links below) is outputting pixel data in
> little endian 16-bit of which a 12-bits signed value is used.  Does it
> make sense to do some basic processing on the data since greyscale is
> going to look weird with temperatures under 0C degrees? Namely a cold
> object is going to be brighter than the hottest object it could read.
> Or should a new V4L2_PIX_FMT_* be defined and processing done in
> software?  Another issue is how to report the scaling value of 0.25 C
> for each LSB of the pixels to the respecting recording application.

Regarding the format for the pixel data:  I did some research into
this when doing some driver work for the Seek Thermal (a product
similar to the FLIR Lepton).  While it would be nice to be able to use
an existing application like VLC or gStreamer to just take the video
and capture from the V4L2 interface with no additional userland code,
the reality is that how you colorize the data is going to be highly
user specific (e.g. what thermal ranges to show with what colors,
etc).  If your goal is really to do a V4L2 driver which returns the
raw data, then you're probably best returning it in the native
greyscale format (whether that be an existing V4L2 PIX_FMT or a new
one needs to be defined), and then in software you can figure out how
to colorize it.

Just my opinion though

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Null pointer dereference in ngene-core.c

[Devin Heitmueller]

On Mon, Sep 19, 2016 at 8:51 PM, Alexandre-Xavier Labonté-Lamoureux
 wrote:
> Hi people,
>
> In the file "/linux/drivers/media/pci/ngene/ngene-core.c", there is a
> null pointer dereference at line 1480.
>
> Code in the function "static int init_channel(struct ngene_channel *chan)"
> ==
> if (io & NGENE_IO_TSIN) {
> chan->fe = NULL;  // Set to NULL
> if (ni->demod_attach[nr]) { // First condition
>ret = ni->demod_attach[nr](chan);
> if (ret < 0)   // Another condition
> goto err; // Goto that avoids
> the problem
> }
> if (chan->fe && ni->tuner_attach[nr]) { // Condition that
> tests the null pointer
> ret = ni->tuner_attach[nr](chan);
> if (ret < 0)
> goto err;
> }
> }
> =
>
> "chan->fe" is set to NULL, then it tests for something (I have no idea
> what it's doing, I know nothing about this driver), if the results of
> the first two if conditions fail to reach the goto, then it will test
> the condition with the null pointer, which will cause a crash. I don't
> know if the kernel can recover from null pointers, I think not.

I would have to actually look at the code, but my guess is because the
call to ni-ni->demod_attach[nr](chan) will actually set chan->fe if
successful.

The code path your describing is actually the primary use case.  The
cases where you see "goto err" will only be followed if there was some
sort of error condition, which means the driver essentially will
*always* hit the if() statement you are referring to during normal
operation (assuming nothing was broken in the hardware).

In short, the code makes sure chan->fe is NULL, then it calls the
demod_attach, then it checks both for the demod_attach returning an
error *and* making sure demod_attach set chan->fe to some non-NULL
value.  If both are the case, then it calls tuner_attach().

This is a pretty standard workflow -- you should see it in many other
drivers, although it's not uncommon in other drivers for something
like chan->fe to actually be the value returned by the demod_attach(),
and the demod attach routine would return NULL if there was some
failure condition.  The problem with that approach is that you can
only report one type of failure to the caller (all the caller knows is
a failure occured, it has no visibility as to the nature of the
failure), whereas with this approach you can return different values
for different error conditions.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Null pointer dereference in ngene-core.c

[Devin Heitmueller]

On Mon, Sep 19, 2016 at 8:51 PM, Alexandre-Xavier Labonté-Lamoureux
 wrote:
> Hi people,
>
> In the file "/linux/drivers/media/pci/ngene/ngene-core.c", there is a
> null pointer dereference at line 1480.
>
> Code in the function "static int init_channel(struct ngene_channel *chan)"
> ==
> if (io & NGENE_IO_TSIN) {
> chan->fe = NULL;  // Set to NULL
> if (ni->demod_attach[nr]) { // First condition
>ret = ni->demod_attach[nr](chan);
> if (ret < 0)   // Another condition
> goto err; // Goto that avoids
> the problem
> }
> if (chan->fe && ni->tuner_attach[nr]) { // Condition that
> tests the null pointer
> ret = ni->tuner_attach[nr](chan);
> if (ret < 0)
> goto err;
> }
> }
> =
>
> "chan->fe" is set to NULL, then it tests for something (I have no idea
> what it's doing, I know nothing about this driver), if the results of
> the first two if conditions fail to reach the goto, then it will test
> the condition with the null pointer, which will cause a crash. I don't
> know if the kernel can recover from null pointers, I think not.

This looks fine to me.  It's a simple test to see if chan->fe got set
to null (presumably in the above block of code).  A null pointer
dereference would be if the first block set *chan* to NULL (as opposed
to chan->fe) and then the if() statement then attempted to inspect
chan->fe.

LGTM.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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)

[Devin Heitmueller]

Hi Steve,

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

Have you actually opened these units up and confirmed what chips are
inside of them?  Or did you determine it's em28xx/saa711x via a Google
search.  The reason I ask is many of these devices will quietly change
their internal design over time, without changing the plastics and/or
model number.  Hence you cannot simply rely on what somebody else may
have said in terms of what chips are inside the device you're holding
in your hand.

First step would probably be to confirm the chips in question.  If
they really are based on the em2882/saa7115, then it should be pretty
easy to get working with a minor code change to the driver.

If you're in the US and you're willing to throw it in a USPS flat rate
box and ship it to me, I can probably have it working in about an
hour.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Digital Devices CI adapter problem

[Devin Heitmueller]

On Wed, Jun 8, 2016 at 4:10 PM, Marcin Kałuża
 wrote:
> Hello!
> I'm looking for someone who was able to successfully use Octopus Dual
> CI bridge by Digital Devices under Linux. We got it _almost_ working -
> we have a strange problem of the module dropping TS packets in sets of
> 30-33 packets rather randomly and this corrupts the whole stream.
>
> Their support ignored the ticket so far
> (http://support.digital-devices.eu/ticket.php?track=UG1-B42-NSGV=marcin.kaluza%40trioptimum.com=51010)
> and we're slowly running out of options.
>
> We've tried rebuilding the module using streaming dma api
> (DDB_ALT_DMA), we changed the kernel (our 3.18.22 and 4.2.3 from FC
> 23), disabled smp, still the same.
>
> Strange things happen when I call read() to get data back from CI, if
> I use any other buffer size then their internal dma buffer (672*188),
> I sometimes get the data not in order I wrote them (we use test TS
> stream with a counter inside ts payload), and the strangest of all -
> if I use bigger buffer (i.e. 1000*188), read() always returns that
> value (188000), but actual amount of content in my read buffer vary
> greatly (although never exeeds their buffer size of 672*188) - we
> clear the read buffer before each read so we know how much data was
> actually read. That's probably a bug, but I have no idea how can this
> even happen (their code
> (https://github.com/DigitalDevices/dddvb/blob/master/ddbridge/ddbridge-core.c#L772)
> looks quite ok as for my not so great knowledge of linux kernel driver
> coding). Has anyone encountered similar problems? It looks like a race
> condition of some sort, but I was unable find/fix it.
>
> I'll be most grateful for any reply/suggestions/help...
> Martin

This mailing list generally isn't for vendors' out-of-kernel drivers.
If Digital Devices wants to not be responsive to users who bought
their products and wants the community to provide free tech support
for their devices, they should get their drivers merged upstream.  :-)

That said, the math in ddb_input_read() looks pretty wonky.  The fact
that it always returns count (i.e. the size of the buffer provided
from userland), without taking into account how much data is actually
in the ring buffer certainly looks wrong.  If there isn't enough data
available, it should return the amount of data that *is* available,
not the size of the buffer passed in from userland.

I would add some more logging to that routine and see what's going on.
You'll probably have to take some time to understand what the actual
buffer filling algorithm is supposed to be for that hardware in order
to figure out what's going wrong.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: HVR-850 2040:b140 fails to initialize

[Devin Heitmueller]

> Appreciate some advice.

Return it and get an HVR-950q.  This has been a known issue for more
than 5 years and nobody's gotten around to fixing it (largely because
of limitations inside the V4L2 framework).

Devin
(a.k.a. the guy who added the original support for the HVR-850).

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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 driver for StarTech SVID2USB2

[Devin Heitmueller]

On Wed, Jan 6, 2016 at 1:27 PM, Schubert, Matthew R.
(LARC-D319)[TEAMS2]  wrote:
> Hello,
>
> We are attempting to use a StarTech Video Capture cable (Part# SVID2USB2) 
> with our CentOS 6.7 machine with no success. The em28xx driver seems to load 
> but cannot properly ID the capture cable. Below are the outputs from "dmesg" 
> and "lsusb -v" run after plugging in the device. Any advice is appreciated.

Try adding "card=9" to the modprobe option.  If that doesn't work than
try "card=29".  Most of those really cheap devices either have an
saa7113 or tvp5150 video decoder, and one of those two board profiles
should work.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] cx231xx: correctly handling failed allocation

[Devin Heitmueller]

On Tue, Dec 29, 2015 at 1:53 PM, Insu Yun  wrote:
> Since kmalloc can be failed in memory pressure,
> if not properly handled, NULL dereference can be happend
>
> Signed-off-by: Insu Yun 
> ---
>  drivers/media/usb/cx231xx/cx231xx-417.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c 
> b/drivers/media/usb/cx231xx/cx231xx-417.c
> index 47a98a2..9725e4f 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-417.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-417.c
> @@ -1382,6 +1382,8 @@ static int cx231xx_bulk_copy(struct cx231xx *dev, 
> struct urb *urb)
> buffer_size = urb->actual_length;
>
> buffer = kmalloc(buffer_size, GFP_ATOMIC);
> +   if (!buffer)
> +   return -ENOMEM;

A kmalloc() call inside a bulk handler running in softirq context?
That doesn't look right.

That said, I don't have any specific objection to the patch (which I'm
assuming came from some automated tool), but I suspect the cx231xx-417
code is probably completely broken.  The only device I've ever seen
that has the cx23102 and cx23417 is one of the Polaris EVKs, which
AFAIK nobody has ever shipped a production design based on.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: ivtv: PVR family datasheet ?

[Devin Heitmueller]

> I actually refer to CX23418 chip, which I don't find its datasheet (I
> saw in code that cx18 can work in YUV raw capture, so I wanted to
> verify that CX23418 can capture YUV raw format).

The cx23418 datasheets are not publicly available, although I have
them under NDA and can likely answer specific questions if you have
any.

Yes, the 418 can capture both raw YUV format and compressed MPEG2, and
it's supported under Linux assuming you're using a card such as the
HVR-1600.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Trying to enable RC6 IR for PCTV T2 290e

[Devin Heitmueller]

On Sun, Nov 15, 2015 at 3:49 PM, Chris Rankin  wrote:
> Hi,
>
> My Hauppauge RC5 remote control finally broke, and the PCTV T2 290e's
> native RC5 remote control isn't suitable for VDR, and so I bought a
> cheap RC6 remote as a replacement. The unit I chose was the Ortek
> VRC-1100 Vista MCE Remote Control, USB ID 05a4:9881. I've been able to
> switch the PCTV device into RC6 mode using "ir-keytable -p rc-6",
> which does seem to execute the correct case of
> em2874_ir_change_protocol(). However, when I press any buttons on my
> new remote, I still don't see em2874_polling_getkey() being called,
> which makes me wonder if the RC6 support is truly enabled.

It's been a few years since I looked at that code, and when I
orginally wrote it I don't think I bothered with the RC6 support.
That said, it's not interrupt driven and the em2874_polling_getkey()
should fire every 100ms regardless of whether the hardware receives an
IR event (the polling code queries a status register and if it sees a
new event it reads the RC code received and announces it to the input
subsystem).

I would probably look through the code and see why the polling routine
isn't setup to run.  There is probably logic in there that causes the
polling to never get enabled if a keymap isn't associated with the
board profile in em28xx-cards.c

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Trying to enable RC6 IR for PCTV T2 290e

[Devin Heitmueller]

> I've dug a bit deeper and discovered that the reason the
> em28xx_info(...) lines that I'd added to em2874_polling_getkey()
> weren't appearing is because I'd loaded the wrong version of the
> em28xx-rc module! (Doh!)

Ok, good.

> The polling function *is* being called regularly, but
> em28xx_ir_handle_key() isn't noticing any keypresses. (However, it
> does notice when I switch back to RC5).

How are you "switching back to RC5"?  Are you actually loading in an
RC6 versus RC5 keymap?  The reason I ask is because the onboard
receiver has to be explicitly configured into one of the two modes,
and IIRC this happens based on code type for the loaded keymap.  Hence
if you have an RC5 keymap loaded, you'll never see the IR receiver
reporting RC6 codes.

And of course it's always possible you've hit a bug/regression.  I
haven't touched that code in years and I know it's been through a
couple of rounds of refactoring.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: dvb-core: Don't force CAN_INVERSION_AUTO in oneshot mode.

[Devin Heitmueller]

Hi Malcolm,

>> The capabilities call interacting with the oneshot setting is rather weird
>> and maybe unexpected.
>>
>>
>
> No, because in normal mode it can do auto inversion.

This isn't my area of expertise, but I suspect this is going to cause
some pretty confusing behavior.  Generally speaking device
capabilities are queried right after the frontend is opened, and the
frontend capabilities typically don't ever change.  In this case, an
application would have to know to check the capabilities a second time
after calling FE_SET_FRONTEND_TUNE_MODE in order to determine whether
auto inversion *really* is available.

If the goal was for the software-emulated auto inversion to be
transparent to userland, perhaps it makes more sense for the oneshot
mode to toggle the inversion if needed.  The oneshot mode would
continue to disable zigzag and the stats monitoring.  I realize that
this is a bit messy since it won't really be "oneshot", but I don't
know what else can be done without breaking the ABI.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Adding support for three new Hauppauge HVR-1275 variants - testers reqd.

[Devin Heitmueller]

 Look at the em28xx driver and you will probably see why it does not work as
 expected. For my eyes, according to em28xx driver, it looks like that bus
 control is aimed for bridge driver. You or em28xx is wrong.

Neither are wrong.  In some cases the call needs to be intercepted by
the frontend in order to disable its TS output.  In other cases it
needs to be intercepted by the bridge to control a MUX chip which
dictates which demodulator's TS output to route from (typically by
toggling a GPIO).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: XC5000C 0x14b4 status

[Devin Heitmueller]

 IMHO, the best is to get the latest firmware licensed is the best
 thing to do.

 Does that new xc5000c come with a firmware pre-loaded already?

I've got firmware here that is indicated as being for the xc5300 (i.e.
0x14b4).  That said, I am not sure if it's the same as the original
5000c firmware.  Definitely makes sense to do an I2C dump and compare
the firmware images since using the wrong firmware can damage the
part.

I'm not against an additional #define for the 0x14b4 part ID, but it
shouldn't be accepted upstream until we have corresponding firmware
and have seen the tuner working.  Do you have digital signal lock
working with this device under Linux and the issue is strictly with
part identification?

Devin


-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: XC5000C 0x14b4 status

[Devin Heitmueller]

 It's not new IC. It's XC5000C. Maybe i was interpreted wrong.
 As I have understood, such behaviour can depends from FW version.
 HW vendor says, that with his latest FW he always gets response 0x14b4.

Ah, so you're running a completely different firmware image?  Well in
that case that would explain the different response for the firmware
loaded indication.

 Not a 0x1388. And I think, that these ICs still come without pre-loaded FW.
 HW vendor also didn't says anything about FW pre-load possibility.

Correct.  These are not parts that have any form of default firmware
in their ROM mask (i.e. not like the silabs or micronas parts which
have a default firmware and the ability to patch the ROM via a
software loaded code update).  The firmware must be loaded every time
the chip is brought out of reset or it won't work at all.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: kconf syntax error when doing make menuconfig

[Devin Heitmueller]

 i also get this with make xconfig. just running ./build, however, works

Yeah, I've seen that before.  It's a whitespace bug in the KConfig
file.  Just open it in a text editor, go to the line in question, and
fix the leading whitespace to match all the other entries (If I
recall, you'll actually see it occur in two or three places).

It's a trivial fix - feel free to submit a patch so people don't hit
it in the future.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: DVB-T Receivers Latency

[Devin Heitmueller]

Hi Pier,

On Sun, Mar 22, 2015 at 6:37 PM, pier@libero.it pier@libero.it wrote:
 Hello,

 I was wondering if anyone had any idea of how much latency a DVB-T receiver
 can add, in the path between radiofrequency and the MPEG-TS stream. I have
 100ms of unwanted latency I can't explain in an application I'm developing, so
 I was faulting either the tx modulator or the rx demodulator latency for it. 
 In
 my specific case I'm using a 292e as receiver, that seems to have DSP filters
 in it, and could very well be the cause of this tenth of a second delay.

The receiver itself is unlikely to add any appreciable amount of
latency.  Those devices typically only have a few kilobytes of onboard
SRAM for buffering to the USB host controller, enough to hold a couple
of dozen packets.  For a stream that is measured in Mbit/second, it's
a trivial amount of buffering.  If we're talking about latency
introduced by the DSP doing demodulation, we would be measuring
latency in microseconds, not tens or hundreds of milliseconds.

It's much more likely any buffering you are seeing which is
introducing latency is in either the driver, the DVB core, or the
application itself.  I don't know how you are measuring, but if you're
actually *watching* the video as a means of measuring latency, then
the latency is almost certainly in the MPEG decoder.  If you're using
instrumentation though that looks at the actual MPEG packets, then
it's likely you would have already ruled that out.

 I was going to buy a couple more receivers to measure the differences, but I
 was wondering if anyone had any analytic input before wasting money that way.

I wouldn't bother.  All of the USB devices are going to provide
comparable behavior.  The PCI/PCIe devices if anything are going to
have a higher latency because they might have shared memory for the
ring buffers, and thus can have a much larger FIFO.  There is likely
tuning you can do to reduce the latency, but it's all in the kernel
and software stack, not the DVB-T receiver hardware itself.

If you can describe your methodology for how you are measuring latency
in greater detail, I might be able to point you in the right
direction.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Dynamic video input/output list

[Devin Heitmueller]

 Note however that it is perfectly fine if the driver detects the presence
 of such an onboard header when it is loaded and then only exposes those
 extra inputs if the header is present. It just can't change the list later
 unless do you an rmmod and modprobe of the driver. It's probably what you
 do anyway.

Ah, good point.  In my case I was working with PCIe cards where I had
no expectation that the cable was plugged/unplugged at runtime.

Sorry for the noise.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Dynamic video input/output list

[Devin Heitmueller]

 I'm looking to enhance video input/output enumeration support in
 GStreamer using VIDIOC_ENUMINPUT/VIDIOC_ENUMOUTPUT ioctls and after some
 discussions we wonder if the input/output list can change dynamically at
 runtime or not.

 So, is v4l2 allow this input/output list to be dynamic ?

I sure how the spec allows it, because I've done it in the past.  I
have cards which have an onboard header for external A/V inputs, and I
am able to tell if the breakout cable is attached due to a dedicated
pin tied to a GPIO.  Thus, I am able to dictate whether the card has
the A/V breakout cable attached and thus whether to expose only the
first input or all three inputs.

That said, in this case the inputs in the list never moved around
because the optional entries were at the end of the list - the list
just got longer if those inputs were available.  I'm not sure what
would happen if you had a configuration where you needed to remove
entries other than those at the end of the list.  For example, if you
had a card with four possible inputs and you removed input 2, does the
list stay the same length and input 2 is now marked as invalid, or
does the length of the list become 3 and inputs 3 and 4 turn into
inputs 2 and 3?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: au0828 - add vidq busy checks to s_std and s_input

[Devin Heitmueller]

On Fri, Mar 13, 2015 at 10:18 PM, Shuah Khan shua...@osg.samsung.com wrote:
 au0828 s_std and s_input are missing queue busy checks. Add
 vb2_is_busy() calls to s_std and s_input and return -EBUSY
 if found busy.

These checks are only needed on devices which support more than a
single format (typically for devices which support standards for both
480 and 576 lines).  The au0828 only supports 720x480 capture, and
thus there are no conditions in which the capture window size can
change due to a standard change (since the device only supports
NTSC-M).  Hans has made clear in the past that it's permitted to
toggle inputs when we can be confident that there will be no change in
video standards (in fact, devices that are targeted at surveillance
prefer this to minimize the time toggling between multiple cameras).
A bridge that only supports one video standard and thus input frame
size falls into this category.

While a patch like this is appropriate for some bridges, it is not
needed for au0828.

Can you guys kindly please stop trying to cleanup/refactor au0828 and
xc5000?  First the xc5000 regression that caused arbitrary memory
corruption/panics and now this junk with the vbi_buffer_filled (btw,
did the submitter even *TRY* that patch to see if it actually
worked?).

I know patches like this are well intentioned, but poorly tested
patches and cleanup fixes which are completely untested are doing
more harm than good.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] xc5000: fix memory corruption when unplugging device

[Devin Heitmueller]

 I would request you to add a comment here indicating the
 hybrid case scenario to avoid any future cleanup type work
 deciding there is no need to set priv-firmware to null
 since priv gets released in hybrid_tuner_release_state(priv);

No, I'm not going to rebase my tree and regenerate the patch just to
add a comment explaining how hybrid_tuner_[request/release]_state()
works (which, btw, is how it works in all hybrid tuner drivers).  I
already wasted enough of my time tracking down the source of the
memory corruption and providing a fix for this regression.  If you
want to submit a subsequent patch with a comment, be my guest.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] xc5000: fix memory corruption when unplugging device

[Devin Heitmueller]

 These are just the issues I would like to implement drivers as standard I2C
 driver model =) Attaching driver for one chip twice is ugly hack!

While I'm not arguing the merits of using the standard I2C driver
model, it won't actually help in this case since we would still need a
structure representing shared state accessible by both the DVB and
V4L2 subsystems.  And that struct will need to be referenced counted,
which is exactly what hybrid_tuner_request_state() does.

In short, what you're proposing doesn't have any relevance to this
case - it just moves the problem to some other mechanism for sharing
private state between two drivers and having to reference count it.
And at least in this case it's done the same way for all the tuner
drivers (as opposed to different tuners re-inventing their own
mechanism for sharing state between the different consumers).

If you ever get around to implementing support for a hybrid device
(where you actually have to worry about how both digital and analog
share a single tuner), you'll appreciate some of these challenges and
why what was done was not as bad/crazy as you might think.

If anything, this little regression is yet another point of evidence
that innocent refactoring and cleanup of existing code without
really understanding what it does and/or performing significant
testing can leave everybody worse off than if the well-intentioned
committer had done nothing at all.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] xc5000: fix memory corruption when unplugging device

[Devin Heitmueller]

This patch addresses a regression introduced in the following patch:

commit 5264a522a597032c009f9143686ebf0fa4e244fb
Author: Shuah Khan shua...@osg.samsung.com
Date:   Mon Sep 22 21:30:46 2014 -0300
[media] media: tuner xc5000 - release firmwware from xc5000_release()

The priv struct is actually reference counted, so the xc5000_release()
function gets called multiple times for hybrid devices.  Because
release_firmware() was always being called, it would work fine as expected
on the first call but then the second call would corrupt aribtrary memory.

Set the pointer to NULL after releasing so that we don't call
release_firmware() twice.

This problem was detected in the HVR-950q where plugging/unplugging the
device multiple times would intermittently show panics in completely
unrelated areas of the kernel.

Signed-off-by: Devin Heitmueller dheitmuel...@kernellabs.com
Cc: Shuah Khan shua...@osg.samsung.com
---
 drivers/media/tuners/xc5000.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index 40f9db6..74b2092 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -1314,7 +1314,10 @@ static int xc5000_release(struct dvb_frontend *fe)
 
if (priv) {
cancel_delayed_work(priv-timer_sleep);
-   release_firmware(priv-firmware);
+   if (priv-firmware) {
+   release_firmware(priv-firmware);
+   priv-firmware = NULL;
+   }
hybrid_tuner_release_state(priv);
}
 
-- 
1.9.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: [PATCHv4 15/25] [media] tuner-core: properly initialize media controller subdev

[Devin Heitmueller]

 Except for PVR-500, I can't remember any case where the same tuner is used
 more than once.

 There is the case of a device with two tuners, one for TV and another one
 for FM. Yet, on such case, the name of the FM tuner will be different,
 anyway. So, I don't think this is a current issue, but if the name should
 be unique, then we need to properly document it.

Perhaps I've misunderstood the comment, but HVR-2200/2250 and numerous
dib0700 designs are dual DVB tuners.  Neither are like the PVR-500 in
that they are a single entity with two tuners (as opposed to the
PVR-500 which is two PCI devices which happen to be on the same PCB).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: [PATCHv4 15/25] [media] tuner-core: properly initialize media controller subdev

[Devin Heitmueller]

Hi Hans,

On Mon, Feb 16, 2015 at 9:46 AM, Hans Verkuil hverk...@xs4all.nl wrote:
 On 02/16/2015 03:39 PM, Devin Heitmueller wrote:
 Except for PVR-500, I can't remember any case where the same tuner is used
 more than once.

 There is the case of a device with two tuners, one for TV and another one
 for FM. Yet, on such case, the name of the FM tuner will be different,
 anyway. So, I don't think this is a current issue, but if the name should
 be unique, then we need to properly document it.

 Perhaps I've misunderstood the comment, but HVR-2200/2250 and numerous
 dib0700 designs are dual DVB tuners.  Neither are like the PVR-500 in
 that they are a single entity with two tuners (as opposed to the
 PVR-500 which is two PCI devices which happen to be on the same PCB).

 DVB, yes, but not analog (V4L2) tuners. For DVB tuners the frontend name
 is used as the entity name, which I assumed is unique. It is, right? If
 that's not unique, then the same issue is there as well. I have ordered
 a dual DVB-T board, but I won't have that for another two weeks.

Sorry, I thought this patch was related to the DVB tuner subdev -
didn't realize it was for tuner-core (which is obviously analog).

The HVR-2200/2250 is a single board with dual hybrid tuners (model
2200 has a DVB-T demod and 2250 has an ATSC demod).  In other words,
it has two tuners, each of which can be independently tuned to either
digital or analog signals.  And unlike the PVR-500, it's a single PCIe
bridge (saa7164).

Regarding your question on DVB tuners, yes - each tuner gets its own
frontendX device node (in reality the frontendX points to the digital
demodulator, but ultimately it passes the tuning request off to the
tuner as well).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Fix ALSA and DVB representation at media controller API

[Devin Heitmueller]

 It is actually trivial to get the device nodes once you have the
 major/minor. The media-ctl library does that for you. See:

No objection then.

On a related note, you would be very well served to consider testing
your dvb changes with a device that has more than one DVB tuner (such
as the hvr-2200/2250).  That will help you shake out any edge cases
related to ensuring that the different DVB nodes appear in different
groups.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Fix ALSA and DVB representation at media controller API

[Devin Heitmueller]

 For media-ctl, it is easier to handle major/minor, in order to identify
 the associated devnode name. Btw, media-ctl currently assumes that all
 devnode devices are specified by v4l.major/v4l.minor.

I suspect part of the motivation for the id that corresponds to the
adapter field was to make it easier to find the actual underlying
device node.  While it's trivial to convert a V4L device node from
major/minor to the device node (since for major number is constant and
the minor corresponds to the X in /dev/videoX), that's tougher with
DVB adapters because of the hierarchical nature of the DVB device
nodes.  Having the adapter number makes it trivial to open
/dev/dvb/adapterX.

Perhaps my POSIX is rusty -- is there a way to identify the device
node based on major minor without having to traverse the entire /dev
tree?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: au0828 - convert to use videobuf2

[Devin Heitmueller]

Hi Shuah,

 TRY_FMT and S_FMT both don't handle invalid pixelformats. Looks like
 there is reason behind this based on the comments:

  /* format-fmt.pix.width only support 720 and height 480 */
 if (width != 720)
 width = 720;
 if (height != 480)
 height = 480;

This actually isn't a bug, and in fact I believe the v4l2-compliance
tool prints out a URL to a thread you should read.  It has to do with
the HVR-950q hardware delivering UYVY video and the behavior being
unspecified for how non-supported pixel formats should be handled
until relatively recently.  As a result, it behaves that way so apps
like tvtime don't break due to expecting legacy behavior.

It has nothing to do with the resolution - in fact the driver is doing
exactly what it is supposed to (if you provide an unsupported
resolution, the driver is supposed to pass back a resolution that is
good and still return success).  It's the application's responsibility
to look at the resolution in the struct after the ioctl call and make
sure it hasn't changed (and if it has, the app should adjust its
expectations accordingly).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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 2/3] media: au0828 - convert to use videobuf2

[Devin Heitmueller]

 -   fh-type = type;
 -   fh-dev = dev;
 -   v4l2_fh_init(fh-fh, vdev);
 -   filp-private_data = fh;
 +   dprintk(1,
 +   %s called std_set %d dev_state %d stream users %d users 
 %d\n,
 +   __func__, dev-std_set_in_tuner_core, dev-dev_state,
 +   dev-streaming_users, dev-users);

 -   if (mutex_lock_interruptible(dev-lock)) {
 -   kfree(fh);
 +   if (mutex_lock_interruptible(dev-lock))
 return -ERESTARTSYS;
 +
 +   ret = v4l2_fh_open(filp);
 +   if (ret) {
 +   au0828_isocdbg(%s: v4l2_fh_open() returned error %d\n,
 +   __func__, ret);
 +   mutex_unlock(dev-lock);
 +   return ret;
 }
 +
 if (dev-users == 0) {

 you can use v4l2_fh_is_singular_file() and get rid of users member ?

That won't work because the underlying resources are shared between
/dev/videoX and /dev/vbiX device nodes.  Hence if you were to move to
v4l2_fh_is_singular_file(), the video device would get opened, the
stream would get reset, the VBI device would get opened, and that
would cause the analog stream to get enabled/reset *again*.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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 3/3] media: au0828 remove video and vbi buffer timeout work-around

[Devin Heitmueller]

 I couldn't reproduce what I was seeing when I did patch v2 series
 work. What I noticed was that I was seeing a few too many green screens
 and I had to re-tune xawtv when the timeout code is in place. My
 thinking was that this timeout handling could be introducing blank
 green frames when there is no need. However, I can't reproduce the
 problem on 3.19-rc4 base which is what I am using to test the changes
 to the patch series. Hence, I am not positive if the timeout code
 indeed was doing anything bad.

IIRC, the timer was set for 40ms, so if a complete video frame doesn't
arrive within that interval we generate a green frame.  It was never
really intended to have perfect clocking (i.e. 29.97 FPS), but is
really just there to prevent the tvtime user interface from blocking
indefinitely.

If you weren't seeing it in the V2 series, then I guess you fixed
whatever bug was present in V1.

 I am seeing tvtime hangs without the timeout. I am fine with this
 patch not going. It does make the code cleaner and also reduces
 buffer handling during streaming. However, there is a clear regression
 to tvtime.

Correct.  I think everybody agrees that the timer code is ugly and it
would be cleaner if it wasn't needed - except it clearly is needed to
prevent regressions in tvtime.

All that said, I'm quite excited to see the driver converted to VB2.  Nice job!

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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 3/3] media: au0828 remove video and vbi buffer timeout work-around

[Devin Heitmueller]

Hi Shuah,

On Mon, Jan 12, 2015 at 9:56 PM, Shuah Khan shua...@osg.samsung.com wrote:
 au0828 does video and vbi buffer timeout handling to prevent
 applications such as tvtime from hanging by ensuring that the
 video frames continue to be delivered even when the ITU-656
 input isn't receiving any data. This work-around is complex
 as it introduces set and clear timer code paths in start/stop
 streaming, and close interfaces. However, tvtime will hang
 without the following tvtime change:

I'm confused.  When we last debated whether this patch would be
accepted, the last message from Mauro said the following:

 That means that we'll need to keep holding such timeout code for
 years, until all distros update to a new tvtime, of course assuming
 that this is the only one application with such issue.

In other words, the timeout code has to stay in there since otherwise
it will cause ABI breakage.  It's great that Hans has submitted a
patch to improve tvtime, and over the next couple of years that patch
might actually start to appear in distributions.  That unfortunately
doesn't change the fact that everybody who updates their kernel (or
has it updated for them as part of their distribution) will go from
works fine to completely broken.

The driver was working before the VB2 conversion, so if there is now
instability then it's likely that some bug was introduced during the
conversion to VB2.  Simply ripping out the timeout code seems like the
wrong approach to addressing a regression likely caused by your own
VB2 conversion patch.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: [REVIEW] au0828-video.c

[Devin Heitmueller]

 No, tvtime no longer hangs if no frames arrive, so there is no need for
 this timeout handling. I'd strip it out, which can be done in a separate
 patch.

Did you actually try it? Do you have some patches to tvtime which
aren't upstream?

I wrote the comment in question (and added the associated code).  The
issue is that tvtime does *everything* in a single thread (except the
recent ALSA audio work), that includes servicing the video/vbi devices
as well as the user interface.  That thread blocks on a DQBUF ioctl
until data arrives, and thus if frames are not being delivered it will
hang the entire tvtime user interface.

Now you can certainly argue that is a bad design decision, but it's
been that way for 15+ years, so we can't break it now.  Hence why I
generate dummy frames on a timeout if the decoder isn't delivering
video.  Unfortunately the au8522 doesn't have a free running mode
(i.e. blue screen if no video), which is why most of the other devices
work fine (decoders by Conexant, NXP, Trident, etc all have such
functionality).

Don't get me wrong - I *hate* that I had to put that timer crap in the
driver, but it was necessary to be compatible with one of the most
popular applications out there.

In short, that code cannot be removed.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: [REVIEW] au0828-video.c

[Devin Heitmueller]

 In short, that code cannot be removed.

 Sure it can. I just tried tvtime and you are right, it blocks the GUI.
 But the fix is very easy as well. So now I've updated tvtime so that
 it timeouts and gives the GUI time to update itself.

That's a nice change to tvtime and I'm sure it will make it more robust.

 No more need for such an ugly hack in au0828. The au0828 isn't the only
 driver that can block, others do as well. Admittedly, they aren't very
 common, but they do exist. So it is much better to fix the application
 than adding application workarounds in the kernel.

You're breaking the ABI.  You're making a change to the kernel that
causes existing applications to stop working.  Sure you can make the
argument that applications probably never should have expected such
behavior (even if it's relied on that behavior for 15+ years).  And
sure, you can make a change to the application in some random git
repository that avoids the issue, and that change might get sucked in
to the major distributions over the next couple of years.  That
doesn't change the fact that you're breaking the ABI and everybody who
has the existing application that updates their kernel will stop
working.

Please don't do this.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: [REVIEW] au0828-video.c

[Devin Heitmueller]

 As we've discussed on IRC channel, it would be good to add support
 for format enumeration on it, but the changes don't seem to be
 trivial. I'm not willing to do it, due to my lack of time, but,
 if someone steps up for doing that, then we can wait for those
 patches before bumping the version.

I agree that format enumeration will be a PITA - I looked at doing it
myself a couple of years ago.  Much of the problems are related to
limitations in the home-grown widget toolkit that tvtime uses.  I've
also got patches to support HD resolutions (i.e. HDMI capture) which
we use internally, but haven't felt they were worthwhile to upstream
since they don't depend on the ENUM_FRAMESIZES/FRAMEINTERVALS.  If
somebody feels the urge to put some time into it, I can make available
the patches.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Kernel 3.17.0 broke xc4000-based DTV1800h

[Devin Heitmueller]

 My understanding is that it covers that specific firmware file.

In this case the license was actually against the header file.  I was
responsible for generating the binary blob based on the header files
in the Xceive sources (and got the appropriate permission).  There
should be no licensing issue generating a second blob that includes
the analog standards (I just never got around to it because I didn't
analog support and hence couldn't test the result).

If somebody wants to send me an updated blob, I'm happy to host a copy
at kernellabs.com alongside the file that is currently there (please
make sure it has a different filename though).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: ISDB caption support

[Devin Heitmueller]

 With regards to CC decoding, IMHO, the best would be to add a parser
 for ISDB CC at libdvbv5.

It probably makes more sense to extend one of the existing libraries
that supports captions/subtitles to include support for ISDB (such as
libzvbi or ccextractor).  The libdvbv5 library has no infrastructure
today for subtitle rendering for any other formats, so generating a
generic caption/subtitle API within libdvbv5 that is extensible enough
to support other formats seems redundant.  It also means that
applications that already use libzvbi will get the support for ISDB
effectively for free (in fact, I'm considering moving VLC over to
using libzvbi for CC rendering - it's already used today for raw VBI
slicing).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: ISDB caption support

[Devin Heitmueller]

Hi David,

ISDB-T subtitles are done in a similar manner to DVB-T subtitles -
there is a PID in the stream which contains the subtitle data, which
needs to be decoded by the application (just as you would handle DVB-T
subtitles or ATSC closed captions).  It's entirely an application
level function, having nothing to do with the driver layer.

In short, this has nothing to do with DVBv5, as that is all about how
the tuner is controlled, not what gets done with the resulting MPEG
stream.  You would need to talk to whoever is responsible for the
application you are working with (whether that be VLC, mplayer,
ccextractor, etc).

Cheers,

Devin

On Fri, Nov 28, 2014 at 2:55 PM, David Liontooth lionte...@cogweb.net wrote:

 What is the status of ISDB-Tb / ISDB-T International / ISDB Japanese closed
 captioning support?

 If anyone is working on this, please get in touch -- we're particularly
 interested in getting Brazilian SBTVD working.

 I see Mauro has been working on DVBv5 support, but does this include
 captioning?

 Cheers,
 David
 --
 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



-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: ISDB caption support

[Devin Heitmueller]

 I realize captions is an application-layer function, and intend to work with
 the CCExtractor team. Do any other applications already have ISDB caption
 support?

Based on a Google search, it looks like dvbviewer can decode them:

http://www.dvbviewer.tv/forum/topic/41933-brazilian-terrestrial-isdb-tb-subtitles-closed-caption/
http://www.dvbviewer.com/en/index.php

It's not open source, and it's not Linux, but at least it may give you
something to compare against if you want to build the functionality
yourself.

 For DVB and ATSC there's quite a bit of code written by several people for
 teletext and captions -- has anything at all been done for ISDB captions?

Not to my knowledge.  I've done a ton of work with CC decoding in VLC,
but haven't poked around at the other formats.

 It's used in nearly all of Central and South America, plus the Philippines
 and of course Japan -- you would have thought someone has started on the
 task?

From what I understand, most terrestrial TV in Japan is encrypted, so
you're likely to not find many open source solutions which targeted at
that market.  Presumably there is less of that in Brazil (why else
would Mauro be doing all that ISDB-T work if there was no way to watch
the actual video?).

 We're looking for a good solution for capturing television in Brazil, when
 the signal is encrypted -- are there set-top boxes or tv capture cards that
 handle the decryption so that the decoded signal is passed on with the
 ISDB-Tb caption stream intact?

This would be very unusual.  Satellite captioning often has the same
issues - the decoders only support overlaying the captions over the
video and provide no means to access the underlying data.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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 2/2] [media] sound: Update au0828 quirks table

[Devin Heitmueller]

Hi Mauro,

 Syncronize it and put them on the same order as found at au0828
 driver, as all the au0828 devices with analog TV need the
 same quirks.

The MXL and Woodbury boards don't support analog under Linux, so
probably shouldn't be included in the list of quirks.

That said, the MXL and Woodbury versions of the PCBs were prototypes
that never made it into production (and since the Auvitek chips are
EOL, they never will).  I wouldn't object to a patch which removed the
board profiles entirely in the interest of removing dead code.

It was certainly nice of Mike Krufky to work to get support into the
open source driver before the product was released, but after four
years it probably makes sense to remove the entries for products that
never actually shipped.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] xc5000: tuner firmware update

[Devin Heitmueller]

 Well, perhaps you could add a printk message warning the user that
 the driver is not using the latest firmware and performance/quality
 could be badly affected.

I wouldn't use the term badly affected.  There are tens of thousands
of units out there for which users are quite happy with the current
firmware.  The firmware fixes an edge case that affected a very small
subset of users when receiving signals from a specific QAM modulator
product.  The vast majority of users would be perfectly fine using the
old firmware indefinitely.

That said, I certainly have no objection to a message stating that
there is newer firmware available than what they are currently
running.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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/3] xc5000: add IF output level control

[Devin Heitmueller]

On Sat, Oct 25, 2014 at 4:17 PM, Michael Krufky mkru...@hotmail.com wrote:
 From: Richard Vollkommer li...@hauppauge.com

 Adds control of the IF output level to the xc5000 tuner
 configuration structure.  Increases the IF level to the
 demodulator to fix failure to lock and picture breakup
 issues (with the au8522 demodulator, in the case of the
 Hauppauge HVR950Q).

 This patch works with all XC5000 firmware versions.

 Signed-off-by: Richard Vollkommer li...@hauppauge.com
 Cc: Devin Heitmueller dheitmuel...@kernellabs.com
 Signed-off-by: Michael Ira Krufky mkru...@linuxtv.org

Reviewed-by: Devin Heitmueller dheitmuel...@kernellabs.com

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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 2/2] [media] sound: Update au0828 quirks table

[Devin Heitmueller]

On Thu, Oct 30, 2014 at 9:37 AM, Mauro Carvalho Chehab
mche...@osg.samsung.com wrote:
 Yeah, if nobody is using such devices, then we should get rid of them,
 but I prefer to have this on a separate patch, in order to give
 people the opportunity to complain.

Yes, I would definitely suggest a separate patch.  There's no reason
it should be mixed in with your general cleanup of the ALSA quirks.

 So, if I'm understanding well, you're suggesting to add a patch
 removing those 5 entries (and the corresponding quirks on alsa),
 right?

 { USB_DEVICE(0x2040, 0x7201),
 .driver_info = AU0828_BOARD_HAUPPAUGE_HVR950Q_MXL },
 { USB_DEVICE(0x2040, 0x7211),
 .driver_info = AU0828_BOARD_HAUPPAUGE_HVR950Q_MXL },
 { USB_DEVICE(0x2040, 0x7281),
 .driver_info = AU0828_BOARD_HAUPPAUGE_HVR950Q_MXL },
 { USB_DEVICE(0x05e1, 0x0480),
 .driver_info = AU0828_BOARD_HAUPPAUGE_WOODBURY },
 { USB_DEVICE(0x2040, 0x8200),
 .driver_info = AU0828_BOARD_HAUPPAUGE_WOODBURY },
 { USB_DEVICE(0x2040, 0x7260),

Exactly.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Devin Heitmueller]

 this seems like a feature, not a bug. PulseAudio starts streaming before
 clients push any data and likewise keeps sources active even after for some
 time after clients stop recording. Closing VLC in your example doesn't
 immediately close the ALSA device. look for module-suspend-on-idle in your
 default.pa config file.

The ALSA userland emulation in PulseAudio is supposed to faithfully emulate
the behavior of the ALSA kernel ABI... except when it doesn't, then it's not
a bug but rather a feature.  :-)

 I also agree that the open/close of the alsa device is the only way to
 control exclusion.

I was also a proponent that we should have fairly coarse locking done
at open/close for the various device nodes (ALSA/V4L/DVB).  The challenge here
is that we have a large installed based of existing applications that
rely on kernel
behavior that isn't formally specified in any specification.  Hence
we're forced to try
to come up with a solution that minimizes the risk of ABI breakage.

If we were doing this from scratch then we could lay down some hard/fast rules
about things apps aren't supposed to do and how apps are supposed to respond
to those exception cases.  Unfortunately we don't have that luxury here.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: [alsa-devel] [PATCH v2 5/6] sound/usb: pcm changes to use media token api

[Devin Heitmueller]

 Sorry, I'm not convinced by that.  If the device has to be controlled
 exclusively, the right position is the open/close.  Otherwise, the
 program cannot know when it becomes inaccessible out of sudden during
 its operation.

I can say that I've definitely seen cases where if you configure a
device as the default capture device in PulseAudio, then pulse will
continue to capture from it even if you're not actively capturing the
audio from pulse.  I only spotted this because I had a USB analyzer on
the device and was dumbfounded when the ISOC packets kept arriving
even after I had closed VLC.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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/5] media: v4l2-core changes to use media tuner token api

[Devin Heitmueller]

Hello Shuah,

 What about G_INPUT and G_TUNER?  Consider the following use case, which is
 entirely legal in the V4L2 API:

 Did you mean G_INPUT and G_STD here? I didn't see G_TUNER mentioned
 below in the use-case.

It can be either ENUM_INPUT or G_TUNER.  Both return status
information that requires communication with the video decoder chip
and/or tuner.  It's probably worth mentioning that ENUMINPUT isn't
like the other ENUM_ calls in that it doesn't return a static list
without  talking to the driver - it contains a field (called .status)
which actually needs to talk to the hardware in order to populate it.

 1.  Program opens /dev/video0
 2.  Program calls G_INPUT/G_STD and sees that the appropriate input and
 standard are already set, since all devices have a default input at
 initialization
 3.  Program never calls S_INPUT, S_STD, or S_TUNER
 4.  Program goes into a loop calling ENUM_INPUT, waiting until it returns
 the input as having signal lock
 5.  When signal lock is seen, program calls STREAMON.

 I am missing vb2 streamon change to hold the tuner in this patch set.
 Without that change vb2 work isn't complete. Unfortunately I don't
 have hybrid hardware that uses a vb2 driver.

I don't think you quite understood my concern.  The concern is that in
the use case above I'm actively using the tuner *before*
VIDIOC_STREAMON is called.  Hence from a locking standpoint you
probably don't want to allow the DVB device to take control of the
tuner.


 In the above case, you would be actively using the au8522 video decoder but
 not holding the lock, so thr DVB device can be opened and screw everything
 up.  Likewise if the DVB device were in use and such a program were run, it
 wouls break.


 I think this use-case will be covered with changes to vb2 streamon
 to check and hold tuner. I am thinking it might not be necessary to
 change g_tuner, g_std, g_input and enum_input at v4l2-core level.
 Does that sounds right??

The more I think about it, the less confident I am that you actually
can take a fine-grained locking approach without adding additional
ioctls to make it explicit.  When is the tuner unlocked?  Is it when
the filehandle is closed?  If so, then the the following script would
behave in an unexpected manner:

#!/bin/sh

while [ 1 ]; do
  v4l2-ctl -n
  # Some code to parse the output and see if the status field for
current input shows no signal
  # If status shows as locked break
done
v4l2-ctl --stream-mmap=500 --stream-to=/tmp/foo.bin

In the above case I'm actively using the tuner but not holding the
lock most of the time, so a separate process can grab the DVB device
between calls to v4l2-ctl -n.

However if you're keeping the device locked until STREAMOFF, then
you'll break all sorts of applications which might just close the
filehandle without calling STREAMOFF, and hence you'll have cases
where the tuner is left locked in analog mode *forever*, preventing
apps from using it in digital mode.

Without adding a new ioctl to lock/unlock the analog side of the
device, there is no real way to deal with this perfectly legal use
case.  The downside of that of course is that applications would have
to be modified in order for the locking to be used, and the default
would have to be to not do locking in order to preserve backward
compatibility with existing applications.

What other ioctls have we not thought of?  I think there is an
argument to be made that we're being too aggressive in trying to
control the locking based on the ioctls called.  It might make sense
to simplify the approach to lock on when the device is opened, and
unlock when closed.  This avoids the complexity of trying to figure
out *which* ioctls we need to set the lock on (which likely varies on
a per device basis anyway), at the cost of not allowing the device to
be used when something has the filehandle opened for the other side of
the device (the behavior of which is currently undefined in the spec).
I know there are concerns that some apps might leave the FD open even
when done using it, but that seems like a less likely case than
properly handling fine-grained locking (causing unexpected behavior
for the applications that don't expect -EBUSY to be returned after the
device has been successfully opened).

We can always start with coarse locking on open/close, and do finer
grained locking down the road if needed - or simply change the
currently undefined behavior in the spec to say that you have to close
the device handle before attempting to open the other side of the
device.

On a related note, you should be testing with MythTV - none of the
applications you are currently testing with support both analog *and*
digital, so you are not seeing all the race conditions that can occur
when you close one side of the device and then *immediately* open the
other side.  In particular, there is a known race that occurs when
closing the DVB device and then opening the V4L device, because the
DVB frontend shuts down the tuner 

Re: [PATCH] au0828-input: Be sure that IR is enabled at polling

[Devin Heitmueller]

On Thu, Aug 7, 2014 at 9:46 AM, Mauro Carvalho Chehab
m.che...@samsung.com wrote:
 When the DVB code sets the frontend, it disables the IR
 INT, probably due to some hardware bug, as there's no code
 there at au8522 frontend that writes on register 0xe0.

 Fixing it at au8522 code is hard, as it doesn't know if the
 IR is enabled or disabled, and just restoring the value of
 register 0xe0 could cause other nasty effects. So, better
 to add a hack at au0828-input polling interval to enable int,
 if disabled.

 Signed-off-by: Mauro Carvalho Chehab m.che...@samsung.com
 ---
  drivers/media/usb/au0828/au0828-input.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

 diff --git a/drivers/media/usb/au0828/au0828-input.c 
 b/drivers/media/usb/au0828/au0828-input.c
 index 94d29c2a6fcf..b4475706dfd2 100644
 --- a/drivers/media/usb/au0828/au0828-input.c
 +++ b/drivers/media/usb/au0828/au0828-input.c
 @@ -94,14 +94,19 @@ static int au8522_rc_read(struct au0828_rc *ir, u16 reg, 
 int val,
  static int au8522_rc_andor(struct au0828_rc *ir, u16 reg, u8 mask, u8 value)
  {
 int rc;
 -   char buf;
 +   char buf, oldbuf;

 rc = au8522_rc_read(ir, reg, -1, buf, 1);
 if (rc  0)
 return rc;

 +   oldbuf = buf;
 buf = (buf  ~mask) | (value  mask);

 +   /* Nothing to do, just return */
 +   if (buf == oldbuf)
 +   return 0;
 +
 return au8522_rc_write(ir, reg, buf);
  }

 @@ -127,8 +132,11 @@ static int au0828_get_key_au8522(struct au0828_rc *ir)

 /* Check IR int */
 rc = au8522_rc_read(ir, 0xe1, -1, buf, 1);
 -   if (rc  0 || !(buf[0]  (1  4)))
 +   if (rc  0 || !(buf[0]  (1  4))) {
 +   /* Be sure that IR is enabled */
 +   au8522_rc_set(ir, 0xe0, 1  4);

Shouldn't this be a call to au8522_rc_andor()  rather than au8522_rc_set()?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] au0828-input: Be sure that IR is enabled at polling

[Devin Heitmueller]

 Well, au8522_rc_set is defined as:

 #define au8522_rc_set(ir, reg, bit) au8522_rc_andor(ir, (reg), (bit), 
 (bit))

Ah, ok.  It's just a really poorly named macro.  Nevermind then.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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: Troubleshooting problematic DVB-T reception

[Devin Heitmueller]

 I am trying to troubleshoot a (non-linux related) DVB-T issue and I basically
 want to create statistics about both DVB and MPEG framing, errors, corruption,
 missing frames, etc.

 The reason is that I believe there is a problem on the transmitting radio
 tower, RF is fine between the tower and me, but the actual payload (MPEG) is
 somehow bogus, errored or sporadically misses frames (due to backhaul problems
 or whatever).

 If I would be able to create some statistics confirming that I see all the DVB
 frames without any errors, but that the actual DVB payload (MPEG) has some
 problems, I could convince the tower guys to actually fix the issue, instead
 of blaming my antennas.


 So, can anyone suggest a tool or method to troubleshoot this issue further?


 tzap output for example confirms not a single BER error and the tuner keeps
 full LOCK on the channel while the actual stream is stuttering.

I probably wouldn't rely on the BER stats from tzap.  Their
implementation varies in quality depending on which tuner you have, as
well as how they are sampled.  Almost all demods will set the TEI bit
on the MPEG frame if it's determined that there was a decoding error -
I would be much more inclined to look at that.

Your best bet is to record the whole mux for a few minutes, then run
it through some different tools to see what class of errors you are
hitting.  Tools such as tsreader or StreamEye will give you a better
idea what's going on.  Once you know what class of failure you have
(e.g. TEI errors, MPEG discontinuities, etc), then you can better
isolate where in the chain the failure is being introduced.

Having the recording of the mux will also let you analyze in depth the
actual nature of the problem, rather than trying to analyze an
ever-changing stream in real-time, where signal conditions can change
over time.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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] xc5000: add product id of xc5000C

[Devin Heitmueller]

 diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
 index 2b3d514..d1f539c 100644
 --- a/drivers/media/tuners/xc5000.c
 +++ b/drivers/media/tuners/xc5000.c
 @@ -85,6 +85,7 @@ struct xc5000_priv {
  /* Product id */
  #define XC_PRODUCT_ID_FW_NOT_LOADED0x2000
  #define XC_PRODUCT_ID_FW_LOADED0x1388
 +#define XC_PRODUCT_ID_FW_LOADED_5000C  0x14b4

  /* Registers */
  #define XREG_INIT 0x00
 @@ -1344,6 +1345,7 @@ struct dvb_frontend *xc5000_attach(struct dvb_frontend 
 *fe,

 switch (id) {
 case XC_PRODUCT_ID_FW_LOADED:
 +   case XC_PRODUCT_ID_FW_LOADED_5000C:
 printk(KERN_INFO
 xc5000: Successfully identified at address 0x%02x\n,
 cfg-i2c_address);

What is the bridge which interfaces the xc5000?  The XC5000C typically
returns 0x1388 just like the xc5000.  It's much more likely that the
I2C bus is broken on the bridge driver (or the chip is in reset at
this stage), the I2C request is silently failing and you're getting
whatever happens to have been in the buffer.

NACK unless you can produce an I2C bus trace showing those bytes
coming back over the wire.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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

Best way to add subdev that doesn't use I2C or SPI?

[Devin Heitmueller]

Hello,

I'm in the process of adding support for a new video decoder.  However
in this case it's an IP block on a USB bridge as opposed to the
typical case which is an I2C device.  Changing registers for the
subdev is the same mechanism as changing registers in the rest of the
bridge (a specific region of registers is allocated for the video
decoder).

Doing a subdev driver seems like the logical approach to keep the
video decoder related routines separate from the rest of the bridge.
It also allows the reuse of the code if we find other cases where the
IP block is present in other devices.  However I'm not really sure
what the mechanics are for creating a subdev that isn't really an I2C
device.

I think we've had similar cases with the Conexant parts where the Mako
was actually a block on the main bridge (i.e. cx23885/7/8, cx231xx).
But in that case the cx25840 subdev just issues I2C commands and
leverages the fact that you can talk to the parts over I2C even though
they're really on-chip.

Are there any other cases today where we have a subdev that uses
traditional register access routines provided by the bridge driver to
read/write the video decoder registers?  In this case I would want to
reuse the register read/write routines provided by the bridge, which
ultimately are send as USB control messages.

Any suggestions welcome (and in particular if you can point me to an
example case where this is already being done).

Thanks in advance,

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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