[PATCH 0/2] Implement WA to fix increased power usage

[PATCH 0/2] Implement WA to fix increased power usage

[Suraj Kandpal]

When DPKGC is enabled we see an increase in power consumption for
PSR1: When gap between vblank and delayed vblank is >= 6
PSR2: When deep sleep is enabled.
WA adds condition to avoid the above conditions

WA: 16023497226
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>

Suraj Kandpal (2):
  drm/i915/psr: Add return bool value for hsw_activate_psr1
  drm/i915/psr: Implment WA to help reach PC10

 drivers/gpu/drm/i915/display/intel_psr.c | 84 ++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 5 deletions(-)

-- 
2.43.2

[PATCH 1/2] drm/i915/psr: Add return bool value for hsw_activate_psr1

[Suraj Kandpal]

Convert hsw_activate_psr1 from void to bool as going forward
there is a chance psr1 is not enabled.

Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 4a4124a92a0d..6fc88f6c6b26 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -811,7 +811,7 @@ static u8 psr_compute_idle_frames(struct intel_dp *intel_dp)
 	return idle_frames;
 }
 
-static void hsw_activate_psr1(struct intel_dp *intel_dp)
+static bool hsw_activate_psr1(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
@@ -839,6 +839,8 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 
 	intel_de_rmw(dev_priv, psr_ctl_reg(dev_priv, cpu_transcoder),
 		     ~EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK, val);
+
+	return true;
 }
 
 static u32 intel_psr2_get_tp_time(struct intel_dp *intel_dp)
@@ -1540,6 +1542,7 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
+	bool ret = true;
 
 	drm_WARN_ON(&dev_priv->drm,
 		    transcoder_has_psr2(dev_priv, cpu_transcoder) &&
@@ -1558,9 +1561,9 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
 	else if (intel_dp->psr.sel_update_enabled)
 		hsw_activate_psr2(intel_dp);
 	else
-		hsw_activate_psr1(intel_dp);
+		ret = hsw_activate_psr1(intel_dp);
 
-	intel_dp->psr.active = true;
+	intel_dp->psr.active = ret;
 }
 
 static u32 wa_16013835468_bit_get(struct intel_dp *intel_dp)
-- 
2.43.2

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

[Suraj Kandpal]

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.

WA: 16023497226
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 75 +++++++++++++++++++++++-
 1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 6fc88f6c6b26..b22745c019df 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -811,12 +811,81 @@ static u8 psr_compute_idle_frames(struct intel_dp *intel_dp)
 	return idle_frames;
 }
 
