[Intel-gfx] [PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC
Souza, Jose
jose.souza at intel.com
Thu Mar 7 23:46:36 UTC 2019
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 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.
Done, thanks.
>
> 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);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190307/428e2fb5/attachment.sig>
More information about the Intel-gfx
mailing list