Re: isochronous receives?

[Stefan Richter]

Robert Crocombe wrote:
> I was setting the tag to -1 in a certain spot (which indicates
> that you want to see all packets, regardless of their tag), but
> unhelpfully changing it to 0 before calling raw1394_iso_recv_start...
> 
> ...dangit, though.  Looking at the data stream, the tag *is* zero.
> 
> Stefan, isn't the line:
> 
>         /* match on specified tags */
>         contextMatch = tag_mask << 28;
> 
> in ohci_iso_recv_start() wrong?  The register looks to work like this.
>  The tag field is two bits.
> 
> if you want to match on 11b, then set bit tag3 (bit 31)
> if you want to match on 10b, then set bit tag2 (bit 30)
> if you want to match on 01b, then set bit tag1 (bit 29)
> if you want to match on 00b, then set bit tag0 (bit 28)
> 
> Which makes the shift obviously wrong.  Passing in '3' to match on tag
> 11b will have you instead set bits 29 and 28, and you will match on
> 01b and 00b.  Passing in '0' will completely bone you: no bits will be
> turned on.  Passing in '-1' to match all bits does work, though.

The question is, what is tag_mask in libraw1394's
    int raw1394_iso_recv_start(raw1394handle_t handle,
                               int start_on_cycle,
                               int tag_mask,
                               int sync);
supposed to mean in the first place?  It is not entirely documented, at
least not in the currently online documentation.

However, as it works now, tag_mask is obviously nothing else than the
highest 4 bits of the OHCI-1394 1.1 iso receive contextMatch register,
which is...

> You'd have to know to pass in 0x8 to match for tag 11b, which is a
> skosh counterintuitive

...counterintuitive and apparently undocumented.

> and probably not what was intended.

How can we tell?  The author of libraw1394's rawiso API should confirm
or deny that. :-)

> Here's my crap patch.  It appears to Work For Me(tm).
> 
> --- ohci1394.c  2006-12-04 16:52:10.916044780 -0700
> +++ modified_ohci1394.c 2006-12-13 07:22:07.613917511 -0700
> @@ -1491,7 +1491,18 @@
>         reg_write(recv->ohci, recv->ContextControlSet, command);
> 
>         /* match on specified tags */
> -       contextMatch = tag_mask << 28;
> +        switch (tag_mask)
> +        {
> +       case -1: contextMatch = tag_mask << 28; break;
> +       case 0: contextMatch = (1 << 28); break;
> +       case 1: contextMatch = (1 << 29); break;
> +       case 2: contextMatch = (1 << 30); break;
> +       case 3: contextMatch = (1 << 31); break;
> +       default:
> +               DBGMSG("Invalid tag_mask %0x, matching all tags",tag_mask);
> +               contextMatch = tag_mask << 28;
> +               break;
> +       }
> 
>         if (iso->channel == -1) {
>                 /* enable multichannel reception */
> 
> 
> So nevermind.  I'm totally vindicated and my code is, as always,
> flawless.  Cough.

We could do this, but this would have two disadvantages:
 - We change the kernel's userspace ABI.  This is only done if serious
   bugs of an ABI need to be fixed.
 - We loose the ability to match two or three tags out of four, FWIW.
   The proposed patch can match only one or all four tags.

How about leaving ohci1394 as it is but document tag_mask better in
libraw1394's inline doxygen(?) comments, and maybe add an enum or macros
to be used as values of raw1394_iso_recv_start's tag_mask argument?

/* can be ORed together */
#define RAW1394_IR_MATCH_TAG_0       1
#define RAW1394_IR_MATCH_TAG_1       2
#define RAW1394_IR_MATCH_TAG_2       4
#define RAW1394_IR_MATCH_TAG_3       8
#define RAW1394_IR_MATCH_ALL_TAGS   -1

or

/* can be used for tag = 0...3 and ORed together for multiple tags */
#define RAW1394_IR_MATCH_TAG(tag)   (1<<((tag)&3))
#define RAW1394_IR_MATCH_ALL_TAGS   -1

-- 
Stefan Richter
-=====-=-==- ==-- -==-=
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/