+static bool intel_psr_check_delayed_vblank_limit(struct drm_i915_private *i915,
+						 enum transcoder cpu_transcoder)
+{
+	return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
+}
+
+static bool intel_psr_is_dpkgc_configured(struct drm_i915_private *i915)
+{
+	return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
+}
+
+static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private *i915)
+{
+	struct intel_crtc *intel_crtc;
+	bool ret = true;
+
+	for_each_intel_crtc(&i915->drm, intel_crtc) {
+		struct intel_encoder *encoder;
+		struct drm_crtc *crtc = &intel_crtc->base;
+		enum pipe pipe = intel_crtc->pipe;
+
+		if (!crtc->active)
+			continue;
+
+		if (!(i915->display.irq.de_irq_mask[pipe] & GEN8_PIPE_VBLANK))
+			ret = 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)
+				ret = false;
+		}
+	}
+
+	return ret;
+}
+
+static bool wa_16023497226_check(struct intel_dp *intel_dp, bool psr1)
+{
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
+
+	if (DISPLAY_VER(i915) != 20)
+		return true;
+
+	if (is_dpkg_c_configured(i915)) {
+		if (psr1 &&
+		    (intel_psr_check_delayed_vblank_limit(i915, cpu_transcoder) ||
+		     intel_psr_is_dc5_entry_possible(i915)))
+			return true;
+		else if (!psr1 && is_dc5_entry_possible(i915))
+			return true;
+		else
+			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);
 	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
 	u32 max_sleep_time = 0x1f;
-	u32 val = EDP_PSR_ENABLE;
+	u32 val = 0;
+
+	/* WA: 16023497226*/
+	if (wa_16023497226_check(intel_dp, true)) {
+		val = EDP_PSR_ENABLE;
+	} else {
+		drm_dbg_kms(&dev_priv->drm, "PSR1 was not activated\n");
+		return false;
+	}
 
 	val |= EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
 
@@ -910,7 +979,9 @@ 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 (wa_16023497226_check(intel_dp, false))
+		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;
-- 
2.43.2

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

[Jani Nikula]

On Thu, 06 Jun 2024, Suraj Kandpal <suraj.kandpal@intel.com> 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.
>
> WA: 16023497226
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 75 +++++++++++++++++++++++-
>  1 file changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 6fc88f6c6b26..b22745c019df 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -811,12 +811,81 @@ static u8 psr_compute_idle_frames(struct intel_dp *intel_dp)
>  	return idle_frames;
>  }
>  
> +static bool intel_psr_check_delayed_vblank_limit(struct drm_i915_private *i915,
> +						 enum transcoder cpu_transcoder)
> +{
> +	return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;

Please don't use the hardware to preserve the state for you. It will get
really complicated to maintain.

> +}
> +
> +static bool intel_psr_is_dpkgc_configured(struct drm_i915_private *i915)
> +{
> +	return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;

Ditto.

> +}
> +
> +static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private *i915)
> +{
> +	struct intel_crtc *intel_crtc;
> +	bool ret = true;
> +
> +	for_each_intel_crtc(&i915->drm, intel_crtc) {
> +		struct intel_encoder *encoder;
> +		struct drm_crtc *crtc = &intel_crtc->base;
> +		enum pipe pipe = intel_crtc->pipe;
> +
> +		if (!crtc->active)
> +			continue;
> +
> +		if (!(i915->display.irq.de_irq_mask[pipe] & GEN8_PIPE_VBLANK))

You have no business looking directly at that. It's for display irq code
*only*.

> +			ret = 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)
> +				ret = false;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static bool wa_16023497226_check(struct intel_dp *intel_dp, bool psr1)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> +
> +	if (DISPLAY_VER(i915) != 20)
> +		return true;
> +
> +	if (is_dpkg_c_configured(i915)) {
> +		if (psr1 &&
> +		    (intel_psr_check_delayed_vblank_limit(i915, cpu_transcoder) ||
> +		     intel_psr_is_dc5_entry_possible(i915)))
> +			return true;
> +		else if (!psr1 && is_dc5_entry_possible(i915))
> +			return true;
> +		else
> +			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);
>  	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>  	u32 max_sleep_time = 0x1f;
> -	u32 val = EDP_PSR_ENABLE;
> +	u32 val = 0;
> +
> +	/* WA: 16023497226*/
> +	if (wa_16023497226_check(intel_dp, true)) {
> +		val = EDP_PSR_ENABLE;
> +	} else {
> +		drm_dbg_kms(&dev_priv->drm, "PSR1 was not activated\n");

Please add reason.

> +		return false;
> +	}

Switch the condition around and use early return.

>  
>  	val |= EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
>  
> @@ -910,7 +979,9 @@ 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 (wa_16023497226_check(intel_dp, false))
> +		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;

-- 
Jani Nikula, Intel

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

[Kandpal, Suraj]

> Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
> 
> On Thu, 06 Jun 2024, Suraj Kandpal <suraj.kandpal@intel.com> 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.
> >
> > WA: 16023497226
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 75
> > +++++++++++++++++++++++-
> >  1 file changed, 73 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 6fc88f6c6b26..b22745c019df 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -811,12 +811,81 @@ static u8 psr_compute_idle_frames(struct
> intel_dp *intel_dp)
> >  	return idle_frames;
> >  }
> >
> > +static bool intel_psr_check_delayed_vblank_limit(struct drm_i915_private
> *i915,
> > +						 enum transcoder
> cpu_transcoder) {
> > +	return intel_de_read(i915,
> > +TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
> 
Hi Jani,
Thanks for the reviews

> Please don't use the hardware to preserve the state for you. It will get really
> complicated to maintain.
> 

Yes wanted to calculate the delayed vblank using the following way
Adjusted_mode->vblank_start - adjusted_mode->vblank_end
But I'll need crtc_state for that and I don't see a way of deriving it
Specially when this function is called from intel_psr_work
One way could be to have this wa check function be called from 
Intel_psr_enable_locked and save the corresponding Booleans in
Intel_psr or make in  drm_i915_private
structure and access that when intel_psr_activate is called from
Intel_psr_resume and intel_psr_work.
Do you think that could be feasible ?

> > +}
> > +
> > +static bool intel_psr_is_dpkgc_configured(struct drm_i915_private
> > +*i915) {
> > +	return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
> 
> Ditto.
> 

Similar question as above only place that I can manage a state to see if it is configured or not
would be in drm_i915_private.

> > +}
> > +
> > +static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private
> > +*i915) {
> > +	struct intel_crtc *intel_crtc;
> > +	bool ret = true;
> > +
> > +	for_each_intel_crtc(&i915->drm, intel_crtc) {
> > +		struct intel_encoder *encoder;
> > +		struct drm_crtc *crtc = &intel_crtc->base;
> > +		enum pipe pipe = intel_crtc->pipe;
> > +
> > +		if (!crtc->active)
> > +			continue;
> > +
> > +		if (!(i915->display.irq.de_irq_mask[pipe] &
> GEN8_PIPE_VBLANK))
> 
> You have no business looking directly at that. It's for display irq code *only*.
> 

Is there another way I can ensure if the vblank interrupt for the particular pipe is disabled?

> > +			ret = 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)
> > +				ret = false;
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static bool wa_16023497226_check(struct intel_dp *intel_dp, bool
> > +psr1) {
> > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > +	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> > +
> > +	if (DISPLAY_VER(i915) != 20)
> > +		return true;
> > +
> > +	if (is_dpkg_c_configured(i915)) {
> > +		if (psr1 &&
> > +		    (intel_psr_check_delayed_vblank_limit(i915,
> cpu_transcoder) ||
> > +		     intel_psr_is_dc5_entry_possible(i915)))
> > +			return true;
> > +		else if (!psr1 && is_dc5_entry_possible(i915))
> > +			return true;
> > +		else
> > +			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);
> >  	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> >  	u32 max_sleep_time = 0x1f;
> > -	u32 val = EDP_PSR_ENABLE;
> > +	u32 val = 0;
> > +
> > +	/* WA: 16023497226*/
> > +	if (wa_16023497226_check(intel_dp, true)) {
> > +		val = EDP_PSR_ENABLE;
> > +	} else {
> > +		drm_dbg_kms(&dev_priv->drm, "PSR1 was not activated\n");
> 
> Please add reason.
> 
> > +		return false;
> > +	}
> 
> Switch the condition around and use early return.
> 

Sure will do.

Regards,
Suraj Kandpal
> >
> >  	val |= EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> >
> > @@ -910,7 +979,9 @@ 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 (wa_16023497226_check(intel_dp, false))
> > +		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;
> 
> --
> Jani Nikula, Intel

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

[Jani Nikula]

On Mon, 10 Jun 2024, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
>> 
>> On Thu, 06 Jun 2024, Suraj Kandpal <suraj.kandpal@intel.com> 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.
>> >
>> > WA: 16023497226
>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_psr.c | 75
>> > +++++++++++++++++++++++-
>> >  1 file changed, 73 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
>> > b/drivers/gpu/drm/i915/display/intel_psr.c
>> > index 6fc88f6c6b26..b22745c019df 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > @@ -811,12 +811,81 @@ static u8 psr_compute_idle_frames(struct
>> intel_dp *intel_dp)
>> >  	return idle_frames;
>> >  }
>> >
>> > +static bool intel_psr_check_delayed_vblank_limit(struct drm_i915_private
>> *i915,
>> > +						 enum transcoder
>> cpu_transcoder) {
>> > +	return intel_de_read(i915,
>> > +TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
>> 
> Hi Jani,
> Thanks for the reviews
>
>> Please don't use the hardware to preserve the state for you. It will get really
>> complicated to maintain.
>> 
>
> Yes wanted to calculate the delayed vblank using the following way
> Adjusted_mode->vblank_start - adjusted_mode->vblank_end
> But I'll need crtc_state for that and I don't see a way of deriving it
> Specially when this function is called from intel_psr_work
> One way could be to have this wa check function be called from 
> Intel_psr_enable_locked and save the corresponding Booleans in
> Intel_psr or make in  drm_i915_private
> structure and access that when intel_psr_activate is called from
> Intel_psr_resume and intel_psr_work.
> Do you think that could be feasible ?

You'll be able to figure out a lot of cases up front at compute config
time, and disable PSR beforehand.

You'll know LNL_PKG_C_LATENCY (we seem to always configure it). You'll
know TRANS_SET_CONTEXT_LATENCY. You'll know whether all transcoders have
PSR enabled.

I think you'll need to split the conditions, and disable PSR as early as
possible when it should not be enabled. Then at actual enabling time,
you'll know the conditions that already have to hold, and you can check
fewer things.

This workaround is a bummer because it's permanent. It also means we
need to do this properly. Can't just poke at random stuff, because it'll
be painful forever.

BR,
Jani.

>
>> > +}
>> > +
>> > +static bool intel_psr_is_dpkgc_configured(struct drm_i915_private
>> > +*i915) {
>> > +	return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
>> 
>> Ditto.
>> 
>
> Similar question as above only place that I can manage a state to see if it is configured or not
> would be in drm_i915_private.
>
>> > +}
>> > +
>> > +static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private
>> > +*i915) {
>> > +	struct intel_crtc *intel_crtc;
>> > +	bool ret = true;
>> > +
>> > +	for_each_intel_crtc(&i915->drm, intel_crtc) {
>> > +		struct intel_encoder *encoder;
>> > +		struct drm_crtc *crtc = &intel_crtc->base;
>> > +		enum pipe pipe = intel_crtc->pipe;
>> > +
>> > +		if (!crtc->active)
>> > +			continue;
>> > +
>> > +		if (!(i915->display.irq.de_irq_mask[pipe] &
>> GEN8_PIPE_VBLANK))
>> 
>> You have no business looking directly at that. It's for display irq code *only*.
>> 
>
> Is there another way I can ensure if the vblank interrupt for the particular pipe is disabled?
>
>> > +			ret = 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)
>> > +				ret = false;
>> > +		}
>> > +	}
>> > +
>> > +	return ret;
>> > +}
>> > +
>> > +static bool wa_16023497226_check(struct intel_dp *intel_dp, bool
>> > +psr1) {
>> > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>> > +	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>> > +
>> > +	if (DISPLAY_VER(i915) != 20)
>> > +		return true;
>> > +
>> > +	if (is_dpkg_c_configured(i915)) {
>> > +		if (psr1 &&
>> > +		    (intel_psr_check_delayed_vblank_limit(i915,
>> cpu_transcoder) ||
>> > +		     intel_psr_is_dc5_entry_possible(i915)))
>> > +			return true;
>> > +		else if (!psr1 && is_dc5_entry_possible(i915))
>> > +			return true;
>> > +		else
>> > +			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);
>> >  	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>> >  	u32 max_sleep_time = 0x1f;
>> > -	u32 val = EDP_PSR_ENABLE;
>> > +	u32 val = 0;
>> > +
>> > +	/* WA: 16023497226*/
>> > +	if (wa_16023497226_check(intel_dp, true)) {
>> > +		val = EDP_PSR_ENABLE;
>> > +	} else {
>> > +		drm_dbg_kms(&dev_priv->drm, "PSR1 was not activated\n");
>> 
>> Please add reason.
>> 
>> > +		return false;
>> > +	}
>> 
>> Switch the condition around and use early return.
>> 
>
> Sure will do.
>
> Regards,
> Suraj Kandpal
>> >
>> >  	val |= EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
>> >
>> > @@ -910,7 +979,9 @@ 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 (wa_16023497226_check(intel_dp, false))
>> > +		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;
>> 
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel

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

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.10-rc2 next-20240606]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-i915-psr-Add-return-bool-value-for-hsw_activate_psr1/20240606-163351
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20240606082926.1816416-4-suraj.kandpal%40intel.com
patch subject: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
config: microblaze-allmodconfig (https://download.01.org/0day-ci/archive/20240607/202406070543.soJpPCOs-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/202406070543.soJpPCOs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406070543.soJpPCOs-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/display/intel_psr.c:35:
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'intel_psr_check_delayed_vblank_limit':
>> drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg.h:4158:62: error: 'dev_priv' undeclared (first use in this function); did you mean 'dev_crit'?
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^~~~~~~~
   drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro 'intel_de_read'
      31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
         |                                                                     ^~~~~~~~~~~
   drivers/gpu/drm/xe/compat-i915-headers/../../i915/display/intel_display_reg_defs.h:42:49: note: in expansion of macro '_MMIO'
      42 | #define _MMIO_TRANS2(display, tran, reg)        _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
         |                                                 ^~~~~
   drivers/gpu/drm/i915/display/intel_display_device.h:185:42: note: in expansion of macro '__to_intel_display'
     185 | #define DISPLAY_INFO(i915)              (__to_intel_display(i915)->info.__device_info)
         |                                          ^~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/xe/compat-i915-headers/../../i915/display/intel_display_reg_defs.h:42:55: note: in expansion of macro 'DISPLAY_INFO'
      42 | #define _MMIO_TRANS2(display, tran, reg)        _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
         |                                                       ^~~~~~~~~~~~
   drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg.h:4158:49: note: in expansion of macro '_MMIO_TRANS2'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                 ^~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c:817:36: note: in expansion of macro 'TRANS_SET_CONTEXT_LATENCY'
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg.h:4158:62: note: each undeclared identifier is reported only once for each function it appears in
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^~~~~~~~
   drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro 'intel_de_read'
      31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
         |                                                                     ^~~~~~~~~~~
   drivers/gpu/drm/xe/compat-i915-headers/../../i915/display/intel_display_reg_defs.h:42:49: note: in expansion of macro '_MMIO'
      42 | #define _MMIO_TRANS2(display, tran, reg)        _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
         |                                                 ^~~~~
   drivers/gpu/drm/i915/display/intel_display_device.h:185:42: note: in expansion of macro '__to_intel_display'
     185 | #define DISPLAY_INFO(i915)              (__to_intel_display(i915)->info.__device_info)
         |                                          ^~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/xe/compat-i915-headers/../../i915/display/intel_display_reg_defs.h:42:55: note: in expansion of macro 'DISPLAY_INFO'
      42 | #define _MMIO_TRANS2(display, tran, reg)        _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
         |                                                       ^~~~~~~~~~~~
   drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg.h:4158:49: note: in expansion of macro '_MMIO_TRANS2'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                 ^~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c:817:36: note: in expansion of macro 'TRANS_SET_CONTEXT_LATENCY'
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/i915/display/intel_psr.c:815:66: error: parameter 'cpu_transcoder' set but not used [-Werror=unused-but-set-parameter]
     815 |                                                  enum transcoder cpu_transcoder)
         |                                                  ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'intel_psr_is_dpkgc_configured':
>> drivers/gpu/drm/i915/display/intel_psr.c:822:36: error: 'LNL_PKG_C_LATENCY' undeclared (first use in this function)
     822 |         return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
         |                                    ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro 'intel_de_read'
      31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
         |                                                                     ^~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'intel_psr_is_dc5_entry_possible':
>> drivers/gpu/drm/i915/display/intel_psr.c:835:26: error: 'struct drm_crtc' has no member named 'active'
     835 |                 if (!crtc->active)
         |                          ^~
>> drivers/gpu/drm/i915/display/intel_psr.c:842:69: error: '_encoder' undeclared (first use in this function); did you mean 'encoder'?
     842 |                         struct intel_dp *intel_dp = enc_to_intel_dp(_encoder);
         |                                                                     ^~~~~~~~
         |                                                                     encoder
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'wa_16023497226_check':
>> drivers/gpu/drm/i915/display/intel_psr.c:861:13: error: implicit declaration of function 'is_dpkg_c_configured' [-Werror=implicit-function-declaration]
     861 |         if (is_dpkg_c_configured(i915)) {
         |             ^~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/i915/display/intel_psr.c:866:35: error: implicit declaration of function 'is_dc5_entry_possible'; did you mean 'intel_psr_is_dc5_entry_possible'? [-Werror=implicit-function-declaration]
     866 |                 else if (!psr1 && is_dc5_entry_possible(i915))
         |                                   ^~~~~~~~~~~~~~~~~~~~~
         |                                   intel_psr_is_dc5_entry_possible
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'intel_psr_check_delayed_vblank_limit':
   drivers/gpu/drm/i915/display/intel_psr.c:818:1: warning: control reaches end of non-void function [-Wreturn-type]
     818 | }
         | ^
   drivers/gpu/drm/i915/display/intel_psr.c: At top level:
>> drivers/gpu/drm/i915/display/intel_psr.c:820:13: error: 'intel_psr_is_dpkgc_configured' defined but not used [-Werror=unused-function]
     820 | static bool intel_psr_is_dpkgc_configured(struct drm_i915_private *i915)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +4158 drivers/gpu/drm/xe/compat-i915-headers/../../i915/i915_reg.h

dae847991a4327 Paulo Zanoni          2012-10-15  4153  
1d53ccdc400c87 José Roberto de Souza 2021-06-16  4154  #define _TRANS_A_SET_CONTEXT_LATENCY		0x6007C
1d53ccdc400c87 José Roberto de Souza 2021-06-16  4155  #define _TRANS_B_SET_CONTEXT_LATENCY		0x6107C
1d53ccdc400c87 José Roberto de Souza 2021-06-16  4156  #define _TRANS_C_SET_CONTEXT_LATENCY		0x6207C
1d53ccdc400c87 José Roberto de Souza 2021-06-16  4157  #define _TRANS_D_SET_CONTEXT_LATENCY		0x6307C
407569ff790979 Jani Nikula           2024-04-23 @4158  #define TRANS_SET_CONTEXT_LATENCY(tran)		_MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
1d53ccdc400c87 José Roberto de Souza 2021-06-16  4159  #define  TRANS_SET_CONTEXT_LATENCY_MASK		REG_GENMASK(15, 0)
1d53ccdc400c87 José Roberto de Souza 2021-06-16  4160  #define  TRANS_SET_CONTEXT_LATENCY_VALUE(x)	REG_FIELD_PREP(TRANS_SET_CONTEXT_LATENCY_MASK, (x))
1d53ccdc400c87 José Roberto de Souza 2021-06-16  4161  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.10-rc2 next-20240606]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-i915-psr-Add-return-bool-value-for-hsw_activate_psr1/20240606-163351
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20240606082926.1816416-4-suraj.kandpal%40intel.com
patch subject: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240607/202406070642.9SbQep4F-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/202406070642.9SbQep4F-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406070642.9SbQep4F-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/display/intel_psr.c:35:
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'intel_psr_check_delayed_vblank_limit':
   drivers/gpu/drm/i915/i915_reg.h:4158:62: error: 'dev_priv' undeclared (first use in this function); did you mean 'dev_crit'?
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^~~~~~~~
   drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro 'intel_de_read'
      31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
         |                                                                     ^~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_display_reg_defs.h:42:49: note: in expansion of macro '_MMIO'
      42 | #define _MMIO_TRANS2(display, tran, reg)        _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
         |                                                 ^~~~~
   drivers/gpu/drm/i915/display/intel_display_device.h:185:42: note: in expansion of macro '__to_intel_display'
     185 | #define DISPLAY_INFO(i915)              (__to_intel_display(i915)->info.__device_info)
         |                                          ^~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_display_reg_defs.h:42:55: note: in expansion of macro 'DISPLAY_INFO'
      42 | #define _MMIO_TRANS2(display, tran, reg)        _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
         |                                                       ^~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_reg.h:4158:49: note: in expansion of macro '_MMIO_TRANS2'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                 ^~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c:817:36: note: in expansion of macro 'TRANS_SET_CONTEXT_LATENCY'
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_reg.h:4158:62: note: each undeclared identifier is reported only once for each function it appears in
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^~~~~~~~
   drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro 'intel_de_read'
      31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
         |                                                                     ^~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_display_reg_defs.h:42:49: note: in expansion of macro '_MMIO'
      42 | #define _MMIO_TRANS2(display, tran, reg)        _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
         |                                                 ^~~~~
   drivers/gpu/drm/i915/display/intel_display_device.h:185:42: note: in expansion of macro '__to_intel_display'
     185 | #define DISPLAY_INFO(i915)              (__to_intel_display(i915)->info.__device_info)
         |                                          ^~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_display_reg_defs.h:42:55: note: in expansion of macro 'DISPLAY_INFO'
      42 | #define _MMIO_TRANS2(display, tran, reg)        _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
         |                                                       ^~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_reg.h:4158:49: note: in expansion of macro '_MMIO_TRANS2'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                 ^~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c:817:36: note: in expansion of macro 'TRANS_SET_CONTEXT_LATENCY'
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c:815:66: warning: parameter 'cpu_transcoder' set but not used [-Wunused-but-set-parameter]
     815 |                                                  enum transcoder cpu_transcoder)
         |                                                  ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'intel_psr_is_dpkgc_configured':
   drivers/gpu/drm/i915/display/intel_psr.c:822:36: error: 'LNL_PKG_C_LATENCY' undeclared (first use in this function)
     822 |         return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
         |                                    ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro 'intel_de_read'
      31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
         |                                                                     ^~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'intel_psr_is_dc5_entry_possible':
   drivers/gpu/drm/i915/display/intel_psr.c:835:26: error: 'struct drm_crtc' has no member named 'active'
     835 |                 if (!crtc->active)
         |                          ^~
   drivers/gpu/drm/i915/display/intel_psr.c:842:69: error: '_encoder' undeclared (first use in this function); did you mean 'encoder'?
     842 |                         struct intel_dp *intel_dp = enc_to_intel_dp(_encoder);
         |                                                                     ^~~~~~~~
         |                                                                     encoder
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'wa_16023497226_check':
   drivers/gpu/drm/i915/display/intel_psr.c:861:13: error: implicit declaration of function 'is_dpkg_c_configured' [-Werror=implicit-function-declaration]
     861 |         if (is_dpkg_c_configured(i915)) {
         |             ^~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c:866:35: error: implicit declaration of function 'is_dc5_entry_possible'; did you mean 'intel_psr_is_dc5_entry_possible'? [-Werror=implicit-function-declaration]
     866 |                 else if (!psr1 && is_dc5_entry_possible(i915))
         |                                   ^~~~~~~~~~~~~~~~~~~~~
         |                                   intel_psr_is_dc5_entry_possible
   drivers/gpu/drm/i915/display/intel_psr.c: In function 'intel_psr_check_delayed_vblank_limit':
   drivers/gpu/drm/i915/display/intel_psr.c:818:1: warning: control reaches end of non-void function [-Wreturn-type]
     818 | }
         | ^
   drivers/gpu/drm/i915/display/intel_psr.c: At top level:
