Skip to contentSkip to conversation

hdpvr: vidioc_g_fmt_vid_cap should not fail

PATCH NAK Leonid Kegulskiy 2 participants 2 messages Mar 22, 2013

Subsystems (2)
Media (V4L/DVB) Hdpvr USB video encoder driver
lore
Share & tools
lore

Leonid Kegulskiy

author
#0 lore raw

PATCH 1/1 hdpvr: vidioc_g_fmt_vid_cap should not fail

Fixed get_video_info() function that used to return NULL if video source is not present, subsequently causing vidioc_g_fmt_vid_cap to fail. Originally issue reported here:
http://www.spinics.net/lists/linux-media/msg54246.html

Signed-off-by Leonid Kegulskiy <[email protected]>
---
 drivers/media/usb/hdpvr/hdpvr-control.c |   29 +++++++++++++----------------
 drivers/media/usb/hdpvr/hdpvr-video.c   |    5 ++++-
 2 files changed, 17 insertions(+), 17 deletions(-)
drivers/media/usb/hdpvr/hdpvr-control.c +13 -16
index ae8f229..ed12159 100644
@@ -53,12 +53,6 @@ struct hdpvr_video_info *get_video_info(struct hdpvr_device *dev)
 #endif
 	int ret;
 
-	vidinf = kzalloc(sizeof(struct hdpvr_video_info), GFP_KERNEL);
-	if (!vidinf) {
-		v4l2_err(&dev->v4l2_dev, "out of memory\n");
-		goto err;
-	}
-
 	mutex_lock(&dev->usbc_mutex);
 	ret = usb_control_msg(dev->udev,
 			      usb_rcvctrlpipe(dev->udev, 0),

@@ -66,12 +60,6 @@ struct hdpvr_video_info *get_video_info(struct hdpvr_device *dev)
 			      0x1400, 0x0003,
 			      dev->usbc_buf, 5,
 			      1000);
