| Message ID | 20191101001422.209326-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/psr: Share the computation of idle frames | expand |
Looks good to me, there is a minor comment see below. On 2019-10-31 at 17:14:20 -0700, José Roberto de Souza wrote: > Both activate functions and the dc3co disable function were doing the > same thing, so better move to a function and share. > Also while at it adding a WARN_ON to catch invalid values. > > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > Cc: Imre Deak <imre.deak@intel.com> Acked-by: Anshuman Gupta <anshuman.gupta@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 43 +++++++++++------------- > 1 file changed, 19 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 6a9f322d3fca..bb9b5349b72a 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -451,22 +451,29 @@ static u32 intel_psr1_get_tp_time(struct intel_dp *intel_dp) > return val; > } > > -static void hsw_activate_psr1(struct intel_dp *intel_dp) > +static u8 psr_compute_idle_frames(struct intel_dp *intel_dp) > { > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > - u32 max_sleep_time = 0x1f; > - u32 val = EDP_PSR_ENABLE; > + 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. > */ > - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > - > - /* sink_sync_latency of 8 means source has to wait for more than 8 > - * frames, we'll go with 9 frames for now > - */ > + idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1); > - val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > + > + WARN_ON(idle_frames > 0xf); > + > + return idle_frames; there would be return type mismatch warning. > +} > + > +static void hsw_activate_psr1(struct intel_dp *intel_dp) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + u32 max_sleep_time = 0x1f; > + u32 val = EDP_PSR_ENABLE; > + > + val |= psr_compute_idle_frames(intel_dp) << EDP_PSR_IDLE_FRAME_SHIFT; > > val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > if (IS_HASWELL(dev_priv)) > @@ -490,13 +497,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > u32 val; > > - /* Let's use 6 as the minimum to cover all known cases including the > - * off-by-one issue that HW has in some cases. > - */ > - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > - > - idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1); > - val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; > + val = psr_compute_idle_frames(intel_dp) << EDP_PSR2_IDLE_FRAME_SHIFT; > > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > @@ -563,16 +564,10 @@ static void tgl_psr2_enable_dc3co(struct drm_i915_private *dev_priv) > > static void tgl_psr2_disable_dc3co(struct drm_i915_private *dev_priv) > { > - int idle_frames; > + struct intel_dp *intel_dp = dev_priv->psr.dp; > > intel_display_power_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6); > - /* > - * Restore PSR2 idle frame 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); > + psr2_program_idle_frames(dev_priv, psr_compute_idle_frames(intel_dp)); > } > > static void tgl_dc5_idle_thread(struct work_struct *work) > -- > 2.23.0 >
On Wed, 2019-11-13 at 18:04 +0530, Anshuamn Gupta wrote: > Looks good to me, there is a minor comment see below. > On 2019-10-31 at 17:14:20 -0700, José Roberto de Souza wrote: > > Both activate functions and the dc3co disable function were doing > > the > > same thing, so better move to a function and share. > > Also while at it adding a WARN_ON to catch invalid values. > > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > Acked-by: Anshuman Gupta <anshuman.gupta@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 43 +++++++++++--------- > > ---- > > 1 file changed, 19 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index 6a9f322d3fca..bb9b5349b72a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -451,22 +451,29 @@ static u32 intel_psr1_get_tp_time(struct > > intel_dp *intel_dp) > > return val; > > } > > > > -static void hsw_activate_psr1(struct intel_dp *intel_dp) > > +static u8 psr_compute_idle_frames(struct intel_dp *intel_dp) > > { > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > - u32 max_sleep_time = 0x1f; > > - u32 val = EDP_PSR_ENABLE; > > + 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. > > */ > > - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > - > > - /* sink_sync_latency of 8 means source has to wait for more > > than 8 > > - * frames, we'll go with 9 frames for now > > - */ > > + idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency > > + 1); > > - val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > > + > > + WARN_ON(idle_frames > 0xf); > > + > > + return idle_frames; > there would be return type mismatch warning. There is not warning, the value will be truncated but if happens we will get the warn_on above. > > +} > > + > > +static void hsw_activate_psr1(struct intel_dp *intel_dp) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + u32 max_sleep_time = 0x1f; > > + u32 val = EDP_PSR_ENABLE; > > + > > + val |= psr_compute_idle_frames(intel_dp) << > > EDP_PSR_IDLE_FRAME_SHIFT; > > > > val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > > if (IS_HASWELL(dev_priv)) > > @@ -490,13 +497,7 @@ static void hsw_activate_psr2(struct intel_dp > > *intel_dp) > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > u32 val; > > > > - /* Let's use 6 as the minimum to cover all known cases > > including the > > - * off-by-one issue that HW has in some cases. > > - */ > > - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > - > > - idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency > > + 1); > > - val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; > > + val = psr_compute_idle_frames(intel_dp) << > > EDP_PSR2_IDLE_FRAME_SHIFT; > > > > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > @@ -563,16 +564,10 @@ static void tgl_psr2_enable_dc3co(struct > > drm_i915_private *dev_priv) > > > > static void tgl_psr2_disable_dc3co(struct drm_i915_private > > *dev_priv) > > { > > - int idle_frames; > > + struct intel_dp *intel_dp = dev_priv->psr.dp; > > > > intel_display_power_set_target_dc_state(dev_priv, > > DC_STATE_EN_UPTO_DC6); > > - /* > > - * Restore PSR2 idle frame 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); > > + psr2_program_idle_frames(dev_priv, > > psr_compute_idle_frames(intel_dp)); > > } > > > > static void tgl_dc5_idle_thread(struct work_struct *work) > > -- > > 2.23.0 > >
On 2019-11-14 at 04:47:35 +0530, Souza, Jose wrote: > On Wed, 2019-11-13 at 18:04 +0530, Anshuamn Gupta wrote: > > Looks good to me, there is a minor comment see below. > > On 2019-10-31 at 17:14:20 -0700, José Roberto de Souza wrote: > > > Both activate functions and the dc3co disable function were doing > > > the > > > same thing, so better move to a function and share. > > > Also while at it adding a WARN_ON to catch invalid values. > > > > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > > > Cc: Imre Deak <imre.deak@intel.com> > > Acked-by: Anshuman Gupta <anshuman.gupta@intel.com> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 43 +++++++++++--------- > > > ---- > > > 1 file changed, 19 insertions(+), 24 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 6a9f322d3fca..bb9b5349b72a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -451,22 +451,29 @@ static u32 intel_psr1_get_tp_time(struct > > > intel_dp *intel_dp) > > > return val; > > > } > > > > > > -static void hsw_activate_psr1(struct intel_dp *intel_dp) > > > +static u8 psr_compute_idle_frames(struct intel_dp *intel_dp) > > > { > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > - u32 max_sleep_time = 0x1f; > > > - u32 val = EDP_PSR_ENABLE; > > > + 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. > > > */ > > > - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > > - > > > - /* sink_sync_latency of 8 means source has to wait for more > > > than 8 > > > - * frames, we'll go with 9 frames for now > > > - */ > > > + idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > > idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency > > > + 1); > > > - val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > > > + > > > + WARN_ON(idle_frames > 0xf); > > > + > > > + return idle_frames; > > there would be return type mismatch warning. > > There is not warning, the value will be truncated but if happens we > will get the warn_on above. > > > > > +} > > > + > > > +static void hsw_activate_psr1(struct intel_dp *intel_dp) > > > +{ > > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > + u32 max_sleep_time = 0x1f; > > > + u32 val = EDP_PSR_ENABLE; > > > + > > > + val |= psr_compute_idle_frames(intel_dp) << > > > EDP_PSR_IDLE_FRAME_SHIFT; > > > > > > val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > > > if (IS_HASWELL(dev_priv)) > > > @@ -490,13 +497,7 @@ static void hsw_activate_psr2(struct intel_dp > > > *intel_dp) > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > u32 val; > > > > > > - /* Let's use 6 as the minimum to cover all known cases > > > including the > > > - * off-by-one issue that HW has in some cases. > > > - */ > > > - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > > - > > > - idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency > > > + 1); > > > - val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; > > > + val = psr_compute_idle_frames(intel_dp) << > > > EDP_PSR2_IDLE_FRAME_SHIFT; > > > > > > val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > > @@ -563,16 +564,10 @@ static void tgl_psr2_enable_dc3co(struct > > > drm_i915_private *dev_priv) > > > > > > static void tgl_psr2_disable_dc3co(struct drm_i915_private > > > *dev_priv) > > > { > > > - int idle_frames; > > > + struct intel_dp *intel_dp = dev_priv->psr.dp; > > > > > > intel_display_power_set_target_dc_state(dev_priv, > > > DC_STATE_EN_UPTO_DC6); > > > - /* > > > - * Restore PSR2 idle frame 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); > > > + psr2_program_idle_frames(dev_priv, > > > psr_compute_idle_frames(intel_dp)); > > > } > > > > > > static void tgl_dc5_idle_thread(struct work_struct *work) > > > -- > > > 2.23.0 > > >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 6a9f322d3fca..bb9b5349b72a 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -451,22 +451,29 @@ static u32 intel_psr1_get_tp_time(struct intel_dp *intel_dp) return val; } -static void hsw_activate_psr1(struct intel_dp *intel_dp) +static u8 psr_compute_idle_frames(struct intel_dp *intel_dp) { struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); - u32 max_sleep_time = 0x1f; - u32 val = EDP_PSR_ENABLE; + 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. */ - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); - - /* sink_sync_latency of 8 means source has to wait for more than 8 - * frames, we'll go with 9 frames for now - */ + idle_frames = max(6, dev_priv->vbt.psr.idle_frames); idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1); - val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; + + WARN_ON(idle_frames > 0xf); + + return idle_frames; +} + +static void hsw_activate_psr1(struct intel_dp *intel_dp) +{ + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); + u32 max_sleep_time = 0x1f; + u32 val = EDP_PSR_ENABLE; + + val |= psr_compute_idle_frames(intel_dp) << EDP_PSR_IDLE_FRAME_SHIFT; val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; if (IS_HASWELL(dev_priv)) @@ -490,13 +497,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); u32 val; - /* Let's use 6 as the minimum to cover all known cases including the - * off-by-one issue that HW has in some cases. - */ - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); - - idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1); - val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; + val = psr_compute_idle_frames(intel_dp) << EDP_PSR2_IDLE_FRAME_SHIFT; val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) @@ -563,16 +564,10 @@ static void tgl_psr2_enable_dc3co(struct drm_i915_private *dev_priv) static void tgl_psr2_disable_dc3co(struct drm_i915_private *dev_priv) { - int idle_frames; + struct intel_dp *intel_dp = dev_priv->psr.dp; intel_display_power_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6); - /* - * Restore PSR2 idle frame 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); + psr2_program_idle_frames(dev_priv, psr_compute_idle_frames(intel_dp)); } static void tgl_dc5_idle_thread(struct work_struct *work)
Both activate functions and the dc3co disable function were doing the same thing, so better move to a function and share. Also while at it adding a WARN_ON to catch invalid values. Cc: Anshuman Gupta <anshuman.gupta@intel.com> Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 43 +++++++++++------------- 1 file changed, 19 insertions(+), 24 deletions(-)