Re: [PATCH] zr36067: Debugging cleanups (updated again)

Jean Delvare <[email protected]>
Newsgroups gmane.comp.video.mjpeg.devel
Message-ID <[email protected]> 
Hi Trent,

I have been reviewing and testing your patches:

(Disclaimer: I am not familiar with the zr36067 driver code.)

> From: Trent Piepho <[email protected]>
> 
> zr36067: Fix problems with module parameters
> 
> Add permissions to all the module parameters so they can be queried and set
> (when possible) via sysfs.
> 
> Add description for the vidmem parameter.
> 
> Change the video_nr parameter to an array, so that the video number can be
> specified when a user has more than one card.  The driver would try to give
> all cards the same number otherwise, which will fail for all cards after the
> first.
> 
> The default_input option would only allow values of 0 or 1, contrary to the
> description.  Allow values up to the number of inputs defined for the card.
> 
> Add description of lock_norm's different behavior for 1 and >1.
> 
> Signed-off-by: Trent Piepho <[email protected]>
> Cc: Ronald S. Bultje <[email protected]>
> 
> diff --git a/linux/drivers/media/video/zoran_card.c b/linux/drivers/media/video/zoran_card.c
> --- a/linux/drivers/media/video/zoran_card.c
> +++ b/linux/drivers/media/video/zoran_card.c
> @@ -67,15 +67,15 @@ extern const struct zoran_format zoran_f
>  extern const struct zoran_format zoran_formats[];
>  
>  static int card[BUZ_MAX] = { -1, -1, -1, -1 };
> -module_param_array(card, int, NULL, 0);
> +module_param_array(card, int, NULL, 0444);
>  MODULE_PARM_DESC(card, "The type of card");
>  
>  static int encoder[BUZ_MAX] = { -1, -1, -1, -1 };
> -module_param_array(encoder, int, NULL, 0);
> +module_param_array(encoder, int, NULL, 0444);
>  MODULE_PARM_DESC(encoder, "i2c TV encoder");
>  
>  static int decoder[BUZ_MAX] = { -1, -1, -1, -1 };
> -module_param_array(decoder, int, NULL, 0);
> +module_param_array(decoder, int, NULL, 0444);
>  MODULE_PARM_DESC(decoder, "i2c TV decoder");
>  
>  /*
> @@ -87,29 +87,31 @@ MODULE_PARM_DESC(decoder, "i2c TV decode
>   */
>  
>  static unsigned long vidmem = 0;	/* Video memory base address */
> -module_param(vidmem, ulong, 0);
> +module_param(vidmem, ulong, 0444);
> +MODULE_PARM_DESC(vidmem, "Default video memory base address");
>  
>  /*
>     Default input and video norm at startup of the driver.
>  */
>  
>  static int default_input = 0;	/* 0=Composite, 1=S-Video */
> -module_param(default_input, int, 0);
> +module_param(default_input, int, 0444);
>  MODULE_PARM_DESC(default_input,
>  		 "Default input (0=Composite, 1=S-Video, 2=Internal)");
>  
>  static int default_mux = 1;	/* 6 Eyes input selection */
> -module_param(default_mux, int, 0);
> +module_param(default_mux, int, 0644);
>  MODULE_PARM_DESC(default_mux,
>  		 "Default 6 Eyes mux setting (Input selection)");
>  
>  static int default_norm = 0;	/* 0=PAL, 1=NTSC 2=SECAM */
> -module_param(default_norm, int, 0);
> +module_param(default_norm, int, 0444);
>  MODULE_PARM_DESC(default_norm, "Default norm (0=PAL, 1=NTSC, 2=SECAM)");
>  
> -static int video_nr = -1;	/* /dev/videoN, -1 for autodetect */
> -module_param(video_nr, int, 0);
> -MODULE_PARM_DESC(video_nr, "video device number");
> +/* /dev/videoN, -1 for autodetect */
> +static int video_nr[BUZ_MAX] = {-1, -1, -1, -1};
> +module_param_array(video_nr, int, NULL, 0444);
> +MODULE_PARM_DESC(video_nr, "video device number (-1=Auto)");
>  
>  /*
>     Number and size of grab buffers for Video 4 Linux
> @@ -130,21 +132,21 @@ MODULE_PARM_DESC(video_nr, "video device
>  
>  int v4l_nbufs = 2;
>  int v4l_bufsize = 128;		/* Everybody should be able to work with this setting */
> -module_param(v4l_nbufs, int, 0);
> +module_param(v4l_nbufs, int, 0644);
>  MODULE_PARM_DESC(v4l_nbufs, "Maximum number of V4L buffers to use");

