On 7/7/19 11:15 PM, Keith Pyle wrote:
> The hdpvr firmware reacts poorly to a fast close/open sequence. Delaying
> a few seconds between the close and next open produces generally reliable
> results. Rather than requiring user programs to implement this delay and
> coordinate among themselves when the device is handed from one program to
> another, add kernel support for delaying the attempt to start streaming if
> the device only recently stopped streaming. A delay of 4 seconds seems to
> be sufficient, but some administrators may wish to push their luck by
> trying shorter delays. To allow administrators to change the delay, add a
> new parameter to the hdpvr module: `hdpvr_close_to_open_ms_delay`, which
> specifies the delay in milliseconds between a close and subsequent
> start-streaming. If the user application has already delayed by at least
> that long for its own reasons, this feature will add no further delay.
>
> Signed-off-by: Keith Pyle <kpyle@xxxxxxxxxxxxx>
> Tested-by: Keith Pyle <kpyle@xxxxxxxxxxxxx>
> ---
> Changes since v1:
> - Rewrapped output at 80 columns, per request from Hans. Literal strings
> still exceed 80 columns where necessary to keep an entire string together,
> since this makes it easier for grep to find the file and line that
> generates a given message.
> - Reviewed Hans request to use `jiffies` instead of `get_jiffies_64()`.
> Per the documentation, raw `jiffies` appears to be inappropriate
> on 32-bit systems, so the patch continues to use `get_jiffies_64()`.
That's news to all the 32-bit systems that have been using jiffies since the
dawn of time.
Please use jiffies, like everyone else. 'jiffies' is an unsigned long, so
32 bits on a 32 bit system and 64 bit on a 64 bit system. Just want you
want.
> On 64-bit systems, `get_jiffies_64()` becomes a direct read of `jiffies`.
> Further, both uses of `get_jiffies_64()` are on relatively cold paths
> (one just before starting streaming, the other just before a 10ms
> hardcoded sleep), so the performance impact even on the 32-bit path
> should be trivial relative to the time required for the surrounding code.
> ---
> drivers/media/usb/hdpvr/hdpvr-core.c | 4 ++++
> drivers/media/usb/hdpvr/hdpvr-video.c | 22 ++++++++++++++++++++++
> drivers/media/usb/hdpvr/hdpvr.h | 5 +++++
> 3 files changed, 31 insertions(+)
>
> diff --git a/drivers/media/usb/hdpvr/hdpvr-core.c b/drivers/media/usb/hdpvr/hdpvr-core.c
> index 23d3d0754308..fd7608e7e94c 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-core.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-core.c
> @@ -39,6 +39,10 @@ int hdpvr_debug;
> module_param(hdpvr_debug, int, S_IRUGO|S_IWUSR);
> MODULE_PARM_DESC(hdpvr_debug, "enable debugging output");
>
> +uint hdpvr_close_to_open_ms_delay = 4000;
> +module_param(hdpvr_close_to_open_ms_delay, uint, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(hdpvr_close_to_open_ms_delay, "delay restarting streaming by the specified number of milliseconds");
> +
> static uint default_video_input = HDPVR_VIDEO_INPUTS;
> module_param(default_video_input, uint, S_IRUGO|S_IWUSR);
> MODULE_PARM_DESC(default_video_input, "default video input: 0=Component / 1=S-Video / 2=Composite");
> diff --git a/drivers/media/usb/hdpvr/hdpvr-video.c b/drivers/media/usb/hdpvr/hdpvr-video.c
> index 3d199d5d6738..8a2b883d372e 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-video.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-video.c
> @@ -278,6 +278,8 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
> {
> int ret;
> struct hdpvr_video_info vidinf;
> + u64 now_jiffies, delta_jiffies;
> + uint msec_to_sleep;
>
> if (dev->status == STATUS_STREAMING)
> return 0;
> @@ -298,6 +300,22 @@ static int hdpvr_start_streaming(struct hdpvr_device *dev)
> v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
> "video signal: %dx%d@%dhz\n", vidinf.width,
> vidinf.height, vidinf.fps);
> + now_jiffies = get_jiffies_64();
> + /* inline time_after64 since the result of the subtraction is needed
> + * for the sleep
> + */
> + delta_jiffies = dev->jiffies_next_start_streaming - now_jiffies;
> + if ((__s64)delta_jiffies > 0) {
> + /* device firmware may not be ready yet */
> + msec_to_sleep = jiffies_to_msecs(delta_jiffies);
> + v4l2_dbg(MSG_INFO, hdpvr_debug, &dev->v4l2_dev,
> + "firmware may not be ready, sleeping for %u ms\n",
> + msec_to_sleep);
> + msleep(msec_to_sleep);
> + } else {
> + v4l2_dbg(MSG_INFO, hdpvr_debug, &dev->v4l2_dev,
> + "firmware assumed to be ready, not sleeping\n");
> + }
>
> /* start streaming 2 request */
> hdpvr_usb_lock(dev, HDPVR_USB_CTRL);
> @@ -332,6 +350,7 @@ static int hdpvr_stop_streaming(struct hdpvr_device *dev)
> int actual_length;
> uint c = 0;
> u8 *buf;
> + u64 now_jiffies;
>
> if (dev->status == STATUS_IDLE)
> return 0;
> @@ -368,6 +387,9 @@ static int hdpvr_stop_streaming(struct hdpvr_device *dev)
> kfree(buf);
> v4l2_dbg(MSG_BUFFER, hdpvr_debug, &dev->v4l2_dev,
> "used %d urbs to empty device buffers\n", c-1);
> + now_jiffies = get_jiffies_64();
> + dev->jiffies_next_start_streaming = now_jiffies +
> + msecs_to_jiffies(hdpvr_close_to_open_ms_delay);
> msleep(10);
>
> dev->status = STATUS_IDLE;
> diff --git a/drivers/media/usb/hdpvr/hdpvr.h b/drivers/media/usb/hdpvr/hdpvr.h
> index 7b3d166da1dd..9e5f88146827 100644
> --- a/drivers/media/usb/hdpvr/hdpvr.h
> +++ b/drivers/media/usb/hdpvr/hdpvr.h
> @@ -43,6 +43,7 @@
> /* #define HDPVR_DEBUG */
>
> extern int hdpvr_debug;
> +extern uint hdpvr_close_to_open_ms_delay;
>
> #define MSG_INFO 1
> #define MSG_BUFFER 2
> @@ -95,6 +96,10 @@ struct hdpvr_device {
> struct v4l2_dv_timings cur_dv_timings;
>
> uint flags;
> + /* earliest jiffies at which the device firmware will be ready to
Multiple comments should put the '/*' on a separate line. Please adjust
both patches for this. Don't forget to run 'checkpatch.pl --strict' for both
patches before posting!
> + * start streaming
> + */
> + u64 jiffies_next_start_streaming;
The indent of jiffies_next_start_streaming doesn't seem to match the other fields.
>
> /* synchronize I/O */
> struct mutex io_mutex;
>
Regards,
Hans
