[Intel-gfx] [PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC
Dhinakaran Pandiyan
dhinakaran.pandiyan at intel.com
Thu Mar 7 22:30:36 UTC 2019
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?
> 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.
>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > ---
> > > 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.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
>
> >
> > > + /* 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);
More information about the Intel-gfx
mailing list