Skip to content

Commit be8dcab

Browse files
andyleisersonStefan Richter
authored andcommitted
firewire: ohci: fix VIA VT6306 video reception
Add quirk for VT6306 wake bit behavior. VT6306 seems to reread the wrong descriptor when the wake bit is written. work around by putting a copy of the branch address in the first descriptor of the block. [Stefan R: This fixes the known broken video reception via gstreamer on VIA VT6306. 100% repeatable testcase: $ gst-launch-0.10 dv1394src \! dvdemux \! dvdec \! xvimagesink with a camcorder or other DV source connected. Likewise for MPEG2-TS reception via gstreamer, e.g. from TV settop boxes. Perhaps this also fixes dv4l on VT6306, but this is as yet untested. Kino, dvgrab or FFADO had not been affected by this chip quirk. Additional comments from Andy:] I've looked into some problems with the wake bit on a vt6306 family chip (1106:3044, rev 46). I used this firewire card in a mythtv setup (ISO receive MPEG2 stream) with Debian 2.6.32 kernels for ~2 years without problems. Since upgrading to 3.2, I've been having problems with the input stream freezing -- input data stops until I restart mythtv (I expect closing and reopening the device would be sufficient). This happens infrequently, maybe one out of 20 recordings. I eventually determined that the problem is more likely to occur if the system is loaded. I isolated the kernel version as the triggering SW factor and then specifically the change from dualbuffer back to packet-per-buffer DMA mode. The possibility that the controller does not properly respond to the wake bit was suggested in https://bugzilla.redhat.com/show_bug.cgi?id=415841, but not proven. Based on the fact that dualbuffer mode worked while packet-per-buffer has trouble, I guessed that upon seeing the wake bit written, the vt6306 controller only checks the branch address in the first descriptor of the block, even if that is not the correct place to look (because the block has multiple descriptors). This theory seems to be correct. When the ISO reception is hung, I am able to resume it by manually writing the branch address to the first descriptor in the block, and then writing the wake bit. I've had luck so far with the attached patch, so I'm including it. It's probably not a complete solution -- I haven't tested transmit modes to see whether they have a similar issue. I doubt that the quirk test is any cheaper than just writing the extra branch address in all cases, but it does reduce the risk of breaking other hardware. [Stefan R: omitted QUIRK_NO_MSI from VT6306 quirks table entry, changed whitespace] Signed-off-by: Andy Leiserson <andy@leiserson.org> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
1 parent 8db4914 commit be8dcab

1 file changed

Lines changed: 35 additions & 3 deletions

File tree

drivers/firewire/ohci.c

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@
6868
#define DESCRIPTOR_BRANCH_ALWAYS (3 << 2)
6969
#define DESCRIPTOR_WAIT (3 << 0)
7070

