Re: [PATCH 1/2]: media: hdpvr: Add adaptive sleeping in hdpvr_start_streaming

[Keith Pyle <kpyle@austin.rr.com>]

On 07/25/19 02:10, Hans Verkuil wrote:
> 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@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.
>> - 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.
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.

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.

>> 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
>