-	if (ret == 5) {
-		vidinf->width	= dev->usbc_buf[1] << 8 | dev->usbc_buf[0];
-		vidinf->height	= dev->usbc_buf[3] << 8 | dev->usbc_buf[2];
-		vidinf->fps	= dev->usbc_buf[4];
-	}
-
 #ifdef HDPVR_DEBUG
 	if (hdpvr_debug & MSG_INFO) {
 		hex_dump_to_buffer(dev->usbc_buf, 5, 16, 1, print_buf,

@@ -82,11 +70,20 @@ struct hdpvr_video_info *get_video_info(struct hdpvr_device *dev)
 #endif
 	mutex_unlock(&dev->usbc_mutex);
 
-	if (!vidinf->width || !vidinf->height || !vidinf->fps) {
-		kfree(vidinf);
-		vidinf = NULL;
+	if (ret == 5)
+	{
+		vidinf = kzalloc(sizeof(struct hdpvr_video_info), GFP_KERNEL);
+		if (vidinf)
+		{
+			vidinf->width	= dev->usbc_buf[1] << 8 | dev->usbc_buf[0];
+			vidinf->height	= dev->usbc_buf[3] << 8 | dev->usbc_buf[2];
+			vidinf->fps	= dev->usbc_buf[4];
+		}
+		else
+		{
+			v4l2_err(&dev->v4l2_dev, "out of memory\n");
+		}
 	}
-err:
 	return vidinf;
 }
drivers/media/usb/hdpvr/hdpvr-video.c +4 -1
index da6b779..fb14012 100644
@@ -268,7 +268,7 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
 
 	vidinf = get_video_info(dev);
 
-	if (vidinf) {
+	if (vidinf && vidinf->width && vidinf->height && vidinf->fps) {
 		v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
 			 "video signal: %dx%d@%dhz\n", vidinf->width,
 			 vidinf->height, vidinf->fps);

@@ -293,6 +293,9 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
 
 		return 0;
 	}
+	
+	if (vidinf)
+		kfree(vidinf);
 	msleep(250);
 	v4l2_dbg(MSG_INFO, hdpvr_debug, &dev->v4l2_dev,
 		 "no video signal at input %d\n", dev->options.video_input);
Signature · 1 line
1.7.10.4
Nacked-by on v1 1/1 NAK
#1 lore raw
Quoted 4 lines · On Fri March 22 2013 11:59:05 Leonid Kegulskiy wrote:

Fixed get_video_info() function that used to return NULL if video source is not present, subsequently causing vidioc_g_fmt_vid_cap to fail. Originally issue reported here:
http://www.spinics.net/lists/linux-media/msg54246.html

Nacked-by Hans Verkuil <[email protected]>

A change like this should be done on top of my hdpvr patch series:

http://www.mail-archive.com/[email protected]/msg60040.html

I worked around it by keeping track if one of the new DV_TIMINGS ioctls for HDTV is used: if so, then the proper v4l2 behavior is used, otherwise I keep the existing behavior.

Janne, it is really strange that EFAULT is returned if there is no signal. Does mythtv or another application rely on that? If so, then we need to keep it like that for now.

gstreamer can try to use the DV_TIMINGS API instead, which has proper behavior.

Regards,

Hans

Quoted 86 lines
Signed-off-by Leonid Kegulskiy <[email protected]>
---
 drivers/media/usb/hdpvr/hdpvr-control.c |   29 +++++++++++++----------------
 drivers/media/usb/hdpvr/hdpvr-video.c   |    5 ++++-
 2 files changed, 17 insertions(+), 17 deletions(-)
drivers/media/usb/hdpvr/hdpvr-control.c +13 -16
index ae8f229..ed12159 100644
@@ -53,12 +53,6 @@ struct hdpvr_video_info *get_video_info(struct hdpvr_device *dev)
 #endif
 	int ret;
 
-	vidinf = kzalloc(sizeof(struct hdpvr_video_info), GFP_KERNEL);
-	if (!vidinf) {
-		v4l2_err(&dev->v4l2_dev, "out of memory\n");
-		goto err;
-	}
-
 	mutex_lock(&dev->usbc_mutex);
 	ret = usb_control_msg(dev->udev,
 			      usb_rcvctrlpipe(dev->udev, 0),

@@ -66,12 +60,6 @@ struct hdpvr_video_info *get_video_info(struct hdpvr_device *dev)
 			      0x1400, 0x0003,
 			      dev->usbc_buf, 5,
 			      1000);
-	if (ret == 5) {
-		vidinf->width	= dev->usbc_buf[1] << 8 | dev->usbc_buf[0];
-		vidinf->height	= dev->usbc_buf[3] << 8 | dev->usbc_buf[2];
-		vidinf->fps	= dev->usbc_buf[4];
-	}
-
 #ifdef HDPVR_DEBUG
 	if (hdpvr_debug & MSG_INFO) {
 		hex_dump_to_buffer(dev->usbc_buf, 5, 16, 1, print_buf,

@@ -82,11 +70,20 @@ struct hdpvr_video_info *get_video_info(struct hdpvr_device *dev)
 #endif
 	mutex_unlock(&dev->usbc_mutex);
 
-	if (!vidinf->width || !vidinf->height || !vidinf->fps) {
-		kfree(vidinf);
-		vidinf = NULL;
+	if (ret == 5)
+	{
+		vidinf = kzalloc(sizeof(struct hdpvr_video_info), GFP_KERNEL);
+		if (vidinf)
+		{
+			vidinf->width	= dev->usbc_buf[1] << 8 | dev->usbc_buf[0];
+			vidinf->height	= dev->usbc_buf[3] << 8 | dev->usbc_buf[2];
+			vidinf->fps	= dev->usbc_buf[4];
+		}
+		else
+		{
+			v4l2_err(&dev->v4l2_dev, "out of memory\n");
+		}
 	}
-err:
 	return vidinf;
 }
drivers/media/usb/hdpvr/hdpvr-video.c +4 -1
index da6b779..fb14012 100644
@@ -268,7 +268,7 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
 
 	vidinf = get_video_info(dev);
 
-	if (vidinf) {
+	if (vidinf && vidinf->width && vidinf->height && vidinf->fps) {
 		v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
 			 "video signal: %dx%d@%dhz\n", vidinf->width,
 			 vidinf->height, vidinf->fps);

@@ -293,6 +293,9 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
 
 		return 0;
 	}
+	
+	if (vidinf)
+		kfree(vidinf);
 	msleep(250);
 	v4l2_dbg(MSG_INFO, hdpvr_debug, &dev->v4l2_dev,
 		 "no video signal at input %d\n", dev->options.video_input);