* [PATCH 2/2]: media: hdpvr: Add optional restart, with optional delay, after restarting streaming
@ 2019-07-07 21:16 Keith Pyle
2019-07-25 7:17 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Keith Pyle @ 2019-07-07 21:16 UTC (permalink / raw)
To: Hans Verkuil, Linux Media Mailing List
`hdpvr_read` attempts to restart streaming if the device is read while
it is both not ready and not disconnected. However, the device is often
still not ready immediately after the call to `hdpvr_start_streaming`
returns, causing the condition `if (buf->status != BUFSTAT_READY)` to
exit the loop without reading any further data. By itself, this would
merely cause a short read, which should be easily recoverable. However,
if no data has been read so far, this causes `hdpvr_read` to return 0,
which results in an end-of-file for the user application.
Compensate for this by adding the ability to delay after the call to
`hdpvr_start_streaming`, then `continue;` back to the top, so that
`hdpvr_read` can call `wait_event_interruptible_timeout` again to wait
for the device to become ready. This delay complements the prior patch.
The prior patch delays before issuing the start-streaming command, to
give the firmware time to stabilize before receiving the command. This
delay is after the start-streaming command, to give the firmware time to
bring the device to a ready state. This delay is configurable through a
new module parameter, `hdpvr_restart_streaming_ms_delay`, which defaults
to a 100 millisecond delay.
To avoid an infinite loop in `hdpvr_read`, add a limit to how many times
`hdpvr_read` can restart the device before returning. This limit is
configurable through a new module parameter,
`hdpvr_restart_streaming_max_tries`, and defaults to one restart.
Administrators may set the limit to 0 to request that `hdpvr_read` never
attempt to restart streaming. Previously, there was no way for
administrators to opt out of an attempted restart.
Signed-off-by: Keith Pyle <kpyle@austin.rr.com>
Tested-by: Keith Pyle <kpyle@austin.rr.com>
---
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.
---
drivers/media/usb/hdpvr/hdpvr-core.c | 8 ++++++
drivers/media/usb/hdpvr/hdpvr-video.c | 40 +++++++++++++++++++++++++++
drivers/media/usb/hdpvr/hdpvr.h | 2 ++
3 files changed, 50 insertions(+)
diff --git a/drivers/media/usb/hdpvr/hdpvr-core.c b/drivers/media/usb/hdpvr/hdpvr-core.c
index fd7608e7e94c..b7ac63113ac0 100644
--- a/drivers/media/usb/hdpvr/hdpvr-core.c
+++ b/drivers/media/usb/hdpvr/hdpvr-core.c
@@ -43,6 +43,14 @@ 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");
+uint hdpvr_restart_streaming_max_tries = 1;
+module_param(hdpvr_restart_streaming_max_tries, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(hdpvr_restart_streaming_max_tries, "restart streaming at most this many times within one read");
+
+uint hdpvr_restart_streaming_ms_delay = 100;
+module_param(hdpvr_restart_streaming_ms_delay, uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(hdpvr_restart_streaming_ms_delay, "delay continue by the specified number of milliseconds after restarting streaming");
+
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 8a2b883d372e..e2ca5d955f4a 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -441,6 +441,8 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
struct hdpvr_buffer *buf = NULL;
struct urb *urb;
unsigned int ret = 0;
+ unsigned int restarts_remaining = hdpvr_restart_streaming_max_tries;
+ unsigned int delay;
int rem, cnt;
if (*pos)
@@ -491,6 +493,20 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
goto err;
}
if (!err) {
+ if (restarts_remaining == 0) {
+ v4l2_dbg(MSG_BUFFER, hdpvr_debug,
+ &dev->v4l2_dev,
+ "timeout: no further restarts allowed by hdpvr_restart_streaming_max_tries; returning to caller with ret=%u",
+ ret);
+ /* This break will return the
+ * count of bytes copied so far,
+ * which may be 0. In that
+ * situation, the user
+ * application will get an EOF.
+ */
+ break;
+ }
+ --restarts_remaining;
v4l2_info(&dev->v4l2_dev,
"timeout: restart streaming\n");
mutex_lock(&dev->io_mutex);
@@ -501,6 +517,30 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
ret = err;
goto err;
}
+ /* hdpvr_start_streaming instructs the
+ * device to stream, but the device is
+ * usually not ready by the time
+ * hdpvr_start_streaming returns.
+ *
+ * Without this continue, the loop would
+ * terminate. If no data had been
+ * copied by a prior iteration of the
+ * loop, then hdpvr_read would return 0,
+ * closing the file descriptor
+ * prematurely. Continue back to the
+ * top of the loop to avoid that.
+ *
+ * The device may not be ready within 1
+ * second, so the
+ * wait_event_interruptible_timeout
+ * would then restart streaming a second
+ * time. Delay here to give the device
+ * time to stabilize first.
+ */
+ delay = hdpvr_restart_streaming_ms_delay;
+ if (delay)
+ msleep(delay);
+ continue;
}
}
diff --git a/drivers/media/usb/hdpvr/hdpvr.h b/drivers/media/usb/hdpvr/hdpvr.h
index 9e5f88146827..b1568adca7f0 100644
--- a/drivers/media/usb/hdpvr/hdpvr.h
+++ b/drivers/media/usb/hdpvr/hdpvr.h
@@ -44,6 +44,8 @@
extern int hdpvr_debug;
extern uint hdpvr_close_to_open_ms_delay;
+extern uint hdpvr_restart_streaming_max_tries;
+extern uint hdpvr_restart_streaming_ms_delay;
#define MSG_INFO 1
#define MSG_BUFFER 2
--
2.22.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2]: media: hdpvr: Add optional restart, with optional delay, after restarting streaming
2019-07-07 21:16 [PATCH 2/2]: media: hdpvr: Add optional restart, with optional delay, after restarting streaming Keith Pyle
@ 2019-07-25 7:17 ` Hans Verkuil
0 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2019-07-25 7:17 UTC (permalink / raw)
To: Keith Pyle, Linux Media Mailing List
On 7/7/19 11:16 PM, Keith Pyle wrote:
> `hdpvr_read` attempts to restart streaming if the device is read while
> it is both not ready and not disconnected. However, the device is often
> still not ready immediately after the call to `hdpvr_start_streaming`
> returns, causing the condition `if (buf->status != BUFSTAT_READY)` to
> exit the loop without reading any further data. By itself, this would
> merely cause a short read, which should be easily recoverable. However,
> if no data has been read so far, this causes `hdpvr_read` to return 0,
> which results in an end-of-file for the user application.
>
> Compensate for this by adding the ability to delay after the call to
> `hdpvr_start_streaming`, then `continue;` back to the top, so that
> `hdpvr_read` can call `wait_event_interruptible_timeout` again to wait
> for the device to become ready. This delay complements the prior patch.
> The prior patch delays before issuing the start-streaming command, to
> give the firmware time to stabilize before receiving the command. This
> delay is after the start-streaming command, to give the firmware time to
> bring the device to a ready state. This delay is configurable through a
> new module parameter, `hdpvr_restart_streaming_ms_delay`, which defaults
> to a 100 millisecond delay.
>
> To avoid an infinite loop in `hdpvr_read`, add a limit to how many times
> `hdpvr_read` can restart the device before returning. This limit is
> configurable through a new module parameter,
> `hdpvr_restart_streaming_max_tries`, and defaults to one restart.
> Administrators may set the limit to 0 to request that `hdpvr_read` never
> attempt to restart streaming. Previously, there was no way for
> administrators to opt out of an attempted restart.
>
> Signed-off-by: Keith Pyle <kpyle@austin.rr.com>
> Tested-by: Keith Pyle <kpyle@austin.rr.com>
> ---
> 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.
> ---
> drivers/media/usb/hdpvr/hdpvr-core.c | 8 ++++++
> drivers/media/usb/hdpvr/hdpvr-video.c | 40 +++++++++++++++++++++++++++
> drivers/media/usb/hdpvr/hdpvr.h | 2 ++
> 3 files changed, 50 insertions(+)
>
> diff --git a/drivers/media/usb/hdpvr/hdpvr-core.c b/drivers/media/usb/hdpvr/hdpvr-core.c
> index fd7608e7e94c..b7ac63113ac0 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-core.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-core.c
> @@ -43,6 +43,14 @@ 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");
>
> +uint hdpvr_restart_streaming_max_tries = 1;
> +module_param(hdpvr_restart_streaming_max_tries, uint, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(hdpvr_restart_streaming_max_tries, "restart streaming at most this many times within one read");
> +
> +uint hdpvr_restart_streaming_ms_delay = 100;
> +module_param(hdpvr_restart_streaming_ms_delay, uint, S_IRUGO|S_IWUSR);
> +MODULE_PARM_DESC(hdpvr_restart_streaming_ms_delay, "delay continue by the specified number of milliseconds after restarting streaming");
> +
> 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 8a2b883d372e..e2ca5d955f4a 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-video.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-video.c
> @@ -441,6 +441,8 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> struct hdpvr_buffer *buf = NULL;
> struct urb *urb;
> unsigned int ret = 0;
> + unsigned int restarts_remaining = hdpvr_restart_streaming_max_tries;
> + unsigned int delay;
> int rem, cnt;
>
> if (*pos)
> @@ -491,6 +493,20 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> goto err;
> }
> if (!err) {
> + if (restarts_remaining == 0) {
> + v4l2_dbg(MSG_BUFFER, hdpvr_debug,
> + &dev->v4l2_dev,
> + "timeout: no further restarts allowed by hdpvr_restart_streaming_max_tries; returning to caller with ret=%u",
> + ret);
> + /* This break will return the
> + * count of bytes copied so far,
> + * which may be 0. In that
> + * situation, the user
> + * application will get an EOF.
The comment blocks in this patch are ugly. There is more room to the right, so
fill each line until column 80. So in this example:
/*
* This break will return the count of
* bytes copied so far, which may be 0.
* In that situation, the user
* application will get an EOF.
*/
> + */
> + break;
> + }
> + --restarts_remaining;
This isn't wrong, but the kernel style is: restarts_remaining--;
> v4l2_info(&dev->v4l2_dev,
> "timeout: restart streaming\n");
> mutex_lock(&dev->io_mutex);
> @@ -501,6 +517,30 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> ret = err;
> goto err;
> }
> + /* hdpvr_start_streaming instructs the
> + * device to stream, but the device is
> + * usually not ready by the time
> + * hdpvr_start_streaming returns.
> + *
> + * Without this continue, the loop would
> + * terminate. If no data had been
> + * copied by a prior iteration of the
> + * loop, then hdpvr_read would return 0,
> + * closing the file descriptor
> + * prematurely. Continue back to the
> + * top of the loop to avoid that.
> + *
> + * The device may not be ready within 1
> + * second, so the
> + * wait_event_interruptible_timeout
> + * would then restart streaming a second
> + * time. Delay here to give the device
> + * time to stabilize first.
> + */
> + delay = hdpvr_restart_streaming_ms_delay;
> + if (delay)
> + msleep(delay);
> + continue;
> }
I wonder if it wouldn't be better to move the code inside the 'if (!err) {' to
a separate function. It's getting hard to read.
Regards,
Hans
> }
>
> diff --git a/drivers/media/usb/hdpvr/hdpvr.h b/drivers/media/usb/hdpvr/hdpvr.h
> index 9e5f88146827..b1568adca7f0 100644
> --- a/drivers/media/usb/hdpvr/hdpvr.h
> +++ b/drivers/media/usb/hdpvr/hdpvr.h
> @@ -44,6 +44,8 @@
>
> extern int hdpvr_debug;
> extern uint hdpvr_close_to_open_ms_delay;
> +extern uint hdpvr_restart_streaming_max_tries;
> +extern uint hdpvr_restart_streaming_ms_delay;
>
> #define MSG_INFO 1
> #define MSG_BUFFER 2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2]: media: hdpvr: Add optional restart, with optional delay, after restarting streaming
2019-08-03 22:09 Keith Pyle
@ 2019-08-07 10:00 ` Hans Verkuil
0 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2019-08-07 10:00 UTC (permalink / raw)
To: Keith Pyle, Linux Media Mailing List
On 8/4/19 12:09 AM, Keith Pyle wrote:
> `hdpvr_read` attempts to restart streaming if the device is read while
> it is both not ready and not disconnected. However, the device is often
> still not ready immediately after the call to `hdpvr_start_streaming`
> returns, causing the condition `if (buf->status != BUFSTAT_READY)` to
> exit the loop without reading any further data. By itself, this would
> merely cause a short read, which should be easily recoverable. However,
> if no data has been read so far, this causes `hdpvr_read` to return 0,
> which results in an end-of-file for the user application.
>
> Compensate for this by adding the ability to delay after the call to
> `hdpvr_start_streaming`, then `continue;` back to the top, so that
> `hdpvr_read` can call `wait_event_interruptible_timeout` again to wait
> for the device to become ready. This delay complements the prior patch.
> The prior patch delays before issuing the start-streaming command, to
> give the firmware time to stabilize before receiving the command. This
> delay is after the start-streaming command, to give the firmware time to
> bring the device to a ready state. This delay is configurable through a
> new module parameter, `hdpvr_restart_streaming_ms_delay`, which defaults
> to a 100 millisecond delay.
>
> To avoid an infinite loop in `hdpvr_read`, add a limit to how many times
> `hdpvr_read` can restart the device before returning. This limit is
> configurable through a new module parameter,
> `hdpvr_restart_streaming_max_tries`, and defaults to one restart.
> Administrators may set the limit to 0 to request that `hdpvr_read` never
> attempt to restart streaming. Previously, there was no way for
> administrators to opt out of an attempted restart.
>
> Signed-off-by: Keith Pyle <kpyle@austin.rr.com>
> Tested-by: Keith Pyle <kpyle@austin.rr.com>
> ---
> 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.
> - Per request from Hans, switched from pre-decrement to post-decrement.
> 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.
> ---
> drivers/media/usb/hdpvr/hdpvr-core.c | 8 ++++++
> drivers/media/usb/hdpvr/hdpvr-video.c | 36 +++++++++++++++++++++++++++
> drivers/media/usb/hdpvr/hdpvr.h | 2 ++
> 3 files changed, 46 insertions(+)
>
> diff --git a/drivers/media/usb/hdpvr/hdpvr-core.c b/drivers/media/usb/hdpvr/hdpvr-core.c
> index a3d2f632fe38..1be3911e43ed 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-core.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-core.c
> @@ -43,6 +43,14 @@ uint hdpvr_close_to_open_ms_delay = 4000;
> module_param(hdpvr_close_to_open_ms_delay, uint, S_IRUGO | S_IWUSR);
As per checkpatch output, use octal here.
> MODULE_PARM_DESC(hdpvr_close_to_open_ms_delay, "delay restarting streaming by the specified number of milliseconds");
>
> +uint hdpvr_restart_streaming_max_tries = 1;
> +module_param(hdpvr_restart_streaming_max_tries, uint, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(hdpvr_restart_streaming_max_tries, "restart streaming at most this many times within one read");
> +
> +uint hdpvr_restart_streaming_ms_delay = 100;
> +module_param(hdpvr_restart_streaming_ms_delay, uint, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(hdpvr_restart_streaming_ms_delay, "delay continue by the specified number of milliseconds after restarting streaming");
> +
> 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 7e5897dd8dff..aa7b473b501b 100644
> --- a/drivers/media/usb/hdpvr/hdpvr-video.c
> +++ b/drivers/media/usb/hdpvr/hdpvr-video.c
> @@ -441,6 +441,8 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> struct hdpvr_buffer *buf = NULL;
> struct urb *urb;
> unsigned int ret = 0;
> + unsigned int restarts_remaining = hdpvr_restart_streaming_max_tries;
> + unsigned int delay;
> int rem, cnt;
>
> if (*pos)
> @@ -491,6 +493,19 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> goto err;
> }
> if (!err) {
> + if (restarts_remaining == 0) {
> + v4l2_dbg(MSG_BUFFER, hdpvr_debug,
> + &dev->v4l2_dev,
> + "timeout: no further restarts allowed by hdpvr_restart_streaming_max_tries; returning to caller with ret=%u",
> + ret);
> + /* This break will return the count of
As per coding style, put /* on a line by itself.
Besides these two points this patch looks good.
Regards,
Hans
> + * bytes copied so far, which may be 0.
> + * In that situation, the user
> + * application will get an EOF.
> + */
> + break;
> + }
> + restarts_remaining--;
> v4l2_info(&dev->v4l2_dev,
> "timeout: restart streaming\n");
> mutex_lock(&dev->io_mutex);
> @@ -501,6 +516,27 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
> ret = err;
> goto err;
> }
> + /* hdpvr_start_streaming instructs the device to
> + * stream, but the device is usually not ready
> + * by the time hdpvr_start_streaming returns.
> + *
> + * Without this continue, the loop would
> + * terminate. If no data had been copied by a
> + * prior iteration of the loop, then hdpvr_read
> + * would return 0, closing the file descriptor
> + * prematurely. Continue back to the top of the
> + * loop to avoid that.
> + *
> + * The device may not be ready within 1 second,
> + * so the wait_event_interruptible_timeout would
> + * then restart streaming a second time. Delay
> + * here to give the device time to stabilize
> + * first.
> + */
> + delay = hdpvr_restart_streaming_ms_delay;
> + if (delay)
> + msleep(delay);
> + continue;
> }
> }
>
> diff --git a/drivers/media/usb/hdpvr/hdpvr.h b/drivers/media/usb/hdpvr/hdpvr.h
> index 32498b7120aa..f6f9ddf89faf 100644
> --- a/drivers/media/usb/hdpvr/hdpvr.h
> +++ b/drivers/media/usb/hdpvr/hdpvr.h
> @@ -44,6 +44,8 @@
>
> extern int hdpvr_debug;
> extern uint hdpvr_close_to_open_ms_delay;
> +extern uint hdpvr_restart_streaming_max_tries;
> +extern uint hdpvr_restart_streaming_ms_delay;
>
> #define MSG_INFO 1
> #define MSG_BUFFER 2
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/2]: media: hdpvr: Add optional restart, with optional delay, after restarting streaming
@ 2019-08-03 22:09 Keith Pyle
2019-08-07 10:00 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Keith Pyle @ 2019-08-03 22:09 UTC (permalink / raw)
To: Linux Media Mailing List
`hdpvr_read` attempts to restart streaming if the device is read while
it is both not ready and not disconnected. However, the device is often
still not ready immediately after the call to `hdpvr_start_streaming`
returns, causing the condition `if (buf->status != BUFSTAT_READY)` to
exit the loop without reading any further data. By itself, this would
merely cause a short read, which should be easily recoverable. However,
if no data has been read so far, this causes `hdpvr_read` to return 0,
which results in an end-of-file for the user application.
Compensate for this by adding the ability to delay after the call to
`hdpvr_start_streaming`, then `continue;` back to the top, so that
`hdpvr_read` can call `wait_event_interruptible_timeout` again to wait
for the device to become ready. This delay complements the prior patch.
The prior patch delays before issuing the start-streaming command, to
give the firmware time to stabilize before receiving the command. This
delay is after the start-streaming command, to give the firmware time to
bring the device to a ready state. This delay is configurable through a
new module parameter, `hdpvr_restart_streaming_ms_delay`, which defaults
to a 100 millisecond delay.
To avoid an infinite loop in `hdpvr_read`, add a limit to how many times
`hdpvr_read` can restart the device before returning. This limit is
configurable through a new module parameter,
`hdpvr_restart_streaming_max_tries`, and defaults to one restart.
Administrators may set the limit to 0 to request that `hdpvr_read` never
attempt to restart streaming. Previously, there was no way for
administrators to opt out of an attempted restart.
Signed-off-by: Keith Pyle <kpyle@austin.rr.com>
Tested-by: Keith Pyle <kpyle@austin.rr.com>
---
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.
- Per request from Hans, switched from pre-decrement to post-decrement.
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.
---
drivers/media/usb/hdpvr/hdpvr-core.c | 8 ++++++
drivers/media/usb/hdpvr/hdpvr-video.c | 36 +++++++++++++++++++++++++++
drivers/media/usb/hdpvr/hdpvr.h | 2 ++
3 files changed, 46 insertions(+)
diff --git a/drivers/media/usb/hdpvr/hdpvr-core.c b/drivers/media/usb/hdpvr/hdpvr-core.c
index a3d2f632fe38..1be3911e43ed 100644
--- a/drivers/media/usb/hdpvr/hdpvr-core.c
+++ b/drivers/media/usb/hdpvr/hdpvr-core.c
@@ -43,6 +43,14 @@ 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");
+uint hdpvr_restart_streaming_max_tries = 1;
+module_param(hdpvr_restart_streaming_max_tries, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(hdpvr_restart_streaming_max_tries, "restart streaming at most this many times within one read");
+
+uint hdpvr_restart_streaming_ms_delay = 100;
+module_param(hdpvr_restart_streaming_ms_delay, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(hdpvr_restart_streaming_ms_delay, "delay continue by the specified number of milliseconds after restarting streaming");
+
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 7e5897dd8dff..aa7b473b501b 100644
--- a/drivers/media/usb/hdpvr/hdpvr-video.c
+++ b/drivers/media/usb/hdpvr/hdpvr-video.c
@@ -441,6 +441,8 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
struct hdpvr_buffer *buf = NULL;
struct urb *urb;
unsigned int ret = 0;
+ unsigned int restarts_remaining = hdpvr_restart_streaming_max_tries;
+ unsigned int delay;
int rem, cnt;
if (*pos)
@@ -491,6 +493,19 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
goto err;
}
if (!err) {
+ if (restarts_remaining == 0) {
+ v4l2_dbg(MSG_BUFFER, hdpvr_debug,
+ &dev->v4l2_dev,
+ "timeout: no further restarts allowed by hdpvr_restart_streaming_max_tries; returning to caller with ret=%u",
+ ret);
+ /* This break will return the count of
+ * bytes copied so far, which may be 0.
+ * In that situation, the user
+ * application will get an EOF.
+ */
+ break;
+ }
+ restarts_remaining--;
v4l2_info(&dev->v4l2_dev,
"timeout: restart streaming\n");
mutex_lock(&dev->io_mutex);
@@ -501,6 +516,27 @@ static ssize_t hdpvr_read(struct file *file, char __user *buffer, size_t count,
ret = err;
goto err;
}
+ /* hdpvr_start_streaming instructs the device to
+ * stream, but the device is usually not ready
+ * by the time hdpvr_start_streaming returns.
+ *
+ * Without this continue, the loop would
+ * terminate. If no data had been copied by a
+ * prior iteration of the loop, then hdpvr_read
+ * would return 0, closing the file descriptor
+ * prematurely. Continue back to the top of the
+ * loop to avoid that.
+ *
+ * The device may not be ready within 1 second,
+ * so the wait_event_interruptible_timeout would
+ * then restart streaming a second time. Delay
+ * here to give the device time to stabilize
+ * first.
+ */
+ delay = hdpvr_restart_streaming_ms_delay;
+ if (delay)
+ msleep(delay);
+ continue;
}
}
diff --git a/drivers/media/usb/hdpvr/hdpvr.h b/drivers/media/usb/hdpvr/hdpvr.h
index 32498b7120aa..f6f9ddf89faf 100644
--- a/drivers/media/usb/hdpvr/hdpvr.h
+++ b/drivers/media/usb/hdpvr/hdpvr.h
@@ -44,6 +44,8 @@
extern int hdpvr_debug;
extern uint hdpvr_close_to_open_ms_delay;
+extern uint hdpvr_restart_streaming_max_tries;
+extern uint hdpvr_restart_streaming_ms_delay;
#define MSG_INFO 1
#define MSG_BUFFER 2
--
2.22.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-07 10:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-07 21:16 [PATCH 2/2]: media: hdpvr: Add optional restart, with optional delay, after restarting streaming Keith Pyle
2019-07-25 7:17 ` Hans Verkuil
2019-08-03 22:09 Keith Pyle
2019-08-07 10:00 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).