| Message ID | 20180810004135.11569-2-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/psr: Print PSR_STATUS when PSR idle wait times out. | expand |
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@intel.com> > Cc: José 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 | 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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.vyas@intel.com> > > Cc: José 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 | 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. > > + 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
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.vyas@intel.com> > > > Cc: José 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 | 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 ? > > > > > + 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
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.vyas@intel.com> > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@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
On Mon, 2018-08-13 at 18:10 +0000, 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.vyas@intel.com> > > > Cc: José 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 | 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. > I'll submit a new version without the comment diff if you are satisfied with the above reasoning. > > > > + 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_MA > > > SK > > > , > > > EDP_PSR_STATUS_STATE_ID > > > LE > > > , 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
On Wed, Aug 15, 2018 at 06:35:15PM -0700, Dhinakaran Pandiyan wrote: > On Mon, 2018-08-13 at 18:10 +0000, 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.vyas@intel.com> > > > > Cc: José 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 | 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. > > > I'll submit a new version without the comment diff if you are satisfied > with the above reasoning. Please add a "FIXME:" here and we can fix psr2 situation later when we understand it better... so we at least unblock PSR for now... > > > > > > > + 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_MA > > > > SK > > > > , > > > > EDP_PSR_STATUS_STATE_ID > > > > LE > > > > , 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 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. */ - 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)) + 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+): + * 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); }
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@intel.com> Cc: José 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 | 39 ++++++++++++-------------------- 1 file changed, 14 insertions(+), 25 deletions(-)