| Message ID | 20220126132249.2133168-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dummy_hcd: add isoc support | expand |
On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote: > With this patch, the dummy_hcd gains first support for isoc transfers. > It will complete the whole urb with all packages. "packets", not "packages". > Even if the gadget > side did not enqueue any request, the urb will be handled. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> I don't like this idea. If support for isochronous transfers is added, it should be done correctly. That is, the implementation should support scheduling of transfers, periodic bandwidth reservation, high-bandwidth transfers, and so on. The whole point of dummy-hcd is to emulate a real host controller as closely as possible. Real isochronous transfers do not complete all at once. Alan Stern
On Wed, Jan 26, 2022 at 11:09:23AM -0500, Alan Stern wrote: >On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote: >> With this patch, the dummy_hcd gains first support for isoc transfers. >> It will complete the whole urb with all packages. > >"packets", not "packages". Right. >> Even if the gadget >> side did not enqueue any request, the urb will be handled. >> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > >I don't like this idea. If support for isochronous transfers is added, >it should be done correctly. That is, the implementation should support >scheduling of transfers, periodic bandwidth reservation, high-bandwidth >transfers, and so on. > >The whole point of dummy-hcd is to emulate a real host controller as >closely as possible. Real isochronous transfers do not complete all at >once. I agree, that whole isoc support needs proper improvement. I could/should have added RFC to the patch. As the whole intention of this code, for now, is to validate the gadget/uvc configfs patchseries mentioned in the the comments. With this patch, it is at least possible to get the gadget running on dummy_hcd and try out the "non-isoc dependent" parts, that are actually changed, in the mentioned series. The validation of the payload path, actually using the mentioned isoc endpoints, is left to the developer who implements the missing parts you mentioned above. Michael
Hi Michael, Alan, Quoting Michael Grzeschik (2022-01-26 18:31:38) > On Wed, Jan 26, 2022 at 11:09:23AM -0500, Alan Stern wrote: > >On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote: > >> With this patch, the dummy_hcd gains first support for isoc transfers. > >> It will complete the whole urb with all packages. > > > >"packets", not "packages". > > Right. > > >> Even if the gadget > >> side did not enqueue any request, the urb will be handled. > >> > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > > >I don't like this idea. If support for isochronous transfers is added, > >it should be done correctly. That is, the implementation should support > >scheduling of transfers, periodic bandwidth reservation, high-bandwidth > >transfers, and so on. > > > >The whole point of dummy-hcd is to emulate a real host controller as > >closely as possible. Real isochronous transfers do not complete all at > >once. Being able to at least test uvc-gadget in a virtual environment would already be a big benefit. As this is emulation, not simulation is it essential that an exact mapping of the hardware is in place? Is there anything we can do to support the progression of this development? I.e. could we support this method first with a WARN_ONCE("This does not fully emulate Isochronous support"); That would allow infrastructure to be built up that uses this functionality, which would then in turn feed back into providing a means to actually test the improvements to the isochronous support on top. > I agree, that whole isoc support needs proper improvement. > > I could/should have added RFC to the patch. As the whole intention of > this code, for now, is to validate the gadget/uvc configfs patchseries > mentioned in the the comments. > > With this patch, it is at least possible to get the gadget running on > dummy_hcd and try out the "non-isoc dependent" parts, that are actually > changed, in the mentioned series. > > The validation of the payload path, actually using the mentioned isoc > endpoints, is left to the developer who implements the missing parts > you mentioned above. > > Michael > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Mon, Jan 31, 2022 at 12:08:20PM +0000, Kieran Bingham wrote: > Hi Michael, Alan, > > Quoting Michael Grzeschik (2022-01-26 18:31:38) > > On Wed, Jan 26, 2022 at 11:09:23AM -0500, Alan Stern wrote: > > >On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote: > > >> With this patch, the dummy_hcd gains first support for isoc transfers. > > >> It will complete the whole urb with all packages. > > > > > >"packets", not "packages". > > > > Right. > > > > >> Even if the gadget > > >> side did not enqueue any request, the urb will be handled. > > >> > > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > > > > >I don't like this idea. If support for isochronous transfers is added, > > >it should be done correctly. That is, the implementation should support > > >scheduling of transfers, periodic bandwidth reservation, high-bandwidth > > >transfers, and so on. > > > > > >The whole point of dummy-hcd is to emulate a real host controller as > > >closely as possible. Real isochronous transfers do not complete all at > > >once. > > Being able to at least test uvc-gadget in a virtual environment would > already be a big benefit. As this is emulation, not simulation is it > essential that an exact mapping of the hardware is in place? Bindly being a sink for all data is not emulation for drivers that require some sort of feedback loop. > Is there anything we can do to support the progression of this > development? > > I.e. could we support this method first with a > WARN_ONCE("This does not fully emulate Isochronous support"); That would instantly trigger syzbot to send us reports for no good reason. Please don't do that :( thanks, greg k-h
On Mon, Jan 31, 2022 at 12:08:20PM +0000, Kieran Bingham wrote: > Hi Michael, Alan, > > Quoting Michael Grzeschik (2022-01-26 18:31:38) > > On Wed, Jan 26, 2022 at 11:09:23AM -0500, Alan Stern wrote: > > >On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote: > > >> With this patch, the dummy_hcd gains first support for isoc transfers. > > >> It will complete the whole urb with all packages. > > > > > >"packets", not "packages". > > > > Right. > > > > >> Even if the gadget > > >> side did not enqueue any request, the urb will be handled. > > >> > > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > > > > >I don't like this idea. If support for isochronous transfers is added, > > >it should be done correctly. That is, the implementation should support > > >scheduling of transfers, periodic bandwidth reservation, high-bandwidth > > >transfers, and so on. > > > > > >The whole point of dummy-hcd is to emulate a real host controller as > > >closely as possible. Real isochronous transfers do not complete all at > > >once. > > Being able to at least test uvc-gadget in a virtual environment would > already be a big benefit. As this is emulation, not simulation is it > essential that an exact mapping of the hardware is in place? It doesn't have to be exact, but it should be close enough to test the essential aspects of isochronous transfers. In particular, people will need to be able to test the timing aspects -- they are a big part of the isochronous mechanism. > Is there anything we can do to support the progression of this > development? > > I.e. could we support this method first with a > WARN_ONCE("This does not fully emulate Isochronous support"); No, as Greg said, don't do that. Improve the emulation in the patch so that it does do proper scheduling for isochronous transfers. You don't have to worry about all the aspects of this -- for example, since dummy-hcd supports only one device at a time, it should be okay to leave out checks for overcommitting periodic transfer times during a frame or microframe. (dummy-hcd doesn't currently do that sort of check for interrupt transfers.) But the emulation should be sufficiently realistic to return -EXDEV errors for URBs or packets that were submitted too late. Alternatively, you can simply keep the patch that Michael submitted as an out-of-tree resource for people who want to test uvc-gadget. > That would allow infrastructure to be built up that uses this > functionality, which would then in turn feed back into providing a means > to actually test the improvements to the isochronous support on top. Without realistic timing emulation, the tests would be only minimally useful. Alan Stern
diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c index a2d956af42a23c..aff5f1fa4feef9 100644 --- a/drivers/usb/gadget/udc/dummy_hcd.c +++ b/drivers/usb/gadget/udc/dummy_hcd.c @@ -147,36 +147,30 @@ static const struct { USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_IN)), EP_INFO("ep2out-bulk", USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_OUT)), -/* EP_INFO("ep3in-iso", USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_IN)), EP_INFO("ep4out-iso", USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_OUT)), -*/ EP_INFO("ep5in-int", USB_EP_CAPS(USB_EP_CAPS_TYPE_INT, USB_EP_CAPS_DIR_IN)), EP_INFO("ep6in-bulk", USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_IN)), EP_INFO("ep7out-bulk", USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_OUT)), -/* EP_INFO("ep8in-iso", USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_IN)), EP_INFO("ep9out-iso", USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_OUT)), -*/ EP_INFO("ep10in-int", USB_EP_CAPS(USB_EP_CAPS_TYPE_INT, USB_EP_CAPS_DIR_IN)), EP_INFO("ep11in-bulk", USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_IN)), EP_INFO("ep12out-bulk", USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_OUT)), -/* EP_INFO("ep13in-iso", USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_IN)), EP_INFO("ep14out-iso", USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_OUT)), -*/ EP_INFO("ep15in-int", USB_EP_CAPS(USB_EP_CAPS_TYPE_INT, USB_EP_CAPS_DIR_IN)), @@ -1402,6 +1396,7 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb, struct dummy *dum = dum_hcd->dum; struct dummy_request *req; int sent = 0; + int count = 0; top: /* if there's no request queued, the device is NAKing; return */ @@ -1459,6 +1454,13 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb, sent += len; urb->actual_length += len; req->req.actual += len; + if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) { + if (count <= urb->number_of_packets) { + urb->iso_frame_desc[count].actual_length = len; + urb->iso_frame_desc[count].status = 0; + count++; + } + } } } @@ -1527,6 +1529,17 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb, if (rescan) goto top; } + + if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) { + int i; + + for (i = count; i < urb->number_of_packets; ++i) { + urb->iso_frame_desc[i].actual_length = 0; + urb->iso_frame_desc[i].status = 0; + } + *status = 0; + } + return sent; } @@ -1950,13 +1963,14 @@ static void dummy_timer(struct timer_list *t) * here are some of the issues we'd have to face: * * Is it urb->interval since the last xfer? - * Use urb->iso_frame_desc[i]. - * Complete whether or not ep has requests queued. * Report random errors, to debug drivers. */ limit = max(limit, periodic_bytes(dum, ep)); - status = -EINVAL; /* fail all xfers */ - break; + ep->last_io = jiffies; + total -= transfer(dum_hcd, urb, ep, limit, &status); + if (status == -EINPROGRESS) + continue; + goto return_urb; case PIPE_INTERRUPT: /* FIXME is it urb->interval since the last xfer?
With this patch, the dummy_hcd gains first support for isoc transfers. It will complete the whole urb with all packages. Even if the gadget side did not enqueue any request, the urb will be handled. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- With this patch it is possible to test the series [1] on any device using the uvc-gadget code [2]. I added some patches on top of uvc-gadget to prove that it is now possible to completely remove the configfs parsing and DATA/SETUP event handling from userspace [3]. To test the gadget, just fill the uvc configfs setup with some script [4] or even use the modern (but optional) way with gt (gadget-tool) [5] including libusbgx (uvc/configfs) [6] support and a scheme file describing the gadget. [1] https://lore.kernel.org/linux-usb/20220105115527.3592860-1-m.grzeschik@pengutronix.de/ [2] https://git.ideasonboard.org/uvc-gadget.git [3] https://git.pengutronix.de/cgit/mgr/uvc-gadget/log/?h=ml [4] https://git.ideasonboard.org/uvc-gadget.git/blob/HEAD:/scripts/uvc-gadget.sh [5] https://github.com/linux-usb-gadgets/libusbgx [6] https://github.com/linux-usb-gadgets/gt drivers/usb/gadget/udc/dummy_hcd.c | 34 +++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 10 deletions(-)