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 at linux.intel.com > escreveu: > > From: Ville Syrjälä <ville.syrjala at 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 at 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 at 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 -- Ville Syrjälä Intel OTC