diff mbox

[6/8,media] vivid: use ktime_t for timestamp calculation

Message ID 20171127132027.1734806-6-arnd@arndb.de (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann Nov. 27, 2017, 1:19 p.m. UTC 
timespec is generally deprecated because of the y2038 overflow.
In vivid, the usage is fine, since we are dealing with monotonic
timestamps, but we can also simplify the code by going to ktime_t.

Using ktime_divns() should be roughly as efficient as the old code,
since the constant 64-bit division gets turned into a multiplication
on modern platforms, and we save multiple 32-bit divisions that can be
expensive e.g. on ARMv7.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I hope I understood the VIVID_RDS_GEN_BLOCKS calculation right,
please review carefully.
---
 drivers/media/platform/vivid/vivid-core.c     |  2 +-
 drivers/media/platform/vivid/vivid-core.h     |  2 +-
 drivers/media/platform/vivid/vivid-radio-rx.c | 11 +++++------
 drivers/media/platform/vivid/vivid-radio-tx.c |  8 +++-----
 drivers/media/platform/vivid/vivid-rds-gen.h  |  1 +
 5 files changed, 11 insertions(+), 13 deletions(-)

Comments

Hans Verkuil Nov. 27, 2017, 3:14 p.m. UTC | #1 
Hi Arnd,

On 11/27/2017 02:19 PM, Arnd Bergmann wrote:
> timespec is generally deprecated because of the y2038 overflow.
> In vivid, the usage is fine, since we are dealing with monotonic
> timestamps, but we can also simplify the code by going to ktime_t.
> 
> Using ktime_divns() should be roughly as efficient as the old code,
> since the constant 64-bit division gets turned into a multiplication
> on modern platforms, and we save multiple 32-bit divisions that can be
> expensive e.g. on ARMv7.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I hope I understood the VIVID_RDS_GEN_BLOCKS calculation right,
> please review carefully.
> ---
>  drivers/media/platform/vivid/vivid-core.c     |  2 +-
>  drivers/media/platform/vivid/vivid-core.h     |  2 +-
>  drivers/media/platform/vivid/vivid-radio-rx.c | 11 +++++------
>  drivers/media/platform/vivid/vivid-radio-tx.c |  8 +++-----
>  drivers/media/platform/vivid/vivid-rds-gen.h  |  1 +
>  5 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
> index 5f316a5e38db..a091cfd93164 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -995,7 +995,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  
>  	dev->edid_max_blocks = dev->edid_blocks = 2;
>  	memcpy(dev->edid, vivid_hdmi_edid, sizeof(vivid_hdmi_edid));
> -	ktime_get_ts(&dev->radio_rds_init_ts);
> +	dev->radio_rds_init_time = ktime_get();
>  
>  	/* create all controls */
>  	ret = vivid_create_controls(dev, ccs_cap == -1, ccs_out == -1, no_error_inj,
> diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
> index 5cdf95bdc4d1..d8bff4dcefb7 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -510,7 +510,7 @@ struct vivid_dev {
>  
>  	/* Shared between radio receiver and transmitter */
>  	bool				radio_rds_loop;
> -	struct timespec			radio_rds_init_ts;
> +	ktime_t				radio_rds_init_time;
>  
>  	/* CEC */
>  	struct cec_adapter		*cec_rx_adap;
> diff --git a/drivers/media/platform/vivid/vivid-radio-rx.c b/drivers/media/platform/vivid/vivid-radio-rx.c
> index 47c36c26096b..1b96cbd7f2ea 100644
> --- a/drivers/media/platform/vivid/vivid-radio-rx.c
> +++ b/drivers/media/platform/vivid/vivid-radio-rx.c
> @@ -38,9 +38,9 @@ ssize_t vivid_radio_rx_read(struct file *file, char __user *buf,
>  			 size_t size, loff_t *offset)
>  {
>  	struct vivid_dev *dev = video_drvdata(file);
> -	struct timespec ts;
>  	struct v4l2_rds_data *data = dev->rds_gen.data;
>  	bool use_alternates;
> +	ktime_t timestamp;
>  	unsigned blk;
>  	int perc;
>  	int i;
> @@ -64,17 +64,16 @@ ssize_t vivid_radio_rx_read(struct file *file, char __user *buf,
>  	}
>  
>  retry:
> -	ktime_get_ts(&ts);
> -	use_alternates = ts.tv_sec % 10 >= 5;
> +	timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
> +	blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
> +	use_alternates = blk % VIVID_RDS_GEN_BLOCKS;
> +

Almost right. This last line should be:

	use_alternates = (blk / VIVID_RDS_GEN_BLOCKS) & 1;

With that in place it works and you can add my:

Tested-by: Hans Verkuil <hans.verkuil@cisco.com>

to this patch.

Regards,

	Hans


>  	if (dev->radio_rx_rds_last_block == 0 ||
>  	    dev->radio_rx_rds_use_alternates != use_alternates) {
>  		dev->radio_rx_rds_use_alternates = use_alternates;
>  		/* Re-init the RDS generator */
>  		vivid_radio_rds_init(dev);
>  	}
> -	ts = timespec_sub(ts, dev->radio_rds_init_ts);
> -	blk = ts.tv_sec * 100 + ts.tv_nsec / 10000000;
> -	blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
>  	if (blk >= dev->radio_rx_rds_last_block + VIVID_RDS_GEN_BLOCKS)
>  		dev->radio_rx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
>  
> diff --git a/drivers/media/platform/vivid/vivid-radio-tx.c b/drivers/media/platform/vivid/vivid-radio-tx.c
> index 0e8025b7b4dd..897b56195ca7 100644
> --- a/drivers/media/platform/vivid/vivid-radio-tx.c
> +++ b/drivers/media/platform/vivid/vivid-radio-tx.c
> @@ -37,7 +37,7 @@ ssize_t vivid_radio_tx_write(struct file *file, const char __user *buf,
>  {
>  	struct vivid_dev *dev = video_drvdata(file);
>  	struct v4l2_rds_data *data = dev->rds_gen.data;
> -	struct timespec ts;
> +	ktime_t timestamp;
>  	unsigned blk;
>  	int i;
>  
> @@ -58,10 +58,8 @@ ssize_t vivid_radio_tx_write(struct file *file, const char __user *buf,
>  	dev->radio_tx_rds_owner = file->private_data;
>  
>  retry:
> -	ktime_get_ts(&ts);
> -	ts = timespec_sub(ts, dev->radio_rds_init_ts);
> -	blk = ts.tv_sec * 100 + ts.tv_nsec / 10000000;
> -	blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
> +	timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
> +	blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
>  	if (blk - VIVID_RDS_GEN_BLOCKS >= dev->radio_tx_rds_last_block)
>  		dev->radio_tx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
>  
> diff --git a/drivers/media/platform/vivid/vivid-rds-gen.h b/drivers/media/platform/vivid/vivid-rds-gen.h
> index eff4bf552ed3..e55e3b22b7ca 100644
> --- a/drivers/media/platform/vivid/vivid-rds-gen.h
> +++ b/drivers/media/platform/vivid/vivid-rds-gen.h
> @@ -29,6 +29,7 @@
>  #define VIVID_RDS_GEN_GROUPS 57
>  #define VIVID_RDS_GEN_BLKS_PER_GRP 4
>  #define VIVID_RDS_GEN_BLOCKS (VIVID_RDS_GEN_BLKS_PER_GRP * VIVID_RDS_GEN_GROUPS)
> +#define VIVID_RDS_NSEC_PER_BLK (u32)(5ull * NSEC_PER_SEC / VIVID_RDS_GEN_BLOCKS)
>  
>  struct vivid_rds_gen {
>  	struct v4l2_rds_data	data[VIVID_RDS_GEN_BLOCKS];
>
Arnd Bergmann Nov. 27, 2017, 3:25 p.m. UTC | #2 
On Mon, Nov 27, 2017 at 4:14 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:

>> -     ktime_get_ts(&ts);
>> -     use_alternates = ts.tv_sec % 10 >= 5;
>> +     timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
>> +     blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
>> +     use_alternates = blk % VIVID_RDS_GEN_BLOCKS;
>> +
>
> Almost right. This last line should be:
>
>         use_alternates = (blk / VIVID_RDS_GEN_BLOCKS) & 1;
>
> With that in place it works and you can add my:
>
> Tested-by: Hans Verkuil <hans.verkuil@cisco.com>

Makes sense. Sending a fixed version now, thanks a lot for testing!

        Arnd
diff mbox

Patch

diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 5f316a5e38db..a091cfd93164 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -995,7 +995,7 @@  static int vivid_create_instance(struct platform_device *pdev, int inst)
 
 	dev->edid_max_blocks = dev->edid_blocks = 2;
 	memcpy(dev->edid, vivid_hdmi_edid, sizeof(vivid_hdmi_edid));
-	ktime_get_ts(&dev->radio_rds_init_ts);
+	dev->radio_rds_init_time = ktime_get();
 
 	/* create all controls */
 	ret = vivid_create_controls(dev, ccs_cap == -1, ccs_out == -1, no_error_inj,
diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
index 5cdf95bdc4d1..d8bff4dcefb7 100644
--- a/drivers/media/platform/vivid/vivid-core.h
+++ b/drivers/media/platform/vivid/vivid-core.h
@@ -510,7 +510,7 @@  struct vivid_dev {
 
 	/* Shared between radio receiver and transmitter */
 	bool				radio_rds_loop;
-	struct timespec			radio_rds_init_ts;
+	ktime_t				radio_rds_init_time;
 
 	/* CEC */
 	struct cec_adapter		*cec_rx_adap;
diff --git a/drivers/media/platform/vivid/vivid-radio-rx.c b/drivers/media/platform/vivid/vivid-radio-rx.c
index 47c36c26096b..1b96cbd7f2ea 100644
--- a/drivers/media/platform/vivid/vivid-radio-rx.c
+++ b/drivers/media/platform/vivid/vivid-radio-rx.c
@@ -38,9 +38,9 @@  ssize_t vivid_radio_rx_read(struct file *file, char __user *buf,
 			 size_t size, loff_t *offset)
 {
 	struct vivid_dev *dev = video_drvdata(file);
-	struct timespec ts;
 	struct v4l2_rds_data *data = dev->rds_gen.data;
 	bool use_alternates;
+	ktime_t timestamp;
 	unsigned blk;
 	int perc;
 	int i;
@@ -64,17 +64,16 @@  ssize_t vivid_radio_rx_read(struct file *file, char __user *buf,
 	}
 
 retry:
-	ktime_get_ts(&ts);
-	use_alternates = ts.tv_sec % 10 >= 5;
+	timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
+	blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
+	use_alternates = blk % VIVID_RDS_GEN_BLOCKS;
+
 	if (dev->radio_rx_rds_last_block == 0 ||
 	    dev->radio_rx_rds_use_alternates != use_alternates) {
 		dev->radio_rx_rds_use_alternates = use_alternates;
 		/* Re-init the RDS generator */
 		vivid_radio_rds_init(dev);
 	}
-	ts = timespec_sub(ts, dev->radio_rds_init_ts);
-	blk = ts.tv_sec * 100 + ts.tv_nsec / 10000000;
-	blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
 	if (blk >= dev->radio_rx_rds_last_block + VIVID_RDS_GEN_BLOCKS)
 		dev->radio_rx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
 
diff --git a/drivers/media/platform/vivid/vivid-radio-tx.c b/drivers/media/platform/vivid/vivid-radio-tx.c
index 0e8025b7b4dd..897b56195ca7 100644
--- a/drivers/media/platform/vivid/vivid-radio-tx.c
+++ b/drivers/media/platform/vivid/vivid-radio-tx.c
@@ -37,7 +37,7 @@  ssize_t vivid_radio_tx_write(struct file *file, const char __user *buf,
 {
 	struct vivid_dev *dev = video_drvdata(file);
 	struct v4l2_rds_data *data = dev->rds_gen.data;
-	struct timespec ts;
+	ktime_t timestamp;
 	unsigned blk;
 	int i;
 
@@ -58,10 +58,8 @@  ssize_t vivid_radio_tx_write(struct file *file, const char __user *buf,
 	dev->radio_tx_rds_owner = file->private_data;
 
 retry:
-	ktime_get_ts(&ts);
-	ts = timespec_sub(ts, dev->radio_rds_init_ts);
-	blk = ts.tv_sec * 100 + ts.tv_nsec / 10000000;
-	blk = (blk * VIVID_RDS_GEN_BLOCKS) / 500;
+	timestamp = ktime_sub(ktime_get(), dev->radio_rds_init_time);
+	blk = ktime_divns(timestamp, VIVID_RDS_NSEC_PER_BLK);
 	if (blk - VIVID_RDS_GEN_BLOCKS >= dev->radio_tx_rds_last_block)
 		dev->radio_tx_rds_last_block = blk - VIVID_RDS_GEN_BLOCKS + 1;
 
diff --git a/drivers/media/platform/vivid/vivid-rds-gen.h b/drivers/media/platform/vivid/vivid-rds-gen.h
index eff4bf552ed3..e55e3b22b7ca 100644
--- a/drivers/media/platform/vivid/vivid-rds-gen.h
+++ b/drivers/media/platform/vivid/vivid-rds-gen.h
@@ -29,6 +29,7 @@ 
 #define VIVID_RDS_GEN_GROUPS 57
 #define VIVID_RDS_GEN_BLKS_PER_GRP 4
 #define VIVID_RDS_GEN_BLOCKS (VIVID_RDS_GEN_BLKS_PER_GRP * VIVID_RDS_GEN_GROUPS)
+#define VIVID_RDS_NSEC_PER_BLK (u32)(5ull * NSEC_PER_SEC / VIVID_RDS_GEN_BLOCKS)
 
 struct vivid_rds_gen {
 	struct v4l2_rds_data	data[VIVID_RDS_GEN_BLOCKS];