On Thu, Nov 14, 2013 at 07:43:07AM +0200, Jani Nikula wrote:
> On Wed, 13 Nov 2013, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > On Fri, 2013-11-08 at 16:49 +0200, Jani Nikula wrote:
> >> We should now have all the information we need to do a full
> >> initialization of the backlight registers.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/intel_panel.c | 145 ++++++++++++++++++++++--------------
> >> 1 file changed, 89 insertions(+), 56 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> index 0e8f0a3..da088e3 100644
> >> --- a/drivers/gpu/drm/i915/intel_panel.c
> >> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> @@ -719,50 +719,71 @@ static void pch_enable_backlight(struct intel_connector *connector)
> >> enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> enum transcoder cpu_transcoder =
> >> intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> >> - u32 tmp;
> >> -
> >> - tmp = I915_READ(BLC_PWM_CPU_CTL2);
> >> + u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >>
> >> - /* Note that this can also get called through dpms changes. And
> >> - * we don't track the backlight dpms state, hence check whether
> >> - * we have to do anything first. */
> >> - if (tmp & BLM_PWM_ENABLE)
> >> - return;
> >> + cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >> + if (cpu_ctl2 & BLM_PWM_ENABLE) {
> >> + WARN(1, "cpu backlight already enabled\n");
> >> + cpu_ctl2 &= ~BLM_PWM_ENABLE;
> >> + I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
> >> + }
> >>
> >> - if (INTEL_INFO(dev)->num_pipes == 3)
> >> - tmp &= ~BLM_PIPE_SELECT_IVB;
> >> - else
> >> - tmp &= ~BLM_PIPE_SELECT;
> >> + pch_ctl1 = I915_READ(BLC_PWM_PCH_CTL1);
> >> + if (pch_ctl1 & BLM_PCH_PWM_ENABLE) {
> >> + DRM_DEBUG_KMS("pch backlight already enabled\n");
> >> + pch_ctl1 &= ~BLM_PCH_PWM_ENABLE;
> >> + I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
> >> + }
> >>
> >> if (cpu_transcoder == TRANSCODER_EDP)
> >> - tmp |= BLM_TRANSCODER_EDP;
> >> + cpu_ctl2 = BLM_TRANSCODER_EDP;
> >> else
> >> - tmp |= BLM_PIPE(cpu_transcoder);
> >> - tmp &= ~BLM_PWM_ENABLE;
> >> -
> >> - I915_WRITE(BLC_PWM_CPU_CTL2, tmp);
> >> + cpu_ctl2 = BLM_PIPE(cpu_transcoder);
> >> + I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
> >> POSTING_READ(BLC_PWM_CPU_CTL2);
> >> - I915_WRITE(BLC_PWM_CPU_CTL2, tmp | BLM_PWM_ENABLE);
> >> -
> >> - if (!(dev_priv->quirks & QUIRK_NO_PCH_PWM_ENABLE)) {
> >> - tmp = I915_READ(BLC_PWM_PCH_CTL1);
> >> - tmp |= BLM_PCH_PWM_ENABLE;
> >> - tmp &= ~BLM_PCH_OVERRIDE_ENABLE;
> >> - I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
> >> - }
> >
> > Like in the previous patch, why is this removed? If the w/a is not
> > needed any more it should be a separate patch.
>
> As I said, I think the quirk is bogus. With the full backlight setup at
> enable, we shouldn't trust this to have been set by the BIOS. Again, it
> is required to be enabled, even on the machines that supposedly need the
> quirk.
>
> I expect the new enable sequence to tackle the problems. And any fallout
> we should fix properly, without quirks - and with this series we are
> much more flexible to adjust the sequence on a per platform basis. For
> example, I was able to test on an IVB that we can't set the backlight
> duty cycle until the cpu pwm has been enabled (hence the comment
> below). But we do it the before the pch pwm enable.
>
> All that said, maybe it should be a separate patch.
Yeah, and then we can also rip out the quirk entries and tables with it,
too. Also maybe mention findings like the one about the cpu pwm register
in the commit message quickly.
-Daniel
>
>
> BR,
> Jani.
>
> >
> >> + I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2 | BLM_PWM_ENABLE);
> >>
> >> - /*
> >> - * Call below after setting BLC_PWM_CPU_CTL2 and BLC_PWM_PCH_CTL1.
> >> - * BLC_PWM_CPU_CTL may be cleared to zero automatically when these
> >> - * registers are set.
> >> - */
> >> + /* This won't stick until the above enable. */
> >> intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> +
> >> + pch_ctl2 = panel->backlight.max << 16;
> >> + I915_WRITE(BLC_PWM_PCH_CTL2, pch_ctl2);
> >> +
> >> + pch_ctl1 = 0;
> >> + if (panel->backlight.active_low_pwm)
> >> + pch_ctl1 |= BLM_PCH_POLARITY;
> >> + I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1);
> >> + POSTING_READ(BLC_PWM_PCH_CTL1);
> >> + I915_WRITE(BLC_PWM_PCH_CTL1, pch_ctl1 | BLM_PCH_PWM_ENABLE);
> >> }
> >>
> >> static void i9xx_enable_backlight(struct intel_connector *connector)
> >> {
> >> + struct drm_device *dev = connector->base.dev;
> >> + struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_panel *panel = &connector->panel;
> >> + u32 ctl, freq;
> >>
> >> + ctl = I915_READ(BLC_PWM_CTL);
> >> + if (ctl & BACKLIGHT_DUTY_CYCLE_MASK_PNV) {
> >> + WARN(1, "backlight already enabled\n");
> >> + I915_WRITE(BLC_PWM_CTL, 0);
> >> + }
> >> +
> >> + freq = panel->backlight.max;
> >> + if (panel->backlight.combination_mode)
> >> + freq /= 0xff;
> >> +
> >> + ctl = freq << 17;
> >> + if (IS_GEN2(dev) && panel->backlight.combination_mode)
> >> + ctl |= BLM_LEGACY_MODE;
> >> + if (IS_PINEVIEW(dev) && panel->backlight.active_low_pwm)
> >> + ctl |= BLM_POLARITY_PNV;
> >> +
> >> + I915_WRITE(BLC_PWM_CTL, ctl);
> >> + POSTING_READ(BLC_PWM_CTL);
> >> +
> >> + /* XXX: combine this into above write? */
> >> intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> }
> >>
> >> @@ -772,25 +793,33 @@ static void i965_enable_backlight(struct intel_connector *connector)
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_panel *panel = &connector->panel;
> >> enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> - u32 tmp;
> >> -
> >> - tmp = I915_READ(BLC_PWM_CTL2);
> >> + u32 ctl, ctl2, freq;
> >>
> >> - /* Note that this can also get called through dpms changes. And
> >> - * we don't track the backlight dpms state, hence check whether
> >> - * we have to do anything first. */
> >> - if (tmp & BLM_PWM_ENABLE)
> >> - return;
> >> + ctl2 = I915_READ(BLC_PWM_CTL2);
> >> + if (ctl2 & BLM_PWM_ENABLE) {
> >> + WARN(1, "backlight already enabled\n");
> >> + ctl2 &= ~BLM_PWM_ENABLE;
> >> + I915_WRITE(BLC_PWM_CTL2, ctl2);
> >> + }
> >>
> >> - tmp &= ~BLM_PIPE_SELECT;
> >> - tmp |= BLM_PIPE(pipe);
> >> - tmp &= ~BLM_PWM_ENABLE;
> >> + freq = panel->backlight.max;
> >> + if (panel->backlight.combination_mode)
> >> + freq /= 0xff;
> >>
> >> - I915_WRITE(BLC_PWM_CTL2, tmp);
> >> - POSTING_READ(BLC_PWM_CTL2);
> >> - I915_WRITE(BLC_PWM_CTL2, tmp | BLM_PWM_ENABLE);
> >> + ctl = freq << 16;
> >> + I915_WRITE(BLC_PWM_CTL, ctl);
> >>
> >> + /* XXX: combine this into above write? */
> >> intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> +
> >> + ctl2 = BLM_PIPE(pipe);
> >> + if (panel->backlight.combination_mode)
> >> + ctl2 |= BLM_COMBINATION_MODE;
> >> + if (panel->backlight.active_low_pwm)
> >> + ctl2 |= BLM_POLARITY_I965;
> >> + I915_WRITE(BLC_PWM_CTL2, ctl2);
> >> + POSTING_READ(BLC_PWM_CTL2);
> >> + I915_WRITE(BLC_PWM_CTL2, ctl2 | BLM_PWM_ENABLE);
> >> }
> >>
> >> static void vlv_enable_backlight(struct intel_connector *connector)
> >> @@ -799,23 +828,27 @@ static void vlv_enable_backlight(struct intel_connector *connector)
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_panel *panel = &connector->panel;
> >> enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> - u32 tmp;
> >> + u32 ctl, ctl2;
> >>
> >> - tmp = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> >> + ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
> >> + if (ctl2 & BLM_PWM_ENABLE) {
> >> + WARN(1, "backlight already enabled\n");
> >> + ctl2 &= ~BLM_PWM_ENABLE;
> >> + I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
> >> + }
> >>
> >> - /* Note that this can also get called through dpms changes. And
> >> - * we don't track the backlight dpms state, hence check whether
> >> - * we have to do anything first. */
> >> - if (tmp & BLM_PWM_ENABLE)
> >> - return;
> >> + ctl = panel->backlight.max << 16;
> >> + I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl);
> >>
> >> - tmp &= ~BLM_PWM_ENABLE;
> >> + /* XXX: combine this into above write? */
> >> + intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >>
> >> - I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp);
> >> + ctl2 = 0;
> >> + if (panel->backlight.active_low_pwm)
> >> + ctl2 |= BLM_POLARITY_I965;
> >> + I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
> >> POSTING_READ(VLV_BLC_PWM_CTL2(pipe));
> >> - I915_WRITE(VLV_BLC_PWM_CTL2(pipe), tmp | BLM_PWM_ENABLE);
> >> -
> >> - intel_panel_actually_set_backlight(connector, panel->backlight.level);
> >> + I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2 | BLM_PWM_ENABLE);
> >> }
> >>
> >> void intel_panel_enable_backlight(struct intel_connector *connector)
> >
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
