| Message ID | 20171010121207.570-10-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 10 Oct 2017, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Untangle intel_enable_ddi() by splitting it into DP and HDMI specific > variants. > > v2: Keep using intel_ddi_get_encoder_port() for now > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 86 +++++++++++++++++++++++----------------- > 1 file changed, 49 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 49cf8d9d2bc1..18bf06c7e43f 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2369,45 +2369,57 @@ void intel_ddi_fdi_post_disable(struct intel_encoder *encoder, > I915_WRITE(FDI_RX_CTL(PIPE_A), val); > } > > -static void intel_enable_ddi(struct intel_encoder *intel_encoder, > - const struct intel_crtc_state *pipe_config, > +static void intel_enable_ddi_dp(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + const struct drm_connector_state *conn_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > + enum port port = intel_ddi_get_encoder_port(encoder); > + > + if (port == PORT_A && INTEL_GEN(dev_priv) < 9) > + intel_dp_stop_link_train(intel_dp); > + > + intel_edp_backlight_on(crtc_state, conn_state); > + intel_psr_enable(intel_dp, crtc_state); This is broken without edp check. BR, Jani. > + intel_edp_drrs_enable(intel_dp, crtc_state); > + > + if (crtc_state->has_audio) > + intel_audio_codec_enable(encoder, crtc_state, conn_state); > +} > + > +static void intel_enable_ddi_hdmi(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > + const struct drm_connector_state *conn_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > + enum port port = intel_ddi_get_encoder_port(encoder); > + > + intel_hdmi_handle_sink_scrambling(encoder, > + conn_state->connector, > + crtc_state->hdmi_high_tmds_clock_ratio, > + crtc_state->hdmi_scrambling); > + > + /* In HDMI/DVI mode, the port width, and swing/emphasis values > + * are ignored so nothing special needs to be done besides > + * enabling the port. > + */ > + I915_WRITE(DDI_BUF_CTL(port), > + dig_port->saved_port_bits | DDI_BUF_CTL_ENABLE); > + > + if (crtc_state->has_audio) > + intel_audio_codec_enable(encoder, crtc_state, conn_state); > +} > + > +static void intel_enable_ddi(struct intel_encoder *encoder, > + const struct intel_crtc_state *crtc_state, > const struct drm_connector_state *conn_state) > { > - struct drm_encoder *encoder = &intel_encoder->base; > - struct drm_i915_private *dev_priv = to_i915(encoder->dev); > - enum port port = intel_ddi_get_encoder_port(intel_encoder); > - int type = intel_encoder->type; > - > - if (type == INTEL_OUTPUT_HDMI) { > - struct intel_digital_port *intel_dig_port = > - enc_to_dig_port(encoder); > - bool clock_ratio = pipe_config->hdmi_high_tmds_clock_ratio; > - bool scrambling = pipe_config->hdmi_scrambling; > - > - intel_hdmi_handle_sink_scrambling(intel_encoder, > - conn_state->connector, > - clock_ratio, scrambling); > - > - /* In HDMI/DVI mode, the port width, and swing/emphasis values > - * are ignored so nothing special needs to be done besides > - * enabling the port. > - */ > - I915_WRITE(DDI_BUF_CTL(port), > - intel_dig_port->saved_port_bits | > - DDI_BUF_CTL_ENABLE); > - } else if (type == INTEL_OUTPUT_EDP) { > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > - > - if (port == PORT_A && INTEL_GEN(dev_priv) < 9) > - intel_dp_stop_link_train(intel_dp); > - > - intel_edp_backlight_on(pipe_config, conn_state); > - intel_psr_enable(intel_dp, pipe_config); > - intel_edp_drrs_enable(intel_dp, pipe_config); > - } > - > - if (pipe_config->has_audio) > - intel_audio_codec_enable(intel_encoder, pipe_config, conn_state); > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) > + intel_enable_ddi_hdmi(encoder, crtc_state, conn_state); > + else > + intel_enable_ddi_dp(encoder, crtc_state, conn_state); > } > > static void intel_disable_ddi_dp(struct intel_encoder *encoder,
On Wed, 11 Oct 2017, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 10 Oct 2017, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Untangle intel_enable_ddi() by splitting it into DP and HDMI specific >> variants. >> >> v2: Keep using intel_ddi_get_encoder_port() for now >> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 86 +++++++++++++++++++++++----------------- >> 1 file changed, 49 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 49cf8d9d2bc1..18bf06c7e43f 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -2369,45 +2369,57 @@ void intel_ddi_fdi_post_disable(struct intel_encoder *encoder, >> I915_WRITE(FDI_RX_CTL(PIPE_A), val); >> } >> >> -static void intel_enable_ddi(struct intel_encoder *intel_encoder, >> - const struct intel_crtc_state *pipe_config, >> +static void intel_enable_ddi_dp(struct intel_encoder *encoder, >> + const struct intel_crtc_state *crtc_state, >> + const struct drm_connector_state *conn_state) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> + enum port port = intel_ddi_get_encoder_port(encoder); >> + >> + if (port == PORT_A && INTEL_GEN(dev_priv) < 9) >> + intel_dp_stop_link_train(intel_dp); I note that this was only run on type == INTEL_OUTPUT_EDP previously. I suppose the condition does effectively imply edp, so this is probably fine. It's just that sometimes I wonder about the amount of background knowledge like this that we impose on the reader. >> + >> + intel_edp_backlight_on(crtc_state, conn_state); >> + intel_psr_enable(intel_dp, crtc_state); > > This is broken without edp check. And fine now with 4d90f2d507ab ("drm/i915: Start tracking PSR state in crtc state") merged. Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > BR, Jani. > > >> + intel_edp_drrs_enable(intel_dp, crtc_state); >> + >> + if (crtc_state->has_audio) >> + intel_audio_codec_enable(encoder, crtc_state, conn_state); >> +} >> + >> +static void intel_enable_ddi_hdmi(struct intel_encoder *encoder, >> + const struct intel_crtc_state *crtc_state, >> + const struct drm_connector_state *conn_state) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); >> + enum port port = intel_ddi_get_encoder_port(encoder); >> + >> + intel_hdmi_handle_sink_scrambling(encoder, >> + conn_state->connector, >> + crtc_state->hdmi_high_tmds_clock_ratio, >> + crtc_state->hdmi_scrambling); >> + >> + /* In HDMI/DVI mode, the port width, and swing/emphasis values >> + * are ignored so nothing special needs to be done besides >> + * enabling the port. >> + */ >> + I915_WRITE(DDI_BUF_CTL(port), >> + dig_port->saved_port_bits | DDI_BUF_CTL_ENABLE); >> + >> + if (crtc_state->has_audio) >> + intel_audio_codec_enable(encoder, crtc_state, conn_state); >> +} >> + >> +static void intel_enable_ddi(struct intel_encoder *encoder, >> + const struct intel_crtc_state *crtc_state, >> const struct drm_connector_state *conn_state) >> { >> - struct drm_encoder *encoder = &intel_encoder->base; >> - struct drm_i915_private *dev_priv = to_i915(encoder->dev); >> - enum port port = intel_ddi_get_encoder_port(intel_encoder); >> - int type = intel_encoder->type; >> - >> - if (type == INTEL_OUTPUT_HDMI) { >> - struct intel_digital_port *intel_dig_port = >> - enc_to_dig_port(encoder); >> - bool clock_ratio = pipe_config->hdmi_high_tmds_clock_ratio; >> - bool scrambling = pipe_config->hdmi_scrambling; >> - >> - intel_hdmi_handle_sink_scrambling(intel_encoder, >> - conn_state->connector, >> - clock_ratio, scrambling); >> - >> - /* In HDMI/DVI mode, the port width, and swing/emphasis values >> - * are ignored so nothing special needs to be done besides >> - * enabling the port. >> - */ >> - I915_WRITE(DDI_BUF_CTL(port), >> - intel_dig_port->saved_port_bits | >> - DDI_BUF_CTL_ENABLE); >> - } else if (type == INTEL_OUTPUT_EDP) { >> - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> - >> - if (port == PORT_A && INTEL_GEN(dev_priv) < 9) >> - intel_dp_stop_link_train(intel_dp); >> - >> - intel_edp_backlight_on(pipe_config, conn_state); >> - intel_psr_enable(intel_dp, pipe_config); >> - intel_edp_drrs_enable(intel_dp, pipe_config); >> - } >> - >> - if (pipe_config->has_audio) >> - intel_audio_codec_enable(intel_encoder, pipe_config, conn_state); >> + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) >> + intel_enable_ddi_hdmi(encoder, crtc_state, conn_state); >> + else >> + intel_enable_ddi_dp(encoder, crtc_state, conn_state); >> } >> >> static void intel_disable_ddi_dp(struct intel_encoder *encoder,
On Fri, Oct 13, 2017 at 05:20:47PM +0300, Jani Nikula wrote: > On Wed, 11 Oct 2017, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Tue, 10 Oct 2017, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > >> Untangle intel_enable_ddi() by splitting it into DP and HDMI specific > >> variants. > >> > >> v2: Keep using intel_ddi_get_encoder_port() for now > >> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_ddi.c | 86 +++++++++++++++++++++++----------------- > >> 1 file changed, 49 insertions(+), 37 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > >> index 49cf8d9d2bc1..18bf06c7e43f 100644 > >> --- a/drivers/gpu/drm/i915/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/intel_ddi.c > >> @@ -2369,45 +2369,57 @@ void intel_ddi_fdi_post_disable(struct intel_encoder *encoder, > >> I915_WRITE(FDI_RX_CTL(PIPE_A), val); > >> } > >> > >> -static void intel_enable_ddi(struct intel_encoder *intel_encoder, > >> - const struct intel_crtc_state *pipe_config, > >> +static void intel_enable_ddi_dp(struct intel_encoder *encoder, > >> + const struct intel_crtc_state *crtc_state, > >> + const struct drm_connector_state *conn_state) > >> +{ > >> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > >> + enum port port = intel_ddi_get_encoder_port(encoder); > >> + > >> + if (port == PORT_A && INTEL_GEN(dev_priv) < 9) > >> + intel_dp_stop_link_train(intel_dp); > > I note that this was only run on type == INTEL_OUTPUT_EDP previously. I > suppose the condition does effectively imply edp, so this is probably > fine. The spec is actually a little vague which one it means. The wording used is "If eDP (DDI A), set DP_TP_CTL link training to Normal". So it's not clear if this change in behaviour is required because the sink is eDP, because the eDP transcoder is used, or because we're dealing with DDI A. Since DDI A is mentioned I'm going to assume that's what it means ;) > It's just that sometimes I wonder about the amount of background > knowledge like this that we impose on the reader. > > >> + > >> + intel_edp_backlight_on(crtc_state, conn_state); > >> + intel_psr_enable(intel_dp, crtc_state); > > > > This is broken without edp check. > > And fine now with 4d90f2d507ab ("drm/i915: Start tracking PSR state in > crtc state") merged. > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > > > > > BR, Jani. > > > > > >> + intel_edp_drrs_enable(intel_dp, crtc_state); > >> + > >> + if (crtc_state->has_audio) > >> + intel_audio_codec_enable(encoder, crtc_state, conn_state); > >> +} > >> + > >> +static void intel_enable_ddi_hdmi(struct intel_encoder *encoder, > >> + const struct intel_crtc_state *crtc_state, > >> + const struct drm_connector_state *conn_state) > >> +{ > >> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > >> + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > >> + enum port port = intel_ddi_get_encoder_port(encoder); > >> + > >> + intel_hdmi_handle_sink_scrambling(encoder, > >> + conn_state->connector, > >> + crtc_state->hdmi_high_tmds_clock_ratio, > >> + crtc_state->hdmi_scrambling); > >> + > >> + /* In HDMI/DVI mode, the port width, and swing/emphasis values > >> + * are ignored so nothing special needs to be done besides > >> + * enabling the port. > >> + */ > >> + I915_WRITE(DDI_BUF_CTL(port), > >> + dig_port->saved_port_bits | DDI_BUF_CTL_ENABLE); > >> + > >> + if (crtc_state->has_audio) > >> + intel_audio_codec_enable(encoder, crtc_state, conn_state); > >> +} > >> + > >> +static void intel_enable_ddi(struct intel_encoder *encoder, > >> + const struct intel_crtc_state *crtc_state, > >> const struct drm_connector_state *conn_state) > >> { > >> - struct drm_encoder *encoder = &intel_encoder->base; > >> - struct drm_i915_private *dev_priv = to_i915(encoder->dev); > >> - enum port port = intel_ddi_get_encoder_port(intel_encoder); > >> - int type = intel_encoder->type; > >> - > >> - if (type == INTEL_OUTPUT_HDMI) { > >> - struct intel_digital_port *intel_dig_port = > >> - enc_to_dig_port(encoder); > >> - bool clock_ratio = pipe_config->hdmi_high_tmds_clock_ratio; > >> - bool scrambling = pipe_config->hdmi_scrambling; > >> - > >> - intel_hdmi_handle_sink_scrambling(intel_encoder, > >> - conn_state->connector, > >> - clock_ratio, scrambling); > >> - > >> - /* In HDMI/DVI mode, the port width, and swing/emphasis values > >> - * are ignored so nothing special needs to be done besides > >> - * enabling the port. > >> - */ > >> - I915_WRITE(DDI_BUF_CTL(port), > >> - intel_dig_port->saved_port_bits | > >> - DDI_BUF_CTL_ENABLE); > >> - } else if (type == INTEL_OUTPUT_EDP) { > >> - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > >> - > >> - if (port == PORT_A && INTEL_GEN(dev_priv) < 9) > >> - intel_dp_stop_link_train(intel_dp); > >> - > >> - intel_edp_backlight_on(pipe_config, conn_state); > >> - intel_psr_enable(intel_dp, pipe_config); > >> - intel_edp_drrs_enable(intel_dp, pipe_config); > >> - } > >> - > >> - if (pipe_config->has_audio) > >> - intel_audio_codec_enable(intel_encoder, pipe_config, conn_state); > >> + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) > >> + intel_enable_ddi_hdmi(encoder, crtc_state, conn_state); > >> + else > >> + intel_enable_ddi_dp(encoder, crtc_state, conn_state); > >> } > >> > >> static void intel_disable_ddi_dp(struct intel_encoder *encoder, > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 49cf8d9d2bc1..18bf06c7e43f 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2369,45 +2369,57 @@ void intel_ddi_fdi_post_disable(struct intel_encoder *encoder, I915_WRITE(FDI_RX_CTL(PIPE_A), val); } -static void intel_enable_ddi(struct intel_encoder *intel_encoder, - const struct intel_crtc_state *pipe_config, +static void intel_enable_ddi_dp(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, + const struct drm_connector_state *conn_state) +{ + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); + enum port port = intel_ddi_get_encoder_port(encoder); + + if (port == PORT_A && INTEL_GEN(dev_priv) < 9) + intel_dp_stop_link_train(intel_dp); + + intel_edp_backlight_on(crtc_state, conn_state); + intel_psr_enable(intel_dp, crtc_state); + intel_edp_drrs_enable(intel_dp, crtc_state); + + if (crtc_state->has_audio) + intel_audio_codec_enable(encoder, crtc_state, conn_state); +} + +static void intel_enable_ddi_hdmi(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, + const struct drm_connector_state *conn_state) +{ + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); + enum port port = intel_ddi_get_encoder_port(encoder); + + intel_hdmi_handle_sink_scrambling(encoder, + conn_state->connector, + crtc_state->hdmi_high_tmds_clock_ratio, + crtc_state->hdmi_scrambling); + + /* In HDMI/DVI mode, the port width, and swing/emphasis values + * are ignored so nothing special needs to be done besides + * enabling the port. + */ + I915_WRITE(DDI_BUF_CTL(port), + dig_port->saved_port_bits | DDI_BUF_CTL_ENABLE); + + if (crtc_state->has_audio) + intel_audio_codec_enable(encoder, crtc_state, conn_state); +} + +static void intel_enable_ddi(struct intel_encoder *encoder, + const struct intel_crtc_state *crtc_state, const struct drm_connector_state *conn_state) { - struct drm_encoder *encoder = &intel_encoder->base; - struct drm_i915_private *dev_priv = to_i915(encoder->dev); - enum port port = intel_ddi_get_encoder_port(intel_encoder); - int type = intel_encoder->type; - - if (type == INTEL_OUTPUT_HDMI) { - struct intel_digital_port *intel_dig_port = - enc_to_dig_port(encoder); - bool clock_ratio = pipe_config->hdmi_high_tmds_clock_ratio; - bool scrambling = pipe_config->hdmi_scrambling; - - intel_hdmi_handle_sink_scrambling(intel_encoder, - conn_state->connector, - clock_ratio, scrambling); - - /* In HDMI/DVI mode, the port width, and swing/emphasis values - * are ignored so nothing special needs to be done besides - * enabling the port. - */ - I915_WRITE(DDI_BUF_CTL(port), - intel_dig_port->saved_port_bits | - DDI_BUF_CTL_ENABLE); - } else if (type == INTEL_OUTPUT_EDP) { - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); - - if (port == PORT_A && INTEL_GEN(dev_priv) < 9) - intel_dp_stop_link_train(intel_dp); - - intel_edp_backlight_on(pipe_config, conn_state); - intel_psr_enable(intel_dp, pipe_config); - intel_edp_drrs_enable(intel_dp, pipe_config); - } - - if (pipe_config->has_audio) - intel_audio_codec_enable(intel_encoder, pipe_config, conn_state); + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) + intel_enable_ddi_hdmi(encoder, crtc_state, conn_state); + else + intel_enable_ddi_dp(encoder, crtc_state, conn_state); } static void intel_disable_ddi_dp(struct intel_encoder *encoder,