From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vandana Kannan Subject: Re: [PATCH v2] drm/i915: State readout and cross-checking for dp_m2_n2 Date: Mon, 19 May 2014 17:00:14 +0530 Message-ID: <5379EB46.9010302@intel.com> References: <20140422191526.GA10722@phenom.ffwll.local> <1399277971-2561-1-git-send-email-vandana.kannan@intel.com> <20140512102725.GD18465@intel.com> <5371D71C.1060506@intel.com> <20140513085855.GM25056@phenom.ffwll.local> <5371E8A2.2070203@intel.com> <5374864A.8020109@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id E30B86E70F for ; Mon, 19 May 2014 04:30:17 -0700 (PDT) In-Reply-To: <5374864A.8020109@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org 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=E4l=E4 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 t= he 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 regis= ter >>>>>> programming will be done only when low RR is being used). >>>>>> >>>>>> Signed-off-by: Vandana Kannan >>>>>> Cc: Daniel Vetter >>>>>> --- >>>>>> 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 =3D 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 =3D crtc->base.dev; >>>>>> + struct drm_i915_private *dev_priv =3D dev->dev_private; >>>>>> + enum transcoder transcoder =3D pipe_config->cpu_transcoder; >>>>>> + >>>>>> + if (INTEL_INFO(dev)->gen >=3D 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 =3D >>>>>> + I915_READ(PIPE_LINK_M2(transcoder)); >>>>>> + pipe_config->dp_m2_n2.link_n =3D >>>>>> + I915_READ(PIPE_LINK_N2(transcoder)); >>>>>> + pipe_config->dp_m2_n2.gmch_m =3D >>>>>> + I915_READ(PIPE_DATA_M2(transcoder)) >>>>>> + & ~TU_SIZE_MASK; >>>>>> + pipe_config->dp_m2_n2.gmch_n =3D >>>>>> + I915_READ(PIPE_DATA_N2(transcoder)); >>>>>> + pipe_config->dp_m2_n2.tu =3D >>>>>> + ((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 int= el_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 =3D dev->dev_private; >>>>>> + struct intel_connector *intel_connector =3D dev_priv->drrs.connect= or; >>>>>> + struct intel_encoder *encoder =3D (intel_connector !=3D NULL) ? >>>>>> + intel_attached_encoder(&intel_connector->base) : >>>>>> + NULL; >>>>>> + struct intel_dp *intel_dp =3D (encoder !=3D NULL) ? >>>>>> + enc_to_intel_dp(&encoder->base) : NULL; >>>>>> + >>>>>> #define PIPE_CONF_CHECK_X(name) \ >>>>>> if (current_config->name !=3D 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 on= ly >>>>>> + * for a real difference and not because of DRRS state. >>>>>> + */ >>>>>> + if ((dev_priv->vbt.drrs_type =3D=3D DRRS_NOT_SUPPORTED) || >>>>>> + (dev_priv->vbt.drrs_type !=3D DRRS_NOT_SUPPORTED && intel_dp && >>>>>> + intel_dp->drrs_state.refresh_rate_type =3D=3D 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 n= ot >>>> programmed in this case. There will be a mismatch in the pipe_config c= heck. >>>> >>>> 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, b= ut >>>> this becomes invalid for gen8 and above where only 1 set of MN registe= rs >>>> 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+exter= nal >>>> * 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 t= his >>> 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_e= ncoder *encoder, >>>>>> = >>>>>> intel_dp_get_m_n(crtc, pipe_config); >>>>>> = >>>>>> + intel_dp_get_m2_n2(crtc, pipe_config); >>>>>> + >>>>>> if (port =3D=3D PORT_A) { >>>>>> if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) =3D=3D DP_PLL_FREQ_160MH= Z) >>>>>> pipe_config->port_clock =3D 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 *pip= e_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 > =