diff mbox

media: uvcvideo: Support realtek's UVC 1.5 device

Message ID 2510852.fx2XduE8hM@avalon (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Pinchart May 22, 2018, 8:32 p.m. UTC 
Hello,

Thank you for the patch.

On Wednesday, 9 May 2018 05:13:08 EEST ming_qian@realsil.com.cn wrote:
> From: ming_qian <ming_qian@realsil.com.cn>
> 
> The length of UVC 1.5 video control is 48, and it id 34 for UVC 1.1.
> Change it to 48 for UVC 1.5 device,
> and the UVC 1.5 device can be recognized.
> 
> More changes to the driver are needed for full UVC 1.5 compatibility.
> However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have
> been reported to work well.

This patch is however not specific to Realtek devices, so I think we should
make the subject line more generic. It's fine mentioning in the commit message
itself that the Realtek RTS5847/RTS5852 cameras have been successfully tested.

> Signed-off-by: ming_qian <ming_qian@realsil.com.cn>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index aa0082f..32dfb32 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -171,6 +171,8 @@ static int uvc_get_video_ctrl(struct uvc_streaming
> *stream, int ret;
> 
>  	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
> +	if (stream->dev->uvc_version >= 0x0150)
> +		size = 48;
>  	if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) &&
>  			query == UVC_GET_DEF)
>  		return -EIO;
> @@ -259,6 +261,8 @@ static int uvc_set_video_ctrl(struct uvc_streaming
> *stream, int ret;
> 
>  	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
> +	if (stream->dev->uvc_version >= 0x0150)
> +		size = 48;
>  	data = kzalloc(size, GFP_KERNEL);
>  	if (data == NULL)
>  		return -ENOMEM;

Instead of duplicating the computation in both functions, I think we should
move the code to a helper function.

Furthermore there are equality checks further down both functions that compare
the size to 34, they should be updated to also support UVC 1.5.

I propose the following updated patch. If you're fine with it there's no need
to resubmit, I'll queue it for v4.19.

I have dropped the Reviewed-by and Tested-by tags as the patch has changed.

commit a9c002732695eab2096580a0d1a1687bc2f95928
Author: ming_qian <ming_qian@realsil.com.cn>
Date:   Wed May 9 10:13:08 2018 +0800

    media: uvcvideo: Support UVC 1.5 video probe & commit controls
    
    The length of UVC 1.5 video control is 48, and it is 34 for UVC 1.1.
    Change it to 48 for UVC 1.5 device, and the UVC 1.5 device can be
    recognized.
    
    More changes to the driver are needed for full UVC 1.5 compatibility.
    However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have been
    reported to work well.
    
    Cc: stable@vger.kernel.org
    Signed-off-by: ming_qian <ming_qian@realsil.com.cn>
    [Factor out code to helper function, update size checks]
    Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Comments

Kai-Heng Feng May 23, 2018, 5:42 a.m. UTC | #1 
> On May 23, 2018, at 4:32 AM, Laurent Pinchart  
> <laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> Thank you for the patch.
>
> On Wednesday, 9 May 2018 05:13:08 EEST ming_qian@realsil.com.cn wrote:
>> From: ming_qian <ming_qian@realsil.com.cn>
>>
>> The length of UVC 1.5 video control is 48, and it id 34 for UVC 1.1.
>> Change it to 48 for UVC 1.5 device,
>> and the UVC 1.5 device can be recognized.
>>
>> More changes to the driver are needed for full UVC 1.5 compatibility.
>> However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have
>> been reported to work well.
>
> This patch is however not specific to Realtek devices, so I think we should
> make the subject line more generic. It's fine mentioning in the commit  
> message
> itself that the Realtek RTS5847/RTS5852 cameras have been successfully  
> tested.
>
>> Signed-off-by: ming_qian <ming_qian@realsil.com.cn>
>> ---
>>  drivers/media/usb/uvc/uvc_video.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index aa0082f..32dfb32 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -171,6 +171,8 @@ static int uvc_get_video_ctrl(struct uvc_streaming
>> *stream, int ret;
>>
>>  	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
>> +	if (stream->dev->uvc_version >= 0x0150)
>> +		size = 48;
>>  	if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) &&
>>  			query == UVC_GET_DEF)
>>  		return -EIO;
>> @@ -259,6 +261,8 @@ static int uvc_set_video_ctrl(struct uvc_streaming
>> *stream, int ret;
>>
>>  	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
>> +	if (stream->dev->uvc_version >= 0x0150)
>> +		size = 48;
>>  	data = kzalloc(size, GFP_KERNEL);
>>  	if (data == NULL)
>>  		return -ENOMEM;
>
> Instead of duplicating the computation in both functions, I think we should
> move the code to a helper function.
>
> Furthermore there are equality checks further down both functions that  
> compare
> the size to 34, they should be updated to also support UVC 1.5.
>
> I propose the following updated patch. If you're fine with it there's no  
> need
> to resubmit, I'll queue it for v4.19.
>
> I have dropped the Reviewed-by and Tested-by tags as the patch has changed.
>
> commit a9c002732695eab2096580a0d1a1687bc2f95928
> Author: ming_qian <ming_qian@realsil.com.cn>
> Date:   Wed May 9 10:13:08 2018 +0800
>
>     media: uvcvideo: Support UVC 1.5 video probe & commit controls
>
>     The length of UVC 1.5 video control is 48, and it is 34 for UVC 1.1.
>     Change it to 48 for UVC 1.5 device, and the UVC 1.5 device can be
>     recognized.
>
>     More changes to the driver are needed for full UVC 1.5 compatibility.
>     However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have been
>     reported to work well.
>
>     Cc: stable@vger.kernel.org
>     Signed-off-by: ming_qian <ming_qian@realsil.com.cn>
>     [Factor out code to helper function, update size checks]
>     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I tested this new patch and it works well.

Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

>
> diff --git a/drivers/media/usb/uvc/uvc_video.c  
> b/drivers/media/usb/uvc/uvc_video.c
> index eb9e04a59427..285b0e813b9d 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -207,14 +207,27 @@ static void uvc_fixup_video_ctrl(struct  
> uvc_streaming *stream,
>  	}
>  }
>
> +static size_t uvc_video_ctrl_size(struct uvc_streaming *stream)
> +{
> +	/*
> +	 * Return the size of the video probe and commit controls, which depends
> +	 * on the protocol version.
> +	 */
> +	if (stream->dev->uvc_version < 0x0110)
> +		return 26;
> +	else if (stream->dev->uvc_version < 0x0150)
> +		return 34;
> +	else
> +		return 48;
> +}
> +
>  static int uvc_get_video_ctrl(struct uvc_streaming *stream,
>  	struct uvc_streaming_control *ctrl, int probe, u8 query)
>  {
> +	u16 size = uvc_video_ctrl_size(stream);
>  	u8 *data;
> -	u16 size;
>  	int ret;
>
> -	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
>  	if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) &&
>  			query == UVC_GET_DEF)
>  		return -EIO;
> @@ -271,7 +284,7 @@ static int uvc_get_video_ctrl(struct uvc_streaming  
> *stream,
>  	ctrl->dwMaxVideoFrameSize = get_unaligned_le32(&data[18]);
>  	ctrl->dwMaxPayloadTransferSize = get_unaligned_le32(&data[22]);
>
> -	if (size == 34) {
> +	if (size >= 34) {
>  		ctrl->dwClockFrequency = get_unaligned_le32(&data[26]);
>  		ctrl->bmFramingInfo = data[30];
>  		ctrl->bPreferedVersion = data[31];
> @@ -300,11 +313,10 @@ static int uvc_get_video_ctrl(struct uvc_streaming  
> *stream,
>  static int uvc_set_video_ctrl(struct uvc_streaming *stream,
>  	struct uvc_streaming_control *ctrl, int probe)
>  {
> +	u16 size = uvc_video_ctrl_size(stream);
>  	u8 *data;
> -	u16 size;
>  	int ret;
>
> -	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
>  	data = kzalloc(size, GFP_KERNEL);
>  	if (data == NULL)
>  		return -ENOMEM;
> @@ -321,7 +333,7 @@ static int uvc_set_video_ctrl(struct uvc_streaming  
> *stream,
>  	put_unaligned_le32(ctrl->dwMaxVideoFrameSize, &data[18]);
>  	put_unaligned_le32(ctrl->dwMaxPayloadTransferSize, &data[22]);
>
> -	if (size == 34) {
> +	if (size >= 34) {
>  		put_unaligned_le32(ctrl->dwClockFrequency, &data[26]);
>  		data[30] = ctrl->bmFramingInfo;
>  		data[31] = ctrl->bPreferedVersion;
>
> -- 
> Regards,
>
> Laurent Pinchart
Ana Guerrero Lopez May 23, 2018, 9:41 a.m. UTC | #2 
> > commit a9c002732695eab2096580a0d1a1687bc2f95928
> > Author: ming_qian <ming_qian@realsil.com.cn>
> > Date:   Wed May 9 10:13:08 2018 +0800
> > 
> >     media: uvcvideo: Support UVC 1.5 video probe & commit controls
> > 
> >     The length of UVC 1.5 video control is 48, and it is 34 for UVC 1.1.
> >     Change it to 48 for UVC 1.5 device, and the UVC 1.5 device can be
> >     recognized.
> > 
> >     More changes to the driver are needed for full UVC 1.5 compatibility.
> >     However, at least the UVC 1.5 Realtek RTS5847/RTS5852 cameras have been
> >     reported to work well.
> > 
> >     Cc: stable@vger.kernel.org
> >     Signed-off-by: ming_qian <ming_qian@realsil.com.cn>
> >     [Factor out code to helper function, update size checks]
> >     Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I tested this new patch and it works well.
> 
> Tested-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

I tested it as well and it works well.

Tested-by: Ana Guerrero Lopez <ana.guerrero@collabora.com>


> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c
> > index eb9e04a59427..285b0e813b9d 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -207,14 +207,27 @@ static void uvc_fixup_video_ctrl(struct
> > uvc_streaming *stream,
> >  	}
> >  }
> > 
> > +static size_t uvc_video_ctrl_size(struct uvc_streaming *stream)
> > +{
> > +	/*
> > +	 * Return the size of the video probe and commit controls, which depends
> > +	 * on the protocol version.
> > +	 */
> > +	if (stream->dev->uvc_version < 0x0110)
> > +		return 26;
> > +	else if (stream->dev->uvc_version < 0x0150)
> > +		return 34;
> > +	else
> > +		return 48;
> > +}
> > +
> >  static int uvc_get_video_ctrl(struct uvc_streaming *stream,
> >  	struct uvc_streaming_control *ctrl, int probe, u8 query)
> >  {
> > +	u16 size = uvc_video_ctrl_size(stream);
> >  	u8 *data;
> > -	u16 size;
> >  	int ret;
> > 
> > -	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
> >  	if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) &&
> >  			query == UVC_GET_DEF)
> >  		return -EIO;
> > @@ -271,7 +284,7 @@ static int uvc_get_video_ctrl(struct uvc_streaming
> > *stream,
> >  	ctrl->dwMaxVideoFrameSize = get_unaligned_le32(&data[18]);
> >  	ctrl->dwMaxPayloadTransferSize = get_unaligned_le32(&data[22]);
> > 
> > -	if (size == 34) {
> > +	if (size >= 34) {
> >  		ctrl->dwClockFrequency = get_unaligned_le32(&data[26]);
> >  		ctrl->bmFramingInfo = data[30];
> >  		ctrl->bPreferedVersion = data[31];
> > @@ -300,11 +313,10 @@ static int uvc_get_video_ctrl(struct uvc_streaming
> > *stream,
> >  static int uvc_set_video_ctrl(struct uvc_streaming *stream,
> >  	struct uvc_streaming_control *ctrl, int probe)
> >  {
> > +	u16 size = uvc_video_ctrl_size(stream);
> >  	u8 *data;
> > -	u16 size;
> >  	int ret;
> > 
> > -	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
> >  	data = kzalloc(size, GFP_KERNEL);
> >  	if (data == NULL)
> >  		return -ENOMEM;
> > @@ -321,7 +333,7 @@ static int uvc_set_video_ctrl(struct uvc_streaming
> > *stream,
> >  	put_unaligned_le32(ctrl->dwMaxVideoFrameSize, &data[18]);
> >  	put_unaligned_le32(ctrl->dwMaxPayloadTransferSize, &data[22]);
> > 
> > -	if (size == 34) {
> > +	if (size >= 34) {
> >  		put_unaligned_le32(ctrl->dwClockFrequency, &data[26]);
> >  		data[30] = ctrl->bmFramingInfo;
> >  		data[31] = ctrl->bPreferedVersion;
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
diff mbox

Patch

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index eb9e04a59427..285b0e813b9d 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -207,14 +207,27 @@  static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
 	}
 }
 
+static size_t uvc_video_ctrl_size(struct uvc_streaming *stream)
+{
+	/*
+	 * Return the size of the video probe and commit controls, which depends
+	 * on the protocol version.
+	 */
+	if (stream->dev->uvc_version < 0x0110)
+		return 26;
+	else if (stream->dev->uvc_version < 0x0150)
+		return 34;
+	else
+		return 48;
+}
+
 static int uvc_get_video_ctrl(struct uvc_streaming *stream,
 	struct uvc_streaming_control *ctrl, int probe, u8 query)
 {
+	u16 size = uvc_video_ctrl_size(stream);
 	u8 *data;
-	u16 size;
 	int ret;
 
-	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
 	if ((stream->dev->quirks & UVC_QUIRK_PROBE_DEF) &&
 			query == UVC_GET_DEF)
 		return -EIO;
@@ -271,7 +284,7 @@  static int uvc_get_video_ctrl(struct uvc_streaming *stream,
 	ctrl->dwMaxVideoFrameSize = get_unaligned_le32(&data[18]);
 	ctrl->dwMaxPayloadTransferSize = get_unaligned_le32(&data[22]);
 
-	if (size == 34) {
+	if (size >= 34) {
 		ctrl->dwClockFrequency = get_unaligned_le32(&data[26]);
 		ctrl->bmFramingInfo = data[30];
 		ctrl->bPreferedVersion = data[31];
@@ -300,11 +313,10 @@  static int uvc_get_video_ctrl(struct uvc_streaming *stream,
 static int uvc_set_video_ctrl(struct uvc_streaming *stream,
 	struct uvc_streaming_control *ctrl, int probe)
 {
+	u16 size = uvc_video_ctrl_size(stream);
 	u8 *data;
-	u16 size;
 	int ret;
 
-	size = stream->dev->uvc_version >= 0x0110 ? 34 : 26;
 	data = kzalloc(size, GFP_KERNEL);
 	if (data == NULL)
 		return -ENOMEM;
@@ -321,7 +333,7 @@  static int uvc_set_video_ctrl(struct uvc_streaming *stream,
 	put_unaligned_le32(ctrl->dwMaxVideoFrameSize, &data[18]);
 	put_unaligned_le32(ctrl->dwMaxPayloadTransferSize, &data[22]);
 
-	if (size == 34) {
+	if (size >= 34) {
 		put_unaligned_le32(ctrl->dwClockFrequency, &data[26]);
 		data[30] = ctrl->bmFramingInfo;
 		data[31] = ctrl->bPreferedVersion;