>> drivers/gpu/drm/i915/display/intel_psr.c:820:13: warning: 'intel_psr_is_dpkgc_configured' defined but not used [-Wunused-function]
     820 | static bool intel_psr_is_dpkgc_configured(struct drm_i915_private *i915)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/intel_psr_is_dpkgc_configured +820 drivers/gpu/drm/i915/display/intel_psr.c

   819	
 > 820	static bool intel_psr_is_dpkgc_configured(struct drm_i915_private *i915)
   821	{
   822		return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
   823	}
   824	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Suraj,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.10-rc2 next-20240606]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Suraj-Kandpal/drm-i915-psr-Add-return-bool-value-for-hsw_activate_psr1/20240606-163351
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20240606082926.1816416-4-suraj.kandpal%40intel.com
patch subject: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
config: i386-buildonly-randconfig-002-20240607 (https://download.01.org/0day-ci/archive/20240607/202406070845.TnConzA7-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240607/202406070845.TnConzA7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406070845.TnConzA7-lkp@intel.com/

All errors (new ones prefixed by >>):

    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^
   include/linux/dev_printk.h:48:6: note: '_dev_crit' declared here
      48 | void _dev_crit(const struct device *dev, const char *fmt, ...);
         |      ^
   drivers/gpu/drm/i915/display/intel_psr.c:817:29: error: use of undeclared identifier 'dev_priv'; did you mean '_dev_crit'?
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^
   drivers/gpu/drm/i915/i915_reg.h:4158:55: note: expanded from macro 'TRANS_SET_CONTEXT_LATENCY'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^
   include/linux/dev_printk.h:48:6: note: '_dev_crit' declared here
      48 | void _dev_crit(const struct device *dev, const char *fmt, ...);
         |      ^
   drivers/gpu/drm/i915/display/intel_psr.c:817:29: error: controlling expression type 'void (*)(const struct device *, const char *, ...)' not compatible with any generic association type
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_reg.h:4158:55: note: expanded from macro 'TRANS_SET_CONTEXT_LATENCY'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^~~~~~~~
   drivers/gpu/drm/i915/display/intel_display_reg_defs.h:43:26: note: expanded from macro '_MMIO_TRANS2'
      43 |                                                       DISPLAY_INFO(display)->trans_offsets[TRANSCODER_A] + \
         |                                                                    ^~~~~~~
   drivers/gpu/drm/i915/display/intel_display_device.h:185:49: note: expanded from macro 'DISPLAY_INFO'
     185 | #define DISPLAY_INFO(i915)              (__to_intel_display(i915)->info.__device_info)
         |                                                             ^~~~
   drivers/gpu/drm/i915/display/intel_display_conversion.h:16:11: note: expanded from macro '__to_intel_display'
      16 |         _Generic(p,                                                     \
         |                  ^
   drivers/gpu/drm/i915/i915_reg_defs.h:267:47: note: expanded from macro '_MMIO'
     267 | #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
         |                                               ^
   drivers/gpu/drm/i915/display/intel_de.h:31:69: note: expanded from macro 'intel_de_read'
      31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
         |                                                                     ^~~~~~~~~~~
   drivers/gpu/drm/i915/display/intel_psr.c:817:29: error: use of undeclared identifier 'dev_priv'; did you mean '_dev_crit'?
   drivers/gpu/drm/i915/i915_reg.h:4158:55: note: expanded from macro 'TRANS_SET_CONTEXT_LATENCY'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^
   include/linux/dev_printk.h:48:6: note: '_dev_crit' declared here
      48 | void _dev_crit(const struct device *dev, const char *fmt, ...);
         |      ^
   drivers/gpu/drm/i915/display/intel_psr.c:817:29: error: use of undeclared identifier 'dev_priv'; did you mean '_dev_crit'?
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^
   drivers/gpu/drm/i915/i915_reg.h:4158:55: note: expanded from macro 'TRANS_SET_CONTEXT_LATENCY'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^
   include/linux/dev_printk.h:48:6: note: '_dev_crit' declared here
      48 | void _dev_crit(const struct device *dev, const char *fmt, ...);
         |      ^
   drivers/gpu/drm/i915/display/intel_psr.c:817:29: error: use of undeclared identifier 'dev_priv'; did you mean '_dev_crit'?
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^
   drivers/gpu/drm/i915/i915_reg.h:4158:55: note: expanded from macro 'TRANS_SET_CONTEXT_LATENCY'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^
   include/linux/dev_printk.h:48:6: note: '_dev_crit' declared here
      48 | void _dev_crit(const struct device *dev, const char *fmt, ...);
         |      ^
   drivers/gpu/drm/i915/display/intel_psr.c:817:29: error: use of undeclared identifier 'dev_priv'; did you mean '_dev_crit'?
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^
   drivers/gpu/drm/i915/i915_reg.h:4158:55: note: expanded from macro 'TRANS_SET_CONTEXT_LATENCY'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^
   include/linux/dev_printk.h:48:6: note: '_dev_crit' declared here
      48 | void _dev_crit(const struct device *dev, const char *fmt, ...);
         |      ^
   drivers/gpu/drm/i915/display/intel_psr.c:817:29: error: use of undeclared identifier 'dev_priv'; did you mean '_dev_crit'?
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^
   drivers/gpu/drm/i915/i915_reg.h:4158:55: note: expanded from macro 'TRANS_SET_CONTEXT_LATENCY'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^
   include/linux/dev_printk.h:48:6: note: '_dev_crit' declared here
      48 | void _dev_crit(const struct device *dev, const char *fmt, ...);
         |      ^
   drivers/gpu/drm/i915/display/intel_psr.c:817:29: error: controlling expression type 'void (*)(const struct device *, const char *, ...)' not compatible with any generic association type
     817 |         return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_reg.h:4158:55: note: expanded from macro 'TRANS_SET_CONTEXT_LATENCY'
    4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)         _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
         |                                                              ^~~~~~~~
   drivers/gpu/drm/i915/display/intel_display_reg_defs.h:44:31: note: expanded from macro '_MMIO_TRANS2'
      44 |                                                       DISPLAY_MMIO_BASE(display) + (reg))
         |                                                                         ^~~~~~~
   drivers/gpu/drm/i915/display/intel_display_reg_defs.h:11:51: note: expanded from macro 'DISPLAY_MMIO_BASE'
      11 | #define DISPLAY_MMIO_BASE(dev_priv)     (DISPLAY_INFO(dev_priv)->mmio_offset)
         |                                                       ^~~~~~~~
   note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
   drivers/gpu/drm/i915/display/intel_display_conversion.h:16:11: note: expanded from macro '__to_intel_display'
      16 |         _Generic(p,                                                     \
         |                  ^
   drivers/gpu/drm/i915/i915_reg_defs.h:267:47: note: expanded from macro '_MMIO'
     267 | #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
         |                                               ^
   drivers/gpu/drm/i915/display/intel_de.h:31:69: note: expanded from macro 'intel_de_read'
      31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
         |                                                                     ^~~~~~~~~~~
>> drivers/gpu/drm/i915/display/intel_psr.c:822:29: error: use of undeclared identifier 'LNL_PKG_C_LATENCY'
     822 |         return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
         |                                    ^
   fatal error: too many errors emitted, stopping now [-ferror-limit=]
   20 errors generated.


vim +/LNL_PKG_C_LATENCY +822 drivers/gpu/drm/i915/display/intel_psr.c

   819	
   820	static bool intel_psr_is_dpkgc_configured(struct drm_i915_private *i915)
   821	{
 > 822		return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
   823	}
   824	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

✗ Fi.CI.BUILD: failure for Implement WA to fix increased power usage

[Patchwork]

== Series Details ==

Series: Implement WA to fix increased power usage
URL   : https://patchwork.freedesktop.org/series/134541/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/display/intel_psr.o
In file included from drivers/gpu/drm/i915/display/intel_psr.c:35:
drivers/gpu/drm/i915/display/intel_psr.c: In function ‘intel_psr_check_delayed_vblank_limit’:
drivers/gpu/drm/i915/i915_reg.h:4158:55: error: ‘dev_priv’ undeclared (first use in this function); did you mean ‘dev_crit’?
 4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)  _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
      |                                                       ^~~~~~~~
drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro ‘intel_de_read’
   31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
      |                                                                     ^~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display_reg_defs.h:42:42: note: in expansion of macro ‘_MMIO’
   42 | #define _MMIO_TRANS2(display, tran, reg) _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
      |                                          ^~~~~
drivers/gpu/drm/i915/display/intel_display_device.h:185:30: note: in expansion of macro ‘__to_intel_display’
  185 | #define DISPLAY_INFO(i915)  (__to_intel_display(i915)->info.__device_info)
      |                              ^~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display_reg_defs.h:42:48: note: in expansion of macro ‘DISPLAY_INFO’
   42 | #define _MMIO_TRANS2(display, tran, reg) _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
      |                                                ^~~~~~~~~~~~
drivers/gpu/drm/i915/i915_reg.h:4158:42: note: in expansion of macro ‘_MMIO_TRANS2’
 4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)  _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
      |                                          ^~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_psr.c:817:29: note: in expansion of macro ‘TRANS_SET_CONTEXT_LATENCY’
  817 |  return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/i915_reg.h:4158:55: note: each undeclared identifier is reported only once for each function it appears in
 4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)  _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
      |                                                       ^~~~~~~~
drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro ‘intel_de_read’
   31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
      |                                                                     ^~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display_reg_defs.h:42:42: note: in expansion of macro ‘_MMIO’
   42 | #define _MMIO_TRANS2(display, tran, reg) _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
      |                                          ^~~~~
