[2/2] drm/i915/psr: Implment WA to help reach PC10

Commit Message

Suraj Kandpal June 19, 2024, 4:37 a.m. UTC

To reach PC10 when PKG_C_LATENCY is configure we must do the following
things
1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be entered
2) Allow PSR2 deep sleep when DC5 can be entered
3) DC5 can be entered when all transocoder have either PSR1, PSR2 or
eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes are
not happening.

--v2
-Switch condition and do an early return [Jani]
-Do some checks in compute_config [Jani]
-Do not use register reads as a method of checking states for
DPKGC or delayed vblank [Jani]
-Use another way to see is vblank interrupts are disabled or not [Jani]

WA: 16023497226
Signed-off-by: Suraj Kandpal <[e-mail]>
---
 .../drm/i915/display/intel_display_types.h    |  2 +
 drivers/gpu/drm/i915/display/intel_psr.c      | 82 ++++++++++++++++++-
 2 files changed, 83 insertions(+), 1 deletion(-)

Comments

Shankar, Uma Aug. 22, 2024, 5:19 a.m. UTC | #1

> -----Original Message-----
> From: Intel-gfx <[e-mail]> On Behalf Of Suraj
> Kandpal
> Sent: Wednesday, June 19, 2024 10:08 AM
> To: [e-mail]
> Cc: Manna, Animesh <[e-mail]>; Murthy, Arun R
> <[e-mail]>; Hogander, Jouni <[e-mail]>;
> [e-mail]; Kandpal, Suraj <[e-mail]>
> Subject: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10

Nit: Typo in Implement

