[PATCH] media: uvc: Don't recompute frame size unnecessarily

[PATCH] media: uvc: Don't recompute frame size unnecessarily

[Laurent Pinchart]

uvc_fixup_video_ctrl() needs the frame size for bandwidth calculation,
which it computes from the frame width, height and bpp. The same
calculation is done earlier and the resulting value is stored in
dwMaxVideoFrameBufferSize. Reuse it instead of recomputing it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 8fa77a81dd7f..a269aeaa9ecc 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -178,7 +178,7 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 		 * high-speed devices) per second and add the UVC header size
 		 * (assumed to be 12 bytes long).
 		 */
-		bandwidth = frame->wWidth * frame->wHeight / 8 * format->bpp;
+		bandwidth = frame->dwMaxVideoFrameBufferSize;
 		bandwidth *= 10000000 / interval + 1;
 		bandwidth /= 1000;
 		if (stream->dev->udev->speed == USB_SPEED_HIGH)
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvc: Don't recompute frame size unnecessarily

[Kieran Bingham]

Hi Laurent,

On 18/12/2019 20:48, Laurent Pinchart wrote:
> uvc_fixup_video_ctrl() needs the frame size for bandwidth calculation,
> which it computes from the frame width, height and bpp. The same
> calculation is done earlier and the resulting value is stored in
> dwMaxVideoFrameBufferSize. Reuse it instead of recomputing it.


I presume you mean earlier during uvc_parse_format()?

uvc_parse_format() is called from uvc_parse_control(), while
uvc_fixup_video_ctrl() is called during uvc_register_chains(), both
call-paths originating from uvc_probe().

uvc_register_chains() does indeed run after uvc_parse_format() within
the uvc_probe() thus the ordering is correct.

However there is a slight difference in that within uvc_parse_format(),
frame->dwMaxVideoFrameBufferSize is set to buffer[17] if the frame-type
is UVC_VS_FRAME_FRAME_BASED, or zero otherwise.

And then it gets overridden again [0] if it is a non-compressed format,
to the calculation which is replaced/desired here.

[0]
https://elixir.bootlin.com/linux/v5.5-rc7/source/drivers/media/usb/uvc/uvc_driver.c#L627

Assuming there is no (supported) occasion where we could be calling
uvc_fixup_video_ctrl() on a compressed (or non-frame based) stream type...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 8fa77a81dd7f..a269aeaa9ecc 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -178,7 +178,7 @@ static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
>  		 * high-speed devices) per second and add the UVC header size
>  		 * (assumed to be 12 bytes long).
>  		 */
> -		bandwidth = frame->wWidth * frame->wHeight / 8 * format->bpp;
> +		bandwidth = frame->dwMaxVideoFrameBufferSize;
>  		bandwidth *= 10000000 / interval + 1;
>  		bandwidth /= 1000;
>  		if (stream->dev->udev->speed == USB_SPEED_HIGH)

-- 
Regards
--
Kieran