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

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Technology Center