On Thu, 2019-03-07 at 14:30 -0800, Dhinakaran Pandiyan wrote: > On Thu, 2019-03-07 at 13:57 -0800, Souza, Jose wrote: > > 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. > > Odd, MMIO writes should trigger DC6 exit, which always results in PSR > exit. If the hardware isnt't in DC6, then I can imagine CRC enabling > not causing a PSR exit. > > > > 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, > Can you include the test name and platform in the commit message? Done > > > 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. > > Jani S had reported the issue, Cc'ing him. As said the issue don't look related to this. > > > > > 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. > The PSR2->PSR1 switching gives us the same window before CRC enabling > as force_hw_tracking_exit(). My question was about if that window is > sufficient. > > > 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. > > Yeah, it should be okay. Please document in the commit message the > idea > is that we rely on the idle frame counter to have not expired before > CRC is enabled and we are only kicking the hardware out of PSR1. > Since > what this patch does is different from what psr_exit() implements, I > think it makes sense to clarify what exit means. > > With the commit message fixed, this looks okay to me but let's see if > the timeouts go away. Done, thanks. > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > > > > > + /* 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
