Re: [PATCH v3 2/3] media: uvcvideo: use usb_alloc_noncoherent/usb_free_noncoherent()

From: Xu Yang
Date: Thu Jul 03 2025 - 04:51:22 EST

Hi Ricardo,

On Wed, Jul 02, 2025 at 02:30:45PM +0200, Ricardo Ribalda wrote:
> Hi Xu
>
> The code looks much cleaner :)
>
> It seems that the hcd.c uses the urb transfer_flags to know the
> direction of the transfer.
> But the uvc driver is not setting it, you probably want to set it.

The USB HCD will get transfer direction from endpoint capability. So
we needn't add such info to transfer_flags.

>
> On Wed, 2 Jul 2025 at 13:01, Xu Yang <xu.yang_2@xxxxxxx> wrote:
> >
> > This will use USB noncoherent API to alloc/free urb buffers, then
> > uvc driver needn't to do dma sync operations by itself.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> >
> > ---
> > Changes in v3:
> > - no changes
> > ---
> > drivers/media/usb/uvc/uvc_video.c | 56 ++++++++-----------------------
> > 1 file changed, 14 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index e3567aeb0007..614cf4781221 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1280,15 +1280,6 @@ static inline struct device *uvc_stream_to_dmadev(struct uvc_streaming *stream)
> > return bus_to_hcd(stream->dev->udev->bus)->self.sysdev;
> > }
>
> The uvc_stream_to_dmadev() function is not used anymore, please drop it.

Sure.

Thanks,
Xu Yang

>
>
>
> >
> > -static int uvc_submit_urb(struct uvc_urb *uvc_urb, gfp_t mem_flags)
> > -{
> > - /* Sync DMA. */
> > - dma_sync_sgtable_for_device(uvc_stream_to_dmadev(uvc_urb->stream),
> > - uvc_urb->sgt,
> > - uvc_stream_dir(uvc_urb->stream));
> > - return usb_submit_urb(uvc_urb->urb, mem_flags);
> > -}
> > -
> > /*
> > * uvc_video_decode_data_work: Asynchronous memcpy processing
> > *
> > @@ -1310,7 +1301,7 @@ static void uvc_video_copy_data_work(struct work_struct *work)
> > uvc_queue_buffer_release(op->buf);
> > }
> >
> > - ret = uvc_submit_urb(uvc_urb, GFP_KERNEL);
> > + ret = usb_submit_urb(uvc_urb->urb, GFP_KERNEL);
> > if (ret < 0)
> > dev_err(&uvc_urb->stream->intf->dev,
> > "Failed to resubmit video URB (%d).\n", ret);
> > @@ -1736,12 +1727,6 @@ static void uvc_video_complete(struct urb *urb)
> > /* Re-initialise the URB async work. */
> > uvc_urb->async_operations = 0;
> >
> > - /* Sync DMA and invalidate vmap range. */
> > - dma_sync_sgtable_for_cpu(uvc_stream_to_dmadev(uvc_urb->stream),
> > - uvc_urb->sgt, uvc_stream_dir(stream));
> > - invalidate_kernel_vmap_range(uvc_urb->buffer,
> > - uvc_urb->stream->urb_size);
> > -
> > /*
> > * Process the URB headers, and optionally queue expensive memcpy tasks
> > * to be deferred to a work queue.
> > @@ -1750,7 +1735,7 @@ static void uvc_video_complete(struct urb *urb)
> >
> > /* If no async work is needed, resubmit the URB immediately. */
> > if (!uvc_urb->async_operations) {
> > - ret = uvc_submit_urb(uvc_urb, GFP_ATOMIC);
> > + ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
> > if (ret < 0)
> > dev_err(&stream->intf->dev,
> > "Failed to resubmit video URB (%d).\n", ret);
> > @@ -1765,17 +1750,15 @@ static void uvc_video_complete(struct urb *urb)
> > */
> > static void uvc_free_urb_buffers(struct uvc_streaming *stream)
> > {
> > - struct device *dma_dev = uvc_stream_to_dmadev(stream);
> > + struct usb_device *udev = stream->dev->udev;
> > struct uvc_urb *uvc_urb;
> >
> > for_each_uvc_urb(uvc_urb, stream) {
> > if (!uvc_urb->buffer)
> > continue;
> >
> > - dma_vunmap_noncontiguous(dma_dev, uvc_urb->buffer);
> > - dma_free_noncontiguous(dma_dev, stream->urb_size, uvc_urb->sgt,
> > - uvc_stream_dir(stream));
> > -
> > + usb_free_noncoherent(udev, stream->urb_size, uvc_urb->buffer,
> > + uvc_stream_dir(stream), uvc_urb->sgt);
> > uvc_urb->buffer = NULL;
> > uvc_urb->sgt = NULL;
> > }
> > @@ -1786,26 +1769,13 @@ static void uvc_free_urb_buffers(struct uvc_streaming *stream)
> > static bool uvc_alloc_urb_buffer(struct uvc_streaming *stream,
> > struct uvc_urb *uvc_urb, gfp_t gfp_flags)
> > {
> > - struct device *dma_dev = uvc_stream_to_dmadev(stream);
> > -
> > - uvc_urb->sgt = dma_alloc_noncontiguous(dma_dev, stream->urb_size,
> > - uvc_stream_dir(stream),
> > - gfp_flags, 0);
> > - if (!uvc_urb->sgt)
> > - return false;
> > - uvc_urb->dma = uvc_urb->sgt->sgl->dma_address;
> > -
> > - uvc_urb->buffer = dma_vmap_noncontiguous(dma_dev, stream->urb_size,
> > - uvc_urb->sgt);
> > - if (!uvc_urb->buffer) {
> > - dma_free_noncontiguous(dma_dev, stream->urb_size,
> > - uvc_urb->sgt,
> > - uvc_stream_dir(stream));
> > - uvc_urb->sgt = NULL;
> > - return false;
> > - }
> > + struct usb_device *udev = stream->dev->udev;
> >
> > - return true;
> > + uvc_urb->buffer = usb_alloc_noncoherent(udev, stream->urb_size,
> > + gfp_flags, &uvc_urb->dma,
> > + uvc_stream_dir(stream),
> > + &uvc_urb->sgt);
> > + return !!uvc_urb->buffer;
> > }
> >
> > /*
> > @@ -1953,6 +1923,7 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
> > urb->complete = uvc_video_complete;
> > urb->number_of_packets = npackets;
> > urb->transfer_buffer_length = size;
> > + urb->sgt = uvc_urb->sgt;
> >
> > for (i = 0; i < npackets; ++i) {
> > urb->iso_frame_desc[i].offset = i * psize;
> > @@ -2009,6 +1980,7 @@ static int uvc_init_video_bulk(struct uvc_streaming *stream,
> > size, uvc_video_complete, uvc_urb);
> > urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
> > urb->transfer_dma = uvc_urb->dma;
> > + urb->sgt = uvc_urb->sgt;
> >
> > uvc_urb->urb = urb;
> > }
> > @@ -2120,7 +2092,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
> >
> > /* Submit the URBs. */
> > for_each_uvc_urb(uvc_urb, stream) {
> > - ret = uvc_submit_urb(uvc_urb, gfp_flags);
> > + ret = usb_submit_urb(uvc_urb->urb, gfp_flags);
> > if (ret < 0) {
> > dev_err(&stream->intf->dev,
> > "Failed to submit URB %u (%d).\n",
> > --
> > 2.34.1
> >
> >
>
>
> --
> Ricardo Ribalda