71+
#define DESCRIPTOR_CMD (0xf << 12)
72+
7173
struct descriptor {
7274
__le16 req_count;
7375
__le16 control;
@@ -149,10 +151,11 @@ struct context {
149151
struct descriptor *last;
150152

151153
/*
152-
* The last descriptor in the DMA program. It contains the branch
154+
* The last descriptor block in the DMA program. It contains the branch
153155
* address that must be updated upon appending a new descriptor.
154156
*/
155157
struct descriptor *prev;
158+
int prev_z;
156159

157160
descriptor_callback_t callback;
158161

@@ -270,14 +273,17 @@ static char ohci_driver_name[] = KBUILD_MODNAME;
270273
#define PCI_DEVICE_ID_TI_TSB12LV22 0x8009
271274
#define PCI_DEVICE_ID_TI_TSB12LV26 0x8020
272275
#define PCI_DEVICE_ID_TI_TSB82AA2 0x8025
276+
#define PCI_DEVICE_ID_VIA_VT630X 0x3044
273277
#define PCI_VENDOR_ID_PINNACLE_SYSTEMS 0x11bd
278+
#define PCI_REV_ID_VIA_VT6306 0x46
274279

275280
#define QUIRK_CYCLE_TIMER 1
276281
#define QUIRK_RESET_PACKET 2
277282
#define QUIRK_BE_HEADERS 4
278283
#define QUIRK_NO_1394A 8
279284
#define QUIRK_NO_MSI 16
280285
#define QUIRK_TI_SLLZ059 32
286+
#define QUIRK_IR_WAKE 64
281287

282288
/* In case of multiple matches in ohci_quirks[], only the first one is used. */
283289
static const struct {
@@ -319,6 +325,9 @@ static const struct {
319325
{PCI_VENDOR_ID_TI, PCI_ANY_ID, PCI_ANY_ID,
320326
QUIRK_RESET_PACKET},
321327

328+
{PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VT630X, PCI_REV_ID_VIA_VT6306,
329+
QUIRK_CYCLE_TIMER | QUIRK_IR_WAKE},
330+
322331
{PCI_VENDOR_ID_VIA, PCI_ANY_ID, PCI_ANY_ID,
323332
QUIRK_CYCLE_TIMER | QUIRK_NO_MSI},
324333
};
@@ -333,6 +342,7 @@ MODULE_PARM_DESC(quirks, "Chip quirks (default = 0"
333342
", no 1394a enhancements = " __stringify(QUIRK_NO_1394A)
334343
", disable MSI = " __stringify(QUIRK_NO_MSI)
335344
", TI SLLZ059 erratum = " __stringify(QUIRK_TI_SLLZ059)
345+
", IR wake unreliable = " __stringify(QUIRK_IR_WAKE)
336346
")");
337347

338348
#define OHCI_PARAM_DEBUG_AT_AR 1
@@ -1157,6 +1167,7 @@ static int context_init(struct context *ctx, struct fw_ohci *ohci,
11571167
ctx->buffer_tail->used += sizeof(*ctx->buffer_tail->buffer);
11581168
ctx->last = ctx->buffer_tail->buffer;
11591169
ctx->prev = ctx->buffer_tail->buffer;
1170+
ctx->prev_z = 1;
11601171

11611172
return 0;
11621173
}
@@ -1221,14 +1232,35 @@ static void context_append(struct context *ctx,
12211232
{
12221233
dma_addr_t d_bus;
12231234
struct descriptor_buffer *desc = ctx->buffer_tail;
1235+
struct descriptor *d_branch;
12241236

12251237
d_bus = desc->buffer_bus + (d - desc->buffer) * sizeof(*d);
12261238

12271239
desc->used += (z + extra) * sizeof(*d);
12281240

12291241
wmb(); /* finish init of new descriptors before branch_address update */
1230-
ctx->prev->branch_address = cpu_to_le32(d_bus | z);
1231-
ctx->prev = find_branch_descriptor(d, z);
1242+
1243+
d_branch = find_branch_descriptor(ctx->prev, ctx->prev_z);
1244+
d_branch->branch_address = cpu_to_le32(d_bus | z);
1245+
1246+
/*
1247+
* VT6306 incorrectly checks only the single descriptor at the
1248+
* CommandPtr when the wake bit is written, so if it's a
1249+
* multi-descriptor block starting with an INPUT_MORE, put a copy of
1250+
* the branch address in the first descriptor.
1251+
*
1252+
* Not doing this for transmit contexts since not sure how it interacts
1253+
* with skip addresses.
1254+
*/
1255+
if (unlikely(ctx->ohci->quirks & QUIRK_IR_WAKE) &&
1256+
d_branch != ctx->prev &&
1257+
(ctx->prev->control & cpu_to_le16(DESCRIPTOR_CMD)) ==
1258+
cpu_to_le16(DESCRIPTOR_INPUT_MORE)) {
1259+
ctx->prev->branch_address = cpu_to_le32(d_bus | z);
1260+
}
1261+
1262+
ctx->prev = d;
1263+
ctx->prev_z = z;
12321264
}
12331265

12341266
static void context_stop(struct context *ctx)

0 commit comments

Comments
 (0)