| Message ID | 1399277971-2561-1-git-send-email-vandana.kannan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: > Adding relevant read out comparison code, in check_crtc_state, for the new > member of crtc_config, dp_m2_n2, which was introduced to store link_m_n > values for a DP downclock mode (if available). Suggested by Daniel. > > v2: Changed patch title. > Daniel's review comments incorporated. > Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done > only when high RR is not in use (This is because alternate m_n register > programming will be done only when low RR is being used). > > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_ddi.c | 1 + > drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- > drivers/gpu/drm/i915/intel_dp.c | 2 + > drivers/gpu/drm/i915/intel_drv.h | 2 + > 4 files changed, 72 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 0ad4e96..6784f0b 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, > case TRANS_DDI_MODE_SELECT_DP_MST: > pipe_config->has_dp_encoder = true; > intel_dp_get_m_n(intel_crtc, pipe_config); > + intel_dp_get_m2_n2(intel_crtc, pipe_config); > break; > default: > break; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 797f01c..2e625eb 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > &pipe_config->dp_m_n); > } > > +void intel_dp_get_m2_n2(struct intel_crtc *crtc, > + struct intel_crtc_config *pipe_config) > +{ > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + enum transcoder transcoder = pipe_config->cpu_transcoder; > + > + if (INTEL_INFO(dev)->gen >= 8) { > + intel_cpu_transcoder_get_m_n(crtc, transcoder, > + &pipe_config->dp_m_n); dm_m2_n2 surely? And why do we even want to do this? > + } else if (INTEL_INFO(dev)->gen > 6) { > + pipe_config->dp_m2_n2.link_m = > + I915_READ(PIPE_LINK_M2(transcoder)); > + pipe_config->dp_m2_n2.link_n = > + I915_READ(PIPE_LINK_N2(transcoder)); > + pipe_config->dp_m2_n2.gmch_m = > + I915_READ(PIPE_DATA_M2(transcoder)) > + & ~TU_SIZE_MASK; > + pipe_config->dp_m2_n2.gmch_n = > + I915_READ(PIPE_DATA_N2(transcoder)); > + pipe_config->dp_m2_n2.tu = > + ((I915_READ(PIPE_DATA_M2(transcoder)) > + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > + } > + > +} > > + > static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, > struct intel_crtc_config *pipe_config) > { > @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, > pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, > pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, > pipe_config->dp_m_n.tu); > + > + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", > + pipe_config->has_dp_encoder, > + pipe_config->dp_m2_n2.gmch_m, > + pipe_config->dp_m2_n2.gmch_n, > + pipe_config->dp_m2_n2.link_m, > + pipe_config->dp_m2_n2.link_n, > + pipe_config->dp_m2_n2.tu); > + > DRM_DEBUG_KMS("requested mode:\n"); > drm_mode_debug_printmodeline(&pipe_config->requested_mode); > DRM_DEBUG_KMS("adjusted mode:\n"); > @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, > struct intel_crtc_config *current_config, > struct intel_crtc_config *pipe_config) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_connector *intel_connector = dev_priv->drrs.connector; > + struct intel_encoder *encoder = (intel_connector != NULL) ? > + intel_attached_encoder(&intel_connector->base) : > + NULL; > + struct intel_dp *intel_dp = (encoder != NULL) ? > + enc_to_intel_dp(&encoder->base) : NULL; > + > #define PIPE_CONF_CHECK_X(name) \ > if (current_config->name != pipe_config->name) { \ > DRM_ERROR("mismatch in " #name " " \ > @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_I(fdi_m_n.tu); > > PIPE_CONF_CHECK_I(has_dp_encoder); > - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); > - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); > - PIPE_CONF_CHECK_I(dp_m_n.link_m); > - PIPE_CONF_CHECK_I(dp_m_n.link_n); > - PIPE_CONF_CHECK_I(dp_m_n.tu); > + > + > + /* DP M1_N1 registers are used when DRRS is disabled or when high RR > + * is used. Else, DP M2_N2 registers are used. The following check > + * has been added to make sure a mismatch (if any) is displayed only > + * for a real difference and not because of DRRS state. > + */ > + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || > + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && > + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { > + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); > + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); > + PIPE_CONF_CHECK_I(dp_m_n.link_m); > + PIPE_CONF_CHECK_I(dp_m_n.link_n); > + PIPE_CONF_CHECK_I(dp_m_n.tu); > + } else { > + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); > + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); > + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); > + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); > + PIPE_CONF_CHECK_I(dp_m2_n2.tu); > + } Can't we just check both dp_m_n and dp_m2_n2 always? > > PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); > PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 34ed143..9aa4dcd 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder, > > intel_dp_get_m_n(crtc, pipe_config); > > + intel_dp_get_m2_n2(crtc, pipe_config); > + > if (port == PORT_A) { > if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ) > pipe_config->port_clock = 162000; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index d8b540b..1013f70 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv); > void hsw_disable_pc8(struct drm_i915_private *dev_priv); > void intel_dp_get_m_n(struct intel_crtc *crtc, > struct intel_crtc_config *pipe_config); > +void intel_dp_get_m2_n2(struct intel_crtc *crtc, > + struct intel_crtc_config *pipe_config); > int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); > void > ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, > -- > 1.9.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 12, 2014 at 01:27:25PM +0300, Ville Syrjälä wrote: > On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: > > Adding relevant read out comparison code, in check_crtc_state, for the new > > member of crtc_config, dp_m2_n2, which was introduced to store link_m_n > > values for a DP downclock mode (if available). Suggested by Daniel. > > > > v2: Changed patch title. > > Daniel's review comments incorporated. > > Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done > > only when high RR is not in use (This is because alternate m_n register > > programming will be done only when low RR is being used). > > > > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 1 + > > drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- > > drivers/gpu/drm/i915/intel_dp.c | 2 + > > drivers/gpu/drm/i915/intel_drv.h | 2 + > > 4 files changed, 72 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 0ad4e96..6784f0b 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, > > case TRANS_DDI_MODE_SELECT_DP_MST: > > pipe_config->has_dp_encoder = true; > > intel_dp_get_m_n(intel_crtc, pipe_config); > > + intel_dp_get_m2_n2(intel_crtc, pipe_config); > > break; > > default: > > break; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 797f01c..2e625eb 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > > &pipe_config->dp_m_n); > > } > > > > +void intel_dp_get_m2_n2(struct intel_crtc *crtc, > > + struct intel_crtc_config *pipe_config) > > +{ > > + struct drm_device *dev = crtc->base.dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + enum transcoder transcoder = pipe_config->cpu_transcoder; > > + > > + if (INTEL_INFO(dev)->gen >= 8) { > > + intel_cpu_transcoder_get_m_n(crtc, transcoder, > > + &pipe_config->dp_m_n); > > dm_m2_n2 surely? And why do we even want to do this? > > > + } else if (INTEL_INFO(dev)->gen > 6) { > > + pipe_config->dp_m2_n2.link_m = > > + I915_READ(PIPE_LINK_M2(transcoder)); > > + pipe_config->dp_m2_n2.link_n = > > + I915_READ(PIPE_LINK_N2(transcoder)); > > + pipe_config->dp_m2_n2.gmch_m = > > + I915_READ(PIPE_DATA_M2(transcoder)) > > + & ~TU_SIZE_MASK; > > + pipe_config->dp_m2_n2.gmch_n = > > + I915_READ(PIPE_DATA_N2(transcoder)); > > + pipe_config->dp_m2_n2.tu = > > + ((I915_READ(PIPE_DATA_M2(transcoder)) > > + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > > + } > > + > > +} > > > > + > > static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, > > struct intel_crtc_config *pipe_config) > > { > > @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, > > pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, > > pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, > > pipe_config->dp_m_n.tu); > > + > > + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", > > + pipe_config->has_dp_encoder, > > + pipe_config->dp_m2_n2.gmch_m, > > + pipe_config->dp_m2_n2.gmch_n, > > + pipe_config->dp_m2_n2.link_m, > > + pipe_config->dp_m2_n2.link_n, > > + pipe_config->dp_m2_n2.tu); > > + > > DRM_DEBUG_KMS("requested mode:\n"); > > drm_mode_debug_printmodeline(&pipe_config->requested_mode); > > DRM_DEBUG_KMS("adjusted mode:\n"); > > @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, > > struct intel_crtc_config *current_config, > > struct intel_crtc_config *pipe_config) > > { > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct intel_connector *intel_connector = dev_priv->drrs.connector; > > + struct intel_encoder *encoder = (intel_connector != NULL) ? > > + intel_attached_encoder(&intel_connector->base) : > > + NULL; > > + struct intel_dp *intel_dp = (encoder != NULL) ? > > + enc_to_intel_dp(&encoder->base) : NULL; > > + > > #define PIPE_CONF_CHECK_X(name) \ > > if (current_config->name != pipe_config->name) { \ > > DRM_ERROR("mismatch in " #name " " \ > > @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, > > PIPE_CONF_CHECK_I(fdi_m_n.tu); > > > > PIPE_CONF_CHECK_I(has_dp_encoder); > > - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); > > - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); > > - PIPE_CONF_CHECK_I(dp_m_n.link_m); > > - PIPE_CONF_CHECK_I(dp_m_n.link_n); > > - PIPE_CONF_CHECK_I(dp_m_n.tu); > > + > > + > > + /* DP M1_N1 registers are used when DRRS is disabled or when high RR > > + * is used. Else, DP M2_N2 registers are used. The following check > > + * has been added to make sure a mismatch (if any) is displayed only > > + * for a real difference and not because of DRRS state. > > + */ > > + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || > > + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && > > + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { Also this check violates the overall pipe config checking. The only things you should look at when comparing the pipe config are: 1. The pipe configurations invovled. 2. Invariant device information like the gpu genearation. If you need to look at other state (like intel_dp state) then either this shouldn't be in the pipe config or something else is missing from the pipe config. Also the DRRS_NOT_SUPPORTED checks should be dropped. Pipe config is about the actual hardware state, not what the sw believes to be true. Which means you need to unconditionally compare all hardware state, whether the feature is enabled on the sw side or not. E.g. we check pfit state even on non-edp/lvds outputs since the hardware can do that, even though our driver does not support this. Similar for DRRS (which in theory is possible for DP outputs, e.g. for video playback at _exactly_ the right frequency). -Daniel > > + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); > > + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); > > + PIPE_CONF_CHECK_I(dp_m_n.link_m); > > + PIPE_CONF_CHECK_I(dp_m_n.link_n); > > + PIPE_CONF_CHECK_I(dp_m_n.tu); > > + } else { > > + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); > > + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); > > + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); > > + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); > > + PIPE_CONF_CHECK_I(dp_m2_n2.tu); > > + } > > Can't we just check both dp_m_n and dp_m2_n2 always? > > > > > PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); > > PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 34ed143..9aa4dcd 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder, > > > > intel_dp_get_m_n(crtc, pipe_config); > > > > + intel_dp_get_m2_n2(crtc, pipe_config); > > + > > if (port == PORT_A) { > > if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ) > > pipe_config->port_clock = 162000; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index d8b540b..1013f70 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv); > > void hsw_disable_pc8(struct drm_i915_private *dev_priv); > > void intel_dp_get_m_n(struct intel_crtc *crtc, > > struct intel_crtc_config *pipe_config); > > +void intel_dp_get_m2_n2(struct intel_crtc *crtc, > > + struct intel_crtc_config *pipe_config); > > int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); > > void > > ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, > > -- > > 1.9.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
On May-12-2014 3:57 PM, Ville Syrjälä wrote: > On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: >> Adding relevant read out comparison code, in check_crtc_state, for the new >> member of crtc_config, dp_m2_n2, which was introduced to store link_m_n >> values for a DP downclock mode (if available). Suggested by Daniel. >> >> v2: Changed patch title. >> Daniel's review comments incorporated. >> Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done >> only when high RR is not in use (This is because alternate m_n register >> programming will be done only when low RR is being used). >> >> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 1 + >> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- >> drivers/gpu/drm/i915/intel_dp.c | 2 + >> drivers/gpu/drm/i915/intel_drv.h | 2 + >> 4 files changed, 72 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 0ad4e96..6784f0b 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >> case TRANS_DDI_MODE_SELECT_DP_MST: >> pipe_config->has_dp_encoder = true; >> intel_dp_get_m_n(intel_crtc, pipe_config); >> + intel_dp_get_m2_n2(intel_crtc, pipe_config); >> break; >> default: >> break; >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 797f01c..2e625eb 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >> &pipe_config->dp_m_n); >> } >> >> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >> + struct intel_crtc_config *pipe_config) >> +{ >> + struct drm_device *dev = crtc->base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + enum transcoder transcoder = pipe_config->cpu_transcoder; >> + >> + if (INTEL_INFO(dev)->gen >= 8) { >> + intel_cpu_transcoder_get_m_n(crtc, transcoder, >> + &pipe_config->dp_m_n); > > dm_m2_n2 surely? And why do we even want to do this? > My miss, will change this. For Gen8, there is only one set of MN registers to be programmed for high and low RR. Hence this check. >> + } else if (INTEL_INFO(dev)->gen > 6) { >> + pipe_config->dp_m2_n2.link_m = >> + I915_READ(PIPE_LINK_M2(transcoder)); >> + pipe_config->dp_m2_n2.link_n = >> + I915_READ(PIPE_LINK_N2(transcoder)); >> + pipe_config->dp_m2_n2.gmch_m = >> + I915_READ(PIPE_DATA_M2(transcoder)) >> + & ~TU_SIZE_MASK; >> + pipe_config->dp_m2_n2.gmch_n = >> + I915_READ(PIPE_DATA_N2(transcoder)); >> + pipe_config->dp_m2_n2.tu = >> + ((I915_READ(PIPE_DATA_M2(transcoder)) >> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >> + } >> + >> +} >> >> + >> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >> struct intel_crtc_config *pipe_config) >> { >> @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, >> pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, >> pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, >> pipe_config->dp_m_n.tu); >> + >> + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", >> + pipe_config->has_dp_encoder, >> + pipe_config->dp_m2_n2.gmch_m, >> + pipe_config->dp_m2_n2.gmch_n, >> + pipe_config->dp_m2_n2.link_m, >> + pipe_config->dp_m2_n2.link_n, >> + pipe_config->dp_m2_n2.tu); >> + >> DRM_DEBUG_KMS("requested mode:\n"); >> drm_mode_debug_printmodeline(&pipe_config->requested_mode); >> DRM_DEBUG_KMS("adjusted mode:\n"); >> @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, >> struct intel_crtc_config *current_config, >> struct intel_crtc_config *pipe_config) >> { >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_connector *intel_connector = dev_priv->drrs.connector; >> + struct intel_encoder *encoder = (intel_connector != NULL) ? >> + intel_attached_encoder(&intel_connector->base) : >> + NULL; >> + struct intel_dp *intel_dp = (encoder != NULL) ? >> + enc_to_intel_dp(&encoder->base) : NULL; >> + >> #define PIPE_CONF_CHECK_X(name) \ >> if (current_config->name != pipe_config->name) { \ >> DRM_ERROR("mismatch in " #name " " \ >> @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, >> PIPE_CONF_CHECK_I(fdi_m_n.tu); >> >> PIPE_CONF_CHECK_I(has_dp_encoder); >> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >> - PIPE_CONF_CHECK_I(dp_m_n.link_m); >> - PIPE_CONF_CHECK_I(dp_m_n.link_n); >> - PIPE_CONF_CHECK_I(dp_m_n.tu); >> + >> + >> + /* DP M1_N1 registers are used when DRRS is disabled or when high RR >> + * is used. Else, DP M2_N2 registers are used. The following check >> + * has been added to make sure a mismatch (if any) is displayed only >> + * for a real difference and not because of DRRS state. >> + */ >> + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || >> + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && >> + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { >> + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >> + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >> + PIPE_CONF_CHECK_I(dp_m_n.link_m); >> + PIPE_CONF_CHECK_I(dp_m_n.link_n); >> + PIPE_CONF_CHECK_I(dp_m_n.tu); >> + } else { >> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); >> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); >> + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); >> + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); >> + PIPE_CONF_CHECK_I(dp_m2_n2.tu); >> + } > > Can't we just check both dp_m_n and dp_m2_n2 always? > Daniel/Ville, pipe_config->dp_m2_n2 is populated along with pipe_config->dp_m_n in compute_config(). while pipe_config dp_m_n values are programmed into MN registers during the mode set, dp_m2_n2 values are programmed only when low RR is used. Given this, when PIPE_CONF_CHECK is done for dp_m2_n2, it would show a mismatch (struct contains values to be programmed but registers still have 0) and return. The reason these checks were added was to avoid the scenario in which a mismatch is highlighted when there is no real failure at all. For example, DRRS is enabled on boot but the scenario to trigger low RR has not yet occurred, dp_m2_n2 has been computed but M2_N2 registers are not programmed in this case. There will be a mismatch in the pipe_config check. Changes can be made to set M2_N2 registers when dp_m2_n2 is populated and toggle the bit (PIPECONF bit) to switch between high and low RR, but this becomes invalid for gen8 and above where only 1 set of MN registers are available. Please suggest some other way to handle this. May be a quirk like the following? /* * FIXME: BIOS likes to set up a cloned config with lvds+external * screen. Since we don't yet re-compute the pipe config when moving * just the lvds port away to another pipe the sw tracking won't match. * * Proper atomic modesets with recomputed global state will fix this. * Until then just don't check gmch state for inherited modes. */ if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { Or go ahead with checking dp_m_n and dp_m2_n2 always ? Please let me know.. -Vandana >> >> PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); >> PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 34ed143..9aa4dcd 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder, >> >> intel_dp_get_m_n(crtc, pipe_config); >> >> + intel_dp_get_m2_n2(crtc, pipe_config); >> + >> if (port == PORT_A) { >> if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ) >> pipe_config->port_clock = 162000; >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index d8b540b..1013f70 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv); >> void hsw_disable_pc8(struct drm_i915_private *dev_priv); >> void intel_dp_get_m_n(struct intel_crtc *crtc, >> struct intel_crtc_config *pipe_config); >> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >> + struct intel_crtc_config *pipe_config); >> int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); >> void >> ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, >> -- >> 1.9.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Tue, May 13, 2014 at 01:56:04PM +0530, Vandana Kannan wrote: > On May-12-2014 3:57 PM, Ville Syrjälä wrote: > > On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: > >> Adding relevant read out comparison code, in check_crtc_state, for the new > >> member of crtc_config, dp_m2_n2, which was introduced to store link_m_n > >> values for a DP downclock mode (if available). Suggested by Daniel. > >> > >> v2: Changed patch title. > >> Daniel's review comments incorporated. > >> Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done > >> only when high RR is not in use (This is because alternate m_n register > >> programming will be done only when low RR is being used). > >> > >> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> --- > >> drivers/gpu/drm/i915/intel_ddi.c | 1 + > >> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- > >> drivers/gpu/drm/i915/intel_dp.c | 2 + > >> drivers/gpu/drm/i915/intel_drv.h | 2 + > >> 4 files changed, 72 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> index 0ad4e96..6784f0b 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, > >> case TRANS_DDI_MODE_SELECT_DP_MST: > >> pipe_config->has_dp_encoder = true; > >> intel_dp_get_m_n(intel_crtc, pipe_config); > >> + intel_dp_get_m2_n2(intel_crtc, pipe_config); > >> break; > >> default: > >> break; > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 797f01c..2e625eb 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > >> &pipe_config->dp_m_n); > >> } > >> > >> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, > >> + struct intel_crtc_config *pipe_config) > >> +{ > >> + struct drm_device *dev = crtc->base.dev; > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + enum transcoder transcoder = pipe_config->cpu_transcoder; > >> + > >> + if (INTEL_INFO(dev)->gen >= 8) { > >> + intel_cpu_transcoder_get_m_n(crtc, transcoder, > >> + &pipe_config->dp_m_n); > > > > dm_m2_n2 surely? And why do we even want to do this? > > > My miss, will change this. > For Gen8, there is only one set of MN registers to be programmed for > high and low RR. Hence this check. > >> + } else if (INTEL_INFO(dev)->gen > 6) { > >> + pipe_config->dp_m2_n2.link_m = > >> + I915_READ(PIPE_LINK_M2(transcoder)); > >> + pipe_config->dp_m2_n2.link_n = > >> + I915_READ(PIPE_LINK_N2(transcoder)); > >> + pipe_config->dp_m2_n2.gmch_m = > >> + I915_READ(PIPE_DATA_M2(transcoder)) > >> + & ~TU_SIZE_MASK; > >> + pipe_config->dp_m2_n2.gmch_n = > >> + I915_READ(PIPE_DATA_N2(transcoder)); > >> + pipe_config->dp_m2_n2.tu = > >> + ((I915_READ(PIPE_DATA_M2(transcoder)) > >> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > >> + } > >> + > >> +} > >> > >> + > >> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, > >> struct intel_crtc_config *pipe_config) > >> { > >> @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, > >> pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, > >> pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, > >> pipe_config->dp_m_n.tu); > >> + > >> + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", > >> + pipe_config->has_dp_encoder, > >> + pipe_config->dp_m2_n2.gmch_m, > >> + pipe_config->dp_m2_n2.gmch_n, > >> + pipe_config->dp_m2_n2.link_m, > >> + pipe_config->dp_m2_n2.link_n, > >> + pipe_config->dp_m2_n2.tu); > >> + > >> DRM_DEBUG_KMS("requested mode:\n"); > >> drm_mode_debug_printmodeline(&pipe_config->requested_mode); > >> DRM_DEBUG_KMS("adjusted mode:\n"); > >> @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, > >> struct intel_crtc_config *current_config, > >> struct intel_crtc_config *pipe_config) > >> { > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> + struct intel_connector *intel_connector = dev_priv->drrs.connector; > >> + struct intel_encoder *encoder = (intel_connector != NULL) ? > >> + intel_attached_encoder(&intel_connector->base) : > >> + NULL; > >> + struct intel_dp *intel_dp = (encoder != NULL) ? > >> + enc_to_intel_dp(&encoder->base) : NULL; > >> + > >> #define PIPE_CONF_CHECK_X(name) \ > >> if (current_config->name != pipe_config->name) { \ > >> DRM_ERROR("mismatch in " #name " " \ > >> @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, > >> PIPE_CONF_CHECK_I(fdi_m_n.tu); > >> > >> PIPE_CONF_CHECK_I(has_dp_encoder); > >> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); > >> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); > >> - PIPE_CONF_CHECK_I(dp_m_n.link_m); > >> - PIPE_CONF_CHECK_I(dp_m_n.link_n); > >> - PIPE_CONF_CHECK_I(dp_m_n.tu); > >> + > >> + > >> + /* DP M1_N1 registers are used when DRRS is disabled or when high RR > >> + * is used. Else, DP M2_N2 registers are used. The following check > >> + * has been added to make sure a mismatch (if any) is displayed only > >> + * for a real difference and not because of DRRS state. > >> + */ > >> + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || > >> + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && > >> + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { > >> + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); > >> + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); > >> + PIPE_CONF_CHECK_I(dp_m_n.link_m); > >> + PIPE_CONF_CHECK_I(dp_m_n.link_n); > >> + PIPE_CONF_CHECK_I(dp_m_n.tu); > >> + } else { > >> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); > >> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); > >> + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); > >> + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); > >> + PIPE_CONF_CHECK_I(dp_m2_n2.tu); > >> + } > > > > Can't we just check both dp_m_n and dp_m2_n2 always? > > > Daniel/Ville, > > pipe_config->dp_m2_n2 is populated along with pipe_config->dp_m_n in > compute_config(). while pipe_config dp_m_n values are programmed into MN > registers during the mode set, dp_m2_n2 values are programmed only when > low RR is used. > Given this, when PIPE_CONF_CHECK is done for dp_m2_n2, it would show a > mismatch (struct contains values to be programmed but registers still > have 0) and return. > The reason these checks were added was to avoid the scenario in which a > mismatch is highlighted when there is no real failure at all. For > example, DRRS is enabled on boot but the scenario to trigger low RR has > not yet occurred, dp_m2_n2 has been computed but M2_N2 registers are not > programmed in this case. There will be a mismatch in the pipe_config check. > > Changes can be made to set M2_N2 registers when dp_m2_n2 is populated > and toggle the bit (PIPECONF bit) to switch between high and low RR, but > this becomes invalid for gen8 and above where only 1 set of MN registers > are available. > > Please suggest some other way to handle this. > May be a quirk like the following? > > /* > * FIXME: BIOS likes to set up a cloned config with lvds+external > * screen. Since we don't yet re-compute the pipe config when moving > * just the lvds port away to another pipe the sw tracking won't > match. > * > * Proper atomic modesets with recomputed global state will fix > this. > * Until then just don't check gmch state for inherited modes. > */ > if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { > > Or go ahead with checking dp_m_n and dp_m2_n2 always ? > Please let me know.. I think for BDW we should check whether the hw values match _either_ dp_m_n or dp_m2_n2. But for hsw and earlier we should check both. That leaves us with the slightly awkward issue of recovering DRRS state on bdw. But I think since we sprinkle idle/busy calls all over the place this is a problem we can just ignore ;-) -Daniel > > -Vandana > > >> > >> PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); > >> PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index 34ed143..9aa4dcd 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder, > >> > >> intel_dp_get_m_n(crtc, pipe_config); > >> > >> + intel_dp_get_m2_n2(crtc, pipe_config); > >> + > >> if (port == PORT_A) { > >> if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ) > >> pipe_config->port_clock = 162000; > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index d8b540b..1013f70 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv); > >> void hsw_disable_pc8(struct drm_i915_private *dev_priv); > >> void intel_dp_get_m_n(struct intel_crtc *crtc, > >> struct intel_crtc_config *pipe_config); > >> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, > >> + struct intel_crtc_config *pipe_config); > >> int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); > >> void > >> ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, > >> -- > >> 1.9.2 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > >
On May-13-2014 2:28 PM, Daniel Vetter wrote: > On Tue, May 13, 2014 at 01:56:04PM +0530, Vandana Kannan wrote: >> On May-12-2014 3:57 PM, Ville Syrjälä wrote: >>> On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: >>>> Adding relevant read out comparison code, in check_crtc_state, for the new >>>> member of crtc_config, dp_m2_n2, which was introduced to store link_m_n >>>> values for a DP downclock mode (if available). Suggested by Daniel. >>>> >>>> v2: Changed patch title. >>>> Daniel's review comments incorporated. >>>> Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done >>>> only when high RR is not in use (This is because alternate m_n register >>>> programming will be done only when low RR is being used). >>>> >>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> --- >>>> drivers/gpu/drm/i915/intel_ddi.c | 1 + >>>> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- >>>> drivers/gpu/drm/i915/intel_dp.c | 2 + >>>> drivers/gpu/drm/i915/intel_drv.h | 2 + >>>> 4 files changed, 72 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>>> index 0ad4e96..6784f0b 100644 >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>> @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >>>> case TRANS_DDI_MODE_SELECT_DP_MST: >>>> pipe_config->has_dp_encoder = true; >>>> intel_dp_get_m_n(intel_crtc, pipe_config); >>>> + intel_dp_get_m2_n2(intel_crtc, pipe_config); >>>> break; >>>> default: >>>> break; >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 797f01c..2e625eb 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >>>> &pipe_config->dp_m_n); >>>> } >>>> >>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >>>> + struct intel_crtc_config *pipe_config) >>>> +{ >>>> + struct drm_device *dev = crtc->base.dev; >>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>> + enum transcoder transcoder = pipe_config->cpu_transcoder; >>>> + >>>> + if (INTEL_INFO(dev)->gen >= 8) { >>>> + intel_cpu_transcoder_get_m_n(crtc, transcoder, >>>> + &pipe_config->dp_m_n); >>> >>> dm_m2_n2 surely? And why do we even want to do this? >>> >> My miss, will change this. >> For Gen8, there is only one set of MN registers to be programmed for >> high and low RR. Hence this check. >>>> + } else if (INTEL_INFO(dev)->gen > 6) { >>>> + pipe_config->dp_m2_n2.link_m = >>>> + I915_READ(PIPE_LINK_M2(transcoder)); >>>> + pipe_config->dp_m2_n2.link_n = >>>> + I915_READ(PIPE_LINK_N2(transcoder)); >>>> + pipe_config->dp_m2_n2.gmch_m = >>>> + I915_READ(PIPE_DATA_M2(transcoder)) >>>> + & ~TU_SIZE_MASK; >>>> + pipe_config->dp_m2_n2.gmch_n = >>>> + I915_READ(PIPE_DATA_N2(transcoder)); >>>> + pipe_config->dp_m2_n2.tu = >>>> + ((I915_READ(PIPE_DATA_M2(transcoder)) >>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>>> + } >>>> + >>>> +} >>>> >>>> + >>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >>>> struct intel_crtc_config *pipe_config) >>>> { >>>> @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, >>>> pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, >>>> pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, >>>> pipe_config->dp_m_n.tu); >>>> + >>>> + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", >>>> + pipe_config->has_dp_encoder, >>>> + pipe_config->dp_m2_n2.gmch_m, >>>> + pipe_config->dp_m2_n2.gmch_n, >>>> + pipe_config->dp_m2_n2.link_m, >>>> + pipe_config->dp_m2_n2.link_n, >>>> + pipe_config->dp_m2_n2.tu); >>>> + >>>> DRM_DEBUG_KMS("requested mode:\n"); >>>> drm_mode_debug_printmodeline(&pipe_config->requested_mode); >>>> DRM_DEBUG_KMS("adjusted mode:\n"); >>>> @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, >>>> struct intel_crtc_config *current_config, >>>> struct intel_crtc_config *pipe_config) >>>> { >>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>> + struct intel_connector *intel_connector = dev_priv->drrs.connector; >>>> + struct intel_encoder *encoder = (intel_connector != NULL) ? >>>> + intel_attached_encoder(&intel_connector->base) : >>>> + NULL; >>>> + struct intel_dp *intel_dp = (encoder != NULL) ? >>>> + enc_to_intel_dp(&encoder->base) : NULL; >>>> + >>>> #define PIPE_CONF_CHECK_X(name) \ >>>> if (current_config->name != pipe_config->name) { \ >>>> DRM_ERROR("mismatch in " #name " " \ >>>> @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, >>>> PIPE_CONF_CHECK_I(fdi_m_n.tu); >>>> >>>> PIPE_CONF_CHECK_I(has_dp_encoder); >>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >>>> - PIPE_CONF_CHECK_I(dp_m_n.link_m); >>>> - PIPE_CONF_CHECK_I(dp_m_n.link_n); >>>> - PIPE_CONF_CHECK_I(dp_m_n.tu); >>>> + >>>> + >>>> + /* DP M1_N1 registers are used when DRRS is disabled or when high RR >>>> + * is used. Else, DP M2_N2 registers are used. The following check >>>> + * has been added to make sure a mismatch (if any) is displayed only >>>> + * for a real difference and not because of DRRS state. >>>> + */ >>>> + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || >>>> + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && >>>> + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { >>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >>>> + PIPE_CONF_CHECK_I(dp_m_n.link_m); >>>> + PIPE_CONF_CHECK_I(dp_m_n.link_n); >>>> + PIPE_CONF_CHECK_I(dp_m_n.tu); >>>> + } else { >>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); >>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); >>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); >>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); >>>> + PIPE_CONF_CHECK_I(dp_m2_n2.tu); >>>> + } >>> >>> Can't we just check both dp_m_n and dp_m2_n2 always? >>> >> Daniel/Ville, >> >> pipe_config->dp_m2_n2 is populated along with pipe_config->dp_m_n in >> compute_config(). while pipe_config dp_m_n values are programmed into MN >> registers during the mode set, dp_m2_n2 values are programmed only when >> low RR is used. >> Given this, when PIPE_CONF_CHECK is done for dp_m2_n2, it would show a >> mismatch (struct contains values to be programmed but registers still >> have 0) and return. >> The reason these checks were added was to avoid the scenario in which a >> mismatch is highlighted when there is no real failure at all. For >> example, DRRS is enabled on boot but the scenario to trigger low RR has >> not yet occurred, dp_m2_n2 has been computed but M2_N2 registers are not >> programmed in this case. There will be a mismatch in the pipe_config check. >> >> Changes can be made to set M2_N2 registers when dp_m2_n2 is populated >> and toggle the bit (PIPECONF bit) to switch between high and low RR, but >> this becomes invalid for gen8 and above where only 1 set of MN registers >> are available. >> >> Please suggest some other way to handle this. >> May be a quirk like the following? >> >> /* >> * FIXME: BIOS likes to set up a cloned config with lvds+external >> * screen. Since we don't yet re-compute the pipe config when moving >> * just the lvds port away to another pipe the sw tracking won't >> match. >> * >> * Proper atomic modesets with recomputed global state will fix >> this. >> * Until then just don't check gmch state for inherited modes. >> */ >> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { >> >> Or go ahead with checking dp_m_n and dp_m2_n2 always ? >> Please let me know.. > > I think for BDW we should check whether the hw values match _either_ > dp_m_n or dp_m2_n2. But for hsw and earlier we should check both. > > That leaves us with the slightly awkward issue of recovering DRRS state on > bdw. But I think since we sprinkle idle/busy calls all over the place this > is a problem we can just ignore ;-) > -Daniel > Ok, So I will make the following changes and resend.. 1. in compute_config(), for gen < 8, set M2_N2 registers (instead of doing it in intel_dp_set_drrs_state()). 2. in intel_pipe_config_compare(), include your inputs given above.. -Vandana >> >> -Vandana >> >>>> >>>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); >>>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>> index 34ed143..9aa4dcd 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder, >>>> >>>> intel_dp_get_m_n(crtc, pipe_config); >>>> >>>> + intel_dp_get_m2_n2(crtc, pipe_config); >>>> + >>>> if (port == PORT_A) { >>>> if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ) >>>> pipe_config->port_clock = 162000; >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>> index d8b540b..1013f70 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv); >>>> void hsw_disable_pc8(struct drm_i915_private *dev_priv); >>>> void intel_dp_get_m_n(struct intel_crtc *crtc, >>>> struct intel_crtc_config *pipe_config); >>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >>>> + struct intel_crtc_config *pipe_config); >>>> int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); >>>> void >>>> ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, >>>> -- >>>> 1.9.2 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> >> >
On May-13-2014 3:10 PM, Vandana Kannan wrote: > On May-13-2014 2:28 PM, Daniel Vetter wrote: >> On Tue, May 13, 2014 at 01:56:04PM +0530, Vandana Kannan wrote: >>> On May-12-2014 3:57 PM, Ville Syrjälä wrote: >>>> On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: >>>>> Adding relevant read out comparison code, in check_crtc_state, for the new >>>>> member of crtc_config, dp_m2_n2, which was introduced to store link_m_n >>>>> values for a DP downclock mode (if available). Suggested by Daniel. >>>>> >>>>> v2: Changed patch title. >>>>> Daniel's review comments incorporated. >>>>> Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done >>>>> only when high RR is not in use (This is because alternate m_n register >>>>> programming will be done only when low RR is being used). >>>>> >>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_ddi.c | 1 + >>>>> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- >>>>> drivers/gpu/drm/i915/intel_dp.c | 2 + >>>>> drivers/gpu/drm/i915/intel_drv.h | 2 + >>>>> 4 files changed, 72 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>>>> index 0ad4e96..6784f0b 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>>> @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >>>>> case TRANS_DDI_MODE_SELECT_DP_MST: >>>>> pipe_config->has_dp_encoder = true; >>>>> intel_dp_get_m_n(intel_crtc, pipe_config); >>>>> + intel_dp_get_m2_n2(intel_crtc, pipe_config); >>>>> break; >>>>> default: >>>>> break; >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>> index 797f01c..2e625eb 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>> @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >>>>> &pipe_config->dp_m_n); >>>>> } >>>>> >>>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >>>>> + struct intel_crtc_config *pipe_config) >>>>> +{ >>>>> + struct drm_device *dev = crtc->base.dev; >>>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>>> + enum transcoder transcoder = pipe_config->cpu_transcoder; >>>>> + >>>>> + if (INTEL_INFO(dev)->gen >= 8) { >>>>> + intel_cpu_transcoder_get_m_n(crtc, transcoder, >>>>> + &pipe_config->dp_m_n); >>>> >>>> dm_m2_n2 surely? And why do we even want to do this? >>>> >>> My miss, will change this. >>> For Gen8, there is only one set of MN registers to be programmed for >>> high and low RR. Hence this check. >>>>> + } else if (INTEL_INFO(dev)->gen > 6) { >>>>> + pipe_config->dp_m2_n2.link_m = >>>>> + I915_READ(PIPE_LINK_M2(transcoder)); >>>>> + pipe_config->dp_m2_n2.link_n = >>>>> + I915_READ(PIPE_LINK_N2(transcoder)); >>>>> + pipe_config->dp_m2_n2.gmch_m = >>>>> + I915_READ(PIPE_DATA_M2(transcoder)) >>>>> + & ~TU_SIZE_MASK; >>>>> + pipe_config->dp_m2_n2.gmch_n = >>>>> + I915_READ(PIPE_DATA_N2(transcoder)); >>>>> + pipe_config->dp_m2_n2.tu = >>>>> + ((I915_READ(PIPE_DATA_M2(transcoder)) >>>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>>>> + } >>>>> + >>>>> +} >>>>> >>>>> + >>>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >>>>> struct intel_crtc_config *pipe_config) >>>>> { >>>>> @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, >>>>> pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, >>>>> pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, >>>>> pipe_config->dp_m_n.tu); >>>>> + >>>>> + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", >>>>> + pipe_config->has_dp_encoder, >>>>> + pipe_config->dp_m2_n2.gmch_m, >>>>> + pipe_config->dp_m2_n2.gmch_n, >>>>> + pipe_config->dp_m2_n2.link_m, >>>>> + pipe_config->dp_m2_n2.link_n, >>>>> + pipe_config->dp_m2_n2.tu); >>>>> + >>>>> DRM_DEBUG_KMS("requested mode:\n"); >>>>> drm_mode_debug_printmodeline(&pipe_config->requested_mode); >>>>> DRM_DEBUG_KMS("adjusted mode:\n"); >>>>> @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, >>>>> struct intel_crtc_config *current_config, >>>>> struct intel_crtc_config *pipe_config) >>>>> { >>>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>>> + struct intel_connector *intel_connector = dev_priv->drrs.connector; >>>>> + struct intel_encoder *encoder = (intel_connector != NULL) ? >>>>> + intel_attached_encoder(&intel_connector->base) : >>>>> + NULL; >>>>> + struct intel_dp *intel_dp = (encoder != NULL) ? >>>>> + enc_to_intel_dp(&encoder->base) : NULL; >>>>> + >>>>> #define PIPE_CONF_CHECK_X(name) \ >>>>> if (current_config->name != pipe_config->name) { \ >>>>> DRM_ERROR("mismatch in " #name " " \ >>>>> @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, >>>>> PIPE_CONF_CHECK_I(fdi_m_n.tu); >>>>> >>>>> PIPE_CONF_CHECK_I(has_dp_encoder); >>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_m); >>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_n); >>>>> - PIPE_CONF_CHECK_I(dp_m_n.tu); >>>>> + >>>>> + >>>>> + /* DP M1_N1 registers are used when DRRS is disabled or when high RR >>>>> + * is used. Else, DP M2_N2 registers are used. The following check >>>>> + * has been added to make sure a mismatch (if any) is displayed only >>>>> + * for a real difference and not because of DRRS state. >>>>> + */ >>>>> + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || >>>>> + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && >>>>> + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { >>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_m); >>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_n); >>>>> + PIPE_CONF_CHECK_I(dp_m_n.tu); >>>>> + } else { >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.tu); >>>>> + } >>>> >>>> Can't we just check both dp_m_n and dp_m2_n2 always? >>>> >>> Daniel/Ville, >>> >>> pipe_config->dp_m2_n2 is populated along with pipe_config->dp_m_n in >>> compute_config(). while pipe_config dp_m_n values are programmed into MN >>> registers during the mode set, dp_m2_n2 values are programmed only when >>> low RR is used. >>> Given this, when PIPE_CONF_CHECK is done for dp_m2_n2, it would show a >>> mismatch (struct contains values to be programmed but registers still >>> have 0) and return. >>> The reason these checks were added was to avoid the scenario in which a >>> mismatch is highlighted when there is no real failure at all. For >>> example, DRRS is enabled on boot but the scenario to trigger low RR has >>> not yet occurred, dp_m2_n2 has been computed but M2_N2 registers are not >>> programmed in this case. There will be a mismatch in the pipe_config check. >>> >>> Changes can be made to set M2_N2 registers when dp_m2_n2 is populated >>> and toggle the bit (PIPECONF bit) to switch between high and low RR, but >>> this becomes invalid for gen8 and above where only 1 set of MN registers >>> are available. >>> >>> Please suggest some other way to handle this. >>> May be a quirk like the following? >>> >>> /* >>> * FIXME: BIOS likes to set up a cloned config with lvds+external >>> * screen. Since we don't yet re-compute the pipe config when moving >>> * just the lvds port away to another pipe the sw tracking won't >>> match. >>> * >>> * Proper atomic modesets with recomputed global state will fix >>> this. >>> * Until then just don't check gmch state for inherited modes. >>> */ >>> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { >>> >>> Or go ahead with checking dp_m_n and dp_m2_n2 always ? >>> Please let me know.. >> >> I think for BDW we should check whether the hw values match _either_ >> dp_m_n or dp_m2_n2. But for hsw and earlier we should check both. >> >> That leaves us with the slightly awkward issue of recovering DRRS state on >> bdw. But I think since we sprinkle idle/busy calls all over the place this >> is a problem we can just ignore ;-) >> -Daniel >> > > Ok, So I will make the following changes and resend.. > 1. in compute_config(), for gen < 8, set M2_N2 registers (instead of > doing it in intel_dp_set_drrs_state()). > 2. in intel_pipe_config_compare(), include your inputs given above.. > -Vandana hi Daniel, I see a few issues with this approach of setting M2 N2 registers ahead and just toggling the bit to switch between refresh rates (for gen 7 and below).. writing into M2_N2 registers in compute_config() would fail when system resumes from sleep. to overcome this, setting M2 N2 can be done in crtc_mode_set functions (similar to set_m_n). given this setting, suppose user space sets a refresh rate based on usage scenario (future implementation maybe), which would come through mode_set calls, M2 N2 registers can be appropriately programmed with user space requested RR. now, (if kernel idleness detection for DRRS is enabled) if the screen image is idle, then additional changes would be required to program M2 N2 registers with the correct low RR values. For example, set_m2_n2 would have to be called just before set_drrs for lowest RR is called. in summary, the alternative approach to avoid DRRS related checks in pipe_config_compare() is doable but creates other inconveniences w.r.t the overall design and interaction with user space. please let me know your opinion on this.. -Vandana >>> >>> -Vandana >>> >>>>> >>>>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); >>>>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); >>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>>> index 34ed143..9aa4dcd 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>>> @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder, >>>>> >>>>> intel_dp_get_m_n(crtc, pipe_config); >>>>> >>>>> + intel_dp_get_m2_n2(crtc, pipe_config); >>>>> + >>>>> if (port == PORT_A) { >>>>> if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ) >>>>> pipe_config->port_clock = 162000; >>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>>> index d8b540b..1013f70 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>>> @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv); >>>>> void hsw_disable_pc8(struct drm_i915_private *dev_priv); >>>>> void intel_dp_get_m_n(struct intel_crtc *crtc, >>>>> struct intel_crtc_config *pipe_config); >>>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >>>>> + struct intel_crtc_config *pipe_config); >>>>> int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); >>>>> void >>>>> ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, >>>>> -- >>>>> 1.9.2 >>>>> >>>>> _______________________________________________ >>>>> Intel-gfx mailing list >>>>> Intel-gfx@lists.freedesktop.org >>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>> >>> >> >
On Thu, May 15, 2014 at 02:48:02PM +0530, Vandana Kannan wrote: > On May-13-2014 3:10 PM, Vandana Kannan wrote: > > On May-13-2014 2:28 PM, Daniel Vetter wrote: > >> On Tue, May 13, 2014 at 01:56:04PM +0530, Vandana Kannan wrote: > >>> On May-12-2014 3:57 PM, Ville Syrjälä wrote: > >>>> On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: > >>>>> Adding relevant read out comparison code, in check_crtc_state, for the new > >>>>> member of crtc_config, dp_m2_n2, which was introduced to store link_m_n > >>>>> values for a DP downclock mode (if available). Suggested by Daniel. > >>>>> > >>>>> v2: Changed patch title. > >>>>> Daniel's review comments incorporated. > >>>>> Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done > >>>>> only when high RR is not in use (This is because alternate m_n register > >>>>> programming will be done only when low RR is being used). > >>>>> > >>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>>> --- > >>>>> drivers/gpu/drm/i915/intel_ddi.c | 1 + > >>>>> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- > >>>>> drivers/gpu/drm/i915/intel_dp.c | 2 + > >>>>> drivers/gpu/drm/i915/intel_drv.h | 2 + > >>>>> 4 files changed, 72 insertions(+), 5 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >>>>> index 0ad4e96..6784f0b 100644 > >>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c > >>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >>>>> @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, > >>>>> case TRANS_DDI_MODE_SELECT_DP_MST: > >>>>> pipe_config->has_dp_encoder = true; > >>>>> intel_dp_get_m_n(intel_crtc, pipe_config); > >>>>> + intel_dp_get_m2_n2(intel_crtc, pipe_config); > >>>>> break; > >>>>> default: > >>>>> break; > >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>>>> index 797f01c..2e625eb 100644 > >>>>> --- a/drivers/gpu/drm/i915/intel_display.c > >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>>>> @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > >>>>> &pipe_config->dp_m_n); > >>>>> } > >>>>> > >>>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, > >>>>> + struct intel_crtc_config *pipe_config) > >>>>> +{ > >>>>> + struct drm_device *dev = crtc->base.dev; > >>>>> + struct drm_i915_private *dev_priv = dev->dev_private; > >>>>> + enum transcoder transcoder = pipe_config->cpu_transcoder; > >>>>> + > >>>>> + if (INTEL_INFO(dev)->gen >= 8) { > >>>>> + intel_cpu_transcoder_get_m_n(crtc, transcoder, > >>>>> + &pipe_config->dp_m_n); > >>>> > >>>> dm_m2_n2 surely? And why do we even want to do this? > >>>> > >>> My miss, will change this. > >>> For Gen8, there is only one set of MN registers to be programmed for > >>> high and low RR. Hence this check. > >>>>> + } else if (INTEL_INFO(dev)->gen > 6) { > >>>>> + pipe_config->dp_m2_n2.link_m = > >>>>> + I915_READ(PIPE_LINK_M2(transcoder)); > >>>>> + pipe_config->dp_m2_n2.link_n = > >>>>> + I915_READ(PIPE_LINK_N2(transcoder)); > >>>>> + pipe_config->dp_m2_n2.gmch_m = > >>>>> + I915_READ(PIPE_DATA_M2(transcoder)) > >>>>> + & ~TU_SIZE_MASK; > >>>>> + pipe_config->dp_m2_n2.gmch_n = > >>>>> + I915_READ(PIPE_DATA_N2(transcoder)); > >>>>> + pipe_config->dp_m2_n2.tu = > >>>>> + ((I915_READ(PIPE_DATA_M2(transcoder)) > >>>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > >>>>> + } > >>>>> + > >>>>> +} > >>>>> > >>>>> + > >>>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, > >>>>> struct intel_crtc_config *pipe_config) > >>>>> { > >>>>> @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, > >>>>> pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, > >>>>> pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, > >>>>> pipe_config->dp_m_n.tu); > >>>>> + > >>>>> + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", > >>>>> + pipe_config->has_dp_encoder, > >>>>> + pipe_config->dp_m2_n2.gmch_m, > >>>>> + pipe_config->dp_m2_n2.gmch_n, > >>>>> + pipe_config->dp_m2_n2.link_m, > >>>>> + pipe_config->dp_m2_n2.link_n, > >>>>> + pipe_config->dp_m2_n2.tu); > >>>>> + > >>>>> DRM_DEBUG_KMS("requested mode:\n"); > >>>>> drm_mode_debug_printmodeline(&pipe_config->requested_mode); > >>>>> DRM_DEBUG_KMS("adjusted mode:\n"); > >>>>> @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, > >>>>> struct intel_crtc_config *current_config, > >>>>> struct intel_crtc_config *pipe_config) > >>>>> { > >>>>> + struct drm_i915_private *dev_priv = dev->dev_private; > >>>>> + struct intel_connector *intel_connector = dev_priv->drrs.connector; > >>>>> + struct intel_encoder *encoder = (intel_connector != NULL) ? > >>>>> + intel_attached_encoder(&intel_connector->base) : > >>>>> + NULL; > >>>>> + struct intel_dp *intel_dp = (encoder != NULL) ? > >>>>> + enc_to_intel_dp(&encoder->base) : NULL; > >>>>> + > >>>>> #define PIPE_CONF_CHECK_X(name) \ > >>>>> if (current_config->name != pipe_config->name) { \ > >>>>> DRM_ERROR("mismatch in " #name " " \ > >>>>> @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, > >>>>> PIPE_CONF_CHECK_I(fdi_m_n.tu); > >>>>> > >>>>> PIPE_CONF_CHECK_I(has_dp_encoder); > >>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); > >>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); > >>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_m); > >>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_n); > >>>>> - PIPE_CONF_CHECK_I(dp_m_n.tu); > >>>>> + > >>>>> + > >>>>> + /* DP M1_N1 registers are used when DRRS is disabled or when high RR > >>>>> + * is used. Else, DP M2_N2 registers are used. The following check > >>>>> + * has been added to make sure a mismatch (if any) is displayed only > >>>>> + * for a real difference and not because of DRRS state. > >>>>> + */ > >>>>> + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || > >>>>> + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && > >>>>> + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { > >>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); > >>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); > >>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_m); > >>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_n); > >>>>> + PIPE_CONF_CHECK_I(dp_m_n.tu); > >>>>> + } else { > >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); > >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); > >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); > >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); > >>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.tu); > >>>>> + } > >>>> > >>>> Can't we just check both dp_m_n and dp_m2_n2 always? > >>>> > >>> Daniel/Ville, > >>> > >>> pipe_config->dp_m2_n2 is populated along with pipe_config->dp_m_n in > >>> compute_config(). while pipe_config dp_m_n values are programmed into MN > >>> registers during the mode set, dp_m2_n2 values are programmed only when > >>> low RR is used. > >>> Given this, when PIPE_CONF_CHECK is done for dp_m2_n2, it would show a > >>> mismatch (struct contains values to be programmed but registers still > >>> have 0) and return. > >>> The reason these checks were added was to avoid the scenario in which a > >>> mismatch is highlighted when there is no real failure at all. For > >>> example, DRRS is enabled on boot but the scenario to trigger low RR has > >>> not yet occurred, dp_m2_n2 has been computed but M2_N2 registers are not > >>> programmed in this case. There will be a mismatch in the pipe_config check. > >>> > >>> Changes can be made to set M2_N2 registers when dp_m2_n2 is populated > >>> and toggle the bit (PIPECONF bit) to switch between high and low RR, but > >>> this becomes invalid for gen8 and above where only 1 set of MN registers > >>> are available. > >>> > >>> Please suggest some other way to handle this. > >>> May be a quirk like the following? > >>> > >>> /* > >>> * FIXME: BIOS likes to set up a cloned config with lvds+external > >>> * screen. Since we don't yet re-compute the pipe config when moving > >>> * just the lvds port away to another pipe the sw tracking won't > >>> match. > >>> * > >>> * Proper atomic modesets with recomputed global state will fix > >>> this. > >>> * Until then just don't check gmch state for inherited modes. > >>> */ > >>> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { > >>> > >>> Or go ahead with checking dp_m_n and dp_m2_n2 always ? > >>> Please let me know.. > >> > >> I think for BDW we should check whether the hw values match _either_ > >> dp_m_n or dp_m2_n2. But for hsw and earlier we should check both. > >> > >> That leaves us with the slightly awkward issue of recovering DRRS state on > >> bdw. But I think since we sprinkle idle/busy calls all over the place this > >> is a problem we can just ignore ;-) > >> -Daniel > >> > > > > Ok, So I will make the following changes and resend.. > > 1. in compute_config(), for gen < 8, set M2_N2 registers (instead of > > doing it in intel_dp_set_drrs_state()). > > 2. in intel_pipe_config_compare(), include your inputs given above.. > > -Vandana > > hi Daniel, > I see a few issues with this approach of setting M2 N2 registers ahead > and just toggling the bit to switch between refresh rates (for gen 7 and > below).. > > writing into M2_N2 registers in compute_config() would fail when system > resumes from sleep. to overcome this, setting M2 N2 can be done in > crtc_mode_set functions (similar to set_m_n). Don't modify anything but the new pipe config in compute_config. As the name suggests it's just supposed to compute the new state, not apply it.
On May-15-2014 3:01 PM, Ville Syrjälä wrote: > On Thu, May 15, 2014 at 02:48:02PM +0530, Vandana Kannan wrote: >> On May-13-2014 3:10 PM, Vandana Kannan wrote: >>> On May-13-2014 2:28 PM, Daniel Vetter wrote: >>>> On Tue, May 13, 2014 at 01:56:04PM +0530, Vandana Kannan wrote: >>>>> On May-12-2014 3:57 PM, Ville Syrjälä wrote: >>>>>> On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: >>>>>>> Adding relevant read out comparison code, in check_crtc_state, for the new >>>>>>> member of crtc_config, dp_m2_n2, which was introduced to store link_m_n >>>>>>> values for a DP downclock mode (if available). Suggested by Daniel. >>>>>>> >>>>>>> v2: Changed patch title. >>>>>>> Daniel's review comments incorporated. >>>>>>> Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done >>>>>>> only when high RR is not in use (This is because alternate m_n register >>>>>>> programming will be done only when low RR is being used). >>>>>>> >>>>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/intel_ddi.c | 1 + >>>>>>> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- >>>>>>> drivers/gpu/drm/i915/intel_dp.c | 2 + >>>>>>> drivers/gpu/drm/i915/intel_drv.h | 2 + >>>>>>> 4 files changed, 72 insertions(+), 5 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>>>>>> index 0ad4e96..6784f0b 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>>>>> @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >>>>>>> case TRANS_DDI_MODE_SELECT_DP_MST: >>>>>>> pipe_config->has_dp_encoder = true; >>>>>>> intel_dp_get_m_n(intel_crtc, pipe_config); >>>>>>> + intel_dp_get_m2_n2(intel_crtc, pipe_config); >>>>>>> break; >>>>>>> default: >>>>>>> break; >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>>>> index 797f01c..2e625eb 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>>> @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >>>>>>> &pipe_config->dp_m_n); >>>>>>> } >>>>>>> >>>>>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >>>>>>> + struct intel_crtc_config *pipe_config) >>>>>>> +{ >>>>>>> + struct drm_device *dev = crtc->base.dev; >>>>>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>>>>> + enum transcoder transcoder = pipe_config->cpu_transcoder; >>>>>>> + >>>>>>> + if (INTEL_INFO(dev)->gen >= 8) { >>>>>>> + intel_cpu_transcoder_get_m_n(crtc, transcoder, >>>>>>> + &pipe_config->dp_m_n); >>>>>> >>>>>> dm_m2_n2 surely? And why do we even want to do this? >>>>>> >>>>> My miss, will change this. >>>>> For Gen8, there is only one set of MN registers to be programmed for >>>>> high and low RR. Hence this check. >>>>>>> + } else if (INTEL_INFO(dev)->gen > 6) { >>>>>>> + pipe_config->dp_m2_n2.link_m = >>>>>>> + I915_READ(PIPE_LINK_M2(transcoder)); >>>>>>> + pipe_config->dp_m2_n2.link_n = >>>>>>> + I915_READ(PIPE_LINK_N2(transcoder)); >>>>>>> + pipe_config->dp_m2_n2.gmch_m = >>>>>>> + I915_READ(PIPE_DATA_M2(transcoder)) >>>>>>> + & ~TU_SIZE_MASK; >>>>>>> + pipe_config->dp_m2_n2.gmch_n = >>>>>>> + I915_READ(PIPE_DATA_N2(transcoder)); >>>>>>> + pipe_config->dp_m2_n2.tu = >>>>>>> + ((I915_READ(PIPE_DATA_M2(transcoder)) >>>>>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>>>>>> + } >>>>>>> + >>>>>>> +} >>>>>>> >>>>>>> + >>>>>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >>>>>>> struct intel_crtc_config *pipe_config) >>>>>>> { >>>>>>> @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, >>>>>>> pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, >>>>>>> pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, >>>>>>> pipe_config->dp_m_n.tu); >>>>>>> + >>>>>>> + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", >>>>>>> + pipe_config->has_dp_encoder, >>>>>>> + pipe_config->dp_m2_n2.gmch_m, >>>>>>> + pipe_config->dp_m2_n2.gmch_n, >>>>>>> + pipe_config->dp_m2_n2.link_m, >>>>>>> + pipe_config->dp_m2_n2.link_n, >>>>>>> + pipe_config->dp_m2_n2.tu); >>>>>>> + >>>>>>> DRM_DEBUG_KMS("requested mode:\n"); >>>>>>> drm_mode_debug_printmodeline(&pipe_config->requested_mode); >>>>>>> DRM_DEBUG_KMS("adjusted mode:\n"); >>>>>>> @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, >>>>>>> struct intel_crtc_config *current_config, >>>>>>> struct intel_crtc_config *pipe_config) >>>>>>> { >>>>>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>>>>> + struct intel_connector *intel_connector = dev_priv->drrs.connector; >>>>>>> + struct intel_encoder *encoder = (intel_connector != NULL) ? >>>>>>> + intel_attached_encoder(&intel_connector->base) : >>>>>>> + NULL; >>>>>>> + struct intel_dp *intel_dp = (encoder != NULL) ? >>>>>>> + enc_to_intel_dp(&encoder->base) : NULL; >>>>>>> + >>>>>>> #define PIPE_CONF_CHECK_X(name) \ >>>>>>> if (current_config->name != pipe_config->name) { \ >>>>>>> DRM_ERROR("mismatch in " #name " " \ >>>>>>> @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, >>>>>>> PIPE_CONF_CHECK_I(fdi_m_n.tu); >>>>>>> >>>>>>> PIPE_CONF_CHECK_I(has_dp_encoder); >>>>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >>>>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >>>>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_m); >>>>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_n); >>>>>>> - PIPE_CONF_CHECK_I(dp_m_n.tu); >>>>>>> + >>>>>>> + >>>>>>> + /* DP M1_N1 registers are used when DRRS is disabled or when high RR >>>>>>> + * is used. Else, DP M2_N2 registers are used. The following check >>>>>>> + * has been added to make sure a mismatch (if any) is displayed only >>>>>>> + * for a real difference and not because of DRRS state. >>>>>>> + */ >>>>>>> + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || >>>>>>> + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && >>>>>>> + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { >>>>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >>>>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >>>>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_m); >>>>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_n); >>>>>>> + PIPE_CONF_CHECK_I(dp_m_n.tu); >>>>>>> + } else { >>>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); >>>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); >>>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); >>>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); >>>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.tu); >>>>>>> + } >>>>>> >>>>>> Can't we just check both dp_m_n and dp_m2_n2 always? >>>>>> >>>>> Daniel/Ville, >>>>> >>>>> pipe_config->dp_m2_n2 is populated along with pipe_config->dp_m_n in >>>>> compute_config(). while pipe_config dp_m_n values are programmed into MN >>>>> registers during the mode set, dp_m2_n2 values are programmed only when >>>>> low RR is used. >>>>> Given this, when PIPE_CONF_CHECK is done for dp_m2_n2, it would show a >>>>> mismatch (struct contains values to be programmed but registers still >>>>> have 0) and return. >>>>> The reason these checks were added was to avoid the scenario in which a >>>>> mismatch is highlighted when there is no real failure at all. For >>>>> example, DRRS is enabled on boot but the scenario to trigger low RR has >>>>> not yet occurred, dp_m2_n2 has been computed but M2_N2 registers are not >>>>> programmed in this case. There will be a mismatch in the pipe_config check. >>>>> >>>>> Changes can be made to set M2_N2 registers when dp_m2_n2 is populated >>>>> and toggle the bit (PIPECONF bit) to switch between high and low RR, but >>>>> this becomes invalid for gen8 and above where only 1 set of MN registers >>>>> are available. >>>>> >>>>> Please suggest some other way to handle this. >>>>> May be a quirk like the following? >>>>> >>>>> /* >>>>> * FIXME: BIOS likes to set up a cloned config with lvds+external >>>>> * screen. Since we don't yet re-compute the pipe config when moving >>>>> * just the lvds port away to another pipe the sw tracking won't >>>>> match. >>>>> * >>>>> * Proper atomic modesets with recomputed global state will fix >>>>> this. >>>>> * Until then just don't check gmch state for inherited modes. >>>>> */ >>>>> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { >>>>> >>>>> Or go ahead with checking dp_m_n and dp_m2_n2 always ? >>>>> Please let me know.. >>>> >>>> I think for BDW we should check whether the hw values match _either_ >>>> dp_m_n or dp_m2_n2. But for hsw and earlier we should check both. >>>> >>>> That leaves us with the slightly awkward issue of recovering DRRS state on >>>> bdw. But I think since we sprinkle idle/busy calls all over the place this >>>> is a problem we can just ignore ;-) >>>> -Daniel >>>> >>> >>> Ok, So I will make the following changes and resend.. >>> 1. in compute_config(), for gen < 8, set M2_N2 registers (instead of >>> doing it in intel_dp_set_drrs_state()). >>> 2. in intel_pipe_config_compare(), include your inputs given above.. >>> -Vandana >> >> hi Daniel, >> I see a few issues with this approach of setting M2 N2 registers ahead >> and just toggling the bit to switch between refresh rates (for gen 7 and >> below).. >> >> writing into M2_N2 registers in compute_config() would fail when system >> resumes from sleep. to overcome this, setting M2 N2 can be done in >> crtc_mode_set functions (similar to set_m_n). > > Don't modify anything but the new pipe config in compute_config. As > the name suggests it's just supposed to compute the new state, not > apply it. > Yes, I'm setting the registers in crtc_mode_set..
On May-15-2014 2:48 PM, Vandana Kannan wrote: > On May-13-2014 3:10 PM, Vandana Kannan wrote: >> On May-13-2014 2:28 PM, Daniel Vetter wrote: >>> On Tue, May 13, 2014 at 01:56:04PM +0530, Vandana Kannan wrote: >>>> On May-12-2014 3:57 PM, Ville Syrjälä wrote: >>>>> On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote: >>>>>> Adding relevant read out comparison code, in check_crtc_state, for the new >>>>>> member of crtc_config, dp_m2_n2, which was introduced to store link_m_n >>>>>> values for a DP downclock mode (if available). Suggested by Daniel. >>>>>> >>>>>> v2: Changed patch title. >>>>>> Daniel's review comments incorporated. >>>>>> Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done >>>>>> only when high RR is not in use (This is because alternate m_n register >>>>>> programming will be done only when low RR is being used). >>>>>> >>>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_ddi.c | 1 + >>>>>> drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- >>>>>> drivers/gpu/drm/i915/intel_dp.c | 2 + >>>>>> drivers/gpu/drm/i915/intel_drv.h | 2 + >>>>>> 4 files changed, 72 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>>>>> index 0ad4e96..6784f0b 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>>>>> @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, >>>>>> case TRANS_DDI_MODE_SELECT_DP_MST: >>>>>> pipe_config->has_dp_encoder = true; >>>>>> intel_dp_get_m_n(intel_crtc, pipe_config); >>>>>> + intel_dp_get_m2_n2(intel_crtc, pipe_config); >>>>>> break; >>>>>> default: >>>>>> break; >>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>>> index 797f01c..2e625eb 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>> @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >>>>>> &pipe_config->dp_m_n); >>>>>> } >>>>>> >>>>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >>>>>> + struct intel_crtc_config *pipe_config) >>>>>> +{ >>>>>> + struct drm_device *dev = crtc->base.dev; >>>>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>>>> + enum transcoder transcoder = pipe_config->cpu_transcoder; >>>>>> + >>>>>> + if (INTEL_INFO(dev)->gen >= 8) { >>>>>> + intel_cpu_transcoder_get_m_n(crtc, transcoder, >>>>>> + &pipe_config->dp_m_n); >>>>> >>>>> dm_m2_n2 surely? And why do we even want to do this? >>>>> >>>> My miss, will change this. >>>> For Gen8, there is only one set of MN registers to be programmed for >>>> high and low RR. Hence this check. >>>>>> + } else if (INTEL_INFO(dev)->gen > 6) { >>>>>> + pipe_config->dp_m2_n2.link_m = >>>>>> + I915_READ(PIPE_LINK_M2(transcoder)); >>>>>> + pipe_config->dp_m2_n2.link_n = >>>>>> + I915_READ(PIPE_LINK_N2(transcoder)); >>>>>> + pipe_config->dp_m2_n2.gmch_m = >>>>>> + I915_READ(PIPE_DATA_M2(transcoder)) >>>>>> + & ~TU_SIZE_MASK; >>>>>> + pipe_config->dp_m2_n2.gmch_n = >>>>>> + I915_READ(PIPE_DATA_N2(transcoder)); >>>>>> + pipe_config->dp_m2_n2.tu = >>>>>> + ((I915_READ(PIPE_DATA_M2(transcoder)) >>>>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>>>>> + } >>>>>> + >>>>>> +} >>>>>> >>>>>> + >>>>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >>>>>> struct intel_crtc_config *pipe_config) >>>>>> { >>>>>> @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, >>>>>> pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, >>>>>> pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, >>>>>> pipe_config->dp_m_n.tu); >>>>>> + >>>>>> + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", >>>>>> + pipe_config->has_dp_encoder, >>>>>> + pipe_config->dp_m2_n2.gmch_m, >>>>>> + pipe_config->dp_m2_n2.gmch_n, >>>>>> + pipe_config->dp_m2_n2.link_m, >>>>>> + pipe_config->dp_m2_n2.link_n, >>>>>> + pipe_config->dp_m2_n2.tu); >>>>>> + >>>>>> DRM_DEBUG_KMS("requested mode:\n"); >>>>>> drm_mode_debug_printmodeline(&pipe_config->requested_mode); >>>>>> DRM_DEBUG_KMS("adjusted mode:\n"); >>>>>> @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, >>>>>> struct intel_crtc_config *current_config, >>>>>> struct intel_crtc_config *pipe_config) >>>>>> { >>>>>> + struct drm_i915_private *dev_priv = dev->dev_private; >>>>>> + struct intel_connector *intel_connector = dev_priv->drrs.connector; >>>>>> + struct intel_encoder *encoder = (intel_connector != NULL) ? >>>>>> + intel_attached_encoder(&intel_connector->base) : >>>>>> + NULL; >>>>>> + struct intel_dp *intel_dp = (encoder != NULL) ? >>>>>> + enc_to_intel_dp(&encoder->base) : NULL; >>>>>> + >>>>>> #define PIPE_CONF_CHECK_X(name) \ >>>>>> if (current_config->name != pipe_config->name) { \ >>>>>> DRM_ERROR("mismatch in " #name " " \ >>>>>> @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, >>>>>> PIPE_CONF_CHECK_I(fdi_m_n.tu); >>>>>> >>>>>> PIPE_CONF_CHECK_I(has_dp_encoder); >>>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >>>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >>>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_m); >>>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_n); >>>>>> - PIPE_CONF_CHECK_I(dp_m_n.tu); >>>>>> + >>>>>> + >>>>>> + /* DP M1_N1 registers are used when DRRS is disabled or when high RR >>>>>> + * is used. Else, DP M2_N2 registers are used. The following check >>>>>> + * has been added to make sure a mismatch (if any) is displayed only >>>>>> + * for a real difference and not because of DRRS state. >>>>>> + */ >>>>>> + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || >>>>>> + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && >>>>>> + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { >>>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); >>>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); >>>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_m); >>>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_n); >>>>>> + PIPE_CONF_CHECK_I(dp_m_n.tu); >>>>>> + } else { >>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); >>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); >>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); >>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); >>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.tu); >>>>>> + } >>>>> >>>>> Can't we just check both dp_m_n and dp_m2_n2 always? >>>>> >>>> Daniel/Ville, >>>> >>>> pipe_config->dp_m2_n2 is populated along with pipe_config->dp_m_n in >>>> compute_config(). while pipe_config dp_m_n values are programmed into MN >>>> registers during the mode set, dp_m2_n2 values are programmed only when >>>> low RR is used. >>>> Given this, when PIPE_CONF_CHECK is done for dp_m2_n2, it would show a >>>> mismatch (struct contains values to be programmed but registers still >>>> have 0) and return. >>>> The reason these checks were added was to avoid the scenario in which a >>>> mismatch is highlighted when there is no real failure at all. For >>>> example, DRRS is enabled on boot but the scenario to trigger low RR has >>>> not yet occurred, dp_m2_n2 has been computed but M2_N2 registers are not >>>> programmed in this case. There will be a mismatch in the pipe_config check. >>>> >>>> Changes can be made to set M2_N2 registers when dp_m2_n2 is populated >>>> and toggle the bit (PIPECONF bit) to switch between high and low RR, but >>>> this becomes invalid for gen8 and above where only 1 set of MN registers >>>> are available. >>>> >>>> Please suggest some other way to handle this. >>>> May be a quirk like the following? >>>> >>>> /* >>>> * FIXME: BIOS likes to set up a cloned config with lvds+external >>>> * screen. Since we don't yet re-compute the pipe config when moving >>>> * just the lvds port away to another pipe the sw tracking won't >>>> match. >>>> * >>>> * Proper atomic modesets with recomputed global state will fix >>>> this. >>>> * Until then just don't check gmch state for inherited modes. >>>> */ >>>> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { >>>> >>>> Or go ahead with checking dp_m_n and dp_m2_n2 always ? >>>> Please let me know.. >>> >>> I think for BDW we should check whether the hw values match _either_ >>> dp_m_n or dp_m2_n2. But for hsw and earlier we should check both. >>> >>> That leaves us with the slightly awkward issue of recovering DRRS state on >>> bdw. But I think since we sprinkle idle/busy calls all over the place this >>> is a problem we can just ignore ;-) >>> -Daniel >>> >> >> Ok, So I will make the following changes and resend.. >> 1. in compute_config(), for gen < 8, set M2_N2 registers (instead of >> doing it in intel_dp_set_drrs_state()). >> 2. in intel_pipe_config_compare(), include your inputs given above.. >> -Vandana > > hi Daniel, > I see a few issues with this approach of setting M2 N2 registers ahead > and just toggling the bit to switch between refresh rates (for gen 7 and > below).. > > writing into M2_N2 registers in compute_config() would fail when system > resumes from sleep. to overcome this, setting M2 N2 can be done in > crtc_mode_set functions (similar to set_m_n). > > given this setting, suppose user space sets a refresh rate based on > usage scenario (future implementation maybe), which would come through > mode_set calls, M2 N2 registers can be appropriately programmed with > user space requested RR. now, (if kernel idleness detection for DRRS is > enabled) if the screen image is idle, then additional changes would be > required to program M2 N2 registers with the correct low RR values. For > example, set_m2_n2 would have to be called just before set_drrs for > lowest RR is called. > > in summary, the alternative approach to avoid DRRS related checks in > pipe_config_compare() is doable but creates other inconveniences w.r.t > the overall design and interaction with user space. > > please let me know your opinion on this.. > > -Vandana > Hi Daniel, Please let me know your inputs on this.. Given that making changes to avoid DRRS related checks in pipe_config_compare affects the overall design and future implementations related to RR switching, I think that using a quirk for downclock_mode to compare dp_m_n and dp_m2_n2 based on the RR in use, if (!low RR) compare dp_m_n else compare dp_m2_n2 would avoid unrelated DRRS checks and make sure extension of DRRS implementation would be possible. Only thing would be that both dp_m_n and dp_m2_n2 cant be compared always.. -Vandana >>>> >>>> -Vandana >>>> >>>>>> >>>>>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); >>>>>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); >>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>>>>> index 34ed143..9aa4dcd 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>>>> @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder, >>>>>> >>>>>> intel_dp_get_m_n(crtc, pipe_config); >>>>>> >>>>>> + intel_dp_get_m2_n2(crtc, pipe_config); >>>>>> + >>>>>> if (port == PORT_A) { >>>>>> if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ) >>>>>> pipe_config->port_clock = 162000; >>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>>>>> index d8b540b..1013f70 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>>>> @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv); >>>>>> void hsw_disable_pc8(struct drm_i915_private *dev_priv); >>>>>> void intel_dp_get_m_n(struct intel_crtc *crtc, >>>>>> struct intel_crtc_config *pipe_config); >>>>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc, >>>>>> + struct intel_crtc_config *pipe_config); >>>>>> int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); >>>>>> void >>>>>> ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config, >>>>>> -- >>>>>> 1.9.2 >>>>>> >>>>>> _______________________________________________ >>>>>> Intel-gfx mailing list >>>>>> Intel-gfx@lists.freedesktop.org >>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>>> >>>> >>> >> > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, May 19, 2014 at 05:00:14PM +0530, Vandana Kannan wrote: > Please let me know your inputs on this.. > Given that making changes to avoid DRRS related checks in > pipe_config_compare affects the overall design and future > implementations related to RR switching, I think that using a quirk for > downclock_mode to compare dp_m_n and dp_m2_n2 based on the RR in use, > > if (!low RR) > compare dp_m_n > else > compare dp_m2_n2 > > would avoid unrelated DRRS checks and make sure extension of DRRS > implementation would be possible. > Only thing would be that both dp_m_n and dp_m2_n2 cant be compared always.. Well my idea was for bdw we'd check that hw.dp_m_n == sw.dp_m_n || hw.dp_m_n == sw.dp_m2_n2 without any checks using software state to figure out whether we're in low RR mode or not. That way the pipe config checker only depends upon the 2 pipe configs, which is a nice proper for it to have. -Daniel
On Mon, May 19, 2014 at 01:45:37PM +0200, Daniel Vetter wrote: > On Mon, May 19, 2014 at 05:00:14PM +0530, Vandana Kannan wrote: > > Please let me know your inputs on this.. > > Given that making changes to avoid DRRS related checks in > > pipe_config_compare affects the overall design and future > > implementations related to RR switching, I think that using a quirk for > > downclock_mode to compare dp_m_n and dp_m2_n2 based on the RR in use, > > > > if (!low RR) > > compare dp_m_n > > else > > compare dp_m2_n2 > > > > would avoid unrelated DRRS checks and make sure extension of DRRS > > implementation would be possible. > > Only thing would be that both dp_m_n and dp_m2_n2 cant be compared always.. > > Well my idea was for bdw we'd check that > > hw.dp_m_n == sw.dp_m_n || hw.dp_m_n == sw.dp_m2_n2 > > without any checks using software state to figure out whether we're in low > RR mode or not. That way the pipe config checker only depends upon the 2 > pipe configs, which is a nice proper for it to have. Also we might need to do fbc/psr frobbing after flips and those again would be easier from process contexts. At least I expect less fuzz if we can wrap up fbc/psr state in a mutex instead of careful spinlock/irqsave spinlock trickery. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 0ad4e96..6784f0b 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1587,6 +1587,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder, case TRANS_DDI_MODE_SELECT_DP_MST: pipe_config->has_dp_encoder = true; intel_dp_get_m_n(intel_crtc, pipe_config); + intel_dp_get_m2_n2(intel_crtc, pipe_config); break; default: break; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 797f01c..2e625eb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6670,6 +6670,34 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, &pipe_config->dp_m_n); } +void intel_dp_get_m2_n2(struct intel_crtc *crtc, + struct intel_crtc_config *pipe_config) +{ + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + enum transcoder transcoder = pipe_config->cpu_transcoder; + + if (INTEL_INFO(dev)->gen >= 8) { + intel_cpu_transcoder_get_m_n(crtc, transcoder, + &pipe_config->dp_m_n); + } else if (INTEL_INFO(dev)->gen > 6) { + pipe_config->dp_m2_n2.link_m = + I915_READ(PIPE_LINK_M2(transcoder)); + pipe_config->dp_m2_n2.link_n = + I915_READ(PIPE_LINK_N2(transcoder)); + pipe_config->dp_m2_n2.gmch_m = + I915_READ(PIPE_DATA_M2(transcoder)) + & ~TU_SIZE_MASK; + pipe_config->dp_m2_n2.gmch_n = + I915_READ(PIPE_DATA_N2(transcoder)); + pipe_config->dp_m2_n2.tu = + ((I915_READ(PIPE_DATA_M2(transcoder)) + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; + } + +} + + static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, struct intel_crtc_config *pipe_config) { @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n, pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n, pipe_config->dp_m_n.tu); + + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n", + pipe_config->has_dp_encoder, + pipe_config->dp_m2_n2.gmch_m, + pipe_config->dp_m2_n2.gmch_n, + pipe_config->dp_m2_n2.link_m, + pipe_config->dp_m2_n2.link_n, + pipe_config->dp_m2_n2.tu); + DRM_DEBUG_KMS("requested mode:\n"); drm_mode_debug_printmodeline(&pipe_config->requested_mode); DRM_DEBUG_KMS("adjusted mode:\n"); @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev, struct intel_crtc_config *current_config, struct intel_crtc_config *pipe_config) { + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_connector *intel_connector = dev_priv->drrs.connector; + struct intel_encoder *encoder = (intel_connector != NULL) ? + intel_attached_encoder(&intel_connector->base) : + NULL; + struct intel_dp *intel_dp = (encoder != NULL) ? + enc_to_intel_dp(&encoder->base) : NULL; + #define PIPE_CONF_CHECK_X(name) \ if (current_config->name != pipe_config->name) { \ DRM_ERROR("mismatch in " #name " " \ @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev, PIPE_CONF_CHECK_I(fdi_m_n.tu); PIPE_CONF_CHECK_I(has_dp_encoder); - PIPE_CONF_CHECK_I(dp_m_n.gmch_m); - PIPE_CONF_CHECK_I(dp_m_n.gmch_n); - PIPE_CONF_CHECK_I(dp_m_n.link_m); - PIPE_CONF_CHECK_I(dp_m_n.link_n); - PIPE_CONF_CHECK_I(dp_m_n.tu); + + + /* DP M1_N1 registers are used when DRRS is disabled or when high RR + * is used. Else, DP M2_N2 registers are used. The following check + * has been added to make sure a mismatch (if any) is displayed only + * for a real difference and not because of DRRS state. + */ + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) || + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp && + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) { + PIPE_CONF_CHECK_I(dp_m_n.gmch_m); + PIPE_CONF_CHECK_I(dp_m_n.gmch_n); + PIPE_CONF_CHECK_I(dp_m_n.link_m); + PIPE_CONF_CHECK_I(dp_m_n.link_n); + PIPE_CONF_CHECK_I(dp_m_n.tu); + } else { + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m); + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n); + PIPE_CONF_CHECK_I(dp_m2_n2.link_m); + PIPE_CONF_CHECK_I(dp_m2_n2.link_n); + PIPE_CONF_CHECK_I(dp_m2_n2.tu); + } PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 34ed143..9aa4dcd 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder, intel_dp_get_m_n(crtc, pipe_config); + intel_dp_get_m2_n2(crtc, pipe_config); + if (port == PORT_A) { if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ) pipe_config->port_clock = 162000; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d8b540b..1013f70 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv); void hsw_disable_pc8(struct drm_i915_private *dev_priv); void intel_dp_get_m_n(struct intel_crtc *crtc, struct intel_crtc_config *pipe_config); +void intel_dp_get_m2_n2(struct intel_crtc *crtc, + struct intel_crtc_config *pipe_config); int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n); void ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
Adding relevant read out comparison code, in check_crtc_state, for the new member of crtc_config, dp_m2_n2, which was introduced to store link_m_n values for a DP downclock mode (if available). Suggested by Daniel. v2: Changed patch title. Daniel's review comments incorporated. Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done only when high RR is not in use (This is because alternate m_n register programming will be done only when low RR is being used). Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_ddi.c | 1 + drivers/gpu/drm/i915/intel_display.c | 72 +++++++++++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_dp.c | 2 + drivers/gpu/drm/i915/intel_drv.h | 2 + 4 files changed, 72 insertions(+), 5 deletions(-)