| Message ID | 20171010121207.570-8-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_disable_ddi() by splitting it into DP and HDMI specific > variants. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 51 ++++++++++++++++++++++++---------------- > 1 file changed, 31 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 945980e71697..c930ef0338c5 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2424,29 +2424,40 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder, > intel_audio_codec_enable(intel_encoder, pipe_config, conn_state); > } > > -static void intel_disable_ddi(struct intel_encoder *intel_encoder, > +static void intel_disable_ddi_dp(struct intel_encoder *encoder, > + const struct intel_crtc_state *old_crtc_state, > + const struct drm_connector_state *old_conn_state) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > + > + if (old_crtc_state->has_audio) > + intel_audio_codec_disable(encoder); > + You remove the eDP check here, and... > + intel_edp_drrs_disable(intel_dp, old_crtc_state); ...this will now depend on old_crtc_state->has_drrs which is probably fine, but... > + intel_psr_disable(intel_dp, old_crtc_state); ...but this will be wrong. I don't mind adding the edp check within. > + intel_edp_backlight_off(old_conn_state); This is ok. I guess easiest would be to keep the type == INTEL_OUTPUT_EDP check and clean up afterwards. Otherwise LGTM. BR, Jani. > +} > + > +static void intel_disable_ddi_hdmi(struct intel_encoder *encoder, > + const struct intel_crtc_state *old_crtc_state, > + const struct drm_connector_state *old_conn_state) > +{ > + if (old_crtc_state->has_audio) > + intel_audio_codec_disable(encoder); > + > + intel_hdmi_handle_sink_scrambling(encoder, > + old_conn_state->connector, > + false, false); > +} > + > +static void intel_disable_ddi(struct intel_encoder *encoder, > const struct intel_crtc_state *old_crtc_state, > const struct drm_connector_state *old_conn_state) > { > - struct drm_encoder *encoder = &intel_encoder->base; > - int type = intel_encoder->type; > - > - if (old_crtc_state->has_audio) > - intel_audio_codec_disable(intel_encoder); > - > - if (type == INTEL_OUTPUT_HDMI) { > - intel_hdmi_handle_sink_scrambling(intel_encoder, > - old_conn_state->connector, > - false, false); > - } > - > - if (type == INTEL_OUTPUT_EDP) { > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > - > - intel_edp_drrs_disable(intel_dp, old_crtc_state); > - intel_psr_disable(intel_dp, old_crtc_state); > - intel_edp_backlight_off(old_conn_state); > - } > + if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI)) > + intel_disable_ddi_hdmi(encoder, old_crtc_state, old_conn_state); > + else > + intel_disable_ddi_dp(encoder, old_crtc_state, old_conn_state); > } > > static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder,
On Wed, Oct 11, 2017 at 11:03:18PM +0300, Jani Nikula 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_disable_ddi() by splitting it into DP and HDMI specific > > variants. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 51 ++++++++++++++++++++++++---------------- > > 1 file changed, 31 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 945980e71697..c930ef0338c5 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2424,29 +2424,40 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder, > > intel_audio_codec_enable(intel_encoder, pipe_config, conn_state); > > } > > > > -static void intel_disable_ddi(struct intel_encoder *intel_encoder, > > +static void intel_disable_ddi_dp(struct intel_encoder *encoder, > > + const struct intel_crtc_state *old_crtc_state, > > + const struct drm_connector_state *old_conn_state) > > +{ > > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > + > > + if (old_crtc_state->has_audio) > > + intel_audio_codec_disable(encoder); > > + > > You remove the eDP check here, and... > > > + intel_edp_drrs_disable(intel_dp, old_crtc_state); > > ...this will now depend on old_crtc_state->has_drrs which is probably > fine, but... > > > + intel_psr_disable(intel_dp, old_crtc_state); > > ...but this will be wrong. I don't mind adding the edp check within. Argh. Yeah, looks like we're still tracking PSR globally for whatever reason :( Let me fix that right up... > > > + intel_edp_backlight_off(old_conn_state); > > This is ok. > > I guess easiest would be to keep the type == INTEL_OUTPUT_EDP check and > clean up afterwards. > > Otherwise LGTM. > > BR, > Jani. > > > > > +} > > + > > +static void intel_disable_ddi_hdmi(struct intel_encoder *encoder, > > + const struct intel_crtc_state *old_crtc_state, > > + const struct drm_connector_state *old_conn_state) > > +{ > > + if (old_crtc_state->has_audio) > > + intel_audio_codec_disable(encoder); > > + > > + intel_hdmi_handle_sink_scrambling(encoder, > > + old_conn_state->connector, > > + false, false); > > +} > > + > > +static void intel_disable_ddi(struct intel_encoder *encoder, > > const struct intel_crtc_state *old_crtc_state, > > const struct drm_connector_state *old_conn_state) > > { > > - struct drm_encoder *encoder = &intel_encoder->base; > > - int type = intel_encoder->type; > > - > > - if (old_crtc_state->has_audio) > > - intel_audio_codec_disable(intel_encoder); > > - > > - if (type == INTEL_OUTPUT_HDMI) { > > - intel_hdmi_handle_sink_scrambling(intel_encoder, > > - old_conn_state->connector, > > - false, false); > > - } > > - > > - if (type == INTEL_OUTPUT_EDP) { > > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > - > > - intel_edp_drrs_disable(intel_dp, old_crtc_state); > > - intel_psr_disable(intel_dp, old_crtc_state); > > - intel_edp_backlight_off(old_conn_state); > > - } > > + if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI)) > > + intel_disable_ddi_hdmi(encoder, old_crtc_state, old_conn_state); > > + else > > + intel_disable_ddi_dp(encoder, old_crtc_state, old_conn_state); > > } > > > > static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder, > > -- > Jani Nikula, Intel Open Source Technology Center
On Thu, 12 Oct 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Oct 11, 2017 at 11:03:18PM +0300, Jani Nikula 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_disable_ddi() by splitting it into DP and HDMI specific >> > variants. >> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_ddi.c | 51 ++++++++++++++++++++++++---------------- >> > 1 file changed, 31 insertions(+), 20 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> > index 945980e71697..c930ef0338c5 100644 >> > --- a/drivers/gpu/drm/i915/intel_ddi.c >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c >> > @@ -2424,29 +2424,40 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder, >> > intel_audio_codec_enable(intel_encoder, pipe_config, conn_state); >> > } >> > >> > -static void intel_disable_ddi(struct intel_encoder *intel_encoder, >> > +static void intel_disable_ddi_dp(struct intel_encoder *encoder, >> > + const struct intel_crtc_state *old_crtc_state, >> > + const struct drm_connector_state *old_conn_state) >> > +{ >> > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> > + >> > + if (old_crtc_state->has_audio) >> > + intel_audio_codec_disable(encoder); >> > + >> >> You remove the eDP check here, and... >> >> > + intel_edp_drrs_disable(intel_dp, old_crtc_state); >> >> ...this will now depend on old_crtc_state->has_drrs which is probably >> fine, but... >> >> > + intel_psr_disable(intel_dp, old_crtc_state); >> >> ...but this will be wrong. I don't mind adding the edp check within. > > Argh. Yeah, looks like we're still tracking PSR globally for whatever > reason :( Let me fix that right up... With 4d90f2d507ab ("drm/i915: Start tracking PSR state in crtc state") this is now Reviewed-by: Jani Nikula <jani.nikula@intel.com> but please resubmit for CI with the new base. > >> >> > + intel_edp_backlight_off(old_conn_state); >> >> This is ok. >> >> I guess easiest would be to keep the type == INTEL_OUTPUT_EDP check and >> clean up afterwards. >> >> Otherwise LGTM. >> >> BR, >> Jani. >> >> >> >> > +} >> > + >> > +static void intel_disable_ddi_hdmi(struct intel_encoder *encoder, >> > + const struct intel_crtc_state *old_crtc_state, >> > + const struct drm_connector_state *old_conn_state) >> > +{ >> > + if (old_crtc_state->has_audio) >> > + intel_audio_codec_disable(encoder); >> > + >> > + intel_hdmi_handle_sink_scrambling(encoder, >> > + old_conn_state->connector, >> > + false, false); >> > +} >> > + >> > +static void intel_disable_ddi(struct intel_encoder *encoder, >> > const struct intel_crtc_state *old_crtc_state, >> > const struct drm_connector_state *old_conn_state) >> > { >> > - struct drm_encoder *encoder = &intel_encoder->base; >> > - int type = intel_encoder->type; >> > - >> > - if (old_crtc_state->has_audio) >> > - intel_audio_codec_disable(intel_encoder); >> > - >> > - if (type == INTEL_OUTPUT_HDMI) { >> > - intel_hdmi_handle_sink_scrambling(intel_encoder, >> > - old_conn_state->connector, >> > - false, false); >> > - } >> > - >> > - if (type == INTEL_OUTPUT_EDP) { >> > - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> > - >> > - intel_edp_drrs_disable(intel_dp, old_crtc_state); >> > - intel_psr_disable(intel_dp, old_crtc_state); >> > - intel_edp_backlight_off(old_conn_state); >> > - } >> > + if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI)) >> > + intel_disable_ddi_hdmi(encoder, old_crtc_state, old_conn_state); >> > + else >> > + intel_disable_ddi_dp(encoder, old_crtc_state, old_conn_state); >> > } >> > >> > static void bxt_ddi_pre_pll_enable(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 945980e71697..c930ef0338c5 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2424,29 +2424,40 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder, intel_audio_codec_enable(intel_encoder, pipe_config, conn_state); } -static void intel_disable_ddi(struct intel_encoder *intel_encoder, +static void intel_disable_ddi_dp(struct intel_encoder *encoder, + const struct intel_crtc_state *old_crtc_state, + const struct drm_connector_state *old_conn_state) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); + + if (old_crtc_state->has_audio) + intel_audio_codec_disable(encoder); + + intel_edp_drrs_disable(intel_dp, old_crtc_state); + intel_psr_disable(intel_dp, old_crtc_state); + intel_edp_backlight_off(old_conn_state); +} + +static void intel_disable_ddi_hdmi(struct intel_encoder *encoder, + const struct intel_crtc_state *old_crtc_state, + const struct drm_connector_state *old_conn_state) +{ + if (old_crtc_state->has_audio) + intel_audio_codec_disable(encoder); + + intel_hdmi_handle_sink_scrambling(encoder, + old_conn_state->connector, + false, false); +} + +static void intel_disable_ddi(struct intel_encoder *encoder, const struct intel_crtc_state *old_crtc_state, const struct drm_connector_state *old_conn_state) { - struct drm_encoder *encoder = &intel_encoder->base; - int type = intel_encoder->type; - - if (old_crtc_state->has_audio) - intel_audio_codec_disable(intel_encoder); - - if (type == INTEL_OUTPUT_HDMI) { - intel_hdmi_handle_sink_scrambling(intel_encoder, - old_conn_state->connector, - false, false); - } - - if (type == INTEL_OUTPUT_EDP) { - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); - - intel_edp_drrs_disable(intel_dp, old_crtc_state); - intel_psr_disable(intel_dp, old_crtc_state); - intel_edp_backlight_off(old_conn_state); - } + if (intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_HDMI)) + intel_disable_ddi_hdmi(encoder, old_crtc_state, old_conn_state); + else + intel_disable_ddi_dp(encoder, old_crtc_state, old_conn_state); } static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder,