diff mbox series

[v2,1/3] usb: gadget: uvc: make uvc_num_requests depend on gadget speed

Message ID 20210530223041.15320-2-m.grzeschik@pengutronix.de
State New
Headers show
Series [v2,1/3] usb: gadget: uvc: make uvc_num_requests depend on gadget speed | expand

Commit Message

Michael Grzeschik May 30, 2021, 10:30 p.m. UTC
While sending bigger images is possible with USB_SPEED_SUPER it is
better to use more isochronous requests in flight. This patch makes the
number uvc_num_requests dynamic by changing it depending on the gadget
speed.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v1 -> v2: - fixed null pointer dereference in uvc_video_free_requests

 drivers/usb/gadget/function/uvc.h       | 11 +++--
 drivers/usb/gadget/function/uvc_queue.c |  7 ++++
 drivers/usb/gadget/function/uvc_video.c | 56 +++++++++++++++----------
 3 files changed, 48 insertions(+), 26 deletions(-)

Comments

Paul Elder June 14, 2021, 10:34 a.m. UTC | #1
Hi Michael,

On Mon, May 31, 2021 at 12:30:39AM +0200, Michael Grzeschik wrote:
> While sending bigger images is possible with USB_SPEED_SUPER it is

> better to use more isochronous requests in flight. This patch makes the

> number uvc_num_requests dynamic by changing it depending on the gadget

> speed.

> 

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>


Looks good to me.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>


> ---

> v1 -> v2: - fixed null pointer dereference in uvc_video_free_requests

> 

>  drivers/usb/gadget/function/uvc.h       | 11 +++--

>  drivers/usb/gadget/function/uvc_queue.c |  7 ++++

>  drivers/usb/gadget/function/uvc_video.c | 56 +++++++++++++++----------

>  3 files changed, 48 insertions(+), 26 deletions(-)

> 

> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h

> index 23ee25383c1f7..83b9e945944e8 100644

> --- a/drivers/usb/gadget/function/uvc.h

> +++ b/drivers/usb/gadget/function/uvc.h

> @@ -65,13 +65,17 @@ extern unsigned int uvc_gadget_trace_param;

>   * Driver specific constants

>   */

>  

> -#define UVC_NUM_REQUESTS			4

>  #define UVC_MAX_REQUEST_SIZE			64

>  #define UVC_MAX_EVENTS				4

>  

>  /* ------------------------------------------------------------------------

>   * Structures

>   */

> +struct uvc_request {

> +	struct usb_request *req;

> +	__u8 *req_buffer;

> +	struct uvc_video *video;

> +};

>  

>  struct uvc_video {

>  	struct uvc_device *uvc;

> @@ -87,10 +91,11 @@ struct uvc_video {

>  	unsigned int imagesize;

>  	struct mutex mutex;	/* protects frame parameters */

>  

> +	int uvc_num_requests;

> +

>  	/* Requests */

>  	unsigned int req_size;

> -	struct usb_request *req[UVC_NUM_REQUESTS];

> -	__u8 *req_buffer[UVC_NUM_REQUESTS];

> +	struct uvc_request *ureq;

>  	struct list_head req_free;

>  	spinlock_t req_lock;

>  

> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c

> index 61e2c94cc0b0c..dcd71304d521c 100644

> --- a/drivers/usb/gadget/function/uvc_queue.c

> +++ b/drivers/usb/gadget/function/uvc_queue.c

> @@ -43,6 +43,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,

>  {

>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);

>  	struct uvc_video *video = container_of(queue, struct uvc_video, queue);

> +	struct uvc_device *uvc = video->uvc;

> +	struct usb_composite_dev *cdev = uvc->func.config->cdev;

>  

>  	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)

>  		*nbuffers = UVC_MAX_VIDEO_BUFFERS;

> @@ -51,6 +53,11 @@ static int uvc_queue_setup(struct vb2_queue *vq,

>  

>  	sizes[0] = video->imagesize;

>  

> +	if (cdev->gadget->speed < USB_SPEED_SUPER)

> +		video->uvc_num_requests = 4;

> +	else

> +		video->uvc_num_requests = 64;

> +

>  	return 0;

>  }

>  

> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c

> index 633e23d58d868..57840083dfdda 100644

> --- a/drivers/usb/gadget/function/uvc_video.c

> +++ b/drivers/usb/gadget/function/uvc_video.c

> @@ -11,6 +11,7 @@

>  #include <linux/errno.h>

>  #include <linux/usb/ch9.h>

>  #include <linux/usb/gadget.h>

> +#include <linux/module.h>

>  #include <linux/usb/video.h>

>  

>  #include <media/v4l2-dev.h>

> @@ -145,7 +146,8 @@ static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)

>  static void

>  uvc_video_complete(struct usb_ep *ep, struct usb_request *req)