drivers/gpu/drm/i915/display/intel_display_device.h:185:30: note: in expansion of macro ‘__to_intel_display’
  185 | #define DISPLAY_INFO(i915)  (__to_intel_display(i915)->info.__device_info)
      |                              ^~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_display_reg_defs.h:42:48: note: in expansion of macro ‘DISPLAY_INFO’
   42 | #define _MMIO_TRANS2(display, tran, reg) _MMIO(DISPLAY_INFO(display)->trans_offsets[(tran)] - \
      |                                                ^~~~~~~~~~~~
drivers/gpu/drm/i915/i915_reg.h:4158:42: note: in expansion of macro ‘_MMIO_TRANS2’
 4158 | #define TRANS_SET_CONTEXT_LATENCY(tran)  _MMIO_TRANS2(dev_priv, tran, _TRANS_A_SET_CONTEXT_LATENCY)
      |                                          ^~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_psr.c:817:29: note: in expansion of macro ‘TRANS_SET_CONTEXT_LATENCY’
  817 |  return intel_de_read(i915, TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
      |                             ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_psr.c:815:24: error: parameter ‘cpu_transcoder’ set but not used [-Werror=unused-but-set-parameter]
  815 |        enum transcoder cpu_transcoder)
      |        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
In file included from drivers/gpu/drm/i915/display/intel_psr.c:35:
drivers/gpu/drm/i915/display/intel_psr.c: In function ‘intel_psr_is_dpkgc_configured’:
drivers/gpu/drm/i915/display/intel_psr.c:822:29: error: ‘LNL_PKG_C_LATENCY’ undeclared (first use in this function)
  822 |  return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
      |                             ^~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_de.h:31:69: note: in definition of macro ‘intel_de_read’
   31 | #define intel_de_read(p,...) __intel_de_read(__to_intel_display(p), __VA_ARGS__)
      |                                                                     ^~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_psr.c: In function ‘intel_psr_is_dc5_entry_possible’:
drivers/gpu/drm/i915/display/intel_psr.c:835:12: error: ‘struct drm_crtc’ has no member named ‘active’
  835 |   if (!crtc->active)
      |            ^~
drivers/gpu/drm/i915/display/intel_psr.c:842:48: error: ‘_encoder’ undeclared (first use in this function); did you mean ‘encoder’?
  842 |    struct intel_dp *intel_dp = enc_to_intel_dp(_encoder);
      |                                                ^~~~~~~~
      |                                                encoder
drivers/gpu/drm/i915/display/intel_psr.c: In function ‘wa_16023497226_check’:
drivers/gpu/drm/i915/display/intel_psr.c:861:6: error: implicit declaration of function ‘is_dpkg_c_configured’ [-Werror=implicit-function-declaration]
  861 |  if (is_dpkg_c_configured(i915)) {
      |      ^~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_psr.c:866:21: error: implicit declaration of function ‘is_dc5_entry_possible’; did you mean ‘intel_psr_is_dc5_entry_possible’? [-Werror=implicit-function-declaration]
  866 |   else if (!psr1 && is_dc5_entry_possible(i915))
      |                     ^~~~~~~~~~~~~~~~~~~~~
      |                     intel_psr_is_dc5_entry_possible
drivers/gpu/drm/i915/display/intel_psr.c: In function ‘intel_psr_check_delayed_vblank_limit’:
drivers/gpu/drm/i915/display/intel_psr.c:818:1: error: control reaches end of non-void function [-Werror=return-type]
  818 | }
      | ^
At top level:
drivers/gpu/drm/i915/display/intel_psr.c:820:13: error: ‘intel_psr_is_dpkgc_configured’ defined but not used [-Werror=unused-function]
  820 | static bool intel_psr_is_dpkgc_configured(struct drm_i915_private *i915)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[6]: *** [scripts/Makefile.build:244: drivers/gpu/drm/i915/display/intel_psr.o] Error 1
make[5]: *** [scripts/Makefile.build:485: drivers/gpu/drm/i915] Error 2
make[4]: *** [scripts/Makefile.build:485: drivers/gpu/drm] Error 2
make[3]: *** [scripts/Makefile.build:485: drivers/gpu] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/home/kbuild/kernel/Makefile:1934: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
Build failed, no error log produced

[PATCH 0/2] Implement WA to fix increased power usage

[Suraj Kandpal]

When DPKGC is enabled we see an increase in power consumption for
PSR1: When gap between vblank and delayed vblank is >= 6
PSR2: When deep sleep is enabled.
WA adds condition to avoid the above conditions

WA: 16023497226
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>

