[Intel-gfx] [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC
Souza, Jose
jose.souza at intel.com
Mon Feb 25 21:30:12 UTC 2019
On Fri, 2019-02-22 at 22:14 -0800, Dhinakaran Pandiyan wrote:
> On Sat, 2019-02-23 at 02:48 +0000, Souza, Jose wrote:
> > On Fri, 2019-02-22 at 18:13 -0800, Dhinakaran Pandiyan wrote:
> > > On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > > > As stated in CRC_CTL spec, after PSR entry state CRC will not
> > > > be
> > > > calculated anymore what is not a problem as IGT tests do some
> > > > screen
> > > > change and then request the pipe CRC right after the change so
> > > > PSR
> > > > will go to idle state and only will entry again after at least
> > > > 6
> > > > idles frames.
> > > >
> > > > But for PSR2 it is more problematic as any change to the screen
> > > > could
> > > > trigger a selective/partial update causing the CRC value not to
> > > > be
> > > > calculated over the full frame.
> > > >
> > > > So here it disables PSR2 and keep it disabled while user is
> > > > requesting pipe CRC.
> > > >
> > > > BSpec: 7536
> > > >
> > > > 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/i915_drv.h | 1 +
> > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> > > > drivers/gpu/drm/i915/intel_psr.c | 23
> > > > +++++++++++++++++++++++
> > > > 4 files changed, 35 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 17fe942eaafa..609e9c5bd453 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -520,6 +520,7 @@ struct i915_psr {
> > > > bool sink_not_reliable;
> > > > bool irq_aux_error;
> > > > u16 su_x_granularity;
> > > > + bool pipe_crc_enabled;
> > > > };
> > > >
> > > > enum intel_pch {
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 3398b28c053b..40ce7a600585 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct
> > > > intel_dp
> > > > *intel_dp);
> > > > int intel_psr_wait_for_idle(const struct intel_crtc_state
> > > > *new_crtc_state,
> > > > u32 *out_value);
> > > > bool intel_psr_enabled(struct intel_dp *intel_dp);
> > > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > > *dev_priv, enum pipe pipe, bool prepare);
> > > >
> > > > /* intel_quirks.c */
> > > > void intel_init_quirks(struct drm_i915_private *dev_priv);
> > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > index a8554dc4f196..5d8772399f60 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name,
> > > > return -EINVAL;
> > > > }
> > > >
> > > > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > > > drm_crtc
> > > > *crtc, bool prepare)
> > > > +{
> > > > + struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > +
> > > > + intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc-
> > > > >pipe,
> > > > prepare);
> > > > +}
> > > > +
> > > > int intel_crtc_set_crc_source(struct drm_crtc *crtc, const
> > > > char
> > > > *source_name)
> > > > {
> > > > struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > > > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct
> > > > drm_crtc
> > > > *crtc, const char *source_name)
> > > > if (ret != 0)
> > > > goto out;
> > > >
> > > > + intel_crtc_crc_prepare_or_finish(crtc, source !=
> > > > INTEL_PIPE_CRC_SOURCE_NONE);
> > > > +
> > > > pipe_crc->source = source;
> > > > I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> > > > POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 08967836b48e..9c93138988aa 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> > > > intel_dp *intel_dp,
> > > > return false;
> > > > }
> > > >
> > > > + if (dev_priv->psr.pipe_crc_enabled)
> > > > + return false;
> > > > +
> > >
> > > Disabling PSR instead of switching to PSR1 is safer considering
> > > the
> > > past bug reports with PSR1.
> >
> > I thought about that but it would break every PSR subtest in
> > kms_frontbuffer_tracking, I guess is better start by disabling PSR2
> > and
> > then discuss about PSR1 if decided to disable, we need to remove
> > PSR1
> > from kms_frontbuffer_tracking first.
>
> I see.
> > >
> > > > return true;
> > > > }
> > > >
> > > > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > > > *intel_dp)
> > > >
> > > > return ret;
> > > > }
> > > > +
> > > > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > > > *dev_priv, enum pipe pipe, bool prepare)
> > > > +{
> > > > + bool fastset = false;
> > > > +
> > > > + if (!CAN_PSR(dev_priv))
> > > > + return;
> > > > +
> > > > + mutex_lock(&dev_priv->psr.lock);
> > > > +
> > > > + if (dev_priv->psr.pipe == pipe) {
> > > > + dev_priv->psr.pipe_crc_enabled = prepare;
> > >
> > > .crc_enabled seems like it belongs in crtc_state rather than in
> > > the
> > > global atomic state.
> > >
> > > Looks like we could rename and re-purpose
> > > crtc_state.ips_force_disable
> > > for this. I don't see that flag being used for anything other
> > > working
> > > around CRC issues.
> > >
> >
> > My understanging is that we should not update crtc_state or any
> > other
> > state outside of atomic_check() call chain, if that is not true I
> > will
> > rename and reuse ips_force_disable.
>
> I don't see it being any different from modifying
> crtc_state->mode_changed to do a fastset. Besides that, all that
> needs
> to be done is rename the flag and shuffle the code a bit.
Ooh okay, I was thiking that you wanted to set it as
psr.pipe_crc_enabled is set in this patch, okay I will do the shuffling
and send other version.
Thanks for the review.
>
> > > > + fastset = !prepare || dev_priv-
> > > > >psr.psr2_enabled;
> > > > + }
> > > > +
> > > > + mutex_unlock(&dev_priv->psr.lock);
> > > > +
> > > > + if (fastset)
> > > > + intel_psr_fastset_force(dev_priv);
> > > > +}
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-------------- 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/20190225/3ad70ada/attachment.sig>
More information about the Intel-gfx
mailing list