[v8,5/7] drm/i915/tgl: DC3CO PSR2 helper

Submitted by Anshuman Gupta on Sept. 13, 2019, 8:23 a.m.

Details

Message ID 20190913082339.1785-6-anshuman.gupta@intel.com
State New
Headers show
Series "DC3CO Support for TGL" ( rev: 9 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Anshuman Gupta Sept. 13, 2019, 8:23 a.m.
Add dc3co helper functions to enable/disable psr2 deep sleep.
Adhere B.Specs by disallow DC3CO state before PSR2 exit.
Enable PSR2 exitline event and program the desired scanlines
to exit DC3CO in intel_psr_enable function at modeset path.
Disable the DC3CO exitline in order to maintian consistent
pipe config state in encoder disable path.

v1: Moved calling of tgl_enable_psr2_transcoder_exitline() to
    intel_psr_enable(). [Imre]

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Animesh Manna <animesh.manna@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 drivers/gpu/drm/i915/display/intel_psr.c | 51 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_psr.h |  2 +
 drivers/gpu/drm/i915/i915_drv.h          |  1 +
 3 files changed, 54 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index b3c7eef53bf3..11d37f96ce71 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -534,6 +534,48 @@  transcoder_has_psr2(struct drm_i915_private *dev_priv, enum transcoder trans)
 		return trans == TRANSCODER_EDP;
 }
 
