[Intel-gfx] [PATCH 11/13] drm/i915: do full backlight setup at enable time
Imre Deak
imre.deak at intel.com
Thu Nov 14 09:28:29 CET 2013
On Thu, 2013-11-14 at 07:43 +0200, Jani Nikula wrote:
> On Wed, 13 Nov 2013, Imre Deak <imre.deak at intel.com> 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 at intel.com>
> >> ---
> >> 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.
Ok, hm I realize now that removing the quirk might not be trivial since
we depend on the new sequence to do things right. So if you decide so,
I'm also fine leaving it as-is, if it's mentioned in the log. Also since
it's not used elsewhere the quirk should be removed fully.
--Imre
>
>
> 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)
> >
>
More information about the Intel-gfx
mailing list