| Message ID | 1410909548-4945-4-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 16, 2014 at 07:19:08PM -0400, Rodrigo Vivi wrote: > 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). > > So if something went wrong PSR will be disabled until next full > enable/disable setup. > > 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..658a911 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 up to 24 ms to get fully disabled > + * and be ready for re-enable. > + */ > + if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) & 24ms = frametime for roughly 40Hz. Looks awfully like something Bspec authors just pulled out of thin air. Generally our timeout for waiting for one vblank is 50ms (20Hz, which is a lot less than any sane panel does even with DRRS). Overall series looks sane, please sign someone up for detailed review. Thanks, Daniel > + EDP_PSR_STATUS_STATE_MASK) == 0, 24)) { > + 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
Yeah, this is true. I'm going to send a v2 with 50ms timeout. I forgot that. I sign up Paulo for this review! :) On Wed, Sep 17, 2014 at 8:50 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Sep 16, 2014 at 07:19:08PM -0400, Rodrigo Vivi wrote: > > 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). > > > > So if something went wrong PSR will be disabled until next full > > enable/disable setup. > > > > 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..658a911 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 up to 24 ms to get fully disabled > > + * and be ready for re-enable. > > + */ > > + if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) & > > 24ms = frametime for roughly 40Hz. Looks awfully like something Bspec > authors just pulled out of thin air. > > Generally our timeout for waiting for one vblank is 50ms (20Hz, which is a > lot less than any sane panel does even with DRRS). > > Overall series looks sane, please sign someone up for detailed review. > > Thanks, Daniel > > > + EDP_PSR_STATUS_STATE_MASK) == 0, 24)) { > > + 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch >
Yeah, this is true. I'm going to send a v2 with 50ms timeout. I forgot that. I sign up Paulo for this review! :) On Wed, Sep 17, 2014 at 8:50 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Sep 16, 2014 at 07:19:08PM -0400, Rodrigo Vivi wrote: > > 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). > > > > So if something went wrong PSR will be disabled until next full > > enable/disable setup. > > > > 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..658a911 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 up to 24 ms to get fully disabled > > + * and be ready for re-enable. > > + */ > > + if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) & > > 24ms = frametime for roughly 40Hz. Looks awfully like something Bspec > authors just pulled out of thin air. > > Generally our timeout for waiting for one vblank is 50ms (20Hz, which is a > lot less than any sane panel does even with DRRS). > > Overall series looks sane, please sign someone up for detailed review. > > Thanks, Daniel > > > + EDP_PSR_STATUS_STATE_MASK) == 0, 24)) { > > + 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2f0eee5..658a911 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 up to 24 ms 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, 24)) { + 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). So if something went wrong PSR will be disabled until next full enable/disable setup. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++ 1 file changed, 11 insertions(+)