There are some issues to handle frame throughput with camera devices
Using devices:
- Logitech Webcam
- Intel(R) RealSense(TM) Depth Camera
To improve efficiency, and maximise throughput,
use dynamic allocation for uvc_copy_op.
Change from struct uvc_copy_op copy_operations[UVC_MAX_PACKETS];
to struct uvc_copy_op *copy_operations;
Now, only tested bulk video options.
On test device & own debug log to check frame duration(us),
refer to test result the below:
Use copy_operations[UVC_MAX_PACKETS]
[UVC] Check time duration(us) : 54732 / 66000
[UVC] Check time duration(us) : 57452 / 66000
[UVC] Check time duration(us) : 57413 / 66000
[UVC] Check time duration(us) : 56713 / 66000
[UVC] Check time duration(us) : 57967 / 66000
Use *copy_operations
[UVC] Check time duration(us) : 30804 / 66000
[UVC] Check time duration(us) : 38642 / 66000
[UVC] Check time duration(us) : 26011 / 66000
[UVC] Check time duration(us) : 30116 / 66000
[UVC] Check time duration(us) : 29265 / 66000
Signed-off-by: Kyeongdon Kim <[email protected]>
---
drivers/media/usb/uvc/uvc_video.c | 55 ++++++++++++++++++++++++++++++++++++---
drivers/media/usb/uvc/uvcvideo.h | 3 ++-
2 files changed, 54 insertions(+), 4 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index e164646..3a7c131 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1614,6 +1614,36 @@ static void uvc_video_complete(struct urb *urb)
queue_work(stream->async_wq, &uvc_urb->work);
}
+static void uvc_free_urb_cop(struct uvc_streaming *stream)
+{
+ struct uvc_urb *uvc_urb;
+
+ for_each_uvc_urb(uvc_urb, stream) {
+ if (uvc_urb->copy_operations) {
+ kfree(uvc_urb->copy_operations);
+ uvc_urb->copy_operations = NULL;
+ }
+ }
+}
+
+static int uvc_alloc_urb_cop(struct uvc_streaming *stream, gfp_t gfp_flags)
+{
+ int max_packet = stream->urb_max_packets;
+ struct uvc_urb *uvc_urb;
+
+ for_each_uvc_urb(uvc_urb, stream) {
+ uvc_urb->copy_operations
+ = kcalloc(max_packet, sizeof(struct uvc_copy_op), gfp_flags);
+ if (uvc_urb->copy_operations == NULL)
+ goto error;
+ }
+ return 0;
+error:
+ uvc_free_urb_cop(stream);
+
+ return -ENOMEM;
+}
+
/*
* Free transfer buffers.
*/
@@ -1687,8 +1717,8 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
* payloads across multiple URBs.
*/
npackets = DIV_ROUND_UP(size, psize);
- if (npackets > UVC_MAX_PACKETS)
- npackets = UVC_MAX_PACKETS;
+ if (npackets > stream->urb_max_packets)
+ npackets = stream->urb_max_packets;
/* Retry allocations until one succeed. */
for (; npackets > 1; npackets /= 2) {
@@ -1744,8 +1774,10 @@ static void uvc_video_stop_transfer(struct uvc_streaming *stream,
uvc_urb->urb = NULL;
}
- if (free_buffers)
+ if (free_buffers) {
uvc_free_urb_buffers(stream);
+ uvc_free_urb_cop(stream);
+ }
}
/*
@@ -1790,10 +1822,18 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
psize = uvc_endpoint_max_bpi(stream->dev->udev, ep);
size = stream->ctrl.dwMaxVideoFrameSize;
+ stream->urb_max_packets = UVC_MAX_PACKETS;
+
npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
if (npackets == 0)
return -ENOMEM;
+ if (uvc_alloc_urb_cop(stream, gfp_flags) != 0) {
+ uvc_dbg(stream->dev, VIDEO,
+ "Failed to init URBs copy operations.\n");
+ return -ENOMEM;
+ }
+
size = npackets * psize;
for_each_uvc_urb(uvc_urb, stream) {
@@ -1842,11 +1882,18 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
psize = usb_endpoint_maxp(&ep->desc);
size = stream->ctrl.dwMaxPayloadTransferSize;
stream->bulk.max_payload_size = size;
+ stream->urb_max_packets = DIV_ROUND_UP(size, psize);
npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
if (npackets == 0)
return -ENOMEM;
+ if (uvc_alloc_urb_cop(stream, gfp_flags) != 0) {
+ uvc_dbg(stream->dev, VIDEO,
+ "Failed to init URBs copy operations.\n");
+ return -ENOMEM;
+ }
+
size = npackets * psize;
if (usb_endpoint_dir_in(&ep->desc))
@@ -2147,6 +2194,8 @@ int uvc_video_init(struct uvc_streaming *stream)
}
}
+ stream->urb_max_packets = UVC_MAX_PACKETS;
+
/* Prepare asynchronous work items. */
for_each_uvc_urb(uvc_urb, stream)
INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index cce5e38..00cf6c9 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -561,7 +561,7 @@ struct uvc_urb {
struct sg_table *sgt;
unsigned int async_operations;
- struct uvc_copy_op copy_operations[UVC_MAX_PACKETS];
+ struct uvc_copy_op *copy_operations;
struct work_struct work;
};
@@ -616,6 +616,7 @@ struct uvc_streaming {
struct uvc_urb uvc_urb[UVC_URBS];
unsigned int urb_size;
+ unsigned int urb_max_packets;
u32 sequence;
u8 last_fid;
--
2.10.2
Hi Kyeongdon
I fail to understand why your approach is faster than the original one.
Is it because of the alignment of struct uvc_copy_op copy_operations ?
Thanks!
On Fri, Sep 3, 2021 at 6:56 AM Kyeongdon Kim <[email protected]> wrote:
>
> There are some issues to handle frame throughput with camera devices
>
> Using devices:
> - Logitech Webcam
> - Intel(R) RealSense(TM) Depth Camera
>
> To improve efficiency, and maximise throughput,
> use dynamic allocation for uvc_copy_op.
>
> Change from struct uvc_copy_op copy_operations[UVC_MAX_PACKETS];
> to struct uvc_copy_op *copy_operations;
>
> Now, only tested bulk video options.
>
> On test device & own debug log to check frame duration(us),
> refer to test result the below:
>
> Use copy_operations[UVC_MAX_PACKETS]
> [UVC] Check time duration(us) : 54732 / 66000
> [UVC] Check time duration(us) : 57452 / 66000
> [UVC] Check time duration(us) : 57413 / 66000
> [UVC] Check time duration(us) : 56713 / 66000
> [UVC] Check time duration(us) : 57967 / 66000
>
> Use *copy_operations
> [UVC] Check time duration(us) : 30804 / 66000
> [UVC] Check time duration(us) : 38642 / 66000
> [UVC] Check time duration(us) : 26011 / 66000
> [UVC] Check time duration(us) : 30116 / 66000
> [UVC] Check time duration(us) : 29265 / 66000
>
> Signed-off-by: Kyeongdon Kim <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_video.c | 55 ++++++++++++++++++++++++++++++++++++---
> drivers/media/usb/uvc/uvcvideo.h | 3 ++-
> 2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index e164646..3a7c131 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1614,6 +1614,36 @@ static void uvc_video_complete(struct urb *urb)
> queue_work(stream->async_wq, &uvc_urb->work);
> }
>
> +static void uvc_free_urb_cop(struct uvc_streaming *stream)
> +{
> + struct uvc_urb *uvc_urb;
> +
> + for_each_uvc_urb(uvc_urb, stream) {
> + if (uvc_urb->copy_operations) {
> + kfree(uvc_urb->copy_operations);
> + uvc_urb->copy_operations = NULL;
> + }
> + }
> +}
> +
> +static int uvc_alloc_urb_cop(struct uvc_streaming *stream, gfp_t gfp_flags)
> +{
> + int max_packet = stream->urb_max_packets;
> + struct uvc_urb *uvc_urb;
> +
> + for_each_uvc_urb(uvc_urb, stream) {
> + uvc_urb->copy_operations
> + = kcalloc(max_packet, sizeof(struct uvc_copy_op), gfp_flags);
> + if (uvc_urb->copy_operations == NULL)
> + goto error;
> + }
> + return 0;
> +error:
> + uvc_free_urb_cop(stream);
> +
> + return -ENOMEM;
> +}
> +
> /*
> * Free transfer buffers.
> */
> @@ -1687,8 +1717,8 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
> * payloads across multiple URBs.
> */
> npackets = DIV_ROUND_UP(size, psize);
> - if (npackets > UVC_MAX_PACKETS)
> - npackets = UVC_MAX_PACKETS;
> + if (npackets > stream->urb_max_packets)
> + npackets = stream->urb_max_packets;
>
> /* Retry allocations until one succeed. */
> for (; npackets > 1; npackets /= 2) {
> @@ -1744,8 +1774,10 @@ static void uvc_video_stop_transfer(struct uvc_streaming *stream,
> uvc_urb->urb = NULL;
> }
>
> - if (free_buffers)
> + if (free_buffers) {
> uvc_free_urb_buffers(stream);
> + uvc_free_urb_cop(stream);
> + }
> }
>
> /*
> @@ -1790,10 +1822,18 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
> psize = uvc_endpoint_max_bpi(stream->dev->udev, ep);
> size = stream->ctrl.dwMaxVideoFrameSize;
>
> + stream->urb_max_packets = UVC_MAX_PACKETS;
> +
> npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
> if (npackets == 0)
> return -ENOMEM;
>
> + if (uvc_alloc_urb_cop(stream, gfp_flags) != 0) {
> + uvc_dbg(stream->dev, VIDEO,
> + "Failed to init URBs copy operations.\n");
> + return -ENOMEM;
> + }
> +
> size = npackets * psize;
>
> for_each_uvc_urb(uvc_urb, stream) {
> @@ -1842,11 +1882,18 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
> psize = usb_endpoint_maxp(&ep->desc);
> size = stream->ctrl.dwMaxPayloadTransferSize;
> stream->bulk.max_payload_size = size;
> + stream->urb_max_packets = DIV_ROUND_UP(size, psize);
>
> npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
> if (npackets == 0)
> return -ENOMEM;
>
> + if (uvc_alloc_urb_cop(stream, gfp_flags) != 0) {
> + uvc_dbg(stream->dev, VIDEO,
> + "Failed to init URBs copy operations.\n");
> + return -ENOMEM;
> + }
> +
> size = npackets * psize;
>
> if (usb_endpoint_dir_in(&ep->desc))
> @@ -2147,6 +2194,8 @@ int uvc_video_init(struct uvc_streaming *stream)
> }
> }
>
> + stream->urb_max_packets = UVC_MAX_PACKETS;
> +
> /* Prepare asynchronous work items. */
> for_each_uvc_urb(uvc_urb, stream)
> INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index cce5e38..00cf6c9 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -561,7 +561,7 @@ struct uvc_urb {
> struct sg_table *sgt;
>
> unsigned int async_operations;
> - struct uvc_copy_op copy_operations[UVC_MAX_PACKETS];
> + struct uvc_copy_op *copy_operations;
> struct work_struct work;
> };
>
> @@ -616,6 +616,7 @@ struct uvc_streaming {
>
> struct uvc_urb uvc_urb[UVC_URBS];
> unsigned int urb_size;
> + unsigned int urb_max_packets;
>
> u32 sequence;
> u8 last_fid;
> --
> 2.10.2
>
--
Ricardo Ribalda
On Fri, Nov 26, 2021 at 05:37:32PM +0100, Ricardo Ribalda Delgado wrote:
> Hi Kyeongdon
>
> I fail to understand why your approach is faster than the original one.
>
> Is it because of the alignment of struct uvc_copy_op copy_operations ?
I also wonder.
> On Fri, Sep 3, 2021 at 6:56 AM Kyeongdon Kim <[email protected]> wrote:
> >
> > There are some issues to handle frame throughput with camera devices
> >
> > Using devices:
> > - Logitech Webcam
> > - Intel(R) RealSense(TM) Depth Camera
> >
> > To improve efficiency, and maximise throughput,
> > use dynamic allocation for uvc_copy_op.
> >
> > Change from struct uvc_copy_op copy_operations[UVC_MAX_PACKETS];
> > to struct uvc_copy_op *copy_operations;
> >
> > Now, only tested bulk video options.
> >
> > On test device & own debug log to check frame duration(us),
> > refer to test result the below:
> >
> > Use copy_operations[UVC_MAX_PACKETS]
> > [UVC] Check time duration(us) : 54732 / 66000
> > [UVC] Check time duration(us) : 57452 / 66000
> > [UVC] Check time duration(us) : 57413 / 66000
> > [UVC] Check time duration(us) : 56713 / 66000
> > [UVC] Check time duration(us) : 57967 / 66000
> >
> > Use *copy_operations
> > [UVC] Check time duration(us) : 30804 / 66000
> > [UVC] Check time duration(us) : 38642 / 66000
> > [UVC] Check time duration(us) : 26011 / 66000
> > [UVC] Check time duration(us) : 30116 / 66000
> > [UVC] Check time duration(us) : 29265 / 66000
> >
> > Signed-off-by: Kyeongdon Kim <[email protected]>
> > ---
> > drivers/media/usb/uvc/uvc_video.c | 55 ++++++++++++++++++++++++++++++++++++---
> > drivers/media/usb/uvc/uvcvideo.h | 3 ++-
> > 2 files changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index e164646..3a7c131 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1614,6 +1614,36 @@ static void uvc_video_complete(struct urb *urb)
> > queue_work(stream->async_wq, &uvc_urb->work);
> > }
> >
> > +static void uvc_free_urb_cop(struct uvc_streaming *stream)
> > +{
> > + struct uvc_urb *uvc_urb;
> > +
> > + for_each_uvc_urb(uvc_urb, stream) {
> > + if (uvc_urb->copy_operations) {
You can drop this check, kfree() is a no-op when called with a NULL
pointer.
> > + kfree(uvc_urb->copy_operations);
> > + uvc_urb->copy_operations = NULL;
Wouldn't it be better to move this to uvc_free_urb_buffers() ?
> > + }
> > + }
> > +}
> > +
> > +static int uvc_alloc_urb_cop(struct uvc_streaming *stream, gfp_t gfp_flags)
> > +{
> > + int max_packet = stream->urb_max_packets;
unsigned int. But you can drop the line, see below.
> > + struct uvc_urb *uvc_urb;
> > +
> > + for_each_uvc_urb(uvc_urb, stream) {
> > + uvc_urb->copy_operations
> > + = kcalloc(max_packet, sizeof(struct uvc_copy_op), gfp_flags);
sizeof(variable) is preferred over sizeof(type).
uvc_urb->copy_operations = kcalloc(stream->urb_max_packets,
sizeof(*uvc_urb->copy_operations),
gfp_flags);
> > + if (uvc_urb->copy_operations == NULL)
> > + goto error;
> > + }
> > + return 0;
Blank line.
> > +error:
> > + uvc_free_urb_cop(stream);
> > +
> > + return -ENOMEM;
> > +}
> > +
> > /*
> > * Free transfer buffers.
> > */
> > @@ -1687,8 +1717,8 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming *stream,
> > * payloads across multiple URBs.
> > */
> > npackets = DIV_ROUND_UP(size, psize);
> > - if (npackets > UVC_MAX_PACKETS)
> > - npackets = UVC_MAX_PACKETS;
> > + if (npackets > stream->urb_max_packets)
> > + npackets = stream->urb_max_packets;
> >
> > /* Retry allocations until one succeed. */
> > for (; npackets > 1; npackets /= 2) {
> > @@ -1744,8 +1774,10 @@ static void uvc_video_stop_transfer(struct uvc_streaming *stream,
> > uvc_urb->urb = NULL;
> > }
> >
> > - if (free_buffers)
> > + if (free_buffers) {
> > uvc_free_urb_buffers(stream);
> > + uvc_free_urb_cop(stream);
> > + }
> > }
> >
> > /*
> > @@ -1790,10 +1822,18 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
> > psize = uvc_endpoint_max_bpi(stream->dev->udev, ep);
> > size = stream->ctrl.dwMaxVideoFrameSize;
> >
> > + stream->urb_max_packets = UVC_MAX_PACKETS;
> > +
> > npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
> > if (npackets == 0)
> > return -ENOMEM;
> >
> > + if (uvc_alloc_urb_cop(stream, gfp_flags) != 0) {
> > + uvc_dbg(stream->dev, VIDEO,
> > + "Failed to init URBs copy operations.\n");
This message should move to uvc_alloc_urb_cop(). I would also propagate
the error:
ret = uvc_alloc_urb_cop(stream, gfp_flags);
if (ret < 0)
return ret;
But it would be better to call this from uvc_alloc_urb_buffers().
> > + return -ENOMEM;
> > + }
> > +
> > size = npackets * psize;
> >
> > for_each_uvc_urb(uvc_urb, stream) {
> > @@ -1842,11 +1882,18 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
> > psize = usb_endpoint_maxp(&ep->desc);
> > size = stream->ctrl.dwMaxPayloadTransferSize;
> > stream->bulk.max_payload_size = size;
> > + stream->urb_max_packets = DIV_ROUND_UP(size, psize);
This is an important change that should be explained in the commit
message. It may even deserve a patch of its own.
> >
> > npackets = uvc_alloc_urb_buffers(stream, size, psize, gfp_flags);
> > if (npackets == 0)
> > return -ENOMEM;
> >
> > + if (uvc_alloc_urb_cop(stream, gfp_flags) != 0) {
> > + uvc_dbg(stream->dev, VIDEO,
> > + "Failed to init URBs copy operations.\n");
> > + return -ENOMEM;
> > + }
> > +
> > size = npackets * psize;
> >
> > if (usb_endpoint_dir_in(&ep->desc))
> > @@ -2147,6 +2194,8 @@ int uvc_video_init(struct uvc_streaming *stream)
> > }
> > }
> >
> > + stream->urb_max_packets = UVC_MAX_PACKETS;
> > +
> > /* Prepare asynchronous work items. */
> > for_each_uvc_urb(uvc_urb, stream)
> > INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index cce5e38..00cf6c9 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -561,7 +561,7 @@ struct uvc_urb {
> > struct sg_table *sgt;
> >
> > unsigned int async_operations;
> > - struct uvc_copy_op copy_operations[UVC_MAX_PACKETS];
> > + struct uvc_copy_op *copy_operations;
> > struct work_struct work;
> > };
> >
> > @@ -616,6 +616,7 @@ struct uvc_streaming {
> >
> > struct uvc_urb uvc_urb[UVC_URBS];
> > unsigned int urb_size;
> > + unsigned int urb_max_packets;
> >
> > u32 sequence;
> > u8 last_fid;
--
Regards,
Laurent Pinchart