+static void psr2_program_idle_frames(struct drm_i915_private *dev_priv,
+				     u32 idle_frames)
+{
+	u32 val;
+
+	idle_frames <<=  EDP_PSR2_IDLE_FRAME_SHIFT;
+	val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
+	val &= ~EDP_PSR2_IDLE_FRAME_MASK;
+	val |= idle_frames;
+	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
+}
+
+void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv)
+{
+	int idle_frames = 0;
+
+	psr2_program_idle_frames(dev_priv, idle_frames);
+}
+
+void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv)
+{
+	int idle_frames;
+
+	/*
+	 * Let's use 6 as the minimum to cover all known cases including the
+	 * off-by-one issue that HW has in some cases.
+	 */
+	idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
+	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
+	psr2_program_idle_frames(dev_priv, idle_frames);
+}
+
+static void tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv)
+{
+	if (!IS_TIGERLAKE(dev_priv))
+		return;
+
+	cancel_delayed_work(&dev_priv->csr.idle_work);
+	/* Before PSR2 exit disallow dc3co*/
+	tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
+}
+
 static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 				    struct intel_crtc_state *crtc_state)
 {
@@ -799,6 +841,10 @@  void intel_psr_enable(struct intel_dp *intel_dp,
 
 	WARN_ON(dev_priv->drrs.dp);
 
+	/* Enable PSR2 transcoder exit line */
+	if (crtc_state->has_psr2)
+		tgl_enable_psr2_transcoder_exitline(crtc_state);
+
 	mutex_lock(&dev_priv->psr.lock);
 
 	if (!psr_global_enabled(dev_priv->psr.debug)) {
@@ -829,6 +875,7 @@  static void intel_psr_exit(struct drm_i915_private *dev_priv)
 	}
 
 	if (dev_priv->psr.psr2_enabled) {
+		tgl_disallow_dc3co_on_psr2_exit(dev_priv);
 		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
 		WARN_ON(!(val & EDP_PSR2_ENABLE));
 		val &= ~EDP_PSR2_ENABLE;
@@ -895,6 +942,10 @@  void intel_psr_disable(struct intel_dp *intel_dp,
 	if (WARN_ON(!CAN_PSR(dev_priv)))
 		return;
 
+	/* Disable PSR2 transcoder exit line */
+	if (old_crtc_state->has_psr2)
+		tgl_disable_psr2_transcoder_exitline(old_crtc_state);
+
 	mutex_lock(&dev_priv->psr.lock);
 
 	intel_psr_disable_locked(intel_dp);
diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
index 46e4de8b8cd5..75a9862f36fd 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.h
+++ b/drivers/gpu/drm/i915/display/intel_psr.h
@@ -35,5 +35,7 @@  void intel_psr_short_pulse(struct intel_dp *intel_dp);
 int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 			    u32 *out_value);
 bool intel_psr_enabled(struct intel_dp *intel_dp);
+void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv);
+void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv);
 
 #endif /* __INTEL_PSR_H__ */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 68fb732c24c8..4521b9381db3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -494,6 +494,7 @@  struct i915_psr {
 	bool link_standby;
 	bool colorimetry_support;
 	bool psr2_enabled;
+	bool psr2_deep_slp_disabled;
 	u8 sink_sync_latency;
 	ktime_t last_entry_attempt;
 	ktime_t last_exit;

Comments

On Fri, Sep 13, 2019 at 01:53:37PM +0530, Anshuman Gupta wrote:
> Add dc3co helper functions to enable/disable psr2 deep sleep.
> Adhere B.Specs by disallow DC3CO state before PSR2 exit.
> Enable PSR2 exitline event and program the desired scanlines
> to exit DC3CO in intel_psr_enable function at modeset path.
> Disable the DC3CO exitline in order to maintian consistent
> pipe config state in encoder disable path.
> 
> v1: Moved calling of tgl_enable_psr2_transcoder_exitline() to
>     intel_psr_enable(). [Imre]
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>

You missed some of the review comments for v7, see below.

> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 51 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_psr.h |  2 +
>  drivers/gpu/drm/i915/i915_drv.h          |  1 +
>  3 files changed, 54 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index b3c7eef53bf3..11d37f96ce71 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -534,6 +534,48 @@ transcoder_has_psr2(struct drm_i915_private *dev_priv, enum transcoder trans)
>  		return trans == TRANSCODER_EDP;
>  }
>  
> +static void psr2_program_idle_frames(struct drm_i915_private *dev_priv,
> +				     u32 idle_frames)
> +{
> +	u32 val;
> +
> +	idle_frames <<=  EDP_PSR2_IDLE_FRAME_SHIFT;
> +	val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
> +	val &= ~EDP_PSR2_IDLE_FRAME_MASK;
> +	val |= idle_frames;
> +	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
> +}
> +
> +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv)
> +{
> +	int idle_frames = 0;

No need for a variable for that.

> +
> +	psr2_program_idle_frames(dev_priv, idle_frames);
> +}
> +
> +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv)
> +{
> +	int idle_frames;
> +
> +	/*
> +	 * Let's use 6 as the minimum to cover all known cases including the
> +	 * off-by-one issue that HW has in some cases.
> +	 */
> +	idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> +	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
> +	psr2_program_idle_frames(dev_priv, idle_frames);
> +}
> +
> +static void tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv)
> +{
> +	if (!IS_TIGERLAKE(dev_priv))
> +		return;

Could we use crtc_state->dc3co_exitline instead (which we would set only
whenever we want to enable DC3co)?

> +
> +	cancel_delayed_work(&dev_priv->csr.idle_work);
> +	/* Before PSR2 exit disallow dc3co*/
> +	tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> +}
> +
>  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  				    struct intel_crtc_state *crtc_state)
>  {
> @@ -799,6 +841,10 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  
>  	WARN_ON(dev_priv->drrs.dp);
>  
> +	/* Enable PSR2 transcoder exit line */
> +	if (crtc_state->has_psr2)
> +		tgl_enable_psr2_transcoder_exitline(crtc_state);

This is too late to program the EXITLINE reg, since the transcoder is
already enabled.

> +
>  	mutex_lock(&dev_priv->psr.lock);
>  
>  	if (!psr_global_enabled(dev_priv->psr.debug)) {
> @@ -829,6 +875,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  	}
>  
>  	if (dev_priv->psr.psr2_enabled) {
> +		tgl_disallow_dc3co_on_psr2_exit(dev_priv);

So DC5/6 will be reenabled, but I can't see how PSR deep sleep gets
reenabled. I think we should have a function that we call both from here
and the idle thread that reenables DC5/6 and also PSR deep sleep.

>  		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
>  		WARN_ON(!(val & EDP_PSR2_ENABLE));
>  		val &= ~EDP_PSR2_ENABLE;
> @@ -895,6 +942,10 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>  	if (WARN_ON(!CAN_PSR(dev_priv)))
>  		return;
>  
> +	/* Disable PSR2 transcoder exit line */
> +	if (old_crtc_state->has_psr2)
> +		tgl_disable_psr2_transcoder_exitline(old_crtc_state);

The transcoder is still active here so we can't program the EXITLINE reg
at this point.

Please add tgl_enable/disable_psr2_transcoder_exitline() in this patch
where the functions are actually used, the review is difficult when I
have to jump between the two patches.

OTOH tgl_psr2_deep_sleep_enable/disable() is used in a follow-up patch,
but you define them already here; those should be also moved where they
are used for easier review.

> +
>  	mutex_lock(&dev_priv->psr.lock);
>  
>  	intel_psr_disable_locked(intel_dp);
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
> index 46e4de8b8cd5..75a9862f36fd 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -35,5 +35,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp);
>  int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
>  			    u32 *out_value);
>  bool intel_psr_enabled(struct intel_dp *intel_dp);
> +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv);
> +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv);
>  
>  #endif /* __INTEL_PSR_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 68fb732c24c8..4521b9381db3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -494,6 +494,7 @@ struct i915_psr {
>  	bool link_standby;
>  	bool colorimetry_support;
>  	bool psr2_enabled;
> +	bool psr2_deep_slp_disabled;
>  	u8 sink_sync_latency;
>  	ktime_t last_entry_attempt;
>  	ktime_t last_exit;
> -- 
> 2.21.0
>
On 2019-09-23 at 19:42:02 +0300, Imre Deak wrote:
> On Fri, Sep 13, 2019 at 01:53:37PM +0530, Anshuman Gupta wrote:
> > Add dc3co helper functions to enable/disable psr2 deep sleep.
> > Adhere B.Specs by disallow DC3CO state before PSR2 exit.
> > Enable PSR2 exitline event and program the desired scanlines
> > to exit DC3CO in intel_psr_enable function at modeset path.
> > Disable the DC3CO exitline in order to maintian consistent
> > pipe config state in encoder disable path.
> > 
> > v1: Moved calling of tgl_enable_psr2_transcoder_exitline() to
> >     intel_psr_enable(). [Imre]
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> You missed some of the review comments for v7, see below.
Ah, somehow i missed this mail itself. https://patchwork.freedesktop.org/patch/329538/?series=64923&rev=8
I will address these comments.
below there are some follow up comments.
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 51 ++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_psr.h |  2 +
> >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> >  3 files changed, 54 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > index b3c7eef53bf3..11d37f96ce71 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -534,6 +534,48 @@ transcoder_has_psr2(struct drm_i915_private *dev_priv, enum transcoder trans)
> >  		return trans == TRANSCODER_EDP;
> >  }
> >  
> > +static void psr2_program_idle_frames(struct drm_i915_private *dev_priv,
> > +				     u32 idle_frames)
> > +{
> > +	u32 val;
> > +
> > +	idle_frames <<=  EDP_PSR2_IDLE_FRAME_SHIFT;
> > +	val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
> > +	val &= ~EDP_PSR2_IDLE_FRAME_MASK;
> > +	val |= idle_frames;
> > +	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
> > +}
> > +
> > +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv)
> > +{
> > +	int idle_frames = 0;
> 
> No need for a variable for that.
> 
> > +
> > +	psr2_program_idle_frames(dev_priv, idle_frames);
> > +}
> > +
> > +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv)
> > +{
> > +	int idle_frames;
> > +
> > +	/*
> > +	 * Let's use 6 as the minimum to cover all known cases including the
> > +	 * off-by-one issue that HW has in some cases.
> > +	 */
> > +	idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> > +	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
> > +	psr2_program_idle_frames(dev_priv, idle_frames);
> > +}
> > +
> > +static void tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv)
> > +{
> > +	if (!IS_TIGERLAKE(dev_priv))
> > +		return;
> 
> Could we use crtc_state->dc3co_exitline instead (which we would set only
> whenever we want to enable DC3co)?
> 
> > +
> > +	cancel_delayed_work(&dev_priv->csr.idle_work);
> > +	/* Before PSR2 exit disallow dc3co*/
> > +	tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> > +}
> > +
> >  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> >  				    struct intel_crtc_state *crtc_state)
> >  {
> > @@ -799,6 +841,10 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> >  
> >  	WARN_ON(dev_priv->drrs.dp);
> >  
> > +	/* Enable PSR2 transcoder exit line */
> > +	if (crtc_state->has_psr2)
> > +		tgl_enable_psr2_transcoder_exitline(crtc_state);
> 
> This is too late to program the EXITLINE reg, since the transcoder is
> already enabled.
intel_psr_enable/disable called by encoder enable/disable fucntion.
AFAIR u seemed to had opinion call it from intel_psr_enable()
https://patchwork.freedesktop.org/patch/323128/?series=64923&rev=3
How about calling these from encoder pre_enable functions ?
or any other better place apart from haswell_crtc_enable/disable.
> 
> > +
> >  	mutex_lock(&dev_priv->psr.lock);
> >  
> >  	if (!psr_global_enabled(dev_priv->psr.debug)) {
> > @@ -829,6 +875,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
> >  	}
> >  
> >  	if (dev_priv->psr.psr2_enabled) {
> > +		tgl_disallow_dc3co_on_psr2_exit(dev_priv);
> 
> So DC5/6 will be reenabled, but I can't see how PSR deep sleep gets
> reenabled. I think we should have a function that we call both from here
> and the idle thread that reenables DC5/6 and also PSR deep sleep.
IMO we don't need to enable PSR2 deep sleep when PSR gets exit, no use of it.
It will always get enabled by intel_psr_activate()->hsw_activate_psr2().
Thanks,
Anshuman Gupta.
> 
> >  		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
> >  		WARN_ON(!(val & EDP_PSR2_ENABLE));
> >  		val &= ~EDP_PSR2_ENABLE;
> > @@ -895,6 +942,10 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> >  	if (WARN_ON(!CAN_PSR(dev_priv)))
> >  		return;
> >  
> > +	/* Disable PSR2 transcoder exit line */
> > +	if (old_crtc_state->has_psr2)
> > +		tgl_disable_psr2_transcoder_exitline(old_crtc_state);
> 
> The transcoder is still active here so we can't program the EXITLINE reg
> at this point.
> 
> Please add tgl_enable/disable_psr2_transcoder_exitline() in this patch
> where the functions are actually used, the review is difficult when I
> have to jump between the two patches.
> 
> OTOH tgl_psr2_deep_sleep_enable/disable() is used in a follow-up patch,
> but you define them already here; those should be also moved where they
> are used for easier review.
> 
> > +
> >  	mutex_lock(&dev_priv->psr.lock);
> >  
> >  	intel_psr_disable_locked(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
> > index 46e4de8b8cd5..75a9862f36fd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> > @@ -35,5 +35,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp);
> >  int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
> >  			    u32 *out_value);
> >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv);
> > +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv);
> >  
> >  #endif /* __INTEL_PSR_H__ */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 68fb732c24c8..4521b9381db3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -494,6 +494,7 @@ struct i915_psr {
> >  	bool link_standby;
> >  	bool colorimetry_support;
> >  	bool psr2_enabled;
> > +	bool psr2_deep_slp_disabled;
> >  	u8 sink_sync_latency;
> >  	ktime_t last_entry_attempt;
> >  	ktime_t last_exit;
> > -- 
> > 2.21.0
> >