This one I don't think is safe in its current form. First reason,
v4l_nbufs is used in VIDIOC_REQBUFS to control user input. If its value
has changed since the device was opened, I expect problems. To be on
the safe side, I believe that you need to change the code in
VIDIOC_REQBUFS to use fh->v4l_buffers.num_buffers instead of v4l_nbufs.
Second reason, there are limit checks done at init time on the value of
v4l_nbufs (in init_dc10_cards). If the user can change the values
through sysfs, these checks are bypassed. Thus I would suggest that you
move the limit checks to zoran_open_init_session, where the value of
v4l_nbufs is copied to the device-specific structure.

> -module_param(v4l_bufsize, int, 0);
> +module_param(v4l_bufsize, int, 0644);
>  MODULE_PARM_DESC(v4l_bufsize, "Maximum size per V4L buffer (in kB)");

My second comment above (limit checking) applies to this one too.

>  
>  int jpg_nbufs = 32;
>  int jpg_bufsize = 512;		/* max size for 100% quality full-PAL frame */
> -module_param(jpg_nbufs, int, 0);
> +module_param(jpg_nbufs, int, 0644);
>  MODULE_PARM_DESC(jpg_nbufs, "Maximum number of JPG buffers to use");

This one is used in BUZIOC_REQBUFS and VIDIOC_REQBUFS, so both comments
above apply.

> -module_param(jpg_bufsize, int, 0);
> +module_param(jpg_bufsize, int, 0644);
>  MODULE_PARM_DESC(jpg_bufsize, "Maximum size per JPG buffer (in kB)");

This one is used in BUZIOC_REQBUFS and also in
zoran_v4l2_calc_bufsize(), both comments above apply as well.

>  
>  int pass_through = 0;		/* 1=Pass through TV signal when device is not used */
>  				/* 0=Show color bar when device is not used (LML33: only if lml33dpath=1) */
> -module_param(pass_through, int, 0);
> +module_param(pass_through, int, 0644);
>  MODULE_PARM_DESC(pass_through,
>  		 "Pass TV signal through to TV-out when idling");
>  
> @@ -1117,7 +1119,9 @@ zr36057_init (struct zoran *zr)
>  		zr->timing = zr->card.tvn[zr->norm];
>  	}
>  
> -	zr->input = default_input = (default_input ? 1 : 0);
> +	if (default_input > zr->card.inputs-1)
> +		default_input = zr->card.inputs-1;

Wouldn't it make more sense to print a warning in the logs and default
to 0 in this case? Also, default_input is an int so checking for
negative values would be welcome too.

> +	zr->input = default_input;
>  
>  	/* Should the following be reset at every open ? */
>  	zr->hue = 32768;
> @@ -1149,7 +1153,7 @@ zr36057_init (struct zoran *zr)
>  	 */
>  	memcpy(zr->video_dev, &zoran_template, sizeof(zoran_template));
>  	strcpy(zr->video_dev->name, ZR_DEVNAME(zr));
> -	err = video_register_device(zr->video_dev, VFL_TYPE_GRABBER, video_nr);
> +	err = video_register_device(zr->video_dev, VFL_TYPE_GRABBER, video_nr[zr->id]);
>  	if (err < 0)
>  		goto exit_unregister;
>  
> diff --git a/linux/drivers/media/video/zoran_device.c b/linux/drivers/media/video/zoran_device.c
> --- a/linux/drivers/media/video/zoran_device.c
> +++ b/linux/drivers/media/video/zoran_device.c
> @@ -70,7 +70,7 @@ static int lml33dpath = 0;	/* 1 will use
>  				 * load on Bt819 input, there will be
>  				 * some image imperfections */
>  
> -module_param(lml33dpath, bool, 0);
> +module_param(lml33dpath, bool, 0644);
>  MODULE_PARM_DESC(lml33dpath,
>  		 "Use digital path capture mode (on LML33 cards)");
>  
> diff --git a/linux/drivers/media/video/zoran_driver.c b/linux/drivers/media/video/zoran_driver.c
> --- a/linux/drivers/media/video/zoran_driver.c
> +++ b/linux/drivers/media/video/zoran_driver.c
> @@ -202,8 +202,8 @@ extern int pass_through;
>  extern int pass_through;
>  
>  static int lock_norm = 0;	/* 1=Don't change TV standard (norm) */
> -module_param(lock_norm, int, 0);
> -MODULE_PARM_DESC(lock_norm, "Users can't change norm");
> +module_param(lock_norm, int, 0644);
> +MODULE_PARM_DESC(lock_norm, "Prevent norm changes (1 = ignore, >1 = fail)");

