[Intel-gfx] [PATCH 3/5] drm/i915: Add support for DRRS to switch RR
Jani Nikula
jani.nikula at linux.intel.com
Thu Jan 30 07:52:33 CET 2014
On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan at intel.com> wrote:
> On Jan-22-2014 7:44 PM, Jani Nikula wrote:
>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan at intel.com> wrote:
>>> From: Pradeep Bhat <pradeep.bhat at intel.com>
>>>
>>> This patch computes and stored 2nd M/N/TU for switching to different
>>> refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
>>> between alternate refresh rates programmed in 2nd M/N/TU registers.
>>>
>>> v2: Daniel's review comments
>>> Computing M2/N2 in compute_config and storing it in crtc_config
>>>
>>> v3: Modified reference to edp_downclock and edp_downclock_avail based on the
>>> changes made to move them from dev_private to intel_panel.
>>>
>>> v4: Modified references to is_drrs_supported based on the changes made to
>>> rename it to drrs_support.
>>>
>>> Signed-off-by: Pradeep Bhat <pradeep.bhat at intel.com>
>>> Signed-off-by: Vandana Kannan <vandana.kannan at intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_reg.h | 1 +
>>> drivers/gpu/drm/i915/intel_dp.c | 106 ++++++++++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/i915/intel_drv.h | 6 ++-
>>> 3 files changed, 112 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index f1eece4..57d2b64 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -3206,6 +3206,7 @@
>>> #define PIPECONF_INTERLACED_DBL_ILK (4 << 21) /* ilk/snb only */
>>> #define PIPECONF_PFIT_PF_INTERLACED_DBL_ILK (5 << 21) /* ilk/snb only */
>>> #define PIPECONF_INTERLACE_MODE_MASK (7 << 21)
>>> +#define PIPECONF_EDP_RR_MODE_SWITCH (1 << 20)
>>> #define PIPECONF_CXSR_DOWNCLOCK (1<<16)
>>> #define PIPECONF_COLOR_RANGE_SELECT (1 << 13)
>>> #define PIPECONF_BPC_MASK (0x7 << 5)
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 079b53f..d1e1d6e 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -791,6 +791,21 @@ intel_dp_set_clock(struct intel_encoder *encoder,
>>> }
>>> }
>>>
>>> +static void
>>> +intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
>>> +{
>>> + struct drm_device *dev = crtc->base.dev;
>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>> + enum transcoder transcoder = crtc->config.cpu_transcoder;
>>> +
>>> + I915_WRITE(PIPE_DATA_M2(transcoder),
>>> + TU_SIZE(m_n->tu) | m_n->gmch_m);
>>> + I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
>>> + I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
>>> + I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
>>> + return;
>>
>> Superfluous return statement.
>>
> Ok.
>>> +}
>>> +
>>> bool
>>> intel_dp_compute_config(struct intel_encoder *encoder,
>>> struct intel_crtc_config *pipe_config)
>>> @@ -894,6 +909,14 @@ found:
>>> pipe_config->port_clock,
>>> &pipe_config->dp_m_n);
>>>
>>> + if (intel_connector->panel.edp_downclock_avail &&
>>> + intel_dp->drrs_state.drrs_support == SEAMLESS_DRRS_SUPPORT) {
>>> + intel_link_compute_m_n(bpp, lane_count,
>>> + intel_connector->panel.edp_downclock,
>>> + pipe_config->port_clock,
>>> + &pipe_config->dp_m2_n2);
>>> + }
>>
>> Indentation in the above block.
>>
> Ok.
>>> +
>>> intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
>>>
>>> return true;
>>> @@ -3522,6 +3545,87 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>>> I915_READ(pp_div_reg));
>>> }
>>>
>>> +void
>>> +intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate) {
>>
>> The opening brace belongs on a new line.
>>
> Ok.
>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>> + struct drm_mode_config *mode_config = &dev->mode_config;
>>> + struct intel_encoder *encoder;
>>> + struct intel_dp *intel_dp = NULL;
>>> + struct intel_crtc_config *config = NULL;
>>> + struct intel_crtc *intel_crtc = NULL;
>>> + struct intel_connector *intel_connector = NULL;
>>> + bool found_edp = false;
>>> + u32 reg, val;
>>> + int index = 0;
>>> +
>>> + if (refresh_rate <= 0) {
>>> + DRM_INFO("Refresh rate should be positive non-zero.\n");
>>> + goto out;
>>> + }
>>
>> Under what conditions do you expect this to happen?
>>
> In future, when refresh rate switching based on user space requests will
> be implemented, intel_dp_set_drrs_state() will be used. This check here
> is to safeguard calls from user space.
>>> +
>>> + list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
>>> + if (encoder->type == INTEL_OUTPUT_EDP) {
>>> + intel_dp = enc_to_intel_dp(&encoder->base);
>>> + intel_crtc = encoder->new_crtc;
>>> + if (!intel_crtc) {
>>> + DRM_INFO("DRRS: intel_crtc not initialized\n");
>>
>> DRM_INFO is probably a bit verbose.
>>
>>> + goto out;
>>> + }
>>> + config = &intel_crtc->config;
>>> + intel_connector = intel_dp->attached_connector;
>>> + found_edp = true;
>>> + break;
>>> + }
>>> + }
>>
>> I'm confused. You go through all the trouble of saving the crtc and the
>> connector (although only in the next patch) but here you iterate all the
>> encoders to find the one. Why?
>>
>>> +
>>> + if (!found_edp) {
>>
>> There's no need to add an extra variable for this. You already have
>> intel_dp, intel_crtc, config and intel_connector you can check!
>>
> Ok. I will make necessary changes.
>>> + DRM_INFO("DRRS supported for eDP only.\n");
>>> + goto out;
>>> + }
>>> +
>>> + if (intel_dp->drrs_state.drrs_support < SEAMLESS_DRRS_SUPPORT) {
>>> + DRM_INFO("Seamless DRRS not supported.\n");
>>> + goto out;
>>> + }
>>> +
>>> + if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
>>> + index = DRRS_HIGH_RR;
>>> + else
>>> + index = DRRS_LOW_RR;
>>> +
>>> + if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
>>> + DRM_INFO("DRRS requested for previously set RR...ignoring\n");
>>> + goto out;
>>> + }
>>> +
>>> + if (!intel_crtc->active) {
>>> + DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
>>> + goto out;
>>> + }
>>> +
>>> + mutex_lock(&intel_dp->drrs_state.mutex);
>>> +
>>> + /* Haswell and below */
>>> + if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
>>> + reg = PIPECONF(intel_crtc->config.cpu_transcoder);
>>> + val = I915_READ(reg);
>>> + if (index > DRRS_HIGH_RR) {
>>> + val |= PIPECONF_EDP_RR_MODE_SWITCH;
>>
>> Please check bspec for workarounds you need to do wrt frame start delay
>> before enabling the pipe/transcoder.
>>
> We checked the BSpec and there was no workaround mentioned for this part.
Oh, I'm sorry, I should have been more specific. This was in the SNB
specs: "Workaround : If this pipe is connected to a port on the PCH and
this power savings mode will be used, then before the pipe and
transcoder are enabled, the frame start delay in the pipe and transcoder
must be set to 11b. When these conditions are no longer true, the frame
start delay must be returned to the previous value after the pipe and
transcoder are disabled."
Going forward, I think it's okay to enable drrs on ivb+ at first, and
enable snb with the workarounds in a follow-up patch. Others may
disagree...
>>> + intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
>>> + } else
>>> + val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
>>
>> Per coding style, if one branch needs braces, then all do.
>>
> Ok.
>>> + I915_WRITE(reg, val);
>>> + }
>>> +
>>> + intel_dp->drrs_state.drrs_refresh_rate_type = index;
>>> + DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
>>> +
>>> + mutex_unlock(&intel_dp->drrs_state.mutex);
>>> +
>>> +out:
>>> + return;
>>
>> Superfluous return statement.
>>
> Ok.
>>> +}
>>> +
>>> static void
>>> intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>> struct intel_connector *intel_connector,
>>> @@ -3555,6 +3659,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>> intel_connector->panel.edp_downclock =
>>> intel_connector->panel.downclock_mode->clock;
>>>
>>> + mutex_init(&intel_dp->drrs_state.mutex);
>>> +
>>> intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
>>>
>>> intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index d208bf5..d1c60fa 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -291,6 +291,9 @@ struct intel_crtc_config {
>>> int pipe_bpp;
>>> struct intel_link_m_n dp_m_n;
>>>
>>> + /* m2_n2 for eDP downclock */
>>> + struct intel_link_m_n dp_m2_n2;
>>> +
>>> /*
>>> * Frequence the dpll for the port should run at. Differs from the
>>> * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
>>> @@ -489,6 +492,7 @@ enum edp_drrs_refresh_rate_type {
>>> struct drrs_info {
>>> enum drrs_support_type drrs_support;
>>> enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
>>> + struct mutex mutex;
>>> };
>>>
>>> struct intel_dp {
>>> @@ -761,7 +765,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>>> void intel_edp_psr_enable(struct intel_dp *intel_dp);
>>> void intel_edp_psr_disable(struct intel_dp *intel_dp);
>>> void intel_edp_psr_update(struct drm_device *dev);
>>> -
>>> +extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
>>
>> No need for the "extern".
>>
> Ok.
>>>
>>> /* intel_dsi.c */
>>> bool intel_dsi_init(struct drm_device *dev);
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list