Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
On Wed, 20 Jun 2018 15:56:53 +0200, Takashi Iwai wrote: > > On Wed, 20 Jun 2018 15:52:49 +0200, > Sebastian Andrzej Siewior wrote: > > > > On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote: > > > > usb_fill_int_urb() ensures that syncinterval is within the allowed range > > > > on HS/SS. The interval value seems to come from > > > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage > > > > 1 … 4. So in order to keep the magic working I pass datainterval + 1. > > > > > > This needs more explanation. By this conversion, the interval > > > computation becomes really tricky. > > > > > > There are two interval calculations. The first one is > > > fp->datainterval and it's from snd_usb_parse_datainterval() as you > > > mentioned. And a tricky part is that fp->datainterval is 0 on > > > FULL_SPEED. Casually, fp->datainterval+1 will be translated to the > > > correct value since (0 + 1) == (1 << 0). > > > > > > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set > > > in snd_usb_add_endpoint(). Luckily, again, ep->syncinterval + 1 works > > > for FULL_SPEED as well, as (1 + 1) == (1 << 1). > > > > Do you want me to add your additional explanation to the description? > > Yes. It's a very implicit assumption, and needs clarification. > In addition, the comment 1...4 is only for fp->datainterval, not about > ep->syncinterval. > > Alternatively, give some comments in the places where putting > fp->datainterval and ep->syncinterval. Oh, and for other patches, something about interval could be mentioned in the change log. You pass 1 to the macro as if it were identical as the original code (urb->interval = 1), but this isn't evaluated equally. Effectively it'll result in the same, but better to be clarified. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
On Wed, 20 Jun 2018 15:52:49 +0200, Sebastian Andrzej Siewior wrote: > > On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote: > > > usb_fill_int_urb() ensures that syncinterval is within the allowed range > > > on HS/SS. The interval value seems to come from > > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage > > > 1 … 4. So in order to keep the magic working I pass datainterval + 1. > > > > This needs more explanation. By this conversion, the interval > > computation becomes really tricky. > > > > There are two interval calculations. The first one is > > fp->datainterval and it's from snd_usb_parse_datainterval() as you > > mentioned. And a tricky part is that fp->datainterval is 0 on > > FULL_SPEED. Casually, fp->datainterval+1 will be translated to the > > correct value since (0 + 1) == (1 << 0). > > > > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set > > in snd_usb_add_endpoint(). Luckily, again, ep->syncinterval + 1 works > > for FULL_SPEED as well, as (1 + 1) == (1 << 1). > > Do you want me to add your additional explanation to the description? Yes. It's a very implicit assumption, and needs clarification. In addition, the comment 1...4 is only for fp->datainterval, not about ep->syncinterval. Alternatively, give some comments in the places where putting fp->datainterval and ep->syncinterval. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote: > > usb_fill_int_urb() ensures that syncinterval is within the allowed range > > on HS/SS. The interval value seems to come from > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage > > 1 … 4. So in order to keep the magic working I pass datainterval + 1. > > This needs more explanation. By this conversion, the interval > computation becomes really tricky. > > There are two interval calculations. The first one is > fp->datainterval and it's from snd_usb_parse_datainterval() as you > mentioned. And a tricky part is that fp->datainterval is 0 on > FULL_SPEED. Casually, fp->datainterval+1 will be translated to the > correct value since (0 + 1) == (1 << 0). > > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set > in snd_usb_add_endpoint(). Luckily, again, ep->syncinterval + 1 works > for FULL_SPEED as well, as (1 + 1) == (1 << 1). Do you want me to add your additional explanation to the description? > thanks, > > Takashi Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()
On Wed, 20 Jun 2018 11:38:27 +0200, Sebastian Andrzej Siewior wrote: > > Using usb_fill_int_urb() helps to find code which initializes an > URB. A grep for members of the struct (like ->complete) reveal lots > of other things, too. > > data_ep_set_params() additionally sets urb->transfer_buffer_length which > was not the case earlier. > usb_fill_int_urb() ensures that syncinterval is within the allowed range > on HS/SS. The interval value seems to come from > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage > 1 … 4. So in order to keep the magic working I pass datainterval + 1. This needs more explanation. By this conversion, the interval computation becomes really tricky. There are two interval calculations. The first one is fp->datainterval and it's from snd_usb_parse_datainterval() as you mentioned. And a tricky part is that fp->datainterval is 0 on FULL_SPEED. Casually, fp->datainterval+1 will be translated to the correct value since (0 + 1) == (1 << 0). OTOH, for the sync EP, it's taken from ep->syncinterval, which is set in snd_usb_add_endpoint(). Luckily, again, ep->syncinterval + 1 works for FULL_SPEED as well, as (1 + 1) == (1 << 1). thanks, Takashi > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Signed-off-by: Sebastian Andrzej Siewior > --- > v1…v2: description changes (spelling + usb_fill_int_urb() ensures >"syncinterval" not data_ep_set_params()) > > sound/usb/endpoint.c | 24 ++-- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > index c90607ebe155..bbc02db5b417 100644 > --- a/sound/usb/endpoint.c > +++ b/sound/usb/endpoint.c > @@ -772,6 +772,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep, > /* allocate and initialize data urbs */ > for (i = 0; i < ep->nurbs; i++) { > struct snd_urb_ctx *u = >urb[i]; > + void *buf; > + > u->index = i; > u->ep = ep; > u->packets = urb_packs; > @@ -783,16 +785,13 @@ static int data_ep_set_params(struct snd_usb_endpoint > *ep, > if (!u->urb) > goto out_of_memory; > > - u->urb->transfer_buffer = > - usb_alloc_coherent(ep->chip->dev, u->buffer_size, > -GFP_KERNEL, >urb->transfer_dma); > - if (!u->urb->transfer_buffer) > + buf = usb_alloc_coherent(ep->chip->dev, u->buffer_size, > + GFP_KERNEL, >urb->transfer_dma); > + if (!buf) > goto out_of_memory; > - u->urb->pipe = ep->pipe; > + usb_fill_int_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size, > + snd_complete_urb, u, ep->datainterval + 1); > u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP; > - u->urb->interval = 1 << ep->datainterval; > - u->urb->context = u; > - u->urb->complete = snd_complete_urb; > INIT_LIST_HEAD(>ready_list); > } > > @@ -823,15 +822,12 @@ static int sync_ep_set_params(struct snd_usb_endpoint > *ep) > u->urb = usb_alloc_urb(1, GFP_KERNEL); > if (!u->urb) > goto out_of_memory; > - u->urb->transfer_buffer = ep->syncbuf + i * 4; > + usb_fill_int_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4, > + snd_complete_urb, u, ep->syncinterval + 1); > + > u->urb->transfer_dma = ep->sync_dma + i * 4; > - u->urb->transfer_buffer_length = 4; > - u->urb->pipe = ep->pipe; > u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP; > u->urb->number_of_packets = 1; > - u->urb->interval = 1 << ep->syncinterval; > - u->urb->context = u; > - u->urb->complete = snd_complete_urb; > } > > ep->nurbs = SYNC_URBS; > -- > 2.17.1 > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html