Suraj Kandpal (2):
  drm/i915/psr: Add return bool value for hsw_activate_psr1
  drm/i915/psr: Implment WA to help reach PC10

 .../drm/i915/display/intel_display_types.h    |  2 +
 drivers/gpu/drm/i915/display/intel_psr.c      | 91 ++++++++++++++++++-
 2 files changed, 89 insertions(+), 4 deletions(-)

-- 
2.43.2

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

[Suraj Kandpal]

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 <suraj.kandpal@intel.com>
---
 .../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;
+	}
+
 	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

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

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Suraj
> Kandpal
> Sent: Wednesday, June 19, 2024 10:08 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Manna, Animesh <animesh.manna@intel.com>; Murthy, Arun R
> <arun.r.murthy@intel.com>; Hogander, Jouni <jouni.hogander@intel.com>;
> jani.nikula@linux.intel.com; Kandpal, Suraj <suraj.kandpal@intel.com>
> 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 <suraj.kandpal@intel.com>
> ---
>  .../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

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

[Kandpal, Suraj]

> -----Original Message-----
> From: Shankar, Uma <uma.shankar@intel.com>
> Sent: Thursday, August 22, 2024 10:50 AM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Manna, Animesh <animesh.manna@intel.com>; Murthy, Arun R
> <arun.r.murthy@intel.com>; Hogander, Jouni <jouni.hogander@intel.com>;
> jani.nikula@linux.intel.com; Kandpal, Suraj <suraj.kandpal@intel.com>
> Subject: RE: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
> 
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Suraj Kandpal
> > Sent: Wednesday, June 19, 2024 10:08 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Manna, Animesh <animesh.manna@intel.com>; Murthy, Arun R
> > <arun.r.murthy@intel.com>; Hogander, Jouni
> <jouni.hogander@intel.com>;
> > jani.nikula@linux.intel.com; Kandpal, Suraj <suraj.kandpal@intel.com>
> > 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 <suraj.kandpal@intel.com>
> > ---
> >  .../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

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

[Hogander, Jouni]

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 <suraj.kandpal@intel.com>
> ---
>  .../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,

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

[Kandpal, Suraj]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander@intel.com>
> Sent: Thursday, August 22, 2024 2:16 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> 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 <suraj.kandpal@intel.com>
> > ---
> >  .../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,

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

[Hogander, Jouni]

On Fri, 2024-08-23 at 04:54 +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Thursday, August 22, 2024 2:16 PM
> > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > 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 <suraj.kandpal@intel.com>
> > > ---
> > >  .../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,
> 

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

[Kandpal, Suraj]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander@intel.com>
> Sent: Friday, August 23, 2024 10:54 AM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> 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 <jouni.hogander@intel.com>
> > > Sent: Thursday, August 22, 2024 2:16 PM
> > > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> > > gfx@lists.freedesktop.org
> > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > 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 <suraj.kandpal@intel.com>
> > > > ---
> > > >  .../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,
> >

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

[Hogander, Jouni]

On Fri, 2024-08-23 at 06:18 +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Friday, August 23, 2024 10:54 AM
> > To: Kandpal, Suraj <suraj.kandpal@intel.com>;
> > intel-gfx@lists.freedesktop.org
> > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > 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 <jouni.hogander@intel.com>
> > > > Sent: Thursday, August 22, 2024 2:16 PM
> > > > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> > > > gfx@lists.freedesktop.org
> > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > > 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 <suraj.kandpal@intel.com>
> > > > > ---
> > > > >  .../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,
> > > 
> 

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

[Kandpal, Suraj]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander@intel.com>
> Sent: Friday, August 23, 2024 12:51 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> 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 <jouni.hogander@intel.com>
> > > Sent: Friday, August 23, 2024 10:54 AM
> > > To: Kandpal, Suraj <suraj.kandpal@intel.com>;
> > > intel-gfx@lists.freedesktop.org
> > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > 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 <jouni.hogander@intel.com>
> > > > > Sent: Thursday, August 22, 2024 2:16 PM
> > > > > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> > > > > gfx@lists.freedesktop.org
> > > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > > > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > > > 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 <suraj.kandpal@intel.com>
> > > > > > ---
> > > > > >  .../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,
> > > >
> >

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

[Hogander, Jouni]

On Fri, 2024-08-23 at 09:49 +0000, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander@intel.com>
> > Sent: Friday, August 23, 2024 12:51 PM
> > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > 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 <jouni.hogander@intel.com>
> > > > Sent: Friday, August 23, 2024 10:54 AM
> > > > To: Kandpal, Suraj <suraj.kandpal@intel.com>;
> > > > intel-gfx@lists.freedesktop.org
> > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > > 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 <jouni.hogander@intel.com>
> > > > > > Sent: Thursday, August 22, 2024 2:16 PM
> > > > > > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> > > > > > gfx@lists.freedesktop.org
> > > > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna,
> > > > > > Animesh
> > > > > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > > > > 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 <suraj.kandpal@intel.com>
> > > > > > > ---
> > > > > > >  .../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,
> > > > > 
> > > 
> 

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

[Kandpal, Suraj]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander@intel.com>
> Sent: Friday, August 23, 2024 3:30 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> 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 <jouni.hogander@intel.com>
> > > Sent: Friday, August 23, 2024 12:51 PM
> > > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> > > gfx@lists.freedesktop.org
> > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > 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 <jouni.hogander@intel.com>
> > > > > Sent: Friday, August 23, 2024 10:54 AM
> > > > > To: Kandpal, Suraj <suraj.kandpal@intel.com>;
> > > > > intel-gfx@lists.freedesktop.org
> > > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > > > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > > > 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 <jouni.hogander@intel.com>
> > > > > > > Sent: Thursday, August 22, 2024 2:16 PM
> > > > > > > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-
> > > > > > > gfx@lists.freedesktop.org
> > > > > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Manna, Animesh
> > > > > > > <animesh.manna@intel.com>; jani.nikula@linux.intel.com
> > > > > > > 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 <suraj.kandpal@intel.com>
> > > > > > > > ---
> > > > > > > >  .../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,
> > > > > >
> > > >
> >