On Mon, 2018-08-13 at 11:17 -0700, Tarun Vyas wrote:
> On Mon, Aug 13, 2018 at 11:10:00AM -0700, Pandiyan, Dhinakaran wrote:
> > On Mon, 2018-08-13 at 09:57 -0700, Rodrigo Vivi wrote:
> > > 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.v...@intel.com>
> > > > Cc: José Roberto de Souza <jose.so...@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> > > > om>
> > > > ---
> > > > 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.
> >
> > Okay.
> > >
> > > > - 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?
> > >
> >
> > When the patch was written, we did not have sufficient tests to
> > tell us
> > the wait_for_idle condition was wrong for PSR2. It was not known
> > whether the wait was *necessary* for PSR2, think of this as a
> > partial
> > revert. Now that CI has pointed out, (and I checked with a PSR2
> > panel)
> > that the condition is wrong, we should be removing it for PSR2. If
> > you
> > think about it, it does improve PSR2 my removing irrelevant code.
>
> Do we have another way to ensure that we dont try to do a pipe update
> or rather
> check for the PIPE DSL when still in a PSR2 sleep state ?
I don't know, the PSR2 source states aren't documented well enough for
us to implement this change. The current check is clearly wrong, I
think we should remove and fix it correctly when we know it is needed.
> >
> >
> > > > + 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@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
