[PATCH v13 0/3] VRR refactoring and panel replay workaround

[PATCH v13 0/3] VRR refactoring and panel replay workaround

[Mitul Golani]

Refactor VRR compute config to account for Panel replay workaround
for VSC SDP.

Previous Patch series links:
https://patchwork.freedesktop.org/series/135629/
https://patchwork.freedesktop.org/series/135851/
https://patchwork.freedesktop.org/series/138232/

Animesh Manna (2):
  drm/i915/vrr: Split vrr-compute-config in two phases
  drm/i915/panelreplay: Panel replay workaround with VRR

Mitul Golani (1):
  drm/i915/vrr: Add helper to check if vrr possible

 drivers/gpu/drm/i915/display/intel_display.c | 23 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h |  1 +
 drivers/gpu/drm/i915/display/intel_vrr.c     | 20 ++++++++++++-----
 drivers/gpu/drm/i915/display/intel_vrr.h     |  2 ++
 4 files changed, 41 insertions(+), 5 deletions(-)

-- 
2.46.0

[PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible

[Mitul Golani]

Add helper to check if vrr is possible based on flipline
is computed.

Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vrr.c | 7 ++++++-
 drivers/gpu/drm/i915/display/intel_vrr.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 9a51f5bac307..79db30e31324 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -56,6 +56,11 @@ bool intel_vrr_is_in_range(struct intel_connector *connector, int vrefresh)
 		vrefresh <= info->monitor_range.max_vfreq;
 }
 
+bool intel_vrr_possible(const struct intel_crtc_state *crtc_state)
+{
+	return (crtc_state->vrr.flipline) ? true : false;
+}
+
 void
 intel_vrr_check_modeset(struct intel_atomic_state *state)
 {
@@ -281,7 +286,7 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
 		intel_de_rmw(display, CHICKEN_TRANS(cpu_transcoder),
 			     0, PIPE_VBLANK_WITH_DELAY);
 
-	if (!crtc_state->vrr.flipline) {
+	if (!intel_vrr_possible(crtc_state)) {
 		intel_de_write(display,
 			       TRANS_VRR_CTL(display, cpu_transcoder), 0);
 		return;
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
index 89937858200d..af921dda4619 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.h
+++ b/drivers/gpu/drm/i915/display/intel_vrr.h
@@ -15,6 +15,7 @@ struct intel_crtc_state;
 
 bool intel_vrr_is_capable(struct intel_connector *connector);
 bool intel_vrr_is_in_range(struct intel_connector *connector, int vrefresh);
+bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
 void intel_vrr_check_modeset(struct intel_atomic_state *state);
 void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
 			      struct drm_connector_state *conn_state);
-- 
2.46.0

RE: [PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Mitul Golani
Sent: Tuesday, October 1, 2024 6:47 AM
To: intel-gfx@lists.freedesktop.org
Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma <uma.shankar@intel.com>
Subject: [PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible
> 
> Add helper to check if vrr is possible based on flipline
> is computed.
> 
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 7 ++++++-
>  drivers/gpu/drm/i915/display/intel_vrr.h | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 9a51f5bac307..79db30e31324 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -56,6 +56,11 @@ bool intel_vrr_is_in_range(struct intel_connector *connector, int vrefresh)
>  		vrefresh <= info->monitor_range.max_vfreq;
>  }
>  
> +bool intel_vrr_possible(const struct intel_crtc_state *crtc_state)
> +{
> +	return (crtc_state->vrr.flipline) ? true : false;

I think this can be compressed to:

"""
	return !!(crtc_state->vrr.flipline);
"""

But otherwise:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
-Jonathan Cavitt

> +}
> +
>  void
>  intel_vrr_check_modeset(struct intel_atomic_state *state)
>  {
> @@ -281,7 +286,7 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
>  		intel_de_rmw(display, CHICKEN_TRANS(cpu_transcoder),
>  			     0, PIPE_VBLANK_WITH_DELAY);
>  
> -	if (!crtc_state->vrr.flipline) {
> +	if (!intel_vrr_possible(crtc_state)) {
>  		intel_de_write(display,
>  			       TRANS_VRR_CTL(display, cpu_transcoder), 0);
>  		return;
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
> index 89937858200d..af921dda4619 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> @@ -15,6 +15,7 @@ struct intel_crtc_state;
>  
>  bool intel_vrr_is_capable(struct intel_connector *connector);
>  bool intel_vrr_is_in_range(struct intel_connector *connector, int vrefresh);
> +bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_check_modeset(struct intel_atomic_state *state);
>  void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  			      struct drm_connector_state *conn_state);
> -- 
> 2.46.0
> 
> 

RE: [PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Sent: Tuesday, October 1, 2024 7:37 PM
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>;
> intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Subject: RE: [PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible
> 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Mitul
> Golani
> Sent: Tuesday, October 1, 2024 6:47 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: [PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible
> >
> > Add helper to check if vrr is possible based on flipline is computed.
> >
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_vrr.c | 7 ++++++-
> > drivers/gpu/drm/i915/display/intel_vrr.h | 1 +
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 9a51f5bac307..79db30e31324 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -56,6 +56,11 @@ bool intel_vrr_is_in_range(struct intel_connector
> *connector, int vrefresh)
> >  		vrefresh <= info->monitor_range.max_vfreq;  }
> >
> > +bool intel_vrr_possible(const struct intel_crtc_state *crtc_state) {
> > +	return (crtc_state->vrr.flipline) ? true : false;
> 
> I think this can be compressed to:
> 
> """
> 	return !!(crtc_state->vrr.flipline);
> """
> 
> But otherwise:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> -Jonathan Cavitt
> 

Thanks @Cavitt, Jonathan for the review,

Agreed, with suggested change. On next version will be returning just
'crtc_state->vrr.flipline' or 'return crtc_state->vrr.flipline != 0;' as suggested by ville
in latest review. 

Again Thanks

> > +}
> > +
> >  void
> >  intel_vrr_check_modeset(struct intel_atomic_state *state)  { @@
> > -281,7 +286,7 @@ void intel_vrr_set_transcoder_timings(const struct
> intel_crtc_state *crtc_state)
> >  		intel_de_rmw(display, CHICKEN_TRANS(cpu_transcoder),
> >  			     0, PIPE_VBLANK_WITH_DELAY);
> >
> > -	if (!crtc_state->vrr.flipline) {
> > +	if (!intel_vrr_possible(crtc_state)) {
> >  		intel_de_write(display,
> >  			       TRANS_VRR_CTL(display, cpu_transcoder), 0);
> >  		return;
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h
> > b/drivers/gpu/drm/i915/display/intel_vrr.h
> > index 89937858200d..af921dda4619 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> > @@ -15,6 +15,7 @@ struct intel_crtc_state;
> >
> >  bool intel_vrr_is_capable(struct intel_connector *connector);  bool
> > intel_vrr_is_in_range(struct intel_connector *connector, int
> > vrefresh);
> > +bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
> >  void intel_vrr_check_modeset(struct intel_atomic_state *state);  void
> > intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
> >  			      struct drm_connector_state *conn_state);
> > --
> > 2.46.0
> >
> >

Re: [PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible

[Ville Syrjälä]

On Tue, Oct 01, 2024 at 07:17:01PM +0530, Mitul Golani wrote:
> Add helper to check if vrr is possible based on flipline
> is computed.
> 
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 7 ++++++-
>  drivers/gpu/drm/i915/display/intel_vrr.h | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 9a51f5bac307..79db30e31324 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -56,6 +56,11 @@ bool intel_vrr_is_in_range(struct intel_connector *connector, int vrefresh)
>  		vrefresh <= info->monitor_range.max_vfreq;
>  }
>  
> +bool intel_vrr_possible(const struct intel_crtc_state *crtc_state)
> +{
> +	return (crtc_state->vrr.flipline) ? true : false;

That can be just 'return crtc_state->vrr.flipline;',
of 'return crtc_state->vrr.flipline != 0;' if you prefer.

> +}
> +
>  void
>  intel_vrr_check_modeset(struct intel_atomic_state *state)
>  {
> @@ -281,7 +286,7 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
>  		intel_de_rmw(display, CHICKEN_TRANS(cpu_transcoder),
>  			     0, PIPE_VBLANK_WITH_DELAY);
>  
> -	if (!crtc_state->vrr.flipline) {
> +	if (!intel_vrr_possible(crtc_state)) {

Hmm. Looks like we have a fairly big mess with the AS SDP 
and CMRR stuff when it comes to programming the hardware
vs. readout. But that's not the fault of this patch obviously.

>  		intel_de_write(display,
>  			       TRANS_VRR_CTL(display, cpu_transcoder), 0);
>  		return;
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
> index 89937858200d..af921dda4619 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> @@ -15,6 +15,7 @@ struct intel_crtc_state;
>  
>  bool intel_vrr_is_capable(struct intel_connector *connector);
>  bool intel_vrr_is_in_range(struct intel_connector *connector, int vrefresh);
> +bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_check_modeset(struct intel_atomic_state *state);
>  void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  			      struct drm_connector_state *conn_state);
> -- 
> 2.46.0

-- 
Ville Syrjälä
Intel

RE: [PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, October 9, 2024 12:55 PM
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Syrjala, Ville <ville.syrjala@intel.com>; Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [PATCH v13 1/3] drm/i915/vrr: Add helper to check if vrr possible
> 
> On Tue, Oct 01, 2024 at 07:17:01PM +0530, Mitul Golani wrote:
> > Add helper to check if vrr is possible based on flipline is computed.
> >
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_vrr.c | 7 ++++++-
> > drivers/gpu/drm/i915/display/intel_vrr.h | 1 +
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 9a51f5bac307..79db30e31324 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -56,6 +56,11 @@ bool intel_vrr_is_in_range(struct intel_connector
> *connector, int vrefresh)
> >  		vrefresh <= info->monitor_range.max_vfreq;  }
> >
> > +bool intel_vrr_possible(const struct intel_crtc_state *crtc_state) {
> > +	return (crtc_state->vrr.flipline) ? true : false;
> 
> That can be just 'return crtc_state->vrr.flipline;', of 'return crtc_state-
> >vrr.flipline != 0;' if you prefer.
> 

Thanks Ville for the review,

I will update it to just return crtc_state->vrr.flipline in next revision.

> > +}
> > +
> >  void
> >  intel_vrr_check_modeset(struct intel_atomic_state *state)  { @@
> > -281,7 +286,7 @@ void intel_vrr_set_transcoder_timings(const struct
> intel_crtc_state *crtc_state)
> >  		intel_de_rmw(display, CHICKEN_TRANS(cpu_transcoder),
> >  			     0, PIPE_VBLANK_WITH_DELAY);
> >
> > -	if (!crtc_state->vrr.flipline) {
> > +	if (!intel_vrr_possible(crtc_state)) {
> 
> Hmm. Looks like we have a fairly big mess with the AS SDP and CMRR stuff
> when it comes to programming the hardware vs. readout. But that's not the
> fault of this patch obviously.
> 
> >  		intel_de_write(display,
> >  			       TRANS_VRR_CTL(display, cpu_transcoder), 0);
> >  		return;
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h
> > b/drivers/gpu/drm/i915/display/intel_vrr.h
> > index 89937858200d..af921dda4619 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> > @@ -15,6 +15,7 @@ struct intel_crtc_state;
> >
> >  bool intel_vrr_is_capable(struct intel_connector *connector);  bool
> > intel_vrr_is_in_range(struct intel_connector *connector, int
> > vrefresh);
> > +bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
> >  void intel_vrr_check_modeset(struct intel_atomic_state *state);  void
> > intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
> >  			      struct drm_connector_state *conn_state);
> > --
> > 2.46.0
> 
> --
> Ville Syrjälä
> Intel

[PATCH v13 2/3] drm/i915/vrr: Split vrr-compute-config in two phases

[Mitul Golani]

From: Animesh Manna <animesh.manna@intel.com>

As vrr guardband calculation is dependent on modified
vblank start so better to compute late after all
vblank adjustement.

v1: Initial version.
v2: Split in a separate patch from panel-replay workaround. [Ankit]
v3: Add a function for late vrr related computation. [Ville]
v4: Use flipline instead of vrr.enable and some cosmetic changes. [Ville]
v5: Use intel_vrr_possible helper.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  2 ++
 drivers/gpu/drm/i915/display/intel_vrr.c     | 13 +++++++++----
 drivers/gpu/drm/i915/display/intel_vrr.h     |  1 +
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index f7667931f9d9..c59d7bffbf57 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4836,6 +4836,8 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
 	struct drm_connector *connector;
 	int i;
 
+	intel_vrr_compute_config_late(crtc_state);
+
 	for_each_new_connector_in_state(&state->base, connector,
 					conn_state, i) {
 		struct intel_encoder *encoder =
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index 79db30e31324..734db543b90c 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -244,11 +244,16 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
 			(crtc_state->hw.adjusted_mode.crtc_vtotal -
 			 crtc_state->hw.adjusted_mode.vsync_end);
 	}
+}
+
+void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
+{
+	struct intel_display *display = to_intel_display(crtc_state);
+	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
+
+	if (!intel_vrr_possible(crtc_state))
+		return;
 
-	/*
-	 * For XE_LPD+, we use guardband and pipeline override
-	 * is deprecated.
-	 */
 	if (DISPLAY_VER(display) >= 13) {
 		crtc_state->vrr.guardband =
 			crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vblank_start;
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
index af921dda4619..b3b45c675020 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.h
+++ b/drivers/gpu/drm/i915/display/intel_vrr.h
@@ -19,6 +19,7 @@ bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
 void intel_vrr_check_modeset(struct intel_atomic_state *state);
 void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
 			      struct drm_connector_state *conn_state);
+void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
 void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
 void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
 void intel_vrr_send_push(const struct intel_crtc_state *crtc_state);
-- 
2.46.0

RE: [PATCH v13 2/3] drm/i915/vrr: Split vrr-compute-config in two phases

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Mitul Golani
Sent: Tuesday, October 1, 2024 6:47 AM
To: intel-gfx@lists.freedesktop.org
Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma <uma.shankar@intel.com>
Subject: [PATCH v13 2/3] drm/i915/vrr: Split vrr-compute-config in two phases
> 
> From: Animesh Manna <animesh.manna@intel.com>
> 
> As vrr guardband calculation is dependent on modified
> vblank start so better to compute late after all
> vblank adjustement.
> 
> v1: Initial version.
> v2: Split in a separate patch from panel-replay workaround. [Ankit]
> v3: Add a function for late vrr related computation. [Ville]
> v4: Use flipline instead of vrr.enable and some cosmetic changes. [Ville]
> v5: Use intel_vrr_possible helper.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c |  2 ++
>  drivers/gpu/drm/i915/display/intel_vrr.c     | 13 +++++++++----
>  drivers/gpu/drm/i915/display/intel_vrr.h     |  1 +
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f7667931f9d9..c59d7bffbf57 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4836,6 +4836,8 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
>  	struct drm_connector *connector;
>  	int i;
>  
> +	intel_vrr_compute_config_late(crtc_state);
> +
>  	for_each_new_connector_in_state(&state->base, connector,
>  					conn_state, i) {
>  		struct intel_encoder *encoder =
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 79db30e31324..734db543b90c 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -244,11 +244,16 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  			(crtc_state->hw.adjusted_mode.crtc_vtotal -
>  			 crtc_state->hw.adjusted_mode.vsync_end);
>  	}
> +}
> +
> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	if (!intel_vrr_possible(crtc_state))
> +		return;
>  
> -	/*
> -	 * For XE_LPD+, we use guardband and pipeline override
> -	 * is deprecated.
> -	 */

Not sure why this comment is being deleted, but I won't block on it.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
-Jonathan Cavitt

>  	if (DISPLAY_VER(display) >= 13) {
>  		crtc_state->vrr.guardband =
>  			crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vblank_start;
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
> index af921dda4619..b3b45c675020 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> @@ -19,6 +19,7 @@ bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_check_modeset(struct intel_atomic_state *state);
>  void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  			      struct drm_connector_state *conn_state);
> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
>  void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
>  void intel_vrr_send_push(const struct intel_crtc_state *crtc_state);
> -- 
> 2.46.0
> 
> 

RE: [PATCH v13 2/3] drm/i915/vrr: Split vrr-compute-config in two phases

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Sent: Tuesday, October 1, 2024 7:40 PM
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>;
> intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Subject: RE: [PATCH v13 2/3] drm/i915/vrr: Split vrr-compute-config in two
> phases
> 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Mitul
> Golani
> Sent: Tuesday, October 1, 2024 6:47 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: [PATCH v13 2/3] drm/i915/vrr: Split vrr-compute-config in two phases
> >
> > From: Animesh Manna <animesh.manna@intel.com>
> >
> > As vrr guardband calculation is dependent on modified vblank start so
> > better to compute late after all vblank adjustement.
> >
> > v1: Initial version.
> > v2: Split in a separate patch from panel-replay workaround. [Ankit]
> > v3: Add a function for late vrr related computation. [Ville]
> > v4: Use flipline instead of vrr.enable and some cosmetic changes.
> > [Ville]
> > v5: Use intel_vrr_possible helper.
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c |  2 ++
> >  drivers/gpu/drm/i915/display/intel_vrr.c     | 13 +++++++++----
> >  drivers/gpu/drm/i915/display/intel_vrr.h     |  1 +
> >  3 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index f7667931f9d9..c59d7bffbf57 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4836,6 +4836,8 @@ intel_modeset_pipe_config_late(struct
> intel_atomic_state *state,
> >  	struct drm_connector *connector;
> >  	int i;
> >
> > +	intel_vrr_compute_config_late(crtc_state);
> > +
> >  	for_each_new_connector_in_state(&state->base, connector,
> >  					conn_state, i) {
> >  		struct intel_encoder *encoder =
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 79db30e31324..734db543b90c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -244,11 +244,16 @@ intel_vrr_compute_config(struct intel_crtc_state
> *crtc_state,
> >  			(crtc_state->hw.adjusted_mode.crtc_vtotal -
> >  			 crtc_state->hw.adjusted_mode.vsync_end);
> >  	}
> > +}
> > +
> > +void intel_vrr_compute_config_late(struct intel_crtc_state
> > +*crtc_state) {
> > +	struct intel_display *display = to_intel_display(crtc_state);
> > +	struct drm_display_mode *adjusted_mode =
> > +&crtc_state->hw.adjusted_mode;
> > +
> > +	if (!intel_vrr_possible(crtc_state))
> > +		return;
> >
> > -	/*
> > -	 * For XE_LPD+, we use guardband and pipeline override
> > -	 * is deprecated.
> > -	 */
> 
> Not sure why this comment is being deleted, but I won't block on it.
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> -Jonathan Cavitt

Thanks @Cavitt, Jonathan for the review,

Comment was deleted as suggested by ville in revision 2 for this series. Also that is coming
As redundant to when first time we calculated guardband. 

> 
> >  	if (DISPLAY_VER(display) >= 13) {
> >  		crtc_state->vrr.guardband =
> >  			crtc_state->vrr.vmin + 1 - adjusted_mode-
> >crtc_vblank_start; diff
> > --git a/drivers/gpu/drm/i915/display/intel_vrr.h
> > b/drivers/gpu/drm/i915/display/intel_vrr.h
> > index af921dda4619..b3b45c675020 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> > @@ -19,6 +19,7 @@ bool intel_vrr_possible(const struct
> > intel_crtc_state *crtc_state);  void intel_vrr_check_modeset(struct
> > intel_atomic_state *state);  void intel_vrr_compute_config(struct
> intel_crtc_state *crtc_state,
> >  			      struct drm_connector_state *conn_state);
> > +void intel_vrr_compute_config_late(struct intel_crtc_state
> > +*crtc_state);
> >  void intel_vrr_set_transcoder_timings(const struct intel_crtc_state
> > *crtc_state);  void intel_vrr_enable(const struct intel_crtc_state
> > *crtc_state);  void intel_vrr_send_push(const struct intel_crtc_state
> > *crtc_state);
> > --
> > 2.46.0
> >
> >

[PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with VRR

[Mitul Golani]

From: Animesh Manna <animesh.manna@intel.com>

Panel Replay VSC SDP not getting sent when VRR is enabled
and W1 and W2 are 0. So Program Set Context Latency in
TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
The same is applicable for PSR1/PSR2 as well.

HSD: 14015406119

v1: Initial version.
v2: Update timings stored in adjusted_mode struct. [Ville]
v3: Add WA in compute_config(). [Ville]
v4:
- Add DISPLAY_VER() check and improve code comment. [Rodrigo]
- Introduce centralized intel_crtc_vblank_delay(). [Ville]
v5: Move to crtc_compute_config(). [Ville]
v6: Restrict DISPLAY_VER till 14. [Mitul]
v7:
- Corrected code-comment. [Mitul]
- dev_priv local variable removed. [Jani]
v8: Introduce late_compute_config() which will take care late
vblank-delay adjustment. [Ville]
v9: Implementation simplified and split into multiple patches.
v10:
- Split vrr changes and use struct intel_display in DISPLAY_VER(). [Ankit]
- Use for_each_new_intel_connector_in_state(). [Jani]
v11: Remove loop and use flipline instead of vrr.enable flag. [Ville]
v12:
- Use intel_Vrr_possible helper.
- Correct flag check for flipline.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 21 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_display.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c59d7bffbf57..a8f846b654e9 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2573,6 +2573,8 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
 		intel_atomic_get_new_crtc_state(state, crtc);
 	int ret;
 
+	intel_crtc_adjust_vblank_delay(crtc_state);
+
 	ret = intel_dpll_crtc_compute_clock(state, crtc);
 	if (ret)
 		return ret;
@@ -3985,6 +3987,25 @@ bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state)
 	return true;
 }
 
+void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state)
+{
+	struct intel_display *display = to_intel_display(crtc_state);
+	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
+
+	/*
+	 * wa_14015401596 for display versions 13, 14.
+	 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY register
+	 * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with VRR.
+	 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by substracting
+	 * crtc_vdisplay from crtc_vblank_start, so incrementing crtc_vblank_start
+	 * by 1 if both are equal.
+	 */
+	if (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
+	    adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay &&
+	    IS_DISPLAY_VER(display, 13, 14))
+		adjusted_mode->crtc_vblank_start += 1;
+}
+
 int intel_dotclock_calculate(int link_freq,
 			     const struct intel_link_m_n *m_n)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 1f0fed5ea7bc..e6bd03ef104d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -446,6 +446,7 @@ u8 _intel_modeset_primary_pipes(const struct intel_crtc_state *crtc_state);
 u8 _intel_modeset_secondary_pipes(const struct intel_crtc_state *crtc_state);
 struct intel_crtc *intel_primary_crtc(const struct intel_crtc_state *crtc_state);
 bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
+void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state);
 bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 			       const struct intel_crtc_state *pipe_config,
 			       bool fastset);
-- 
2.46.0

RE: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with VRR

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Mitul Golani
Sent: Tuesday, October 1, 2024 6:47 AM
To: intel-gfx@lists.freedesktop.org
Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma <uma.shankar@intel.com>
Subject: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with VRR
> 
> From: Animesh Manna <animesh.manna@intel.com>
> 
> Panel Replay VSC SDP not getting sent when VRR is enabled
> and W1 and W2 are 0. So Program Set Context Latency in
> TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
> The same is applicable for PSR1/PSR2 as well.
> 
> HSD: 14015406119
> 
> v1: Initial version.
> v2: Update timings stored in adjusted_mode struct. [Ville]
> v3: Add WA in compute_config(). [Ville]
> v4:
> - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> v5: Move to crtc_compute_config(). [Ville]
> v6: Restrict DISPLAY_VER till 14. [Mitul]
> v7:
> - Corrected code-comment. [Mitul]
> - dev_priv local variable removed. [Jani]
> v8: Introduce late_compute_config() which will take care late
> vblank-delay adjustment. [Ville]
> v9: Implementation simplified and split into multiple patches.
> v10:
> - Split vrr changes and use struct intel_display in DISPLAY_VER(). [Ankit]
> - Use for_each_new_intel_connector_in_state(). [Jani]
> v11: Remove loop and use flipline instead of vrr.enable flag. [Ville]
> v12:
> - Use intel_Vrr_possible helper.
> - Correct flag check for flipline.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>

I left some notes below, but nothing blocking, so:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 21 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c59d7bffbf57..a8f846b654e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2573,6 +2573,8 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	int ret;
>  
> +	intel_crtc_adjust_vblank_delay(crtc_state);


If the purpose of adjusting the vblank delay is for workaround 14015401596,
we might want to consider refactoring this into two steps.  Specifically, we would
first separately check if the workaround is needed:

"""
/*
 * wa_14015401596 for display versions 13, 14.
 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY register
 * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with VRR.
 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by subtracting
 * crtc_vdisplay from crtc_vblank_start, so this workaround is needed when
 * both are equal.
 */
static bool intel_crtc_needs_wa_14015401596(struct intel_crtc_state *crtc_state)
{
	struct intel_display *display = to_intel_display(crtc_state);
	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;

	return (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
		adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay &&
		IS_DISPLAY_VER(display, 13, 14));
}
"""

Then, here, instead of calling intel_crtc_adjust_vblank_delay, we would do this:

"""
	if (intel_crtc_needs_wa_14015401596(crtc_state)) {
		struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
		adjusted_mode->crtc_vblank_start += 1;
	}
"""

I don't think this refactor is strictly necessary, though, so feel free to ignore this.


> +
>  	ret = intel_dpll_crtc_compute_clock(state, crtc);
>  	if (ret)
>  		return ret;
> @@ -3985,6 +3987,25 @@ bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state)
>  	return true;
>  }
>  
> +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	/*
> +	 * wa_14015401596 for display versions 13, 14.
> +	 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY register
> +	 * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with VRR.
> +	 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by substracting


s/substracting/subtracting


> +	 * crtc_vdisplay from crtc_vblank_start, so incrementing crtc_vblank_start
> +	 * by 1 if both are equal.
> +	 */
> +	if (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
> +	    adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay &&
> +	    IS_DISPLAY_VER(display, 13, 14))
> +		adjusted_mode->crtc_vblank_start += 1;
> +}
> +
>  int intel_dotclock_calculate(int link_freq,
>  			     const struct intel_link_m_n *m_n)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 1f0fed5ea7bc..e6bd03ef104d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -446,6 +446,7 @@ u8 _intel_modeset_primary_pipes(const struct intel_crtc_state *crtc_state);
>  u8 _intel_modeset_secondary_pipes(const struct intel_crtc_state *crtc_state);
>  struct intel_crtc *intel_primary_crtc(const struct intel_crtc_state *crtc_state);
>  bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
> +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state);


You may want to make this a static function in intel_display.c.  Doing so would eliminate the need for
the added function declaration in intel_display.h.  The only caveat is that you would need to move the
current definition of this function in intel_display.c from where it is now to above
intel_crtc_compute_config if you wanted to go that route.
-Jonathan Cavitt


>  bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  			       const struct intel_crtc_state *pipe_config,
>  			       bool fastset);
> -- 
> 2.46.0
> 
> 

RE: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with VRR

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Sent: Tuesday, October 1, 2024 8:37 PM
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>;
> intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>; Cavitt, Jonathan <jonathan.cavitt@intel.com>
> Subject: RE: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround
> with VRR
> 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Mitul
> Golani
> Sent: Tuesday, October 1, 2024 6:47 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
> Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with
> VRR
> >
> > From: Animesh Manna <animesh.manna@intel.com>
> >
> > Panel Replay VSC SDP not getting sent when VRR is enabled and W1 and
> > W2 are 0. So Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> > register to at least a value of 1.
> > The same is applicable for PSR1/PSR2 as well.
> >
> > HSD: 14015406119
> >
> > v1: Initial version.
> > v2: Update timings stored in adjusted_mode struct. [Ville]
> > v3: Add WA in compute_config(). [Ville]
> > v4:
> > - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> > - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> > v5: Move to crtc_compute_config(). [Ville]
> > v6: Restrict DISPLAY_VER till 14. [Mitul]
> > v7:
> > - Corrected code-comment. [Mitul]
> > - dev_priv local variable removed. [Jani]
> > v8: Introduce late_compute_config() which will take care late
> > vblank-delay adjustment. [Ville]
> > v9: Implementation simplified and split into multiple patches.
> > v10:
> > - Split vrr changes and use struct intel_display in DISPLAY_VER().
> > [Ankit]
> > - Use for_each_new_intel_connector_in_state(). [Jani]
> > v11: Remove loop and use flipline instead of vrr.enable flag. [Ville]
> > v12:
> > - Use intel_Vrr_possible helper.
> > - Correct flag check for flipline.
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> 
> I left some notes below, but nothing blocking, so:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 21
> > ++++++++++++++++++++  drivers/gpu/drm/i915/display/intel_display.h |
> > 1 +
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index c59d7bffbf57..a8f846b654e9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2573,6 +2573,8 @@ static int intel_crtc_compute_config(struct
> intel_atomic_state *state,
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> >  	int ret;
> >
> > +	intel_crtc_adjust_vblank_delay(crtc_state);
> 
> 
> If the purpose of adjusting the vblank delay is for workaround 14015401596, we
> might want to consider refactoring this into two steps.  Specifically, we would
> first separately check if the workaround is needed:
> 
> """
> /*
>  * wa_14015401596 for display versions 13, 14.
>  * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY register
>  * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with VRR.
>  * Value for TRANS_SET_CONTEXT_LATENCY is calculated by subtracting
>  * crtc_vdisplay from crtc_vblank_start, so this workaround is needed when
>  * both are equal.
>  */
> static bool intel_crtc_needs_wa_14015401596(struct intel_crtc_state
> *crtc_state) {
> 	struct intel_display *display = to_intel_display(crtc_state);
> 	struct drm_display_mode *adjusted_mode = &crtc_state-
> >hw.adjusted_mode;
> 
> 	return (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
> 		adjusted_mode->crtc_vblank_start == adjusted_mode-
> >crtc_vdisplay &&
> 		IS_DISPLAY_VER(display, 13, 14));
> }
> """
> 
> Then, here, instead of calling intel_crtc_adjust_vblank_delay, we would do this:
> 
> """
> 	if (intel_crtc_needs_wa_14015401596(crtc_state)) {
> 		struct drm_display_mode *adjusted_mode = &crtc_state-
> >hw.adjusted_mode;
> 		adjusted_mode->crtc_vblank_start += 1;
> 	}
> """
> 
> I don't think this refactor is strictly necessary, though, so feel free to ignore this.
> 
> 
> > +
> >  	ret = intel_dpll_crtc_compute_clock(state, crtc);
> >  	if (ret)
> >  		return ret;
> > @@ -3985,6 +3987,25 @@ bool intel_crtc_get_pipe_config(struct
> intel_crtc_state *crtc_state)
> >  	return true;
> >  }
> >
> > +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state
> > +*crtc_state) {
> > +	struct intel_display *display = to_intel_display(crtc_state);
> > +	struct drm_display_mode *adjusted_mode =
> > +&crtc_state->hw.adjusted_mode;
> > +
> > +	/*
> > +	 * wa_14015401596 for display versions 13, 14.
> > +	 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> register
> > +	 * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with
> VRR.
> > +	 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by
> substracting
> 
> 
> s/substracting/subtracting
> 
> 
> > +	 * crtc_vdisplay from crtc_vblank_start, so incrementing
> crtc_vblank_start
> > +	 * by 1 if both are equal.
> > +	 */
> > +	if (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
> > +	    adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay
> &&
> > +	    IS_DISPLAY_VER(display, 13, 14))
> > +		adjusted_mode->crtc_vblank_start += 1; }
> > +
> >  int intel_dotclock_calculate(int link_freq,
> >  			     const struct intel_link_m_n *m_n)  { diff --git
> > a/drivers/gpu/drm/i915/display/intel_display.h
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index 1f0fed5ea7bc..e6bd03ef104d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -446,6 +446,7 @@ u8 _intel_modeset_primary_pipes(const struct
> > intel_crtc_state *crtc_state);
> >  u8 _intel_modeset_secondary_pipes(const struct intel_crtc_state
> > *crtc_state);  struct intel_crtc *intel_primary_crtc(const struct
> > intel_crtc_state *crtc_state);  bool intel_crtc_get_pipe_config(struct
> > intel_crtc_state *crtc_state);
> > +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state
> > +*crtc_state);
> 
> 
> You may want to make this a static function in intel_display.c.  Doing so would
> eliminate the need for the added function declaration in intel_display.h.  The
> only caveat is that you would need to move the current definition of this
> function in intel_display.c from where it is now to above
> intel_crtc_compute_config if you wanted to go that route.
> -Jonathan Cavitt
> 

Thanks @Cavitt, Jonathan
I agreed to your suggestion related to needs_wa function.

I will address all suggested comments in next revision.

> 
> >  bool intel_pipe_config_compare(const struct intel_crtc_state
> *current_config,
> >  			       const struct intel_crtc_state *pipe_config,
> >  			       bool fastset);
> > --
> > 2.46.0
> >
> >

Re: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with VRR

[Ville Syrjälä]

On Tue, Oct 01, 2024 at 07:17:03PM +0530, Mitul Golani wrote:
> From: Animesh Manna <animesh.manna@intel.com>
> 
> Panel Replay VSC SDP not getting sent when VRR is enabled
> and W1 and W2 are 0. So Program Set Context Latency in
> TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
> The same is applicable for PSR1/PSR2 as well.
> 
> HSD: 14015406119
> 
> v1: Initial version.
> v2: Update timings stored in adjusted_mode struct. [Ville]
> v3: Add WA in compute_config(). [Ville]
> v4:
> - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> v5: Move to crtc_compute_config(). [Ville]
> v6: Restrict DISPLAY_VER till 14. [Mitul]
> v7:
> - Corrected code-comment. [Mitul]
> - dev_priv local variable removed. [Jani]
> v8: Introduce late_compute_config() which will take care late
> vblank-delay adjustment. [Ville]
> v9: Implementation simplified and split into multiple patches.
> v10:
> - Split vrr changes and use struct intel_display in DISPLAY_VER(). [Ankit]
> - Use for_each_new_intel_connector_in_state(). [Jani]
> v11: Remove loop and use flipline instead of vrr.enable flag. [Ville]
> v12:
> - Use intel_Vrr_possible helper.
> - Correct flag check for flipline.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 21 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c59d7bffbf57..a8f846b654e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2573,6 +2573,8 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	int ret;
>  
> +	intel_crtc_adjust_vblank_delay(crtc_state);
> +
>  	ret = intel_dpll_crtc_compute_clock(state, crtc);
>  	if (ret)
>  		return ret;
> @@ -3985,6 +3987,25 @@ bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state)
>  	return true;
>  }
>  
> +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	/*
> +	 * wa_14015401596 for display versions 13, 14.
> +	 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY register
> +	 * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with VRR.
> +	 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by substracting
> +	 * crtc_vdisplay from crtc_vblank_start, so incrementing crtc_vblank_start
> +	 * by 1 if both are equal.

You're just paraphrasing the code in different words here. 
Please don't, and just drop the whole comment (apart from the
w/a number ofc).

> +	 */
> +	if (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
> +	    adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay &&
> +	    IS_DISPLAY_VER(display, 13, 14))
> +		adjusted_mode->crtc_vblank_start += 1;
> +}
> +
>  int intel_dotclock_calculate(int link_freq,
>  			     const struct intel_link_m_n *m_n)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 1f0fed5ea7bc..e6bd03ef104d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -446,6 +446,7 @@ u8 _intel_modeset_primary_pipes(const struct intel_crtc_state *crtc_state);
>  u8 _intel_modeset_secondary_pipes(const struct intel_crtc_state *crtc_state);
>  struct intel_crtc *intel_primary_crtc(const struct intel_crtc_state *crtc_state);
>  bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
> +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state);
>  bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  			       const struct intel_crtc_state *pipe_config,
>  			       bool fastset);
> -- 
> 2.46.0

-- 
Ville Syrjälä
Intel

RE: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with VRR

[Golani, Mitulkumar Ajitkumar]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, October 9, 2024 1:01 PM
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Syrjala, Ville <ville.syrjala@intel.com>; Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround
> with VRR
> 
> On Tue, Oct 01, 2024 at 07:17:03PM +0530, Mitul Golani wrote:
> > From: Animesh Manna <animesh.manna@intel.com>
> >
> > Panel Replay VSC SDP not getting sent when VRR is enabled and W1 and
> > W2 are 0. So Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> > register to at least a value of 1.
> > The same is applicable for PSR1/PSR2 as well.
> >
> > HSD: 14015406119
> >
> > v1: Initial version.
> > v2: Update timings stored in adjusted_mode struct. [Ville]
> > v3: Add WA in compute_config(). [Ville]
> > v4:
> > - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> > - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> > v5: Move to crtc_compute_config(). [Ville]
> > v6: Restrict DISPLAY_VER till 14. [Mitul]
> > v7:
> > - Corrected code-comment. [Mitul]
> > - dev_priv local variable removed. [Jani]
> > v8: Introduce late_compute_config() which will take care late
> > vblank-delay adjustment. [Ville]
> > v9: Implementation simplified and split into multiple patches.
> > v10:
> > - Split vrr changes and use struct intel_display in DISPLAY_VER().
> > [Ankit]
> > - Use for_each_new_intel_connector_in_state(). [Jani]
> > v11: Remove loop and use flipline instead of vrr.enable flag. [Ville]
> > v12:
> > - Use intel_Vrr_possible helper.
> > - Correct flag check for flipline.
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 21
> > ++++++++++++++++++++  drivers/gpu/drm/i915/display/intel_display.h |
> > 1 +
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index c59d7bffbf57..a8f846b654e9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2573,6 +2573,8 @@ static int intel_crtc_compute_config(struct
> intel_atomic_state *state,
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> >  	int ret;
> >
> > +	intel_crtc_adjust_vblank_delay(crtc_state);
> > +
> >  	ret = intel_dpll_crtc_compute_clock(state, crtc);
> >  	if (ret)
> >  		return ret;
> > @@ -3985,6 +3987,25 @@ bool intel_crtc_get_pipe_config(struct
> intel_crtc_state *crtc_state)
> >  	return true;
> >  }
> >
> > +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state
> > +*crtc_state) {
> > +	struct intel_display *display = to_intel_display(crtc_state);
> > +	struct drm_display_mode *adjusted_mode =
> > +&crtc_state->hw.adjusted_mode;
> > +
> > +	/*
> > +	 * wa_14015401596 for display versions 13, 14.
> > +	 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> register
> > +	 * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with
> VRR.
> > +	 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by
> substracting
> > +	 * crtc_vdisplay from crtc_vblank_start, so incrementing
> crtc_vblank_start
> > +	 * by 1 if both are equal.
> 
> You're just paraphrasing the code in different words here.
> Please don't, and just drop the whole comment (apart from the w/a number
> ofc).

Thanks @Syrjala, Ville for the review,

I will remove comment in next revision.

> 
> > +	 */
> > +	if (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
> > +	    adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay
> &&
> > +	    IS_DISPLAY_VER(display, 13, 14))
> > +		adjusted_mode->crtc_vblank_start += 1; }
> > +
> >  int intel_dotclock_calculate(int link_freq,
> >  			     const struct intel_link_m_n *m_n)  { diff --git
> > a/drivers/gpu/drm/i915/display/intel_display.h
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index 1f0fed5ea7bc..e6bd03ef104d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -446,6 +446,7 @@ u8 _intel_modeset_primary_pipes(const struct
> > intel_crtc_state *crtc_state);
> >  u8 _intel_modeset_secondary_pipes(const struct intel_crtc_state
> > *crtc_state);  struct intel_crtc *intel_primary_crtc(const struct
> > intel_crtc_state *crtc_state);  bool intel_crtc_get_pipe_config(struct
> > intel_crtc_state *crtc_state);
> > +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state
> > +*crtc_state);
> >  bool intel_pipe_config_compare(const struct intel_crtc_state
> *current_config,
> >  			       const struct intel_crtc_state *pipe_config,
> >  			       bool fastset);
> > --
> > 2.46.0
> 
> --
> Ville Syrjälä
> Intel