Skip to contentSkip to conversation

drm/sti: Use 64-bit timestamps

PATCH In review Tina Ruchandani 3 participants 4 messages Apr 13, 2016 – Apr 18, 2016

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 drm/sti: Use 64-bit timestamps

'struct timespec' uses a 32-bit field for seconds, which will overflow in year 2038 and beyond. This patch is part of a larger attempt to remove instances of timeval, timespec and time_t, all of which suffer from the y2038 issue, from the kernel.

---
 drivers/gpu/drm/sti/sti_plane.c | 16 +++-------------
 drivers/gpu/drm/sti/sti_plane.h |  2 +-
 2 files changed, 4 insertions(+), 14 deletions(-)
drivers/gpu/drm/sti/sti_plane.c +3 -13
index f10c98d..3b46899 100644
@@ -45,25 +45,15 @@ const char *sti_plane_to_str(struct sti_plane *plane)
 
 #define STI_FPS_INTERVAL_MS     3000
 
-static int sti_plane_timespec_ms_diff(struct timespec lhs, struct timespec rhs)
-{
-	struct timespec tmp_ts = timespec_sub(lhs, rhs);
-	u64 tmp_ns = (u64)timespec_to_ns(&tmp_ts);
-
-	do_div(tmp_ns, NSEC_PER_MSEC);
-
-	return (u32)tmp_ns;
-}
-
 void sti_plane_update_fps(struct sti_plane *plane,
 			  bool new_frame,
 			  bool new_field)
 {
-	struct timespec now;
+	ktime_t now;
 	struct sti_fps_info *fps;
 	int fpks, fipks, ms_since_last, num_frames, num_fields;
 
-	getrawmonotonic(&now);
+	now = ktime_get();
 
 	/* Compute number of frame updates */
 	fps = &plane->fps_info;
@@ -76,7 +66,7 @@ void sti_plane_update_fps(struct sti_plane *plane,
 		return;
 
 	fps->curr_frame_counter++;
-	ms_since_last = sti_plane_timespec_ms_diff(now, fps->last_timestamp);
+	ms_since_last = ktime_to_ms(ktime_sub(now, fps->last_timestamp));
 	num_frames = fps->curr_frame_counter - fps->last_frame_counter;
 
 	if (num_frames <= 0  || ms_since_last < STI_FPS_INTERVAL_MS)
drivers/gpu/drm/sti/sti_plane.h +1 -1
index c50a3b9..0a64eb0 100644
@@ -57,7 +57,7 @@ struct sti_fps_info {
 	unsigned int last_frame_counter;
 	unsigned int curr_field_counter;
 	unsigned int last_field_counter;
-	struct timespec last_timestamp;
+	ktime_t	     last_timestamp;
 	char fps_str[FPS_LENGTH];
 	char fips_str[FPS_LENGTH];
 };

--
2.8.0.rc3.226.g39d4020

_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038

#1 lore raw
Quoted 7 lines · On Wednesday 13 April 2016 02:28:02 Tina Ruchandani wrote:

'struct timespec' uses a 32-bit field for seconds, which will overflow in year 2038 and beyond. This patch is part of a larger attempt to remove instances of timeval, timespec and time_t, all of which suffer from the y2038 issue, from the kernel.

Looks good in principle. Two small points:

drivers/gpu/drm/sti/sti_plane.c const char *sti_plane_to_str(struct sti_plane *plane) · line 56

Quoted 11 lines

void sti_plane_update_fps(struct sti_plane *plane,

drivers/gpu/drm/sti/sti_plane.c +2 -2
 			  bool new_frame,
 			  bool new_field)
 {
-	struct timespec now;
+	ktime_t now;
 	struct sti_fps_info *fps;
 	int fpks, fipks, ms_since_last, num_frames, num_fields;
 
-	getrawmonotonic(&now);
+	now = ktime_get();

It's unclear why the driver was using getrawmonotonic() here rather than ktime_get_ts(). The code is fairly new, so Vincent can probably explain this.

If it was intentional, we should use ktime_get_raw() instead of ktime_get().

drivers/gpu/drm/sti/sti_plane.c void sti_plane_update_fps(struct sti_plane *plane, · line 70

Quoted 7 lines
drivers/gpu/drm/sti/sti_plane.c +1 -1
@@ -76,7 +66,7 @@ void sti_plane_update_fps(struct sti_plane *plane,
 		return;
 
 	fps->curr_frame_counter++;
-	ms_since_last = sti_plane_timespec_ms_diff(now, fps->last_timestamp);
+	ms_since_last = ktime_to_ms(ktime_sub(now, fps->last_timestamp));
 	num_frames = fps->curr_frame_counter - fps->last_frame_counter;

This could be expressed in a more compact way using ktime_ms_delta().

Arnd
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#2 lore raw
Quoted 27 lines · Arnd Bergmann
Quoted 7 lines · On Wednesday 13 April 2016 02:28:02 Tina Ruchandani wrote:

'struct timespec' uses a 32-bit field for seconds, which will overflow in year 2038 and beyond. This patch is part of a larger attempt to remove instances of timeval, timespec and time_t, all of which suffer from the y2038 issue, from the kernel.

Looks good in principle. Two small points:

drivers/gpu/drm/sti/sti_plane.c const char *sti_plane_to_str(struct sti_plane *plane) · line 56

Quoted 11 lines

void sti_plane_update_fps(struct sti_plane *plane,
bool new_frame, bool new_field)
{

drivers/gpu/drm/sti/sti_plane.c +2 -2
-	struct timespec now;
+	ktime_t now;
  	struct sti_fps_info *fps;
  	int fpks, fipks, ms_since_last, num_frames, num_fields;
 
-	getrawmonotonic(&now);
+	now = ktime_get();

It's unclear why the driver was using getrawmonotonic() here rather than ktime_get_ts(). The code is fairly new, so Vincent can probably explain this.

Quoted 3 lines

If it was intentional, we should use ktime_get_raw() instead of ktime_get().

getrawmonotonic comes from a legacy code so the use is not intentional. Honestly, it is not clear to me the difference between monotonic and rawmonotonic. But in the debug context in which it is used, ktime_get and ktime_get_raw will deliver the same level of information we need. So implementation done by Tina is fine for me.

Vincent

Quoted 12 lines

drivers/gpu/drm/sti/sti_plane.c void sti_plane_update_fps(struct sti_plane *plane, · line 70

Quoted 7 lines
drivers/gpu/drm/sti/sti_plane.c +1 -1
@@ -76,7 +66,7 @@ void sti_plane_update_fps(struct sti_plane *plane,
  		return;
 
  	fps->curr_frame_counter++;
-	ms_since_last = sti_plane_timespec_ms_diff(now, fps->last_timestamp);
+	ms_since_last = ktime_to_ms(ktime_sub(now, fps->last_timestamp));
  	num_frames = fps->curr_frame_counter - fps->last_frame_counter;

This could be expressed in a more compact way using ktime_ms_delta().

Arnd

_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038

#3 lore raw
Quoted 7 lines · On Monday 18 April 2016 12:02:35 Vincent ABRIOU wrote:

getrawmonotonic comes from a legacy code so the use is not intentional. Honestly, it is not clear to me the difference between monotonic and rawmonotonic. But in the debug context in which it is used, ktime_get and ktime_get_raw will deliver the same level of information we need. So implementation done by Tina is fine for me.

Ok, cool, thanks for confirming!

FWIW, the best way I can see for illustrating the difference is that rawmonotonic time is for things that should be synchronized with the machines clock generators (e.g. A/V sync), while monotonic time is for the case where you want to synchronize with another machine (or the internet) that may have a slightly different clock generator but uses NTP to correct for that.

In most cases the difference is irrelevant and we tend to use monotonic time by default.

Arnd