[PATCH] drm/vblank: Extract get_vblank_counter_and_timestamp()

[PATCH] drm/vblank: Extract get_vblank_counter_and_timestamp()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have three copies of the "read vblank counter and timestamp
in a loop" code. Consolidate to a single a function.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f78bf37f1e0a..f90fb2d13e42 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -236,6 +236,21 @@ static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
 	return drm_vblank_no_hw_counter(dev, pipe);
 }
 
+static bool get_vblank_counter_and_timestamp(struct drm_device *dev, unsigned int pipe,
+					     u32 *cur_vblank, ktime_t *t_vblank,
+					     bool in_vblank_irq)
+{
+	int count = DRM_TIMESTAMP_MAXRETRIES;
+	bool rc;
+
+	do {
+		*cur_vblank = __get_vblank_counter(dev, pipe);
+		rc = drm_get_last_vbltimestamp(dev, pipe, t_vblank, in_vblank_irq);
+	} while (*cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
+
+	return rc;
+}
+
 /*
  * Reset the stored timestamp for the current vblank count to correspond
  * to the last vblank occurred.
@@ -250,7 +265,6 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
 	u32 cur_vblank;
 	bool rc;
 	ktime_t t_vblank;
-	int count = DRM_TIMESTAMP_MAXRETRIES;
 
 	spin_lock(&dev->vblank_time_lock);
 
@@ -258,10 +272,8 @@ static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe
 	 * sample the current counter to avoid random jumps
 	 * when drm_vblank_enable() applies the diff
 	 */
-	do {
-		cur_vblank = __get_vblank_counter(dev, pipe);
-		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
-	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
+	rc = get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
+					      &t_vblank, false);
 
 	/*
 	 * Only reinitialize corresponding vblank timestamp if high-precision query
@@ -299,7 +311,6 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	u32 cur_vblank, diff;
 	bool rc;
 	ktime_t t_vblank;
-	int count = DRM_TIMESTAMP_MAXRETRIES;
 	int framedur_ns = vblank->framedur_ns;
 	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
 
@@ -315,10 +326,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
 	 * updating its hardware counter while we are retrieving the
 	 * corresponding vblank timestamp.
 	 */
-	do {
-		cur_vblank = __get_vblank_counter(dev, pipe);
-		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
-	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
+	rc = get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
+					      &t_vblank, in_vblank_irq);
 
 	if (max_vblank_count) {
 		/* trust the hw counter when it's around */
@@ -1543,7 +1552,6 @@ static void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
 	int framedur_ns;
 	u64 diff_ns;
 	u32 cur_vblank, diff = 1;
-	int count = DRM_TIMESTAMP_MAXRETRIES;
 	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
 
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
@@ -1558,10 +1566,8 @@ static void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
 		      "Cannot compute missed vblanks without frame duration\n");
 	framedur_ns = vblank->framedur_ns;
 
-	do {
-		cur_vblank = __get_vblank_counter(dev, pipe);
-		drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
-	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
+	get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
+					 &t_vblank, false);
 
 	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
 	if (framedur_ns)
-- 
2.52.0

RE: [PATCH] drm/vblank: Extract get_vblank_counter_and_timestamp()

[Kandpal, Suraj]

> Subject: [PATCH] drm/vblank: Extract get_vblank_counter_and_timestamp()
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We have three copies of the "read vblank counter and timestamp in a loop"
> code. Consolidate to a single a function.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

LGTM,
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>

> ---
>  drivers/gpu/drm/drm_vblank.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f78bf37f1e0a..f90fb2d13e42 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -236,6 +236,21 @@ static u32 __get_vblank_counter(struct drm_device
> *dev, unsigned int pipe)
>  	return drm_vblank_no_hw_counter(dev, pipe);  }
> 
> +static bool get_vblank_counter_and_timestamp(struct drm_device *dev,
> unsigned int pipe,
> +					     u32 *cur_vblank, ktime_t
> *t_vblank,
> +					     bool in_vblank_irq)
> +{
> +	int count = DRM_TIMESTAMP_MAXRETRIES;
> +	bool rc;
> +
> +	do {
> +		*cur_vblank = __get_vblank_counter(dev, pipe);
> +		rc = drm_get_last_vbltimestamp(dev, pipe, t_vblank,
> in_vblank_irq);
> +	} while (*cur_vblank != __get_vblank_counter(dev, pipe) && --count >
> +0);
> +
> +	return rc;
> +}
> +
>  /*
>   * Reset the stored timestamp for the current vblank count to correspond
>   * to the last vblank occurred.
> @@ -250,7 +265,6 @@ static void drm_reset_vblank_timestamp(struct
> drm_device *dev, unsigned int pipe
>  	u32 cur_vblank;
>  	bool rc;
>  	ktime_t t_vblank;
> -	int count = DRM_TIMESTAMP_MAXRETRIES;
> 
>  	spin_lock(&dev->vblank_time_lock);
> 
> @@ -258,10 +272,8 @@ static void drm_reset_vblank_timestamp(struct
> drm_device *dev, unsigned int pipe
>  	 * sample the current counter to avoid random jumps
>  	 * when drm_vblank_enable() applies the diff
>  	 */
> -	do {
> -		cur_vblank = __get_vblank_counter(dev, pipe);
> -		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
> -	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count >
> 0);
> +	rc = get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
> +					      &t_vblank, false);
> 
>  	/*
>  	 * Only reinitialize corresponding vblank timestamp if high-precision
> query @@ -299,7 +311,6 @@ static void drm_update_vblank_count(struct
> drm_device *dev, unsigned int pipe,
>  	u32 cur_vblank, diff;
>  	bool rc;
>  	ktime_t t_vblank;
> -	int count = DRM_TIMESTAMP_MAXRETRIES;
>  	int framedur_ns = vblank->framedur_ns;
>  	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> 
> @@ -315,10 +326,8 @@ static void drm_update_vblank_count(struct
> drm_device *dev, unsigned int pipe,
>  	 * updating its hardware counter while we are retrieving the
>  	 * corresponding vblank timestamp.
>  	 */
> -	do {
> -		cur_vblank = __get_vblank_counter(dev, pipe);
> -		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank,
> in_vblank_irq);
> -	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count >
> 0);
> +	rc = get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
> +					      &t_vblank, in_vblank_irq);
> 
>  	if (max_vblank_count) {
>  		/* trust the hw counter when it's around */ @@ -1543,7
> +1552,6 @@ static void drm_vblank_restore(struct drm_device *dev, unsigned
> int pipe)
>  	int framedur_ns;
>  	u64 diff_ns;
>  	u32 cur_vblank, diff = 1;
> -	int count = DRM_TIMESTAMP_MAXRETRIES;
>  	u32 max_vblank_count = drm_max_vblank_count(dev, pipe);
> 
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs)) @@ -1558,10
> +1566,8 @@ static void drm_vblank_restore(struct drm_device *dev, unsigned
> int pipe)
>  		      "Cannot compute missed vblanks without frame
> duration\n");
>  	framedur_ns = vblank->framedur_ns;
> 
> -	do {
> -		cur_vblank = __get_vblank_counter(dev, pipe);
> -		drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
> -	} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count >
> 0);
> +	get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
> +					 &t_vblank, false);
> 
>  	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
>  	if (framedur_ns)
> --
> 2.52.0

Claude review: drm/vblank: Extract get_vblank_counter_and_timestamp()

[Claude Code Review Bot]

Overall Series Review

Subject: drm/vblank: Extract get_vblank_counter_and_timestamp()
Author: Ville Syrjala <ville.syrjala@linux.intel.com>
Patches: 2
Reviewed: 2026-03-25T07:15:59.433526

---

This is a single-patch cleanup that extracts a repeated "read vblank counter and timestamp in a retry loop" pattern from three call sites in `drm_vblank.c` into a new helper function `get_vblank_counter_and_timestamp()`. The refactoring is straightforward, correct, and reduces code duplication. There is one minor semantic difference worth noting but it is harmless.

**Verdict: Looks good.** Clean, simple refactoring.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/vblank: Extract get_vblank_counter_and_timestamp()

[Claude Code Review Bot]

Patch Review

**Correctness:**

The three original do/while loops are identical in structure:
```c
do {
    cur_vblank = __get_vblank_counter(dev, pipe);
    rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, in_vblank_irq);
} while (cur_vblank != __get_vblank_counter(dev, pipe) && --count > 0);
```

The new helper faithfully reproduces this pattern. Two of the three call sites (`drm_reset_vblank_timestamp` and `drm_update_vblank_count`) used the return value (`rc`) — the refactored code preserves this.

**Minor observation:** In `drm_vblank_restore()` (line 1563 in the original), the return value of `drm_get_last_vbltimestamp()` was **not** captured:
```c
drm_get_last_vbltimestamp(dev, pipe, &t_vblank, false);
```
The new code now captures it via the helper's `rc` return, but the caller discards the return value:
```c
get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
                                 &t_vblank, false);
```
This is harmless — the return value is simply ignored — but it does mean the refactoring subtly changes the code from "never captured" to "captured then discarded." No functional impact.

**Style:** The helper is well-placed (right after `__get_vblank_counter()`), the naming is descriptive, and the parameter list matches the existing patterns in the file.

**Reviewed-by worthy:** Yes, this is a clean extraction with no behavioral changes.

---
Generated by Claude Code Patch Reviewer