>  {

> -	struct uvc_video *video = req->context;

> +	struct uvc_request *ureq = req->context;

> +	struct uvc_video *video = ureq->video;

>  	struct uvc_video_queue *queue = &video->queue;

>  	unsigned long flags;

>  

> @@ -177,16 +179,19 @@ uvc_video_free_requests(struct uvc_video *video)

>  {

>  	unsigned int i;

>  

> -	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {

> -		if (video->req[i]) {

> -			usb_ep_free_request(video->ep, video->req[i]);

> -			video->req[i] = NULL;

> -		}

> -

> -		if (video->req_buffer[i]) {

> -			kfree(video->req_buffer[i]);

> -			video->req_buffer[i] = NULL;

> +	if (video->ureq) {

> +		for (i = 0; i < video->uvc_num_requests; ++i) {

> +			if (video->ureq[i].req) {

> +				usb_ep_free_request(video->ep, video->ureq[i].req);

> +				video->ureq[i].req = NULL;

> +			}

> +			if (video->ureq[i].req_buffer) {

> +				kfree(video->ureq[i].req_buffer);

> +				video->ureq[i].req_buffer = NULL;

> +			}

>  		}

> +		kfree(video->ureq);

> +		video->ureq = NULL;

>  	}

>  

>  	INIT_LIST_HEAD(&video->req_free);

> @@ -207,21 +212,26 @@ uvc_video_alloc_requests(struct uvc_video *video)

>  		 * max_t(unsigned int, video->ep->maxburst, 1)

>  		 * (video->ep->mult);

>  

> -	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {

> -		video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);

> -		if (video->req_buffer[i] == NULL)

> +	video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);

> +	if (video->ureq == NULL)

> +		return ret;

> +

> +	for (i = 0; i < video->uvc_num_requests; ++i) {

> +		video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL);

> +		if (video->ureq[i].req_buffer == NULL)

>  			goto error;

>  

> -		video->req[i] = usb_ep_alloc_request(video->ep, GFP_KERNEL);

> -		if (video->req[i] == NULL)

> +		video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL);

> +		if (video->ureq[i].req == NULL)

>  			goto error;

>  

> -		video->req[i]->buf = video->req_buffer[i];

> -		video->req[i]->length = 0;

> -		video->req[i]->complete = uvc_video_complete;

> -		video->req[i]->context = video;

> +		video->ureq[i].req->buf = video->ureq[i].req_buffer;

> +		video->ureq[i].req->length = 0;

> +		video->ureq[i].req->complete = uvc_video_complete;

> +		video->ureq[i].req->context = &video->ureq[i];

> +		video->ureq[i].video = video;

>  

> -		list_add_tail(&video->req[i]->list, &video->req_free);

> +		list_add_tail(&video->ureq[i].req->list, &video->req_free);

>  	}

>  

>  	video->req_size = req_size;

> @@ -312,9 +322,9 @@ int uvcg_video_enable(struct uvc_video *video, int enable)

>  		cancel_work_sync(&video->pump);

>  		uvcg_queue_cancel(&video->queue, 0);

>  

> -		for (i = 0; i < UVC_NUM_REQUESTS; ++i)

> -			if (video->req[i])

> -				usb_ep_dequeue(video->ep, video->req[i]);

> +		for (i = 0; i < video->uvc_num_requests; ++i)

> +			if (video->ureq && video->ureq[i].req)

> +				usb_ep_dequeue(video->ep, video->ureq[i].req);

>  

>  		uvc_video_free_requests(video);

>  		uvcg_queue_enable(&video->queue, 0);

> -- 

> 2.29.2

>
Laurent Pinchart June 15, 2021, 1:28 a.m. UTC | #2
Hi Michael,

Thank you for the patch.

On Mon, May 31, 2021 at 12:30:39AM +0200, Michael Grzeschik wrote:
> While sending bigger images is possible with USB_SPEED_SUPER it is

> better to use more isochronous requests in flight. This patch makes the

> number uvc_num_requests dynamic by changing it depending on the gadget

> speed.

> 

> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

> ---

> v1 -> v2: - fixed null pointer dereference in uvc_video_free_requests

> 

>  drivers/usb/gadget/function/uvc.h       | 11 +++--

>  drivers/usb/gadget/function/uvc_queue.c |  7 ++++

>  drivers/usb/gadget/function/uvc_video.c | 56 +++++++++++++++----------

>  3 files changed, 48 insertions(+), 26 deletions(-)

> 

> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h

> index 23ee25383c1f7..83b9e945944e8 100644

> --- a/drivers/usb/gadget/function/uvc.h

> +++ b/drivers/usb/gadget/function/uvc.h

> @@ -65,13 +65,17 @@ extern unsigned int uvc_gadget_trace_param;

>   * Driver specific constants

>   */

>  

> -#define UVC_NUM_REQUESTS			4

>  #define UVC_MAX_REQUEST_SIZE			64

>  #define UVC_MAX_EVENTS				4

>  

>  /* ------------------------------------------------------------------------

>   * Structures

>   */

> +struct uvc_request {

> +	struct usb_request *req;

> +	__u8 *req_buffer;


While at it, you could s/__u8/u8/ as this header isn't used by
userspace.

> +	struct uvc_video *video;

> +};

>  

>  struct uvc_video {

>  	struct uvc_device *uvc;

> @@ -87,10 +91,11 @@ struct uvc_video {

>  	unsigned int imagesize;

>  	struct mutex mutex;	/* protects frame parameters */

>  

> +	int uvc_num_requests;


Never negative, you can make it an unsigned int.

> +

>  	/* Requests */

>  	unsigned int req_size;

> -	struct usb_request *req[UVC_NUM_REQUESTS];

> -	__u8 *req_buffer[UVC_NUM_REQUESTS];

> +	struct uvc_request *ureq;

>  	struct list_head req_free;

>  	spinlock_t req_lock;

>  

> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c

> index 61e2c94cc0b0c..dcd71304d521c 100644

> --- a/drivers/usb/gadget/function/uvc_queue.c

> +++ b/drivers/usb/gadget/function/uvc_queue.c

> @@ -43,6 +43,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,

>  {

>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);

>  	struct uvc_video *video = container_of(queue, struct uvc_video, queue);

> +	struct uvc_device *uvc = video->uvc;

> +	struct usb_composite_dev *cdev = uvc->func.config->cdev;


	struct usb_composite_dev *cdev = video->uvc->func.config->cdev;

>  

>  	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)

>  		*nbuffers = UVC_MAX_VIDEO_BUFFERS;

> @@ -51,6 +53,11 @@ static int uvc_queue_setup(struct vb2_queue *vq,

>  

>  	sizes[0] = video->imagesize;

>  

> +	if (cdev->gadget->speed < USB_SPEED_SUPER)

> +		video->uvc_num_requests = 4;

> +	else

> +		video->uvc_num_requests = 64;

> +

>  	return 0;

>  }

>  

> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c

> index 633e23d58d868..57840083dfdda 100644

> --- a/drivers/usb/gadget/function/uvc_video.c

> +++ b/drivers/usb/gadget/function/uvc_video.c

> @@ -11,6 +11,7 @@

>  #include <linux/errno.h>

>  #include <linux/usb/ch9.h>

>  #include <linux/usb/gadget.h>

> +#include <linux/module.h>


Alphabetical order please.

>  #include <linux/usb/video.h>

>  

>  #include <media/v4l2-dev.h>

> @@ -145,7 +146,8 @@ static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)

>  static void

>  uvc_video_complete(struct usb_ep *ep, struct usb_request *req)

>  {

> -	struct uvc_video *video = req->context;

> +	struct uvc_request *ureq = req->context;

> +	struct uvc_video *video = ureq->video;

>  	struct uvc_video_queue *queue = &video->queue;

>  	unsigned long flags;

>  

> @@ -177,16 +179,19 @@ uvc_video_free_requests(struct uvc_video *video)

>  {

>  	unsigned int i;

>  

> -	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {

> -		if (video->req[i]) {

> -			usb_ep_free_request(video->ep, video->req[i]);

> -			video->req[i] = NULL;

> -		}

> -

> -		if (video->req_buffer[i]) {

> -			kfree(video->req_buffer[i]);

> -			video->req_buffer[i] = NULL;

> +	if (video->ureq) {

> +		for (i = 0; i < video->uvc_num_requests; ++i) {

> +			if (video->ureq[i].req) {

> +				usb_ep_free_request(video->ep, video->ureq[i].req);

> +				video->ureq[i].req = NULL;

> +			}


Blank line here please.

> +			if (video->ureq[i].req_buffer) {

> +				kfree(video->ureq[i].req_buffer);

> +				video->ureq[i].req_buffer = NULL;

> +			}

>  		}


Here too.

> +		kfree(video->ureq);

> +		video->ureq = NULL;

>  	}

>  

>  	INIT_LIST_HEAD(&video->req_free);

> @@ -207,21 +212,26 @@ uvc_video_alloc_requests(struct uvc_video *video)

>  		 * max_t(unsigned int, video->ep->maxburst, 1)

>  		 * (video->ep->mult);

>  

> -	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {

> -		video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);

> -		if (video->req_buffer[i] == NULL)

> +	video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);

> +	if (video->ureq == NULL)

> +		return ret;


		return -ENOMEM;

would be more readable (I had to check the value of ret to review this
patch).

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


> +

> +	for (i = 0; i < video->uvc_num_requests; ++i) {

> +		video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL);

> +		if (video->ureq[i].req_buffer == NULL)

>  			goto error;

>  

> -		video->req[i] = usb_ep_alloc_request(video->ep, GFP_KERNEL);

> -		if (video->req[i] == NULL)

> +		video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL);

> +		if (video->ureq[i].req == NULL)

>  			goto error;

>  

> -		video->req[i]->buf = video->req_buffer[i];

> -		video->req[i]->length = 0;

> -		video->req[i]->complete = uvc_video_complete;

> -		video->req[i]->context = video;

> +		video->ureq[i].req->buf = video->ureq[i].req_buffer;

> +		video->ureq[i].req->length = 0;

> +		video->ureq[i].req->complete = uvc_video_complete;

> +		video->ureq[i].req->context = &video->ureq[i];

> +		video->ureq[i].video = video;

>  

> -		list_add_tail(&video->req[i]->list, &video->req_free);

> +		list_add_tail(&video->ureq[i].req->list, &video->req_free);

>  	}

>  

>  	video->req_size = req_size;

> @@ -312,9 +322,9 @@ int uvcg_video_enable(struct uvc_video *video, int enable)

>  		cancel_work_sync(&video->pump);

>  		uvcg_queue_cancel(&video->queue, 0);

>  

> -		for (i = 0; i < UVC_NUM_REQUESTS; ++i)

> -			if (video->req[i])

> -				usb_ep_dequeue(video->ep, video->req[i]);

> +		for (i = 0; i < video->uvc_num_requests; ++i)

> +			if (video->ureq && video->ureq[i].req)

> +				usb_ep_dequeue(video->ep, video->ureq[i].req);

>  

>  		uvc_video_free_requests(video);

>  		uvcg_queue_enable(&video->queue, 0);


-- 
Regards,

Laurent Pinchart
Laurent Pinchart June 15, 2021, 1:38 a.m. UTC | #3
Hi Michael,

Another comment.

On Tue, Jun 15, 2021 at 04:28:05AM +0300, Laurent Pinchart wrote:
> On Mon, May 31, 2021 at 12:30:39AM +0200, Michael Grzeschik wrote:

> > While sending bigger images is possible with USB_SPEED_SUPER it is

> > better to use more isochronous requests in flight. This patch makes the

> > number uvc_num_requests dynamic by changing it depending on the gadget

> > speed.

> > 

> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

> > ---

> > v1 -> v2: - fixed null pointer dereference in uvc_video_free_requests

> > 

> >  drivers/usb/gadget/function/uvc.h       | 11 +++--

> >  drivers/usb/gadget/function/uvc_queue.c |  7 ++++

> >  drivers/usb/gadget/function/uvc_video.c | 56 +++++++++++++++----------

> >  3 files changed, 48 insertions(+), 26 deletions(-)

> > 

> > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h

> > index 23ee25383c1f7..83b9e945944e8 100644

> > --- a/drivers/usb/gadget/function/uvc.h

> > +++ b/drivers/usb/gadget/function/uvc.h

> > @@ -65,13 +65,17 @@ extern unsigned int uvc_gadget_trace_param;

> >   * Driver specific constants

> >   */

> >  

> > -#define UVC_NUM_REQUESTS			4

> >  #define UVC_MAX_REQUEST_SIZE			64

> >  #define UVC_MAX_EVENTS				4

> >  

> >  /* ------------------------------------------------------------------------

> >   * Structures

> >   */

> > +struct uvc_request {

> > +	struct usb_request *req;

> > +	__u8 *req_buffer;

> 

> While at it, you could s/__u8/u8/ as this header isn't used by

> userspace.

> 

> > +	struct uvc_video *video;

> > +};

> >  

> >  struct uvc_video {

> >  	struct uvc_device *uvc;

> > @@ -87,10 +91,11 @@ struct uvc_video {

> >  	unsigned int imagesize;

> >  	struct mutex mutex;	/* protects frame parameters */

> >  

> > +	int uvc_num_requests;

> 

> Never negative, you can make it an unsigned int.

> 

> > +

> >  	/* Requests */

> >  	unsigned int req_size;

> > -	struct usb_request *req[UVC_NUM_REQUESTS];

> > -	__u8 *req_buffer[UVC_NUM_REQUESTS];

> > +	struct uvc_request *ureq;

> >  	struct list_head req_free;

> >  	spinlock_t req_lock;

> >  

> > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c

> > index 61e2c94cc0b0c..dcd71304d521c 100644

> > --- a/drivers/usb/gadget/function/uvc_queue.c

> > +++ b/drivers/usb/gadget/function/uvc_queue.c

> > @@ -43,6 +43,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,

> >  {

> >  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);

> >  	struct uvc_video *video = container_of(queue, struct uvc_video, queue);

> > +	struct uvc_device *uvc = video->uvc;

> > +	struct usb_composite_dev *cdev = uvc->func.config->cdev;

> 

> 	struct usb_composite_dev *cdev = video->uvc->func.config->cdev;

> 

> >  

> >  	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)

> >  		*nbuffers = UVC_MAX_VIDEO_BUFFERS;

> > @@ -51,6 +53,11 @@ static int uvc_queue_setup(struct vb2_queue *vq,

> >  

> >  	sizes[0] = video->imagesize;

> >  

> > +	if (cdev->gadget->speed < USB_SPEED_SUPER)

> > +		video->uvc_num_requests = 4;

> > +	else

> > +		video->uvc_num_requests = 64;

> > +

> >  	return 0;

> >  }

> >  

> > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c

> > index 633e23d58d868..57840083dfdda 100644

> > --- a/drivers/usb/gadget/function/uvc_video.c

> > +++ b/drivers/usb/gadget/function/uvc_video.c

> > @@ -11,6 +11,7 @@

> >  #include <linux/errno.h>

> >  #include <linux/usb/ch9.h>

> >  #include <linux/usb/gadget.h>

> > +#include <linux/module.h>

> 

> Alphabetical order please.


Why is this header needed ? I can't see anything below related to it.

> >  #include <linux/usb/video.h>

> >  

> >  #include <media/v4l2-dev.h>

> > @@ -145,7 +146,8 @@ static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)

> >  static void

> >  uvc_video_complete(struct usb_ep *ep, struct usb_request *req)

> >  {

> > -	struct uvc_video *video = req->context;

> > +	struct uvc_request *ureq = req->context;

> > +	struct uvc_video *video = ureq->video;

> >  	struct uvc_video_queue *queue = &video->queue;

> >  	unsigned long flags;

> >  

> > @@ -177,16 +179,19 @@ uvc_video_free_requests(struct uvc_video *video)

> >  {

> >  	unsigned int i;

> >  

> > -	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {

> > -		if (video->req[i]) {

> > -			usb_ep_free_request(video->ep, video->req[i]);

> > -			video->req[i] = NULL;

> > -		}

> > -

> > -		if (video->req_buffer[i]) {

> > -			kfree(video->req_buffer[i]);

> > -			video->req_buffer[i] = NULL;

> > +	if (video->ureq) {

> > +		for (i = 0; i < video->uvc_num_requests; ++i) {

> > +			if (video->ureq[i].req) {

> > +				usb_ep_free_request(video->ep, video->ureq[i].req);

> > +				video->ureq[i].req = NULL;

> > +			}

> 

> Blank line here please.

> 

> > +			if (video->ureq[i].req_buffer) {

> > +				kfree(video->ureq[i].req_buffer);

> > +				video->ureq[i].req_buffer = NULL;

> > +			}

> >  		}

> 

> Here too.

> 

> > +		kfree(video->ureq);

> > +		video->ureq = NULL;

> >  	}

> >  

> >  	INIT_LIST_HEAD(&video->req_free);

> > @@ -207,21 +212,26 @@ uvc_video_alloc_requests(struct uvc_video *video)

> >  		 * max_t(unsigned int, video->ep->maxburst, 1)

> >  		 * (video->ep->mult);

> >  

> > -	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {

> > -		video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);

> > -		if (video->req_buffer[i] == NULL)

> > +	video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);

> > +	if (video->ureq == NULL)

> > +		return ret;

> 

> 		return -ENOMEM;

> 

> would be more readable (I had to check the value of ret to review this

> patch).

> 

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 

> > +

> > +	for (i = 0; i < video->uvc_num_requests; ++i) {

> > +		video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL);

> > +		if (video->ureq[i].req_buffer == NULL)

> >  			goto error;

> >  

> > -		video->req[i] = usb_ep_alloc_request(video->ep, GFP_KERNEL);

> > -		if (video->req[i] == NULL)

> > +		video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL);

> > +		if (video->ureq[i].req == NULL)

> >  			goto error;

> >  

> > -		video->req[i]->buf = video->req_buffer[i];

> > -		video->req[i]->length = 0;

> > -		video->req[i]->complete = uvc_video_complete;

> > -		video->req[i]->context = video;

> > +		video->ureq[i].req->buf = video->ureq[i].req_buffer;

> > +		video->ureq[i].req->length = 0;

> > +		video->ureq[i].req->complete = uvc_video_complete;

> > +		video->ureq[i].req->context = &video->ureq[i];

> > +		video->ureq[i].video = video;

> >  

> > -		list_add_tail(&video->req[i]->list, &video->req_free);

> > +		list_add_tail(&video->ureq[i].req->list, &video->req_free);

> >  	}

> >  

> >  	video->req_size = req_size;

> > @@ -312,9 +322,9 @@ int uvcg_video_enable(struct uvc_video *video, int enable)

> >  		cancel_work_sync(&video->pump);

> >  		uvcg_queue_cancel(&video->queue, 0);

> >  

> > -		for (i = 0; i < UVC_NUM_REQUESTS; ++i)

> > -			if (video->req[i])

> > -				usb_ep_dequeue(video->ep, video->req[i]);

> > +		for (i = 0; i < video->uvc_num_requests; ++i)

> > +			if (video->ureq && video->ureq[i].req)

> > +				usb_ep_dequeue(video->ep, video->ureq[i].req);

> >  

> >  		uvc_video_free_requests(video);

> >  		uvcg_queue_enable(&video->queue, 0);


-- 
Regards,

Laurent Pinchart
Michael Grzeschik June 22, 2021, 3 p.m. UTC | #4
Hi Laurent!

On Tue, Jun 15, 2021 at 04:38:29AM +0300, Laurent Pinchart wrote:
>Hi Michael,

>

>Another comment.

>

>On Tue, Jun 15, 2021 at 04:28:05AM +0300, Laurent Pinchart wrote:

>> On Mon, May 31, 2021 at 12:30:39AM +0200, Michael Grzeschik wrote:

>> > While sending bigger images is possible with USB_SPEED_SUPER it is

>> > better to use more isochronous requests in flight. This patch makes the

>> > number uvc_num_requests dynamic by changing it depending on the gadget

>> > speed.

>> >

>> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

>> > ---

>> > v1 -> v2: - fixed null pointer dereference in uvc_video_free_requests

>> >

>> >  drivers/usb/gadget/function/uvc.h       | 11 +++--

>> >  drivers/usb/gadget/function/uvc_queue.c |  7 ++++

>> >  drivers/usb/gadget/function/uvc_video.c | 56 +++++++++++++++----------

>> >  3 files changed, 48 insertions(+), 26 deletions(-)

>> >

>> > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h

>> > index 23ee25383c1f7..83b9e945944e8 100644

>> > --- a/drivers/usb/gadget/function/uvc.h

>> > +++ b/drivers/usb/gadget/function/uvc.h

>> > @@ -65,13 +65,17 @@ extern unsigned int uvc_gadget_trace_param;

>> >   * Driver specific constants

>> >   */

>> >

>> > -#define UVC_NUM_REQUESTS			4

>> >  #define UVC_MAX_REQUEST_SIZE			64

>> >  #define UVC_MAX_EVENTS				4

>> >

>> >  /* ------------------------------------------------------------------------

>> >   * Structures

>> >   */

>> > +struct uvc_request {

>> > +	struct usb_request *req;

>> > +	__u8 *req_buffer;

>>

>> While at it, you could s/__u8/u8/ as this header isn't used by

>> userspace.

>>

>> > +	struct uvc_video *video;

>> > +};

>> >

>> >  struct uvc_video {

>> >  	struct uvc_device *uvc;

>> > @@ -87,10 +91,11 @@ struct uvc_video {

>> >  	unsigned int imagesize;

>> >  	struct mutex mutex;	/* protects frame parameters */

>> >

>> > +	int uvc_num_requests;

>>

>> Never negative, you can make it an unsigned int.

>>

>> > +

>> >  	/* Requests */

>> >  	unsigned int req_size;

>> > -	struct usb_request *req[UVC_NUM_REQUESTS];

>> > -	__u8 *req_buffer[UVC_NUM_REQUESTS];

>> > +	struct uvc_request *ureq;

>> >  	struct list_head req_free;

>> >  	spinlock_t req_lock;

>> >

>> > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c

>> > index 61e2c94cc0b0c..dcd71304d521c 100644

>> > --- a/drivers/usb/gadget/function/uvc_queue.c

>> > +++ b/drivers/usb/gadget/function/uvc_queue.c

>> > @@ -43,6 +43,8 @@ static int uvc_queue_setup(struct vb2_queue *vq,

>> >  {

>> >  	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);

>> >  	struct uvc_video *video = container_of(queue, struct uvc_video, queue);

>> > +	struct uvc_device *uvc = video->uvc;

>> > +	struct usb_composite_dev *cdev = uvc->func.config->cdev;

>>

>> 	struct usb_composite_dev *cdev = video->uvc->func.config->cdev;

>>

>> >

>> >  	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)

>> >  		*nbuffers = UVC_MAX_VIDEO_BUFFERS;

>> > @@ -51,6 +53,11 @@ static int uvc_queue_setup(struct vb2_queue *vq,

>> >

>> >  	sizes[0] = video->imagesize;

>> >

>> > +	if (cdev->gadget->speed < USB_SPEED_SUPER)

>> > +		video->uvc_num_requests = 4;

>> > +	else

>> > +		video->uvc_num_requests = 64;

>> > +

>> >  	return 0;

>> >  }

>> >

>> > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c

>> > index 633e23d58d868..57840083dfdda 100644

>> > --- a/drivers/usb/gadget/function/uvc_video.c

>> > +++ b/drivers/usb/gadget/function/uvc_video.c

>> > @@ -11,6 +11,7 @@

>> >  #include <linux/errno.h>

>> >  #include <linux/usb/ch9.h>

>> >  #include <linux/usb/gadget.h>

>> > +#include <linux/module.h>

>>

>> Alphabetical order please.

>

>Why is this header needed ? I can't see anything below related to it.


It must have slipped through. I removed the header and worked in all
your feedback.

Thanks for the Review!

Regards,

Michael Grzeschik

>> >  #include <linux/usb/video.h>

>> >

>> >  #include <media/v4l2-dev.h>

>> > @@ -145,7 +146,8 @@ static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)

>> >  static void

>> >  uvc_video_complete(struct usb_ep *ep, struct usb_request *req)

>> >  {

>> > -	struct uvc_video *video = req->context;

>> > +	struct uvc_request *ureq = req->context;

>> > +	struct uvc_video *video = ureq->video;

>> >  	struct uvc_video_queue *queue = &video->queue;

>> >  	unsigned long flags;

>> >

>> > @@ -177,16 +179,19 @@ uvc_video_free_requests(struct uvc_video *video)

>> >  {

>> >  	unsigned int i;

>> >

>> > -	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {

>> > -		if (video->req[i]) {

>> > -			usb_ep_free_request(video->ep, video->req[i]);

>> > -			video->req[i] = NULL;

>> > -		}

>> > -

>> > -		if (video->req_buffer[i]) {

>> > -			kfree(video->req_buffer[i]);

>> > -			video->req_buffer[i] = NULL;

>> > +	if (video->ureq) {

>> > +		for (i = 0; i < video->uvc_num_requests; ++i) {

>> > +			if (video->ureq[i].req) {

>> > +				usb_ep_free_request(video->ep, video->ureq[i].req);

>> > +				video->ureq[i].req = NULL;

>> > +			}

>>

>> Blank line here please.

>>

>> > +			if (video->ureq[i].req_buffer) {

>> > +				kfree(video->ureq[i].req_buffer);

>> > +				video->ureq[i].req_buffer = NULL;

>> > +			}

>> >  		}

>>

>> Here too.

>>

>> > +		kfree(video->ureq);

>> > +		video->ureq = NULL;

>> >  	}

>> >

>> >  	INIT_LIST_HEAD(&video->req_free);

>> > @@ -207,21 +212,26 @@ uvc_video_alloc_requests(struct uvc_video *video)

>> >  		 * max_t(unsigned int, video->ep->maxburst, 1)

>> >  		 * (video->ep->mult);

>> >

>> > -	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {

>> > -		video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);

>> > -		if (video->req_buffer[i] == NULL)

>> > +	video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);

>> > +	if (video->ureq == NULL)

>> > +		return ret;

>>

>> 		return -ENOMEM;

>>

>> would be more readable (I had to check the value of ret to review this

>> patch).

>>

>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>>

>> > +

>> > +	for (i = 0; i < video->uvc_num_requests; ++i) {

>> > +		video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL);

>> > +		if (video->ureq[i].req_buffer == NULL)

>> >  			goto error;

>> >

>> > -		video->req[i] = usb_ep_alloc_request(video->ep, GFP_KERNEL);

>> > -		if (video->req[i] == NULL)

>> > +		video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL);

>> > +		if (video->ureq[i].req == NULL)

>> >  			goto error;

>> >

>> > -		video->req[i]->buf = video->req_buffer[i];

>> > -		video->req[i]->length = 0;

>> > -		video->req[i]->complete = uvc_video_complete;

>> > -		video->req[i]->context = video;

>> > +		video->ureq[i].req->buf = video->ureq[i].req_buffer;

>> > +		video->ureq[i].req->length = 0;

>> > +		video->ureq[i].req->complete = uvc_video_complete;

>> > +		video->ureq[i].req->context = &video->ureq[i];

>> > +		video->ureq[i].video = video;

>> >

>> > -		list_add_tail(&video->req[i]->list, &video->req_free);

>> > +		list_add_tail(&video->ureq[i].req->list, &video->req_free);

>> >  	}

>> >

>> >  	video->req_size = req_size;

>> > @@ -312,9 +322,9 @@ int uvcg_video_enable(struct uvc_video *video, int enable)

>> >  		cancel_work_sync(&video->pump);

>> >  		uvcg_queue_cancel(&video->queue, 0);

>> >

>> > -		for (i = 0; i < UVC_NUM_REQUESTS; ++i)

>> > -			if (video->req[i])

>> > -				usb_ep_dequeue(video->ep, video->req[i]);

>> > +		for (i = 0; i < video->uvc_num_requests; ++i)

>> > +			if (video->ureq && video->ureq[i].req)

>> > +				usb_ep_dequeue(video->ep, video->ureq[i].req);

>> >

>> >  		uvc_video_free_requests(video);

>> >  		uvcg_queue_enable(&video->queue, 0);

>

>-- 

>Regards,

>

>Laurent Pinchart

>


-- 
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 |
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 23ee25383c1f7..83b9e945944e8 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -65,13 +65,17 @@  extern unsigned int uvc_gadget_trace_param;
  * Driver specific constants
  */
 
-#define UVC_NUM_REQUESTS			4
 #define UVC_MAX_REQUEST_SIZE			64
 #define UVC_MAX_EVENTS				4
 
 /* ------------------------------------------------------------------------
  * Structures
  */
+struct uvc_request {
+	struct usb_request *req;
+	__u8 *req_buffer;
+	struct uvc_video *video;
+};
 
 struct uvc_video {
 	struct uvc_device *uvc;
@@ -87,10 +91,11 @@  struct uvc_video {
 	unsigned int imagesize;
 	struct mutex mutex;	/* protects frame parameters */
 
+	int uvc_num_requests;
+
 	/* Requests */
 	unsigned int req_size;
-	struct usb_request *req[UVC_NUM_REQUESTS];
-	__u8 *req_buffer[UVC_NUM_REQUESTS];
+	struct uvc_request *ureq;
 	struct list_head req_free;
 	spinlock_t req_lock;
 
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index 61e2c94cc0b0c..dcd71304d521c 100644
--- a/drivers/usb/gadget/function/uvc_queue.c
+++ b/drivers/usb/gadget/function/uvc_queue.c
@@ -43,6 +43,8 @@  static int uvc_queue_setup(struct vb2_queue *vq,
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
 	struct uvc_video *video = container_of(queue, struct uvc_video, queue);
+	struct uvc_device *uvc = video->uvc;
+	struct usb_composite_dev *cdev = uvc->func.config->cdev;
 
 	if (*nbuffers > UVC_MAX_VIDEO_BUFFERS)
 		*nbuffers = UVC_MAX_VIDEO_BUFFERS;
@@ -51,6 +53,11 @@  static int uvc_queue_setup(struct vb2_queue *vq,
 
 	sizes[0] = video->imagesize;
 
+	if (cdev->gadget->speed < USB_SPEED_SUPER)
+		video->uvc_num_requests = 4;
+	else
+		video->uvc_num_requests = 64;
+
 	return 0;
 }
 
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 633e23d58d868..57840083dfdda 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -11,6 +11,7 @@ 
 #include <linux/errno.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
+#include <linux/module.h>
 #include <linux/usb/video.h>
 
 #include <media/v4l2-dev.h>
@@ -145,7 +146,8 @@  static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req)
 static void
 uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 {
-	struct uvc_video *video = req->context;
+	struct uvc_request *ureq = req->context;
+	struct uvc_video *video = ureq->video;
 	struct uvc_video_queue *queue = &video->queue;
 	unsigned long flags;
 
@@ -177,16 +179,19 @@  uvc_video_free_requests(struct uvc_video *video)
 {
 	unsigned int i;
 
-	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
-		if (video->req[i]) {
-			usb_ep_free_request(video->ep, video->req[i]);
-			video->req[i] = NULL;
-		}
-
-		if (video->req_buffer[i]) {
-			kfree(video->req_buffer[i]);
-			video->req_buffer[i] = NULL;
+	if (video->ureq) {
+		for (i = 0; i < video->uvc_num_requests; ++i) {
+			if (video->ureq[i].req) {
+				usb_ep_free_request(video->ep, video->ureq[i].req);
+				video->ureq[i].req = NULL;
+			}
+			if (video->ureq[i].req_buffer) {
+				kfree(video->ureq[i].req_buffer);
+				video->ureq[i].req_buffer = NULL;
+			}
 		}
+		kfree(video->ureq);
+		video->ureq = NULL;
 	}
 
 	INIT_LIST_HEAD(&video->req_free);
@@ -207,21 +212,26 @@  uvc_video_alloc_requests(struct uvc_video *video)
 		 * max_t(unsigned int, video->ep->maxburst, 1)
 		 * (video->ep->mult);
 
-	for (i = 0; i < UVC_NUM_REQUESTS; ++i) {
-		video->req_buffer[i] = kmalloc(req_size, GFP_KERNEL);
-		if (video->req_buffer[i] == NULL)
+	video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);
+	if (video->ureq == NULL)
+		return ret;
+
+	for (i = 0; i < video->uvc_num_requests; ++i) {
+		video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL);
+		if (video->ureq[i].req_buffer == NULL)
 			goto error;
 
-		video->req[i] = usb_ep_alloc_request(video->ep, GFP_KERNEL);
-		if (video->req[i] == NULL)
+		video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL);
+		if (video->ureq[i].req == NULL)
 			goto error;
 
-		video->req[i]->buf = video->req_buffer[i];
-		video->req[i]->length = 0;
-		video->req[i]->complete = uvc_video_complete;
-		video->req[i]->context = video;
+		video->ureq[i].req->buf = video->ureq[i].req_buffer;
+		video->ureq[i].req->length = 0;
+		video->ureq[i].req->complete = uvc_video_complete;
+		video->ureq[i].req->context = &video->ureq[i];
+		video->ureq[i].video = video;
 
-		list_add_tail(&video->req[i]->list, &video->req_free);
+		list_add_tail(&video->ureq[i].req->list, &video->req_free);
 	}
 
 	video->req_size = req_size;
@@ -312,9 +322,9 @@  int uvcg_video_enable(struct uvc_video *video, int enable)
 		cancel_work_sync(&video->pump);
 		uvcg_queue_cancel(&video->queue, 0);
 
-		for (i = 0; i < UVC_NUM_REQUESTS; ++i)
-			if (video->req[i])
-				usb_ep_dequeue(video->ep, video->req[i]);
+		for (i = 0; i < video->uvc_num_requests; ++i)
+			if (video->ureq && video->ureq[i].req)
+				usb_ep_dequeue(video->ep, video->ureq[i].req);
 
 		uvc_video_free_requests(video);
 		uvcg_queue_enable(&video->queue, 0);