On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan wrote:
> CI runs show PSR2 does not go to IDLE with selective update enabled on
> all PSR exit triggers. Specifically, logs indicate the hardware enters
> "SLEEP Selective Update" and not "IDLE Reset state' like the kernel
> expects. This check was added for PSR1 but incorrectly extended to PSR2,
> remove this check for PSR2 as there is a plan to test only PSR1 on PSR2
> panels.
>
> Also add bspec reference to the comment about idle timeout.
>
> Cc: Tarun Vyas <tarun.vyas@xxxxxxxxx>
> Cc: José 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 | 39 ++++++++++++--------------------
> 1 file changed, 14 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5686ddaa6a72..09be9bfee2be 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
> {
> struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - i915_reg_t reg;
> - u32 mask;
> -
> - if (!new_crtc_state->has_psr)
> - return 0;
>
> /*
> - * The sole user right now is intel_pipe_update_start(),
> - * which won't race with psr_enable/disable, which is
> - * where psr2_enabled is written to. So, we don't need
> - * to acquire the psr.lock. More importantly, we want the
> - * latency inside intel_pipe_update_start() to be as low
> - * as possible, so no need to acquire psr.lock when it is
> - * not needed and will induce latencies in the atomic
> - * update path.
> + * The sole user right now is intel_pipe_update_start(), which won't
> + * race with psr_enable/disable where psr2_enabled is written to. So, we
> + * don't need to acquire the psr.lock. More importantly, we want the
> + * latency inside intel_pipe_update_start() to be as low as possible, so
> + * no need to acquire psr.lock when it is not needed and will induce
> + * latencies in the atomic update path.
> */
I think we shouldn't change this format here to keep patch cleaner...
if there is any change here I couldn't see because it is changing all
lines and if there is no change I think it is better not to touch because
it removes the focus of the real changes.
> - if (dev_priv->psr.psr2_enabled) {
> - reg = EDP_PSR2_STATUS;
> - mask = EDP_PSR2_STATUS_STATE_MASK;
> - } else {
> - reg = EDP_PSR_STATUS;
> - mask = EDP_PSR_STATUS_STATE_MASK;
> - }
> + if (!new_crtc_state->has_psr || READ_ONCE(dev_priv->psr.psr2_enabled))
I now see that we are removing psr2 of the picture, but I don't see how we are
improving psr2 situation here.
what am I missing?
> + return 0;
>
> /*
> - * Max time for PSR to idle = Inverse of the refresh rate +
> - * 6 ms of exit training time + 1.5 ms of aux channel
> - * handshake. 50 msec is defesive enough to cover everything.
> + * From Bspec Panel Self Refresh (BDW+):
This is another case, if we didn't change the format only this line ^
would be in the patch and it would be cleaner and easier to review the
changes.
but my biggest concern with this patch is how do we check now wait_psr2 idle
> + * Max. time for PSR to idle = inverse of the refresh rate + 6 ms of
> + * exit training time + 1.5 ms of aux channel handshake. 50 ms is
> + * defensive enough to cover everything.
> */
> -
> - return __intel_wait_for_register(dev_priv, reg, mask,
> + return __intel_wait_for_register(dev_priv, EDP_PSR_STATUS,
> + EDP_PSR_STATUS_STATE_MASK,
> EDP_PSR_STATUS_STATE_IDLE, 2, 50,
> out_value);
> }
> --
> 2.17.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
