| Message ID | 1406637880-6918-1-git-send-email-rafael.barbalho@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 29, 2014 at 01:44:40PM +0100, rafael.barbalho@intel.com wrote: > From: Rafael Barbalho <rafael.barbalho@intel.com> > > Make the vlv/chv backlight setup more generic by actually looking at which > pipe the panel is attached to and read the backlight PWM registers that were > setup by the bios from that pipe. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 24 ++++++++++-------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index 59b028f..be75d76 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -1200,32 +1200,28 @@ static int vlv_setup_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; > - enum pipe pipe; > + enum pipe pipe = intel_get_pipe_from_connector(connector); This won't work unless the connector is already enabled. The power sequencer has a similar problem where we have to somehow deduce the correct pipe. vlv_power_sequencer_pipe() tries to guess the correct pipe there. We could start with intel_get_pipe_from_connector() and if that fails we'd do something like vlv_power_sequencer_pipe(). Hmm, except the backlight registers don't have the port information, so I guess we'd need to pick the pipe simply based on the DP port control register. > u32 ctl, ctl2, val; > > - for_each_pipe(pipe) { > - u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(pipe)); > + ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe)); > + panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; > > - /* Skip if the modulation freq is already set */ > - if (cur_val & ~BACKLIGHT_DUTY_CYCLE_MASK) > - continue; > + ctl = I915_READ(VLV_BLC_PWM_CTL(pipe)); > > - cur_val &= BACKLIGHT_DUTY_CYCLE_MASK; > - I915_WRITE(VLV_BLC_PWM_CTL(pipe), (0xf42 << 16) | > - cur_val); > + /* Skip if the modulation freq is already set */ > + if ((ctl & ~BACKLIGHT_DUTY_CYCLE_MASK) == 0) { > + ctl &= BACKLIGHT_DUTY_CYCLE_MASK; > + ctl |= (0xf42 << 16); > + I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl); > } > > - ctl2 = I915_READ(VLV_BLC_PWM_CTL2(PIPE_A)); > - panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; > - > - ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A)); > panel->backlight.max = ctl >> 16; > if (!panel->backlight.max) > return -ENODEV; > > panel->backlight.min = get_backlight_min_vbt(connector); > > - val = _vlv_get_backlight(dev, PIPE_A); > + val = _vlv_get_backlight(dev, pipe); > panel->backlight.level = intel_panel_compute_brightness(connector, val); > > panel->backlight.enabled = (ctl2 & BLM_PWM_ENABLE) && > -- > 2.0.3
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Tuesday, July 29, 2014 2:13 PM > To: Barbalho, Rafael > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915: Correctly read backlight PWM for pipe B on > vlv/chv > > On Tue, Jul 29, 2014 at 01:44:40PM +0100, rafael.barbalho@intel.com wrote: > > From: Rafael Barbalho <rafael.barbalho@intel.com> > > > > Make the vlv/chv backlight setup more generic by actually looking at which > > pipe the panel is attached to and read the backlight PWM registers that > were > > setup by the bios from that pipe. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com> > > --- > > drivers/gpu/drm/i915/intel_panel.c | 24 ++++++++++-------------- > > 1 file changed, 10 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > b/drivers/gpu/drm/i915/intel_panel.c > > index 59b028f..be75d76 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -1200,32 +1200,28 @@ static int vlv_setup_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; > > - enum pipe pipe; > > + enum pipe pipe = intel_get_pipe_from_connector(connector); > > This won't work unless the connector is already enabled. > > The power sequencer has a similar problem where we have to somehow > deduce > the correct pipe. vlv_power_sequencer_pipe() tries to guess the correct > pipe > there. > > We could start with intel_get_pipe_from_connector() and if that fails we'd > do something like vlv_power_sequencer_pipe(). Hmm, except the backlight > registers don't have the port information, so I guess we'd need to pick the > pipe simply based on the DP port control register. > Fair point, I didn't realise that intel_get_pipe_from_connector could return INVALID_PIPE. How about if I add: + enum pipe pipe = intel_get_pipe_from_connector(connector); + + if (pipe == INVALID_PIPE) + pipe = PIPE_A; It would return the driver to the current behaviour but still enable pipe B when that is present at start up. > > u32 ctl, ctl2, val; > > > > - for_each_pipe(pipe) { > > - u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(pipe)); > > + ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe)); > > + panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; > > > > - /* Skip if the modulation freq is already set */ > > - if (cur_val & ~BACKLIGHT_DUTY_CYCLE_MASK) > > - continue; > > + ctl = I915_READ(VLV_BLC_PWM_CTL(pipe)); > > > > - cur_val &= BACKLIGHT_DUTY_CYCLE_MASK; > > - I915_WRITE(VLV_BLC_PWM_CTL(pipe), (0xf42 << 16) | > > - cur_val); > > + /* Skip if the modulation freq is already set */ > > + if ((ctl & ~BACKLIGHT_DUTY_CYCLE_MASK) == 0) { > > + ctl &= BACKLIGHT_DUTY_CYCLE_MASK; > > + ctl |= (0xf42 << 16); > > + I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl); > > } > > > > - ctl2 = I915_READ(VLV_BLC_PWM_CTL2(PIPE_A)); > > - panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; > > - > > - ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A)); > > panel->backlight.max = ctl >> 16; > > if (!panel->backlight.max) > > return -ENODEV; > > > > panel->backlight.min = get_backlight_min_vbt(connector); > > > > - val = _vlv_get_backlight(dev, PIPE_A); > > + val = _vlv_get_backlight(dev, pipe); > > panel->backlight.level = > intel_panel_compute_brightness(connector, val); > > > > panel->backlight.enabled = (ctl2 & BLM_PWM_ENABLE) && > > -- > > 2.0.3 > > -- > Ville Syrjälä > Intel OTC
> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf > Of Barbalho, Rafael > Sent: Tuesday, July 29, 2014 2:38 PM > To: Ville Syrjälä > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correctly read backlight PWM for > pipe B on vlv/chv > Importance: High > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Tuesday, July 29, 2014 2:13 PM > > To: Barbalho, Rafael > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [PATCH] drm/i915: Correctly read backlight PWM for pipe B on > > vlv/chv > > > > On Tue, Jul 29, 2014 at 01:44:40PM +0100, rafael.barbalho@intel.com > wrote: > > > From: Rafael Barbalho <rafael.barbalho@intel.com> > > > > > > Make the vlv/chv backlight setup more generic by actually looking at > which > > > pipe the panel is attached to and read the backlight PWM registers that > > were > > > setup by the bios from that pipe. > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_panel.c | 24 ++++++++++-------------- > > > 1 file changed, 10 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c > > b/drivers/gpu/drm/i915/intel_panel.c > > > index 59b028f..be75d76 100644 > > > --- a/drivers/gpu/drm/i915/intel_panel.c > > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > > @@ -1200,32 +1200,28 @@ static int vlv_setup_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; > > > - enum pipe pipe; > > > + enum pipe pipe = intel_get_pipe_from_connector(connector); > > > > This won't work unless the connector is already enabled. > > > > The power sequencer has a similar problem where we have to somehow > > deduce > > the correct pipe. vlv_power_sequencer_pipe() tries to guess the correct > > pipe > > there. > > > > We could start with intel_get_pipe_from_connector() and if that fails we'd > > do something like vlv_power_sequencer_pipe(). Hmm, except the > backlight > > registers don't have the port information, so I guess we'd need to pick the > > pipe simply based on the DP port control register. > > > > Fair point, I didn't realise that intel_get_pipe_from_connector could return > INVALID_PIPE. > > How about if I add: > + enum pipe pipe = intel_get_pipe_from_connector(connector); > + > + if (pipe == INVALID_PIPE) > + pipe = PIPE_A; > > It would return the driver to the current behaviour but still enable pipe B > when that is present > at start up. > Thinking about it I need to still keep the initial for_each_pipe loop to very subtly initialise both pipe A & B to a known PWM modulation frequency, which would explain why setting the backlight brightness works on pipe B. However, inheriting it from the bios is not working, which is what I am seeing. Let me rejig the patch and I'll send out a new version. > > > u32 ctl, ctl2, val; > > > > > > - for_each_pipe(pipe) { > > > - u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(pipe)); > > > + ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe)); > > > + panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; > > > > > > - /* Skip if the modulation freq is already set */ > > > - if (cur_val & ~BACKLIGHT_DUTY_CYCLE_MASK) > > > - continue; > > > + ctl = I915_READ(VLV_BLC_PWM_CTL(pipe)); > > > > > > - cur_val &= BACKLIGHT_DUTY_CYCLE_MASK; > > > - I915_WRITE(VLV_BLC_PWM_CTL(pipe), (0xf42 << 16) | > > > - cur_val); > > > + /* Skip if the modulation freq is already set */ > > > + if ((ctl & ~BACKLIGHT_DUTY_CYCLE_MASK) == 0) { > > > + ctl &= BACKLIGHT_DUTY_CYCLE_MASK; > > > + ctl |= (0xf42 << 16); > > > + I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl); > > > } > > > > > > - ctl2 = I915_READ(VLV_BLC_PWM_CTL2(PIPE_A)); > > > - panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; > > > - > > > - ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A)); > > > panel->backlight.max = ctl >> 16; > > > if (!panel->backlight.max) > > > return -ENODEV; > > > > > > panel->backlight.min = get_backlight_min_vbt(connector); > > > > > > - val = _vlv_get_backlight(dev, PIPE_A); > > > + val = _vlv_get_backlight(dev, pipe); > > > panel->backlight.level = > > intel_panel_compute_brightness(connector, val); > > > > > > panel->backlight.enabled = (ctl2 & BLM_PWM_ENABLE) && > > > -- > > > 2.0.3 > > > > -- > > Ville Syrjälä > > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index 59b028f..be75d76 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -1200,32 +1200,28 @@ static int vlv_setup_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; - enum pipe pipe; + enum pipe pipe = intel_get_pipe_from_connector(connector); u32 ctl, ctl2, val; - for_each_pipe(pipe) { - u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(pipe)); + ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe)); + panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; - /* Skip if the modulation freq is already set */ - if (cur_val & ~BACKLIGHT_DUTY_CYCLE_MASK) - continue; + ctl = I915_READ(VLV_BLC_PWM_CTL(pipe)); - cur_val &= BACKLIGHT_DUTY_CYCLE_MASK; - I915_WRITE(VLV_BLC_PWM_CTL(pipe), (0xf42 << 16) | - cur_val); + /* Skip if the modulation freq is already set */ + if ((ctl & ~BACKLIGHT_DUTY_CYCLE_MASK) == 0) { + ctl &= BACKLIGHT_DUTY_CYCLE_MASK; + ctl |= (0xf42 << 16); + I915_WRITE(VLV_BLC_PWM_CTL(pipe), ctl); } - ctl2 = I915_READ(VLV_BLC_PWM_CTL2(PIPE_A)); - panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965; - - ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A)); panel->backlight.max = ctl >> 16; if (!panel->backlight.max) return -ENODEV; panel->backlight.min = get_backlight_min_vbt(connector); - val = _vlv_get_backlight(dev, PIPE_A); + val = _vlv_get_backlight(dev, pipe); panel->backlight.level = intel_panel_compute_brightness(connector, val); panel->backlight.enabled = (ctl2 & BLM_PWM_ENABLE) &&