On 8/4/19 12:09 AM, 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 v2:
> - Rewrapped comments again, per request from Hans.
> - Per advice from checkpatch.pl --strict (suggested by Hans), added
> spacing around `|` for mode permissions. This satisfies checkpatch,
> but reduces consistency in hdpvr-core.c, which had preexisting uses that
> violate checkpatch --strict.
That's OK.
> - Changed indentation of declaration of jiffies_next_start_streaming to
> line up when viewed with tabstop=8.
> 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
Per what documentation? 'jiffies' is used everywhere, so this makes no sense.
Just look at the use counts:
$ git grep [^_a-z0-9]jiffies[^_a-z0-9]|wc
10712 60812 810344
$ git grep get_jiffies_64|wc
83 401 6342
get_jiffies_64() should only be used in the rare cases where you need a
64-bit jiffies value, even on a 32-bit system. That's not needed here.
Doing unusual things requires a good reason, and there is no good reason
to use get_jiffies_64 here.
> on 32-bit systems, so the patch continues to use `get_jiffies_64()`.
> 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..a3d2f632fe38 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);
I'm getting this warning from checkpatch:
WARNING: Symbolic permissions 'S_IRUGO | S_IWUSR' are not preferred. Consider using octal permissions '0644'.
#10: FILE: drivers/media/usb/hdpvr/hdpvr-core.c:39:
+module_param(hdpvr_close_to_open_ms_delay, uint, S_IRUGO | S_IWUSR);
Same elsewhere. Please use 0644 instead.
> +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..7e5897dd8dff 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 preferred style (Documentation/process/coding-style.rst) for multi-line comments
is to have the /* on a line of its own.
Regards,
Hans
> + * 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..32498b7120aa 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 start
> + * streaming
> + */
> + u64 jiffies_next_start_streaming;
>
> /* synchronize I/O */
> struct mutex io_mutex;
>
