* [PATCH v4 1/6] usb: gadget: uvc: fix dropped frame after missed isoc
2022-10-18 21:50 [PATCH v4 0/6] uvc gadget performance issues Dan Vacura
@ 2022-10-18 21:50 ` Dan Vacura
2022-10-18 21:50 ` [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release " Dan Vacura
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Dan Vacura @ 2022-10-18 21:50 UTC (permalink / raw)
To: linux-usb
Cc: Daniel Scally, Thinh Nguyen, Jeff Vanhoof, Dan Vacura, stable,
Greg Kroah-Hartman, Jonathan Corbet, Laurent Pinchart,
Felipe Balbi, Michael Grzeschik, Paul Elder, linux-kernel,
linux-doc
With the re-use of the previous completion status in 0d1c407b1a749
("usb: dwc3: gadget: Return proper request status") it could be possible
that the next frame would also get dropped if the current frame has a
missed isoc error. Ensure that an interrupt is requested for the start
of a new frame.
Fixes: fc78941d8169 ("usb: gadget: uvc: decrease the interrupt load to a quarter")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V3:
- no change, new patch in series
V3 -> V4:
- no change
drivers/usb/gadget/function/uvc_video.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index bb037fcc90e6..323977716f5a 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -431,7 +431,8 @@ static void uvcg_video_pump(struct work_struct *work)
/* Endpoint now owns the request */
req = NULL;
- video->req_int_count++;
+ if (buf->state != UVC_BUF_STATE_DONE)
+ video->req_int_count++;
}
if (!req)
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc
2022-10-18 21:50 [PATCH v4 0/6] uvc gadget performance issues Dan Vacura
2022-10-18 21:50 ` [PATCH v4 1/6] usb: gadget: uvc: fix dropped frame after missed isoc Dan Vacura
@ 2022-10-18 21:50 ` Dan Vacura
2022-10-22 11:31 ` Greg Kroah-Hartman
2022-10-18 21:50 ` [PATCH v4 3/6] usb: gadget: uvc: fix sg handling in error case Dan Vacura
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Dan Vacura @ 2022-10-18 21:50 UTC (permalink / raw)
To: linux-usb
Cc: Daniel Scally, Thinh Nguyen, Jeff Vanhoof, stable, Dan Vacura,
Greg Kroah-Hartman, Jonathan Corbet, Laurent Pinchart,
Felipe Balbi, Paul Elder, Michael Grzeschik, linux-kernel,
linux-doc
From: Jeff Vanhoof <qjv001@motorola.com>
arm-smmu related crashes seen after a Missed ISOC interrupt when
no_interrupt=1 is used. This can happen if the hardware is still using
the data associated with a TRB after the usb_request's ->complete call
has been made. Instead of immediately releasing a request when a Missed
ISOC interrupt has occurred, this change will add logic to cancel the
request instead where it will eventually be released when the
END_TRANSFER command has completed. This logic is similar to some of the
cleanup done in dwc3_gadget_ep_dequeue.
Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
Cc: <stable@vger.kernel.org>
Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
Co-developed-by: Dan Vacura <w36195@motorola.com>
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V3:
- no change, new patch in series
V3 -> V4:
- no change
drivers/usb/dwc3/core.h | 1 +
drivers/usb/dwc3/gadget.c | 38 ++++++++++++++++++++++++++------------
2 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8f9959ba9fd4..9b005d912241 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -943,6 +943,7 @@ struct dwc3_request {
#define DWC3_REQUEST_STATUS_DEQUEUED 3
#define DWC3_REQUEST_STATUS_STALLED 4
#define DWC3_REQUEST_STATUS_COMPLETED 5
+#define DWC3_REQUEST_STATUS_MISSED_ISOC 6
#define DWC3_REQUEST_STATUS_UNKNOWN -1
u8 epnum;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 079cd333632e..411532c5c378 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2021,6 +2021,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
case DWC3_REQUEST_STATUS_STALLED:
dwc3_gadget_giveback(dep, req, -EPIPE);
break;
+ case DWC3_REQUEST_STATUS_MISSED_ISOC:
+ dwc3_gadget_giveback(dep, req, -EXDEV);
+ break;
default:
dev_err(dwc->dev, "request cancelled with wrong reason:%d\n", req->status);
dwc3_gadget_giveback(dep, req, -ECONNRESET);
@@ -3402,21 +3405,32 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
struct dwc3 *dwc = dep->dwc;
bool no_started_trb = true;
- dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
+ if (status == -EXDEV) {
+ struct dwc3_request *tmp;
+ struct dwc3_request *req;
- if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
- goto out;
+ if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
+ dwc3_stop_active_transfer(dep, true, true);
- if (!dep->endpoint.desc)
- return no_started_trb;
+ list_for_each_entry_safe(req, tmp, &dep->started_list, list)
+ dwc3_gadget_move_cancelled_request(req,
+ DWC3_REQUEST_STATUS_MISSED_ISOC);
+ } else {
+ dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
- list_empty(&dep->started_list) &&
- (list_empty(&dep->pending_list) || status == -EXDEV))
- dwc3_stop_active_transfer(dep, true, true);
- else if (dwc3_gadget_ep_should_continue(dep))
- if (__dwc3_gadget_kick_transfer(dep) == 0)
- no_started_trb = false;
+ if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
+ goto out;
+
+ if (!dep->endpoint.desc)
+ return no_started_trb;
+
+ if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+ list_empty(&dep->started_list) && list_empty(&dep->pending_list))
+ dwc3_stop_active_transfer(dep, true, true);
+ else if (dwc3_gadget_ep_should_continue(dep))
+ if (__dwc3_gadget_kick_transfer(dep) == 0)
+ no_started_trb = false;
+ }
out:
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc
2022-10-18 21:50 ` [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release " Dan Vacura
@ 2022-10-22 11:31 ` Greg Kroah-Hartman
2022-10-22 13:35 ` Jeff Vanhoof
0 siblings, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2022-10-22 11:31 UTC (permalink / raw)
To: Dan Vacura
Cc: linux-usb, Daniel Scally, Thinh Nguyen, Jeff Vanhoof, stable,
Jonathan Corbet, Laurent Pinchart, Felipe Balbi, Paul Elder,
Michael Grzeschik, linux-kernel, linux-doc
On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
> From: Jeff Vanhoof <qjv001@motorola.com>
>
> arm-smmu related crashes seen after a Missed ISOC interrupt when
> no_interrupt=1 is used. This can happen if the hardware is still using
> the data associated with a TRB after the usb_request's ->complete call
> has been made. Instead of immediately releasing a request when a Missed
> ISOC interrupt has occurred, this change will add logic to cancel the
> request instead where it will eventually be released when the
> END_TRANSFER command has completed. This logic is similar to some of the
> cleanup done in dwc3_gadget_ep_dequeue.
>
> Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
> Co-developed-by: Dan Vacura <w36195@motorola.com>
> Signed-off-by: Dan Vacura <w36195@motorola.com>
> ---
> V1 -> V3:
> - no change, new patch in series
> V3 -> V4:
> - no change
I need an ack from the dwc3 maintainer before I can take this one.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc
2022-10-22 11:31 ` Greg Kroah-Hartman
@ 2022-10-22 13:35 ` Jeff Vanhoof
2022-10-24 22:47 ` Thinh Nguyen
0 siblings, 1 reply; 14+ messages in thread
From: Jeff Vanhoof @ 2022-10-22 13:35 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Dan Vacura, linux-usb, Daniel Scally, Thinh Nguyen, Jeff Vanhoof,
stable, Jonathan Corbet, Laurent Pinchart, Felipe Balbi,
Paul Elder, Michael Grzeschik, linux-kernel, linux-doc
Hi Greg,
On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
> > From: Jeff Vanhoof <qjv001@motorola.com>
> >
> > arm-smmu related crashes seen after a Missed ISOC interrupt when
> > no_interrupt=1 is used. This can happen if the hardware is still using
> > the data associated with a TRB after the usb_request's ->complete call
> > has been made. Instead of immediately releasing a request when a Missed
> > ISOC interrupt has occurred, this change will add logic to cancel the
> > request instead where it will eventually be released when the
> > END_TRANSFER command has completed. This logic is similar to some of the
> > cleanup done in dwc3_gadget_ep_dequeue.
> >
> > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
> > Co-developed-by: Dan Vacura <w36195@motorola.com>
> > Signed-off-by: Dan Vacura <w36195@motorola.com>
> > ---
> > V1 -> V3:
> > - no change, new patch in series
> > V3 -> V4:
> > - no change
>
> I need an ack from the dwc3 maintainer before I can take this one.
>
> thanks,
>
> greg k-h
Thinh has rejected this version of the patch. He has provided an alternative
implementation which has been testing well for us so far. Either Thinh or Dan
will formalize this patch within the next few days.
The latest proposed changes are:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index dfaf9ac24c4f..50287437d6de 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
if (event->status & DEPEVT_STATUS_SHORT && !chain)
return 1;
+ if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
+ return 1;
+
if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
(trb->ctrl & DWC3_TRB_CTRL_LST))
return 1;
@@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
struct scatterlist *s;
unsigned int num_queued = req->num_queued_sgs;
unsigned int i;
+ bool missed_isoc = false;
int ret = 0;
for_each_sg(sg, s, num_queued, i) {
@@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
req->sg = sg_next(s);
req->num_queued_sgs--;
+ if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
+ missed_isoc = true;
+
ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
trb, event, status, true);
if (ret)
break;
}
+ if (missed_isoc)
+ ret = 1;
+
return ret;
}
Thanks,
Jeff
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc
2022-10-22 13:35 ` Jeff Vanhoof
@ 2022-10-24 22:47 ` Thinh Nguyen
2023-09-19 9:10 ` Michael Grzeschik
0 siblings, 1 reply; 14+ messages in thread
From: Thinh Nguyen @ 2022-10-24 22:47 UTC (permalink / raw)
To: Jeff Vanhoof
Cc: Greg Kroah-Hartman, Dan Vacura, linux-usb, Daniel Scally,
Thinh Nguyen, Jeff Vanhoof, stable, Jonathan Corbet,
Laurent Pinchart, Felipe Balbi, Paul Elder, Michael Grzeschik,
linux-kernel, linux-doc
On Sat, Oct 22, 2022, Jeff Vanhoof wrote:
> Hi Greg,
>
> On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
> > > From: Jeff Vanhoof <qjv001@motorola.com>
> > >
> > > arm-smmu related crashes seen after a Missed ISOC interrupt when
> > > no_interrupt=1 is used. This can happen if the hardware is still using
> > > the data associated with a TRB after the usb_request's ->complete call
> > > has been made. Instead of immediately releasing a request when a Missed
> > > ISOC interrupt has occurred, this change will add logic to cancel the
> > > request instead where it will eventually be released when the
> > > END_TRANSFER command has completed. This logic is similar to some of the
> > > cleanup done in dwc3_gadget_ep_dequeue.
> > >
> > > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
> > > Co-developed-by: Dan Vacura <w36195@motorola.com>
> > > Signed-off-by: Dan Vacura <w36195@motorola.com>
> > > ---
> > > V1 -> V3:
> > > - no change, new patch in series
> > > V3 -> V4:
> > > - no change
> >
> > I need an ack from the dwc3 maintainer before I can take this one.
> >
> > thanks,
> >
> > greg k-h
>
> Thinh has rejected this version of the patch. He has provided an alternative
> implementation which has been testing well for us so far. Either Thinh or Dan
> will formalize this patch within the next few days.
> The latest proposed changes are:
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index dfaf9ac24c4f..50287437d6de 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> if (event->status & DEPEVT_STATUS_SHORT && !chain)
> return 1;
>
> + if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
> + return 1;
> +
> if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
> (trb->ctrl & DWC3_TRB_CTRL_LST))
> return 1;
> @@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
> struct scatterlist *s;
> unsigned int num_queued = req->num_queued_sgs;
> unsigned int i;
> + bool missed_isoc = false;
> int ret = 0;
>
> for_each_sg(sg, s, num_queued, i) {
> @@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
> req->sg = sg_next(s);
> req->num_queued_sgs--;
>
> + if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
> + missed_isoc = true;
> +
> ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
> trb, event, status, true);
> if (ret)
> break;
> }
>
> + if (missed_isoc)
> + ret = 1;
> +
> return ret;
> }
>
>
That's just a debug patch. I'll send out proper fix patches.
Thanks,
Thinh
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc
2022-10-24 22:47 ` Thinh Nguyen
@ 2023-09-19 9:10 ` Michael Grzeschik
2023-09-19 9:18 ` Michael Grzeschik
0 siblings, 1 reply; 14+ messages in thread
From: Michael Grzeschik @ 2023-09-19 9:10 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Jeff Vanhoof, Greg Kroah-Hartman, Dan Vacura, linux-usb,
Daniel Scally, Jeff Vanhoof, stable, Jonathan Corbet,
Laurent Pinchart, Felipe Balbi, Paul Elder, linux-kernel,
linux-doc
[-- Attachment #1: Type: text/plain, Size: 3880 bytes --]
Hi Thinh,
On Mon, Oct 24, 2022 at 10:47:53PM +0000, Thinh Nguyen wrote:
>On Sat, Oct 22, 2022, Jeff Vanhoof wrote:
>> Hi Greg,
>>
>> On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote:
>> > On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
>> > > From: Jeff Vanhoof <qjv001@motorola.com>
>> > >
>> > > arm-smmu related crashes seen after a Missed ISOC interrupt when
>> > > no_interrupt=1 is used. This can happen if the hardware is still using
>> > > the data associated with a TRB after the usb_request's ->complete call
>> > > has been made. Instead of immediately releasing a request when a Missed
>> > > ISOC interrupt has occurred, this change will add logic to cancel the
>> > > request instead where it will eventually be released when the
>> > > END_TRANSFER command has completed. This logic is similar to some of the
>> > > cleanup done in dwc3_gadget_ep_dequeue.
>> > >
>> > > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
>> > > Cc: <stable@vger.kernel.org>
>> > > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
>> > > Co-developed-by: Dan Vacura <w36195@motorola.com>
>> > > Signed-off-by: Dan Vacura <w36195@motorola.com>
>> > > ---
>> > > V1 -> V3:
>> > > - no change, new patch in series
>> > > V3 -> V4:
>> > > - no change
>> >
>> > I need an ack from the dwc3 maintainer before I can take this one.
>> >
>> > thanks,
>> >
>> > greg k-h
>>
>> Thinh has rejected this version of the patch. He has provided an alternative
>> implementation which has been testing well for us so far. Either Thinh or Dan
>> will formalize this patch within the next few days.
>> The latest proposed changes are:
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index dfaf9ac24c4f..50287437d6de 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>> if (event->status & DEPEVT_STATUS_SHORT && !chain)
>> return 1;
>>
>> + if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
>> + return 1;
>> +
>> if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
>> (trb->ctrl & DWC3_TRB_CTRL_LST))
>> return 1;
>> @@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>> struct scatterlist *s;
>> unsigned int num_queued = req->num_queued_sgs;
>> unsigned int i;
>> + bool missed_isoc = false;
>> int ret = 0;
>>
>> for_each_sg(sg, s, num_queued, i) {
>> @@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>> req->sg = sg_next(s);
>> req->num_queued_sgs--;
>>
>> + if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
>> + missed_isoc = true;
>> +
>> ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>> trb, event, status, true);
>> if (ret)
>> break;
>> }
>>
>> + if (missed_isoc)
>> + ret = 1;
>> +
>> return ret;
>> }
>>
>>
>
>That's just a debug patch. I'll send out proper fix patches.
Ping!
While digging out this thread, I did not find any followup patch
for this suggestion. Did it hit the mailinglist anywhere?
If not, will you send one?
Regards,
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 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc
2023-09-19 9:10 ` Michael Grzeschik
@ 2023-09-19 9:18 ` Michael Grzeschik
0 siblings, 0 replies; 14+ messages in thread
From: Michael Grzeschik @ 2023-09-19 9:18 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Jeff Vanhoof, Greg Kroah-Hartman, Dan Vacura, linux-usb,
Daniel Scally, Jeff Vanhoof, stable, Jonathan Corbet,
Laurent Pinchart, Felipe Balbi, Paul Elder, linux-kernel,
linux-doc
[-- Attachment #1: Type: text/plain, Size: 4107 bytes --]
On Tue, Sep 19, 2023 at 11:10:55AM +0200, Michael Grzeschik wrote:
>On Mon, Oct 24, 2022 at 10:47:53PM +0000, Thinh Nguyen wrote:
>>On Sat, Oct 22, 2022, Jeff Vanhoof wrote:
>>>Hi Greg,
>>>
>>>On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote:
>>>> On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
>>>> > From: Jeff Vanhoof <qjv001@motorola.com>
>>>> >
>>>> > arm-smmu related crashes seen after a Missed ISOC interrupt when
>>>> > no_interrupt=1 is used. This can happen if the hardware is still using
>>>> > the data associated with a TRB after the usb_request's ->complete call
>>>> > has been made. Instead of immediately releasing a request when a Missed
>>>> > ISOC interrupt has occurred, this change will add logic to cancel the
>>>> > request instead where it will eventually be released when the
>>>> > END_TRANSFER command has completed. This logic is similar to some of the
>>>> > cleanup done in dwc3_gadget_ep_dequeue.
>>>> >
>>>> > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
>>>> > Cc: <stable@vger.kernel.org>
>>>> > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
>>>> > Co-developed-by: Dan Vacura <w36195@motorola.com>
>>>> > Signed-off-by: Dan Vacura <w36195@motorola.com>
>>>> > ---
>>>> > V1 -> V3:
>>>> > - no change, new patch in series
>>>> > V3 -> V4:
>>>> > - no change
>>>>
>>>> I need an ack from the dwc3 maintainer before I can take this one.
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>
>>>Thinh has rejected this version of the patch. He has provided an alternative
>>>implementation which has been testing well for us so far. Either Thinh or Dan
>>>will formalize this patch within the next few days.
>>>The latest proposed changes are:
>>>
>>>diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>index dfaf9ac24c4f..50287437d6de 100644
>>>--- a/drivers/usb/dwc3/gadget.c
>>>+++ b/drivers/usb/dwc3/gadget.c
>>>@@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>> if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>> return 1;
>>>
>>>+ if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
>>>+ return 1;
>>>+
>>> if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
>>> (trb->ctrl & DWC3_TRB_CTRL_LST))
>>> return 1;
>>>@@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>>> struct scatterlist *s;
>>> unsigned int num_queued = req->num_queued_sgs;
>>> unsigned int i;
>>>+ bool missed_isoc = false;
>>> int ret = 0;
>>>
>>> for_each_sg(sg, s, num_queued, i) {
>>>@@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>>> req->sg = sg_next(s);
>>> req->num_queued_sgs--;
>>>
>>>+ if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
>>>+ missed_isoc = true;
>>>+
>>> ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>>> trb, event, status, true);
>>> if (ret)
>>> break;
>>> }
>>>
>>>+ if (missed_isoc)
>>>+ ret = 1;
>>>+
>>> return ret;
>>> }
>>>
>>>
>>
>>That's just a debug patch. I'll send out proper fix patches.
>
>Ping!
>
>While digging out this thread, I did not find any followup patch
>for this suggestion. Did it hit the mailinglist anywhere?
>
>If not, will you send one?
Nevermind, I think I found the hunk in a variated version in this series.
https://lore.kernel.org/linux-usb/cover.1666735451.git.Thinh.Nguyen@synopsys.com/
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 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 3/6] usb: gadget: uvc: fix sg handling in error case
2022-10-18 21:50 [PATCH v4 0/6] uvc gadget performance issues Dan Vacura
2022-10-18 21:50 ` [PATCH v4 1/6] usb: gadget: uvc: fix dropped frame after missed isoc Dan Vacura
2022-10-18 21:50 ` [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release " Dan Vacura
@ 2022-10-18 21:50 ` Dan Vacura
2022-10-18 21:50 ` [PATCH v4 4/6] usb: gadget: uvc: fix sg handling during video encode Dan Vacura
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Dan Vacura @ 2022-10-18 21:50 UTC (permalink / raw)
To: linux-usb
Cc: Daniel Scally, Thinh Nguyen, Jeff Vanhoof, Dan Vacura, stable,
Greg Kroah-Hartman, Jonathan Corbet, Laurent Pinchart,
Felipe Balbi, Michael Grzeschik, Paul Elder, linux-kernel,
linux-doc
If there is a transmission error the buffer will be returned too early,
causing a memory fault as subsequent requests for that buffer are still
queued up to be sent. Refactor the error handling to wait for the final
request to come in before reporting back the buffer to userspace for all
transfer types (bulk/isoc/isoc_sg). This ensures userspace knows if the
frame was successfully sent.
Fixes: e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V2:
- undo error rename
- change uvcg_info to uvcg_dbg
V2 -> V3:
- no changes
V3 -> V4:
- drop extra cc stable cherry-picks, as a request was made to stable
drivers/usb/gadget/function/uvc_queue.c | 8 +++++---
drivers/usb/gadget/function/uvc_video.c | 18 ++++++++++++++----
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index ec500ee499ee..0aa3d7e1f3cc 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -304,6 +304,7 @@ int uvcg_queue_enable(struct uvc_video_queue *queue, int enable)
queue->sequence = 0;
queue->buf_used = 0;
+ queue->flags &= ~UVC_QUEUE_DROP_INCOMPLETE;
} else {
ret = vb2_streamoff(&queue->queue, queue->queue.type);
if (ret < 0)
@@ -329,10 +330,11 @@ int uvcg_queue_enable(struct uvc_video_queue *queue, int enable)
void uvcg_complete_buffer(struct uvc_video_queue *queue,
struct uvc_buffer *buf)
{
- if ((queue->flags & UVC_QUEUE_DROP_INCOMPLETE) &&
- buf->length != buf->bytesused) {
- buf->state = UVC_BUF_STATE_QUEUED;
+ if (queue->flags & UVC_QUEUE_DROP_INCOMPLETE) {
+ queue->flags &= ~UVC_QUEUE_DROP_INCOMPLETE;
+ buf->state = UVC_BUF_STATE_ERROR;
vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
+ vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR);
return;
}
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 323977716f5a..5993e083819c 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -88,6 +88,7 @@ uvc_video_encode_bulk(struct usb_request *req, struct uvc_video *video,
struct uvc_buffer *buf)
{
void *mem = req->buf;
+ struct uvc_request *ureq = req->context;
int len = video->req_size;
int ret;
@@ -113,13 +114,14 @@ uvc_video_encode_bulk(struct usb_request *req, struct uvc_video *video,
video->queue.buf_used = 0;
buf->state = UVC_BUF_STATE_DONE;
list_del(&buf->queue);
- uvcg_complete_buffer(&video->queue, buf);
video->fid ^= UVC_STREAM_FID;
+ ureq->last_buf = buf;
video->payload_size = 0;
}
if (video->payload_size == video->max_payload_size ||
+ video->queue.flags & UVC_QUEUE_DROP_INCOMPLETE ||
buf->bytesused == video->queue.buf_used)
video->payload_size = 0;
}
@@ -180,7 +182,8 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
req->length -= len;
video->queue.buf_used += req->length - header_len;
- if (buf->bytesused == video->queue.buf_used || !buf->sg) {
+ if (buf->bytesused == video->queue.buf_used || !buf->sg ||
+ video->queue.flags & UVC_QUEUE_DROP_INCOMPLETE) {
video->queue.buf_used = 0;
buf->state = UVC_BUF_STATE_DONE;
buf->offset = 0;
@@ -195,6 +198,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
struct uvc_buffer *buf)
{
void *mem = req->buf;
+ struct uvc_request *ureq = req->context;
int len = video->req_size;
int ret;
@@ -209,12 +213,13 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
req->length = video->req_size - len;
- if (buf->bytesused == video->queue.buf_used) {
+ if (buf->bytesused == video->queue.buf_used ||
+ video->queue.flags & UVC_QUEUE_DROP_INCOMPLETE) {
video->queue.buf_used = 0;
buf->state = UVC_BUF_STATE_DONE;
list_del(&buf->queue);
- uvcg_complete_buffer(&video->queue, buf);
video->fid ^= UVC_STREAM_FID;
+ ureq->last_buf = buf;
}
}
@@ -255,6 +260,11 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
case 0:
break;
+ case -EXDEV:
+ uvcg_dbg(&video->uvc->func, "VS request missed xfer.\n");
+ queue->flags |= UVC_QUEUE_DROP_INCOMPLETE;
+ break;
+
case -ESHUTDOWN: /* disconnect from host. */
uvcg_dbg(&video->uvc->func, "VS request cancelled.\n");
uvcg_queue_cancel(queue, 1);
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v4 4/6] usb: gadget: uvc: fix sg handling during video encode
2022-10-18 21:50 [PATCH v4 0/6] uvc gadget performance issues Dan Vacura
` (2 preceding siblings ...)
2022-10-18 21:50 ` [PATCH v4 3/6] usb: gadget: uvc: fix sg handling in error case Dan Vacura
@ 2022-10-18 21:50 ` Dan Vacura
2022-10-18 21:50 ` [PATCH v4 5/6] usb: gadget: uvc: make interrupt skip logic configurable Dan Vacura
2022-10-18 21:50 ` [PATCH v4 6/6] usb: gadget: uvc: add configfs option for sg support Dan Vacura
5 siblings, 0 replies; 14+ messages in thread
From: Dan Vacura @ 2022-10-18 21:50 UTC (permalink / raw)
To: linux-usb
Cc: Daniel Scally, Thinh Nguyen, Jeff Vanhoof, stable, Dan Vacura,
Greg Kroah-Hartman, Jonathan Corbet, Laurent Pinchart,
Felipe Balbi, Paul Elder, Michael Grzeschik, linux-kernel,
linux-doc
From: Jeff Vanhoof <qjv001@motorola.com>
In uvc_video_encode_isoc_sg, the uvc_request's sg list is
incorrectly being populated leading to corrupt video being
received by the remote end. When building the sg list the
usage of buf->sg's 'dma_length' field is not correct and
instead its 'length' field should be used.
Fixes: e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V3:
- no change, new patch in series
V3 -> V4:
- no change
drivers/usb/gadget/function/uvc_video.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 5993e083819c..dd1c6b2ca7c6 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -157,10 +157,10 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
sg = sg_next(sg);
for_each_sg(sg, iter, ureq->sgt.nents - 1, i) {
- if (!len || !buf->sg || !sg_dma_len(buf->sg))
+ if (!len || !buf->sg || !buf->sg->length)
break;
- sg_left = sg_dma_len(buf->sg) - buf->offset;
+ sg_left = buf->sg->length - buf->offset;
part = min_t(unsigned int, len, sg_left);
sg_set_page(iter, sg_page(buf->sg), part, buf->offset);
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v4 5/6] usb: gadget: uvc: make interrupt skip logic configurable
2022-10-18 21:50 [PATCH v4 0/6] uvc gadget performance issues Dan Vacura
` (3 preceding siblings ...)
2022-10-18 21:50 ` [PATCH v4 4/6] usb: gadget: uvc: fix sg handling during video encode Dan Vacura
@ 2022-10-18 21:50 ` Dan Vacura
2022-10-22 11:32 ` Greg Kroah-Hartman
2022-10-18 21:50 ` [PATCH v4 6/6] usb: gadget: uvc: add configfs option for sg support Dan Vacura
5 siblings, 1 reply; 14+ messages in thread
From: Dan Vacura @ 2022-10-18 21:50 UTC (permalink / raw)
To: linux-usb
Cc: Daniel Scally, Thinh Nguyen, Jeff Vanhoof, Dan Vacura,
Greg Kroah-Hartman, Jonathan Corbet, Laurent Pinchart,
Felipe Balbi, Paul Elder, Michael Grzeschik, linux-kernel,
linux-doc
Some UDC hw may not support skipping interrupts, but still support the
request. Allow the interrupt frequency to be configurable to the user.
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V2:
- no change, new patch in series
V2 -> V3:
- default to baseline value of 4, fix storing the initial value
V3 -> V4:
- no change
Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
Documentation/usb/gadget-testing.rst | 2 ++
drivers/usb/gadget/function/f_uvc.c | 3 +++
drivers/usb/gadget/function/u_uvc.h | 1 +
drivers/usb/gadget/function/uvc.h | 2 ++
drivers/usb/gadget/function/uvc_configfs.c | 2 ++
drivers/usb/gadget/function/uvc_queue.c | 6 ++++++
drivers/usb/gadget/function/uvc_video.c | 3 ++-
8 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 611b23e6488d..5dfaa3f7f6a4 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -8,6 +8,7 @@ Description: UVC function directory
streaming_maxpacket 1..1023 (fs), 1..3072 (hs/ss)
streaming_interval 1..16
function_name string [32]
+ req_int_skip_div unsigned int
=================== =============================
What: /config/usb-gadget/gadget/functions/uvc.name/control
diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index 2278c9ffb74a..f9b5a09be1f4 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -794,6 +794,8 @@ The uvc function provides these attributes in its function directory:
sending or receiving when this configuration is
selected
function_name name of the interface
+ req_int_skip_div divisor of total requests to aid in calculating
+ interrupt frequency, 0 indicates all interrupt
=================== ================================================
There are also "control" and "streaming" subdirectories, each of which contain
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index 6e196e06181e..e40ca26b9c55 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -655,6 +655,8 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f)
cpu_to_le16(max_packet_size * max_packet_mult *
(opts->streaming_maxburst + 1));
+ uvc->config_skip_int_div = opts->req_int_skip_div;
+
/* Allocate endpoints. */
ep = usb_ep_autoconfig(cdev->gadget, &uvc_control_ep);
if (!ep) {
@@ -872,6 +874,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
opts->streaming_interval = 1;
opts->streaming_maxpacket = 1024;
+ opts->req_int_skip_div = 4;
snprintf(opts->function_name, sizeof(opts->function_name), "UVC Camera");
ret = uvcg_attach_configfs(opts);
diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
index 24b8681b0d6f..6f73bd5638ed 100644
--- a/drivers/usb/gadget/function/u_uvc.h
+++ b/drivers/usb/gadget/function/u_uvc.h
@@ -24,6 +24,7 @@ struct f_uvc_opts {
unsigned int streaming_interval;
unsigned int streaming_maxpacket;
unsigned int streaming_maxburst;
+ unsigned int req_int_skip_div;
unsigned int control_interface;
unsigned int streaming_interface;
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 40226b1f7e14..29f9477c92cc 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -107,6 +107,7 @@ struct uvc_video {
spinlock_t req_lock;
unsigned int req_int_count;
+ unsigned int req_int_skip_div;
void (*encode) (struct usb_request *req, struct uvc_video *video,
struct uvc_buffer *buf);
@@ -155,6 +156,7 @@ struct uvc_device {
/* Events */
unsigned int event_length;
unsigned int event_setup_out : 1;
+ unsigned int config_skip_int_div;
};
static inline struct uvc_device *to_uvc(struct usb_function *f)
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 4303a3283ba0..419e926ab57e 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2350,6 +2350,7 @@ UVC_ATTR(f_uvc_opts_, cname, cname)
UVCG_OPTS_ATTR(streaming_interval, streaming_interval, 16);
UVCG_OPTS_ATTR(streaming_maxpacket, streaming_maxpacket, 3072);
UVCG_OPTS_ATTR(streaming_maxburst, streaming_maxburst, 15);
+UVCG_OPTS_ATTR(req_int_skip_div, req_int_skip_div, UINT_MAX);
#undef UVCG_OPTS_ATTR
@@ -2399,6 +2400,7 @@ static struct configfs_attribute *uvc_attrs[] = {
&f_uvc_opts_attr_streaming_interval,
&f_uvc_opts_attr_streaming_maxpacket,
&f_uvc_opts_attr_streaming_maxburst,
+ &f_uvc_opts_attr_req_int_skip_div,
&f_uvc_opts_string_attr_function_name,
NULL,
};
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 0aa3d7e1f3cc..02559906a55a 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -63,6 +63,12 @@ static int uvc_queue_setup(struct vb2_queue *vq,
*/
nreq = DIV_ROUND_UP(DIV_ROUND_UP(sizes[0], 2), req_size);
nreq = clamp(nreq, 4U, 64U);
+ if (0 == video->uvc->config_skip_int_div) {
+ video->req_int_skip_div = nreq;
+ } else {
+ video->req_int_skip_div = min_t(unsigned int, nreq,
+ video->uvc->config_skip_int_div);
+ }
video->uvc_num_requests = nreq;
return 0;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index dd1c6b2ca7c6..c1a80c5d1f63 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -423,7 +423,8 @@ static void uvcg_video_pump(struct work_struct *work)
if (list_empty(&video->req_free) ||
buf->state == UVC_BUF_STATE_DONE ||
!(video->req_int_count %
- DIV_ROUND_UP(video->uvc_num_requests, 4))) {
+ DIV_ROUND_UP(video->uvc_num_requests,
+ video->req_int_skip_div))) {
video->req_int_count = 0;
req->no_interrupt = 0;
} else {
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v4 5/6] usb: gadget: uvc: make interrupt skip logic configurable
2022-10-18 21:50 ` [PATCH v4 5/6] usb: gadget: uvc: make interrupt skip logic configurable Dan Vacura
@ 2022-10-22 11:32 ` Greg Kroah-Hartman
0 siblings, 0 replies; 14+ messages in thread
From: Greg Kroah-Hartman @ 2022-10-22 11:32 UTC (permalink / raw)
To: Dan Vacura
Cc: linux-usb, Daniel Scally, Thinh Nguyen, Jeff Vanhoof,
Jonathan Corbet, Laurent Pinchart, Felipe Balbi, Paul Elder,
Michael Grzeschik, linux-kernel, linux-doc
On Tue, Oct 18, 2022 at 04:50:41PM -0500, Dan Vacura wrote:
> Some UDC hw may not support skipping interrupts, but still support the
> request. Allow the interrupt frequency to be configurable to the user.
>
> Signed-off-by: Dan Vacura <w36195@motorola.com>
> ---
> V1 -> V2:
> - no change, new patch in series
> V2 -> V3:
> - default to baseline value of 4, fix storing the initial value
> V3 -> V4:
> - no change
>
> Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
> Documentation/usb/gadget-testing.rst | 2 ++
Adding more tunable knobs always just adds complexity and fragility as
things do not get tested.
Can't we just make this dynamic and work properly on all systems? What
UDC hardware types do not allow skipping interrupts? Why can't we just
detect that and only do that if this is the case or not?
> drivers/usb/gadget/function/f_uvc.c | 3 +++
> drivers/usb/gadget/function/u_uvc.h | 1 +
> drivers/usb/gadget/function/uvc.h | 2 ++
> drivers/usb/gadget/function/uvc_configfs.c | 2 ++
> drivers/usb/gadget/function/uvc_queue.c | 6 ++++++
> drivers/usb/gadget/function/uvc_video.c | 3 ++-
> 8 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 611b23e6488d..5dfaa3f7f6a4 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -8,6 +8,7 @@ Description: UVC function directory
> streaming_maxpacket 1..1023 (fs), 1..3072 (hs/ss)
> streaming_interval 1..16
> function_name string [32]
> + req_int_skip_div unsigned int
> =================== =============================
>
> What: /config/usb-gadget/gadget/functions/uvc.name/control
> diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
> index 2278c9ffb74a..f9b5a09be1f4 100644
> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -794,6 +794,8 @@ The uvc function provides these attributes in its function directory:
> sending or receiving when this configuration is
> selected
> function_name name of the interface
> + req_int_skip_div divisor of total requests to aid in calculating
> + interrupt frequency, 0 indicates all interrupt
This provides no information that would help to determine how to
configure this at all. I wouldn't know what to do with this, so that's
a huge sign that this is not a good interface :(
thanks,
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 6/6] usb: gadget: uvc: add configfs option for sg support
2022-10-18 21:50 [PATCH v4 0/6] uvc gadget performance issues Dan Vacura
` (4 preceding siblings ...)
2022-10-18 21:50 ` [PATCH v4 5/6] usb: gadget: uvc: make interrupt skip logic configurable Dan Vacura
@ 2022-10-18 21:50 ` Dan Vacura
2022-10-22 11:34 ` Greg Kroah-Hartman
5 siblings, 1 reply; 14+ messages in thread
From: Dan Vacura @ 2022-10-18 21:50 UTC (permalink / raw)
To: linux-usb
Cc: Daniel Scally, Thinh Nguyen, Jeff Vanhoof, Dan Vacura,
Greg Kroah-Hartman, Jonathan Corbet, Laurent Pinchart,
Felipe Balbi, Michael Grzeschik, Paul Elder, linux-kernel,
linux-doc
The scatter gather support doesn't appear to work well with some UDC hw.
Add the ability to turn off the feature depending on the controller in
use or other platform quirks. The default is for the uvc gadget to
support sg as long as the UDC hw supports it.
The specific failure was with the dwc3 controller, but fixes and
improvements are pending for those failures. This capability is now
more intended for future unexpected failures or poor sg support on a
given platform.
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V2:
- no change, new patch in series
V2 -> V3:
- default on, same as baseline
V3 -> V4:
- update comment and documentation, refactor use of opts->sg_supported
directly in uvc_queue
Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
Documentation/usb/gadget-testing.rst | 4 ++++
drivers/usb/gadget/function/f_uvc.c | 1 +
drivers/usb/gadget/function/u_uvc.h | 1 +
drivers/usb/gadget/function/uvc_configfs.c | 2 ++
drivers/usb/gadget/function/uvc_queue.c | 4 +++-
6 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 5dfaa3f7f6a4..839a75fc28ee 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -9,6 +9,7 @@ Description: UVC function directory
streaming_interval 1..16
function_name string [32]
req_int_skip_div unsigned int
+ sg_supported 0..1
=================== =============================
What: /config/usb-gadget/gadget/functions/uvc.name/control
diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index f9b5a09be1f4..be72577a03e9 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -796,6 +796,10 @@ The uvc function provides these attributes in its function directory:
function_name name of the interface
req_int_skip_div divisor of total requests to aid in calculating
interrupt frequency, 0 indicates all interrupt
+ sg_supported allow for scatter gather to be used if the UDC
+ hw supports it, this is default on and only
+ intended to be temporally turned off if a given
+ platform doesn't work well with scatter gather
=================== ================================================
There are also "control" and "streaming" subdirectories, each of which contain
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index e40ca26b9c55..83d96e81c05f 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -875,6 +875,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
opts->streaming_interval = 1;
opts->streaming_maxpacket = 1024;
opts->req_int_skip_div = 4;
+ opts->sg_supported = 1;
snprintf(opts->function_name, sizeof(opts->function_name), "UVC Camera");
ret = uvcg_attach_configfs(opts);
diff --git a/drivers/usb/gadget/function/u_uvc.h b/drivers/usb/gadget/function/u_uvc.h
index 6f73bd5638ed..5ccced629925 100644
--- a/drivers/usb/gadget/function/u_uvc.h
+++ b/drivers/usb/gadget/function/u_uvc.h
@@ -25,6 +25,7 @@ struct f_uvc_opts {
unsigned int streaming_maxpacket;
unsigned int streaming_maxburst;
unsigned int req_int_skip_div;
+ unsigned int sg_supported;
unsigned int control_interface;
unsigned int streaming_interface;
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 419e926ab57e..3784c0e02d01 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2351,6 +2351,7 @@ UVCG_OPTS_ATTR(streaming_interval, streaming_interval, 16);
UVCG_OPTS_ATTR(streaming_maxpacket, streaming_maxpacket, 3072);
UVCG_OPTS_ATTR(streaming_maxburst, streaming_maxburst, 15);
UVCG_OPTS_ATTR(req_int_skip_div, req_int_skip_div, UINT_MAX);
+UVCG_OPTS_ATTR(sg_supported, sg_supported, 1);
#undef UVCG_OPTS_ATTR
@@ -2401,6 +2402,7 @@ static struct configfs_attribute *uvc_attrs[] = {
&f_uvc_opts_attr_streaming_maxpacket,
&f_uvc_opts_attr_streaming_maxburst,
&f_uvc_opts_attr_req_int_skip_div,
+ &f_uvc_opts_attr_sg_supported,
&f_uvc_opts_string_attr_function_name,
NULL,
};
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 02559906a55a..aee405663d6e 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -21,6 +21,7 @@
#include <media/videobuf2-vmalloc.h>
#include "uvc.h"
+#include "u_uvc.h"
/* ------------------------------------------------------------------------
* Video buffers queue management.
@@ -141,6 +142,7 @@ int uvcg_queue_init(struct uvc_video_queue *queue, struct device *dev, enum v4l2
{
struct uvc_video *video = container_of(queue, struct uvc_video, queue);
struct usb_composite_dev *cdev = video->uvc->func.config->cdev;
+ struct f_uvc_opts *opts = fi_to_f_uvc_opts(video->uvc->func.fi);
int ret;
queue->queue.type = type;
@@ -149,7 +151,7 @@ int uvcg_queue_init(struct uvc_video_queue *queue, struct device *dev, enum v4l2
queue->queue.buf_struct_size = sizeof(struct uvc_buffer);
queue->queue.ops = &uvc_queue_qops;
queue->queue.lock = lock;
- if (cdev->gadget->sg_supported) {
+ if (opts->sg_supported && cdev->gadget->sg_supported) {
queue->queue.mem_ops = &vb2_dma_sg_memops;
queue->use_sg = 1;
} else {
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v4 6/6] usb: gadget: uvc: add configfs option for sg support
2022-10-18 21:50 ` [PATCH v4 6/6] usb: gadget: uvc: add configfs option for sg support Dan Vacura
@ 2022-10-22 11:34 ` Greg Kroah-Hartman
0 siblings, 0 replies; 14+ messages in thread
From: Greg Kroah-Hartman @ 2022-10-22 11:34 UTC (permalink / raw)
To: Dan Vacura
Cc: linux-usb, Daniel Scally, Thinh Nguyen, Jeff Vanhoof,
Jonathan Corbet, Laurent Pinchart, Felipe Balbi,
Michael Grzeschik, Paul Elder, linux-kernel, linux-doc
On Tue, Oct 18, 2022 at 04:50:42PM -0500, Dan Vacura wrote:
> The scatter gather support doesn't appear to work well with some UDC hw.
> Add the ability to turn off the feature depending on the controller in
> use or other platform quirks. The default is for the uvc gadget to
> support sg as long as the UDC hw supports it.
>
> The specific failure was with the dwc3 controller, but fixes and
> improvements are pending for those failures. This capability is now
> more intended for future unexpected failures or poor sg support on a
> given platform.
>
> Signed-off-by: Dan Vacura <w36195@motorola.com>
Again, this should be dynamic. Can't we detect this based on the packet
size and either do sg or not?
If the UDC hardware says it is supported, it should be supported.
Otherwise we need to fix the UDC hardware or it saying it is allowed.
> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -796,6 +796,10 @@ The uvc function provides these attributes in its function directory:
> function_name name of the interface
> req_int_skip_div divisor of total requests to aid in calculating
> interrupt frequency, 0 indicates all interrupt
> + sg_supported allow for scatter gather to be used if the UDC
> + hw supports it, this is default on and only
> + intended to be temporally turned off if a given
> + platform doesn't work well with scatter gather
How do you know if it "doesn't work well"?
That's vague and not good and nothing we want to support for forever,
sorry.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread