Re: [PATCH 3/5] drm/i915: Add support for DRRS to switch RR

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
> From: Pradeep Bhat <pradeep.bhat@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@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@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.

> +}
> +
>  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.

> +
>  	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.

> +	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?

> +
> +	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!

> +		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.

> +			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.

> +		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.

> +}
> +
>  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".

>  
>  /* intel_dsi.c */
>  bool intel_dsi_init(struct drm_device *dev);
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center