| Message ID | 1396289637-1013-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > To make sure we properly follow the enable/disable sequences. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_dp.c | 62 ++++++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_panel.c | 5 ++- > 3 files changed, 65 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index bf73771..b6f7087 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp) > return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp)); > } > > +static void assert_pwm(struct intel_connector *connector, > + bool expected_state) > +{ > + bool state; > + > + state = intel_panel_get_backlight(connector); If the duty cycle is regarded as a binary on/off, I'd rather add an additional "is enabled" call to intel_panel.c. Especially so because the duty cycle value returned by intel_panel_get_backlight is meaningless without the max value. > + > + WARN(state != expected_state, "pwm state failure, expected %d, found " > + "%d\n", expected_state, state); > +} > + > +#define assert_pwm_enabled(c) assert_pwm((c), true) > +#define assert_pwm_disabled(c) assert_pwm((c), false) > + > static bool edp_have_panel_power(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -884,6 +898,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder) > struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > struct drm_display_mode *adjusted_mode = &crtc->config.adjusted_mode; > > + assert_pwm_disabled(intel_dp->attached_connector); > + > /* > * There are four kinds of DP registers: > * > @@ -1167,6 +1183,23 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) > } > } > > +/* > + * For this and the disable sequence below, Google for actual eDP LCD timing > + * diagrams or check the eDP spec. Below is for reference on asserts and > + * does not contain Tx values for delays between steps. > + * > + * For panel on, the sequence should be: > + * - LCD power supply on (PP regs or VDD AUX) > + * - eDP should display black at this point > + * - HPD (if present) should go high > + * - AUX channel becomes available > + * - link training begins > + * - LED backlight power on > + * - LED PWM_EN goes high, duty cycle >min (PWM regs) > + * - link training completes > + * - LED_EN goes high (PP BLC_EN bit) > + */ > + > void intel_edp_panel_on(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -1212,6 +1245,19 @@ void intel_edp_panel_on(struct intel_dp *intel_dp) > } > } > > +/* > + * For panel off the sequence should be: > + * - LED_EN goes low (BLC_EN in our PP regs) > + * - LED PWM_EN goes low (PWM duty cycle 0 and PWM enable = 0) > + * - eDP should display black video at this point > + * - LED VCCS goes low (power for backlight) > + * - DP link goes to idle or off > + * - AUX goes down > + * - HPD line (if present) drops to low > + * - eDP black video stops > + * - LCD power supply shuts down (PP regs and VDD AUX) > + */ > + > void intel_edp_panel_off(struct intel_dp *intel_dp) > { > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > @@ -1231,11 +1277,17 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > > WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); > > + /* By this time the PWM and BLC bits should be off already */ > + assert_pwm_disabled(intel_dp->attached_connector); > + > pp = ironlake_get_pp_control(intel_dp); > + > + WARN(pp & EDP_BLC_ENABLE, > + "backlight controller still on at panel off time\n"); > + > /* We need to switch off panel power _and_ force vdd, for otherwise some > * panels get very unhappy and cease to work. */ > - pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD | > - EDP_BLC_ENABLE); > + pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD); > > pp_ctrl_reg = _pp_ctrl_reg(intel_dp); > > @@ -1271,6 +1323,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) > * allowing it to appear. > */ > wait_backlight_on(intel_dp); > + > + assert_pwm_enabled(intel_dp->attached_connector); > + > pp = ironlake_get_pp_control(intel_dp); > pp |= EDP_BLC_ENABLE; > > @@ -1292,6 +1347,9 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) > if (!is_edp(intel_dp)) > return; > > + /* PWM must still be enabled here */ > + assert_pwm_enabled(intel_dp->attached_connector); > + > intel_panel_disable_backlight(intel_dp->attached_connector); > > DRM_DEBUG_KMS("\n"); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 9002e77..0e91c40 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -861,6 +861,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc, > int fitting_mode); > void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > u32 max); > +u32 intel_panel_get_backlight(struct intel_connector *connector); > int intel_panel_setup_backlight(struct drm_connector *connector); > void intel_panel_enable_backlight(struct intel_connector *connector); > void intel_panel_disable_backlight(struct intel_connector *connector); > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index cb05840..21c5e6f 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -384,6 +384,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE)) > + return 0; > + If our internal state is consistent, I don't think this should be necessary. And if our internal state isn't consistent, we should fix that and maybe add internal asserts within intel_panel.c. > return I915_READ(VLV_BLC_PWM_CTL(pipe)) & BACKLIGHT_DUTY_CYCLE_MASK; > } > > @@ -395,7 +398,7 @@ static u32 vlv_get_backlight(struct intel_connector *connector) > return _vlv_get_backlight(dev, pipe); > } > > -static u32 intel_panel_get_backlight(struct intel_connector *connector) > +u32 intel_panel_get_backlight(struct intel_connector *connector) > { > struct drm_device *dev = connector->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 01 Apr 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> To make sure we properly follow the enable/disable sequences. >> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 62 ++++++++++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> drivers/gpu/drm/i915/intel_panel.c | 5 ++- >> 3 files changed, 65 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index bf73771..b6f7087 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp) >> return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp)); >> } >> >> +static void assert_pwm(struct intel_connector *connector, >> + bool expected_state) >> +{ >> + bool state; >> + >> + state = intel_panel_get_backlight(connector); > > If the duty cycle is regarded as a binary on/off, I'd rather add an > additional "is enabled" call to intel_panel.c. Especially so because the > duty cycle value returned by intel_panel_get_backlight is meaningless > without the max value. > >> + >> + WARN(state != expected_state, "pwm state failure, expected %d, found " >> + "%d\n", expected_state, state); >> +} >> + >> +#define assert_pwm_enabled(c) assert_pwm((c), true) >> +#define assert_pwm_disabled(c) assert_pwm((c), false) >> + >> static bool edp_have_panel_power(struct intel_dp *intel_dp) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> @@ -884,6 +898,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder) >> struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); >> struct drm_display_mode *adjusted_mode = &crtc->config.adjusted_mode; >> >> + assert_pwm_disabled(intel_dp->attached_connector); >> + >> /* >> * There are four kinds of DP registers: >> * >> @@ -1167,6 +1183,23 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) >> } >> } >> >> +/* >> + * For this and the disable sequence below, Google for actual eDP LCD timing >> + * diagrams or check the eDP spec. Below is for reference on asserts and >> + * does not contain Tx values for delays between steps. >> + * >> + * For panel on, the sequence should be: >> + * - LCD power supply on (PP regs or VDD AUX) >> + * - eDP should display black at this point >> + * - HPD (if present) should go high >> + * - AUX channel becomes available >> + * - link training begins >> + * - LED backlight power on >> + * - LED PWM_EN goes high, duty cycle >min (PWM regs) >> + * - link training completes >> + * - LED_EN goes high (PP BLC_EN bit) >> + */ >> + >> void intel_edp_panel_on(struct intel_dp *intel_dp) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> @@ -1212,6 +1245,19 @@ void intel_edp_panel_on(struct intel_dp *intel_dp) >> } >> } >> >> +/* >> + * For panel off the sequence should be: >> + * - LED_EN goes low (BLC_EN in our PP regs) >> + * - LED PWM_EN goes low (PWM duty cycle 0 and PWM enable = 0) >> + * - eDP should display black video at this point >> + * - LED VCCS goes low (power for backlight) >> + * - DP link goes to idle or off >> + * - AUX goes down >> + * - HPD line (if present) drops to low >> + * - eDP black video stops >> + * - LCD power supply shuts down (PP regs and VDD AUX) >> + */ >> + >> void intel_edp_panel_off(struct intel_dp *intel_dp) >> { >> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); >> @@ -1231,11 +1277,17 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) >> >> WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); >> >> + /* By this time the PWM and BLC bits should be off already */ >> + assert_pwm_disabled(intel_dp->attached_connector); >> + >> pp = ironlake_get_pp_control(intel_dp); >> + >> + WARN(pp & EDP_BLC_ENABLE, >> + "backlight controller still on at panel off time\n"); >> + >> /* We need to switch off panel power _and_ force vdd, for otherwise some >> * panels get very unhappy and cease to work. */ >> - pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD | >> - EDP_BLC_ENABLE); >> + pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD); >> >> pp_ctrl_reg = _pp_ctrl_reg(intel_dp); >> >> @@ -1271,6 +1323,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) >> * allowing it to appear. >> */ >> wait_backlight_on(intel_dp); >> + >> + assert_pwm_enabled(intel_dp->attached_connector); >> + >> pp = ironlake_get_pp_control(intel_dp); >> pp |= EDP_BLC_ENABLE; >> >> @@ -1292,6 +1347,9 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) >> if (!is_edp(intel_dp)) >> return; >> >> + /* PWM must still be enabled here */ >> + assert_pwm_enabled(intel_dp->attached_connector); >> + >> intel_panel_disable_backlight(intel_dp->attached_connector); >> >> DRM_DEBUG_KMS("\n"); >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 9002e77..0e91c40 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -861,6 +861,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc, >> int fitting_mode); >> void intel_panel_set_backlight(struct intel_connector *connector, u32 level, >> u32 max); >> +u32 intel_panel_get_backlight(struct intel_connector *connector); >> int intel_panel_setup_backlight(struct drm_connector *connector); >> void intel_panel_enable_backlight(struct intel_connector *connector); >> void intel_panel_disable_backlight(struct intel_connector *connector); >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index cb05840..21c5e6f 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -384,6 +384,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> + if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE)) >> + return 0; >> + > > If our internal state is consistent, I don't think this should be > necessary. And if our internal state isn't consistent, we should fix > that and maybe add internal asserts within intel_panel.c. I wrote this with the bigger picture in mind, but your check above is also doubly wrong! BR, Jani. > >> return I915_READ(VLV_BLC_PWM_CTL(pipe)) & BACKLIGHT_DUTY_CYCLE_MASK; >> } >> >> @@ -395,7 +398,7 @@ static u32 vlv_get_backlight(struct intel_connector *connector) >> return _vlv_get_backlight(dev, pipe); >> } >> >> -static u32 intel_panel_get_backlight(struct intel_connector *connector) >> +u32 intel_panel_get_backlight(struct intel_connector *connector) >> { >> struct drm_device *dev = connector->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> -- >> 1.8.4.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 01 Apr 2014 10:19:29 +0300 Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > To make sure we properly follow the enable/disable sequences. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 62 ++++++++++++++++++++++++++++++++++++-- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_panel.c | 5 ++- > > 3 files changed, 65 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index bf73771..b6f7087 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp) > > return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp)); > > } > > > > +static void assert_pwm(struct intel_connector *connector, > > + bool expected_state) > > +{ > > + bool state; > > + > > + state = intel_panel_get_backlight(connector); > > If the duty cycle is regarded as a binary on/off, I'd rather add an > additional "is enabled" call to intel_panel.c. Especially so because the > duty cycle value returned by intel_panel_get_backlight is meaningless > without the max value. Hm I guess that would be cleaner; for my purposes I thought any non-zero PWM duty cycle would be sufficient, but of course other checks are needed as well, like whether the PWM enable bit is on, and checks against the BLC_EN bit in the PP regs, but those are logically separate. is_enabled might better map back to the PWM_EN bit rather than a non-zero duty cycle though. > > > > + if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE)) > > + return 0; > > + > > If our internal state is consistent, I don't think this should be > necessary. And if our internal state isn't consistent, we should fix > that and maybe add internal asserts within intel_panel.c. Yeah this could be covered with other asserts as long as we have them in all the right places.
On Tue, 01 Apr 2014 12:27:43 +0300 Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 01 Apr 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > >> To make sure we properly follow the enable/disable sequences. > >> > >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > >> --- > >> drivers/gpu/drm/i915/intel_dp.c | 62 ++++++++++++++++++++++++++++++++++++-- > >> drivers/gpu/drm/i915/intel_drv.h | 1 + > >> drivers/gpu/drm/i915/intel_panel.c | 5 ++- > >> 3 files changed, 65 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index bf73771..b6f7087 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp) > >> return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp)); > >> } > >> > >> +static void assert_pwm(struct intel_connector *connector, > >> + bool expected_state) > >> +{ > >> + bool state; > >> + > >> + state = intel_panel_get_backlight(connector); > > > > If the duty cycle is regarded as a binary on/off, I'd rather add an > > additional "is enabled" call to intel_panel.c. Especially so because the > > duty cycle value returned by intel_panel_get_backlight is meaningless > > without the max value. > > > >> + > >> + WARN(state != expected_state, "pwm state failure, expected %d, found " > >> + "%d\n", expected_state, state); > >> +} > >> + > >> +#define assert_pwm_enabled(c) assert_pwm((c), true) > >> +#define assert_pwm_disabled(c) assert_pwm((c), false) > >> + > >> static bool edp_have_panel_power(struct intel_dp *intel_dp) > >> { > >> struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> @@ -884,6 +898,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder) > >> struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); > >> struct drm_display_mode *adjusted_mode = &crtc->config.adjusted_mode; > >> > >> + assert_pwm_disabled(intel_dp->attached_connector); > >> + > >> /* > >> * There are four kinds of DP registers: > >> * > >> @@ -1167,6 +1183,23 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) > >> } > >> } > >> > >> +/* > >> + * For this and the disable sequence below, Google for actual eDP LCD timing > >> + * diagrams or check the eDP spec. Below is for reference on asserts and > >> + * does not contain Tx values for delays between steps. > >> + * > >> + * For panel on, the sequence should be: > >> + * - LCD power supply on (PP regs or VDD AUX) > >> + * - eDP should display black at this point > >> + * - HPD (if present) should go high > >> + * - AUX channel becomes available > >> + * - link training begins > >> + * - LED backlight power on > >> + * - LED PWM_EN goes high, duty cycle >min (PWM regs) > >> + * - link training completes > >> + * - LED_EN goes high (PP BLC_EN bit) > >> + */ > >> + > >> void intel_edp_panel_on(struct intel_dp *intel_dp) > >> { > >> struct drm_device *dev = intel_dp_to_dev(intel_dp); > >> @@ -1212,6 +1245,19 @@ void intel_edp_panel_on(struct intel_dp *intel_dp) > >> } > >> } > >> > >> +/* > >> + * For panel off the sequence should be: > >> + * - LED_EN goes low (BLC_EN in our PP regs) > >> + * - LED PWM_EN goes low (PWM duty cycle 0 and PWM enable = 0) > >> + * - eDP should display black video at this point > >> + * - LED VCCS goes low (power for backlight) > >> + * - DP link goes to idle or off > >> + * - AUX goes down > >> + * - HPD line (if present) drops to low > >> + * - eDP black video stops > >> + * - LCD power supply shuts down (PP regs and VDD AUX) > >> + */ > >> + > >> void intel_edp_panel_off(struct intel_dp *intel_dp) > >> { > >> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > >> @@ -1231,11 +1277,17 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > >> > >> WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); > >> > >> + /* By this time the PWM and BLC bits should be off already */ > >> + assert_pwm_disabled(intel_dp->attached_connector); > >> + > >> pp = ironlake_get_pp_control(intel_dp); > >> + > >> + WARN(pp & EDP_BLC_ENABLE, > >> + "backlight controller still on at panel off time\n"); > >> + > >> /* We need to switch off panel power _and_ force vdd, for otherwise some > >> * panels get very unhappy and cease to work. */ > >> - pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD | > >> - EDP_BLC_ENABLE); > >> + pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD); > >> > >> pp_ctrl_reg = _pp_ctrl_reg(intel_dp); > >> > >> @@ -1271,6 +1323,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) > >> * allowing it to appear. > >> */ > >> wait_backlight_on(intel_dp); > >> + > >> + assert_pwm_enabled(intel_dp->attached_connector); > >> + > >> pp = ironlake_get_pp_control(intel_dp); > >> pp |= EDP_BLC_ENABLE; > >> > >> @@ -1292,6 +1347,9 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) > >> if (!is_edp(intel_dp)) > >> return; > >> > >> + /* PWM must still be enabled here */ > >> + assert_pwm_enabled(intel_dp->attached_connector); > >> + > >> intel_panel_disable_backlight(intel_dp->attached_connector); > >> > >> DRM_DEBUG_KMS("\n"); > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index 9002e77..0e91c40 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -861,6 +861,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc, > >> int fitting_mode); > >> void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > >> u32 max); > >> +u32 intel_panel_get_backlight(struct intel_connector *connector); > >> int intel_panel_setup_backlight(struct drm_connector *connector); > >> void intel_panel_enable_backlight(struct intel_connector *connector); > >> void intel_panel_disable_backlight(struct intel_connector *connector); > >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > >> index cb05840..21c5e6f 100644 > >> --- a/drivers/gpu/drm/i915/intel_panel.c > >> +++ b/drivers/gpu/drm/i915/intel_panel.c > >> @@ -384,6 +384,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe) > >> { > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> > >> + if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE)) > >> + return 0; > >> + > > > > If our internal state is consistent, I don't think this should be > > necessary. And if our internal state isn't consistent, we should fix > > that and maybe add internal asserts within intel_panel.c. > > I wrote this with the bigger picture in mind, but your check above is > also doubly wrong! Hah it is! I had it correct in one tree but must have copy & pasted wrong into this one. :)
On Tue, 01 Apr 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Tue, 01 Apr 2014 10:19:29 +0300 > Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > To make sure we properly follow the enable/disable sequences. >> > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> > --- >> > drivers/gpu/drm/i915/intel_dp.c | 62 ++++++++++++++++++++++++++++++++++++-- >> > drivers/gpu/drm/i915/intel_drv.h | 1 + >> > drivers/gpu/drm/i915/intel_panel.c | 5 ++- >> > 3 files changed, 65 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index bf73771..b6f7087 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp) >> > return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp)); >> > } >> > >> > +static void assert_pwm(struct intel_connector *connector, >> > + bool expected_state) >> > +{ >> > + bool state; >> > + >> > + state = intel_panel_get_backlight(connector); >> >> If the duty cycle is regarded as a binary on/off, I'd rather add an >> additional "is enabled" call to intel_panel.c. Especially so because the >> duty cycle value returned by intel_panel_get_backlight is meaningless >> without the max value. > > Hm I guess that would be cleaner; for my purposes I thought any > non-zero PWM duty cycle would be sufficient, but of course other checks > are needed as well, like whether the PWM enable bit is on, and checks > against the BLC_EN bit in the PP regs, but those are logically > separate. is_enabled might better map back to the PWM_EN bit rather > than a non-zero duty cycle though. We could add intel_panel_backlight_enabled() call that returns connector->panel.backlight.enabled. BR, Jani. > >> > >> > + if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE)) >> > + return 0; >> > + >> >> If our internal state is consistent, I don't think this should be >> necessary. And if our internal state isn't consistent, we should fix >> that and maybe add internal asserts within intel_panel.c. > > Yeah this could be covered with other asserts as long as we have them > in all the right places. > > -- > Jesse Barnes, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index bf73771..b6f7087 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -301,6 +301,20 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp) return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp)); } +static void assert_pwm(struct intel_connector *connector, + bool expected_state) +{ + bool state; + + state = intel_panel_get_backlight(connector); + + WARN(state != expected_state, "pwm state failure, expected %d, found " + "%d\n", expected_state, state); +} + +#define assert_pwm_enabled(c) assert_pwm((c), true) +#define assert_pwm_disabled(c) assert_pwm((c), false) + static bool edp_have_panel_power(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -884,6 +898,8 @@ static void intel_dp_mode_set(struct intel_encoder *encoder) struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc); struct drm_display_mode *adjusted_mode = &crtc->config.adjusted_mode; + assert_pwm_disabled(intel_dp->attached_connector); + /* * There are four kinds of DP registers: * @@ -1167,6 +1183,23 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync) } } +/* + * For this and the disable sequence below, Google for actual eDP LCD timing + * diagrams or check the eDP spec. Below is for reference on asserts and + * does not contain Tx values for delays between steps. + * + * For panel on, the sequence should be: + * - LCD power supply on (PP regs or VDD AUX) + * - eDP should display black at this point + * - HPD (if present) should go high + * - AUX channel becomes available + * - link training begins + * - LED backlight power on + * - LED PWM_EN goes high, duty cycle >min (PWM regs) + * - link training completes + * - LED_EN goes high (PP BLC_EN bit) + */ + void intel_edp_panel_on(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -1212,6 +1245,19 @@ void intel_edp_panel_on(struct intel_dp *intel_dp) } } +/* + * For panel off the sequence should be: + * - LED_EN goes low (BLC_EN in our PP regs) + * - LED PWM_EN goes low (PWM duty cycle 0 and PWM enable = 0) + * - eDP should display black video at this point + * - LED VCCS goes low (power for backlight) + * - DP link goes to idle or off + * - AUX goes down + * - HPD line (if present) drops to low + * - eDP black video stops + * - LCD power supply shuts down (PP regs and VDD AUX) + */ + void intel_edp_panel_off(struct intel_dp *intel_dp) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -1231,11 +1277,17 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); + /* By this time the PWM and BLC bits should be off already */ + assert_pwm_disabled(intel_dp->attached_connector); + pp = ironlake_get_pp_control(intel_dp); + + WARN(pp & EDP_BLC_ENABLE, + "backlight controller still on at panel off time\n"); + /* We need to switch off panel power _and_ force vdd, for otherwise some * panels get very unhappy and cease to work. */ - pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD | - EDP_BLC_ENABLE); + pp &= ~(POWER_TARGET_ON | PANEL_POWER_RESET | EDP_FORCE_VDD); pp_ctrl_reg = _pp_ctrl_reg(intel_dp); @@ -1271,6 +1323,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) * allowing it to appear. */ wait_backlight_on(intel_dp); + + assert_pwm_enabled(intel_dp->attached_connector); + pp = ironlake_get_pp_control(intel_dp); pp |= EDP_BLC_ENABLE; @@ -1292,6 +1347,9 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) if (!is_edp(intel_dp)) return; + /* PWM must still be enabled here */ + assert_pwm_enabled(intel_dp->attached_connector); + intel_panel_disable_backlight(intel_dp->attached_connector); DRM_DEBUG_KMS("\n"); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 9002e77..0e91c40 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -861,6 +861,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc, int fitting_mode); void intel_panel_set_backlight(struct intel_connector *connector, u32 level, u32 max); +u32 intel_panel_get_backlight(struct intel_connector *connector); int intel_panel_setup_backlight(struct drm_connector *connector); void intel_panel_enable_backlight(struct intel_connector *connector); void intel_panel_disable_backlight(struct intel_connector *connector); diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index cb05840..21c5e6f 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -384,6 +384,9 @@ static u32 _vlv_get_backlight(struct drm_device *dev, enum pipe pipe) { struct drm_i915_private *dev_priv = dev->dev_private; + if (I915_READ(VLV_BLC_PWM_CTL2(pipe) & BLM_PWM_ENABLE)) + return 0; + return I915_READ(VLV_BLC_PWM_CTL(pipe)) & BACKLIGHT_DUTY_CYCLE_MASK; } @@ -395,7 +398,7 @@ static u32 vlv_get_backlight(struct intel_connector *connector) return _vlv_get_backlight(dev, pipe); } -static u32 intel_panel_get_backlight(struct intel_connector *connector) +u32 intel_panel_get_backlight(struct intel_connector *connector) { struct drm_device *dev = connector->base.dev; struct drm_i915_private *dev_priv = dev->dev_private;
To make sure we properly follow the enable/disable sequences. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_dp.c | 62 ++++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_panel.c | 5 ++- 3 files changed, 65 insertions(+), 3 deletions(-)