| Message ID | 1400741300-4566-1-git-send-email-vandana.kannan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, Please let me know if this patch (http://lists.freedesktop.org/archives/intel-gfx/2014-May/045877.html) and the patch http://lists.freedesktop.org/archives/intel-gfx/2014-May/045804.html require any other changes.. I have included all your review comments till date.. Thanks, Vandana On May-22-2014 12:18 PM, Kannan, Vandana 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). > > v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. > Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures > based on DRRS state for gen 8 and above. > Save and restore M2 N2 registers for gen 7 and below > > v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is > only one set of M_N registers > > v5: Removed the chunk which saves and restores M2_N2 registers. Modified > get_m_n() to get M2_N2 registers as well. Modified the macro which compares > hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. > > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- > 1 file changed, 60 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cf3ad87..d593897 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, > > static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > enum transcoder transcoder, > - struct intel_link_m_n *m_n) > + struct intel_link_m_n *m_n, > + struct intel_link_m_n *m2_n2) > { > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); > m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) > & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { > + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); > + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); > + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) > + & ~TU_SIZE_MASK; > + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); > + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) > + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > + } > } else { > m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); > m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); > @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); > else > intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > - &pipe_config->dp_m_n); > + &pipe_config->dp_m_n, > + &pipe_config->dp_m2_n2); > } > > static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, > struct intel_crtc_config *pipe_config) > { > intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > - &pipe_config->fdi_m_n); > + &pipe_config->fdi_m_n, NULL); > } > > static void ironlake_get_pfit_config(struct intel_crtc *crtc, > @@ -9485,6 +9496,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"); > @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, > return false; \ > } > > +/* This is required for BDW+ where there is only one set of registers for > + * switching between high and low RR. > + * This macro can be used whenever a comparison has to be made between one > + * hw state and multiple sw state variables. > + */ > +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ > + if ((current_config->name != pipe_config->name) && \ > + (current_config->alt_name != pipe_config->name)) { \ > + DRM_ERROR("mismatch in " #name " " \ > + "(expected %i or %i, found %i)\n", \ > + current_config->name, \ > + current_config->alt_name, \ > + pipe_config->name); \ > + return false; \ > + } > + > #define PIPE_CONF_CHECK_FLAGS(name, mask) \ > if ((current_config->name ^ pipe_config->name) & (mask)) { \ > DRM_ERROR("mismatch in " #name "(" #mask ") " \ > @@ -9899,11 +9935,26 @@ 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); > + > + if (INTEL_INFO(dev)->gen < 8) { > + 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); > + > + 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); > + } else { > + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); > + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); > + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); > + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); > + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); > + } > > PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); > PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); > @@ -9980,6 +10031,7 @@ intel_pipe_config_compare(struct drm_device *dev, > > #undef PIPE_CONF_CHECK_X > #undef PIPE_CONF_CHECK_I > +#undef PIPE_CONF_CHECK_I_ALT > #undef PIPE_CONF_CHECK_FLAGS > #undef PIPE_CONF_CHECK_CLOCK_FUZZY > #undef PIPE_CONF_QUIRK >
On Thu, 22 May 2014, Vandana Kannan <vandana.kannan@intel.com> 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). > > v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. > Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures > based on DRRS state for gen 8 and above. > Save and restore M2 N2 registers for gen 7 and below > > v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is > only one set of M_N registers > > v5: Removed the chunk which saves and restores M2_N2 registers. Modified > get_m_n() to get M2_N2 registers as well. Modified the macro which compares > hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. > > Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- > 1 file changed, 60 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cf3ad87..d593897 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, > > static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > enum transcoder transcoder, > - struct intel_link_m_n *m_n) > + struct intel_link_m_n *m_n, > + struct intel_link_m_n *m2_n2) > { > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); > m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) > & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { > + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); > + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); > + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) > + & ~TU_SIZE_MASK; > + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); > + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) > + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > + } > } else { > m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); > m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); > @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); > else > intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > - &pipe_config->dp_m_n); > + &pipe_config->dp_m_n, > + &pipe_config->dp_m2_n2); > } > > static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, > struct intel_crtc_config *pipe_config) > { > intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > - &pipe_config->fdi_m_n); > + &pipe_config->fdi_m_n, NULL); > } > > static void ironlake_get_pfit_config(struct intel_crtc *crtc, > @@ -9485,6 +9496,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"); > @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, > return false; \ > } > > +/* This is required for BDW+ where there is only one set of registers for > + * switching between high and low RR. > + * This macro can be used whenever a comparison has to be made between one > + * hw state and multiple sw state variables. > + */ > +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ > + if ((current_config->name != pipe_config->name) && \ > + (current_config->alt_name != pipe_config->name)) { \ > + DRM_ERROR("mismatch in " #name " " \ > + "(expected %i or %i, found %i)\n", \ > + current_config->name, \ > + current_config->alt_name, \ > + pipe_config->name); \ > + return false; \ > + } > + > #define PIPE_CONF_CHECK_FLAGS(name, mask) \ > if ((current_config->name ^ pipe_config->name) & (mask)) { \ > DRM_ERROR("mismatch in " #name "(" #mask ") " \ > @@ -9899,11 +9935,26 @@ 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); > + > + if (INTEL_INFO(dev)->gen < 8) { > + 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); > + > + 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); > + } else { > + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); > + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); > + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); > + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); > + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); If there's no downclock mode (e.g. because it's not eDP), this now accepts register value 0 as okay for each state check. That's not cool. Perhaps drrs state should be part of pipe config. BR, Jani. > + } > > PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); > PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); > @@ -9980,6 +10031,7 @@ intel_pipe_config_compare(struct drm_device *dev, > > #undef PIPE_CONF_CHECK_X > #undef PIPE_CONF_CHECK_I > +#undef PIPE_CONF_CHECK_I_ALT > #undef PIPE_CONF_CHECK_FLAGS > #undef PIPE_CONF_CHECK_CLOCK_FUZZY > #undef PIPE_CONF_QUIRK > -- > 1.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Jun-13-2014 7:42 PM, Jani Nikula wrote: > On Thu, 22 May 2014, Vandana Kannan <vandana.kannan@intel.com> 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). >> >> v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. >> Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures >> based on DRRS state for gen 8 and above. >> Save and restore M2 N2 registers for gen 7 and below >> >> v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is >> only one set of M_N registers >> >> v5: Removed the chunk which saves and restores M2_N2 registers. Modified >> get_m_n() to get M2_N2 registers as well. Modified the macro which compares >> hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. >> >> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- >> 1 file changed, 60 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index cf3ad87..d593897 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, >> >> static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, >> enum transcoder transcoder, >> - struct intel_link_m_n *m_n) >> + struct intel_link_m_n *m_n, >> + struct intel_link_m_n *m2_n2) >> { >> struct drm_device *dev = crtc->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, >> m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); >> m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) >> & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >> + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { >> + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); >> + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); >> + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) >> + & ~TU_SIZE_MASK; >> + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); >> + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) >> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >> + } >> } else { >> m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); >> m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); >> @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >> intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); >> else >> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, >> - &pipe_config->dp_m_n); >> + &pipe_config->dp_m_n, >> + &pipe_config->dp_m2_n2); >> } >> >> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >> struct intel_crtc_config *pipe_config) >> { >> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, >> - &pipe_config->fdi_m_n); >> + &pipe_config->fdi_m_n, NULL); >> } >> >> static void ironlake_get_pfit_config(struct intel_crtc *crtc, >> @@ -9485,6 +9496,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"); >> @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, >> return false; \ >> } >> >> +/* This is required for BDW+ where there is only one set of registers for >> + * switching between high and low RR. >> + * This macro can be used whenever a comparison has to be made between one >> + * hw state and multiple sw state variables. >> + */ >> +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ >> + if ((current_config->name != pipe_config->name) && \ >> + (current_config->alt_name != pipe_config->name)) { \ >> + DRM_ERROR("mismatch in " #name " " \ >> + "(expected %i or %i, found %i)\n", \ >> + current_config->name, \ >> + current_config->alt_name, \ >> + pipe_config->name); \ >> + return false; \ >> + } >> + >> #define PIPE_CONF_CHECK_FLAGS(name, mask) \ >> if ((current_config->name ^ pipe_config->name) & (mask)) { \ >> DRM_ERROR("mismatch in " #name "(" #mask ") " \ >> @@ -9899,11 +9935,26 @@ 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); >> + >> + if (INTEL_INFO(dev)->gen < 8) { >> + 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); >> + >> + 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); >> + } else { >> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); >> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); >> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); >> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); >> + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); > > If there's no downclock mode (e.g. because it's not eDP), this now > accepts register value 0 as okay for each state check. That's not cool. > > Perhaps drrs state should be part of pipe config. > > BR, > Jani. > Ok, shall I go ahead with the following approach? pipe_config{ drrs_state; } in pipe_config_compare() { if gen < 8 { compare dp_m1_n1 if drrs_state == seamless compare hw.dp_m2_n2 and sw.dp_m2_n2 /* this drrs_state would be set only in edp_init_connector and only when a downclock_mode is found */ } else compare hw.dp_m_n and sw.dp_m_n or sw.dp_m2_n2 } drrs_state is stored in vbt struct and intel_dp as of now. some changes would be required around this to avoid saving this state at a third place (pipe_config). option 1. move drrs_state from intel_dp to dev_priv. option 2. move drrs_state from intel_dp to pipe_config. with the above changes, the patches accessing intel_dp->drrs_state would require changes. Please let me know your inputs on this. -Vandana > > > > > > >> + } >> >> PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); >> PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); >> @@ -9980,6 +10031,7 @@ intel_pipe_config_compare(struct drm_device *dev, >> >> #undef PIPE_CONF_CHECK_X >> #undef PIPE_CONF_CHECK_I >> +#undef PIPE_CONF_CHECK_I_ALT >> #undef PIPE_CONF_CHECK_FLAGS >> #undef PIPE_CONF_CHECK_CLOCK_FUZZY >> #undef PIPE_CONF_QUIRK >> -- >> 1.9.3 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, 16 Jun 2014, Vandana Kannan <vandana.kannan@intel.com> wrote: > On Jun-13-2014 7:42 PM, Jani Nikula wrote: >> On Thu, 22 May 2014, Vandana Kannan <vandana.kannan@intel.com> 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). >>> >>> v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. >>> Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures >>> based on DRRS state for gen 8 and above. >>> Save and restore M2 N2 registers for gen 7 and below >>> >>> v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is >>> only one set of M_N registers >>> >>> v5: Removed the chunk which saves and restores M2_N2 registers. Modified >>> get_m_n() to get M2_N2 registers as well. Modified the macro which compares >>> hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. >>> >>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> --- >>> drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- >>> 1 file changed, 60 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>> index cf3ad87..d593897 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, >>> >>> static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, >>> enum transcoder transcoder, >>> - struct intel_link_m_n *m_n) >>> + struct intel_link_m_n *m_n, >>> + struct intel_link_m_n *m2_n2) >>> { >>> struct drm_device *dev = crtc->base.dev; >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, >>> m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); >>> m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) >>> & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>> + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { >>> + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); >>> + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); >>> + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) >>> + & ~TU_SIZE_MASK; >>> + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); >>> + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) >>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>> + } >>> } else { >>> m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); >>> m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); >>> @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >>> intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); >>> else >>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, >>> - &pipe_config->dp_m_n); >>> + &pipe_config->dp_m_n, >>> + &pipe_config->dp_m2_n2); >>> } >>> >>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >>> struct intel_crtc_config *pipe_config) >>> { >>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, >>> - &pipe_config->fdi_m_n); >>> + &pipe_config->fdi_m_n, NULL); >>> } >>> >>> static void ironlake_get_pfit_config(struct intel_crtc *crtc, >>> @@ -9485,6 +9496,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"); >>> @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, >>> return false; \ >>> } >>> >>> +/* This is required for BDW+ where there is only one set of registers for >>> + * switching between high and low RR. >>> + * This macro can be used whenever a comparison has to be made between one >>> + * hw state and multiple sw state variables. >>> + */ >>> +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ >>> + if ((current_config->name != pipe_config->name) && \ >>> + (current_config->alt_name != pipe_config->name)) { \ >>> + DRM_ERROR("mismatch in " #name " " \ >>> + "(expected %i or %i, found %i)\n", \ >>> + current_config->name, \ >>> + current_config->alt_name, \ >>> + pipe_config->name); \ >>> + return false; \ >>> + } >>> + >>> #define PIPE_CONF_CHECK_FLAGS(name, mask) \ >>> if ((current_config->name ^ pipe_config->name) & (mask)) { \ >>> DRM_ERROR("mismatch in " #name "(" #mask ") " \ >>> @@ -9899,11 +9935,26 @@ 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); >>> + >>> + if (INTEL_INFO(dev)->gen < 8) { >>> + 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); >>> + >>> + 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); >>> + } else { >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); >> >> If there's no downclock mode (e.g. because it's not eDP), this now >> accepts register value 0 as okay for each state check. That's not cool. >> >> Perhaps drrs state should be part of pipe config. >> >> BR, >> Jani. >> > Ok, shall I go ahead with the following approach? > > pipe_config{ > drrs_state; > } > > in pipe_config_compare() { > if gen < 8 { > compare dp_m1_n1 > if drrs_state == seamless > compare hw.dp_m2_n2 and sw.dp_m2_n2 > /* this drrs_state would be set only in edp_init_connector and only when > a downclock_mode is found */ > } > else > compare hw.dp_m_n and sw.dp_m_n or sw.dp_m2_n2 > } > > drrs_state is stored in vbt struct and intel_dp as of now. some changes > would be required around this to avoid saving this state at a third > place (pipe_config). > option 1. move drrs_state from intel_dp to dev_priv. > option 2. move drrs_state from intel_dp to pipe_config. > > with the above changes, the patches accessing intel_dp->drrs_state would > require changes. > > Please let me know your inputs on this. Daniel, any further thoughts on this? What fits best with future work? Or go ahead with the patch as-is, even though it has the corner case? BR, Jani. > > -Vandana > > >> >> >> >> >> >> >>> + } >>> >>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); >>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); >>> @@ -9980,6 +10031,7 @@ intel_pipe_config_compare(struct drm_device *dev, >>> >>> #undef PIPE_CONF_CHECK_X >>> #undef PIPE_CONF_CHECK_I >>> +#undef PIPE_CONF_CHECK_I_ALT >>> #undef PIPE_CONF_CHECK_FLAGS >>> #undef PIPE_CONF_CHECK_CLOCK_FUZZY >>> #undef PIPE_CONF_QUIRK >>> -- >>> 1.9.3 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >
On Tue, Jun 17, 2014 at 05:52:24PM +0300, Jani Nikula wrote: > On Mon, 16 Jun 2014, Vandana Kannan <vandana.kannan@intel.com> wrote: > > On Jun-13-2014 7:42 PM, Jani Nikula wrote: > >> On Thu, 22 May 2014, Vandana Kannan <vandana.kannan@intel.com> 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). > >>> > >>> v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. > >>> Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures > >>> based on DRRS state for gen 8 and above. > >>> Save and restore M2 N2 registers for gen 7 and below > >>> > >>> v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is > >>> only one set of M_N registers > >>> > >>> v5: Removed the chunk which saves and restores M2_N2 registers. Modified > >>> get_m_n() to get M2_N2 registers as well. Modified the macro which compares > >>> hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. > >>> > >>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>> --- > >>> drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- > >>> 1 file changed, 60 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>> index cf3ad87..d593897 100644 > >>> --- a/drivers/gpu/drm/i915/intel_display.c > >>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>> @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, > >>> > >>> static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > >>> enum transcoder transcoder, > >>> - struct intel_link_m_n *m_n) > >>> + struct intel_link_m_n *m_n, > >>> + struct intel_link_m_n *m2_n2) > >>> { > >>> struct drm_device *dev = crtc->base.dev; > >>> struct drm_i915_private *dev_priv = dev->dev_private; > >>> @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > >>> m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); > >>> m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) > >>> & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > >>> + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { > >>> + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); > >>> + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); > >>> + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) > >>> + & ~TU_SIZE_MASK; > >>> + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); > >>> + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) > >>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > >>> + } > >>> } else { > >>> m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); > >>> m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); > >>> @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > >>> intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); > >>> else > >>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > >>> - &pipe_config->dp_m_n); > >>> + &pipe_config->dp_m_n, > >>> + &pipe_config->dp_m2_n2); > >>> } > >>> > >>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, > >>> struct intel_crtc_config *pipe_config) > >>> { > >>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > >>> - &pipe_config->fdi_m_n); > >>> + &pipe_config->fdi_m_n, NULL); > >>> } > >>> > >>> static void ironlake_get_pfit_config(struct intel_crtc *crtc, > >>> @@ -9485,6 +9496,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"); > >>> @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, > >>> return false; \ > >>> } > >>> > >>> +/* This is required for BDW+ where there is only one set of registers for > >>> + * switching between high and low RR. > >>> + * This macro can be used whenever a comparison has to be made between one > >>> + * hw state and multiple sw state variables. > >>> + */ > >>> +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ > >>> + if ((current_config->name != pipe_config->name) && \ > >>> + (current_config->alt_name != pipe_config->name)) { \ > >>> + DRM_ERROR("mismatch in " #name " " \ > >>> + "(expected %i or %i, found %i)\n", \ > >>> + current_config->name, \ > >>> + current_config->alt_name, \ > >>> + pipe_config->name); \ > >>> + return false; \ > >>> + } > >>> + > >>> #define PIPE_CONF_CHECK_FLAGS(name, mask) \ > >>> if ((current_config->name ^ pipe_config->name) & (mask)) { \ > >>> DRM_ERROR("mismatch in " #name "(" #mask ") " \ > >>> @@ -9899,11 +9935,26 @@ 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); > >>> + > >>> + if (INTEL_INFO(dev)->gen < 8) { > >>> + 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); > >>> + > >>> + 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); > >>> + } else { > >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); > >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); > >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); > >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); > >>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); > >> > >> If there's no downclock mode (e.g. because it's not eDP), this now > >> accepts register value 0 as okay for each state check. That's not cool. > >> > >> Perhaps drrs state should be part of pipe config. > >> > >> BR, > >> Jani. > >> > > Ok, shall I go ahead with the following approach? > > > > pipe_config{ > > drrs_state; > > } > > > > in pipe_config_compare() { > > if gen < 8 { > > compare dp_m1_n1 > > if drrs_state == seamless > > compare hw.dp_m2_n2 and sw.dp_m2_n2 > > /* this drrs_state would be set only in edp_init_connector and only when > > a downclock_mode is found */ > > } > > else > > compare hw.dp_m_n and sw.dp_m_n or sw.dp_m2_n2 > > } > > > > drrs_state is stored in vbt struct and intel_dp as of now. some changes > > would be required around this to avoid saving this state at a third > > place (pipe_config). > > option 1. move drrs_state from intel_dp to dev_priv. > > option 2. move drrs_state from intel_dp to pipe_config. > > > > with the above changes, the patches accessing intel_dp->drrs_state would > > require changes. > > > > Please let me know your inputs on this. > > Daniel, any further thoughts on this? What fits best with future work? > Or go ahead with the patch as-is, even though it has the corner case? I think we should have a flag that says whether the 2nd set of regs is valid, then only use those if that's set. So amounts to adding a static copy to pipe_config, e.g. config.has_drrs -> 2nd set of values is valid. Or we just compare against 0 and don't use them if they're all 0 since that's just not a valid m/n setting. Future plans for drrs are more about tracking and will require moving things around in general all over the place. Imo no concern for this patch here. -Daniel
On Jun-17-2014 10:12 PM, Daniel Vetter wrote: > On Tue, Jun 17, 2014 at 05:52:24PM +0300, Jani Nikula wrote: >> On Mon, 16 Jun 2014, Vandana Kannan <vandana.kannan@intel.com> wrote: >>> On Jun-13-2014 7:42 PM, Jani Nikula wrote: >>>> On Thu, 22 May 2014, Vandana Kannan <vandana.kannan@intel.com> 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). >>>>> >>>>> v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. >>>>> Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures >>>>> based on DRRS state for gen 8 and above. >>>>> Save and restore M2 N2 registers for gen 7 and below >>>>> >>>>> v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is >>>>> only one set of M_N registers >>>>> >>>>> v5: Removed the chunk which saves and restores M2_N2 registers. Modified >>>>> get_m_n() to get M2_N2 registers as well. Modified the macro which compares >>>>> hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. >>>>> >>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- >>>>> 1 file changed, 60 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>> index cf3ad87..d593897 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>> @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, >>>>> >>>>> static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, >>>>> enum transcoder transcoder, >>>>> - struct intel_link_m_n *m_n) >>>>> + struct intel_link_m_n *m_n, >>>>> + struct intel_link_m_n *m2_n2) >>>>> { >>>>> struct drm_device *dev = crtc->base.dev; >>>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>>> @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, >>>>> m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); >>>>> m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) >>>>> & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>>>> + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { >>>>> + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); >>>>> + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); >>>>> + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) >>>>> + & ~TU_SIZE_MASK; >>>>> + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); >>>>> + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) >>>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>>>> + } >>>>> } else { >>>>> m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); >>>>> m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); >>>>> @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >>>>> intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); >>>>> else >>>>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, >>>>> - &pipe_config->dp_m_n); >>>>> + &pipe_config->dp_m_n, >>>>> + &pipe_config->dp_m2_n2); >>>>> } >>>>> >>>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >>>>> struct intel_crtc_config *pipe_config) >>>>> { >>>>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, >>>>> - &pipe_config->fdi_m_n); >>>>> + &pipe_config->fdi_m_n, NULL); >>>>> } >>>>> >>>>> static void ironlake_get_pfit_config(struct intel_crtc *crtc, >>>>> @@ -9485,6 +9496,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"); >>>>> @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, >>>>> return false; \ >>>>> } >>>>> >>>>> +/* This is required for BDW+ where there is only one set of registers for >>>>> + * switching between high and low RR. >>>>> + * This macro can be used whenever a comparison has to be made between one >>>>> + * hw state and multiple sw state variables. >>>>> + */ >>>>> +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ >>>>> + if ((current_config->name != pipe_config->name) && \ >>>>> + (current_config->alt_name != pipe_config->name)) { \ >>>>> + DRM_ERROR("mismatch in " #name " " \ >>>>> + "(expected %i or %i, found %i)\n", \ >>>>> + current_config->name, \ >>>>> + current_config->alt_name, \ >>>>> + pipe_config->name); \ >>>>> + return false; \ >>>>> + } >>>>> + >>>>> #define PIPE_CONF_CHECK_FLAGS(name, mask) \ >>>>> if ((current_config->name ^ pipe_config->name) & (mask)) { \ >>>>> DRM_ERROR("mismatch in " #name "(" #mask ") " \ >>>>> @@ -9899,11 +9935,26 @@ 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); >>>>> + >>>>> + if (INTEL_INFO(dev)->gen < 8) { >>>>> + 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); >>>>> + >>>>> + 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); >>>>> + } else { >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); >>>> >>>> If there's no downclock mode (e.g. because it's not eDP), this now >>>> accepts register value 0 as okay for each state check. That's not cool. >>>> >>>> Perhaps drrs state should be part of pipe config. >>>> >>>> BR, >>>> Jani. >>>> >>> Ok, shall I go ahead with the following approach? >>> >>> pipe_config{ >>> drrs_state; >>> } >>> >>> in pipe_config_compare() { >>> if gen < 8 { >>> compare dp_m1_n1 >>> if drrs_state == seamless >>> compare hw.dp_m2_n2 and sw.dp_m2_n2 >>> /* this drrs_state would be set only in edp_init_connector and only when >>> a downclock_mode is found */ >>> } >>> else >>> compare hw.dp_m_n and sw.dp_m_n or sw.dp_m2_n2 >>> } >>> >>> drrs_state is stored in vbt struct and intel_dp as of now. some changes >>> would be required around this to avoid saving this state at a third >>> place (pipe_config). >>> option 1. move drrs_state from intel_dp to dev_priv. >>> option 2. move drrs_state from intel_dp to pipe_config. >>> >>> with the above changes, the patches accessing intel_dp->drrs_state would >>> require changes. >>> >>> Please let me know your inputs on this. >> >> Daniel, any further thoughts on this? What fits best with future work? >> Or go ahead with the patch as-is, even though it has the corner case? > > I think we should have a flag that says whether the 2nd set of regs is > valid, then only use those if that's set. So amounts to adding a static > copy to pipe_config, e.g. config.has_drrs -> 2nd set of values is valid. > I had introduced HAS_DRRS(dev) in patch#1 of this 2-patch series. I could use that along with intel_crtc->config.has_dp_encoder check to proceed with dp_m2_n2 comparison. What are you thoughts on this? -Vandana > Or we just compare against 0 and don't use them if they're all 0 since > that's just not a valid m/n setting. > > Future plans for drrs are more about tracking and will require moving > things around in general all over the place. Imo no concern for this patch > here. > -Daniel >
On Wed, Jun 18, 2014 at 10:11:20AM +0530, Vandana Kannan wrote: > On Jun-17-2014 10:12 PM, Daniel Vetter wrote: > > On Tue, Jun 17, 2014 at 05:52:24PM +0300, Jani Nikula wrote: > >> On Mon, 16 Jun 2014, Vandana Kannan <vandana.kannan@intel.com> wrote: > >>> On Jun-13-2014 7:42 PM, Jani Nikula wrote: > >>>> On Thu, 22 May 2014, Vandana Kannan <vandana.kannan@intel.com> 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). > >>>>> > >>>>> v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. > >>>>> Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures > >>>>> based on DRRS state for gen 8 and above. > >>>>> Save and restore M2 N2 registers for gen 7 and below > >>>>> > >>>>> v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is > >>>>> only one set of M_N registers > >>>>> > >>>>> v5: Removed the chunk which saves and restores M2_N2 registers. Modified > >>>>> get_m_n() to get M2_N2 registers as well. Modified the macro which compares > >>>>> hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. > >>>>> > >>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> > >>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>>> --- > >>>>> drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- > >>>>> 1 file changed, 60 insertions(+), 8 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>>>> index cf3ad87..d593897 100644 > >>>>> --- a/drivers/gpu/drm/i915/intel_display.c > >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>>>> @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, > >>>>> > >>>>> static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > >>>>> enum transcoder transcoder, > >>>>> - struct intel_link_m_n *m_n) > >>>>> + struct intel_link_m_n *m_n, > >>>>> + struct intel_link_m_n *m2_n2) > >>>>> { > >>>>> struct drm_device *dev = crtc->base.dev; > >>>>> struct drm_i915_private *dev_priv = dev->dev_private; > >>>>> @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > >>>>> m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); > >>>>> m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) > >>>>> & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > >>>>> + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { > >>>>> + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); > >>>>> + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); > >>>>> + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) > >>>>> + & ~TU_SIZE_MASK; > >>>>> + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); > >>>>> + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) > >>>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; > >>>>> + } > >>>>> } else { > >>>>> m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); > >>>>> m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); > >>>>> @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, > >>>>> intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); > >>>>> else > >>>>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > >>>>> - &pipe_config->dp_m_n); > >>>>> + &pipe_config->dp_m_n, > >>>>> + &pipe_config->dp_m2_n2); > >>>>> } > >>>>> > >>>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, > >>>>> struct intel_crtc_config *pipe_config) > >>>>> { > >>>>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > >>>>> - &pipe_config->fdi_m_n); > >>>>> + &pipe_config->fdi_m_n, NULL); > >>>>> } > >>>>> > >>>>> static void ironlake_get_pfit_config(struct intel_crtc *crtc, > >>>>> @@ -9485,6 +9496,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"); > >>>>> @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, > >>>>> return false; \ > >>>>> } > >>>>> > >>>>> +/* This is required for BDW+ where there is only one set of registers for > >>>>> + * switching between high and low RR. > >>>>> + * This macro can be used whenever a comparison has to be made between one > >>>>> + * hw state and multiple sw state variables. > >>>>> + */ > >>>>> +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ > >>>>> + if ((current_config->name != pipe_config->name) && \ > >>>>> + (current_config->alt_name != pipe_config->name)) { \ > >>>>> + DRM_ERROR("mismatch in " #name " " \ > >>>>> + "(expected %i or %i, found %i)\n", \ > >>>>> + current_config->name, \ > >>>>> + current_config->alt_name, \ > >>>>> + pipe_config->name); \ > >>>>> + return false; \ > >>>>> + } > >>>>> + > >>>>> #define PIPE_CONF_CHECK_FLAGS(name, mask) \ > >>>>> if ((current_config->name ^ pipe_config->name) & (mask)) { \ > >>>>> DRM_ERROR("mismatch in " #name "(" #mask ") " \ > >>>>> @@ -9899,11 +9935,26 @@ 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); > >>>>> + > >>>>> + if (INTEL_INFO(dev)->gen < 8) { > >>>>> + 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); > >>>>> + > >>>>> + 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); > >>>>> + } else { > >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); > >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); > >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); > >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); > >>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); > >>>> > >>>> If there's no downclock mode (e.g. because it's not eDP), this now > >>>> accepts register value 0 as okay for each state check. That's not cool. > >>>> > >>>> Perhaps drrs state should be part of pipe config. > >>>> > >>>> BR, > >>>> Jani. > >>>> > >>> Ok, shall I go ahead with the following approach? > >>> > >>> pipe_config{ > >>> drrs_state; > >>> } > >>> > >>> in pipe_config_compare() { > >>> if gen < 8 { > >>> compare dp_m1_n1 > >>> if drrs_state == seamless > >>> compare hw.dp_m2_n2 and sw.dp_m2_n2 > >>> /* this drrs_state would be set only in edp_init_connector and only when > >>> a downclock_mode is found */ > >>> } > >>> else > >>> compare hw.dp_m_n and sw.dp_m_n or sw.dp_m2_n2 > >>> } > >>> > >>> drrs_state is stored in vbt struct and intel_dp as of now. some changes > >>> would be required around this to avoid saving this state at a third > >>> place (pipe_config). > >>> option 1. move drrs_state from intel_dp to dev_priv. > >>> option 2. move drrs_state from intel_dp to pipe_config. > >>> > >>> with the above changes, the patches accessing intel_dp->drrs_state would > >>> require changes. > >>> > >>> Please let me know your inputs on this. > >> > >> Daniel, any further thoughts on this? What fits best with future work? > >> Or go ahead with the patch as-is, even though it has the corner case? > > > > I think we should have a flag that says whether the 2nd set of regs is > > valid, then only use those if that's set. So amounts to adding a static > > copy to pipe_config, e.g. config.has_drrs -> 2nd set of values is valid. > > > I had introduced HAS_DRRS(dev) in patch#1 of this 2-patch series. I > could use that along with intel_crtc->config.has_dp_encoder check to > proceed with dp_m2_n2 comparison. > What are you thoughts on this? Yeah, makes sense. -Daniel
On Jun-18-2014 4:16 PM, Daniel Vetter wrote: > On Wed, Jun 18, 2014 at 10:11:20AM +0530, Vandana Kannan wrote: >> On Jun-17-2014 10:12 PM, Daniel Vetter wrote: >>> On Tue, Jun 17, 2014 at 05:52:24PM +0300, Jani Nikula wrote: >>>> On Mon, 16 Jun 2014, Vandana Kannan <vandana.kannan@intel.com> wrote: >>>>> On Jun-13-2014 7:42 PM, Jani Nikula wrote: >>>>>> On Thu, 22 May 2014, Vandana Kannan <vandana.kannan@intel.com> 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). >>>>>>> >>>>>>> v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. >>>>>>> Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures >>>>>>> based on DRRS state for gen 8 and above. >>>>>>> Save and restore M2 N2 registers for gen 7 and below >>>>>>> >>>>>>> v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is >>>>>>> only one set of M_N registers >>>>>>> >>>>>>> v5: Removed the chunk which saves and restores M2_N2 registers. Modified >>>>>>> get_m_n() to get M2_N2 registers as well. Modified the macro which compares >>>>>>> hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. >>>>>>> >>>>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> >>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- >>>>>>> 1 file changed, 60 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>>>>> index cf3ad87..d593897 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>>>>> @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, >>>>>>> >>>>>>> static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, >>>>>>> enum transcoder transcoder, >>>>>>> - struct intel_link_m_n *m_n) >>>>>>> + struct intel_link_m_n *m_n, >>>>>>> + struct intel_link_m_n *m2_n2) >>>>>>> { >>>>>>> struct drm_device *dev = crtc->base.dev; >>>>>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>>>>> @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, >>>>>>> m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); >>>>>>> m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) >>>>>>> & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>>>>>> + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { >>>>>>> + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); >>>>>>> + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); >>>>>>> + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) >>>>>>> + & ~TU_SIZE_MASK; >>>>>>> + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); >>>>>>> + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) >>>>>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; >>>>>>> + } >>>>>>> } else { >>>>>>> m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); >>>>>>> m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); >>>>>>> @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, >>>>>>> intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); >>>>>>> else >>>>>>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, >>>>>>> - &pipe_config->dp_m_n); >>>>>>> + &pipe_config->dp_m_n, >>>>>>> + &pipe_config->dp_m2_n2); >>>>>>> } >>>>>>> >>>>>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, >>>>>>> struct intel_crtc_config *pipe_config) >>>>>>> { >>>>>>> intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, >>>>>>> - &pipe_config->fdi_m_n); >>>>>>> + &pipe_config->fdi_m_n, NULL); >>>>>>> } >>>>>>> >>>>>>> static void ironlake_get_pfit_config(struct intel_crtc *crtc, >>>>>>> @@ -9485,6 +9496,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"); >>>>>>> @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, >>>>>>> return false; \ >>>>>>> } >>>>>>> >>>>>>> +/* This is required for BDW+ where there is only one set of registers for >>>>>>> + * switching between high and low RR. >>>>>>> + * This macro can be used whenever a comparison has to be made between one >>>>>>> + * hw state and multiple sw state variables. >>>>>>> + */ >>>>>>> +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ >>>>>>> + if ((current_config->name != pipe_config->name) && \ >>>>>>> + (current_config->alt_name != pipe_config->name)) { \ >>>>>>> + DRM_ERROR("mismatch in " #name " " \ >>>>>>> + "(expected %i or %i, found %i)\n", \ >>>>>>> + current_config->name, \ >>>>>>> + current_config->alt_name, \ >>>>>>> + pipe_config->name); \ >>>>>>> + return false; \ >>>>>>> + } >>>>>>> + >>>>>>> #define PIPE_CONF_CHECK_FLAGS(name, mask) \ >>>>>>> if ((current_config->name ^ pipe_config->name) & (mask)) { \ >>>>>>> DRM_ERROR("mismatch in " #name "(" #mask ") " \ >>>>>>> @@ -9899,11 +9935,26 @@ 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); >>>>>>> + >>>>>>> + if (INTEL_INFO(dev)->gen < 8) { >>>>>>> + 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); >>>>>>> + >>>>>>> + 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); >>>>>>> + } else { >>>>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); >>>>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); >>>>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); >>>>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); >>>>>>> + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); >>>>>> >>>>>> If there's no downclock mode (e.g. because it's not eDP), this now >>>>>> accepts register value 0 as okay for each state check. That's not cool. >>>>>> >>>>>> Perhaps drrs state should be part of pipe config. >>>>>> >>>>>> BR, >>>>>> Jani. >>>>>> >>>>> Ok, shall I go ahead with the following approach? >>>>> >>>>> pipe_config{ >>>>> drrs_state; >>>>> } >>>>> >>>>> in pipe_config_compare() { >>>>> if gen < 8 { >>>>> compare dp_m1_n1 >>>>> if drrs_state == seamless >>>>> compare hw.dp_m2_n2 and sw.dp_m2_n2 >>>>> /* this drrs_state would be set only in edp_init_connector and only when >>>>> a downclock_mode is found */ >>>>> } >>>>> else >>>>> compare hw.dp_m_n and sw.dp_m_n or sw.dp_m2_n2 >>>>> } >>>>> >>>>> drrs_state is stored in vbt struct and intel_dp as of now. some changes >>>>> would be required around this to avoid saving this state at a third >>>>> place (pipe_config). >>>>> option 1. move drrs_state from intel_dp to dev_priv. >>>>> option 2. move drrs_state from intel_dp to pipe_config. >>>>> >>>>> with the above changes, the patches accessing intel_dp->drrs_state would >>>>> require changes. >>>>> >>>>> Please let me know your inputs on this. >>>> >>>> Daniel, any further thoughts on this? What fits best with future work? >>>> Or go ahead with the patch as-is, even though it has the corner case? >>> >>> I think we should have a flag that says whether the 2nd set of regs is >>> valid, then only use those if that's set. So amounts to adding a static >>> copy to pipe_config, e.g. config.has_drrs -> 2nd set of values is valid. >>> >> I had introduced HAS_DRRS(dev) in patch#1 of this 2-patch series. I >> could use that along with intel_crtc->config.has_dp_encoder check to >> proceed with dp_m2_n2 comparison. >> What are you thoughts on this? > > Yeah, makes sense. > -Daniel > I will re-submit this patch with the change. I have rebased and resent patch#1. Additionally, once these 2 patches are ready for merge, I will rebase and resend the remaining 3 DRRS related patches.. 1. drm/i915: Idleness detection for DRRS (http://lists.freedesktop.org/archives/intel-gfx/2014-April/043493.html) 2. drm/i915/bdw: Add support for DRRS to switch RR (http://lists.freedesktop.org/archives/intel-gfx/2014-April/042804.html). This would have to be modified on top of the changes made in "drm/i915: Set M2_N2 registers during mode set". 3. drm/i915: Support for RR switching on VLV (http://lists.freedesktop.org/archives/intel-gfx/2014-April/042805.html) - Vandana
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cf3ad87..d593897 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6945,7 +6945,8 @@ static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc, static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, enum transcoder transcoder, - struct intel_link_m_n *m_n) + struct intel_link_m_n *m_n, + struct intel_link_m_n *m2_n2) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -6959,6 +6960,15 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, m_n->gmch_n = I915_READ(PIPE_DATA_N1(transcoder)); m_n->tu = ((I915_READ(PIPE_DATA_M1(transcoder)) & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; + if (m2_n2 && INTEL_INFO(dev)->gen < 8) { + m2_n2->link_m = I915_READ(PIPE_LINK_M2(transcoder)); + m2_n2->link_n = I915_READ(PIPE_LINK_N2(transcoder)); + m2_n2->gmch_m = I915_READ(PIPE_DATA_M2(transcoder)) + & ~TU_SIZE_MASK; + m2_n2->gmch_n = I915_READ(PIPE_DATA_N2(transcoder)); + m2_n2->tu = ((I915_READ(PIPE_DATA_M2(transcoder)) + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1; + } } else { m_n->link_m = I915_READ(PIPE_LINK_M_G4X(pipe)); m_n->link_n = I915_READ(PIPE_LINK_N_G4X(pipe)); @@ -6977,14 +6987,15 @@ void intel_dp_get_m_n(struct intel_crtc *crtc, intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); else intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, - &pipe_config->dp_m_n); + &pipe_config->dp_m_n, + &pipe_config->dp_m2_n2); } static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc, struct intel_crtc_config *pipe_config) { intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, - &pipe_config->fdi_m_n); + &pipe_config->fdi_m_n, NULL); } static void ironlake_get_pfit_config(struct intel_crtc *crtc, @@ -9485,6 +9496,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"); @@ -9867,6 +9887,22 @@ intel_pipe_config_compare(struct drm_device *dev, return false; \ } +/* This is required for BDW+ where there is only one set of registers for + * switching between high and low RR. + * This macro can be used whenever a comparison has to be made between one + * hw state and multiple sw state variables. + */ +#define PIPE_CONF_CHECK_I_ALT(name, alt_name) \ + if ((current_config->name != pipe_config->name) && \ + (current_config->alt_name != pipe_config->name)) { \ + DRM_ERROR("mismatch in " #name " " \ + "(expected %i or %i, found %i)\n", \ + current_config->name, \ + current_config->alt_name, \ + pipe_config->name); \ + return false; \ + } + #define PIPE_CONF_CHECK_FLAGS(name, mask) \ if ((current_config->name ^ pipe_config->name) & (mask)) { \ DRM_ERROR("mismatch in " #name "(" #mask ") " \ @@ -9899,11 +9935,26 @@ 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); + + if (INTEL_INFO(dev)->gen < 8) { + 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); + + 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); + } else { + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m); + PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n); + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m); + PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n); + PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu); + } PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay); PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal); @@ -9980,6 +10031,7 @@ intel_pipe_config_compare(struct drm_device *dev, #undef PIPE_CONF_CHECK_X #undef PIPE_CONF_CHECK_I +#undef PIPE_CONF_CHECK_I_ALT #undef PIPE_CONF_CHECK_FLAGS #undef PIPE_CONF_CHECK_CLOCK_FUZZY #undef PIPE_CONF_QUIRK
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). v3: Modified call to get_m2_n2 which had dp_m_n as param by mistake. Compare dp_m_n and dp_m2_n2 for gen 7 and below. compare the structures based on DRRS state for gen 8 and above. Save and restore M2 N2 registers for gen 7 and below v4: For Gen>=8, check M_N registers against dp_m_n and dp_m2_n2 as there is only one set of M_N registers v5: Removed the chunk which saves and restores M2_N2 registers. Modified get_m_n() to get M2_N2 registers as well. Modified the macro which compares hw.dp_m_n against sw.dp_m2_n2/sw.dp_m_n for gen > 8. Signed-off-by: Vandana Kannan <vandana.kannan@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 8 deletions(-)