Re: [Qemu-devel] [PATCH 04/12] usb: Add support for input pipelining

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

>>> 2a) The combining entity needs to know when the hcd is done with
>>> queuing up packets for an ep.
>>
>> Yes.  We can add a USBDeviceClass method for that.
> 
> Ok, this assumes that the combining / uncombining gets moved down
> into the usb-device level code. Which clearly has your preference,
> but I don't completely agree with, so lets have that discussion first,
> once we've decided what to do there, details like this will likely
> sort out themselves.

Yes.

>> The core can and should do that for packets it owns (USBPacketState ==
>> USB_PACKET_QUEUED) because they are not (yet) passed to the USBDevice.
>>
>> Packets owned by USBDevice (USBPacketState == USB_PACKET_ASYNC) must be
>> handled by the USBDevice itself.
> 
> Getting offtopic a bit here, but this not how we currently handle
> things, currently the hcd code cancels packets after a queue halt,

Ah, right.  Well, that should continue to work.  USB_RET_NOT_USED would
make the hcd code just free the packet, and anything not-yet freed will
be zapped by the queue halt handling.

> Notice that arguments 2 and 3 could be made for combining output packets
> too, but what we do their now is nice and KISS, and the possible speed
> improvement is not worth the complication IMHO.

Handling in/out eps the same way has its merits too (code sharing).

> So again lets try to split the discussion in answering multiple questions:
> 
> 1) I believe it is better, and would greatly prefer to, do the combining
> on the qemu side rather then on the usb-redir-host side, do you agree?

Your call, you know the bits much better than I do.
Your arguments make sense.

> 2) If you agree with 1, then I assume you agree we will want to share
> the combining code between host-linux.c (or host-* for that matter) and
> redirect.c ?

I'm not sure there is that much to share.

A helper function which takes a USBEndpoint and returns an iovec for all
USBPackets lined up there would probably be useful.  Likewise for one
for completing the packets (takes xfer length + status, then fill
USBPacket->result & call usb_packet_complete for each packet).

But beyond that?

> 3) If you agree with 2, then all we need is a place for the shared logic
> to live, we could put it in a new file called input-pipeline.c ?

Just stick the helpers into hw/usb/core.c?

cheers,
  Gerd