| Message ID | 1410974587-11491-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-09-17 14:23 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > Let's make sure PSR is propperly disabled before to re-enabled it. > > According to Spec, after disabled PSR CTL, the Idle state might occur > up to 24ms, that is one full frame time (1/refresh rate), > plus SRD exit training time (max of 6ms), > plus SRD aux channel handshake (max of 1.5ms). > > v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However > on low frequency modes this can take longer. So let's use 50ms for safeness. Well, the patch looks correct, but it doesn't seem to take into consideration the fact that we already waited for 100ms before triggering psr.work. Also, we do the wait that you added with psr.lock locked, so we could be blocking user-space from doing other stuff for the whole 50ms, and that's an eternity and a half. So maybe we should tune the schedule_delayed_work() call at intel_edp_psr_flush() based on the calculation you did above (or just keep the 100ms, since it seems to be above the timeout for any modes bigger than 11Hz). And then when we're inside the work function, we should just I915_READ(EDP_PSR_STATUS_CTL) - instead of doing wait_for() -, and in case PSR is not idle yet, there's a huge probability that waiting for more 50ms won't really help. We could also try to reschedule psr.work to be triggered again in the future in case the bits we want are not ready, but by doing this we also risk rescheduling psr.work forever. More bikeshed on the timeout thing: can't we try discover the exact amount of time we need to sleep based on the refresh rate? We could try to look at the mode structure... tl;dr: if you remove the wait_for() call and keep just the I915_READ, I can give a R-B tag, but other patches could be acceptable too. > > So if something went wrong PSR will be disabled until next full > enable/disable setup. > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2f0eee5..2e8c544 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) > WARN_ON(dev_priv->psr.active); > lockdep_assert_held(&dev_priv->psr.lock); > > + /* We have to make sure PSR is ready for re-enable > + * otherwise it keeps disabled until next full enable/disable cycle. > + * PSR might take some time to get fully disabled > + * and be ready for re-enable. > + */ > + if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) & > + EDP_PSR_STATUS_STATE_MASK) == 0, 50)) { > + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > + return; > + } > + > /* Enable/Re-enable PSR on the host */ > intel_edp_psr_enable_source(intel_dp); > > -- > 1.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Sep 24, 2014 at 12:40:20PM -0300, Paulo Zanoni wrote: > 2014-09-17 14:23 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>: > > Let's make sure PSR is propperly disabled before to re-enabled it. > > > > According to Spec, after disabled PSR CTL, the Idle state might occur > > up to 24ms, that is one full frame time (1/refresh rate), > > plus SRD exit training time (max of 6ms), > > plus SRD aux channel handshake (max of 1.5ms). > > > > v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However > > on low frequency modes this can take longer. So let's use 50ms for safeness. > > Well, the patch looks correct, but it doesn't seem to take into > consideration the fact that we already waited for 100ms before > triggering psr.work. Also, we do the wait that you added with psr.lock > locked, so we could be blocking user-space from doing other stuff for > the whole 50ms, and that's an eternity and a half. > > So maybe we should tune the schedule_delayed_work() call at > intel_edp_psr_flush() based on the calculation you did above (or just > keep the 100ms, since it seems to be above the timeout for any modes > bigger than 11Hz). And then when we're inside the work function, we > should just I915_READ(EDP_PSR_STATUS_CTL) - instead of doing > wait_for() -, and in case PSR is not idle yet, there's a huge > probability that waiting for more 50ms won't really help. We could > also try to reschedule psr.work to be triggered again in the future in > case the bits we want are not ready, but by doing this we also risk > rescheduling psr.work forever. > > More bikeshed on the timeout thing: can't we try discover the exact > amount of time we need to sleep based on the refresh rate? We could > try to look at the mode structure... > > tl;dr: if you remove the wait_for() call and keep just the I915_READ, > I can give a R-B tag, but other patches could be acceptable too. Hm, I think just moving the wait_for outside of the psr.lock critical section should be good enough. Only the work item here can enable PSR, so there's not really a race. And on the disable side we always sync with the work before shutting down the psr work, so no synchronization issues either. At worst the dpms off will take a few ms more. Merged the 3rd patch meanwhile, thanks. -Daniel > > > > > > So if something went wrong PSR will be disabled until next full > > enable/disable setup. > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 2f0eee5..2e8c544 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) > > WARN_ON(dev_priv->psr.active); > > lockdep_assert_held(&dev_priv->psr.lock); > > > > + /* We have to make sure PSR is ready for re-enable > > + * otherwise it keeps disabled until next full enable/disable cycle. > > + * PSR might take some time to get fully disabled > > + * and be ready for re-enable. > > + */ > > + if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) & > > + EDP_PSR_STATUS_STATE_MASK) == 0, 50)) { > > + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > + return; > > + } > > + > > /* Enable/Re-enable PSR on the host */ > > intel_edp_psr_enable_source(intel_dp); > > > > -- > > 1.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2f0eee5..2e8c544 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp) WARN_ON(dev_priv->psr.active); lockdep_assert_held(&dev_priv->psr.lock); + /* We have to make sure PSR is ready for re-enable + * otherwise it keeps disabled until next full enable/disable cycle. + * PSR might take some time to get fully disabled + * and be ready for re-enable. + */ + if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) & + EDP_PSR_STATUS_STATE_MASK) == 0, 50)) { + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); + return; + } + /* Enable/Re-enable PSR on the host */ intel_edp_psr_enable_source(intel_dp);
Let's make sure PSR is propperly disabled before to re-enabled it. According to Spec, after disabled PSR CTL, the Idle state might occur up to 24ms, that is one full frame time (1/refresh rate), plus SRD exit training time (max of 6ms), plus SRD aux channel handshake (max of 1.5ms). v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However on low frequency modes this can take longer. So let's use 50ms for safeness. So if something went wrong PSR will be disabled until next full enable/disable setup. Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++ 1 file changed, 11 insertions(+)