ieee1934: dv1394 interrupt enabling/disabling broken on big-endian
PATCH 2/2 ieee1934: dv1394 interrupt enabling/disabling broken on big-endian
After annotating the frame structs, this was left:drivers/ieee1394/dv1394.c:2113:23: warning: invalid assignment: |=drivers/ieee1394/dv1394.c:2113:23: left side has type restricted __le32drivers/ieee1394/dv1394.c:2113:23: right side has type intdrivers/ieee1394/dv1394.c:2121:24: warning: invalid assignment: &=drivers/ieee1394/dv1394.c:2121:24: left side has type restricted __le32drivers/ieee1394/dv1394.c:2121:24: right side has type intdrivers/ieee1394/dv1394.c:2123:24: warning: invalid assignment: |=drivers/ieee1394/dv1394.c:2123:24: left side has type restricted __le32drivers/ieee1394/dv1394.c:2123:24: right side has type int
Which looks like a real bug on a big-endian arch as it would set/clear the wrong bit.
--- drivers/ieee1394/dv1394.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
drivers/ieee1394/dv1394.c +4 -4
@@ -2110,17 +2110,17 @@ static void ir_tasklet_func(unsigned long data) f = video->frames[next_i /MAX_PACKETS ]; next = &(f->descriptor_pool[next_i %MAX_PACKETS ]); next_dma = ((unsigned long ) block - (unsigned long ) f->descriptor_pool) + f->descriptor_pool_dma; - next->u.in.il.q[0 ] |=3 <<20 ;/* enable interrupt */ - next->u.in.il.q[2 ] =0 ;/* disable branch */ + next->u.in.il.q[0 ] |=cpu_to_le32 (3 <<20 );/* enable interrupt */ + next->u.in.il.q[2 ] =cpu_to_le32 (0 );/* disable branch */ /* link previous to next */ prev_i = (next_i ==0 ) ? (MAX_PACKETS * video->n_frames -1 ) : (next_i -1 ); f = video->frames[prev_i /MAX_PACKETS ]; prev = &(f->descriptor_pool[prev_i %MAX_PACKETS ]);if (prev_i % (MAX_PACKETS /2 )) { - prev->u.in.il.q[0 ] &= ~(3 <<20 );/* no interrupt */ + prev->u.in.il.q[0 ] &= ~cpu_to_le32 (3 <<20 );/* no interrupt */ }else { - prev->u.in.il.q[0 ] |=3 <<20 ;/* enable interrupt */ + prev->u.in.il.q[0 ] |=cpu_to_le32 (3 <<20 );/* enable interrupt */ } prev->u.in.il.q[2 ] =cpu_to_le32 (next_dma |1 );/* set Z=1 */ wmb ();
Signature · 1 line
1.6.1.rc2.306.ge5d5e
For linux1394-devel to know:
Harvey Harrison wrote at LKML:
Quoted 35 lines
After annotating the frame structs, this was left:
drivers/ieee1394/dv1394.c:2113:23: warning: invalid assignment: |=drivers/ieee1394/dv1394.c:2113:23: left side has type restricted__le32drivers/ieee1394/dv1394.c:2113:23: right side has type intdrivers/ieee1394/dv1394.c:2121:24: warning: invalid assignment: &=drivers/ieee1394/dv1394.c:2121:24: left side has type restricted__le32drivers/ieee1394/dv1394.c:2121:24: right side has type intdrivers/ieee1394/dv1394.c:2123:24: warning: invalid assignment: |=drivers/ieee1394/dv1394.c:2123:24: left side has type restricted__le32drivers/ieee1394/dv1394.c:2123:24: right side has type intWhich looks like a real bug on a big-endian arch as it would set/clear the wrong bit.
Signed-off-by Harvey Harrison <[email protected]>--- drivers/ieee1394/dv1394.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)drivers/ieee1394/dv1394.c +2
-2@@ -2110,17 +2110,17 @@ static void ir_tasklet_func(unsigned long data) f = video->frames[next_i /MAX_PACKETS ]; next = &(f->descriptor_pool[next_i %MAX_PACKETS ]); next_dma = ((unsigned long ) block - (unsigned long ) f->descriptor_pool) + f->descriptor_pool_dma; - next->u.in.il.q[0 ] |=3 <<20 ;/* enable interrupt */ - next->u.in.il.q[2 ] =0 ;/* disable branch */ + next->u.in.il.q[0 ] |=cpu_to_le32 (3 <<20 );/* enable interrupt */ + next->u.in.il.q[2 ] =cpu_to_le32 (0 );/* disable branch */ /* link previous to next */ prev_i = (next_i ==0 ) ? (MAX_PACKETS * video->n_frames -1 ) : (next_i -1 ); f = video->frames[prev_i /MAX_PACKETS ];
drivers/ieee1394/dv1394.c static void ir_tasklet_func(unsigned long data) · line 2126
Quoted 10 lines
drivers/ieee1394/dv1394.c +2
-2prev = &(f->descriptor_pool[prev_i %MAX_PACKETS ]);if (prev_i % (MAX_PACKETS /2 )) { - prev->u.in.il.q[0 ] &= ~(3 <<20 );/* no interrupt */ + prev->u.in.il.q[0 ] &= ~cpu_to_le32 (3 <<20 );/* no interrupt */ }else { - prev->u.in.il.q[0 ] |=3 <<20 ;/* enable interrupt */ + prev->u.in.il.q[0 ] |=cpu_to_le32 (3 <<20 );/* enable interrupt */ } prev->u.in.il.q[2 ] =cpu_to_le32 (next_dma |1 );/* set Z=1 */ wmb ();
Looks like a bug + correct fix indeed. Apparently dv1394 was never used on big endian PCs. Which is good, because we want to phase out dv1394 eventually.
Signature · 3 lines
Stefan Richter -=====-==--- ==-- -===- http://arcgraph.de/sr/
Quoted 35 lines · Stefan Richter
For linux1394-devel to know:
Harvey Harrison wrote at LKML:
Quoted 32 lines
After annotating the frame structs, this was left:
drivers/ieee1394/dv1394.c:2113:23: warning: invalid assignment: |=drivers/ieee1394/dv1394.c:2113:23: left side has type restricted__le32drivers/ieee1394/dv1394.c:2113:23: right side has type intdrivers/ieee1394/dv1394.c:2121:24: warning: invalid assignment: &=drivers/ieee1394/dv1394.c:2121:24: left side has type restricted__le32drivers/ieee1394/dv1394.c:2121:24: right side has type intdrivers/ieee1394/dv1394.c:2123:24: warning: invalid assignment: |=drivers/ieee1394/dv1394.c:2123:24: left side has type restricted__le32drivers/ieee1394/dv1394.c:2123:24: right side has type intWhich looks like a real bug on a big-endian arch as it would set/clear the wrong bit.
Signed-off-by Harvey Harrison <[email protected]>--- drivers/ieee1394/dv1394.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)drivers/ieee1394/dv1394.c +2
-2@@ -2110,17 +2110,17 @@ static void ir_tasklet_func(unsigned long data) f = video->frames[next_i /MAX_PACKETS ]; next = &(f->descriptor_pool[next_i %MAX_PACKETS ]); next_dma = ((unsigned long ) block - (unsigned long ) f->descriptor_pool) + f->descriptor_pool_dma; - next->u.in.il.q[0 ] |=3 <<20 ;/* enable interrupt */ - next->u.in.il.q[2 ] =0 ;/* disable branch */ + next->u.in.il.q[0 ] |=cpu_to_le32 (3 <<20 );/* enable interrupt */ + next->u.in.il.q[2 ] =cpu_to_le32 (0 );/* disable branch */
Quoted 16 lines
drivers/ieee1394/dv1394.c static void ir_tasklet_func(unsigned long data) · line 2126
Quoted 13 lines
drivers/ieee1394/dv1394.c +2
-2/* link previous to next */ prev_i = (next_i ==0 ) ? (MAX_PACKETS * video->n_frames -1 ) : (next_i -1 ); f = video->frames[prev_i /MAX_PACKETS ]; prev = &(f->descriptor_pool[prev_i %MAX_PACKETS ]);if (prev_i % (MAX_PACKETS /2 )) { - prev->u.in.il.q[0 ] &= ~(3 <<20 );/* no interrupt */ + prev->u.in.il.q[0 ] &= ~cpu_to_le32 (3 <<20 );/* no interrupt */ }else { - prev->u.in.il.q[0 ] |=3 <<20 ;/* enable interrupt */ + prev->u.in.il.q[0 ] |=cpu_to_le32 (3 <<20 );/* enable interrupt */ } prev->u.in.il.q[2 ] =cpu_to_le32 (next_dma |1 );/* set Z=1 */ wmb ();Looks like a bug + correct fix indeed. Apparently dv1394 was never used on big endian PCs. Which is good, because we want to phase out dv1394 eventually.
I use dv1394 on a big-endian PPC system (2.6.15 kernel) to watch DV video via xine, and this has always worked fine for me. And this shouldn't be phased out until the equivalent functionality is added to ffmpeg via the new APIs.
-Bill
Quoted 3 lines · Bill Fink
Quoted 2 lines · Stefan Richter
Looks like a bug + correct fix indeed. Apparently dv1394 was never used on big endian PCs.
...
Quoted 2 lines
I use dv1394 on a big-endian PPC system (2.6.15 kernel) to watch DV video via xine, and this has always worked fine for me.
Could you try to apply the patch to your kernel to test it? You can get it in plaintext e.g. from
http://user.in-berlin.de/~s5r6/linux1394/submitted/843-ieee1934-dv1394-interrupt-enabling_disabling-broken-on-big-endian.patch
Quoted 2 lines
And this shouldn't be phased out until the equivalent functionality is added to ffmpeg via the new APIs.
Or perhaps by a dv1394 replacement module which plugs to the V4L2 API.
Signature · 3 lines
Stefan Richter -=====-==--- ==-- -===- http://arcgraph.de/sr/
Quoted 11 lines · Stefan Richter
Quoted 3 lines · Bill Fink
Quoted 2 lines · Stefan Richter
Looks like a bug + correct fix indeed. Apparently dv1394 was never used on big endian PCs.
...
Quoted 2 lines
I use dv1394 on a big-endian PPC system (2.6.15 kernel) to watch DV video via xine, and this has always worked fine for me.
Could you try to apply the patch to your kernel to test it? You can get it in plaintext e.g. from
http://user.in-berlin.de/~s5r6/linux1394/submitted/843-ieee1934-dv1394-interrupt-enabling_disabling-broken-on-big-endian.patch
I had hoped to test this by now, but due to not feeling well and the holidays, I haven't been able to. I still hope to test this later, but I can't say exactly when.
-Happy Holidays to all
-Bill
Quoted 10 lines · Bill Fink
Quoted 8 lines · Stefan Richter
Quoted 3 lines · Bill Fink
Quoted 2 lines · Stefan Richter
Looks like a bug + correct fix indeed. Apparently dv1394 was never used on big endian PCs.
...
Quoted 2 lines
I use dv1394 on a big-endian PPC system (2.6.15 kernel) to watch DV video via xine, and this has always worked fine for me.
Quoted 7 lines
Quoted 3 lines
Could you try to apply the patch to your kernel to test it? You can get it in plaintext e.g. from
http://user.in-berlin.de/~s5r6/linux1394/submitted/843-ieee1934-dv1394-interrupt-enabling_disabling-broken-on-big-endian.patchI had hoped to test this by now, but due to not feeling well and the holidays, I haven't been able to. I still hope to test this later, but I can't say exactly when.
Good news. I finally got a chance to test the patch on my kernel, and live DV viewing using xine still worked fine.
-Thanks
-Bill
Bill Fink wrote on 2008-12-26:
Quoted 2 lines
Good news. I finally got a chance to test the patch on my kernel, and live DV viewing using xine still worked fine.
Thanks a lot. If you don't mind, I'll add "Tested-by: Bill Fink <[email protected]>" to the changelog when I enqueue this for Linus, and add a note that it actually worked for you before as well as after this patch.
Signature · 3 lines
Stefan Richter -=====-==--= ---= ---== http://arcgraph.de/sr/
Quoted 8 lines · Stefan Richter
Bill Fink wrote on 2008-12-26:
Quoted 2 lines
Good news. I finally got a chance to test the patch on my kernel, and live DV viewing using xine still worked fine.
Thanks a lot. If you don't mind, I'll add "Tested-by: Bill Fink <[email protected]>" to the changelog when I enqueue this for Linus, and add a note that it actually worked for you before as well as after this patch.
That's fine. Although I admit to being mystified how it works both before and after the patch, since the cpu_to_le32() calls that were added should result in byte swapping on PPC that wasn't being done before. I guess that either the code paths involved aren't actually being triggered by my xine DV viewing, or there's some fortuitous palindromic setting of bits.
-Bill
Quoted 6 lines · Bill Fink
Although I admit to being mystified how it works both before and after the patch, since the
cpu_to_le32()calls that were added should result in byte swapping on PPC that wasn't being done before. I guess that either the code paths involved aren't actually being triggered by my xine DV viewing, or there's some fortuitous palindromic setting of bits.
I agree. If I had more spare time for these things I would try to find out what's really going on. But since the patch really looks like the right thing to do and doesn't regress, I will send it in without further investigation.
Signature · 3 lines
Stefan Richter -=====-==--= ---= --=-- http://arcgraph.de/sr/