Re: [PATCH] media: uvcvideo: Use streaming DMA APIs to transfer buffers
On Mon, Sep 30, 2019 at 01:23:10AM -0700, Christoph Hellwig wrote: > And drivers really have no business looking at the dma mask. I have > a plan for dma_alloc_pages API that could replace that cruft, but > until then please use GFP_KERNEL and let the dma subsystem bounce > buffer if needed. Can you try this series: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma_alloc_pages and see if it does whay you need for usb?
Re: [PATCH] media: uvcvideo: Use streaming DMA APIs to transfer buffers
On Sat, Sep 28, 2019 at 11:33:16AM +0800, Nicolas Boichat wrote:
> > +static gfp_t uvc_alloc_gfp_flags(struct device *dev)
> > +{
> > + u64 mask = dma_get_mask(dev);
> > +
> > + if (dev->bus_dma_mask)
> > + mask &= dev->bus_dma_mask;
> > +
> > + if (mask < DMA_BIT_MASK(32) && IS_ENABLED(CONFIG_ZONE_DMA))
> > + return GFP_DMA;
> > +
> > + if (mask < DMA_BIT_MASK(64)) {
> > + if (IS_ENABLED(CONFIG_ZONE_DMA32))
> > + return GFP_DMA32;
>
> We're hitting issues with this on 64-bit ARM platform, where
> ZONE_DMA32 is enabled (default), the kmalloc allocation with GFP_DMA32
> fails.
>
> There is no slab cache for GFP_DMA32, so those calls are expected to
> fail, AFAICT there are no other (working) kmalloc(..., .. | GFP_DMA32)
> users in the kernel, so I don't think we want to add a cache. If this
> helps, some discussion here why the cache wasn't added:
> https://lore.kernel.org/patchwork/patch/1009563/#1198622
And drivers really have no business looking at the dma mask. I have
a plan for dma_alloc_pages API that could replace that cruft, but
until then please use GFP_KERNEL and let the dma subsystem bounce
buffer if needed.
Re: [PATCH] media: uvcvideo: Use streaming DMA APIs to transfer buffers
On Fri, Aug 2, 2019 at 9:13 PM Shik Chen wrote:
>
> Similar to the commit 1161db6776bd ("media: usb: pwc: Don't use coherent
> DMA buffers for ISO transfer") [1] for the pwc driver. Use streaming DMA
> APIs to transfer buffers and sync them explicitly, because accessing
> buffers allocated by usb_alloc_coherent() is slow on platforms without
> hardware coherent DMA.
>
> Tested on x86-64 (Intel Celeron 4305U) and armv7l (Rockchip RK3288) with
> Logitech Brio 4K camera at 1920x1080 using the WebRTC sample site [3].
>
> | | URB (us) | Decode (Gbps) | CPU (%) |
> |--||---|-|
> | x86-64 coherent | 53 +- 20 | 50.6 |0.24 |
> | x86-64 streaming | 55 +- 19 | 50.1 |0.25 |
> | armv7l coherent | 342 +- 379 | 1.8 |2.16 |
> | armv7l streaming | 99 +- 98 | 11.0 |0.36 |
>
> The performance characteristics are improved a lot on armv7l, and
> remained (almost) unchanged on x86-64. The code used for measurement can
> be found at [2].
>
> [1] https://git.kernel.org/torvalds/c/1161db6776bd
> [2] https://crrev.com/c/1729133
> [3] https://webrtc.github.io/samples/src/content/getusermedia/resolution/
>
> Signed-off-by: Shik Chen
> ---
> The allocated buffer could be as large as 768K when streaming 4K video.
> Ideally we should use some generic helper to allocate non-coherent
> memory in a more efficient way, such as https://lwn.net/Articles/774429/
> ("make the non-consistent DMA allocator more userful").
>
> But I cannot find any existing helper so a simple kzalloc() is used in
> this patch. The logic to figure out the DMA addressable GFP flags is
> similar to __dma_direct_alloc_pages() without the optimistic retries:
> https://elixir.bootlin.com/linux/v5.2.5/source/kernel/dma/direct.c#L96
>
> drivers/media/usb/uvc/uvc_video.c | 65 +--
> 1 file changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index 8fa77a81dd7f2c..962c35478896c4 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1539,6 +1539,8 @@ static void uvc_video_complete(struct urb *urb)
> * Process the URB headers, and optionally queue expensive memcpy
> tasks
> * to be deferred to a work queue.
> */
> + dma_sync_single_for_cpu(>dev->dev, urb->transfer_dma,
> + urb->transfer_buffer_length, DMA_FROM_DEVICE);
> stream->decode(uvc_urb, buf, buf_meta);
>
> /* If no async work is needed, resubmit the URB immediately. */
> @@ -1565,18 +1567,51 @@ static void uvc_free_urb_buffers(struct uvc_streaming
> *stream)
> if (!uvc_urb->buffer)
> continue;
>
> -#ifndef CONFIG_DMA_NONCOHERENT
> - usb_free_coherent(stream->dev->udev, stream->urb_size,
> - uvc_urb->buffer, uvc_urb->dma);
> -#else
> + dma_unmap_single(>dev->udev->dev, uvc_urb->dma,
> +stream->urb_size, DMA_FROM_DEVICE);
> kfree(uvc_urb->buffer);
> -#endif
> - uvc_urb->buffer = NULL;
> }
>
> stream->urb_size = 0;
> }
>
> +static gfp_t uvc_alloc_gfp_flags(struct device *dev)
> +{
> + u64 mask = dma_get_mask(dev);
> +
> + if (dev->bus_dma_mask)
> + mask &= dev->bus_dma_mask;
> +
> + if (mask < DMA_BIT_MASK(32) && IS_ENABLED(CONFIG_ZONE_DMA))
> + return GFP_DMA;
> +
> + if (mask < DMA_BIT_MASK(64)) {
> + if (IS_ENABLED(CONFIG_ZONE_DMA32))
> + return GFP_DMA32;
We're hitting issues with this on 64-bit ARM platform, where
ZONE_DMA32 is enabled (default), the kmalloc allocation with GFP_DMA32
fails.
There is no slab cache for GFP_DMA32, so those calls are expected to
fail, AFAICT there are no other (working) kmalloc(..., .. | GFP_DMA32)
users in the kernel, so I don't think we want to add a cache. If this
helps, some discussion here why the cache wasn't added:
https://lore.kernel.org/patchwork/patch/1009563/#1198622
This function looks out of place in a high-level driver, but from your
comment above (below ---), I guess we may have to live with that until
the kernel provides a better API?
For the time being, looking at how __dma_direct_alloc_pages works,
could you use alloc_pages_node (or dma_alloc_contiguous?) instead of
kmalloc, that would let you use GFP_DMA32 as needed?
> + if (IS_ENABLED(CONFIG_ZONE_DMA))
> + return GFP_DMA;
> + }
> +
> + return 0;
> +}
> +
> +static char *uvc_alloc_urb_buffer(struct device *dev, size_t size,
> + gfp_t gfp_flags, dma_addr_t *dma_handle)
> +{
> + void *buffer = kzalloc(size, gfp_flags | uvc_alloc_gfp_flags(dev));
> +
> + if (!buffer)
> + return NULL;
> +
> +
Re: [PATCH] media: uvcvideo: Use streaming DMA APIs to transfer buffers
On Fri, Aug 2, 2019 at 10:12 PM Shik Chen wrote:
>
> Similar to the commit 1161db6776bd ("media: usb: pwc: Don't use coherent
> DMA buffers for ISO transfer") [1] for the pwc driver. Use streaming DMA
> APIs to transfer buffers and sync them explicitly, because accessing
> buffers allocated by usb_alloc_coherent() is slow on platforms without
> hardware coherent DMA.
>
> Tested on x86-64 (Intel Celeron 4305U) and armv7l (Rockchip RK3288) with
> Logitech Brio 4K camera at 1920x1080 using the WebRTC sample site [3].
>
> | | URB (us) | Decode (Gbps) | CPU (%) |
> |--||---|-|
> | x86-64 coherent | 53 +- 20 | 50.6 |0.24 |
> | x86-64 streaming | 55 +- 19 | 50.1 |0.25 |
> | armv7l coherent | 342 +- 379 | 1.8 |2.16 |
> | armv7l streaming | 99 +- 98 | 11.0 |0.36 |
>
> The performance characteristics are improved a lot on armv7l, and
> remained (almost) unchanged on x86-64. The code used for measurement can
> be found at [2].
>
> [1] https://git.kernel.org/torvalds/c/1161db6776bd
> [2] https://crrev.com/c/1729133
> [3] https://webrtc.github.io/samples/src/content/getusermedia/resolution/
>
> Signed-off-by: Shik Chen
> ---
> The allocated buffer could be as large as 768K when streaming 4K video.
> Ideally we should use some generic helper to allocate non-coherent
> memory in a more efficient way, such as https://lwn.net/Articles/774429/
> ("make the non-consistent DMA allocator more userful").
>
> But I cannot find any existing helper so a simple kzalloc() is used in
> this patch. The logic to figure out the DMA addressable GFP flags is
> similar to __dma_direct_alloc_pages() without the optimistic retries:
> https://elixir.bootlin.com/linux/v5.2.5/source/kernel/dma/direct.c#L96
>
> drivers/media/usb/uvc/uvc_video.c | 65 +--
> 1 file changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index 8fa77a81dd7f2c..962c35478896c4 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1539,6 +1539,8 @@ static void uvc_video_complete(struct urb *urb)
> * Process the URB headers, and optionally queue expensive memcpy
> tasks
> * to be deferred to a work queue.
> */
> + dma_sync_single_for_cpu(>dev->dev, urb->transfer_dma,
> + urb->transfer_buffer_length, DMA_FROM_DEVICE);
> stream->decode(uvc_urb, buf, buf_meta);
>
> /* If no async work is needed, resubmit the URB immediately. */
> @@ -1565,18 +1567,51 @@ static void uvc_free_urb_buffers(struct uvc_streaming
> *stream)
> if (!uvc_urb->buffer)
> continue;
>
> -#ifndef CONFIG_DMA_NONCOHERENT
> - usb_free_coherent(stream->dev->udev, stream->urb_size,
> - uvc_urb->buffer, uvc_urb->dma);
> -#else
> + dma_unmap_single(>dev->udev->dev, uvc_urb->dma,
> +stream->urb_size, DMA_FROM_DEVICE);
> kfree(uvc_urb->buffer);
> -#endif
> - uvc_urb->buffer = NULL;
> }
>
> stream->urb_size = 0;
> }
>
> +static gfp_t uvc_alloc_gfp_flags(struct device *dev)
> +{
> + u64 mask = dma_get_mask(dev);
> +
> + if (dev->bus_dma_mask)
> + mask &= dev->bus_dma_mask;
> +
> + if (mask < DMA_BIT_MASK(32) && IS_ENABLED(CONFIG_ZONE_DMA))
> + return GFP_DMA;
> +
> + if (mask < DMA_BIT_MASK(64)) {
> + if (IS_ENABLED(CONFIG_ZONE_DMA32))
> + return GFP_DMA32;
> + if (IS_ENABLED(CONFIG_ZONE_DMA))
> + return GFP_DMA;
> + }
> +
> + return 0;
> +}
> +
> +static char *uvc_alloc_urb_buffer(struct device *dev, size_t size,
> + gfp_t gfp_flags, dma_addr_t *dma_handle)
> +{
> + void *buffer = kzalloc(size, gfp_flags | uvc_alloc_gfp_flags(dev));
> +
> + if (!buffer)
> + return NULL;
> +
> + *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, *dma_handle)) {
> + kfree(buffer);
> + return NULL;
> + }
> +
> + return buffer;
> +}
> +
> /*
> * Allocate transfer buffers. This function can be called with buffers
> * already allocated when resuming from suspend, in which case it will
> @@ -1607,18 +1642,14 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
> *stream,
>
> /* Retry allocations until one succeed. */
> for (; npackets > 1; npackets /= 2) {
> + stream->urb_size = psize * npackets;
> +
> for (i = 0; i < UVC_URBS; ++i) {
> struct uvc_urb *uvc_urb = >uvc_urb[i];
>
> - stream->urb_size = psize * npackets;
>
[PATCH] media: uvcvideo: Use streaming DMA APIs to transfer buffers
Similar to the commit 1161db6776bd ("media: usb: pwc: Don't use coherent
DMA buffers for ISO transfer") [1] for the pwc driver. Use streaming DMA
APIs to transfer buffers and sync them explicitly, because accessing
buffers allocated by usb_alloc_coherent() is slow on platforms without
hardware coherent DMA.
Tested on x86-64 (Intel Celeron 4305U) and armv7l (Rockchip RK3288) with
Logitech Brio 4K camera at 1920x1080 using the WebRTC sample site [3].
| | URB (us) | Decode (Gbps) | CPU (%) |
|--||---|-|
| x86-64 coherent | 53 +- 20 | 50.6 |0.24 |
| x86-64 streaming | 55 +- 19 | 50.1 |0.25 |
| armv7l coherent | 342 +- 379 | 1.8 |2.16 |
| armv7l streaming | 99 +- 98 | 11.0 |0.36 |
The performance characteristics are improved a lot on armv7l, and
remained (almost) unchanged on x86-64. The code used for measurement can
be found at [2].
[1] https://git.kernel.org/torvalds/c/1161db6776bd
[2] https://crrev.com/c/1729133
[3] https://webrtc.github.io/samples/src/content/getusermedia/resolution/
Signed-off-by: Shik Chen
---
The allocated buffer could be as large as 768K when streaming 4K video.
Ideally we should use some generic helper to allocate non-coherent
memory in a more efficient way, such as https://lwn.net/Articles/774429/
("make the non-consistent DMA allocator more userful").
But I cannot find any existing helper so a simple kzalloc() is used in
this patch. The logic to figure out the DMA addressable GFP flags is
similar to __dma_direct_alloc_pages() without the optimistic retries:
https://elixir.bootlin.com/linux/v5.2.5/source/kernel/dma/direct.c#L96
drivers/media/usb/uvc/uvc_video.c | 65 +--
1 file changed, 45 insertions(+), 20 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 8fa77a81dd7f2c..962c35478896c4 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1539,6 +1539,8 @@ static void uvc_video_complete(struct urb *urb)
* Process the URB headers, and optionally queue expensive memcpy tasks
* to be deferred to a work queue.
*/
+ dma_sync_single_for_cpu(>dev->dev, urb->transfer_dma,
+ urb->transfer_buffer_length, DMA_FROM_DEVICE);
stream->decode(uvc_urb, buf, buf_meta);
/* If no async work is needed, resubmit the URB immediately. */
@@ -1565,18 +1567,51 @@ static void uvc_free_urb_buffers(struct uvc_streaming
*stream)
if (!uvc_urb->buffer)
continue;
-#ifndef CONFIG_DMA_NONCOHERENT
- usb_free_coherent(stream->dev->udev, stream->urb_size,
- uvc_urb->buffer, uvc_urb->dma);
-#else
+ dma_unmap_single(>dev->udev->dev, uvc_urb->dma,
+stream->urb_size, DMA_FROM_DEVICE);
kfree(uvc_urb->buffer);
-#endif
- uvc_urb->buffer = NULL;
}
stream->urb_size = 0;
}
+static gfp_t uvc_alloc_gfp_flags(struct device *dev)
+{
+ u64 mask = dma_get_mask(dev);
+
+ if (dev->bus_dma_mask)
+ mask &= dev->bus_dma_mask;
+
+ if (mask < DMA_BIT_MASK(32) && IS_ENABLED(CONFIG_ZONE_DMA))
+ return GFP_DMA;
+
+ if (mask < DMA_BIT_MASK(64)) {
+ if (IS_ENABLED(CONFIG_ZONE_DMA32))
+ return GFP_DMA32;
+ if (IS_ENABLED(CONFIG_ZONE_DMA))
+ return GFP_DMA;
+ }
+
+ return 0;
+}
+
+static char *uvc_alloc_urb_buffer(struct device *dev, size_t size,
+ gfp_t gfp_flags, dma_addr_t *dma_handle)
+{
+ void *buffer = kzalloc(size, gfp_flags | uvc_alloc_gfp_flags(dev));
+
+ if (!buffer)
+ return NULL;
+
+ *dma_handle = dma_map_single(dev, buffer, size, DMA_FROM_DEVICE);
+ if (dma_mapping_error(dev, *dma_handle)) {
+ kfree(buffer);
+ return NULL;
+ }
+
+ return buffer;
+}
+
/*
* Allocate transfer buffers. This function can be called with buffers
* already allocated when resuming from suspend, in which case it will
@@ -1607,18 +1642,14 @@ static int uvc_alloc_urb_buffers(struct uvc_streaming
*stream,
/* Retry allocations until one succeed. */
for (; npackets > 1; npackets /= 2) {
+ stream->urb_size = psize * npackets;
+
for (i = 0; i < UVC_URBS; ++i) {
struct uvc_urb *uvc_urb = >uvc_urb[i];
- stream->urb_size = psize * npackets;
-#ifndef CONFIG_DMA_NONCOHERENT
- uvc_urb->buffer = usb_alloc_coherent(
- stream->dev->udev, stream->urb_size,
+ uvc_urb->buffer = uvc_alloc_urb_buffer(
+ >dev->udev->dev,
