| Message ID | 20180425215757.2316-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 25, 2018 at 02:57:57PM -0700, Dhinakaran Pandiyan wrote: > Sink crc is calculated by the sink for static frames irrespective of > what the driver sets in TEST_SINK_START dpcd. Since PSR is the only use > case for sink crc, we don't really need the sink_crc_{start, stop} code. > > The second problem with the current implementation is vblank waits. > Enabling vblank interrupts triggers PSR exit, which means we aren't > really reading the correct CRC values for PSR tests. vblank waits are > replaced by delays. > > With the changes made in this patch, sink CRC is available only for > static frames. I have tested this on a SKL laptop with PSR panel. > > v2: Use refresh rate to calculate frame time. Cool! :) > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 4 +- > drivers/gpu/drm/i915/intel_dp.c | 115 +++++------------------------------- > drivers/gpu/drm/i915/intel_drv.h | 3 +- > 3 files changed, 18 insertions(+), 104 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 2f05f5262bba..e4ba6527c16e 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2749,6 +2749,7 @@ static int i915_sink_crc(struct seq_file *m, void *data) > struct drm_crtc *crtc; > struct drm_connector_state *state; > struct intel_crtc_state *crtc_state; > + int vrefresh; > > if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP) > continue; > @@ -2783,8 +2784,9 @@ static int i915_sink_crc(struct seq_file *m, void *data) > } > > intel_dp = enc_to_intel_dp(state->best_encoder); > + vrefresh = crtc_state->base.adjusted_mode.vrefresh; > > - ret = intel_dp_sink_crc(intel_dp, crtc_state, crc); > + ret = intel_dp_sink_crc(intel_dp, vrefresh, crc); > if (ret) > goto err; > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 7dcc874b7d8f..3ab3f82e33f6 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3876,32 +3876,19 @@ intel_dp_configure_mst(struct intel_dp *intel_dp) > intel_dp->is_mst); > } > > -static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp, > - struct intel_crtc_state *crtc_state, bool disable_wa) > +int intel_dp_sink_crc(struct intel_dp *intel_dp, int vrefresh, u8 *crc) > { > - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); > - u8 buf; > - int ret = 0; > - int count = 0; > - int attempts = 10; > + int count = 0, ret = 0, attempts; > + int half_frame_us = DIV_ROUND_UP(1000000/2, vrefresh); > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) { > - DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > - ret = -EIO; > - goto out; > - } > + for (attempts = 0; attempts < 3 && count == 0; attempts++) { > + u8 buf; > > - if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > - buf & ~DP_TEST_SINK_START) < 0) { > - DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > - ret = -EIO; > - goto out; > - } > - > - do { > - intel_wait_for_vblank(dev_priv, intel_crtc->pipe); > + /* Wait for approximately half a frame, we cannot wait for a > + * vblank interrupt as it triggers PSR exit. > + */ > + if (attempts) > + usleep_range(half_frame_us, half_frame_us + 100); > > if (drm_dp_dpcd_readb(&intel_dp->aux, > DP_TEST_SINK_MISC, &buf) < 0) { > @@ -3909,93 +3896,19 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp, > goto out; > } > count = buf & DP_TEST_COUNT_MASK; > - } while (--attempts && count); > - > - if (attempts == 0) { > - DRM_DEBUG_KMS("TIMEOUT: Sink CRC counter is not zeroed after calculation is stopped\n"); > - ret = -ETIMEDOUT; > } > > - out: > - if (disable_wa) > - hsw_enable_ips(crtc_state); > - return ret; > -} > - > -static int intel_dp_sink_crc_start(struct intel_dp *intel_dp, > - struct intel_crtc_state *crtc_state) > -{ > - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); > - u8 buf; > - int ret; > - > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) > - return -EIO; > - > - if (!(buf & DP_TEST_CRC_SUPPORTED)) > - return -ENOTTY; > - > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) > - return -EIO; > - > - if (buf & DP_TEST_SINK_START) { > - ret = intel_dp_sink_crc_stop(intel_dp, crtc_state, false); > - if (ret) > - return ret; > - } > - > - hsw_disable_ips(crtc_state); > - > - if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > - buf | DP_TEST_SINK_START) < 0) { > - hsw_enable_ips(crtc_state); > - return -EIO; > - } > - > - intel_wait_for_vblank(dev_priv, intel_crtc->pipe); > - return 0; > -} > - > -int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state, u8 *crc) > -{ > - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); > - u8 buf; > - int count, ret; > - int attempts = 6; > - > - ret = intel_dp_sink_crc_start(intel_dp, crtc_state); > - if (ret) > - return ret; > - > - do { > - intel_wait_for_vblank(dev_priv, intel_crtc->pipe); > - > - if (drm_dp_dpcd_readb(&intel_dp->aux, > - DP_TEST_SINK_MISC, &buf) < 0) { > - ret = -EIO; > - goto stop; > - } > - count = buf & DP_TEST_COUNT_MASK; > - > - } while (--attempts && count == 0); > - > - if (attempts == 0) { > - DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n"); > + if (attempts == 3) { > ret = -ETIMEDOUT; > - goto stop; > + goto out; > } > > if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) != 6) { > ret = -EIO; > - goto stop; > + goto out; > } > > -stop: > - intel_dp_sink_crc_stop(intel_dp, crtc_state, true); > +out: > return ret; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 44ed248f1fe9..48073be8a023 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1644,8 +1644,7 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > void intel_dp_encoder_reset(struct drm_encoder *encoder); > void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder); > void intel_dp_encoder_destroy(struct drm_encoder *encoder); > -int intel_dp_sink_crc(struct intel_dp *intel_dp, > - struct intel_crtc_state *crtc_state, u8 *crc); > +int intel_dp_sink_crc(struct intel_dp *intel_dp, int vrefresh, u8 *crc); > bool intel_dp_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > struct drm_connector_state *conn_state); > -- > 2.14.1 >
On Wed, Apr 25, 2018 at 02:57:57PM -0700, Dhinakaran Pandiyan wrote: > Sink crc is calculated by the sink for static frames irrespective of > what the driver sets in TEST_SINK_START dpcd. Since PSR is the only use > case for sink crc, we don't really need the sink_crc_{start, stop} code. > > The second problem with the current implementation is vblank waits. > Enabling vblank interrupts triggers PSR exit, which means we aren't > really reading the correct CRC values for PSR tests. vblank waits are > replaced by delays. > > With the changes made in this patch, sink CRC is available only for > static frames. I have tested this on a SKL laptop with PSR panel. > > v2: Use refresh rate to calculate frame time. > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 4 +- > drivers/gpu/drm/i915/intel_dp.c | 115 +++++------------------------------- > drivers/gpu/drm/i915/intel_drv.h | 3 +- > 3 files changed, 18 insertions(+), 104 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 2f05f5262bba..e4ba6527c16e 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2749,6 +2749,7 @@ static int i915_sink_crc(struct seq_file *m, void *data) > struct drm_crtc *crtc; > struct drm_connector_state *state; > struct intel_crtc_state *crtc_state; > + int vrefresh; > > if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP) > continue; > @@ -2783,8 +2784,9 @@ static int i915_sink_crc(struct seq_file *m, void *data) > } > > intel_dp = enc_to_intel_dp(state->best_encoder); > + vrefresh = crtc_state->base.adjusted_mode.vrefresh; Hmm. Is vrefresh always populated correctly for us?
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 2f05f5262bba..e4ba6527c16e 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2749,6 +2749,7 @@ static int i915_sink_crc(struct seq_file *m, void *data) struct drm_crtc *crtc; struct drm_connector_state *state; struct intel_crtc_state *crtc_state; + int vrefresh; if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP) continue; @@ -2783,8 +2784,9 @@ static int i915_sink_crc(struct seq_file *m, void *data) } intel_dp = enc_to_intel_dp(state->best_encoder); + vrefresh = crtc_state->base.adjusted_mode.vrefresh; - ret = intel_dp_sink_crc(intel_dp, crtc_state, crc); + ret = intel_dp_sink_crc(intel_dp, vrefresh, crc); if (ret) goto err; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 7dcc874b7d8f..3ab3f82e33f6 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3876,32 +3876,19 @@ intel_dp_configure_mst(struct intel_dp *intel_dp) intel_dp->is_mst); } -static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp, - struct intel_crtc_state *crtc_state, bool disable_wa) +int intel_dp_sink_crc(struct intel_dp *intel_dp, int vrefresh, u8 *crc) { - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); - struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); - u8 buf; - int ret = 0; - int count = 0; - int attempts = 10; + int count = 0, ret = 0, attempts; + int half_frame_us = DIV_ROUND_UP(1000000/2, vrefresh); - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) { - DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); - ret = -EIO; - goto out; - } + for (attempts = 0; attempts < 3 && count == 0; attempts++) { + u8 buf; - if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, - buf & ~DP_TEST_SINK_START) < 0) { - DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); - ret = -EIO; - goto out; - } - - do { - intel_wait_for_vblank(dev_priv, intel_crtc->pipe); + /* Wait for approximately half a frame, we cannot wait for a + * vblank interrupt as it triggers PSR exit. + */ + if (attempts) + usleep_range(half_frame_us, half_frame_us + 100); if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) { @@ -3909,93 +3896,19 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp, goto out; } count = buf & DP_TEST_COUNT_MASK; - } while (--attempts && count); - - if (attempts == 0) { - DRM_DEBUG_KMS("TIMEOUT: Sink CRC counter is not zeroed after calculation is stopped\n"); - ret = -ETIMEDOUT; } - out: - if (disable_wa) - hsw_enable_ips(crtc_state); - return ret; -} - -static int intel_dp_sink_crc_start(struct intel_dp *intel_dp, - struct intel_crtc_state *crtc_state) -{ - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); - struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); - u8 buf; - int ret; - - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) - return -EIO; - - if (!(buf & DP_TEST_CRC_SUPPORTED)) - return -ENOTTY; - - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) - return -EIO; - - if (buf & DP_TEST_SINK_START) { - ret = intel_dp_sink_crc_stop(intel_dp, crtc_state, false); - if (ret) - return ret; - } - - hsw_disable_ips(crtc_state); - - if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, - buf | DP_TEST_SINK_START) < 0) { - hsw_enable_ips(crtc_state); - return -EIO; - } - - intel_wait_for_vblank(dev_priv, intel_crtc->pipe); - return 0; -} - -int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state, u8 *crc) -{ - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); - struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); - u8 buf; - int count, ret; - int attempts = 6; - - ret = intel_dp_sink_crc_start(intel_dp, crtc_state); - if (ret) - return ret; - - do { - intel_wait_for_vblank(dev_priv, intel_crtc->pipe); - - if (drm_dp_dpcd_readb(&intel_dp->aux, - DP_TEST_SINK_MISC, &buf) < 0) { - ret = -EIO; - goto stop; - } - count = buf & DP_TEST_COUNT_MASK; - - } while (--attempts && count == 0); - - if (attempts == 0) { - DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n"); + if (attempts == 3) { ret = -ETIMEDOUT; - goto stop; + goto out; } if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) != 6) { ret = -EIO; - goto stop; + goto out; } -stop: - intel_dp_sink_crc_stop(intel_dp, crtc_state, true); +out: return ret; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 44ed248f1fe9..48073be8a023 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1644,8 +1644,7 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); void intel_dp_encoder_reset(struct drm_encoder *encoder); void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder); void intel_dp_encoder_destroy(struct drm_encoder *encoder); -int intel_dp_sink_crc(struct intel_dp *intel_dp, - struct intel_crtc_state *crtc_state, u8 *crc); +int intel_dp_sink_crc(struct intel_dp *intel_dp, int vrefresh, u8 *crc); bool intel_dp_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state);
Sink crc is calculated by the sink for static frames irrespective of what the driver sets in TEST_SINK_START dpcd. Since PSR is the only use case for sink crc, we don't really need the sink_crc_{start, stop} code. The second problem with the current implementation is vblank waits. Enabling vblank interrupts triggers PSR exit, which means we aren't really reading the correct CRC values for PSR tests. vblank waits are replaced by delays. With the changes made in this patch, sink CRC is available only for static frames. I have tested this on a SKL laptop with PSR panel. v2: Use refresh rate to calculate frame time. Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 4 +- drivers/gpu/drm/i915/intel_dp.c | 115 +++++------------------------------- drivers/gpu/drm/i915/intel_drv.h | 3 +- 3 files changed, 18 insertions(+), 104 deletions(-)