From patchwork Sun Jul 7 21:15:49 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Pyle X-Patchwork-Id: 57459 X-Patchwork-Delegate: hverkuil@xs4all.nl Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hkEVn-0007xd-QL; Sun, 07 Jul 2019 21:15:56 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727638AbfGGVPy (ORCPT + 1 other); Sun, 7 Jul 2019 17:15:54 -0400 Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.225]:19695 "EHLO cdptpa-cmomta01.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727636AbfGGVPy (ORCPT ); Sun, 7 Jul 2019 17:15:54 -0400 Received: from [192.168.2.97] ([72.182.16.184]) by cmsmtp with ESMTP id kEVhhVAj2E13vkEVjhW2Wi; Sun, 07 Jul 2019 21:15:52 +0000 To: Hans Verkuil , Linux Media Mailing List From: Keith Pyle Subject: [PATCH 1/2]: media: hdpvr: Add adaptive sleeping in hdpvr_start_streaming Message-ID: Date: Sun, 7 Jul 2019 16:15:49 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.0 MIME-Version: 1.0 Content-Language: en-US X-CMAE-Envelope: MS4wfBnywf9417CCjH10hocprlNSb+EKZofVm7Zh3kaHkXizB0CLcIO3lBmnziw2Qa3yF5gz/jeC9WmbCntALVjakFfJemD3wSFciCnZKQ/BbTha208nSikf tCxI2elFnRMG1oeA8VkO/9G3+OWGrgqsmsXg5LBsQPqygRsTZE7QMhVQnQGErxWvTgd8BkLVPr0pklUXrhMt4yCfpW9Z4oEqKm8= Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org 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 Tested-by: Keith Pyle --- 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()`. 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 + * start streaming + */ + u64 jiffies_next_start_streaming; /* synchronize I/O */ struct mutex io_mutex; From patchwork Sun Jul 7 21:16:00 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Pyle X-Patchwork-Id: 57460 X-Patchwork-Delegate: hverkuil@xs4all.nl Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hkEVz-0007xd-AI; Sun, 07 Jul 2019 21:16:07 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727645AbfGGVQG (ORCPT + 1 other); Sun, 7 Jul 2019 17:16:06 -0400 Received: from cdptpa-outbound-snat.email.rr.com ([107.14.166.230]:17327 "EHLO cdptpa-cmomta02.email.rr.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727636AbfGGVQG (ORCPT ); Sun, 7 Jul 2019 17:16:06 -0400 Received: from [192.168.2.97] ([72.182.16.184]) by cmsmtp with ESMTP id kEVshGg0P3JQGkEVvh6H5s; Sun, 07 Jul 2019 21:16:03 +0000 To: Hans Verkuil , Linux Media Mailing List From: Keith Pyle Subject: [PATCH 2/2]: media: hdpvr: Add optional restart, with optional delay, after restarting streaming Message-ID: Date: Sun, 7 Jul 2019 16:16:00 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.0 MIME-Version: 1.0 Content-Language: en-US X-CMAE-Envelope: MS4wfCxkdka6JNkU3XGXbBZmmszJuiYH+IHvRJiajkIl7kWkmO0ihi6FY9/vq8F5zLuiGsNw4wl0tNNGFXWQIxMWa0HYTvNkYVdRHxx7SCyD7dGeojmTfxp/ HQ0piK+25WLM6Ikf8HTJe/VR3QegIagS9+xwCJtj/PRXWdwOFSLM4dQ3iBqetU2lPgzOA5RQOiBSzfrYfDpTGTMFHckrAg/AyNo= Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org `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 Tested-by: Keith Pyle --- 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