I have some problems with tw686x and I need help.

PCIE device driver: tw686x In review Krzysztof Hałasa 2 participants 9 messages May 10, 2017 – Jun 23, 2017 3 messages not in the archive

Subsystems (2)

Media (V4L/DVB) TW686X VIDEO4LINUX driver

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Krzysztof Hałasa

author

2017-05-10 09:58 UTC #1 lore raw

Hello Singh,

I've added linux-media as well as the current TW686x driver maintainer to Cc: list. Perhaps they will be better prepared to help you, the driver differs much from what it was when I originally wrote it.

Quoted 8 lines · <[email protected]> writes:

First, I download source code from website
https://github.com/torvalds/linux/tree/master/drivers/media/pci/tw686x,
build the driver in my project. when i start the machine the machine printk the log
"[ 5.557782] tw6869: PCI 0000:01:00.0, IRQ 170, MMIO 0xc8000000 (memcpy mode)
[ 5.565010] tw686x 0000:01:00.0: enabling device (0000 -> 0002)".
from the log we can see that pcie-bus read the ID is 6869, but out chip is tw6865. this is not our main problem.

The truth is I don't have a real TW6865, I simply assumed its PCI ID would be 0x6865 because it worked this way for TW6864 and TW6869. Could you please do a "lspci -vvn" on the machine with this card and send me (with CC: [email protected]) this command's output (you may omit other devices, I'm only interested in the TW chip information). Perhaps the invalid identification can be fixed.

Thanks.

Quoted 4 lines

After we booted the machine. then we lunch out application to display video that we capture from tw6865. But there are a lot of log , such as
"[ 24.671551] tw686x 0000:01:00.0: video10: unexpected p-b buffer!

Quoted 8 lines

[ 24.671561] tw686x 0000:01:00.0: video11: unexpected p-b buffer!
[ 24.671566] tw686x 0000:01:00.0: video12: unexpected p-b buffer!
[ 24.671570] tw686x 0000:01:00.0: video13: unexpected p-b buffer!"
and the log print always.
those log means tw6865 always reset dma channel.
sometimes we can see the right picture, but the video moves slowly, about 2 frame per 5 seconds. after a few minute, the linux kernel crashed. I post the crash log
below.

Unfortunately there is no meaningful stack dump. Perhaps the crashes will go away when the "unexpected p-b buffer" issue is fixed so I wouldn't worry too much about the dumps at the moment.

Quoted 10 lines

Sometimes, after I booted the machine, and lunched the application. To the begin, we can see a few frame displaying on screen. after a few seconds, the monitor are freezed. and I use "cat /proc/interrupts" to look up tw6865's interrput. and we find that there is no tw6865 interrupt any more. after a few minutes, most probely the machine will crash and reboot(crash log is same with the before which I have posted).
we use command "dmesg" to look up log. we found that there ever happend "tw686x 0000:01:00.0: video10: FIFO error" or "we found that there ever happend" frequently.

This could be caused by the invalid identification (and handling) of the chip.

OTOH, the "memcpy" mode of this driver may be too slow on your machine (ARM usually doesn't have hardware-coherent cache memory and certain cache sync operations are apparently very slow). You may want to try the DMA mode instead (modprobe tw686x dma_mode=contig or sg). Be aware that the DMA modes aren't as flexible as memcpy mode, the frame formats are more limited.

I don't have experience with any ARM64, though.

Also I think the current code will OOPS on any video initialization error (which will most probably happen on ARM in contig mode because ARM machines don't allow that much continuous allocation). At least this can be easily fixed, I'll attach the patch.

Signature · 4 lines

Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Krzysztof Hałasa

author

2017-05-10 09:58 UTC #2 lore raw

PATCH 1/1 TW686x: Fix OOPS on buffer alloc failure

Signed-off-by Krzysztof Hałasa <[email protected]>

drivers/media/pci/tw686x/tw686x-video.c +7 -3

index c3fafa9..d637f47 100644
@@ -1190,6 +1190,13 @@ int tw686x_video_init(struct tw686x_dev *dev)
 			return err;
 	}
 
+	/* Initialize vc->dev and vc->ch for the error path first */
+	for (ch = 0; ch < max_channels(dev); ch++) {
+		struct tw686x_video_channel *vc = &dev->video_channels[ch];
+		vc->dev = dev;
+		vc->ch = ch;
+	}
+
 	for (ch = 0; ch < max_channels(dev); ch++) {
 		struct tw686x_video_channel *vc = &dev->video_channels[ch];
 		struct video_device *vdev;

@@ -1198,9 +1205,6 @@ int tw686x_video_init(struct tw686x_dev *dev)
 		spin_lock_init(&vc->qlock);
 		INIT_LIST_HEAD(&vc->vidq_queued);
 
-		vc->dev = dev;
-		vc->ch = ch;
-
 		/* default settings */
 		err = tw686x_set_standard(vc, V4L2_STD_NTSC);
 		if (err)

Ezequiel Garcia

maintainer

2017-05-10 16:18 UTC #3 lore raw

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

Hi Krzysztof,

Quoted 7 lines · Krzysztof Hałasa

Signed-off-by Krzysztof Hałasa <[email protected]>
drivers/media/pci/tw686x/tw686x-video.c +0 -0
index c3fafa9..d637f47 100644

drivers/media/pci/tw686x/tw686x-video.c int tw686x_video_init(struct tw686x_dev *dev) · line 1199

Quoted 10 lines

drivers/media/pci/tw686x/tw686x-video.c +7 -0

@@ -1190,6 +1190,13 @@ int tw686x_video_init(struct tw686x_dev *dev)
                        return err;
        }
 
+       /* Initialize vc->dev and vc->ch for the error path first */
+       for (ch = 0; ch < max_channels(dev); ch++) {
+               struct tw686x_video_channel *vc = &dev->video_channels[ch];
+               vc->dev = dev;
+               vc->ch = ch;
+       }
+

I'm not sure where is the oops this commit fixes, care to explain it to me?

drivers/media/pci/tw686x/tw686x-video.c int tw686x_video_init(struct tw686x_dev *dev) · line 1210

Quoted 13 lines

drivers/media/pci/tw686x/tw686x-video.c +0 -3

        for (ch = 0; ch < max_channels(dev); ch++) {
                struct tw686x_video_channel *vc = &dev->video_channels[ch];
                struct video_device *vdev;

@@ -1198,9 +1205,6 @@ int tw686x_video_init(struct tw686x_dev *dev)
                spin_lock_init(&vc->qlock);
                INIT_LIST_HEAD(&vc->vidq_queued);
 
-               vc->dev = dev;
-               vc->ch = ch;
-
                /* default settings */
                err = tw686x_set_standard(vc, V4L2_STD_NTSC);
                if (err)

Thanks,

Signature · 2 lines

Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Krzysztof Hałasa

author

2017-05-11 07:41 UTC #4 lore raw

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

Quoted 9 lines · Ezequiel Garcia

drivers/media/pci/tw686x/tw686x-video.c int tw686x_video_init(struct tw686x_dev *dev) · line 1199
Quoted 7 lines
drivers/media/pci/tw686x/tw686x-video.c +7 -0
+       /* Initialize vc->dev and vc->ch for the error path first */
+       for (ch = 0; ch < max_channels(dev); ch++) {
+               struct tw686x_video_channel *vc = &dev->video_channels[ch];
+               vc->dev = dev;
+               vc->ch = ch;
+       }
+
I'm not sure where is the oops this commit fixes, care to explain it to me?

The error path apparently calls tw686x_video_free() which requires vc->dev to be initialized. Now, the vc->dev is set for the channel being currently initialized (unsuccesfully), but not for ones which haven't been initialized yet. tw686x_video_free() iterates over the whole set.

It seems it also happens in "memcpy" mode. I didn't test it before since on my ARMv7 "memcpy" mode is unusable, it's way too slow. Also, does the driver attempt to use consistent memory for entire buffers in this mode? This may work on i686/x86_64 because the caches are coherent by design and there is no difference between consistent and non-consistent RAM (if one isn't using SWIOTLB etc).

tw6869: PCI 0000:07:00.0, IRQ 24, MMIO 0x1100000 (memcpy mode) tw686x 0000:07:00.0: enabling device (0140 -> 0142) tw686x 0000:07:00.0: dma0: unable to allocate P-buffer

Unable to handle kernel NULL pointer dereference at virtual address 00000000
PC is at _raw_spin_lock_irqsave+0x10/0x4c
LR is at tw686x_memcpy_dma_free+0x1c/0x124
pc : [<805a8b14>]    lr : [<7f04a3c0>]    psr: 20010093

sp : be915c80 ip : 00000000 fp : bea1b000
r10: 00000000 r9 : fffffff4 r8 : 0000b000
r7 : 00000000 r6 : 000003f0 r5 : 00000000 r4 : bf0e21f8 r3 : 7f04a3a4 r2 : 00000000 r1 : 00000000 r0 : 20010013
Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 4e91804a DAC: 00000051 Process udevd (pid: 88, stack limit = 0xbe914210) (_raw_spin_lock_irqsave) from (tw686x_memcpy_dma_free+0x1c/0x124) (tw686x_memcpy_dma_free) from (tw686x_video_free+0x50/0x78) (tw686x_video_free) from (tw686x_video_init+0x478/0x5e8) (tw686x_video_init) from (tw686x_probe+0x36c/0x3fc) (tw686x_probe) from (pci_device_probe+0x88/0xf4) (pci_device_probe) from (driver_probe_device+0x238/0x2d8) (driver_probe_device) from (__driver_attach+0xac/0xb0) (__driver_attach) from (bus_for_each_dev+0x6c/0xa0) (bus_for_each_dev) from (bus_add_driver+0x1a0/0x218) (bus_add_driver) from (driver_register+0x78/0xf8) (driver_register) from (do_one_initcall+0x40/0x168) (do_one_initcall) from (do_init_module+0x60/0x3a4) (do_init_module) from (load_module+0x1c90/0x20e4) (load_module) from (SyS_finit_module+0x8c/0x9c) (SyS_finit_module) from (ret_fast_syscall+0x0/0x3c)

Code: e1a02000 e10f0000 f10c0080 f592f000 (e1923f9f)

With the patch:
tw6869: PCI 0000:07:00.0, IRQ 24, MMIO 0x1100000 (memcpy mode) tw686x 0000:07:00.0: enabling device (0140 -> 0142) tw686x 0000:07:00.0: dma0: unable to allocate P-buffer tw686x 0000:07:00.0: can't register video
tw686x: probe of 0000:07:00.0 failed with error -12 --
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Ezequiel Garcia

maintainer

2017-05-11 16:04 UTC #5 lore raw

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

Quoted 12 lines · Krzysztof Hałasa

Quoted 9 lines · Ezequiel Garcia
Review
Quoted 7 lines
(quoted diff) +7 -0
+       /* Initialize vc->dev and vc->ch for the error path first */
+       for (ch = 0; ch < max_channels(dev); ch++) {
+               struct tw686x_video_channel *vc = &dev->video_channels[ch];
+               vc->dev = dev;
+               vc->ch = ch;
+       }
+
I'm not sure where is the oops this commit fixes, care to explain it to me?

Quoted 8 lines

The error path apparently calls tw686x_video_free() which requires vc->dev to be initialized. Now, the vc->dev is set for the channel being currently initialized (unsuccesfully), but not for ones which haven't been initialized yet. tw686x_video_free() iterates over the whole set.
It seems it also happens in "memcpy" mode. I didn't test it before since on my ARMv7 "memcpy" mode is unusable, it's way too slow. Also, does the driver attempt to use consistent memory for entire buffers in this mode?

Yes, memcpy mode allocates a couple P and B dma-coherent bounce buffers for each channel.

Quoted 36 lines

This may work on i686/x86_64 because the caches are coherent by design and there is no difference between consistent and non-consistent RAM (if one isn't using SWIOTLB etc).
tw6869: PCI 0000:07:00.0, IRQ 24, MMIO 0x1100000 (memcpy mode) tw686x 0000:07:00.0: enabling device (0140 -> 0142) tw686x 0000:07:00.0: dma0: unable to allocate P-buffer
Unable to handle kernel NULL pointer dereference at virtual address 00000000
PC is at _raw_spin_lock_irqsave+0x10/0x4c
LR is at tw686x_memcpy_dma_free+0x1c/0x124
pc : [<805a8b14>]    lr : [<7f04a3c0>]    psr: 20010093
sp : be915c80 ip : 00000000 fp : bea1b000
r10: 00000000 r9 : fffffff4 r8 : 0000b000
r7 : 00000000 r6 : 000003f0 r5 : 00000000 r4 : bf0e21f8 r3 : 7f04a3a4 r2 : 00000000 r1 : 00000000 r0 : 20010013
Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 4e91804a DAC: 00000051 Process udevd (pid: 88, stack limit = 0xbe914210) (_raw_spin_lock_irqsave) from (tw686x_memcpy_dma_free+0x1c/0x124) (tw686x_memcpy_dma_free) from (tw686x_video_free+0x50/0x78) (tw686x_video_free) from (tw686x_video_init+0x478/0x5e8) (tw686x_video_init) from (tw686x_probe+0x36c/0x3fc) (tw686x_probe) from (pci_device_probe+0x88/0xf4) (pci_device_probe) from (driver_probe_device+0x238/0x2d8) (driver_probe_device) from (__driver_attach+0xac/0xb0) (__driver_attach) from (bus_for_each_dev+0x6c/0xa0) (bus_for_each_dev) from (bus_add_driver+0x1a0/0x218) (bus_add_driver) from (driver_register+0x78/0xf8) (driver_register) from (do_one_initcall+0x40/0x168) (do_one_initcall) from (do_init_module+0x60/0x3a4) (do_init_module) from (load_module+0x1c90/0x20e4) (load_module) from (SyS_finit_module+0x8c/0x9c) (SyS_finit_module) from (ret_fast_syscall+0x0/0x3c)
Code: e1a02000 e10f0000 f10c0080 f592f000 (e1923f9f)

Quoted 6 lines

With the patch:
tw6869: PCI 0000:07:00.0, IRQ 24, MMIO 0x1100000 (memcpy mode) tw686x 0000:07:00.0: enabling device (0140 -> 0142) tw686x 0000:07:00.0: dma0: unable to allocate P-buffer tw686x 0000:07:00.0: can't register video
tw686x: probe of 0000:07:00.0 failed with error -12

Oh, I see. Thanks for the details!

How about this one (untested) ?

(quoted diff) +0 -0

b/drivers/media/pci/tw686x/tw686x-video.c
index c3fafa97b2d0..77b8d2dbd995 100644

drivers/media/pci/tw686x/tw686x-video.c +0 -0

@@ -86,6 +86,9 @@ static void tw686x_memcpy_dma_free(struct

tw686x_video_channel *vc,

        struct pci_dev *pci_dev;
        unsigned long flags;

+ /* Make sure this channel is initialized */
+ if (!dev)

+               return;
        /* Check device presence. Shouldn't really happen! */
        spin_lock_irqsave(&dev->lock, flags);
        pci_dev = dev->pci_dev;

Also, when submitting a patch please make sure you write a commit log. Only trivial patches can have empty commit logs.

The description you made in your last mail is perfect, including the oops details, the backtrace and
the "With the patch" note. Doing a git log --grep="fix.*oops" might give you some ideas on commit logs.

The subject of the patch should be:

"tw686x: Fix oops on buffer alloc failure"

If you git log on tw686x, you'll notice all the
commits use "tw686x" prefix.

When submitting fixes, it's useful to add a "Fixes" tag. See https://static.lwn.net/kerneldoc/process/submitting-patches.html
for details.

Signature · 2 lines

Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Krzysztof Hałasa

author

2017-05-12 04:39 UTC #6 lore raw

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

Quoted 8 lines · Ezequiel Garcia

How about this one (untested) ?
drivers/media/pci/tw686x/tw686x-video.c +0 -0
index c3fafa97b2d0..77b8d2dbd995 100644

drivers/media/pci/tw686x/tw686x-video.c static void tw686x_memcpy_dma_free(struct · line 92

Quoted 7 lines

drivers/media/pci/tw686x/tw686x-video.c +3 -0

@@ -86,6 +86,9 @@ static void tw686x_memcpy_dma_free(struct
 tw686x_video_channel *vc,
        struct pci_dev *pci_dev;
        unsigned long flags;
 
+       /* Make sure this channel is initialized */
+       if (!dev)
+               return;

Whatever you wish. Just make sure it doesn't bomb out by default, when one happens to have such a card in his or her machine.

Signature · 4 lines

Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

A message that is not in the archive (sent off-list or not yet received): [email protected]

Krzysztof Hałasa

author

2017-06-23 08:27 UTC #8 lore raw

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

Quoted 1 line · Hans Verkuil

Any progress on this? I gather I can expect a new patch from someone?

Well, the issue is trivial and very easy to test, though not present on common x86 hw. That patch I've sent fixes it, but I'm not the one who decides.

Signature · 4 lines

Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Ezequiel Garcia

maintainer

2017-06-23 14:52 UTC #9 lore raw

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

Quoted 7 lines · Krzysztof Hałasa

Quoted 1 line · Hans Verkuil
Any progress on this? I gather I can expect a new patch from someone?
Well, the issue is trivial and very easy to test, though not present on common x86 hw. That patch I've sent fixes it, but I'm not the one who decides.

If you can re-submit your patch addressing all the comments, I'd be happy to Ack it.

As it stands, with the wrong subject style and without a commit log, it's a NAK on my side.

Signature · 2 lines

Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

A message that is not in the archive (sent off-list or not yet received): [email protected]

Krzysztof Hałasa

author

2017-05-11 08:02 UTC #11 lore raw

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

Quoted 3 lines · zhaoxuegang

In my opinion, the oops occur to tw686x_free_dma(struct tw686x_video_channel *vc, unsigned int pb). In function tw686x_video_init, if coherent-DMA is not enough, tw686x_alloc_dma will failed. Then tw686x_video_free will be called. but some channel's dev is null（in other words, vc->dev is null）

Exactly.

Signature · 4 lines

Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland