| Message ID | 20180525033047.7596-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 24 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@gmail.com> wrote: > DPCD 2009h "Synchronization latency in sink" has bits that tell us the > maximum number of frames sink can take to resynchronize to source timing > when exiting PSR. More importantly, as per eDP 1.4b, this is the "Minimum > number of frames following PSR exit that the Source device needs to > wait for PSR entry." > > We currently use this value only to setup the number frames to wait before > PSR2 selective update. But, based on the above description it makes more > sense to use this to configure idle frames for both PSR1 and and PSR2. This > will ensure we wait the required number of frames before > activation whether it is PSR1 or PSR2. > > The minimum number of idle frames remains 6, while allowing sink > synchronization latency and VBT to increase this value. > > This also solves the flip-flop between sink and source frames that I > noticed on my Thinkpad X260 during PSR exit. This specific panel has a > value of 8h, which according to the spec means the "Source device must > wait for more than eight active frames after PSR exit before initiating PSR > entry. (In this case, should be provided by the panel supplier.)" VBT > however has a value of 0. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Jose Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> How badly does this depend on the last psr series that was just merged? I.e. does cc: stable make any sense? BR, Jani. > --- > drivers/gpu/drm/i915/intel_psr.c | 40 ++++++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index ebc483f06c6f..71dfe541740f 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -247,6 +247,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) > return; > } > dev_priv->psr.sink_support = true; > + dev_priv->psr.sink_sync_latency = > + intel_dp_get_sink_sync_latency(intel_dp); > > if (INTEL_GEN(dev_priv) >= 9 && > (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > @@ -272,8 +274,6 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) > if (dev_priv->psr.sink_psr2_support) { > dev_priv->psr.colorimetry_support = > intel_dp_get_colorimetry_status(intel_dp); > - dev_priv->psr.sink_sync_latency = > - intel_dp_get_sink_sync_latency(intel_dp); > } > } > } > @@ -370,21 +370,21 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp) > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = dig_port->base.base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > + u32 max_sleep_time = 0x1f; > + u32 val = EDP_PSR_ENABLE; > > - uint32_t max_sleep_time = 0x1f; > - /* > - * Let's respect VBT in case VBT asks a higher idle_frame value. > - * Let's use 6 as the minimum to cover all known cases including > - * the off-by-one issue that HW has in some cases. Also there are > - * cases where sink should be able to train > - * with the 5 or 6 idle patterns. > + /* Let's use 6 as the minimum to cover all known cases including the > + * off-by-one issue that HW has in some cases. > */ > - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > - uint32_t val = EDP_PSR_ENABLE; > + int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > - val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > + /* 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(idle_frames, dev_priv->psr.sink_sync_latency + 1); > val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > > + val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > if (IS_HASWELL(dev_priv)) > val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > @@ -424,15 +424,15 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = dig_port->base.base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > - /* > - * Let's respect VBT in case VBT asks a higher idle_frame value. > - * Let's use 6 as the minimum to cover all known cases including > - * the off-by-one issue that HW has in some cases. Also there are > - * cases where sink should be able to train > - * with the 5 or 6 idle patterns. > + 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. > */ > - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > - u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; > + 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; > > /* FIXME: selective update is probably totally broken because it doesn't > * mesh at all with our frontbuffer tracking. And the hw alone isn't
On Thu, May 24, 2018 at 08:30:47PM -0700, Dhinakaran Pandiyan wrote: > DPCD 2009h "Synchronization latency in sink" has bits that tell us the > maximum number of frames sink can take to resynchronize to source timing > when exiting PSR. More importantly, as per eDP 1.4b, this is the "Minimum > number of frames following PSR exit that the Source device needs to > wait for PSR entry." > > We currently use this value only to setup the number frames to wait before > PSR2 selective update. But, based on the above description it makes more > sense to use this to configure idle frames for both PSR1 and and PSR2. This > will ensure we wait the required number of frames before > activation whether it is PSR1 or PSR2. > > The minimum number of idle frames remains 6, while allowing sink > synchronization latency and VBT to increase this value. > > This also solves the flip-flop between sink and source frames that I > noticed on my Thinkpad X260 during PSR exit. This specific panel has a > value of 8h, which according to the spec means the "Source device must > wait for more than eight active frames after PSR exit before initiating PSR > entry. (In this case, should be provided by the panel supplier.)" VBT > however has a value of 0. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Jose Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 40 ++++++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index ebc483f06c6f..71dfe541740f 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -247,6 +247,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) > return; > } > dev_priv->psr.sink_support = true; > + dev_priv->psr.sink_sync_latency = > + intel_dp_get_sink_sync_latency(intel_dp); > > if (INTEL_GEN(dev_priv) >= 9 && > (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > @@ -272,8 +274,6 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) > if (dev_priv->psr.sink_psr2_support) { > dev_priv->psr.colorimetry_support = > intel_dp_get_colorimetry_status(intel_dp); > - dev_priv->psr.sink_sync_latency = > - intel_dp_get_sink_sync_latency(intel_dp); > } > } > } > @@ -370,21 +370,21 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp) > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = dig_port->base.base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > + u32 max_sleep_time = 0x1f; > + u32 val = EDP_PSR_ENABLE; > > - uint32_t max_sleep_time = 0x1f; > - /* > - * Let's respect VBT in case VBT asks a higher idle_frame value. > - * Let's use 6 as the minimum to cover all known cases including > - * the off-by-one issue that HW has in some cases. Also there are > - * cases where sink should be able to train > - * with the 5 or 6 idle patterns. > + /* Let's use 6 as the minimum to cover all known cases including the > + * off-by-one issue that HW has in some cases. > */ > - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > - uint32_t val = EDP_PSR_ENABLE; > + int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > - val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > + /* 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(idle_frames, dev_priv->psr.sink_sync_latency + 1); > val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > > + val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > if (IS_HASWELL(dev_priv)) > val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > @@ -424,15 +424,15 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = dig_port->base.base.dev; > struct drm_i915_private *dev_priv = to_i915(dev); > - /* > - * Let's respect VBT in case VBT asks a higher idle_frame value. > - * Let's use 6 as the minimum to cover all known cases including > - * the off-by-one issue that HW has in some cases. Also there are > - * cases where sink should be able to train > - * with the 5 or 6 idle patterns. > + 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. > */ > - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > - u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; > + 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; I wonder if we should consolidate all this login in a single function since they are identical and only the shift is different... But the logic is correct. I checked eDP 1.3 and 1.4 specs and I believe we need to move forward with this change and do clean-ups on follow-ups. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Also I don't believe that we need cc:stable because we never enabled it anywhere anyway besides the fact that it wont be a clean backport. > > /* FIXME: selective update is probably totally broken because it doesn't > * mesh at all with our frontbuffer tracking. And the hw alone isn't > -- > 2.14.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, May 29, 2018 at 12:51:23PM -0700, Rodrigo Vivi wrote: > On Thu, May 24, 2018 at 08:30:47PM -0700, Dhinakaran Pandiyan wrote: > > DPCD 2009h "Synchronization latency in sink" has bits that tell us the > > maximum number of frames sink can take to resynchronize to source timing > > when exiting PSR. More importantly, as per eDP 1.4b, this is the "Minimum > > number of frames following PSR exit that the Source device needs to > > wait for PSR entry." > > > > We currently use this value only to setup the number frames to wait before > > PSR2 selective update. But, based on the above description it makes more > > sense to use this to configure idle frames for both PSR1 and and PSR2. This > > will ensure we wait the required number of frames before > > activation whether it is PSR1 or PSR2. > > > > The minimum number of idle frames remains 6, while allowing sink > > synchronization latency and VBT to increase this value. > > > > This also solves the flip-flop between sink and source frames that I > > noticed on my Thinkpad X260 during PSR exit. This specific panel has a > > value of 8h, which according to the spec means the "Source device must > > wait for more than eight active frames after PSR exit before initiating PSR > > entry. (In this case, should be provided by the panel supplier.)" VBT > > however has a value of 0. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Jose Roberto de Souza <jose.souza@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 40 ++++++++++++++++++++-------------------- > > 1 file changed, 20 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index ebc483f06c6f..71dfe541740f 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -247,6 +247,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) > > return; > > } > > dev_priv->psr.sink_support = true; > > + dev_priv->psr.sink_sync_latency = > > + intel_dp_get_sink_sync_latency(intel_dp); > > > > if (INTEL_GEN(dev_priv) >= 9 && > > (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > > @@ -272,8 +274,6 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) > > if (dev_priv->psr.sink_psr2_support) { > > dev_priv->psr.colorimetry_support = > > intel_dp_get_colorimetry_status(intel_dp); > > - dev_priv->psr.sink_sync_latency = > > - intel_dp_get_sink_sync_latency(intel_dp); > > } > > } > > } > > @@ -370,21 +370,21 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp) > > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > + u32 max_sleep_time = 0x1f; > > + u32 val = EDP_PSR_ENABLE; > > > > - uint32_t max_sleep_time = 0x1f; > > - /* > > - * Let's respect VBT in case VBT asks a higher idle_frame value. > > - * Let's use 6 as the minimum to cover all known cases including > > - * the off-by-one issue that HW has in some cases. Also there are > > - * cases where sink should be able to train > > - * with the 5 or 6 idle patterns. > > + /* Let's use 6 as the minimum to cover all known cases including the > > + * off-by-one issue that HW has in some cases. > > */ > > - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > - uint32_t val = EDP_PSR_ENABLE; > > + int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > > > - val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > > + /* 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(idle_frames, dev_priv->psr.sink_sync_latency + 1); > > val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > > > > + val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > > if (IS_HASWELL(dev_priv)) > > val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > > > @@ -424,15 +424,15 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) > > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > - /* > > - * Let's respect VBT in case VBT asks a higher idle_frame value. > > - * Let's use 6 as the minimum to cover all known cases including > > - * the off-by-one issue that HW has in some cases. Also there are > > - * cases where sink should be able to train > > - * with the 5 or 6 idle patterns. > > + 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. > > */ > > - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > - u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; > > + 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; > > I wonder if we should consolidate all this login in a single function since they > are identical and only the shift is different... > > But the logic is correct. I checked eDP 1.3 and 1.4 specs and I believe we > need to move forward with this change and do clean-ups on follow-ups. > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> and pushed to dinq. thanks > > Also I don't believe that we need cc:stable because we never enabled it anywhere > anyway besides the fact that it wont be a clean backport. > > > > > /* FIXME: selective update is probably totally broken because it doesn't > > * mesh at all with our frontbuffer tracking. And the hw alone isn't > > -- > > 2.14.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2018-05-29 at 12:51 -0700, Rodrigo Vivi wrote: > On Thu, May 24, 2018 at 08:30:47PM -0700, Dhinakaran Pandiyan wrote: > > > > DPCD 2009h "Synchronization latency in sink" has bits that tell us > > the > > maximum number of frames sink can take to resynchronize to source > > timing > > when exiting PSR. More importantly, as per eDP 1.4b, this is the > > "Minimum > > number of frames following PSR exit that the Source device needs to > > wait for PSR entry." > > > > We currently use this value only to setup the number frames to wait > > before > > PSR2 selective update. But, based on the above description it makes > > more > > sense to use this to configure idle frames for both PSR1 and and > > PSR2. This > > will ensure we wait the required number of frames before > > activation whether it is PSR1 or PSR2. > > > > The minimum number of idle frames remains 6, while allowing sink > > synchronization latency and VBT to increase this value. > > > > This also solves the flip-flop between sink and source frames that > > I > > noticed on my Thinkpad X260 during PSR exit. This specific panel > > has a > > value of 8h, which according to the spec means the "Source device > > must > > wait for more than eight active frames after PSR exit before > > initiating PSR > > entry. (In this case, should be provided by the panel supplier.)" > > VBT > > however has a value of 0. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Jose Roberto de Souza <jose.souza@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 40 ++++++++++++++++++++------ > > -------------- > > 1 file changed, 20 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index ebc483f06c6f..71dfe541740f 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -247,6 +247,8 @@ void intel_psr_init_dpcd(struct intel_dp > > *intel_dp) > > return; > > } > > dev_priv->psr.sink_support = true; > > + dev_priv->psr.sink_sync_latency = > > + intel_dp_get_sink_sync_latency(intel_dp); > > > > if (INTEL_GEN(dev_priv) >= 9 && > > (intel_dp->psr_dpcd[0] == > > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { > > @@ -272,8 +274,6 @@ void intel_psr_init_dpcd(struct intel_dp > > *intel_dp) > > if (dev_priv->psr.sink_psr2_support) { > > dev_priv->psr.colorimetry_support = > > intel_dp_get_colorimetry_status(in > > tel_dp); > > - dev_priv->psr.sink_sync_latency = > > - intel_dp_get_sink_sync_latency(int > > el_dp); > > } > > } > > } > > @@ -370,21 +370,21 @@ static void hsw_activate_psr1(struct intel_dp > > *intel_dp) > > struct intel_digital_port *dig_port = > > dp_to_dig_port(intel_dp); > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > + u32 max_sleep_time = 0x1f; > > + u32 val = EDP_PSR_ENABLE; > > > > - uint32_t max_sleep_time = 0x1f; > > - /* > > - * Let's respect VBT in case VBT asks a higher idle_frame > > value. > > - * Let's use 6 as the minimum to cover all known cases > > including > > - * the off-by-one issue that HW has in some cases. Also > > there are > > - * cases where sink should be able to train > > - * with the 5 or 6 idle patterns. > > + /* Let's use 6 as the minimum to cover all known cases > > including the > > + * off-by-one issue that HW has in some cases. > > */ > > - uint32_t idle_frames = max(6, dev_priv- > > >vbt.psr.idle_frames); > > - uint32_t val = EDP_PSR_ENABLE; > > + int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > > > - val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > > + /* 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(idle_frames, dev_priv- > > >psr.sink_sync_latency + 1); > > val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > > > > + val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > > if (IS_HASWELL(dev_priv)) > > val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > > > > @@ -424,15 +424,15 @@ static void hsw_activate_psr2(struct intel_dp > > *intel_dp) > > struct intel_digital_port *dig_port = > > dp_to_dig_port(intel_dp); > > struct drm_device *dev = dig_port->base.base.dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > - /* > > - * Let's respect VBT in case VBT asks a higher idle_frame > > value. > > - * Let's use 6 as the minimum to cover all known cases > > including > > - * the off-by-one issue that HW has in some cases. Also > > there are > > - * cases where sink should be able to train > > - * with the 5 or 6 idle patterns. > > + 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. > > */ > > - uint32_t idle_frames = max(6, dev_priv- > > >vbt.psr.idle_frames); > > - u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; > > + 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; > I wonder if we should consolidate all this login in a single function > since they > are identical and only the shift is different... > > But the logic is correct. I checked eDP 1.3 and 1.4 specs and I > believe we > need to move forward with this change and do clean-ups on follow-ups. > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Also I don't believe that we need cc:stable because we never enabled > it anywhere > anyway besides the fact that it wont be a clean backport. Yeah, we've had a couple of commits that touch this code, won't be a clean back port. Thanks for reviewing and merging this patch. > > > > > > > /* FIXME: selective update is probably totally broken > > because it doesn't > > * mesh at all with our frontbuffer tracking. And the hw > > alone isn't
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index ebc483f06c6f..71dfe541740f 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -247,6 +247,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) return; } dev_priv->psr.sink_support = true; + dev_priv->psr.sink_sync_latency = + intel_dp_get_sink_sync_latency(intel_dp); if (INTEL_GEN(dev_priv) >= 9 && (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) { @@ -272,8 +274,6 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp) if (dev_priv->psr.sink_psr2_support) { dev_priv->psr.colorimetry_support = intel_dp_get_colorimetry_status(intel_dp); - dev_priv->psr.sink_sync_latency = - intel_dp_get_sink_sync_latency(intel_dp); } } } @@ -370,21 +370,21 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp) struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct drm_device *dev = dig_port->base.base.dev; struct drm_i915_private *dev_priv = to_i915(dev); + u32 max_sleep_time = 0x1f; + u32 val = EDP_PSR_ENABLE; - uint32_t max_sleep_time = 0x1f; - /* - * Let's respect VBT in case VBT asks a higher idle_frame value. - * Let's use 6 as the minimum to cover all known cases including - * the off-by-one issue that HW has in some cases. Also there are - * cases where sink should be able to train - * with the 5 or 6 idle patterns. + /* Let's use 6 as the minimum to cover all known cases including the + * off-by-one issue that HW has in some cases. */ - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); - uint32_t val = EDP_PSR_ENABLE; + int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); - val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; + /* 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(idle_frames, dev_priv->psr.sink_sync_latency + 1); val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; + val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; if (IS_HASWELL(dev_priv)) val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; @@ -424,15 +424,15 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp) struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct drm_device *dev = dig_port->base.base.dev; struct drm_i915_private *dev_priv = to_i915(dev); - /* - * Let's respect VBT in case VBT asks a higher idle_frame value. - * Let's use 6 as the minimum to cover all known cases including - * the off-by-one issue that HW has in some cases. Also there are - * cases where sink should be able to train - * with the 5 or 6 idle patterns. + 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. */ - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); - u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT; + 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; /* FIXME: selective update is probably totally broken because it doesn't * mesh at all with our frontbuffer tracking. And the hw alone isn't
DPCD 2009h "Synchronization latency in sink" has bits that tell us the maximum number of frames sink can take to resynchronize to source timing when exiting PSR. More importantly, as per eDP 1.4b, this is the "Minimum number of frames following PSR exit that the Source device needs to wait for PSR entry." We currently use this value only to setup the number frames to wait before PSR2 selective update. But, based on the above description it makes more sense to use this to configure idle frames for both PSR1 and and PSR2. This will ensure we wait the required number of frames before activation whether it is PSR1 or PSR2. The minimum number of idle frames remains 6, while allowing sink synchronization latency and VBT to increase this value. This also solves the flip-flop between sink and source frames that I noticed on my Thinkpad X260 during PSR exit. This specific panel has a value of 8h, which according to the spec means the "Source device must wait for more than eight active frames after PSR exit before initiating PSR entry. (In this case, should be provided by the panel supplier.)" VBT however has a value of 0. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Jose Roberto de Souza <jose.souza@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)