> To reach PC10 when PKG_C_LATENCY is configure we must do the following
> things
> 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be entered
> 2) Allow PSR2 deep sleep when DC5 can be entered
> 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or eDP 1.5 PR
> ALPM enabled and VBI is disabled and flips and pushes are not happening.
> 
> --v2
> -Switch condition and do an early return [Jani] -Do some checks in
> compute_config [Jani] -Do not use register reads as a method of checking states
> for DPKGC or delayed vblank [Jani] -Use another way to see is vblank interrupts
> are disabled or not [Jani]
> 
> WA: 16023497226
> Signed-off-by: Suraj Kandpal <[e-mail]>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 +
>  drivers/gpu/drm/i915/display/intel_psr.c      | 82 ++++++++++++++++++-
>  2 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 46b3cbeb4a82..031f8e889b65 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1708,6 +1708,8 @@ struct intel_psr {
>  	bool sink_support;
>  	bool source_support;
>  	bool enabled;
> +	bool delayed_vblank;
> +	bool is_dpkgc_configured;
>  	bool paused;
>  	enum pipe pipe;
>  	enum transcoder transcoder;
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 080bf5e51148..4ddea6737386 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -808,6 +808,73 @@ static u8 psr_compute_idle_frames(struct intel_dp
> *intel_dp)
>  	return idle_frames;
>  }
> 
> +static bool intel_psr_check_delayed_vblank_limit(struct
> +intel_crtc_state *crtc_state) {
> +	struct drm_display_mode *adjusted_mode =
> +&crtc_state->hw.adjusted_mode;
> +
> +	return (adjusted_mode->crtc_vblank_start -
> +adjusted_mode->crtc_vdisplay) >= 6; }
> +
> +/*
> + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and
> + * VRR is not enabled
> + */
> +static bool intel_psr_is_dpkgc_configured(struct drm_i915_private
> +*i915) {
> +	struct intel_crtc *intel_crtc;
> +
> +	if (DISPLAY_VER(i915) < 20)
> +		return false;
> +
> +	for_each_intel_crtc(&i915->drm, intel_crtc) {
> +		struct intel_crtc_state *crtc_state;
> +
> +		if (!intel_crtc->active)
> +			continue;
> +
> +		crtc_state = intel_crtc->config;
> +
> +		if (crtc_state->vrr.enable)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * DC5 entry is only possible if vblank interrupt is disabled
> + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
> + * enabled encoders.
> + */
> +static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private
> +*i915) {
> +	struct intel_crtc *intel_crtc;
> +
> +	for_each_intel_crtc(&i915->drm, intel_crtc) {
> +		struct intel_encoder *encoder;
> +		struct drm_crtc *crtc = &intel_crtc->base;
> +		struct drm_vblank_crtc *vblank;
> +
> +		if (!intel_crtc->active)
> +			continue;
> +
> +		vblank = drm_crtc_vblank_crtc(crtc);
> +
> +		if (vblank->enabled)
> +			return false;
> +
> +		for_each_encoder_on_crtc(&i915->drm, crtc, encoder) {
> +			struct intel_dp *intel_dp = enc_to_intel_dp(encoder);

All encoders on crtc may not be of type DP, need to be handled here.

> +			struct intel_psr *psr = &intel_dp->psr;
> +
> +			if (!psr->enabled)
> +				return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  static bool hsw_activate_psr1(struct intel_dp *intel_dp)  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); @@ -815,6
> +882,14 @@ static bool hsw_activate_psr1(struct intel_dp *intel_dp)
>  	u32 max_sleep_time = 0x1f;
>  	u32 val = EDP_PSR_ENABLE;
> 
> +	/* WA: 16023497226*/
> +	if (intel_dp->psr.is_dpkgc_configured &&
> +	    (intel_dp->psr.delayed_vblank ||
> intel_psr_is_dc5_entry_possible(dev_priv))) {

In intel_psr_is_dc5_entry_possible function we use psr->enabled and based on that deciding
to return from PSR1 activate, logic looks suspicious. Can you re-check once.

> +		drm_dbg_kms(&dev_priv->drm,
> +			    "PSR1 not activated as it doesn't meet requirements
> of WA:16023497226\n");
> +		return false;
> +	}
> +
>  	val |= EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> 
>  	if (DISPLAY_VER(dev_priv) < 20)
> @@ -907,7 +982,10 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	u32 val = EDP_PSR2_ENABLE;
>  	u32 psr_val = 0;
> 
> -	val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> +	/* WA: 16023497226*/
> +	if (intel_dp->psr.is_dpkgc_configured &&
> +	    intel_psr_is_dc5_entry_possible(dev_priv))
> +		val |=
> EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> 
>  	if (DISPLAY_VER(dev_priv) < 14 && !IS_ALDERLAKE_P(dev_priv))
>  		val |= EDP_SU_TRACK_ENABLE;
> @@ -1502,6 +1580,8 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>  		return;
> 
>  	crtc_state->has_sel_update = intel_sel_update_config_valid(intel_dp,
> crtc_state);
> +	intel_dp->psr.delayed_vblank =
> intel_psr_check_delayed_vblank_limit(crtc_state);
> +	intel_dp->psr.is_dpkgc_configured =
> +intel_psr_is_dpkgc_configured(dev_priv);
>  }
> 
>  void intel_psr_get_config(struct intel_encoder *encoder,
> --
> 2.43.2

Suraj Kandpal Aug. 22, 2024, 6:25 a.m. UTC | #2

> -----Original Message-----
> From: Shankar, Uma <[e-mail]>
> Sent: Thursday, August 22, 2024 10:50 AM
> To: Kandpal, Suraj <[e-mail]>; intel-
> [e-mail]
> Cc: Manna, Animesh <[e-mail]>; Murthy, Arun R
> <[e-mail]>; Hogander, Jouni <[e-mail]>;
> [e-mail]; Kandpal, Suraj <[e-mail]>
> Subject: RE: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
> 
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <[e-mail]> On Behalf Of
> > Suraj Kandpal
> > Sent: Wednesday, June 19, 2024 10:08 AM
> > To: [e-mail]
> > Cc: Manna, Animesh <[e-mail]>; Murthy, Arun R
> > <[e-mail]>; Hogander, Jouni
> <[e-mail]>;
> > [e-mail]; Kandpal, Suraj <[e-mail]>
> > Subject: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
> 
> Nit: Typo in Implement
> 

Wil fix.
> > To reach PC10 when PKG_C_LATENCY is configure we must do the
> following
> > things
> > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be
> > entered
> > 2) Allow PSR2 deep sleep when DC5 can be entered
> > 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or
> > eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes are not
> happening.
> >
> > --v2
> > -Switch condition and do an early return [Jani] -Do some checks in
> > compute_config [Jani] -Do not use register reads as a method of
> > checking states for DPKGC or delayed vblank [Jani] -Use another way to
> > see is vblank interrupts are disabled or not [Jani]
> >
> > WA: 16023497226
> > Signed-off-by: Suraj Kandpal <[e-mail]>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 +
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 82 ++++++++++++++++++-
> >  2 files changed, 83 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 46b3cbeb4a82..031f8e889b65 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1708,6 +1708,8 @@ struct intel_psr {
> >  	bool sink_support;
> >  	bool source_support;
> >  	bool enabled;
> > +	bool delayed_vblank;
> > +	bool is_dpkgc_configured;
> >  	bool paused;
> >  	enum pipe pipe;
> >  	enum transcoder transcoder;
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 080bf5e51148..4ddea6737386 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -808,6 +808,73 @@ static u8 psr_compute_idle_frames(struct
> intel_dp
> > *intel_dp)
> >  	return idle_frames;
> >  }
> >
> > +static bool intel_psr_check_delayed_vblank_limit(struct
> > +intel_crtc_state *crtc_state) {
> > +	struct drm_display_mode *adjusted_mode =
> > +&crtc_state->hw.adjusted_mode;
> > +
> > +	return (adjusted_mode->crtc_vblank_start -
> > +adjusted_mode->crtc_vdisplay) >= 6; }
> > +
> > +/*
> > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and
> > + * VRR is not enabled
> > + */
> > +static bool intel_psr_is_dpkgc_configured(struct drm_i915_private
> > +*i915) {
> > +	struct intel_crtc *intel_crtc;
> > +
> > +	if (DISPLAY_VER(i915) < 20)
> > +		return false;
> > +
> > +	for_each_intel_crtc(&i915->drm, intel_crtc) {
> > +		struct intel_crtc_state *crtc_state;
> > +
> > +		if (!intel_crtc->active)
> > +			continue;
> > +
> > +		crtc_state = intel_crtc->config;
> > +
> > +		if (crtc_state->vrr.enable)
> > +			return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * DC5 entry is only possible if vblank interrupt is disabled
> > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
> > + * enabled encoders.
> > + */
> > +static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private
> > +*i915) {
> > +	struct intel_crtc *intel_crtc;
> > +
> > +	for_each_intel_crtc(&i915->drm, intel_crtc) {
> > +		struct intel_encoder *encoder;
> > +		struct drm_crtc *crtc = &intel_crtc->base;
> > +		struct drm_vblank_crtc *vblank;
> > +
> > +		if (!intel_crtc->active)
> > +			continue;
> > +
> > +		vblank = drm_crtc_vblank_crtc(crtc);
> > +
> > +		if (vblank->enabled)
> > +			return false;
> > +
> > +		for_each_encoder_on_crtc(&i915->drm, crtc, encoder) {
> > +			struct intel_dp *intel_dp =
> enc_to_intel_dp(encoder);
> 
> All encoders on crtc may not be of type DP, need to be handled here.
> 

Ahh ohkay will add a null check for intel_dp here and continue based on that

> > +			struct intel_psr *psr = &intel_dp->psr;
> > +
> > +			if (!psr->enabled)
> > +				return false;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  static bool hsw_activate_psr1(struct intel_dp *intel_dp)  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); @@ -
> 815,6
> > +882,14 @@ static bool hsw_activate_psr1(struct intel_dp *intel_dp)
> >  	u32 max_sleep_time = 0x1f;
> >  	u32 val = EDP_PSR_ENABLE;
> >
> > +	/* WA: 16023497226*/
> > +	if (intel_dp->psr.is_dpkgc_configured &&
> > +	    (intel_dp->psr.delayed_vblank ||
> > intel_psr_is_dc5_entry_possible(dev_priv))) {
> 
> In intel_psr_is_dc5_entry_possible function we use psr->enabled and based
> on that deciding to return from PSR1 activate, logic looks suspicious. Can
> you re-check once.

So this is so we can see if psr can be enabled on that encoder or not which will indicate we
We can enter dc5 or not and if we can then not to activate psr1 incase dpkgc is configured
 
Regards,
Suraj Kandpal

> 
> > +		drm_dbg_kms(&dev_priv->drm,
> > +			    "PSR1 not activated as it doesn't meet
> requirements
> > of WA:16023497226\n");
> > +		return false;
> > +	}
> > +
> >  	val |= EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> >
> >  	if (DISPLAY_VER(dev_priv) < 20)
> > @@ -907,7 +982,10 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
> >  	u32 val = EDP_PSR2_ENABLE;
> >  	u32 psr_val = 0;
> >
> > -	val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > +	/* WA: 16023497226*/
> > +	if (intel_dp->psr.is_dpkgc_configured &&
> > +	    intel_psr_is_dc5_entry_possible(dev_priv))
> > +		val |=
> > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> >
> >  	if (DISPLAY_VER(dev_priv) < 14 && !IS_ALDERLAKE_P(dev_priv))
> >  		val |= EDP_SU_TRACK_ENABLE;
> > @@ -1502,6 +1580,8 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  		return;
> >
> >  	crtc_state->has_sel_update = intel_sel_update_config_valid(intel_dp,
> > crtc_state);
> > +	intel_dp->psr.delayed_vblank =
> > intel_psr_check_delayed_vblank_limit(crtc_state);
> > +	intel_dp->psr.is_dpkgc_configured =
> > +intel_psr_is_dpkgc_configured(dev_priv);
> >  }
> >
> >  void intel_psr_get_config(struct intel_encoder *encoder,
> > --
> > 2.43.2

Jouni Högander Aug. 22, 2024, 8:45 a.m. UTC | #3

On Wed, 2024-06-19 at 10:07 +0530, Suraj Kandpal wrote:
> To reach PC10 when PKG_C_LATENCY is configure we must do the
> following
> things
> 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be
> entered
> 2) Allow PSR2 deep sleep when DC5 can be entered
> 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or
> eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes are
> not happening.
> 
> --v2
> -Switch condition and do an early return [Jani]
> -Do some checks in compute_config [Jani]
> -Do not use register reads as a method of checking states for
> DPKGC or delayed vblank [Jani]
> -Use another way to see is vblank interrupts are disabled or not
> [Jani]
> 
> WA: 16023497226
> Signed-off-by: Suraj Kandpal <[e-mail]>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 +
>  drivers/gpu/drm/i915/display/intel_psr.c      | 82
> ++++++++++++++++++-
>  2 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 46b3cbeb4a82..031f8e889b65 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1708,6 +1708,8 @@ struct intel_psr {
>         bool sink_support;
>         bool source_support;
>         bool enabled;
> +       bool delayed_vblank;
> +       bool is_dpkgc_configured;
>         bool paused;
>         enum pipe pipe;
>         enum transcoder transcoder;
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 080bf5e51148..4ddea6737386 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -808,6 +808,73 @@ static u8 psr_compute_idle_frames(struct
> intel_dp *intel_dp)
>         return idle_frames;
>  }
>  
> +static bool intel_psr_check_delayed_vblank_limit(struct
> intel_crtc_state *crtc_state)
> +{
> +       struct drm_display_mode *adjusted_mode = &crtc_state-
> >hw.adjusted_mode;
> +
> +       return (adjusted_mode->crtc_vblank_start - adjusted_mode-
> >crtc_vdisplay) >= 6;
> +}
> +
> +/*
> + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and
> + * VRR is not enabled
> + */
> +static bool intel_psr_is_dpkgc_configured(struct drm_i915_private
> *i915)
> +{
> +       struct intel_crtc *intel_crtc;
> +
> +       if (DISPLAY_VER(i915) < 20)
> +               return false;
> +
> +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> +               struct intel_crtc_state *crtc_state;
> +
> +               if (!intel_crtc->active)
> +                       continue;
> +
> +               crtc_state = intel_crtc->config;
> +
> +               if (crtc_state->vrr.enable)
> +                       return false;
> +       }
> +
> +       return true;
> +}
> +
> +/*
> + * DC5 entry is only possible if vblank interrupt is disabled
> + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
> + * enabled encoders.
> + */
> +static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private
> *i915)
> +{
> +       struct intel_crtc *intel_crtc;
> +
> +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> +               struct intel_encoder *encoder;
> +               struct drm_crtc *crtc = &intel_crtc->base;
> +               struct drm_vblank_crtc *vblank;
> +
> +               if (!intel_crtc->active)
> +                       continue;
> +
> +               vblank = drm_crtc_vblank_crtc(crtc);
> +
> +               if (vblank->enabled)
> +                       return false;
> +
> +               for_each_encoder_on_crtc(&i915->drm, crtc, encoder) {
> +                       struct intel_dp *intel_dp =
> enc_to_intel_dp(encoder);
> +                       struct intel_psr *psr = &intel_dp->psr;
> +
> +                       if (!psr->enabled)
> +                               return false;
> +               }
> +       }
> +
> +       return true;
> +}
> +
>  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
>  {
>         struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> @@ -815,6 +882,14 @@ static bool hsw_activate_psr1(struct intel_dp
> *intel_dp)
>         u32 max_sleep_time = 0x1f;
>         u32 val = EDP_PSR_ENABLE;
>  
> +       /* WA: 16023497226*/
> +       if (intel_dp->psr.is_dpkgc_configured &&
> +           (intel_dp->psr.delayed_vblank ||
> intel_psr_is_dc5_entry_possible(dev_priv))) {
> +               drm_dbg_kms(&dev_priv->drm,
> +                           "PSR1 not activated as it doesn't meet
> requirements of WA:16023497226\n");
> +               return false;
> +       }
> +

I would recommend doing this in intel_psr_compute_config as a last step
and drop patch 1. Doing it this way would be safer as it's not opening
new sequence/state where psr.enabled = true and psr.active = false
after intel_psr_enable_locked.

BR,

Jouni Högander

>         val |=
> EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
>  
>         if (DISPLAY_VER(dev_priv) < 20)
> @@ -907,7 +982,10 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
>         u32 val = EDP_PSR2_ENABLE;
>         u32 psr_val = 0;
>  
> -       val |=
> EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> +       /* WA: 16023497226*/
> +       if (intel_dp->psr.is_dpkgc_configured &&
> +           intel_psr_is_dc5_entry_possible(dev_priv))
> +               val |=
> EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
>  
>         if (DISPLAY_VER(dev_priv) < 14 && !IS_ALDERLAKE_P(dev_priv))
>                 val |= EDP_SU_TRACK_ENABLE;
> @@ -1502,6 +1580,8 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>                 return;
>  
>         crtc_state->has_sel_update =
> intel_sel_update_config_valid(intel_dp, crtc_state);
> +       intel_dp->psr.delayed_vblank =
> intel_psr_check_delayed_vblank_limit(crtc_state);
> +       intel_dp->psr.is_dpkgc_configured =
> intel_psr_is_dpkgc_configured(dev_priv);
>  }
>  
>  void intel_psr_get_config(struct intel_encoder *encoder,

Suraj Kandpal Aug. 23, 2024, 4:54 a.m. UTC | #4

> -----Original Message-----
> From: Hogander, Jouni <[e-mail]>
> Sent: Thursday, August 22, 2024 2:16 PM
> To: Kandpal, Suraj <[e-mail]>; intel-
> [e-mail]
> Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
> 
> On Wed, 2024-06-19 at 10:07 +0530, Suraj Kandpal wrote:
> > To reach PC10 when PKG_C_LATENCY is configure we must do the
> following
> > things
> > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be
> > entered
> > 2) Allow PSR2 deep sleep when DC5 can be entered
> > 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or
> > eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes are
> > not happening.
> >
> > --v2
> > -Switch condition and do an early return [Jani] -Do some checks in
> > compute_config [Jani] -Do not use register reads as a method of
> > checking states for DPKGC or delayed vblank [Jani] -Use another way to
> > see is vblank interrupts are disabled or not [Jani]
> >
> > WA: 16023497226
> > Signed-off-by: Suraj Kandpal <[e-mail]>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 +
> >  drivers/gpu/drm/i915/display/intel_psr.c      | 82
> > ++++++++++++++++++-
> >  2 files changed, 83 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 46b3cbeb4a82..031f8e889b65 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1708,6 +1708,8 @@ struct intel_psr {
> >         bool sink_support;
> >         bool source_support;
> >         bool enabled;
> > +       bool delayed_vblank;
> > +       bool is_dpkgc_configured;
> >         bool paused;
> >         enum pipe pipe;
> >         enum transcoder transcoder;
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 080bf5e51148..4ddea6737386 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -808,6 +808,73 @@ static u8 psr_compute_idle_frames(struct
> intel_dp
> > *intel_dp)
> >         return idle_frames;
> >  }
> >
> > +static bool intel_psr_check_delayed_vblank_limit(struct
> > intel_crtc_state *crtc_state)
> > +{
> > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> > >hw.adjusted_mode;
> > +
> > +       return (adjusted_mode->crtc_vblank_start - adjusted_mode-
> > >crtc_vdisplay) >= 6;
> > +}
> > +
> > +/*
> > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and
> > + * VRR is not enabled
> > + */
> > +static bool intel_psr_is_dpkgc_configured(struct drm_i915_private
> > *i915)
> > +{
> > +       struct intel_crtc *intel_crtc;
> > +
> > +       if (DISPLAY_VER(i915) < 20)
> > +               return false;
> > +
> > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > +               struct intel_crtc_state *crtc_state;
> > +
> > +               if (!intel_crtc->active)
> > +                       continue;
> > +
> > +               crtc_state = intel_crtc->config;
> > +
> > +               if (crtc_state->vrr.enable)
> > +                       return false;
> > +       }
> > +
> > +       return true;
> > +}
> > +
> > +/*
> > + * DC5 entry is only possible if vblank interrupt is disabled
> > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
> > + * enabled encoders.
> > + */
> > +static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private
> > *i915)
> > +{
> > +       struct intel_crtc *intel_crtc;
> > +
> > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > +               struct intel_encoder *encoder;
> > +               struct drm_crtc *crtc = &intel_crtc->base;
> > +               struct drm_vblank_crtc *vblank;
> > +
> > +               if (!intel_crtc->active)
> > +                       continue;
> > +
> > +               vblank = drm_crtc_vblank_crtc(crtc);
> > +
> > +               if (vblank->enabled)
> > +                       return false;
> > +
> > +               for_each_encoder_on_crtc(&i915->drm, crtc, encoder) {
> > +                       struct intel_dp *intel_dp =
> > enc_to_intel_dp(encoder);
> > +                       struct intel_psr *psr = &intel_dp->psr;
> > +
> > +                       if (!psr->enabled)
> > +                               return false;
> > +               }
> > +       }
> > +
> > +       return true;
> > +}
> > +
> >  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
> >  {
> >         struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); @@
> > -815,6 +882,14 @@ static bool hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >         u32 max_sleep_time = 0x1f;
> >         u32 val = EDP_PSR_ENABLE;
> >
> > +       /* WA: 16023497226*/
> > +       if (intel_dp->psr.is_dpkgc_configured &&
> > +           (intel_dp->psr.delayed_vblank ||
> > intel_psr_is_dc5_entry_possible(dev_priv))) {
> > +               drm_dbg_kms(&dev_priv->drm,
> > +                           "PSR1 not activated as it doesn't meet
> > requirements of WA:16023497226\n");
> > +               return false;
> > +       }
> > +
> 
> I would recommend doing this in intel_psr_compute_config as a last step
> and drop patch 1. Doing it this way would be safer as it's not opening new
> sequence/state where psr.enabled = true and psr.active = false after
> intel_psr_enable_locked.

The reason for this was I wanted to disable only psr1 based on if dc5 entry is possible or not.
Even if I call the dc5_entry_is_possible function from compute_config and save it in the intel_psr
state we would still end up with the seq psr.enabled = true and psr.active = false unless you see a
param which will only activate psr2 and not psr1 in such scenario ?

Regards,
Suraj Kandpal

> 
> BR,
> 
> Jouni Högander
> 
> >         val |=
> > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> >
> >         if (DISPLAY_VER(dev_priv) < 20) @@ -907,7 +982,10 @@ static
> > void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >         u32 val = EDP_PSR2_ENABLE;
> >         u32 psr_val = 0;
> >
> > -       val |=
> > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > +       /* WA: 16023497226*/
> > +       if (intel_dp->psr.is_dpkgc_configured &&
> > +           intel_psr_is_dc5_entry_possible(dev_priv))
> > +               val |=
> > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> >
> >         if (DISPLAY_VER(dev_priv) < 14 && !IS_ALDERLAKE_P(dev_priv))
> >                 val |= EDP_SU_TRACK_ENABLE; @@ -1502,6 +1580,8 @@ void
> > intel_psr_compute_config(struct intel_dp *intel_dp,
> >                 return;
> >
> >         crtc_state->has_sel_update =
> > intel_sel_update_config_valid(intel_dp, crtc_state);
> > +       intel_dp->psr.delayed_vblank =
> > intel_psr_check_delayed_vblank_limit(crtc_state);
> > +       intel_dp->psr.is_dpkgc_configured =
> > intel_psr_is_dpkgc_configured(dev_priv);
> >  }
> >
> >  void intel_psr_get_config(struct intel_encoder *encoder,

Jouni Högander Aug. 23, 2024, 5:24 a.m. UTC | #5

On Fri, 2024-08-23 at 04:54 +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <[e-mail]>
> > Sent: Thursday, August 22, 2024 2:16 PM
> > To: Kandpal, Suraj <[e-mail]>; intel-
> > [e-mail]
> > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach
> > PC10
> > 
> > On Wed, 2024-06-19 at 10:07 +0530, Suraj Kandpal wrote:
> > > To reach PC10 when PKG_C_LATENCY is configure we must do the
> > following
> > > things
> > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be
> > > entered
> > > 2) Allow PSR2 deep sleep when DC5 can be entered
> > > 3) DC5 can be entered when all transocoder have either PSR1, PSR2
> > > or
> > > eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes
> > > are
> > > not happening.
> > > 
> > > --v2
> > > -Switch condition and do an early return [Jani] -Do some checks
> > > in
> > > compute_config [Jani] -Do not use register reads as a method of
> > > checking states for DPKGC or delayed vblank [Jani] -Use another
> > > way to
> > > see is vblank interrupts are disabled or not [Jani]
> > > 
> > > WA: 16023497226
> > > Signed-off-by: Suraj Kandpal <[e-mail]>
> > > ---
> > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > >  drivers/gpu/drm/i915/display/intel_psr.c      | 82
> > > ++++++++++++++++++-
> > >  2 files changed, 83 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 46b3cbeb4a82..031f8e889b65 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1708,6 +1708,8 @@ struct intel_psr {
> > >         bool sink_support;
> > >         bool source_support;
> > >         bool enabled;
> > > +       bool delayed_vblank;
> > > +       bool is_dpkgc_configured;
> > >         bool paused;
> > >         enum pipe pipe;
> > >         enum transcoder transcoder;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 080bf5e51148..4ddea6737386 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -808,6 +808,73 @@ static u8 psr_compute_idle_frames(struct
> > intel_dp
> > > *intel_dp)
> > >         return idle_frames;
> > >  }
> > > 
> > > +static bool intel_psr_check_delayed_vblank_limit(struct
> > > intel_crtc_state *crtc_state)
> > > +{
> > > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> > > > hw.adjusted_mode;
> > > +
> > > +       return (adjusted_mode->crtc_vblank_start - adjusted_mode-
> > > > crtc_vdisplay) >= 6;
> > > +}
> > > +
> > > +/*
> > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and
> > > + * VRR is not enabled
> > > + */
> > > +static bool intel_psr_is_dpkgc_configured(struct
> > > drm_i915_private
> > > *i915)
> > > +{
> > > +       struct intel_crtc *intel_crtc;
> > > +
> > > +       if (DISPLAY_VER(i915) < 20)
> > > +               return false;
> > > +
> > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > +               struct intel_crtc_state *crtc_state;
> > > +
> > > +               if (!intel_crtc->active)
> > > +                       continue;
> > > +
> > > +               crtc_state = intel_crtc->config;
> > > +
> > > +               if (crtc_state->vrr.enable)
> > > +                       return false;
> > > +       }
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +/*
> > > + * DC5 entry is only possible if vblank interrupt is disabled
> > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
> > > + * enabled encoders.
> > > + */
> > > +static bool intel_psr_is_dc5_entry_possible(struct
> > > drm_i915_private
> > > *i915)
> > > +{
> > > +       struct intel_crtc *intel_crtc;
> > > +
> > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > +               struct intel_encoder *encoder;
> > > +               struct drm_crtc *crtc = &intel_crtc->base;
> > > +               struct drm_vblank_crtc *vblank;
> > > +
> > > +               if (!intel_crtc->active)
> > > +                       continue;
> > > +
> > > +               vblank = drm_crtc_vblank_crtc(crtc);
> > > +
> > > +               if (vblank->enabled)
> > > +                       return false;
> > > +
> > > +               for_each_encoder_on_crtc(&i915->drm, crtc,
> > > encoder) {
> > > +                       struct intel_dp *intel_dp =
> > > enc_to_intel_dp(encoder);
> > > +                       struct intel_psr *psr = &intel_dp->psr;
> > > +
> > > +                       if (!psr->enabled)
> > > +                               return false;
> > > +               }
> > > +       }
> > > +
> > > +       return true;
> > > +}
> > > +
> > >  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
> > >  {
> > >         struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > @@
> > > -815,6 +882,14 @@ static bool hsw_activate_psr1(struct intel_dp
> > > *intel_dp)
> > >         u32 max_sleep_time = 0x1f;
> > >         u32 val = EDP_PSR_ENABLE;
> > > 
> > > +       /* WA: 16023497226*/
> > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > +           (intel_dp->psr.delayed_vblank ||
> > > intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > +               drm_dbg_kms(&dev_priv->drm,
> > > +                           "PSR1 not activated as it doesn't
> > > meet
> > > requirements of WA:16023497226\n");
> > > +               return false;
> > > +       }
> > > +
> > 
> > I would recommend doing this in intel_psr_compute_config as a last
> > step
> > and drop patch 1. Doing it this way would be safer as it's not
> > opening new
> > sequence/state where psr.enabled = true and psr.active = false
> > after
> > intel_psr_enable_locked.
> 
> The reason for this was I wanted to disable only psr1 based on if dc5
> entry is possible or not.
> Even if I call the dc5_entry_is_possible function from compute_config
> and save it in the intel_psr
> state we would still end up with the seq psr.enabled = true and
> psr.active = false unless you see a
> param which will only activate psr2 and not psr1 in such scenario ?
> 

I was thinking doing it like this:

+static void wa_16023497226(struct intel_crtc_state * crtc_state)
+{
+	/* PSR2 not handled here. Wa not needed for Panel Replay */
+	if (crtc_state->has_sel_update || crtc_state-
>has_panel_replay)
+	    return;
+	
+       if (intel_dp->psr.is_dpkgc_configured &&
+           (intel_dp->psr.delayed_vblank ||
+	    intel_psr_is_dc5_entry_possible(dev_priv))) {
+               drm_dbg_kms(&dev_priv->drm,
+                           "PSR1 not enabled as it doesn't meet
+			   requirements of WA:16023497226\n");
+               crtc_state->has_psr = false;
+       }
+}
+
 void intel_psr_compute_config(struct intel_dp *intel_dp,
 			      struct intel_crtc_state *crtc_state,
 			      struct drm_connector_state *conn_state)
@@ -1635,6 +1651,8 @@ void intel_psr_compute_config(struct intel_dp
*intel_dp,
 		return;
 
 	crtc_state->has_sel_update =
intel_sel_update_config_valid(intel_dp, crtc_state);
+
+	wa_16023497226(crtc_state);
 }
 
 void intel_psr_get_config(struct intel_encoder *encoder,

Do you see this would be possible? Current PSR2 handling in your
patches is ok to me.

BR,

Jouni Högander

> Regards,
> Suraj Kandpal
> 
> > 
> > BR,
> > 
> > Jouni Högander
> > 
> > >         val |=
> > > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > 
> > >         if (DISPLAY_VER(dev_priv) < 20) @@ -907,7 +982,10 @@
> > > static
> > > void hsw_activate_psr2(struct intel_dp
> > > *intel_dp)
> > >         u32 val = EDP_PSR2_ENABLE;
> > >         u32 psr_val = 0;
> > > 
> > > -       val |=
> > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > +       /* WA: 16023497226*/
> > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > +           intel_psr_is_dc5_entry_possible(dev_priv))
> > > +               val |=
> > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > 
> > >         if (DISPLAY_VER(dev_priv) < 14 &&
> > > !IS_ALDERLAKE_P(dev_priv))
> > >                 val |= EDP_SU_TRACK_ENABLE; @@ -1502,6 +1580,8 @@
> > > void
> > > intel_psr_compute_config(struct intel_dp *intel_dp,
> > >                 return;
> > > 
> > >         crtc_state->has_sel_update =
> > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > +       intel_dp->psr.delayed_vblank =
> > > intel_psr_check_delayed_vblank_limit(crtc_state);
> > > +       intel_dp->psr.is_dpkgc_configured =
> > > intel_psr_is_dpkgc_configured(dev_priv);
> > >  }
> > > 
> > >  void intel_psr_get_config(struct intel_encoder *encoder,
>

Suraj Kandpal Aug. 23, 2024, 6:18 a.m. UTC | #6

> -----Original Message-----
> From: Hogander, Jouni <[e-mail]>
> Sent: Friday, August 23, 2024 10:54 AM
> To: Kandpal, Suraj <[e-mail]>; [e-mail]
> Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
> 
> On Fri, 2024-08-23 at 04:54 +0000, Kandpal, Suraj wrote:
> >
> >
> > > -----Original Message-----
> > > From: Hogander, Jouni <[e-mail]>
> > > Sent: Thursday, August 22, 2024 2:16 PM
> > > To: Kandpal, Suraj <[e-mail]>; intel-
> > > [e-mail]
> > > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach
> > > PC10
> > >
> > > On Wed, 2024-06-19 at 10:07 +0530, Suraj Kandpal wrote:
> > > > To reach PC10 when PKG_C_LATENCY is configure we must do the
> > > following
> > > > things
> > > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be
> > > > entered
> > > > 2) Allow PSR2 deep sleep when DC5 can be entered
> > > > 3) DC5 can be entered when all transocoder have either PSR1, PSR2
> > > > or eDP 1.5 PR ALPM enabled and VBI is disabled and flips and
> > > > pushes are not happening.
> > > >
> > > > --v2
> > > > -Switch condition and do an early return [Jani] -Do some checks in
> > > > compute_config [Jani] -Do not use register reads as a method of
> > > > checking states for DPKGC or delayed vblank [Jani] -Use another
> > > > way to see is vblank interrupts are disabled or not [Jani]
> > > >
> > > > WA: 16023497226
> > > > Signed-off-by: Suraj Kandpal <[e-mail]>
> > > > ---
> > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 82
> > > > ++++++++++++++++++-
> > > >  2 files changed, 83 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 46b3cbeb4a82..031f8e889b65 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1708,6 +1708,8 @@ struct intel_psr {
> > > >         bool sink_support;
> > > >         bool source_support;
> > > >         bool enabled;
> > > > +       bool delayed_vblank;
> > > > +       bool is_dpkgc_configured;
> > > >         bool paused;
> > > >         enum pipe pipe;
> > > >         enum transcoder transcoder; diff --git
> > > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 080bf5e51148..4ddea6737386 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -808,6 +808,73 @@ static u8 psr_compute_idle_frames(struct
> > > intel_dp
> > > > *intel_dp)
> > > >         return idle_frames;
> > > >  }
> > > >
> > > > +static bool intel_psr_check_delayed_vblank_limit(struct
> > > > intel_crtc_state *crtc_state)
> > > > +{
> > > > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> > > > > hw.adjusted_mode;
> > > > +
> > > > +       return (adjusted_mode->crtc_vblank_start - adjusted_mode-
> > > > > crtc_vdisplay) >= 6;
> > > > +}
> > > > +
> > > > +/*
> > > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and
> > > > + * VRR is not enabled
> > > > + */
> > > > +static bool intel_psr_is_dpkgc_configured(struct
> > > > drm_i915_private
> > > > *i915)
> > > > +{
> > > > +       struct intel_crtc *intel_crtc;
> > > > +
> > > > +       if (DISPLAY_VER(i915) < 20)
> > > > +               return false;
> > > > +
> > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > +               struct intel_crtc_state *crtc_state;
> > > > +
> > > > +               if (!intel_crtc->active)
> > > > +                       continue;
> > > > +
> > > > +               crtc_state = intel_crtc->config;
> > > > +
> > > > +               if (crtc_state->vrr.enable)
> > > > +                       return false;
> > > > +       }
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * DC5 entry is only possible if vblank interrupt is disabled
> > > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
> > > > + * enabled encoders.
> > > > + */
> > > > +static bool intel_psr_is_dc5_entry_possible(struct
> > > > drm_i915_private
> > > > *i915)
> > > > +{
> > > > +       struct intel_crtc *intel_crtc;
> > > > +
> > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > +               struct intel_encoder *encoder;
> > > > +               struct drm_crtc *crtc = &intel_crtc->base;
> > > > +               struct drm_vblank_crtc *vblank;
> > > > +
> > > > +               if (!intel_crtc->active)
> > > > +                       continue;
> > > > +
> > > > +               vblank = drm_crtc_vblank_crtc(crtc);
> > > > +
> > > > +               if (vblank->enabled)
> > > > +                       return false;
> > > > +
> > > > +               for_each_encoder_on_crtc(&i915->drm, crtc,
> > > > encoder) {
> > > > +                       struct intel_dp *intel_dp =
> > > > enc_to_intel_dp(encoder);
> > > > +                       struct intel_psr *psr = &intel_dp->psr;
> > > > +
> > > > +                       if (!psr->enabled)
> > > > +                               return false;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > >  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
> > > >  {
> > > >         struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > @@
> > > > -815,6 +882,14 @@ static bool hsw_activate_psr1(struct intel_dp
> > > > *intel_dp)
> > > >         u32 max_sleep_time = 0x1f;
> > > >         u32 val = EDP_PSR_ENABLE;
> > > >
> > > > +       /* WA: 16023497226*/
> > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > +           (intel_dp->psr.delayed_vblank ||
> > > > intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > > +               drm_dbg_kms(&dev_priv->drm,
> > > > +                           "PSR1 not activated as it doesn't
> > > > meet
> > > > requirements of WA:16023497226\n");
> > > > +               return false;
> > > > +       }
> > > > +
> > >
> > > I would recommend doing this in intel_psr_compute_config as a last
> > > step and drop patch 1. Doing it this way would be safer as it's not
> > > opening new sequence/state where psr.enabled = true and psr.active =
> > > false after intel_psr_enable_locked.
> >
> > The reason for this was I wanted to disable only psr1 based on if dc5
> > entry is possible or not.
> > Even if I call the dc5_entry_is_possible function from compute_config
> > and save it in the intel_psr state we would still end up with the seq
> > psr.enabled = true and psr.active = false unless you see a param which
> > will only activate psr2 and not psr1 in such scenario ?
> >
> 
> I was thinking doing it like this:
> 
> +static void wa_16023497226(struct intel_crtc_state * crtc_state) {
> +	/* PSR2 not handled here. Wa not needed for Panel Replay */
> +	if (crtc_state->has_sel_update || crtc_state-
> >has_panel_replay)
> +	    return;
> +
> +       if (intel_dp->psr.is_dpkgc_configured &&
> +           (intel_dp->psr.delayed_vblank ||
> +	    intel_psr_is_dc5_entry_possible(dev_priv))) {
> +               drm_dbg_kms(&dev_priv->drm,
> +                           "PSR1 not enabled as it doesn't meet
> +			   requirements of WA:16023497226\n");
> +               crtc_state->has_psr = false;
> +       }
> +}
> +
>  void intel_psr_compute_config(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state,
>  			      struct drm_connector_state *conn_state) @@ -
> 1635,6 +1651,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		return;
> 
>  	crtc_state->has_sel_update =
> intel_sel_update_config_valid(intel_dp, crtc_state);
> +
> +	wa_16023497226(crtc_state);
>  }
> 
>  void intel_psr_get_config(struct intel_encoder *encoder,
> 
> Do you see this would be possible? Current PSR2 handling in your patches is ok
> to me.

Even if has_psr as false I see that hsw_psr1_activate can be invoked since there is
No real check stopping it from getting activated

/* psr1, psr2 and panel-replay are mutually exclusive.*/
        if (intel_dp->psr.panel_replay_enabled)
                dg2_activate_panel_replay(intel_dp);
        else if (intel_dp->psr.sel_update_enabled)
                hsw_activate_psr2(intel_dp);
        else
                ret = hsw_activate_psr1(intel_dp);

so if it isn't psr2 or panel replay we will activate psr1 do we need to add an else if statement here in that case.

Regards,
Suraj Kandpal

> 
> BR,
> 
> Jouni Högander
> 
> > Regards,
> > Suraj Kandpal
> >
> > >
> > > BR,
> > >
> > > Jouni Högander
> > >
> > > >         val |=
> > > > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > >
> > > >         if (DISPLAY_VER(dev_priv) < 20) @@ -907,7 +982,10 @@
> > > > static void hsw_activate_psr2(struct intel_dp
> > > > *intel_dp)
> > > >         u32 val = EDP_PSR2_ENABLE;
> > > >         u32 psr_val = 0;
> > > >
> > > > -       val |=
> > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > +       /* WA: 16023497226*/
> > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > +           intel_psr_is_dc5_entry_possible(dev_priv))
> > > > +               val |=
> > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > >
> > > >         if (DISPLAY_VER(dev_priv) < 14 &&
> > > > !IS_ALDERLAKE_P(dev_priv))
> > > >                 val |= EDP_SU_TRACK_ENABLE; @@ -1502,6 +1580,8 @@
> > > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > >                 return;
> > > >
> > > >         crtc_state->has_sel_update =
> > > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > > +       intel_dp->psr.delayed_vblank =
> > > > intel_psr_check_delayed_vblank_limit(crtc_state);
> > > > +       intel_dp->psr.is_dpkgc_configured =
> > > > intel_psr_is_dpkgc_configured(dev_priv);
> > > >  }
> > > >
> > > >  void intel_psr_get_config(struct intel_encoder *encoder,
> >

Jouni Högander Aug. 23, 2024, 7:20 a.m. UTC | #7

On Fri, 2024-08-23 at 06:18 +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <[e-mail]>
> > Sent: Friday, August 23, 2024 10:54 AM
> > To: Kandpal, Suraj <[e-mail]>;
> > [e-mail]
> > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach
> > PC10
> > 
> > On Fri, 2024-08-23 at 04:54 +0000, Kandpal, Suraj wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Hogander, Jouni <[e-mail]>
> > > > Sent: Thursday, August 22, 2024 2:16 PM
> > > > To: Kandpal, Suraj <[e-mail]>; intel-
> > > > [e-mail]
> > > > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > > > <[e-mail]>; [e-mail]
> > > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help
> > > > reach
> > > > PC10
> > > > 
> > > > On Wed, 2024-06-19 at 10:07 +0530, Suraj Kandpal wrote:
> > > > > To reach PC10 when PKG_C_LATENCY is configure we must do the
> > > > following
> > > > > things
> > > > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can
> > > > > be
> > > > > entered
> > > > > 2) Allow PSR2 deep sleep when DC5 can be entered
> > > > > 3) DC5 can be entered when all transocoder have either PSR1,
> > > > > PSR2
> > > > > or eDP 1.5 PR ALPM enabled and VBI is disabled and flips and
> > > > > pushes are not happening.
> > > > > 
> > > > > --v2
> > > > > -Switch condition and do an early return [Jani] -Do some
> > > > > checks in
> > > > > compute_config [Jani] -Do not use register reads as a method
> > > > > of
> > > > > checking states for DPKGC or delayed vblank [Jani] -Use
> > > > > another
> > > > > way to see is vblank interrupts are disabled or not [Jani]
> > > > > 
> > > > > WA: 16023497226
> > > > > Signed-off-by: Suraj Kandpal <[e-mail]>
> > > > > ---
> > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 82
> > > > > ++++++++++++++++++-
> > > > >  2 files changed, 83 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git
> > > > > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > index 46b3cbeb4a82..031f8e889b65 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > @@ -1708,6 +1708,8 @@ struct intel_psr {
> > > > >         bool sink_support;
> > > > >         bool source_support;
> > > > >         bool enabled;
> > > > > +       bool delayed_vblank;
> > > > > +       bool is_dpkgc_configured;
> > > > >         bool paused;
> > > > >         enum pipe pipe;
> > > > >         enum transcoder transcoder; diff --git
> > > > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 080bf5e51148..4ddea6737386 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -808,6 +808,73 @@ static u8 psr_compute_idle_frames(struct
> > > > intel_dp
> > > > > *intel_dp)
> > > > >         return idle_frames;
> > > > >  }
> > > > > 
> > > > > +static bool intel_psr_check_delayed_vblank_limit(struct
> > > > > intel_crtc_state *crtc_state)
> > > > > +{
> > > > > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> > > > > > hw.adjusted_mode;
> > > > > +
> > > > > +       return (adjusted_mode->crtc_vblank_start -
> > > > > adjusted_mode-
> > > > > > crtc_vdisplay) >= 6;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20
> > > > > and
> > > > > + * VRR is not enabled
> > > > > + */
> > > > > +static bool intel_psr_is_dpkgc_configured(struct
> > > > > drm_i915_private
> > > > > *i915)
> > > > > +{
> > > > > +       struct intel_crtc *intel_crtc;
> > > > > +
> > > > > +       if (DISPLAY_VER(i915) < 20)
> > > > > +               return false;
> > > > > +
> > > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > > +               struct intel_crtc_state *crtc_state;
> > > > > +
> > > > > +               if (!intel_crtc->active)
> > > > > +                       continue;
> > > > > +
> > > > > +               crtc_state = intel_crtc->config;
> > > > > +
> > > > > +               if (crtc_state->vrr.enable)
> > > > > +                       return false;
> > > > > +       }
> > > > > +
> > > > > +       return true;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * DC5 entry is only possible if vblank interrupt is
> > > > > disabled
> > > > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
> > > > > + * enabled encoders.
> > > > > + */
> > > > > +static bool intel_psr_is_dc5_entry_possible(struct
> > > > > drm_i915_private
> > > > > *i915)
> > > > > +{
> > > > > +       struct intel_crtc *intel_crtc;
> > > > > +
> > > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > > +               struct intel_encoder *encoder;
> > > > > +               struct drm_crtc *crtc = &intel_crtc->base;
> > > > > +               struct drm_vblank_crtc *vblank;
> > > > > +
> > > > > +               if (!intel_crtc->active)
> > > > > +                       continue;
> > > > > +
> > > > > +               vblank = drm_crtc_vblank_crtc(crtc);
> > > > > +
> > > > > +               if (vblank->enabled)
> > > > > +                       return false;
> > > > > +
> > > > > +               for_each_encoder_on_crtc(&i915->drm, crtc,
> > > > > encoder) {
> > > > > +                       struct intel_dp *intel_dp =
> > > > > enc_to_intel_dp(encoder);
> > > > > +                       struct intel_psr *psr = &intel_dp-
> > > > > >psr;
> > > > > +
> > > > > +                       if (!psr->enabled)
> > > > > +                               return false;
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > > +       return true;
> > > > > +}
> > > > > +
> > > > >  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
> > > > >  {
> > > > >         struct drm_i915_private *dev_priv =
> > > > > dp_to_i915(intel_dp);
> > > > > @@
> > > > > -815,6 +882,14 @@ static bool hsw_activate_psr1(struct
> > > > > intel_dp
> > > > > *intel_dp)
> > > > >         u32 max_sleep_time = 0x1f;
> > > > >         u32 val = EDP_PSR_ENABLE;
> > > > > 
> > > > > +       /* WA: 16023497226*/
> > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > +           (intel_dp->psr.delayed_vblank ||
> > > > > intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > > > +               drm_dbg_kms(&dev_priv->drm,
> > > > > +                           "PSR1 not activated as it doesn't
> > > > > meet
> > > > > requirements of WA:16023497226\n");
> > > > > +               return false;
> > > > > +       }
> > > > > +
> > > > 
> > > > I would recommend doing this in intel_psr_compute_config as a
> > > > last
> > > > step and drop patch 1. Doing it this way would be safer as it's
> > > > not
> > > > opening new sequence/state where psr.enabled = true and
> > > > psr.active =
> > > > false after intel_psr_enable_locked.
> > > 
> > > The reason for this was I wanted to disable only psr1 based on if
> > > dc5
> > > entry is possible or not.
> > > Even if I call the dc5_entry_is_possible function from
> > > compute_config
> > > and save it in the intel_psr state we would still end up with the
> > > seq
> > > psr.enabled = true and psr.active = false unless you see a param
> > > which
> > > will only activate psr2 and not psr1 in such scenario ?
> > > 
> > 
> > I was thinking doing it like this:
> > 
> > +static void wa_16023497226(struct intel_crtc_state * crtc_state) {
> > +       /* PSR2 not handled here. Wa not needed for Panel Replay */
> > +       if (crtc_state->has_sel_update || crtc_state-
> > > has_panel_replay)
> > +           return;
> > +
> > +       if (intel_dp->psr.is_dpkgc_configured &&
> > +           (intel_dp->psr.delayed_vblank ||
> > +           intel_psr_is_dc5_entry_possible(dev_priv))) {
> > +               drm_dbg_kms(&dev_priv->drm,
> > +                           "PSR1 not enabled as it doesn't meet
> > +                          requirements of WA:16023497226\n");
> > +               crtc_state->has_psr = false;
> > +       }
> > +}
> > +
> >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> >                               struct intel_crtc_state *crtc_state,
> >                               struct drm_connector_state
> > *conn_state) @@ -
> > 1635,6 +1651,8 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >                 return;
> > 
> >         crtc_state->has_sel_update =
> > intel_sel_update_config_valid(intel_dp, crtc_state);
> > +
> > +       wa_16023497226(crtc_state);
> >  }
> > 
> >  void intel_psr_get_config(struct intel_encoder *encoder,
> > 
> > Do you see this would be possible? Current PSR2 handling in your
> > patches is ok
> > to me.
> 
> Even if has_psr as false I see that hsw_psr1_activate can be invoked
> since there is
> No real check stopping it from getting activated
> 
> /* psr1, psr2 and panel-replay are mutually exclusive.*/
>         if (intel_dp->psr.panel_replay_enabled)
>                 dg2_activate_panel_replay(intel_dp);
>         else if (intel_dp->psr.sel_update_enabled)
>                 hsw_activate_psr2(intel_dp);
>         else
>                 ret = hsw_activate_psr1(intel_dp);
> 
> so if it isn't psr2 or panel replay we will activate psr1 do we need
> to add an else if statement here in that case.

No. That is taken care in caller of intel_psr_enable_locked:

void intel_psr_post_plane_update(struct intel_atomic_state *state,
				 struct intel_crtc *crtc)
{
	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
	const struct intel_crtc_state *crtc_state =
		intel_atomic_get_new_crtc_state(state, crtc);
	struct intel_encoder *encoder;

	if (!crtc_state->has_psr)
		return;

path to intel_psr_activate is intel_psr_post_plane_update-
>intel_psr_enable_locked->intel_psr_activate.

BR,

Jouni Högander

> 
> Regards,
> Suraj Kandpal
> 
> > 
> > BR,
> > 
> > Jouni Högander
> > 
> > > Regards,
> > > Suraj Kandpal
> > > 
> > > > 
> > > > BR,
> > > > 
> > > > Jouni Högander
> > > > 
> > > > >         val |=
> > > > > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > 
> > > > >         if (DISPLAY_VER(dev_priv) < 20) @@ -907,7 +982,10 @@
> > > > > static void hsw_activate_psr2(struct intel_dp
> > > > > *intel_dp)
> > > > >         u32 val = EDP_PSR2_ENABLE;
> > > > >         u32 psr_val = 0;
> > > > > 
> > > > > -       val |=
> > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > +       /* WA: 16023497226*/
> > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > +           intel_psr_is_dc5_entry_possible(dev_priv))
> > > > > +               val |=
> > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > 
> > > > >         if (DISPLAY_VER(dev_priv) < 14 &&
> > > > > !IS_ALDERLAKE_P(dev_priv))
> > > > >                 val |= EDP_SU_TRACK_ENABLE; @@ -1502,6
> > > > > +1580,8 @@
> > > > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > > >                 return;
> > > > > 
> > > > >         crtc_state->has_sel_update =
> > > > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > > > +       intel_dp->psr.delayed_vblank =
> > > > > intel_psr_check_delayed_vblank_limit(crtc_state);
> > > > > +       intel_dp->psr.is_dpkgc_configured =
> > > > > intel_psr_is_dpkgc_configured(dev_priv);
> > > > >  }
> > > > > 
> > > > >  void intel_psr_get_config(struct intel_encoder *encoder,
> > > 
>

Suraj Kandpal Aug. 23, 2024, 9:49 a.m. UTC | #8

> -----Original Message-----
> From: Hogander, Jouni <[e-mail]>
> Sent: Friday, August 23, 2024 12:51 PM
> To: Kandpal, Suraj <[e-mail]>; intel-
> [e-mail]
> Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
> 
> On Fri, 2024-08-23 at 06:18 +0000, Kandpal, Suraj wrote:
> >
> >
> > > -----Original Message-----
> > > From: Hogander, Jouni <[e-mail]>
> > > Sent: Friday, August 23, 2024 10:54 AM
> > > To: Kandpal, Suraj <[e-mail]>;
> > > [e-mail]
> > > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach
> > > PC10
> > >
> > > On Fri, 2024-08-23 at 04:54 +0000, Kandpal, Suraj wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Hogander, Jouni <[e-mail]>
> > > > > Sent: Thursday, August 22, 2024 2:16 PM
> > > > > To: Kandpal, Suraj <[e-mail]>; intel-
> > > > > [e-mail]
> > > > > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > > > > <[e-mail]>; [e-mail]
> > > > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach
> > > > > PC10
> > > > >
> > > > > On Wed, 2024-06-19 at 10:07 +0530, Suraj Kandpal wrote:
> > > > > > To reach PC10 when PKG_C_LATENCY is configure we must do the
> > > > > following
> > > > > > things
> > > > > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can
> > > > > > be entered
> > > > > > 2) Allow PSR2 deep sleep when DC5 can be entered
> > > > > > 3) DC5 can be entered when all transocoder have either PSR1,
> > > > > > PSR2
> > > > > > or eDP 1.5 PR ALPM enabled and VBI is disabled and flips and
> > > > > > pushes are not happening.
> > > > > >
> > > > > > --v2
> > > > > > -Switch condition and do an early return [Jani] -Do some
> > > > > > checks in compute_config [Jani] -Do not use register reads as
> > > > > > a method of checking states for DPKGC or delayed vblank [Jani]
> > > > > > -Use another way to see is vblank interrupts are disabled or
> > > > > > not [Jani]
> > > > > >
> > > > > > WA: 16023497226
> > > > > > Signed-off-by: Suraj Kandpal <[e-mail]>
> > > > > > ---
> > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 82
> > > > > > ++++++++++++++++++-
> > > > > >  2 files changed, 83 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > index 46b3cbeb4a82..031f8e889b65 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > @@ -1708,6 +1708,8 @@ struct intel_psr {
> > > > > >         bool sink_support;
> > > > > >         bool source_support;
> > > > > >         bool enabled;
> > > > > > +       bool delayed_vblank;
> > > > > > +       bool is_dpkgc_configured;
> > > > > >         bool paused;
> > > > > >         enum pipe pipe;
> > > > > >         enum transcoder transcoder; diff --git
> > > > > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > index 080bf5e51148..4ddea6737386 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > @@ -808,6 +808,73 @@ static u8 psr_compute_idle_frames(struct
> > > > > intel_dp
> > > > > > *intel_dp)
> > > > > >         return idle_frames;
> > > > > >  }
> > > > > >
> > > > > > +static bool intel_psr_check_delayed_vblank_limit(struct
> > > > > > intel_crtc_state *crtc_state)
> > > > > > +{
> > > > > > +       struct drm_display_mode *adjusted_mode = &crtc_state-
> > > > > > > hw.adjusted_mode;
> > > > > > +
> > > > > > +       return (adjusted_mode->crtc_vblank_start -
> > > > > > adjusted_mode-
> > > > > > > crtc_vdisplay) >= 6;
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20
> > > > > > and
> > > > > > + * VRR is not enabled
> > > > > > + */
> > > > > > +static bool intel_psr_is_dpkgc_configured(struct
> > > > > > drm_i915_private
> > > > > > *i915)
> > > > > > +{
> > > > > > +       struct intel_crtc *intel_crtc;
> > > > > > +
> > > > > > +       if (DISPLAY_VER(i915) < 20)
> > > > > > +               return false;
> > > > > > +
> > > > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > > > +               struct intel_crtc_state *crtc_state;
> > > > > > +
> > > > > > +               if (!intel_crtc->active)
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               crtc_state = intel_crtc->config;
> > > > > > +
> > > > > > +               if (crtc_state->vrr.enable)
> > > > > > +                       return false;
> > > > > > +       }
> > > > > > +
> > > > > > +       return true;
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * DC5 entry is only possible if vblank interrupt is
> > > > > > disabled
> > > > > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
> > > > > > + * enabled encoders.
> > > > > > + */
> > > > > > +static bool intel_psr_is_dc5_entry_possible(struct
> > > > > > drm_i915_private
> > > > > > *i915)
> > > > > > +{
> > > > > > +       struct intel_crtc *intel_crtc;
> > > > > > +
> > > > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > > > +               struct intel_encoder *encoder;
> > > > > > +               struct drm_crtc *crtc = &intel_crtc->base;
> > > > > > +               struct drm_vblank_crtc *vblank;
> > > > > > +
> > > > > > +               if (!intel_crtc->active)
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               vblank = drm_crtc_vblank_crtc(crtc);
> > > > > > +
> > > > > > +               if (vblank->enabled)
> > > > > > +                       return false;
> > > > > > +
> > > > > > +               for_each_encoder_on_crtc(&i915->drm, crtc,
> > > > > > encoder) {
> > > > > > +                       struct intel_dp *intel_dp =
> > > > > > enc_to_intel_dp(encoder);
> > > > > > +                       struct intel_psr *psr = &intel_dp-
> > > > > > >psr;
> > > > > > +
> > > > > > +                       if (!psr->enabled)
> > > > > > +                               return false;
> > > > > > +               }
> > > > > > +       }
> > > > > > +
> > > > > > +       return true;
> > > > > > +}
> > > > > > +
> > > > > >  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
> > > > > >  {
> > > > > >         struct drm_i915_private *dev_priv =
> > > > > > dp_to_i915(intel_dp); @@
> > > > > > -815,6 +882,14 @@ static bool hsw_activate_psr1(struct
> > > > > > intel_dp
> > > > > > *intel_dp)
> > > > > >         u32 max_sleep_time = 0x1f;
> > > > > >         u32 val = EDP_PSR_ENABLE;
> > > > > >
> > > > > > +       /* WA: 16023497226*/
> > > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > > +           (intel_dp->psr.delayed_vblank ||
> > > > > > intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > > > > +               drm_dbg_kms(&dev_priv->drm,
> > > > > > +                           "PSR1 not activated as it doesn't
> > > > > > meet
> > > > > > requirements of WA:16023497226\n");
> > > > > > +               return false;
> > > > > > +       }
> > > > > > +
> > > > >
> > > > > I would recommend doing this in intel_psr_compute_config as a
> > > > > last step and drop patch 1. Doing it this way would be safer as
> > > > > it's not opening new sequence/state where psr.enabled = true and
> > > > > psr.active = false after intel_psr_enable_locked.
> > > >
> > > > The reason for this was I wanted to disable only psr1 based on if
> > > > dc5
> > > > entry is possible or not.
> > > > Even if I call the dc5_entry_is_possible function from
> > > > compute_config and save it in the intel_psr state we would still
> > > > end up with the seq psr.enabled = true and psr.active = false
> > > > unless you see a param which will only activate psr2 and not psr1
> > > > in such scenario ?
> > > >
> > >
> > > I was thinking doing it like this:
> > >
> > > +static void wa_16023497226(struct intel_crtc_state * crtc_state) {
> > > +       /* PSR2 not handled here. Wa not needed for Panel Replay */
> > > +       if (crtc_state->has_sel_update || crtc_state-
> > > > has_panel_replay)
> > > +           return;
> > > +
> > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > +           (intel_dp->psr.delayed_vblank ||
> > > +           intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > +               drm_dbg_kms(&dev_priv->drm,
> > > +                           "PSR1 not enabled as it doesn't meet
> > > +                          requirements of WA:16023497226\n");
> > > +               crtc_state->has_psr = false;
> > > +       }
> > > +}
> > > +
> > >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> > >                               struct intel_crtc_state *crtc_state,
> > >                               struct drm_connector_state
> > > *conn_state) @@ -
> > > 1635,6 +1651,8 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > >                 return;
> > >
> > >         crtc_state->has_sel_update =
> > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > +
> > > +       wa_16023497226(crtc_state);
> > >  }
> > >
> > >  void intel_psr_get_config(struct intel_encoder *encoder,
> > >
> > > Do you see this would be possible? Current PSR2 handling in your
> > > patches is ok to me.
> >
> > Even if has_psr as false I see that hsw_psr1_activate can be invoked
> > since there is No real check stopping it from getting activated
> >
> > /* psr1, psr2 and panel-replay are mutually exclusive.*/
> >         if (intel_dp->psr.panel_replay_enabled)
> >                 dg2_activate_panel_replay(intel_dp);
> >         else if (intel_dp->psr.sel_update_enabled)
> >                 hsw_activate_psr2(intel_dp);
> >         else
> >                 ret = hsw_activate_psr1(intel_dp);
> >
> > so if it isn't psr2 or panel replay we will activate psr1 do we need
> > to add an else if statement here in that case.
> 
> No. That is taken care in caller of intel_psr_enable_locked:
> 

Wouldn’t this stop us from enabling psr2 when could have been enabled?

Regards,
Suraj Kandpal

> void intel_psr_post_plane_update(struct intel_atomic_state *state,
> 				 struct intel_crtc *crtc)
> {
> 	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> 	const struct intel_crtc_state *crtc_state =
> 		intel_atomic_get_new_crtc_state(state, crtc);
> 	struct intel_encoder *encoder;
> 
> 	if (!crtc_state->has_psr)
> 		return;
> 
> path to intel_psr_activate is intel_psr_post_plane_update-
> >intel_psr_enable_locked->intel_psr_activate.
> 
> BR,
> 
> Jouni Högander
> 
> >
> > Regards,
> > Suraj Kandpal
> >
> > >
> > > BR,
> > >
> > > Jouni Högander
> > >
> > > > Regards,
> > > > Suraj Kandpal
> > > >
> > > > >
> > > > > BR,
> > > > >
> > > > > Jouni Högander
> > > > >
> > > > > >         val |=
> > > > > > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > >
> > > > > >         if (DISPLAY_VER(dev_priv) < 20) @@ -907,7 +982,10 @@
> > > > > > static void hsw_activate_psr2(struct intel_dp
> > > > > > *intel_dp)
> > > > > >         u32 val = EDP_PSR2_ENABLE;
> > > > > >         u32 psr_val = 0;
> > > > > >
> > > > > > -       val |=
> > > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > > +       /* WA: 16023497226*/
> > > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > > +           intel_psr_is_dc5_entry_possible(dev_priv))
> > > > > > +               val |=
> > > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > >
> > > > > >         if (DISPLAY_VER(dev_priv) < 14 &&
> > > > > > !IS_ALDERLAKE_P(dev_priv))
> > > > > >                 val |= EDP_SU_TRACK_ENABLE; @@ -1502,6
> > > > > > +1580,8 @@
> > > > > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > > > >                 return;
> > > > > >
> > > > > >         crtc_state->has_sel_update =
> > > > > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > > > > +       intel_dp->psr.delayed_vblank =
> > > > > > intel_psr_check_delayed_vblank_limit(crtc_state);
> > > > > > +       intel_dp->psr.is_dpkgc_configured =
> > > > > > intel_psr_is_dpkgc_configured(dev_priv);
> > > > > >  }
> > > > > >
> > > > > >  void intel_psr_get_config(struct intel_encoder *encoder,
> > > >
> >

Jouni Högander Aug. 23, 2024, 10 a.m. UTC | #9

On Fri, 2024-08-23 at 09:49 +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <[e-mail]>
> > Sent: Friday, August 23, 2024 12:51 PM
> > To: Kandpal, Suraj <[e-mail]>; intel-
> > [e-mail]
> > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach
> > PC10
> > 
> > On Fri, 2024-08-23 at 06:18 +0000, Kandpal, Suraj wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Hogander, Jouni <[e-mail]>
> > > > Sent: Friday, August 23, 2024 10:54 AM
> > > > To: Kandpal, Suraj <[e-mail]>;
> > > > [e-mail]
> > > > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > > > <[e-mail]>; [e-mail]
> > > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help
> > > > reach
> > > > PC10
> > > > 
> > > > On Fri, 2024-08-23 at 04:54 +0000, Kandpal, Suraj wrote:
> > > > > 
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Hogander, Jouni <[e-mail]>
> > > > > > Sent: Thursday, August 22, 2024 2:16 PM
> > > > > > To: Kandpal, Suraj <[e-mail]>; intel-
> > > > > > [e-mail]
> > > > > > Cc: Murthy, Arun R <[e-mail]>; Manna,
> > > > > > Animesh
> > > > > > <[e-mail]>; [e-mail]
> > > > > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help
> > > > > > reach
> > > > > > PC10
> > > > > > 
> > > > > > On Wed, 2024-06-19 at 10:07 +0530, Suraj Kandpal wrote:
> > > > > > > To reach PC10 when PKG_C_LATENCY is configure we must do
> > > > > > > the
> > > > > > following
> > > > > > > things
> > > > > > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5
> > > > > > > can
> > > > > > > be entered
> > > > > > > 2) Allow PSR2 deep sleep when DC5 can be entered
> > > > > > > 3) DC5 can be entered when all transocoder have either
> > > > > > > PSR1,
> > > > > > > PSR2
> > > > > > > or eDP 1.5 PR ALPM enabled and VBI is disabled and flips
> > > > > > > and
> > > > > > > pushes are not happening.
> > > > > > > 
> > > > > > > --v2
> > > > > > > -Switch condition and do an early return [Jani] -Do some
> > > > > > > checks in compute_config [Jani] -Do not use register
> > > > > > > reads as
> > > > > > > a method of checking states for DPKGC or delayed vblank
> > > > > > > [Jani]
> > > > > > > -Use another way to see is vblank interrupts are disabled
> > > > > > > or
> > > > > > > not [Jani]
> > > > > > > 
> > > > > > > WA: 16023497226
> > > > > > > Signed-off-by: Suraj Kandpal <[e-mail]>
> > > > > > > ---
> > > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 82
> > > > > > > ++++++++++++++++++-
> > > > > > >  2 files changed, 83 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git
> > > > > > > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > index 46b3cbeb4a82..031f8e889b65 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > @@ -1708,6 +1708,8 @@ struct intel_psr {
> > > > > > >         bool sink_support;
> > > > > > >         bool source_support;
> > > > > > >         bool enabled;
> > > > > > > +       bool delayed_vblank;
> > > > > > > +       bool is_dpkgc_configured;
> > > > > > >         bool paused;
> > > > > > >         enum pipe pipe;
> > > > > > >         enum transcoder transcoder; diff --git
> > > > > > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > index 080bf5e51148..4ddea6737386 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > @@ -808,6 +808,73 @@ static u8
> > > > > > > psr_compute_idle_frames(struct
> > > > > > intel_dp
> > > > > > > *intel_dp)
> > > > > > >         return idle_frames;
> > > > > > >  }
> > > > > > > 
> > > > > > > +static bool intel_psr_check_delayed_vblank_limit(struct
> > > > > > > intel_crtc_state *crtc_state)
> > > > > > > +{
> > > > > > > +       struct drm_display_mode *adjusted_mode =
> > > > > > > &crtc_state-
> > > > > > > > hw.adjusted_mode;
> > > > > > > +
> > > > > > > +       return (adjusted_mode->crtc_vblank_start -
> > > > > > > adjusted_mode-
> > > > > > > > crtc_vdisplay) >= 6;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >=
> > > > > > > 20
> > > > > > > and
> > > > > > > + * VRR is not enabled
> > > > > > > + */
> > > > > > > +static bool intel_psr_is_dpkgc_configured(struct
> > > > > > > drm_i915_private
> > > > > > > *i915)
> > > > > > > +{
> > > > > > > +       struct intel_crtc *intel_crtc;
> > > > > > > +
> > > > > > > +       if (DISPLAY_VER(i915) < 20)
> > > > > > > +               return false;
> > > > > > > +
> > > > > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > > > > +               struct intel_crtc_state *crtc_state;
> > > > > > > +
> > > > > > > +               if (!intel_crtc->active)
> > > > > > > +                       continue;
> > > > > > > +
> > > > > > > +               crtc_state = intel_crtc->config;
> > > > > > > +
> > > > > > > +               if (crtc_state->vrr.enable)
> > > > > > > +                       return false;
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       return true;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * DC5 entry is only possible if vblank interrupt is
> > > > > > > disabled
> > > > > > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on
> > > > > > > all
> > > > > > > + * enabled encoders.
> > > > > > > + */
> > > > > > > +static bool intel_psr_is_dc5_entry_possible(struct
> > > > > > > drm_i915_private
> > > > > > > *i915)
> > > > > > > +{
> > > > > > > +       struct intel_crtc *intel_crtc;
> > > > > > > +
> > > > > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > > > > +               struct intel_encoder *encoder;
> > > > > > > +               struct drm_crtc *crtc = &intel_crtc-
> > > > > > > >base;
> > > > > > > +               struct drm_vblank_crtc *vblank;
> > > > > > > +
> > > > > > > +               if (!intel_crtc->active)
> > > > > > > +                       continue;
> > > > > > > +
> > > > > > > +               vblank = drm_crtc_vblank_crtc(crtc);
> > > > > > > +
> > > > > > > +               if (vblank->enabled)
> > > > > > > +                       return false;
> > > > > > > +
> > > > > > > +               for_each_encoder_on_crtc(&i915->drm,
> > > > > > > crtc,
> > > > > > > encoder) {
> > > > > > > +                       struct intel_dp *intel_dp =
> > > > > > > enc_to_intel_dp(encoder);
> > > > > > > +                       struct intel_psr *psr =
> > > > > > > &intel_dp-
> > > > > > > > psr;
> > > > > > > +
> > > > > > > +                       if (!psr->enabled)
> > > > > > > +                               return false;
> > > > > > > +               }
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       return true;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
> > > > > > >  {
> > > > > > >         struct drm_i915_private *dev_priv =
> > > > > > > dp_to_i915(intel_dp); @@
> > > > > > > -815,6 +882,14 @@ static bool hsw_activate_psr1(struct
> > > > > > > intel_dp
> > > > > > > *intel_dp)
> > > > > > >         u32 max_sleep_time = 0x1f;
> > > > > > >         u32 val = EDP_PSR_ENABLE;
> > > > > > > 
> > > > > > > +       /* WA: 16023497226*/
> > > > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > > > +           (intel_dp->psr.delayed_vblank ||
> > > > > > > intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > > > > > +               drm_dbg_kms(&dev_priv->drm,
> > > > > > > +                           "PSR1 not activated as it
> > > > > > > doesn't
> > > > > > > meet
> > > > > > > requirements of WA:16023497226\n");
> > > > > > > +               return false;
> > > > > > > +       }
> > > > > > > +
> > > > > > 
> > > > > > I would recommend doing this in intel_psr_compute_config as
> > > > > > a
> > > > > > last step and drop patch 1. Doing it this way would be
> > > > > > safer as
> > > > > > it's not opening new sequence/state where psr.enabled =
> > > > > > true and
> > > > > > psr.active = false after intel_psr_enable_locked.
> > > > > 
> > > > > The reason for this was I wanted to disable only psr1 based
> > > > > on if
> > > > > dc5
> > > > > entry is possible or not.
> > > > > Even if I call the dc5_entry_is_possible function from
> > > > > compute_config and save it in the intel_psr state we would
> > > > > still
> > > > > end up with the seq psr.enabled = true and psr.active = false
> > > > > unless you see a param which will only activate psr2 and not
> > > > > psr1
> > > > > in such scenario ?
> > > > > 
> > > > 
> > > > I was thinking doing it like this:
> > > > 
> > > > +static void wa_16023497226(struct intel_crtc_state *
> > > > crtc_state) {
> > > > +       /* PSR2 not handled here. Wa not needed for Panel
> > > > Replay */
> > > > +       if (crtc_state->has_sel_update || crtc_state-
> > > > > has_panel_replay)
> > > > +           return;
> > > > +
> > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > +           (intel_dp->psr.delayed_vblank ||
> > > > +           intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > > +               drm_dbg_kms(&dev_priv->drm,
> > > > +                           "PSR1 not enabled as it doesn't
> > > > meet
> > > > +                          requirements of WA:16023497226\n");
> > > > +               crtc_state->has_psr = false;
> > > > +       }
> > > > +}
> > > > +
> > > >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > >                               struct intel_crtc_state
> > > > *crtc_state,
> > > >                               struct drm_connector_state
> > > > *conn_state) @@ -
> > > > 1635,6 +1651,8 @@ void intel_psr_compute_config(struct intel_dp
> > > > *intel_dp,
> > > >                 return;
> > > > 
> > > >         crtc_state->has_sel_update =
> > > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > > +
> > > > +       wa_16023497226(crtc_state);
> > > >  }
> > > > 
> > > >  void intel_psr_get_config(struct intel_encoder *encoder,
> > > > 
> > > > Do you see this would be possible? Current PSR2 handling in
> > > > your
> > > > patches is ok to me.
> > > 
> > > Even if has_psr as false I see that hsw_psr1_activate can be
> > > invoked
> > > since there is No real check stopping it from getting activated
> > > 
> > > /* psr1, psr2 and panel-replay are mutually exclusive.*/
> > >         if (intel_dp->psr.panel_replay_enabled)
> > >                 dg2_activate_panel_replay(intel_dp);
> > >         else if (intel_dp->psr.sel_update_enabled)
> > >                 hsw_activate_psr2(intel_dp);
> > >         else
> > >                 ret = hsw_activate_psr1(intel_dp);
> > > 
> > > so if it isn't psr2 or panel replay we will activate psr1 do we
> > > need
> > > to add an else if statement here in that case.
> > 
> > No. That is taken care in caller of intel_psr_enable_locked:
> > 
> 
> Wouldn’t this stop us from enabling psr2 when could have been
> enabled?

No.

If you do it like in my example has_psr is cleared due to
wa_16023497226 only for PSR1. I.e when crtc_state->has_psr == true &&
crtc_state->has_sel_update == false && crtc_state->has_panel_replay ==
false. See  has_psr + has_panel_replay + has_sel_update documentation
in the begin of intel_psr.c.

BR,

Jouni Högander

> 
> Regards,
> Suraj Kandpal
> 
> > void intel_psr_post_plane_update(struct intel_atomic_state *state,
> >                                  struct intel_crtc *crtc)
> > {
> >         struct drm_i915_private *dev_priv = to_i915(state-
> > >base.dev);
> >         const struct intel_crtc_state *crtc_state =
> >                 intel_atomic_get_new_crtc_state(state, crtc);
> >         struct intel_encoder *encoder;
> > 
> >         if (!crtc_state->has_psr)
> >                 return;
> > 
> > path to intel_psr_activate is intel_psr_post_plane_update-
> > > intel_psr_enable_locked->intel_psr_activate.
> > 
> > BR,
> > 
> > Jouni Högander
> > 
> > > 
> > > Regards,
> > > Suraj Kandpal
> > > 
> > > > 
> > > > BR,
> > > > 
> > > > Jouni Högander
> > > > 
> > > > > Regards,
> > > > > Suraj Kandpal
> > > > > 
> > > > > > 
> > > > > > BR,
> > > > > > 
> > > > > > Jouni Högander
> > > > > > 
> > > > > > >         val |=
> > > > > > > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > > > 
> > > > > > >         if (DISPLAY_VER(dev_priv) < 20) @@ -907,7 +982,10
> > > > > > > @@
> > > > > > > static void hsw_activate_psr2(struct intel_dp
> > > > > > > *intel_dp)
> > > > > > >         u32 val = EDP_PSR2_ENABLE;
> > > > > > >         u32 psr_val = 0;
> > > > > > > 
> > > > > > > -       val |=
> > > > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > > > +       /* WA: 16023497226*/
> > > > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > > > +           intel_psr_is_dc5_entry_possible(dev_priv))
> > > > > > > +               val |=
> > > > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > > > 
> > > > > > >         if (DISPLAY_VER(dev_priv) < 14 &&
> > > > > > > !IS_ALDERLAKE_P(dev_priv))
> > > > > > >                 val |= EDP_SU_TRACK_ENABLE; @@ -1502,6
> > > > > > > +1580,8 @@
> > > > > > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > > > > >                 return;
> > > > > > > 
> > > > > > >         crtc_state->has_sel_update =
> > > > > > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > > > > > +       intel_dp->psr.delayed_vblank =
> > > > > > > intel_psr_check_delayed_vblank_limit(crtc_state);
> > > > > > > +       intel_dp->psr.is_dpkgc_configured =
> > > > > > > intel_psr_is_dpkgc_configured(dev_priv);
> > > > > > >  }
> > > > > > > 
> > > > > > >  void intel_psr_get_config(struct intel_encoder *encoder,
> > > > > 
> > > 
>

Suraj Kandpal Aug. 27, 2024, 4:32 a.m. UTC | #10

> -----Original Message-----
> From: Hogander, Jouni <[e-mail]>
> Sent: Friday, August 23, 2024 3:30 PM
> To: Kandpal, Suraj <[e-mail]>; [e-mail]
> Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
> 
> On Fri, 2024-08-23 at 09:49 +0000, Kandpal, Suraj wrote:
> >
> >
> > > -----Original Message-----
> > > From: Hogander, Jouni <[e-mail]>
> > > Sent: Friday, August 23, 2024 12:51 PM
> > > To: Kandpal, Suraj <[e-mail]>; intel-
> > > [e-mail]
> > > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > > <[e-mail]>; [e-mail]
> > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach
> > > PC10
> > >
> > > On Fri, 2024-08-23 at 06:18 +0000, Kandpal, Suraj wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Hogander, Jouni <[e-mail]>
> > > > > Sent: Friday, August 23, 2024 10:54 AM
> > > > > To: Kandpal, Suraj <[e-mail]>;
> > > > > [e-mail]
> > > > > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > > > > <[e-mail]>; [e-mail]
> > > > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach
> > > > > PC10
> > > > >
> > > > > On Fri, 2024-08-23 at 04:54 +0000, Kandpal, Suraj wrote:
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Hogander, Jouni <[e-mail]>
> > > > > > > Sent: Thursday, August 22, 2024 2:16 PM
> > > > > > > To: Kandpal, Suraj <[e-mail]>; intel-
> > > > > > > [e-mail]
> > > > > > > Cc: Murthy, Arun R <[e-mail]>; Manna, Animesh
> > > > > > > <[e-mail]>; [e-mail]
> > > > > > > Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help
> > > > > > > reach
> > > > > > > PC10
> > > > > > >
> > > > > > > On Wed, 2024-06-19 at 10:07 +0530, Suraj Kandpal wrote:
> > > > > > > > To reach PC10 when PKG_C_LATENCY is configure we must do
> > > > > > > > the
> > > > > > > following
> > > > > > > > things
> > > > > > > > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5
> > > > > > > > can be entered
> > > > > > > > 2) Allow PSR2 deep sleep when DC5 can be entered
> > > > > > > > 3) DC5 can be entered when all transocoder have either
> > > > > > > > PSR1,
> > > > > > > > PSR2
> > > > > > > > or eDP 1.5 PR ALPM enabled and VBI is disabled and flips
> > > > > > > > and pushes are not happening.
> > > > > > > >
> > > > > > > > --v2
> > > > > > > > -Switch condition and do an early return [Jani] -Do some
> > > > > > > > checks in compute_config [Jani] -Do not use register reads
> > > > > > > > as a method of checking states for DPKGC or delayed vblank
> > > > > > > > [Jani] -Use another way to see is vblank interrupts are
> > > > > > > > disabled or not [Jani]
> > > > > > > >
> > > > > > > > WA: 16023497226
> > > > > > > > Signed-off-by: Suraj Kandpal <[e-mail]>
> > > > > > > > ---
> > > > > > > >  .../drm/i915/display/intel_display_types.h    |  2 +
> > > > > > > >  drivers/gpu/drm/i915/display/intel_psr.c      | 82
> > > > > > > > ++++++++++++++++++-
> > > > > > > >  2 files changed, 83 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git
> > > > > > > > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > index 46b3cbeb4a82..031f8e889b65 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > > > > @@ -1708,6 +1708,8 @@ struct intel_psr {
> > > > > > > >         bool sink_support;
> > > > > > > >         bool source_support;
> > > > > > > >         bool enabled;
> > > > > > > > +       bool delayed_vblank;
> > > > > > > > +       bool is_dpkgc_configured;
> > > > > > > >         bool paused;
> > > > > > > >         enum pipe pipe;
> > > > > > > >         enum transcoder transcoder; diff --git
> > > > > > > > a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > index 080bf5e51148..4ddea6737386 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > @@ -808,6 +808,73 @@ static u8
> > > > > > > > psr_compute_idle_frames(struct
> > > > > > > intel_dp
> > > > > > > > *intel_dp)
> > > > > > > >         return idle_frames;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static bool intel_psr_check_delayed_vblank_limit(struct
> > > > > > > > intel_crtc_state *crtc_state)
> > > > > > > > +{
> > > > > > > > +       struct drm_display_mode *adjusted_mode =
> > > > > > > > &crtc_state-
> > > > > > > > > hw.adjusted_mode;
> > > > > > > > +
> > > > > > > > +       return (adjusted_mode->crtc_vblank_start -
> > > > > > > > adjusted_mode-
> > > > > > > > > crtc_vdisplay) >= 6;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * PKG_C_LATENCY is configured only when DISPLAY_VER >=
> > > > > > > > 20
> > > > > > > > and
> > > > > > > > + * VRR is not enabled
> > > > > > > > + */
> > > > > > > > +static bool intel_psr_is_dpkgc_configured(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915)
> > > > > > > > +{
> > > > > > > > +       struct intel_crtc *intel_crtc;
> > > > > > > > +
> > > > > > > > +       if (DISPLAY_VER(i915) < 20)
> > > > > > > > +               return false;
> > > > > > > > +
> > > > > > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > > > > > +               struct intel_crtc_state *crtc_state;
> > > > > > > > +
> > > > > > > > +               if (!intel_crtc->active)
> > > > > > > > +                       continue;
> > > > > > > > +
> > > > > > > > +               crtc_state = intel_crtc->config;
> > > > > > > > +
> > > > > > > > +               if (crtc_state->vrr.enable)
> > > > > > > > +                       return false;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       return true;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * DC5 entry is only possible if vblank interrupt is
> > > > > > > > disabled
> > > > > > > > + * and either psr1, psr2, edp 1.5 pr alpm is enabled on
> > > > > > > > all
> > > > > > > > + * enabled encoders.
> > > > > > > > + */
> > > > > > > > +static bool intel_psr_is_dc5_entry_possible(struct
> > > > > > > > drm_i915_private
> > > > > > > > *i915)
> > > > > > > > +{
> > > > > > > > +       struct intel_crtc *intel_crtc;
> > > > > > > > +
> > > > > > > > +       for_each_intel_crtc(&i915->drm, intel_crtc) {
> > > > > > > > +               struct intel_encoder *encoder;
> > > > > > > > +               struct drm_crtc *crtc = &intel_crtc-
> > > > > > > > >base;
> > > > > > > > +               struct drm_vblank_crtc *vblank;
> > > > > > > > +
> > > > > > > > +               if (!intel_crtc->active)
> > > > > > > > +                       continue;
> > > > > > > > +
> > > > > > > > +               vblank = drm_crtc_vblank_crtc(crtc);
> > > > > > > > +
> > > > > > > > +               if (vblank->enabled)
> > > > > > > > +                       return false;
> > > > > > > > +
> > > > > > > > +               for_each_encoder_on_crtc(&i915->drm,
> > > > > > > > crtc,
> > > > > > > > encoder) {
> > > > > > > > +                       struct intel_dp *intel_dp =
> > > > > > > > enc_to_intel_dp(encoder);
> > > > > > > > +                       struct intel_psr *psr =
> > > > > > > > &intel_dp-
> > > > > > > > > psr;
> > > > > > > > +
> > > > > > > > +                       if (!psr->enabled)
> > > > > > > > +                               return false;
> > > > > > > > +               }
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       return true;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
> > > > > > > >  {
> > > > > > > >         struct drm_i915_private *dev_priv =
> > > > > > > > dp_to_i915(intel_dp); @@
> > > > > > > > -815,6 +882,14 @@ static bool hsw_activate_psr1(struct
> > > > > > > > intel_dp
> > > > > > > > *intel_dp)
> > > > > > > >         u32 max_sleep_time = 0x1f;
> > > > > > > >         u32 val = EDP_PSR_ENABLE;
> > > > > > > >
> > > > > > > > +       /* WA: 16023497226*/
> > > > > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > > > > +           (intel_dp->psr.delayed_vblank ||
> > > > > > > > intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > > > > > > +               drm_dbg_kms(&dev_priv->drm,
> > > > > > > > +                           "PSR1 not activated as it
> > > > > > > > doesn't
> > > > > > > > meet
> > > > > > > > requirements of WA:16023497226\n");
> > > > > > > > +               return false;
> > > > > > > > +       }
> > > > > > > > +
> > > > > > >
> > > > > > > I would recommend doing this in intel_psr_compute_config as
> > > > > > > a last step and drop patch 1. Doing it this way would be
> > > > > > > safer as it's not opening new sequence/state where
> > > > > > > psr.enabled = true and psr.active = false after
> > > > > > > intel_psr_enable_locked.
> > > > > >
> > > > > > The reason for this was I wanted to disable only psr1 based on
> > > > > > if
> > > > > > dc5
> > > > > > entry is possible or not.
> > > > > > Even if I call the dc5_entry_is_possible function from
> > > > > > compute_config and save it in the intel_psr state we would
> > > > > > still end up with the seq psr.enabled = true and psr.active =
> > > > > > false unless you see a param which will only activate psr2 and
> > > > > > not
> > > > > > psr1
> > > > > > in such scenario ?
> > > > > >
> > > > >
> > > > > I was thinking doing it like this:
> > > > >
> > > > > +static void wa_16023497226(struct intel_crtc_state *
> > > > > crtc_state) {
> > > > > +       /* PSR2 not handled here. Wa not needed for Panel
> > > > > Replay */
> > > > > +       if (crtc_state->has_sel_update || crtc_state-
> > > > > > has_panel_replay)
> > > > > +           return;
> > > > > +
> > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > +           (intel_dp->psr.delayed_vblank ||
> > > > > +           intel_psr_is_dc5_entry_possible(dev_priv))) {
> > > > > +               drm_dbg_kms(&dev_priv->drm,
> > > > > +                           "PSR1 not enabled as it doesn't
> > > > > meet
> > > > > +                          requirements of WA:16023497226\n");
> > > > > +               crtc_state->has_psr = false;
> > > > > +       }
> > > > > +}
> > > > > +
> > > > >  void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > > >                               struct intel_crtc_state
> > > > > *crtc_state,
> > > > >                               struct drm_connector_state
> > > > > *conn_state) @@ -
> > > > > 1635,6 +1651,8 @@ void intel_psr_compute_config(struct intel_dp
> > > > > *intel_dp,
> > > > >                 return;
> > > > >
> > > > >         crtc_state->has_sel_update =
> > > > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > > > +
> > > > > +       wa_16023497226(crtc_state);
> > > > >  }
> > > > >
> > > > >  void intel_psr_get_config(struct intel_encoder *encoder,
> > > > >
> > > > > Do you see this would be possible? Current PSR2 handling in your
> > > > > patches is ok to me.
> > > >
> > > > Even if has_psr as false I see that hsw_psr1_activate can be
> > > > invoked since there is No real check stopping it from getting
> > > > activated
> > > >
> > > > /* psr1, psr2 and panel-replay are mutually exclusive.*/
> > > >         if (intel_dp->psr.panel_replay_enabled)
> > > >                 dg2_activate_panel_replay(intel_dp);
> > > >         else if (intel_dp->psr.sel_update_enabled)
> > > >                 hsw_activate_psr2(intel_dp);
> > > >         else
> > > >                 ret = hsw_activate_psr1(intel_dp);
> > > >
> > > > so if it isn't psr2 or panel replay we will activate psr1 do we
> > > > need to add an else if statement here in that case.
> > >
> > > No. That is taken care in caller of intel_psr_enable_locked:
> > >
> >
> > Wouldn’t this stop us from enabling psr2 when could have been enabled?
> 
> No.
> 
> If you do it like in my example has_psr is cleared due to
> wa_16023497226 only for PSR1. I.e when crtc_state->has_psr == true &&
> crtc_state->has_sel_update == false && crtc_state->has_panel_replay == false.
> See  has_psr + has_panel_replay + has_sel_update documentation in the begin
> of intel_psr.c.

Ahh okay got it wil do it in psr_compute_config then

Regards,
Suraj Kandpal

> 
> BR,
> 
> Jouni Högander
> 
> >
> > Regards,
> > Suraj Kandpal
> >
> > > void intel_psr_post_plane_update(struct intel_atomic_state *state,
> > >                                  struct intel_crtc *crtc) {
> > >         struct drm_i915_private *dev_priv = to_i915(state-
> > > >base.dev);
> > >         const struct intel_crtc_state *crtc_state =
> > >                 intel_atomic_get_new_crtc_state(state, crtc);
> > >         struct intel_encoder *encoder;
> > >
> > >         if (!crtc_state->has_psr)
> > >                 return;
> > >
> > > path to intel_psr_activate is intel_psr_post_plane_update-
> > > > intel_psr_enable_locked->intel_psr_activate.
> > >
> > > BR,
> > >
> > > Jouni Högander
> > >
> > > >
> > > > Regards,
> > > > Suraj Kandpal
> > > >
> > > > >
> > > > > BR,
> > > > >
> > > > > Jouni Högander
> > > > >
> > > > > > Regards,
> > > > > > Suraj Kandpal
> > > > > >
> > > > > > >
> > > > > > > BR,
> > > > > > >
> > > > > > > Jouni Högander
> > > > > > >
> > > > > > > >         val |=
> > > > > > > > EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > > > >
> > > > > > > >         if (DISPLAY_VER(dev_priv) < 20) @@ -907,7 +982,10
> > > > > > > > @@ static void hsw_activate_psr2(struct intel_dp
> > > > > > > > *intel_dp)
> > > > > > > >         u32 val = EDP_PSR2_ENABLE;
> > > > > > > >         u32 psr_val = 0;
> > > > > > > >
> > > > > > > > -       val |=
> > > > > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > > > > +       /* WA: 16023497226*/
> > > > > > > > +       if (intel_dp->psr.is_dpkgc_configured &&
> > > > > > > > +           intel_psr_is_dc5_entry_possible(dev_priv))
> > > > > > > > +               val |=
> > > > > > > > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > > > > > > >
> > > > > > > >         if (DISPLAY_VER(dev_priv) < 14 &&
> > > > > > > > !IS_ALDERLAKE_P(dev_priv))
> > > > > > > >                 val |= EDP_SU_TRACK_ENABLE; @@ -1502,6
> > > > > > > > +1580,8 @@
> > > > > > > > void intel_psr_compute_config(struct intel_dp *intel_dp,
> > > > > > > >                 return;
> > > > > > > >
> > > > > > > >         crtc_state->has_sel_update =
> > > > > > > > intel_sel_update_config_valid(intel_dp, crtc_state);
> > > > > > > > +       intel_dp->psr.delayed_vblank =
> > > > > > > > intel_psr_check_delayed_vblank_limit(crtc_state);
> > > > > > > > +       intel_dp->psr.is_dpkgc_configured =
> > > > > > > > intel_psr_is_dpkgc_configured(dev_priv);
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  void intel_psr_get_config(struct intel_encoder *encoder,
> > > > > >
> > > >
> >

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 46b3cbeb4a82..031f8e889b65 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1708,6 +1708,8 @@  struct intel_psr {
 	bool sink_support;
 	bool source_support;
 	bool enabled;
+	bool delayed_vblank;
+	bool is_dpkgc_configured;
 	bool paused;
 	enum pipe pipe;
 	enum transcoder transcoder;
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 080bf5e51148..4ddea6737386 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -808,6 +808,73 @@  static u8 psr_compute_idle_frames(struct intel_dp *intel_dp)
 	return idle_frames;
 }
 
+static bool intel_psr_check_delayed_vblank_limit(struct intel_crtc_state *crtc_state)
+{
+	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
+
+	return (adjusted_mode->crtc_vblank_start - adjusted_mode->crtc_vdisplay) >= 6;
+}
+
+/*
+ * PKG_C_LATENCY is configured only when DISPLAY_VER >= 20 and
+ * VRR is not enabled
+ */
+static bool intel_psr_is_dpkgc_configured(struct drm_i915_private *i915)
+{
+	struct intel_crtc *intel_crtc;
+
+	if (DISPLAY_VER(i915) < 20)
+		return false;
+
+	for_each_intel_crtc(&i915->drm, intel_crtc) {
+		struct intel_crtc_state *crtc_state;
+
+		if (!intel_crtc->active)
+			continue;
+
+		crtc_state = intel_crtc->config;
+
+		if (crtc_state->vrr.enable)
+			return false;
+	}
+
+	return true;
+}
+
+/*
+ * DC5 entry is only possible if vblank interrupt is disabled
+ * and either psr1, psr2, edp 1.5 pr alpm is enabled on all
+ * enabled encoders.
+ */
+static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private *i915)
+{
+	struct intel_crtc *intel_crtc;
+
+	for_each_intel_crtc(&i915->drm, intel_crtc) {
+		struct intel_encoder *encoder;
+		struct drm_crtc *crtc = &intel_crtc->base;
+		struct drm_vblank_crtc *vblank;
+
+		if (!intel_crtc->active)
+			continue;
+
+		vblank = drm_crtc_vblank_crtc(crtc);
+
+		if (vblank->enabled)
+			return false;
+
+		for_each_encoder_on_crtc(&i915->drm, crtc, encoder) {
+			struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+			struct intel_psr *psr = &intel_dp->psr;
+
+			if (!psr->enabled)
+				return false;
+		}
+	}
+
+	return true;
+}
+
 static bool hsw_activate_psr1(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
@@ -815,6 +882,14 @@  static bool hsw_activate_psr1(struct intel_dp *intel_dp)
 	u32 max_sleep_time = 0x1f;
 	u32 val = EDP_PSR_ENABLE;
 
+	/* WA: 16023497226*/
+	if (intel_dp->psr.is_dpkgc_configured &&
+	    (intel_dp->psr.delayed_vblank || intel_psr_is_dc5_entry_possible(dev_priv))) {
+		drm_dbg_kms(&dev_priv->drm,
+			    "PSR1 not activated as it doesn't meet requirements of WA:16023497226\n");
+		return false;
+	}
+
 	val |= EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
 
 	if (DISPLAY_VER(dev_priv) < 20)
@@ -907,7 +982,10 @@  static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	u32 val = EDP_PSR2_ENABLE;
 	u32 psr_val = 0;
 
-	val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
+	/* WA: 16023497226*/
+	if (intel_dp->psr.is_dpkgc_configured &&
+	    intel_psr_is_dc5_entry_possible(dev_priv))
+		val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
 
 	if (DISPLAY_VER(dev_priv) < 14 && !IS_ALDERLAKE_P(dev_priv))
 		val |= EDP_SU_TRACK_ENABLE;
@@ -1502,6 +1580,8 @@  void intel_psr_compute_config(struct intel_dp *intel_dp,
 		return;
 
 	crtc_state->has_sel_update = intel_sel_update_config_valid(intel_dp, crtc_state);
+	intel_dp->psr.delayed_vblank = intel_psr_check_delayed_vblank_limit(crtc_state);
+	intel_dp->psr.is_dpkgc_configured = intel_psr_is_dpkgc_configured(dev_priv);
 }
 
 void intel_psr_get_config(struct intel_encoder *encoder,