diff mbox series

[v11,2/2] drm/i915/panelreplay: Panel replay workaround with VRR

Message ID 20240916075406.3521433-3-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show
Series Vrr refactoring and panel replay workaround | expand

Commit Message

Animesh Manna Sept. 16, 2024, 7:54 a.m. UTC
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]

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 33 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_display.h |  2 ++
 2 files changed, 34 insertions(+), 1 deletion(-)

Comments

Ville Syrjälä Sept. 20, 2024, 11:39 a.m. UTC | #1
On Mon, Sep 16, 2024 at 01:24:06PM +0530, Animesh Manna wrote:
> 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]
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 33 +++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_display.h |  2 ++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 111e61eceafc..a0bd29b0d29a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2529,7 +2529,18 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
>  {
>  	struct intel_crtc_state *crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
> -	int ret;
> +	struct intel_connector *connector;
> +	struct intel_digital_connector_state *conn_state;
> +	int ret, i;
> +
> +	for_each_new_intel_connector_in_state(state, connector, conn_state, i) {
> +		struct intel_encoder *encoder = connector->encoder;
> +
> +		if (conn_state->base.crtc != &crtc->base)
> +			continue;
> +
> +		intel_crtc_adjust_vblank_delay(crtc_state, encoder);
> +	}

Why is this loop here?

>  
>  	ret = intel_dpll_crtc_compute_clock(state, crtc);
>  	if (ret)
> @@ -3940,6 +3951,26 @@ 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_encoder *encoder)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	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 (crtc_state->vrr.enable &&

Another case of the do not use.

> crtc_state->has_psr &&

Does that cover panel replay as well?

Can this change dynamically during fastsets? If yes, then you
can't use it for this, again due to fastset VRR requirements.


Did you actually test this code? AFAIK the fastset checks should
catch this and refuse to toggle VRR with a fastset. If that's not
the case then we have even bigger problems somewhere...

> +	    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 7ca26e5cb20e..db7bb5cac2f5 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -429,6 +429,8 @@ bool intel_crtc_is_joiner_primary(const struct intel_crtc_state *crtc_state);
>  u8 intel_crtc_joiner_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,
> +				    struct intel_encoder *encoder);
>  bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  			       const struct intel_crtc_state *pipe_config,
>  			       bool fastset);
> -- 
> 2.29.0
Animesh Manna Sept. 24, 2024, 10:15 a.m. UTC | #2
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Friday, September 20, 2024 5:10 PM
> To: Manna, Animesh <animesh.manna@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Hogander, Jouni <jouni.hogander@intel.com>; Murthy, Arun R
> <arun.r.murthy@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: Re: [PATCH v11 2/2] drm/i915/panelreplay: Panel replay
> workaround with VRR
> 
> On Mon, Sep 16, 2024 at 01:24:06PM +0530, Animesh Manna wrote:
> > 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]
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 33
> > +++++++++++++++++++-  drivers/gpu/drm/i915/display/intel_display.h |
> > 2 ++
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 111e61eceafc..a0bd29b0d29a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2529,7 +2529,18 @@ static int intel_crtc_compute_config(struct
> > intel_atomic_state *state,  {
> >  	struct intel_crtc_state *crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> > -	int ret;
> > +	struct intel_connector *connector;
> > +	struct intel_digital_connector_state *conn_state;
> > +	int ret, i;
> > +
> > +	for_each_new_intel_connector_in_state(state, connector,
> conn_state, i) {
> > +		struct intel_encoder *encoder = connector->encoder;
> > +
> > +		if (conn_state->base.crtc != &crtc->base)
> > +			continue;
> > +
> > +		intel_crtc_adjust_vblank_delay(crtc_state, encoder);
> > +	}
> 
> Why is this loop here?

We can drop this loop, it was added when this piece of code is added in encoder compute-config.
 
> 
> >
> >  	ret = intel_dpll_crtc_compute_clock(state, crtc);
> >  	if (ret)
> > @@ -3940,6 +3951,26 @@ 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_encoder *encoder) {
> > +	struct intel_display *display = to_intel_display(encoder);
> > +	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 (crtc_state->vrr.enable &&
> 
> Another case of the do not use.

Ok.

> 
> > crtc_state->has_psr &&
> 
> Does that cover panel replay as well?

Yes for panel replay also has_psr flag will be true.

> 
> Can this change dynamically during fastsets? If yes, then you can't use it for
> this, again due to fastset VRR requirements.
> 

has_psr flag will be set if both source and sink support panel-replay. It is applicable for psr/psr2 as well, but if vrr is enabled it will not be set. I do not see any dynamic condition will change the has_psr flag for panel-replay.
 
> 
> Did you actually test this code? AFAIK the fastset checks should catch this and
> refuse to toggle VRR with a fastset. If that's not the case then we have even
> bigger problems somewhere...

Have not tested the code due to unavailability of panel.
I will check fastest VRR path once more.

Regards,
Animesh

> 
> > +	    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 7ca26e5cb20e..db7bb5cac2f5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -429,6 +429,8 @@ bool intel_crtc_is_joiner_primary(const struct
> > intel_crtc_state *crtc_state);
> >  u8 intel_crtc_joiner_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,
> > +				    struct intel_encoder *encoder);
> >  bool intel_pipe_config_compare(const struct intel_crtc_state
> *current_config,
> >  			       const struct intel_crtc_state *pipe_config,
> >  			       bool fastset);
> > --
> > 2.29.0
> 
> --
> Ville Syrjälä
> Intel
Animesh Manna Sept. 26, 2024, 2:19 a.m. UTC | #3
> -----Original Message-----
> From: Manna, Animesh
> Sent: Tuesday, September 24, 2024 3:46 PM
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Hogander, Jouni <jouni.hogander@intel.com>; Murthy, Arun R
> <arun.r.murthy@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: RE: [PATCH v11 2/2] drm/i915/panelreplay: Panel replay
> workaround with VRR
> 
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Friday, September 20, 2024 5:10 PM
> > To: Manna, Animesh <animesh.manna@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> > <jani.nikula@intel.com>; Hogander, Jouni <jouni.hogander@intel.com>;
> > Murthy, Arun R <arun.r.murthy@intel.com>; Nautiyal, Ankit K
> > <ankit.k.nautiyal@intel.com>; Golani, Mitulkumar Ajitkumar
> > <mitulkumar.ajitkumar.golani@intel.com>
> > Subject: Re: [PATCH v11 2/2] drm/i915/panelreplay: Panel replay
> > workaround with VRR
> >
> > On Mon, Sep 16, 2024 at 01:24:06PM +0530, Animesh Manna wrote:
> > > 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]
> > >
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 33
> > > +++++++++++++++++++-  drivers/gpu/drm/i915/display/intel_display.h |
> > > 2 ++
> > >  2 files changed, 34 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 111e61eceafc..a0bd29b0d29a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -2529,7 +2529,18 @@ static int intel_crtc_compute_config(struct
> > > intel_atomic_state *state,  {
> > >  	struct intel_crtc_state *crtc_state =
> > >  		intel_atomic_get_new_crtc_state(state, crtc);
> > > -	int ret;
> > > +	struct intel_connector *connector;
> > > +	struct intel_digital_connector_state *conn_state;
> > > +	int ret, i;
> > > +
> > > +	for_each_new_intel_connector_in_state(state, connector,
> > conn_state, i) {
> > > +		struct intel_encoder *encoder = connector->encoder;
> > > +
> > > +		if (conn_state->base.crtc != &crtc->base)
> > > +			continue;
> > > +
> > > +		intel_crtc_adjust_vblank_delay(crtc_state, encoder);
> > > +	}
> >
> > Why is this loop here?
> 
> We can drop this loop, it was added when this piece of code is added in
> encoder compute-config.
> 
> >
> > >
> > >  	ret = intel_dpll_crtc_compute_clock(state, crtc);
> > >  	if (ret)
> > > @@ -3940,6 +3951,26 @@ 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_encoder *encoder) {
> > > +	struct intel_display *display = to_intel_display(encoder);
> > > +	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 (crtc_state->vrr.enable &&
> >
> > Another case of the do not use.
> 
> Ok.
> 
> >
> > > crtc_state->has_psr &&
> >
> > Does that cover panel replay as well?
> 
> Yes for panel replay also has_psr flag will be true.
> 
> >
> > Can this change dynamically during fastsets? If yes, then you can't
> > use it for this, again due to fastset VRR requirements.
> >
> 
> has_psr flag will be set if both source and sink support panel-replay. It is
> applicable for psr/psr2 as well, but if vrr is enabled it will not be set. I do not
> see any dynamic condition will change the has_psr flag for panel-replay.
> 
> >
> > Did you actually test this code? AFAIK the fastset checks should catch
> > this and refuse to toggle VRR with a fastset. If that's not the case
> > then we have even bigger problems somewhere...
> 
> Have not tested the code due to unavailability of panel.
> I will check fastest VRR path once more.

I could not find a scenario for which VRR toggling will be blocked, maybe if I missed something can you please let me know which scenario VRR toggling will be blocked.

Regards,
Animesh
> 
> Regards,
> Animesh
> 
> >
> > > +	    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 7ca26e5cb20e..db7bb5cac2f5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > > @@ -429,6 +429,8 @@ bool intel_crtc_is_joiner_primary(const struct
> > > intel_crtc_state *crtc_state);
> > >  u8 intel_crtc_joiner_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,
> > > +				    struct intel_encoder *encoder);
> > >  bool intel_pipe_config_compare(const struct intel_crtc_state
> > *current_config,
> > >  			       const struct intel_crtc_state *pipe_config,
> > >  			       bool fastset);
> > > --
> > > 2.29.0
> >
> > --
> > Ville Syrjälä
> > Intel
Mitul Golani Oct. 1, 2024, 1:55 p.m. UTC | #4
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Friday, September 20, 2024 5:10 PM
> To: Manna, Animesh <animesh.manna@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>;
> Hogander, Jouni <jouni.hogander@intel.com>; Murthy, Arun R
> <arun.r.murthy@intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>;
> Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.golani@intel.com>
> Subject: Re: [PATCH v11 2/2] drm/i915/panelreplay: Panel replay workaround
> with VRR
> 
> On Mon, Sep 16, 2024 at 01:24:06PM +0530, Animesh Manna wrote:
> > 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]
> >
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 33
> > +++++++++++++++++++-  drivers/gpu/drm/i915/display/intel_display.h |
> > 2 ++
> >  2 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 111e61eceafc..a0bd29b0d29a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2529,7 +2529,18 @@ static int intel_crtc_compute_config(struct
> > intel_atomic_state *state,  {
> >  	struct intel_crtc_state *crtc_state =
> >  		intel_atomic_get_new_crtc_state(state, crtc);
> > -	int ret;
> > +	struct intel_connector *connector;
> > +	struct intel_digital_connector_state *conn_state;
> > +	int ret, i;
> > +
> > +	for_each_new_intel_connector_in_state(state, connector, conn_state,
> i) {
> > +		struct intel_encoder *encoder = connector->encoder;
> > +
> > +		if (conn_state->base.crtc != &crtc->base)
> > +			continue;
> > +
> > +		intel_crtc_adjust_vblank_delay(crtc_state, encoder);
> > +	}
> 
> Why is this loop here?
> 
> >
> >  	ret = intel_dpll_crtc_compute_clock(state, crtc);
> >  	if (ret)
> > @@ -3940,6 +3951,26 @@ 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_encoder *encoder) {
> > +	struct intel_display *display = to_intel_display(encoder);
> > +	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 (crtc_state->vrr.enable &&
> 
> Another case of the do not use.
> 
> > crtc_state->has_psr &&
> 
> Does that cover panel replay as well?
> 
> Can this change dynamically during fastsets? If yes, then you can't use it for this,
> again due to fastset VRR requirements.
> 
> 
> Did you actually test this code? AFAIK the fastset checks should catch this and
> refuse to toggle VRR with a fastset. If that's not the case then we have even
> bigger problems somewhere...

Hello Ville,

Agreed, I have addressed suggested changes and validated on edp1.5 panel. Kept panel replay on and enabled/disabled
VRR with fastest.

https://patchwork.freedesktop.org/series/138232/#rev4

Please let me know if any test case we can try to validate.

> 
> > +	    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 7ca26e5cb20e..db7bb5cac2f5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -429,6 +429,8 @@ bool intel_crtc_is_joiner_primary(const struct
> > intel_crtc_state *crtc_state);
> >  u8 intel_crtc_joiner_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,
> > +				    struct intel_encoder *encoder);
> >  bool intel_pipe_config_compare(const struct intel_crtc_state
> *current_config,
> >  			       const struct intel_crtc_state *pipe_config,
> >  			       bool fastset);
> > --
> > 2.29.0
> 
> --
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 111e61eceafc..a0bd29b0d29a 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2529,7 +2529,18 @@  static int intel_crtc_compute_config(struct intel_atomic_state *state,
 {
 	struct intel_crtc_state *crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
-	int ret;
+	struct intel_connector *connector;
+	struct intel_digital_connector_state *conn_state;
+	int ret, i;
+
+	for_each_new_intel_connector_in_state(state, connector, conn_state, i) {
+		struct intel_encoder *encoder = connector->encoder;
+
+		if (conn_state->base.crtc != &crtc->base)
+			continue;
+
+		intel_crtc_adjust_vblank_delay(crtc_state, encoder);
+	}
 
 	ret = intel_dpll_crtc_compute_clock(state, crtc);
 	if (ret)
@@ -3940,6 +3951,26 @@  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_encoder *encoder)
+{
+	struct intel_display *display = to_intel_display(encoder);
+	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 (crtc_state->vrr.enable && 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 7ca26e5cb20e..db7bb5cac2f5 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -429,6 +429,8 @@  bool intel_crtc_is_joiner_primary(const struct intel_crtc_state *crtc_state);
 u8 intel_crtc_joiner_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,
+				    struct intel_encoder *encoder);
 bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 			       const struct intel_crtc_state *pipe_config,
 			       bool fastset);