[PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

[PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

[Ankit Nautiyal]

For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
bits are not required. Since the support for these bits is going to
be deprecated in upcoming platforms, avoid writing these bits for the
platforms that do not use legacy Timing Generator.

Since for these platforms TRAN_VMIN is always filled with crtc_vtotal,
use TRAN_VRR_VMIN to get the vtotal for adjusted_mode.

v2: Avoid having a helper for manipulating VTOTAL register, and instead
just make the change where required. (Ville)

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 41 ++++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_vrr.c     | 15 +++++--
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index bde53b2de70c..37e27dcfda05 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2639,6 +2639,7 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
 	u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
+	u32 vtotal_bits;
 	int vsyncshift = 0;
 
 	drm_WARN_ON(display->drm, !transcoder_has_vrr(cpu_transcoder));
@@ -2695,9 +2696,21 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
 		       HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
 		       HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
 
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not required. Since the support for these bits is going to
+	 * be deprecated in upcoming platforms, avoid writing these bits for the
+	 * platforms that do not use legacy Timing Generator.
+	 */
+	if (intel_vrr_always_use_vrr_tg(display))
+		vtotal_bits = 0;
+	else
+		vtotal_bits = VTOTAL(crtc_vtotal - 1);
+
 	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
 		       VACTIVE(crtc_vdisplay - 1) |
-		       VTOTAL(crtc_vtotal - 1));
+		       vtotal_bits);
+
 	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
 		       VBLANK_START(crtc_vblank_start - 1) |
 		       VBLANK_END(crtc_vblank_end - 1));
@@ -2722,6 +2735,7 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
 	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
 	u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
+	u32 vtotal_bits;
 
 	drm_WARN_ON(display->drm, !transcoder_has_vrr(cpu_transcoder));
 
@@ -2755,13 +2769,24 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
 	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
 		       VBLANK_START(crtc_vblank_start - 1) |
 		       VBLANK_END(crtc_vblank_end - 1));
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not required. Since the support for these bits is going to
+	 * be deprecated in upcoming platforms, avoid writing these bits for the
+	 * platforms that do not use legacy Timing Generator.
+	 */
+	if (intel_vrr_always_use_vrr_tg(display))
+		vtotal_bits = 0;
+	else
+		vtotal_bits = VTOTAL(crtc_vtotal - 1);
+
 	/*
 	 * The double buffer latch point for TRANS_VTOTAL
 	 * is the transcoder's undelayed vblank.
 	 */
 	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
 		       VACTIVE(crtc_vdisplay - 1) |
-		       VTOTAL(crtc_vtotal - 1));
+		       vtotal_bits);
 
 	intel_vrr_set_fixed_rr_timings(crtc_state);
 	intel_vrr_transcoder_enable(crtc_state);
