Re: [PATCH 3/3] media: stk1160: use dma_alloc_noncontiguous API

[Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>]

Hi Dafna,

Very nice work.

I specially like all the testing that you mentioned in the cover-letter.

Back in the day, there were a few users trying to use STK1160 (Easycap)
with Beaglebone boards, without success due to lack of USB throughput.

If anything else, I think it's good to see additional noncontiguous API users.

On Tue, Jan 11, 2022 at 08:55:05AM +0200, Dafna Hirschfeld wrote:
> Replace the urb buffers allocation to
> use the noncontiguous API. This improve performance
> on Arm.
> The noncontiguous API require handling
> synchronization.
> This commit is similar to the one sent to
> uvc: [1]
> 
> [1] https://lkml.org/lkml/2021/3/12/1506
> 

This commit description needs lots of attention. In particular,
please add the test results here.

> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/media/usb/stk1160/stk1160-v4l.c   |   3 +
>  drivers/media/usb/stk1160/stk1160-video.c | 109 +++++++++++++---------
>  drivers/media/usb/stk1160/stk1160.h       |  10 ++
>  3 files changed, 79 insertions(+), 43 deletions(-)
> 
[..]
> @@ -501,7 +524,7 @@ int stk1160_alloc_isoc(struct stk1160 *dev)
>  
>  free_i_bufs:
>  	/* Save the allocated buffers so far, so we can properly free them */
> -	dev->isoc_ctl.num_bufs = i+1;
> +	dev->isoc_ctl.num_bufs = i;

This looks like a separate fix, similar to 1/3 ?

>  	stk1160_free_isoc(dev);
>  	return -ENOMEM;
>  }
> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h
> index 1ffca1343d88..52bea7815ae5 100644
> --- a/drivers/media/usb/stk1160/stk1160.h
> +++ b/drivers/media/usb/stk1160/stk1160.h
> @@ -16,6 +16,8 @@
>  #include <media/videobuf2-v4l2.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ctrls.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
>  
>  #define STK1160_VERSION		"0.9.5"
>  #define STK1160_VERSION_NUM	0x000905
> @@ -87,6 +89,9 @@ struct stk1160_buffer {
>  struct stk1160_urb {
>  	struct urb *urb;
>  	char *transfer_buffer;
> +	struct sg_table *sgt;
> +	struct stk1160 *dev;
> +	dma_addr_t dma;
>  };
>  
>  struct stk1160_isoc_ctl {
> @@ -190,3 +195,8 @@ void stk1160_select_input(struct stk1160 *dev);
>  
>  /* Provided by stk1160-ac97.c */
>  void stk1160_ac97_setup(struct stk1160 *dev);
> +
> +static inline struct device *stk1160_get_dmadev(struct stk1160 *dev)
> +{
> +	return bus_to_hcd(dev->udev->bus)->self.sysdev;

This function looks truly horrible, is there no other way to get the device ?

I suppose this function return something different than (struct stk1160*)dev->dev ?

Thanks,
Ezequiel