| Message ID | 1396289637-1013-3-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 31, 2014 at 11:13:57AM -0700, Jesse Barnes 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. "to prevent the panel form failing to come up" I hope? -Daniel > > 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; > + > panel->backlight.level = level; > if (panel->backlight.device) > panel->backlight.device->props.brightness = level; > @@ -1047,7 +1050,12 @@ static int vlv_setup_backlight(struct intel_connector *connector) > > ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A)); > panel->backlight.max = ctl >> 16; > - if (!panel->backlight.max) > + panel->backlight.min = dev_priv->vbt.backlight.min_brightness; > + /* sane (i.e. checked on scope) default */ > + if (!panel->backlight.min) > + panel->backlight.min = 64; > + if (!panel->backlight.max || > + panel->backlight.max < panel->backlight.min) > return -ENODEV; > > val = _vlv_get_backlight(dev, PIPE_A); > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 31 Mar 2014 21:07:04 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Mar 31, 2014 at 11:13:57AM -0700, Jesse Barnes 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. > > "to prevent the panel form failing to come up" I hope? Yes I suppose that makes more sense. I'm not trying to deliberately sabotage things, even though it may appear that way at times. :)
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? BR, Jani. > + > panel->backlight.level = level; > if (panel->backlight.device) > panel->backlight.device->props.brightness = level; > @@ -1047,7 +1050,12 @@ static int vlv_setup_backlight(struct intel_connector *connector) > > ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A)); > panel->backlight.max = ctl >> 16; > - if (!panel->backlight.max) > + panel->backlight.min = dev_priv->vbt.backlight.min_brightness; > + /* sane (i.e. checked on scope) default */ > + if (!panel->backlight.min) > + panel->backlight.min = 64; > + if (!panel->backlight.max || > + panel->backlight.max < panel->backlight.min) > return -ENODEV; > > val = _vlv_get_backlight(dev, PIPE_A); > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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.
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; + panel->backlight.level = level; if (panel->backlight.device) panel->backlight.device->props.brightness = level; @@ -1047,7 +1050,12 @@ static int vlv_setup_backlight(struct intel_connector *connector) ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A)); panel->backlight.max = ctl >> 16; - if (!panel->backlight.max) + panel->backlight.min = dev_priv->vbt.backlight.min_brightness; + /* sane (i.e. checked on scope) default */ + if (!panel->backlight.min) + panel->backlight.min = 64; + if (!panel->backlight.max || + panel->backlight.max < panel->backlight.min) return -ENODEV; val = _vlv_get_backlight(dev, PIPE_A);
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(-)