There is a minor race with this one:

	if (lock_norm && norm != zr->norm) {
		if (lock_norm > 1) {
			dprintk(1,
				KERN_WARNING
				"%s: set_norm() - TV standard is locked, can not switch norm\n",
				ZR_DEVNAME(zr));
			return -EPERM;
		} else {
			dprintk(1,
				KERN_WARNING
				"%s: set_norm() - TV standard is locked, norm was not changed\n",
				ZR_DEVNAME(zr));
			norm = zr->norm;
		}
	}

Imagine that lock_norm is originally set to 2, and the user switches it
to 0 between the 1st and 2nd line. The test "lock_norm > 1"  will fail,
and the "else" statement will be executed, while it corresponds to
lock_norm = 1 which was never set.

I agree it's not very important in practice, but OTOH this can be easily
fixed that way:

	if (norm != zr->norm) {
		if (lock_norm > 1) {
			dprintk(1,
				KERN_WARNING
				"%s: set_norm() - TV standard is locked, can not switch norm\n",
				ZR_DEVNAME(zr));
			return -EPERM;
		} else if (lock_norm == 1)  {
			dprintk(1,
				KERN_WARNING
				"%s: set_norm() - TV standard is locked, norm was not changed\n",
				ZR_DEVNAME(zr));
			norm = zr->norm;
		}
	}

(As a side note, I'm curious as to why this module parameter exists in
the first place, it doesn't seem particularly useful, and it gives
users an easy way to fill the kernel log buffer when used. I couldn't
find any other media/video driver offering such a parameter.)

> From: Trent Piepho <[email protected]>
> 
> zr36067: Fix problem setting norms
> 
> The zr36067 driver doesn't make a distinction between the different sub-types
> of NTSC, PAL, or SECAM norms.  For example, when the enum std ioctl returns
> the PAL standard it returns PAL_BG|PAL_DK|PAL_H|PAL_I.
> 
> When setting the norm, it required the bitmask to match exactly the set of
> norms used during the enumeration.  If just one norm was specified, for
> example PAL_BG or NTSC_M, it would fail.  This violates the V4L2 spec,
> "VIDIOC_S_STD accepts *one* or more flags..."
> 
> This fixes the S_STD function so that it will accept any set of one or more
> PAL norms as PAL, and the same for NTSC and SECAM.
> 
> Signed-off-by: Trent Piepho <[email protected]>
> 
> diff --git a/linux/drivers/media/video/zoran_driver.c b/linux/drivers/media/video/zoran_driver.c
> --- a/linux/drivers/media/video/zoran_driver.c
> +++ b/linux/drivers/media/video/zoran_driver.c
> @@ -3760,11 +3760,11 @@ zoran_do_ioctl (struct inode *inode,
>  		dprintk(3, KERN_DEBUG "%s: VIDIOC_S_STD - norm=0x%llx\n",
>  			ZR_DEVNAME(zr), (unsigned long long)*std);
>  
> -		if (*std == V4L2_STD_PAL)
> +		if ((*std & V4L2_STD_PAL) && !(*std & ~V4L2_STD_PAL))
>  			norm = VIDEO_MODE_PAL;
> -		else if (*std == V4L2_STD_NTSC)
> +		else if ((*std & V4L2_STD_NTSC) && !(*std & ~V4L2_STD_NTSC))
>  			norm = VIDEO_MODE_NTSC;
> -		else if (*std == V4L2_STD_SECAM)
> +		else if ((*std & V4L2_STD_SECAM) && !(*std & ~V4L2_STD_SECAM))
>  			norm = VIDEO_MODE_SECAM;
>  		else if (*std == V4L2_STD_ALL)
>  			norm = VIDEO_MODE_AUTO;

Very nice. I can confirm this fixes an error I was receiving from
tvtime. This still isn't enough for tvtime to work with my DC10+, but
at least this is a move in the right direction. (The remaining error
is: "videoinput: Can't get tuner info: Invalid argument".)

-- 
Jean Delvare

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.