| Message ID | 20190214020206.26046-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/psr: Only lookup for enabled CRTCs when forcing a fastset | expand |
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. Okay, that reasoning runs counter to my understanding. My understanding is that the whole frame is fetched and processed at the pipe level but the DDI sends selective blocks of pixels. So, if the CRC's are calculated at the pipe level, the CRC should be for the full frame with PSR2 having no effect? Checking bspec, I see there are DDI CRCs as well, which should reflect the partial frame that PSR2 sends. To get a better understanding, I'd like to know what the source for mismatching CRCs is? > So here it disables PSR2 and keep it disabled while user is > requesting pipe CRC. > > BSpec: 7536 > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@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; > + > 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; > + fastset = !prepare || dev_priv->psr.psr2_enabled; > + } > + > + mutex_unlock(&dev_priv->psr.lock); > + > + if (fastset) > + intel_psr_fastset_force(dev_priv); > +}
On Thu, 2019-02-14 at 11:00 -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. > Okay, that reasoning runs counter to my understanding. My > understanding > is that the whole frame is fetched and processed at the pipe level > but > the DDI sends selective blocks of pixels. So, if the CRC's are > calculated at the pipe level, the CRC should be for the full frame > with > PSR2 having no effect? Checking bspec, I see there are DDI CRCs as > well, which should reflect the partial frame that PSR2 sends. > > To get a better understanding, I'd like to know what the source for > mismatching CRCs is? Well the naming is misleading for newer gens, before BDW this was a pipe register(8067) but now it is a DDI register(7536) but the register offset was kept the same. In my testing hardware generate 4 interruptions with wrong CRC values then stops forever probably because vblank interruptions are off. > > > > So here it disables PSR2 and keep it disabled while user is > > requesting pipe CRC. > > > > BSpec: 7536 > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@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; > > + > > 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; > > + fastset = !prepare || dev_priv->psr.psr2_enabled; > > + } > > + > > + mutex_unlock(&dev_priv->psr.lock); > > + > > + if (fastset) > > + intel_psr_fastset_force(dev_priv); > > +}
On Thu, 2019-02-14 at 15:19 -0800, Souza, Jose wrote: > On Thu, 2019-02-14 at 11:00 -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. > > > > Okay, that reasoning runs counter to my understanding. My > > understanding > > is that the whole frame is fetched and processed at the pipe level > > but > > the DDI sends selective blocks of pixels. So, if the CRC's are > > calculated at the pipe level, the CRC should be for the full frame > > with > > PSR2 having no effect? Checking bspec, I see there are DDI CRCs as > > well, which should reflect the partial frame that PSR2 sends. > > > > To get a better understanding, I'd like to know what the source for > > mismatching CRCs is? > > Well the naming is misleading for newer gens, before BDW this was a > pipe register(8067) but now it is a DDI register(7536) but the > register > offset was kept the same. Do we even read this register DDI_CRC_CTL? Don't see it defined in i915_reg.h or referenced in intel_pipe_crc.c. > > In my testing hardware generate 4 interruptions with wrong CRC values > then stops forever probably because vblank interruptions are off. Yeah, my guess is that power management is affecting the CRC generation rather than pipe CRC's being calculated only on the SU update blocks. > > > > > > > So here it disables PSR2 and keep it disabled while user is > > > requesting pipe CRC. > > > > > > BSpec: 7536 > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@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; > > > + > > > 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; > > > + fastset = !prepare || dev_priv->psr.psr2_enabled; > > > + } > > > + > > > + mutex_unlock(&dev_priv->psr.lock); > > > + > > > + if (fastset) > > > + intel_psr_fastset_force(dev_priv); > > > +}
On Thu, 2019-02-14 at 16:10 -0800, Pandiyan, Dhinakaran wrote: > On Thu, 2019-02-14 at 15:19 -0800, Souza, Jose wrote: > > On Thu, 2019-02-14 at 11:00 -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. > > > > > > Okay, that reasoning runs counter to my understanding. My > > > understanding > > > is that the whole frame is fetched and processed at the pipe > > > level > > > but > > > the DDI sends selective blocks of pixels. So, if the CRC's are > > > calculated at the pipe level, the CRC should be for the full > > > frame > > > with > > > PSR2 having no effect? Checking bspec, I see there are DDI CRCs > > > as > > > well, which should reflect the partial frame that PSR2 sends. > > > > > > To get a better understanding, I'd like to know what the source > > > for > > > mismatching CRCs is? > > > > Well the naming is misleading for newer gens, before BDW this was a > > pipe register(8067) but now it is a DDI register(7536) but the > > register > > offset was kept the same. > Do we even read this register DDI_CRC_CTL? Don't see it defined in > i915_reg.h or referenced in intel_pipe_crc.c. We do: PIPE_CRC_CTL(). #define _PIPE_CRC_CTL_A 0x60050 > > In my testing hardware generate 4 interruptions with wrong CRC > > values > > then stops forever probably because vblank interruptions are off. > Yeah, my guess is that power management is affecting the CRC > generation > rather than pipe CRC's being calculated only on the SU update blocks. > > > > > > > > > So here it disables PSR2 and keep it disabled while user is > > > > requesting pipe CRC. > > > > > > > > BSpec: 7536 > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@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; > > > > + > > > > 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; > > > > + fastset = !prepare || dev_priv- > > > > >psr.psr2_enabled; > > > > + } > > > > + > > > > + mutex_unlock(&dev_priv->psr.lock); > > > > + > > > > + if (fastset) > > > > + intel_psr_fastset_force(dev_priv); > > > > +}
On Fri, 2019-02-15 at 00:17 +0000, Souza, Jose wrote: > On Thu, 2019-02-14 at 16:10 -0800, Pandiyan, Dhinakaran wrote: > > On Thu, 2019-02-14 at 15:19 -0800, Souza, Jose wrote: > > > On Thu, 2019-02-14 at 11:00 -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. > > > > > > > > Okay, that reasoning runs counter to my understanding. My > > > > understanding > > > > is that the whole frame is fetched and processed at the pipe > > > > level > > > > but > > > > the DDI sends selective blocks of pixels. So, if the CRC's are > > > > calculated at the pipe level, the CRC should be for the full > > > > frame > > > > with > > > > PSR2 having no effect? Checking bspec, I see there are DDI CRCs > > > > as > > > > well, which should reflect the partial frame that PSR2 sends. > > > > > > > > To get a better understanding, I'd like to know what the source > > > > for > > > > mismatching CRCs is? > > > > > > Well the naming is misleading for newer gens, before BDW this was > > > a > > > pipe register(8067) but now it is a DDI register(7536) but the > > > register > > > offset was kept the same. > > Do we even read this register DDI_CRC_CTL? Don't see it defined in > > i915_reg.h or referenced in intel_pipe_crc.c. > > We do: PIPE_CRC_CTL(). > #define _PIPE_CRC_CTL_A 0x60050 I was wrong, the first CRC generated is wrong but it is not read by userspace as we skip it using pipe_crc->skipped it is docummented in the code too. The problem here is that after PSR2 is activated(after 'Frames Before SU Entry') there is no more CRC interruptions so no calls to drm_crtc_add_crc_entry(), what don't happen with PSR1 even setting a 'Idle Frames' to 1. Other odd thing that I discovered it that we have CRC_CTL register in DDI(Bspec 7536) and other in PIPE(Bspec 7646) with diferent bits and we write in the transcoder offset using the pipe bits. I tried to use the PIPE offset but it do not generate any interruption, anyone know more about that? > > > > In my testing hardware generate 4 interruptions with wrong CRC > > > values > > > then stops forever probably because vblank interruptions are off. > > Yeah, my guess is that power management is affecting the CRC > > generation > > rather than pipe CRC's being calculated only on the SU update > > blocks. > > > > > > > > > So here it disables PSR2 and keep it disabled while user is > > > > > requesting pipe CRC. > > > > > > > > > > BSpec: 7536 > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@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; > > > > > + > > > > > 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; > > > > > + 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2019-02-15 at 16:20 -0800, José Roberto de Souza wrote: > On Fri, 2019-02-15 at 00:17 +0000, Souza, Jose wrote: > > On Thu, 2019-02-14 at 16:10 -0800, Pandiyan, Dhinakaran wrote: > > > On Thu, 2019-02-14 at 15:19 -0800, Souza, Jose wrote: > > > > On Thu, 2019-02-14 at 11:00 -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. > > > > > > > > > > Okay, that reasoning runs counter to my understanding. My > > > > > understanding > > > > > is that the whole frame is fetched and processed at the pipe > > > > > level > > > > > but > > > > > the DDI sends selective blocks of pixels. So, if the CRC's > > > > > are > > > > > calculated at the pipe level, the CRC should be for the full > > > > > frame > > > > > with > > > > > PSR2 having no effect? Checking bspec, I see there are DDI > > > > > CRCs > > > > > as > > > > > well, which should reflect the partial frame that PSR2 sends. > > > > > > > > > > To get a better understanding, I'd like to know what the > > > > > source > > > > > for > > > > > mismatching CRCs is? > > > > > > > > Well the naming is misleading for newer gens, before BDW this > > > > was > > > > a > > > > pipe register(8067) but now it is a DDI register(7536) but the > > > > register > > > > offset was kept the same. > > > Do we even read this register DDI_CRC_CTL? Don't see it defined > > > in > > > i915_reg.h or referenced in intel_pipe_crc.c. > > > > We do: PIPE_CRC_CTL(). > > #define _PIPE_CRC_CTL_A 0x60050 > > I was wrong, the first CRC generated is wrong but it is not read by > userspace as we skip it using pipe_crc->skipped it is docummented in > the code too. > > The problem here is that after PSR2 is activated(after 'Frames Before > SU Entry') there is no more CRC interruptions so no calls to > drm_crtc_add_crc_entry(), what don't happen with PSR1 even setting a > 'Idle Frames' to 1. So I will update the commit description to: When PSR2 is active aka after the number of frames programmed in PSR2_CTL 'Frames Before SU Entry' hardware stops to generate CRC interruptions causing IGT tests to fail due timeout. Oddly that don't happen when PSR1 active, so here it switches from PSR2 to PSR1 while user is requesting pipe CRC. v2: Changed commit description to describe that PSR2 inhibit CRC calculations. > > Other odd thing that I discovered it that we have CRC_CTL register in > DDI(Bspec 7536) and other in PIPE(Bspec 7646) with diferent bits and > we > write in the transcoder offset using the pipe bits. I tried to use > the > PIPE offset but it do not generate any interruption, anyone know more > about that? > > > > > In my testing hardware generate 4 interruptions with wrong CRC > > > > values > > > > then stops forever probably because vblank interruptions are > > > > off. > > > Yeah, my guess is that power management is affecting the CRC > > > generation > > > rather than pipe CRC's being calculated only on the SU update > > > blocks. > > > > > > > > > > > > So here it disables PSR2 and keep it disabled while user is > > > > > > requesting pipe CRC. > > > > > > > > > > > > BSpec: 7536 > > > > > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@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; > > > > > > + > > > > > > 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; > > > > > > + 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@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. > 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. > + fastset = !prepare || dev_priv->psr.psr2_enabled; > + } > + > + mutex_unlock(&dev_priv->psr.lock); > + > + if (fastset) > + intel_psr_fastset_force(dev_priv); > +}
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@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@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. > > > 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. > > > + fastset = !prepare || dev_priv->psr.psr2_enabled; > > + } > > + > > + mutex_unlock(&dev_priv->psr.lock); > > + > > + if (fastset) > > + intel_psr_fastset_force(dev_priv); > > +}
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@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@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. > > > > > > + 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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; + 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; + fastset = !prepare || dev_priv->psr.psr2_enabled; + } + + mutex_unlock(&dev_priv->psr.lock); + + if (fastset) + intel_psr_fastset_force(dev_priv); +}
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@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: José Roberto de Souza <jose.souza@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(+)