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@xxxxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
