On Tue, 01 Apr 2014 12:27:43 +0300
Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Tue, 01 Apr 2014, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> > On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> wrote:
> >> To make sure we properly follow the enable/disable sequences.
> >>
> >> Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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. :)
--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
