On 8/11/19 7:54 PM, Keith Pyle wrote:
> On 08/07/19 04:58, Hans Verkuil wrote:
>> 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 <[email protected]>
>>> Tested-by: Keith Pyle <[email protected]>
>>> ---
>>> 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.
>
> As I explained on July 26, there is a good reason to use a 64 jiffies: to
> avoid jiffies rollover on a 32-bit
> system. I stated:
>
>> Actually, it isn't. Contrary to your interpretation, we intentionally used
>> the 64 bit value for jiffies on both 32 and 64
>> bit systems since the get_jiffies_64 macro returns a u64 in all cases.
>> Recording systems using HD-PVR devices frequently
>> have long uptimes, so rollover of a 32-bit jiffies value could be a problem
>> (i.e., the necessary delay before a streaming
>> restart would be missing in the event of a rollover). Extra code for
>> rollover detection would be needed.
I think I missed that reply, or just simply forgot it.
Rollover does indeed require more precision. But in that case I prefer that we
switch to
ktime_get() (u64, ns since boot). That's much more common for such situations.
Regards,
Hans
>
>> Also, include/linux/jiffies.h specifically says "The 64-bit value is not
>> atomic - you MUST NOT read it...", so we use the
>> get_jiffies_64 macro as the header recommends. On a 64-bit system, the
>> macro becomes an inline return of jiffies. On a
>> 32-bit system, it becomes an appropriate function call.
>
> I don't see any reason to knowingly introduce a roll-over bug for the 32-bit
> builds, even if it would be a low probability
> issue. Since we continue to disagree on this point, you can consider both of
> these patches withdrawn.
>
>>> 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;
>>>
>>
>
