Re: [PATCH 3/3] drm/i915/vlv: use min brightness from VBT

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 01 Apr 2014 11:08:13 +0300
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Going below the minimum value may affect the BLC_EN line, so try to use
> > the VBT provided minimum where possible, otherwise use an experimentally
> > derived value to prevent the panel from coming up.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h    |  1 +
> >  drivers/gpu/drm/i915/intel_bios.c  |  3 ++-
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  drivers/gpu/drm/i915/intel_panel.c | 10 +++++++++-
> >  4 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ff02225..3c40dcb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1148,6 +1148,7 @@ struct intel_vbt_data {
> >  	struct {
> >  		u16 pwm_freq_hz;
> >  		bool active_low_pwm;
> > +		u8 min_brightness;
> >  	} backlight;
> >  
> >  	/* MIPI DSI */
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 4867f4c..e8dedf5 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -301,11 +301,12 @@ parse_lfp_backlight(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
> >  
> >  	dev_priv->vbt.backlight.pwm_freq_hz = entry->pwm_freq_hz;
> >  	dev_priv->vbt.backlight.active_low_pwm = entry->active_low_pwm;
> > +	dev_priv->vbt.backlight.min_brightness = entry->min_brightness;
> >  	DRM_DEBUG_KMS("VBT backlight PWM modulation frequency %u Hz, "
> >  		      "active %s, min brightness %u, level %u\n",
> >  		      dev_priv->vbt.backlight.pwm_freq_hz,
> >  		      dev_priv->vbt.backlight.active_low_pwm ? "low" : "high",
> > -		      entry->min_brightness,
> > +		      dev_priv->vbt.backlight.min_brightness,
> >  		      backlight_data->level[panel_type]);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 0e91c40..053a968 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -166,6 +166,7 @@ struct intel_panel {
> >  		bool present;
> >  		u32 level;
> >  		u32 max;
> > +		u32 min;
> >  		bool enabled;
> >  		bool combination_mode;	/* gen 2/4 only */
> >  		bool active_low_pwm;
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 21c5e6f..27d7508 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -510,6 +510,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level,
> >  	else
> >  		level = freq / max * level;
> >  
> > +	if (level < panel->backlight.min)
> > +		level = panel->backlight.min;
> 
> Are you sure the VBT minimum is the *absolute* minimum duty cycle value?
> The spec I have says, "This field specifies the minimum brightness",
> which could be an absolute value, percentage, multiplier, anything
> really. I most doubt it's an absolute value because you have to go out
> of your way to figure out what the max is because it's defined in terms
> of the PWM frequency in Hz.
> 
> No matter what the unit is, this is wrong for any gen 2/4 platform where
> combination mode is enabled.
> 
> ---
> 
> I'm also hesitant about the change. First, what does it mean for the
> userspace that 0 no longer switches off the backlight? (Which I realize
> was sort of broken due to not really disabling the PWM since gen4, but
> that's what the perceived effect was anyway.) This may not be a big
> issue since 0 is not necessarily off for acpi backlight.
> 
> Second, what's the usability implication of backlight change requests
> potentially having no effect at the low values? Imagine maybe 10-20
> levels of brightness in the UI but no change between the lowest
> levels. I fear we might get regression reports for this. The only way to
> handle this would be scaling of some sort I think.
> 
> I suppose our original thinking was mechanism not policy, and just
> exposed the full range to userspace. This is very different from the
> acpi backlight approach, which looks at vbt/opregion and exposes I think
> up to 20 discrete levels with 0 being min, not necessarily off. The
> opregion tables may also have a non-linear mapping from backlight
> percentage to duty cycle. Maybe we should bite the bullet and follow
> suit?

Yeah I should have marked this one as RFC; the VBT usage is new and we
really do need to scale things.

The main bug fix here is to prevent the BYT duty cycle from dropping
below ~64.  As you say, to do that we really shouldn't expose those
lower 64 values to userspace as they'll do nothing.

-- 
Jesse Barnes, Intel Open Source Technology Center