On Thu, 2019-03-07 at 13:25 -0800, Dhinakaran Pandiyan wrote: > On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote: > > If PSR is active when pipe CRC is enabled the CRC calculations will > > be inhibit by the transition to low power states that PSR brings. > The MMIO write to enable CRCs should bring the hardware out from PSR, > but I can imagine some initial CRCs are going to be corrupt or > unavailable. That do not happen, if PSR is active and CRC is configured PSR will remain active and no CRC will be calculated. > > > So lets for a PSR exit and as soon as pipe CRC is enabled it will > There is a missing word. Thanks "So lets force a PSR exit and as soon as pipe CRC is enabled it will block PSR activation and avoid CRC timeouts when running IGT tests." > > > block PSR activation avoid CRC timeouts when running IGT tests. > > This surely has fdo bugs, please include them in the commit message. I did this patch because of the regressions that I got from CI in my testings, the only bug that I found related is https://bugs.freedesktop.org/show_bug.cgi?id=107814 but we don't have PSR enabled by default on HSW. > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 36 ++++++++++++++++++++-------- > > ---- > > 1 file changed, 23 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index d3e3996551c6..5d66e7313c75 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -452,6 +452,7 @@ static void hsw_activate_psr1(struct intel_dp > > *intel_dp) > > * frames, we'll go with 9 frames for now > > */ > > idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency > > + 1); > > + > > val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > > > > val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > > @@ -851,6 +852,20 @@ void intel_psr_disable(struct intel_dp > > *intel_dp, > > cancel_work_sync(&dev_priv->psr.work); > > } > > > > +static void psr_force_hw_tracking_exit(struct drm_i915_private > > *dev_priv) > > +{ > > + /* > > + * Display WA #0884: all > > + * This documented WA for bxt can be safely applied > > + * broadly so we can force HW tracking to exit PSR > > + * instead of disabling and re-enabling. > > + * Workaround tells us to write 0 to CUR_SURFLIVE_A, > > + * but it makes more sense write to the current active > > + * pipe. > > + */ > > + I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0); > > +} > > + > > /** > > * intel_psr_update - Update PSR state > > * @intel_dp: Intel DP > > @@ -875,8 +890,13 @@ void intel_psr_update(struct intel_dp > > *intel_dp, > > enable = crtc_state->has_psr && psr_global_enabled(psr->debug); > > psr2_enable = intel_psr2_enabled(dev_priv, crtc_state); > > > > - if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) > > + if (enable == psr->enabled && psr2_enable == psr- > > >psr2_enabled) > > { > > PSR2 is enabled, then user requests CRCs, the mode_changed commit > switches to PSR1. The above condition isn't true in that case. Not sure if I understood the question but if PSR2 is enabled and user request CRC it will switch to PSR1 already forcing a PSR exit so we don't need to call psr_force_hw_tracking_exit() in this case. > > Also, since the CRC workaround is done before enabling pipe CRC, > isn't > there a possibility that the idle frame counter times out and > activates > PSR1 before CRC is enabled? There still the possibility since syscalls can be preempted too but this is a huge improvement over current scenario, now it will give at least the time of 6 idle frames between intel_crtc_crc_setup_workarounds() and the I915_WRITE() to the PIPE CRC registers and it happens right after intel_crtc_crc_setup_workarounds() returns. > > > + /* Force a PSR exit when enabling CRC to avoid CRC > > timeouts */ > > + if (crtc_state->crc_enabled && psr->enabled) > > + psr_force_hw_tracking_exit(dev_priv); > The patch fixes a PSR1 issue, why isn't there any reference to PSR1 > anywhere? fair, going to update the commit message. > > > > + > > goto unlock; > > + } > > > > if (psr->enabled) > > intel_psr_disable_locked(intel_dp); > > @@ -1146,18 +1166,8 @@ void intel_psr_flush(struct drm_i915_private > > *dev_priv, > > dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits; > > > > /* By definition flush = invalidate + flush */ > > - if (frontbuffer_bits) { > > - /* > > - * Display WA #0884: all > > - * This documented WA for bxt can be safely applied > > - * broadly so we can force HW tracking to exit PSR > > - * instead of disabling and re-enabling. > > - * Workaround tells us to write 0 to CUR_SURFLIVE_A, > > - * but it makes more sense write to the current active > > - * pipe. > > - */ > > - I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0); > > - } > > + if (frontbuffer_bits) > > + psr_force_hw_tracking_exit(dev_priv); > > > > if (!dev_priv->psr.active && !dev_priv- > > > psr.busy_frontbuffer_bits) > > schedule_work(&dev_priv->psr.work);
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
