[Intel-gfx] [PATCH] drm/i915: Correctly read backlight PWM for pipe B on vlv/chv

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf
> Of Barbalho, Rafael
> Sent: Tuesday, July 29, 2014 2:38 PM
> To: Ville Syrjälä
> Cc: intel-gfx at 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 at linux.intel.com]
> > Sent: Tuesday, July 29, 2014 2:13 PM
> > To: Barbalho, Rafael
> > Cc: intel-gfx at 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 at intel.com
> wrote:
> > > From: Rafael Barbalho <rafael.barbalho at 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 at linux.intel.com>
> > > Signed-off-by: Rafael Barbalho <rafael.barbalho at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx