[3/4] drm/i915: Enable/disable TMDS output buffers in DP++ adaptor as needed

Submitted by Ville Syrjälä on Feb. 23, 2016, 4:46 p.m.

Details

Message ID 1456245988-19442-4-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show
Series "drm: DP++ adaptor support" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Ville Syrjälä Feb. 23, 2016, 4:46 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

To save a bit of power, let's try to turn off the TMDS output buffers
in DP++ adaptors when we're not driving the port.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c | 27 +++++++++++++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 944eacbfb6a9..3ca29a181e64 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -706,6 +706,7 @@  struct intel_hdmi {
 	int ddc_bus;
 	struct {
 		int max_tmds_clock;
+		bool tmds_output_control;
 	} dp_dual_mode;
 	bool limited_color_range;
 	bool color_range_auto;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1e8cfdb18dc7..660a65f48fd8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -846,6 +846,13 @@  static void intel_hdmi_prepare(struct intel_encoder *encoder)
 	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
 	u32 hdmi_val;
 
+	if (intel_hdmi->dp_dual_mode.tmds_output_control) {
+		struct i2c_adapter *adapter =
+			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+		drm_dp_dual_mode_set_tmds_output(adapter, true);
+	}
+
 	hdmi_val = SDVO_ENCODING_HDMI;
 	if (!HAS_PCH_SPLIT(dev) && crtc->config->limited_color_range)
 		hdmi_val |= HDMI_COLOR_RANGE_16_235;
@@ -1144,6 +1151,13 @@  static void intel_disable_hdmi(struct intel_encoder *encoder)
 	}
 
 	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
+
+	if (intel_hdmi->dp_dual_mode.tmds_output_control) {
+		struct i2c_adapter *adapter =
+			intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+		drm_dp_dual_mode_set_tmds_output(adapter, false);
+	}
 }
 
 static void g4x_disable_hdmi(struct intel_encoder *encoder)
@@ -1369,6 +1383,7 @@  intel_hdmi_unset_edid(struct drm_connector *connector)
 	intel_hdmi->rgb_quant_range_selectable = false;
 
 	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
+	intel_hdmi->dp_dual_mode.tmds_output_control = false;
 
 	kfree(to_intel_connector(connector)->detect_edid);
 	to_intel_connector(connector)->detect_edid = NULL;
@@ -1392,15 +1407,23 @@  intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector)
 	 */
 	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
 	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
+		bool tmds_enabled;
+
 		hdmi->dp_dual_mode.max_tmds_clock =
 			drm_dp_dual_mode_max_tmds_clock(adapter);
+
+		hdmi->dp_dual_mode.tmds_output_control =
+			drm_dp_dual_mode_get_tmds_output(adapter, &tmds_enabled) == 0 &&
+			drm_dp_dual_mode_set_tmds_output(adapter, tmds_enabled) == 0;
 	} else {
 		hdmi->dp_dual_mode.max_tmds_clock = 165000;
+		hdmi->dp_dual_mode.tmds_output_control = false;
 	}
 
-	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz)\n",
+	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS clock: %d kHz, TMDS OE# control: %s)\n",
 		      drm_dp_get_dual_mode_type_name(type),
-		      hdmi->dp_dual_mode.max_tmds_clock);
+		      hdmi->dp_dual_mode.max_tmds_clock,
+		      yesno(hdmi->dp_dual_mode.tmds_output_control));
 }
 
 static bool

Comments

Em Qui, 2016-02-25 às 14:51 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 

> To save a bit of power, let's try to turn off the TMDS output buffers

> in DP++ adaptors when we're not driving the port.

> 

> v2: Let's not forget DDI, toss in a debug message while at it

> 

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---

>  drivers/gpu/drm/i915/intel_ddi.c  | 12 ++++++++++++

>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++

>  drivers/gpu/drm/i915/intel_hdmi.c | 31

> +++++++++++++++++++++++++++++--

>  3 files changed, 43 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/intel_ddi.c

> b/drivers/gpu/drm/i915/intel_ddi.c

> index 21a9b83f3bfc..458c41788b80 100644

> --- a/drivers/gpu/drm/i915/intel_ddi.c

> +++ b/drivers/gpu/drm/i915/intel_ddi.c

> @@ -2301,6 +2301,12 @@ static void intel_ddi_pre_enable(struct

> intel_encoder *intel_encoder)

>  	enum port port = intel_ddi_get_encoder_port(intel_encoder);

>  	int type = intel_encoder->type;

>  

> +	if (type == INTEL_OUTPUT_HDMI) {

> +		struct intel_hdmi *intel_hdmi =

> enc_to_intel_hdmi(encoder);

> +

> +		intel_dp_dual_mode_set_tmds_output(intel_hdmi,

> true);

> +	}

> +

>  	intel_prepare_ddi_buffer(intel_encoder);

>  

>  	if (type == INTEL_OUTPUT_EDP) {

> @@ -2367,6 +2373,12 @@ static void intel_ddi_post_disable(struct

> intel_encoder *intel_encoder)

>  					DPLL_CTRL2_DDI_CLK_OFF(port)

> ));

>  	else if (INTEL_INFO(dev)->gen < 9)

>  		I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);

> +

> +	if (type == INTEL_OUTPUT_HDMI) {

> +		struct intel_hdmi *intel_hdmi =

> enc_to_intel_hdmi(encoder);

> +

> +		intel_dp_dual_mode_set_tmds_output(intel_hdmi,

> false);

> +	}

>  }

>  

>  static void intel_enable_ddi(struct intel_encoder *intel_encoder)

> diff --git a/drivers/gpu/drm/i915/intel_drv.h

> b/drivers/gpu/drm/i915/intel_drv.h

> index 944eacbfb6a9..2e4fa4337c59 100644

> --- a/drivers/gpu/drm/i915/intel_drv.h

> +++ b/drivers/gpu/drm/i915/intel_drv.h

> @@ -706,6 +706,7 @@ struct intel_hdmi {

>  	int ddc_bus;

>  	struct {

>  		int max_tmds_clock;

> +		bool tmds_output_control;

>  	} dp_dual_mode;

>  	bool limited_color_range;

>  	bool color_range_auto;

> @@ -1355,6 +1356,7 @@ void intel_hdmi_init_connector(struct

> intel_digital_port *intel_dig_port,

>  struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);

>  bool intel_hdmi_compute_config(struct intel_encoder *encoder,

>  			       struct intel_crtc_state

> *pipe_config);

> +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi,

> bool enable);

>  

>  

>  /* intel_lvds.c */

> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c

> b/drivers/gpu/drm/i915/intel_hdmi.c

> index 1e8cfdb18dc7..4b528a669f8e 100644

> --- a/drivers/gpu/drm/i915/intel_hdmi.c

> +++ b/drivers/gpu/drm/i915/intel_hdmi.c

> @@ -837,6 +837,20 @@ static void hsw_set_infoframes(struct

> drm_encoder *encoder,

>  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);

>  }

>  

> +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi,

> bool enable)

> +{

> +	if (hdmi->dp_dual_mode.tmds_output_control) {


Or you could just do an early return here and save an indentation level
:)

> +		struct drm_i915_private *dev_priv =

> to_i915(intel_hdmi_to_dev(hdmi));

> +		struct i2c_adapter *adapter =

> +			intel_gmbus_get_adapter(dev_priv, hdmi-

> >ddc_bus);

> +

> +		DRM_DEBUG_KMS("%s DP dual mode adaptor TMDS

> output\n",

> +			      enable ? "Enabling" : "Disabling");

> +

> +		drm_dp_dual_mode_set_tmds_output(adapter, enable);

> +	}

> +}

> +

>  static void intel_hdmi_prepare(struct intel_encoder *encoder)

>  {

>  	struct drm_device *dev = encoder->base.dev;

> @@ -846,6 +860,8 @@ static void intel_hdmi_prepare(struct

> intel_encoder *encoder)

>  	const struct drm_display_mode *adjusted_mode = &crtc-

> >config->base.adjusted_mode;

>  	u32 hdmi_val;

>  

> +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);

> +

>  	hdmi_val = SDVO_ENCODING_HDMI;

>  	if (!HAS_PCH_SPLIT(dev) && crtc->config-

> >limited_color_range)

>  		hdmi_val |= HDMI_COLOR_RANGE_16_235;

> @@ -1144,6 +1160,8 @@ static void intel_disable_hdmi(struct

> intel_encoder *encoder)

>  	}

>  

>  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);

> +

> +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);

>  }

>  

>  static void g4x_disable_hdmi(struct intel_encoder *encoder)

> @@ -1369,6 +1387,7 @@ intel_hdmi_unset_edid(struct drm_connector

> *connector)

>  	intel_hdmi->rgb_quant_range_selectable = false;

>  

>  	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;

> +	intel_hdmi->dp_dual_mode.tmds_output_control = false;

>  

>  	kfree(to_intel_connector(connector)->detect_edid);

>  	to_intel_connector(connector)->detect_edid = NULL;

> @@ -1392,15 +1411,23 @@ intel_hdmi_dp_dual_mode_detect(struct

> drm_connector *connector)

>  	 */

>  	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||

>  	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {

> +		bool tmds_enabled;

> +

>  		hdmi->dp_dual_mode.max_tmds_clock =

>  			drm_dp_dual_mode_max_tmds_clock(adapter);

> +

> +		hdmi->dp_dual_mode.tmds_output_control =

> +			drm_dp_dual_mode_get_tmds_output(adapter,

> &tmds_enabled) == 0 &&

> +			drm_dp_dual_mode_set_tmds_output(adapter,

> tmds_enabled) == 0;

>  	} else {

>  		hdmi->dp_dual_mode.max_tmds_clock = 165000;

> +		hdmi->dp_dual_mode.tmds_output_control = false;


While type 1 adaptors are not required to implement the register, what
if they do? We could potentially keep TMDS disabled forever. Maybe my
suggestions on p1 would help a little bit here?

Anyway, what we have here is already better than the previous state, so
if no rebase is required:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


>  	}

>  

> -	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS

> clock: %d kHz)\n",

> +	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS

> clock: %d kHz, TMDS OE# control: %s)\n",

>  		      drm_dp_get_dual_mode_type_name(type),

> -		      hdmi->dp_dual_mode.max_tmds_clock);

> +		      hdmi->dp_dual_mode.max_tmds_clock,

> +		      yesno(hdmi-

> >dp_dual_mode.tmds_output_control));

>  }

>  

>  static bool
On Thu, Mar 31, 2016 at 07:26:25PM +0000, Zanoni, Paulo R wrote:
> Em Qui, 2016-02-25 às 14:51 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > To save a bit of power, let's try to turn off the TMDS output buffers
> > in DP++ adaptors when we're not driving the port.
> > 
> > v2: Let's not forget DDI, toss in a debug message while at it
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c  | 12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
> >  drivers/gpu/drm/i915/intel_hdmi.c | 31
> > +++++++++++++++++++++++++++++--
> >  3 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 21a9b83f3bfc..458c41788b80 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2301,6 +2301,12 @@ static void intel_ddi_pre_enable(struct
> > intel_encoder *intel_encoder)
> >  	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> >  	int type = intel_encoder->type;
> >  
> > +	if (type == INTEL_OUTPUT_HDMI) {
> > +		struct intel_hdmi *intel_hdmi =
> > enc_to_intel_hdmi(encoder);
> > +
> > +		intel_dp_dual_mode_set_tmds_output(intel_hdmi,
> > true);
> > +	}
> > +
> >  	intel_prepare_ddi_buffer(intel_encoder);
> >  
> >  	if (type == INTEL_OUTPUT_EDP) {
> > @@ -2367,6 +2373,12 @@ static void intel_ddi_post_disable(struct
> > intel_encoder *intel_encoder)
> >  					DPLL_CTRL2_DDI_CLK_OFF(port)
> > ));
> >  	else if (INTEL_INFO(dev)->gen < 9)
> >  		I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);
> > +
> > +	if (type == INTEL_OUTPUT_HDMI) {
> > +		struct intel_hdmi *intel_hdmi =
> > enc_to_intel_hdmi(encoder);
> > +
> > +		intel_dp_dual_mode_set_tmds_output(intel_hdmi,
> > false);
> > +	}
> >  }
> >  
> >  static void intel_enable_ddi(struct intel_encoder *intel_encoder)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 944eacbfb6a9..2e4fa4337c59 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -706,6 +706,7 @@ struct intel_hdmi {
> >  	int ddc_bus;
> >  	struct {
> >  		int max_tmds_clock;
> > +		bool tmds_output_control;
> >  	} dp_dual_mode;
> >  	bool limited_color_range;
> >  	bool color_range_auto;
> > @@ -1355,6 +1356,7 @@ void intel_hdmi_init_connector(struct
> > intel_digital_port *intel_dig_port,
> >  struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
> >  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >  			       struct intel_crtc_state
> > *pipe_config);
> > +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi,
> > bool enable);
> >  
> >  
> >  /* intel_lvds.c */
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 1e8cfdb18dc7..4b528a669f8e 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -837,6 +837,20 @@ static void hsw_set_infoframes(struct
> > drm_encoder *encoder,
> >  	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
> >  }
> >  
> > +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi,
> > bool enable)
> > +{
> > +	if (hdmi->dp_dual_mode.tmds_output_control) {
> 
> Or you could just do an early return here and save an indentation level
> :)

Indeed. I had the code inlined originally so the extra indentation
made sense, and the I just copypasted it to this separate function,
and in my haste forgot to flatten it.

> 
> > +		struct drm_i915_private *dev_priv =
> > to_i915(intel_hdmi_to_dev(hdmi));
> > +		struct i2c_adapter *adapter =
> > +			intel_gmbus_get_adapter(dev_priv, hdmi-
> > >ddc_bus);
> > +
> > +		DRM_DEBUG_KMS("%s DP dual mode adaptor TMDS
> > output\n",
> > +			      enable ? "Enabling" : "Disabling");
> > +
> > +		drm_dp_dual_mode_set_tmds_output(adapter, enable);
> > +	}
> > +}
> > +
> >  static void intel_hdmi_prepare(struct intel_encoder *encoder)
> >  {
> >  	struct drm_device *dev = encoder->base.dev;
> > @@ -846,6 +860,8 @@ static void intel_hdmi_prepare(struct
> > intel_encoder *encoder)
> >  	const struct drm_display_mode *adjusted_mode = &crtc-
> > >config->base.adjusted_mode;
> >  	u32 hdmi_val;
> >  
> > +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
> > +
> >  	hdmi_val = SDVO_ENCODING_HDMI;
> >  	if (!HAS_PCH_SPLIT(dev) && crtc->config-
> > >limited_color_range)
> >  		hdmi_val |= HDMI_COLOR_RANGE_16_235;
> > @@ -1144,6 +1160,8 @@ static void intel_disable_hdmi(struct
> > intel_encoder *encoder)
> >  	}
> >  
> >  	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
> > +
> > +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
> >  }
> >  
> >  static void g4x_disable_hdmi(struct intel_encoder *encoder)
> > @@ -1369,6 +1387,7 @@ intel_hdmi_unset_edid(struct drm_connector
> > *connector)
> >  	intel_hdmi->rgb_quant_range_selectable = false;
> >  
> >  	intel_hdmi->dp_dual_mode.max_tmds_clock = 0;
> > +	intel_hdmi->dp_dual_mode.tmds_output_control = false;
> >  
> >  	kfree(to_intel_connector(connector)->detect_edid);
> >  	to_intel_connector(connector)->detect_edid = NULL;
> > @@ -1392,15 +1411,23 @@ intel_hdmi_dp_dual_mode_detect(struct
> > drm_connector *connector)
> >  	 */
> >  	if (type == DRM_DP_DUAL_MODE_TYPE2_DVI ||
> >  	    type == DRM_DP_DUAL_MODE_TYPE2_HDMI) {
> > +		bool tmds_enabled;
> > +
> >  		hdmi->dp_dual_mode.max_tmds_clock =
> >  			drm_dp_dual_mode_max_tmds_clock(adapter);
> > +
> > +		hdmi->dp_dual_mode.tmds_output_control =
> > +			drm_dp_dual_mode_get_tmds_output(adapter,
> > &tmds_enabled) == 0 &&
> > +			drm_dp_dual_mode_set_tmds_output(adapter,
> > tmds_enabled) == 0;
> >  	} else {
> >  		hdmi->dp_dual_mode.max_tmds_clock = 165000;
> > +		hdmi->dp_dual_mode.tmds_output_control = false;
> 
> While type 1 adaptors are not required to implement the register, what
> if they do?

I suppose we could just always do the i2c write and if the adaptor
supports it everything is peachy, and if it doesn't, well, then I
suppose the write just goes to /dev/null and nothing should happen.

> We could potentially keep TMDS disabled forever.

That would be an entirely broken adaptor since it would not work at all
with source devices that are not dual mode aware.

> Maybe my
> suggestions on p1 would help a little bit here?
> 
> Anyway, what we have here is already better than the previous state, so
> if no rebase is required:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >  	}
> >  
> > -	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS
> > clock: %d kHz)\n",
> > +	DRM_DEBUG_KMS("DP dual mode adaptor (%s) detected (max TMDS
> > clock: %d kHz, TMDS OE# control: %s)\n",
> >  		      drm_dp_get_dual_mode_type_name(type),
> > -		      hdmi->dp_dual_mode.max_tmds_clock);
> > +		      hdmi->dp_dual_mode.max_tmds_clock,
> > +		      yesno(hdmi-
> > >dp_dual_mode.tmds_output_control));
> >  }
> >  
> >  static bool