@@ -2824,7 +2849,17 @@ static void intel_get_transcoder_timings(struct intel_crtc *crtc,
 
 	tmp = intel_de_read(display, TRANS_VTOTAL(display, cpu_transcoder));
 	adjusted_mode->crtc_vdisplay = REG_FIELD_GET(VACTIVE_MASK, tmp) + 1;
-	adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
+
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not filled. The value for adjusted_mode->crtc_vtotal is read
+	 * from VRR_VMIN register in intel_vrr_get_config.
+	 * Just set this to 0 here.
+	 */
+	if (intel_vrr_always_use_vrr_tg(display))
+		adjusted_mode->crtc_vtotal = 0;
+	else
+		adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
 
 	/* FIXME TGL+ DSI transcoders have this! */
 	if (!transcoder_is_dsi(cpu_transcoder)) {
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 414f93851059..cace1d7c99d5 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -674,9 +674,19 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
 {
 	struct intel_display *display = to_intel_display(crtc_state);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
-	u32 trans_vrr_ctl, trans_vrr_vsync;
+	u32 trans_vrr_ctl, trans_vrr_vsync, trans_vrr_vmin;
 	bool vrr_enable;
 
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not filled. Since for these platforms TRAN_VMIN is always
+	 * filled with crtc_vtotal, use TRAN_VRR_VMIN to get the vtotal for
+	 * adjusted_mode.
+	 */
+	trans_vrr_vmin = intel_de_read(display, TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
+	if (intel_vrr_always_use_vrr_tg(display))
+		crtc_state->hw.adjusted_mode.crtc_vtotal = trans_vrr_vmin;
+
 	trans_vrr_ctl = intel_de_read(display,
 				      TRANS_VRR_CTL(display, cpu_transcoder));
 
@@ -705,8 +715,7 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
 							 TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1;
 		crtc_state->vrr.vmax = intel_de_read(display,
 						     TRANS_VRR_VMAX(display, cpu_transcoder)) + 1;
-		crtc_state->vrr.vmin = intel_de_read(display,
-						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
+		crtc_state->vrr.vmin = trans_vrr_vmin;
 
 		if (HAS_AS_SDP(display)) {
 			trans_vrr_vsync =
-- 
2.45.2

Re: [PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

[Ville Syrjälä]

On Wed, Mar 26, 2025 at 09:35:38AM +0530, Ankit Nautiyal wrote:
> For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> bits are not required. Since the support for these bits is going to
> be deprecated in upcoming platforms, avoid writing these bits for the
> platforms that do not use legacy Timing Generator.
> 
> Since for these platforms TRAN_VMIN is always filled with crtc_vtotal,
> use TRAN_VRR_VMIN to get the vtotal for adjusted_mode.
> 
> v2: Avoid having a helper for manipulating VTOTAL register, and instead
> just make the change where required. (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 41 ++++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_vrr.c     | 15 +++++--
>  2 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index bde53b2de70c..37e27dcfda05 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2639,6 +2639,7 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>  	u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
> +	u32 vtotal_bits;
>  	int vsyncshift = 0;
>  
>  	drm_WARN_ON(display->drm, !transcoder_has_vrr(cpu_transcoder));
> @@ -2695,9 +2696,21 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
>  		       HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
>  		       HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
>  
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not required. Since the support for these bits is going to
> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
> +	 * platforms that do not use legacy Timing Generator.
> +	 */
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		vtotal_bits = 0;

I think just setting crtc_vtotal=1 here (like we do for crtc_vblank_start)
would take care of this without the need for extra variables.


> +	else
> +		vtotal_bits = VTOTAL(crtc_vtotal - 1);
> +
>  	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
>  		       VACTIVE(crtc_vdisplay - 1) |
> -		       VTOTAL(crtc_vtotal - 1));
> +		       vtotal_bits);
> +
>  	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>  		       VBLANK_START(crtc_vblank_start - 1) |
>  		       VBLANK_END(crtc_vblank_end - 1));
> @@ -2722,6 +2735,7 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>  	u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
> +	u32 vtotal_bits;
>  
>  	drm_WARN_ON(display->drm, !transcoder_has_vrr(cpu_transcoder));
>  
> @@ -2755,13 +2769,24 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
>  	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>  		       VBLANK_START(crtc_vblank_start - 1) |
>  		       VBLANK_END(crtc_vblank_end - 1));
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not required. Since the support for these bits is going to
> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
> +	 * platforms that do not use legacy Timing Generator.
> +	 */
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		vtotal_bits = 0;
> +	else
> +		vtotal_bits = VTOTAL(crtc_vtotal - 1);
> +
>  	/*
>  	 * The double buffer latch point for TRANS_VTOTAL
>  	 * is the transcoder's undelayed vblank.
>  	 */
>  	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
>  		       VACTIVE(crtc_vdisplay - 1) |
> -		       VTOTAL(crtc_vtotal - 1));
> +		       vtotal_bits);
>  
>  	intel_vrr_set_fixed_rr_timings(crtc_state);
>  	intel_vrr_transcoder_enable(crtc_state);
> @@ -2824,7 +2849,17 @@ static void intel_get_transcoder_timings(struct intel_crtc *crtc,
>  
>  	tmp = intel_de_read(display, TRANS_VTOTAL(display, cpu_transcoder));
>  	adjusted_mode->crtc_vdisplay = REG_FIELD_GET(VACTIVE_MASK, tmp) + 1;
> -	adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
> +
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not filled. The value for adjusted_mode->crtc_vtotal is read
> +	 * from VRR_VMIN register in intel_vrr_get_config.
> +	 * Just set this to 0 here.
> +	 */
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		adjusted_mode->crtc_vtotal = 0;
> +	else
> +		adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
>  
>  	/* FIXME TGL+ DSI transcoders have this! */
>  	if (!transcoder_is_dsi(cpu_transcoder)) {
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 414f93851059..cace1d7c99d5 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -674,9 +674,19 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> -	u32 trans_vrr_ctl, trans_vrr_vsync;
> +	u32 trans_vrr_ctl, trans_vrr_vsync, trans_vrr_vmin;
>  	bool vrr_enable;
>  
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not filled. Since for these platforms TRAN_VMIN is always
> +	 * filled with crtc_vtotal, use TRAN_VRR_VMIN to get the vtotal for
> +	 * adjusted_mode.
> +	 */
> +	trans_vrr_vmin = intel_de_read(display, TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		crtc_state->hw.adjusted_mode.crtc_vtotal = trans_vrr_vmin;

I think this should rather use intel_vrr_vmin_vtotal(), and for
reason it needs to be near the end so thaI guess for that
reason it has to be done after the actual vmin readout.

> +
>  	trans_vrr_ctl = intel_de_read(display,
>  				      TRANS_VRR_CTL(display, cpu_transcoder));
>  
> @@ -705,8 +715,7 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>  							 TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1;
>  		crtc_state->vrr.vmax = intel_de_read(display,
>  						     TRANS_VRR_VMAX(display, cpu_transcoder)) + 1;
> -		crtc_state->vrr.vmin = intel_de_read(display,
> -						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
> +		crtc_state->vrr.vmin = trans_vrr_vmin;
>  
>  		if (HAS_AS_SDP(display)) {
>  			trans_vrr_vsync =
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel

[PATCH 0/2] VRR Register Read/Write Updates

[Ankit Nautiyal]

Now that we have switched to VRR Timing generator from PTL onwards, we
no longer need to program VTOTAL.Vtotal bits, which were used by Legacy
Timing Generator.
This patch series is a continuation from discussion of another patch for
avoid reading/writing VTOTAL.Vtotal bits [1].
First patch introduces a macro to exclude DSI transcoded from VRR
programming in a consistent manner. The next patch actually modifies
reading/writing VTOTAL register.

[1] https://patchwork.freedesktop.org/patch/644683/?series=134383&rev=17

Rev2: Address comments from Ville.

Ankit Nautiyal (2):
  drm/i915/display: Introduce transcoder_has_vrr() helper
  drm/i915/display: Avoid use of VTOTAL.Vtotal bits

 drivers/gpu/drm/i915/display/intel_display.c | 46 ++++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_vrr.c     | 10 +++++
 2 files changed, 52 insertions(+), 4 deletions(-)

-- 
2.45.2

[PATCH 1/2] drm/i915/display: Introduce transcoder_has_vrr() helper

[Ankit Nautiyal]

To avoid having VRR read/write for DSI transcoders, we currently use
!transcoder_is_dsi() in many places.
Instead introduce a new helper to check transcoder_has_vrr() and use
that to exclude transcoders which do not support VRR.

v2: Include HAS_VRR into the helper. (Ville)

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index ee7812126129..0db1cd4fc963 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2625,6 +2625,15 @@ void intel_cpu_transcoder_set_m2_n2(struct intel_crtc *crtc,
 		      PIPE_LINK_N2(display, transcoder));
 }
 
+static bool
+transcoder_has_vrr(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_display *display = to_intel_display(crtc_state);
+	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
+
+	return HAS_VRR(display) && !transcoder_is_dsi(cpu_transcoder);
+}
+
 static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_display *display = to_intel_display(crtc_state);
@@ -2635,7 +2644,7 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
 	u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
 	int vsyncshift = 0;
 
-	drm_WARN_ON(display->drm, transcoder_is_dsi(cpu_transcoder));
+	drm_WARN_ON(display->drm, !transcoder_has_vrr(crtc_state));
 
 	/* We need to be careful not to changed the adjusted mode, for otherwise
 	 * the hw state checker will get angry at the mismatch. */
@@ -2717,7 +2726,7 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
 	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
 	u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
 
-	drm_WARN_ON(display->drm, transcoder_is_dsi(cpu_transcoder));
+	drm_WARN_ON(display->drm, !transcoder_has_vrr(crtc_state));
 
 	crtc_vdisplay = adjusted_mode->crtc_vdisplay;
 	crtc_vtotal = adjusted_mode->crtc_vtotal;
@@ -3908,7 +3917,7 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
 	    DISPLAY_VER(display) >= 11)
 		intel_get_transcoder_timings(crtc, pipe_config);
 
-	if (HAS_VRR(display) && !transcoder_is_dsi(pipe_config->cpu_transcoder))
+	if (transcoder_has_vrr(pipe_config))
 		intel_vrr_get_config(pipe_config);
 
 	intel_get_pipe_src_size(crtc, pipe_config);
-- 
2.45.2

[PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

[Ankit Nautiyal]

For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
bits are not required. Since the support for these bits is going to
be deprecated in upcoming platforms, avoid writing these bits for the
platforms that do not use legacy Timing Generator.

Since for these platforms TRAN_VMIN is always filled with crtc_vtotal,
use TRAN_VRR_VMIN to get the vtotal for adjusted_mode.

v2: Avoid having a helper for manipulating VTOTAL register, and instead
just make the change where required. (Ville)
v3: Set `crtc_vtotal` instead of working with the bits directly (Ville).
Use intel_vrr_vmin_vtotal() to set the vtotal during readout. (Ville)

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 31 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_vrr.c     | 10 +++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 0db1cd4fc963..6796dd0307a6 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2698,9 +2698,19 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
 		       HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
 		       HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
 
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not required. Since the support for these bits is going to
+	 * be deprecated in upcoming platforms, avoid writing these bits for the
+	 * platforms that do not use legacy Timing Generator.
+	 */
+	if (intel_vrr_always_use_vrr_tg(display))
+		crtc_vtotal = 1;
+
 	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
 		       VACTIVE(crtc_vdisplay - 1) |
 		       VTOTAL(crtc_vtotal - 1));
+
 	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
 		       VBLANK_START(crtc_vblank_start - 1) |
 		       VBLANK_END(crtc_vblank_end - 1));
@@ -2758,6 +2768,15 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
 	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
 		       VBLANK_START(crtc_vblank_start - 1) |
 		       VBLANK_END(crtc_vblank_end - 1));
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not required. Since the support for these bits is going to
+	 * be deprecated in upcoming platforms, avoid writing these bits for the
+	 * platforms that do not use legacy Timing Generator.
+	 */
+	if (intel_vrr_always_use_vrr_tg(display))
+		crtc_vtotal = 1;
+
 	/*
 	 * The double buffer latch point for TRANS_VTOTAL
 	 * is the transcoder's undelayed vblank.
@@ -2827,7 +2846,17 @@ static void intel_get_transcoder_timings(struct intel_crtc *crtc,
 
 	tmp = intel_de_read(display, TRANS_VTOTAL(display, cpu_transcoder));
 	adjusted_mode->crtc_vdisplay = REG_FIELD_GET(VACTIVE_MASK, tmp) + 1;
-	adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
+
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not filled. The value for adjusted_mode->crtc_vtotal is read
+	 * from VRR_VMIN register in intel_vrr_get_config.
+	 * Just set this to 0 here.
+	 */
+	if (intel_vrr_always_use_vrr_tg(display))
+		adjusted_mode->crtc_vtotal = 0;
+	else
+		adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
 
 	/* FIXME TGL+ DSI transcoders have this! */
 	if (!transcoder_is_dsi(cpu_transcoder)) {
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 414f93851059..7359d66fc091 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -708,6 +708,16 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
 		crtc_state->vrr.vmin = intel_de_read(display,
 						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
 
+		/*
+		 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+		 * bits are not filled. Since for these platforms TRAN_VMIN is always
+		 * filled with crtc_vtotal, use TRAN_VRR_VMIN to get the vtotal for
+		 * adjusted_mode.
+		 */
+		if (intel_vrr_always_use_vrr_tg(display))
+			crtc_state->hw.adjusted_mode.crtc_vtotal =
+				intel_vrr_vmin_vtotal(crtc_state);
+
 		if (HAS_AS_SDP(display)) {
 			trans_vrr_vsync =
 				intel_de_read(display,
-- 
2.45.2

✗ i915.CI.BAT: failure for VRR Register Read/Write Updates (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 8651 bytes --]

== Series Details ==

Series: VRR Register Read/Write Updates (rev2)
URL   : https://patchwork.freedesktop.org/series/146778/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_16321 -> Patchwork_146778v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_146778v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_146778v2, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/index.html

Participating hosts (40 -> 40)
------------------------------

  Additional (2): bat-jsl-4 fi-pnv-d510 
  Missing    (2): bat-twl-1 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_146778v2:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-ilk-650:         [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-ilk-650/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-ilk-650/igt@i915_module_load@load.html
    - fi-blb-e6850:       [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-blb-e6850/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-blb-e6850/igt@i915_module_load@load.html
    - fi-bsw-n3050:       [PASS][5] -> [ABORT][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-bsw-n3050/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-bsw-n3050/igt@i915_module_load@load.html
    - fi-pnv-d510:        NOTRUN -> [ABORT][7]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-pnv-d510/igt@i915_module_load@load.html
    - fi-kbl-7567u:       [PASS][8] -> [ABORT][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-kbl-7567u/igt@i915_module_load@load.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-kbl-7567u/igt@i915_module_load@load.html
    - fi-cfl-8700k:       [PASS][10] -> [ABORT][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-cfl-8700k/igt@i915_module_load@load.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-cfl-8700k/igt@i915_module_load@load.html
    - bat-apl-1:          [PASS][12] -> [ABORT][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/bat-apl-1/igt@i915_module_load@load.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/bat-apl-1/igt@i915_module_load@load.html
    - fi-elk-e7500:       [PASS][14] -> [ABORT][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-elk-e7500/igt@i915_module_load@load.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-elk-e7500/igt@i915_module_load@load.html
    - fi-bsw-nick:        [PASS][16] -> [ABORT][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-bsw-nick/igt@i915_module_load@load.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-bsw-nick/igt@i915_module_load@load.html
    - fi-cfl-guc:         [PASS][18] -> [ABORT][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-cfl-guc/igt@i915_module_load@load.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-cfl-guc/igt@i915_module_load@load.html
    - fi-hsw-4770:        [PASS][20] -> [ABORT][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-hsw-4770/igt@i915_module_load@load.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-hsw-4770/igt@i915_module_load@load.html
    - fi-ivb-3770:        [PASS][22] -> [ABORT][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-ivb-3770/igt@i915_module_load@load.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-ivb-3770/igt@i915_module_load@load.html

  * igt@kms_busy@basic:
    - fi-skl-6600u:       [PASS][24] -> [ABORT][25] +1 other test abort
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-skl-6600u/igt@kms_busy@basic.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-skl-6600u/igt@kms_busy@basic.html

  * igt@kms_force_connector_basic@force-connector-state:
    - bat-kbl-2:          [PASS][26] -> [ABORT][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/bat-kbl-2/igt@kms_force_connector_basic@force-connector-state.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/bat-kbl-2/igt@kms_force_connector_basic@force-connector-state.html
    - fi-kbl-x1275:       [PASS][28] -> [ABORT][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-kbl-x1275/igt@kms_force_connector_basic@force-connector-state.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-kbl-x1275/igt@kms_force_connector_basic@force-connector-state.html
    - fi-kbl-8809g:       [PASS][30] -> [ABORT][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-kbl-8809g/igt@kms_force_connector_basic@force-connector-state.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-kbl-8809g/igt@kms_force_connector_basic@force-connector-state.html
    - fi-kbl-guc:         [PASS][32] -> [ABORT][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-kbl-guc/igt@kms_force_connector_basic@force-connector-state.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-kbl-guc/igt@kms_force_connector_basic@force-connector-state.html

  
#### Warnings ####

  * igt@i915_module_load@load:
    - fi-cfl-8109u:       [DMESG-WARN][34] ([i915#13735]) -> [ABORT][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/fi-cfl-8109u/igt@i915_module_load@load.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/fi-cfl-8109u/igt@i915_module_load@load.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_module_load@load:
    - {bat-jsl-4}:        NOTRUN -> [INCOMPLETE][36]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/bat-jsl-4/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in Patchwork_146778v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_chamelium_hpd@vga-hpd-fast:
    - bat-dg2-13:         NOTRUN -> [SKIP][37] ([Intel XE#484] / [i915#4550]) +1 other test skip
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/bat-dg2-13/igt@kms_chamelium_hpd@vga-hpd-fast.html

  
#### Possible fixes ####

  * igt@i915_selftest@live:
    - bat-adlp-11:        [ABORT][38] ([i915#13696]) -> [PASS][39] +1 other test pass
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/bat-adlp-11/igt@i915_selftest@live.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/bat-adlp-11/igt@i915_selftest@live.html

  * igt@kms_chamelium_edid@dp-edid-read:
    - bat-dg2-13:         [ABORT][40] ([i915#13940]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16321/bat-dg2-13/igt@kms_chamelium_edid@dp-edid-read.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/bat-dg2-13/igt@kms_chamelium_edid@dp-edid-read.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [Intel XE#484]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/484
  [i915#13696]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13696
  [i915#13735]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13735
  [i915#13940]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13940
  [i915#4550]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4550


Build changes
-------------

  * Linux: CI_DRM_16321 -> Patchwork_146778v2

  CI-20190529: 20190529
  CI_DRM_16321: 14c330bc015ded4a1f1dd1f5aeb8617077aaa7e8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8284: 8284
  Patchwork_146778v2: 14c330bc015ded4a1f1dd1f5aeb8617077aaa7e8 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_146778v2/index.html

[-- Attachment #2: Type: text/html, Size: 9532 bytes --]

Re: [PATCH 1/2] drm/i915/display: Introduce transcoder_has_vrr() helper

[Ville Syrjälä]

On Wed, Mar 26, 2025 at 09:33:20PM +0530, Ankit Nautiyal wrote:
> To avoid having VRR read/write for DSI transcoders, we currently use
> !transcoder_is_dsi() in many places.
> Instead introduce a new helper to check transcoder_has_vrr() and use
> that to exclude transcoders which do not support VRR.
> 
> v2: Include HAS_VRR into the helper. (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index ee7812126129..0db1cd4fc963 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2625,6 +2625,15 @@ void intel_cpu_transcoder_set_m2_n2(struct intel_crtc *crtc,
>  		      PIPE_LINK_N2(display, transcoder));
>  }
>  
> +static bool
> +transcoder_has_vrr(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +
> +	return HAS_VRR(display) && !transcoder_is_dsi(cpu_transcoder);
> +}
> +
>  static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
> @@ -2635,7 +2644,7 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
>  	u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
>  	int vsyncshift = 0;
>  
> -	drm_WARN_ON(display->drm, transcoder_is_dsi(cpu_transcoder));
> +	drm_WARN_ON(display->drm, !transcoder_has_vrr(crtc_state));

Actually, this one should stay as is. We don't use this for
DSI even on hardware that lacks VRR support.

>  
>  	/* We need to be careful not to changed the adjusted mode, for otherwise
>  	 * the hw state checker will get angry at the mismatch. */
> @@ -2717,7 +2726,7 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
>  	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>  	u32 crtc_vdisplay, crtc_vtotal, crtc_vblank_start, crtc_vblank_end;
>  
> -	drm_WARN_ON(display->drm, transcoder_is_dsi(cpu_transcoder));
> +	drm_WARN_ON(display->drm, !transcoder_has_vrr(crtc_state));

same here.

>  
>  	crtc_vdisplay = adjusted_mode->crtc_vdisplay;
>  	crtc_vtotal = adjusted_mode->crtc_vtotal;
> @@ -3908,7 +3917,7 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>  	    DISPLAY_VER(display) >= 11)
>  		intel_get_transcoder_timings(crtc, pipe_config);
>  
> -	if (HAS_VRR(display) && !transcoder_is_dsi(pipe_config->cpu_transcoder))
> +	if (transcoder_has_vrr(pipe_config))
>  		intel_vrr_get_config(pipe_config);

This one is fine.

>  
>  	intel_get_pipe_src_size(crtc, pipe_config);
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

[Ville Syrjälä]

On Wed, Mar 26, 2025 at 09:33:21PM +0530, Ankit Nautiyal wrote:
> For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> bits are not required. Since the support for these bits is going to
> be deprecated in upcoming platforms, avoid writing these bits for the
> platforms that do not use legacy Timing Generator.
> 
> Since for these platforms TRAN_VMIN is always filled with crtc_vtotal,
> use TRAN_VRR_VMIN to get the vtotal for adjusted_mode.
> 
> v2: Avoid having a helper for manipulating VTOTAL register, and instead
> just make the change where required. (Ville)
> v3: Set `crtc_vtotal` instead of working with the bits directly (Ville).
> Use intel_vrr_vmin_vtotal() to set the vtotal during readout. (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 31 +++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_vrr.c     | 10 +++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 0db1cd4fc963..6796dd0307a6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2698,9 +2698,19 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
>  		       HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
>  		       HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
>  
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not required. Since the support for these bits is going to
> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
> +	 * platforms that do not use legacy Timing Generator.
> +	 */
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		crtc_vtotal = 1;
> +
>  	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
>  		       VACTIVE(crtc_vdisplay - 1) |
>  		       VTOTAL(crtc_vtotal - 1));
> +

spurious whitespace change

>  	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>  		       VBLANK_START(crtc_vblank_start - 1) |
>  		       VBLANK_END(crtc_vblank_end - 1));
> @@ -2758,6 +2768,15 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
>  	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>  		       VBLANK_START(crtc_vblank_start - 1) |
>  		       VBLANK_END(crtc_vblank_end - 1));
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not required. Since the support for these bits is going to
> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
> +	 * platforms that do not use legacy Timing Generator.
> +	 */
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		crtc_vtotal = 1;
> +
>  	/*
>  	 * The double buffer latch point for TRANS_VTOTAL
>  	 * is the transcoder's undelayed vblank.
> @@ -2827,7 +2846,17 @@ static void intel_get_transcoder_timings(struct intel_crtc *crtc,
>  
>  	tmp = intel_de_read(display, TRANS_VTOTAL(display, cpu_transcoder));
>  	adjusted_mode->crtc_vdisplay = REG_FIELD_GET(VACTIVE_MASK, tmp) + 1;
> -	adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
> +
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not filled. The value for adjusted_mode->crtc_vtotal is read
> +	 * from VRR_VMIN register in intel_vrr_get_config.
> +	 * Just set this to 0 here.
> +	 */
> +	if (intel_vrr_always_use_vrr_tg(display))

This one either needs the transcoder_has_vrr() check, or we could just
keep on blindly reading this anyway, and let intel_vrr_get_config()
overwrite it afterwards. That's kinda how we deal with
TRANS_SET_CONTEXT_LATENCY as well.

> +		adjusted_mode->crtc_vtotal = 0;
> +	else
> +		adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
>  
>  	/* FIXME TGL+ DSI transcoders have this! */
>  	if (!transcoder_is_dsi(cpu_transcoder)) {
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 414f93851059..7359d66fc091 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -708,6 +708,16 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>  		crtc_state->vrr.vmin = intel_de_read(display,
>  						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
>  
> +		/*
> +		 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +		 * bits are not filled. Since for these platforms TRAN_VMIN is always
> +		 * filled with crtc_vtotal, use TRAN_VRR_VMIN to get the vtotal for
> +		 * adjusted_mode.
> +		 */
> +		if (intel_vrr_always_use_vrr_tg(display))
> +			crtc_state->hw.adjusted_mode.crtc_vtotal =
> +				intel_vrr_vmin_vtotal(crtc_state);
> +
>  		if (HAS_AS_SDP(display)) {
>  			trans_vrr_vsync =
>  				intel_de_read(display,
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

[Nautiyal, Ankit K]

On 3/26/2025 10:39 PM, Ville Syrjälä wrote:
> On Wed, Mar 26, 2025 at 09:33:21PM +0530, Ankit Nautiyal wrote:
>> For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> bits are not required. Since the support for these bits is going to
>> be deprecated in upcoming platforms, avoid writing these bits for the
>> platforms that do not use legacy Timing Generator.
>>
>> Since for these platforms TRAN_VMIN is always filled with crtc_vtotal,
>> use TRAN_VRR_VMIN to get the vtotal for adjusted_mode.
>>
>> v2: Avoid having a helper for manipulating VTOTAL register, and instead
>> just make the change where required. (Ville)
>> v3: Set `crtc_vtotal` instead of working with the bits directly (Ville).
>> Use intel_vrr_vmin_vtotal() to set the vtotal during readout. (Ville)
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 31 +++++++++++++++++++-
>>   drivers/gpu/drm/i915/display/intel_vrr.c     | 10 +++++++
>>   2 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 0db1cd4fc963..6796dd0307a6 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -2698,9 +2698,19 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
>>   		       HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
>>   		       HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
>>   
>> +	/*
>> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> +	 * bits are not required. Since the support for these bits is going to
>> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
>> +	 * platforms that do not use legacy Timing Generator.
>> +	 */
>> +	if (intel_vrr_always_use_vrr_tg(display))
>> +		crtc_vtotal = 1;
>> +
>>   	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
>>   		       VACTIVE(crtc_vdisplay - 1) |
>>   		       VTOTAL(crtc_vtotal - 1));
>> +
> spurious whitespace change
>
>>   	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>>   		       VBLANK_START(crtc_vblank_start - 1) |
>>   		       VBLANK_END(crtc_vblank_end - 1));
>> @@ -2758,6 +2768,15 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
>>   	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>>   		       VBLANK_START(crtc_vblank_start - 1) |
>>   		       VBLANK_END(crtc_vblank_end - 1));
>> +	/*
>> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> +	 * bits are not required. Since the support for these bits is going to
>> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
>> +	 * platforms that do not use legacy Timing Generator.
>> +	 */
>> +	if (intel_vrr_always_use_vrr_tg(display))
>> +		crtc_vtotal = 1;
>> +
>>   	/*
>>   	 * The double buffer latch point for TRANS_VTOTAL
>>   	 * is the transcoder's undelayed vblank.
>> @@ -2827,7 +2846,17 @@ static void intel_get_transcoder_timings(struct intel_crtc *crtc,
>>   
>>   	tmp = intel_de_read(display, TRANS_VTOTAL(display, cpu_transcoder));
>>   	adjusted_mode->crtc_vdisplay = REG_FIELD_GET(VACTIVE_MASK, tmp) + 1;
>> -	adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
>> +
>> +	/*
>> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> +	 * bits are not filled. The value for adjusted_mode->crtc_vtotal is read
>> +	 * from VRR_VMIN register in intel_vrr_get_config.
>> +	 * Just set this to 0 here.
>> +	 */
>> +	if (intel_vrr_always_use_vrr_tg(display))
> This one either needs the transcoder_has_vrr() check, or we could just
> keep on blindly reading this anyway, and let intel_vrr_get_config()
> overwrite it afterwards. That's kinda how we deal with
> TRANS_SET_CONTEXT_LATENCY as well.

Understood. I think will just let vtotal be filled here and for 
platforms that always use VRR TG it will get overwritten in 
intel_vrr_get_config.

Regards,

Ankit

>
>> +		adjusted_mode->crtc_vtotal = 0;
>> +	else
>> +		adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
>>   
>>   	/* FIXME TGL+ DSI transcoders have this! */
>>   	if (!transcoder_is_dsi(cpu_transcoder)) {
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 414f93851059..7359d66fc091 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -708,6 +708,16 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>>   		crtc_state->vrr.vmin = intel_de_read(display,
>>   						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
>>   
>> +		/*
>> +		 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> +		 * bits are not filled. Since for these platforms TRAN_VMIN is always
>> +		 * filled with crtc_vtotal, use TRAN_VRR_VMIN to get the vtotal for
>> +		 * adjusted_mode.
>> +		 */
>> +		if (intel_vrr_always_use_vrr_tg(display))
>> +			crtc_state->hw.adjusted_mode.crtc_vtotal =
>> +				intel_vrr_vmin_vtotal(crtc_state);
>> +
>>   		if (HAS_AS_SDP(display)) {
>>   			trans_vrr_vsync =
>>   				intel_de_read(display,
>> -- 
>> 2.45.2

[PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

[Ankit Nautiyal]

For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
bits are not required. Since the support for these bits is going to
be deprecated in upcoming platforms, avoid writing these bits for the
platforms that do not use legacy Timing Generator.

Since for these platforms vrr.vmin is always filled with crtc_vtotal,
use TRAN_VRR_VMIN to get the vtotal for adjusted_mode.

v2: Avoid having a helper for manipulating VTOTAL register, and instead
just make the change where required. (Ville)
v3: Set crtc_vtotal instead of working with the bits directly (Ville).
Use intel_vrr_vmin_vtotal() to set the vtotal during readout. (Ville)
v4: Keep the reading part unchanged, and let it get overwritten for
cases where we use vrr.vmin. (Ville)

Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_vrr.c     | 10 ++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b82b3d63be73..b447fca1c616 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2698,6 +2698,15 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
 		       HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
 		       HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
 
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not required. Since the support for these bits is going to
+	 * be deprecated in upcoming platforms, avoid writing these bits for the
+	 * platforms that do not use legacy Timing Generator.
+	 */
+	if (intel_vrr_always_use_vrr_tg(display))
+		crtc_vtotal = 1;
+
 	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
 		       VACTIVE(crtc_vdisplay - 1) |
 		       VTOTAL(crtc_vtotal - 1));
@@ -2758,6 +2767,15 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
 	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
 		       VBLANK_START(crtc_vblank_start - 1) |
 		       VBLANK_END(crtc_vblank_end - 1));
+	/*
+	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+	 * bits are not required. Since the support for these bits is going to
+	 * be deprecated in upcoming platforms, avoid writing these bits for the
+	 * platforms that do not use legacy Timing Generator.
+	 */
+	if (intel_vrr_always_use_vrr_tg(display))
+		crtc_vtotal = 1;
+
 	/*
 	 * The double buffer latch point for TRANS_VTOTAL
 	 * is the transcoder's undelayed vblank.
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 414f93851059..7359d66fc091 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -708,6 +708,16 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
 		crtc_state->vrr.vmin = intel_de_read(display,
 						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
 
+		/*
+		 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
+		 * bits are not filled. Since for these platforms TRAN_VMIN is always
+		 * filled with crtc_vtotal, use TRAN_VRR_VMIN to get the vtotal for
+		 * adjusted_mode.
+		 */
+		if (intel_vrr_always_use_vrr_tg(display))
+			crtc_state->hw.adjusted_mode.crtc_vtotal =
+				intel_vrr_vmin_vtotal(crtc_state);
+
 		if (HAS_AS_SDP(display)) {
 			trans_vrr_vsync =
 				intel_de_read(display,
-- 
2.45.2

Re: [PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

[Ville Syrjälä]

On Thu, Mar 27, 2025 at 08:16:29PM +0530, Ankit Nautiyal wrote:
> For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> bits are not required. Since the support for these bits is going to
> be deprecated in upcoming platforms, avoid writing these bits for the
> platforms that do not use legacy Timing Generator.
> 
> Since for these platforms vrr.vmin is always filled with crtc_vtotal,
> use TRAN_VRR_VMIN to get the vtotal for adjusted_mode.
> 
> v2: Avoid having a helper for manipulating VTOTAL register, and instead
> just make the change where required. (Ville)
> v3: Set crtc_vtotal instead of working with the bits directly (Ville).
> Use intel_vrr_vmin_vtotal() to set the vtotal during readout. (Ville)
> v4: Keep the reading part unchanged, and let it get overwritten for
> cases where we use vrr.vmin. (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_vrr.c     | 10 ++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index b82b3d63be73..b447fca1c616 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2698,6 +2698,15 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
>  		       HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
>  		       HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
>  
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not required. Since the support for these bits is going to
> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
> +	 * platforms that do not use legacy Timing Generator.
> +	 */
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		crtc_vtotal = 1;
> +
>  	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
>  		       VACTIVE(crtc_vdisplay - 1) |
>  		       VTOTAL(crtc_vtotal - 1));
> @@ -2758,6 +2767,15 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
>  	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>  		       VBLANK_START(crtc_vblank_start - 1) |
>  		       VBLANK_END(crtc_vblank_end - 1));
> +	/*
> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +	 * bits are not required. Since the support for these bits is going to
> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
> +	 * platforms that do not use legacy Timing Generator.
> +	 */
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		crtc_vtotal = 1;
> +
>  	/*
>  	 * The double buffer latch point for TRANS_VTOTAL
>  	 * is the transcoder's undelayed vblank.
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 414f93851059..7359d66fc091 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -708,6 +708,16 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>  		crtc_state->vrr.vmin = intel_de_read(display,
>  						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
>  
> +		/*
> +		 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
> +		 * bits are not filled. Since for these platforms TRAN_VMIN is always
> +		 * filled with crtc_vtotal, use TRAN_VRR_VMIN to get the vtotal for
> +		 * adjusted_mode.
> +		 */
> +		if (intel_vrr_always_use_vrr_tg(display))
> +			crtc_state->hw.adjusted_mode.crtc_vtotal =
> +				intel_vrr_vmin_vtotal(crtc_state);
> +
>  		if (HAS_AS_SDP(display)) {
>  			trans_vrr_vsync =
>  				intel_de_read(display,
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel