On May-13-2014 3:10 PM, Vandana Kannan wrote:
> On May-13-2014 2:28 PM, Daniel Vetter wrote:
>> On Tue, May 13, 2014 at 01:56:04PM +0530, Vandana Kannan wrote:
>>> On May-12-2014 3:57 PM, Ville Syrjälä wrote:
>>>> On Mon, May 05, 2014 at 01:49:31PM +0530, Vandana Kannan wrote:
>>>>> Adding relevant read out comparison code, in check_crtc_state, for the new
>>>>> member of crtc_config, dp_m2_n2, which was introduced to store link_m_n
>>>>> values for a DP downclock mode (if available). Suggested by Daniel.
>>>>>
>>>>> v2: Changed patch title.
>>>>> Daniel's review comments incorporated.
>>>>> Added relevant state readout code for M2_N2. dp_m2_n2 comparison to be done
>>>>> only when high RR is not in use (This is because alternate m_n register
>>>>> programming will be done only when low RR is being used).
>>>>>
>>>>> Signed-off-by: Vandana Kannan <vandana.kannan@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?
>>>>
>>> My miss, will change this.
>>> For Gen8, there is only one set of MN registers to be programmed for
>>> high and low RR. Hence this check.
>>>>> + } else if (INTEL_INFO(dev)->gen > 6) {
>>>>> + pipe_config->dp_m2_n2.link_m =
>>>>> + I915_READ(PIPE_LINK_M2(transcoder));
>>>>> + pipe_config->dp_m2_n2.link_n =
>>>>> + I915_READ(PIPE_LINK_N2(transcoder));
>>>>> + pipe_config->dp_m2_n2.gmch_m =
>>>>> + I915_READ(PIPE_DATA_M2(transcoder))
>>>>> + & ~TU_SIZE_MASK;
>>>>> + pipe_config->dp_m2_n2.gmch_n =
>>>>> + I915_READ(PIPE_DATA_N2(transcoder));
>>>>> + pipe_config->dp_m2_n2.tu =
>>>>> + ((I915_READ(PIPE_DATA_M2(transcoder))
>>>>> + & TU_SIZE_MASK) >> TU_SIZE_SHIFT) + 1;
>>>>> + }
>>>>> +
>>>>> +}
>>>>>
>>>>> +
>>>>> static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc,
>>>>> struct intel_crtc_config *pipe_config)
>>>>> {
>>>>> @@ -9169,6 +9197,15 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>>>>> pipe_config->dp_m_n.gmch_m, pipe_config->dp_m_n.gmch_n,
>>>>> pipe_config->dp_m_n.link_m, pipe_config->dp_m_n.link_n,
>>>>> pipe_config->dp_m_n.tu);
>>>>> +
>>>>> + DRM_DEBUG_KMS("dp: %i, gmch_m2: %u, gmch_n2: %u, link_m2: %u, link_n2: %u, tu2: %u\n",
>>>>> + pipe_config->has_dp_encoder,
>>>>> + pipe_config->dp_m2_n2.gmch_m,
>>>>> + pipe_config->dp_m2_n2.gmch_n,
>>>>> + pipe_config->dp_m2_n2.link_m,
>>>>> + pipe_config->dp_m2_n2.link_n,
>>>>> + pipe_config->dp_m2_n2.tu);
>>>>> +
>>>>> DRM_DEBUG_KMS("requested mode:\n");
>>>>> drm_mode_debug_printmodeline(&pipe_config->requested_mode);
>>>>> DRM_DEBUG_KMS("adjusted mode:\n");
>>>>> @@ -9533,6 +9570,14 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>> struct intel_crtc_config *current_config,
>>>>> struct intel_crtc_config *pipe_config)
>>>>> {
>>>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>>>> + struct intel_connector *intel_connector = dev_priv->drrs.connector;
>>>>> + struct intel_encoder *encoder = (intel_connector != NULL) ?
>>>>> + intel_attached_encoder(&intel_connector->base) :
>>>>> + NULL;
>>>>> + struct intel_dp *intel_dp = (encoder != NULL) ?
>>>>> + enc_to_intel_dp(&encoder->base) : NULL;
>>>>> +
>>>>> #define PIPE_CONF_CHECK_X(name) \
>>>>> if (current_config->name != pipe_config->name) { \
>>>>> DRM_ERROR("mismatch in " #name " " \
>>>>> @@ -9583,11 +9628,28 @@ intel_pipe_config_compare(struct drm_device *dev,
>>>>> PIPE_CONF_CHECK_I(fdi_m_n.tu);
>>>>>
>>>>> PIPE_CONF_CHECK_I(has_dp_encoder);
>>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m);
>>>>> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n);
>>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_m);
>>>>> - PIPE_CONF_CHECK_I(dp_m_n.link_n);
>>>>> - PIPE_CONF_CHECK_I(dp_m_n.tu);
>>>>> +
>>>>> +
>>>>> + /* DP M1_N1 registers are used when DRRS is disabled or when high RR
>>>>> + * is used. Else, DP M2_N2 registers are used. The following check
>>>>> + * has been added to make sure a mismatch (if any) is displayed only
>>>>> + * for a real difference and not because of DRRS state.
>>>>> + */
>>>>> + if ((dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) ||
>>>>> + (dev_priv->vbt.drrs_type != DRRS_NOT_SUPPORTED && intel_dp &&
>>>>> + intel_dp->drrs_state.refresh_rate_type == DRRS_HIGH_RR)) {
>>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_m);
>>>>> + PIPE_CONF_CHECK_I(dp_m_n.gmch_n);
>>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_m);
>>>>> + PIPE_CONF_CHECK_I(dp_m_n.link_n);
>>>>> + PIPE_CONF_CHECK_I(dp_m_n.tu);
>>>>> + } else {
>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m);
>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n);
>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_m);
>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.link_n);
>>>>> + PIPE_CONF_CHECK_I(dp_m2_n2.tu);
>>>>> + }
>>>>
>>>> Can't we just check both dp_m_n and dp_m2_n2 always?
>>>>
>>> Daniel/Ville,
>>>
>>> pipe_config->dp_m2_n2 is populated along with pipe_config->dp_m_n in
>>> compute_config(). while pipe_config dp_m_n values are programmed into MN
>>> registers during the mode set, dp_m2_n2 values are programmed only when
>>> low RR is used.
>>> Given this, when PIPE_CONF_CHECK is done for dp_m2_n2, it would show a
>>> mismatch (struct contains values to be programmed but registers still
>>> have 0) and return.
>>> The reason these checks were added was to avoid the scenario in which a
>>> mismatch is highlighted when there is no real failure at all. For
>>> example, DRRS is enabled on boot but the scenario to trigger low RR has
>>> not yet occurred, dp_m2_n2 has been computed but M2_N2 registers are not
>>> programmed in this case. There will be a mismatch in the pipe_config check.
>>>
>>> Changes can be made to set M2_N2 registers when dp_m2_n2 is populated
>>> and toggle the bit (PIPECONF bit) to switch between high and low RR, but
>>> this becomes invalid for gen8 and above where only 1 set of MN registers
>>> are available.
>>>
>>> Please suggest some other way to handle this.
>>> May be a quirk like the following?
>>>
>>> /*
>>> * FIXME: BIOS likes to set up a cloned config with lvds+external
>>> * screen. Since we don't yet re-compute the pipe config when moving
>>> * just the lvds port away to another pipe the sw tracking won't
>>> match.
>>> *
>>> * Proper atomic modesets with recomputed global state will fix
>>> this.
>>> * Until then just don't check gmch state for inherited modes.
>>> */
>>> if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) {
>>>
>>> Or go ahead with checking dp_m_n and dp_m2_n2 always ?
>>> Please let me know..
>>
>> I think for BDW we should check whether the hw values match _either_
>> dp_m_n or dp_m2_n2. But for hsw and earlier we should check both.
>>
>> That leaves us with the slightly awkward issue of recovering DRRS state on
>> bdw. But I think since we sprinkle idle/busy calls all over the place this
>> is a problem we can just ignore ;-)
>> -Daniel
>>
>
> Ok, So I will make the following changes and resend..
> 1. in compute_config(), for gen < 8, set M2_N2 registers (instead of
> doing it in intel_dp_set_drrs_state()).
> 2. in intel_pipe_config_compare(), include your inputs given above..
> -Vandana
hi Daniel,
I see a few issues with this approach of setting M2 N2 registers ahead
and just toggling the bit to switch between refresh rates (for gen 7 and
below)..
writing into M2_N2 registers in compute_config() would fail when system
resumes from sleep. to overcome this, setting M2 N2 can be done in
crtc_mode_set functions (similar to set_m_n).
given this setting, suppose user space sets a refresh rate based on
usage scenario (future implementation maybe), which would come through
mode_set calls, M2 N2 registers can be appropriately programmed with
user space requested RR. now, (if kernel idleness detection for DRRS is
enabled) if the screen image is idle, then additional changes would be
required to program M2 N2 registers with the correct low RR values. For
example, set_m2_n2 would have to be called just before set_drrs for
lowest RR is called.
in summary, the alternative approach to avoid DRRS related checks in
pipe_config_compare() is doable but creates other inconveniences w.r.t
the overall design and interaction with user space.
please let me know your opinion on this..
-Vandana
>>>
>>> -Vandana
>>>
>>>>>
>>>>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_hdisplay);
>>>>> PIPE_CONF_CHECK_I(adjusted_mode.crtc_htotal);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index 34ed143..9aa4dcd 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -1511,6 +1511,8 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
>>>>>
>>>>> intel_dp_get_m_n(crtc, pipe_config);
>>>>>
>>>>> + intel_dp_get_m2_n2(crtc, pipe_config);
>>>>> +
>>>>> if (port == PORT_A) {
>>>>> if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ)
>>>>> pipe_config->port_clock = 162000;
>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>> index d8b540b..1013f70 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>> @@ -763,6 +763,8 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv);
>>>>> void hsw_disable_pc8(struct drm_i915_private *dev_priv);
>>>>> void intel_dp_get_m_n(struct intel_crtc *crtc,
>>>>> struct intel_crtc_config *pipe_config);
>>>>> +void intel_dp_get_m2_n2(struct intel_crtc *crtc,
>>>>> + struct intel_crtc_config *pipe_config);
>>>>> int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
>>>>> void
>>>>> ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
>>>>> --
>>>>> 1.9.2
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>
>>>
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
