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@xxxxxxxxx>
> Cc: Jose Roberto de Souza <jose.souza@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
