Re: [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness

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

On Tue, 20 May 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 29 Apr 2014 23:30:49 +0300
> Jani Nikula <jani.nikula@intel.com> wrote:
>
>> Historically we've exposed the full backlight PWM duty cycle range to
>> the userspace, in the name of "mechanism, not policy". However, it turns
>> out there are both panels and board designs where there is a minimum
>> duty cycle that is required for proper operation. The minimum duty cycle
>> is available in the VBT.
>> 
>> The backlight class sysfs interface does not make any promises to the
>> userspace about the physical meaning of the range
>> 0..max_brightness. Specifically there is no guarantee that 0 means off;
>> indeed for acpi_backlight 0 usually is not off, but the minimum
>> acceptable value.
>> 
>> Respect the minimum backlight, and expose the range acceptable to the
>> hardware as 0..max_brightness to the userspace via the backlight class
>> device; 0 means the minimum acceptable enabled value. To switch off the
>> backlight, the user must disable the encoder.
>> 
>> As a side effect, make the backlight class device max brightness and
>> physical PWM modulation frequency (i.e. max duty cycle) independent.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> I like the direction here... I wonder if we should always virtualize
> the max too, and just always expose 0-2047 or something.

IIUC Windows 8 assumes 101 levels, 0..100 inclusive. (Curiously I didn't
find mention what 0 means there.)

The higher the backlight modulation frequency, the less flicker, and the
higher the range we expose - but, as far as I've understood, fewer
perceivable physical brightness levels. The panel just won't follow the
signal fast enough. So we could just as well expose a smaller range, and
it shouldn't make a difference.

But I'd like that to be a follow-up later on when the dust has settled.

>> @@ -965,6 +1008,18 @@ static void intel_backlight_device_unregister(struct intel_connector *connector)
>>   * XXX: Query mode clock or hardware clock and program PWM modulation frequency
>>   * appropriately when it's 0. Use VBT and/or sane defaults.
>>   */
>> +static inline u32 get_backlight_min(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;
>> +
>> +	BUG_ON(panel->backlight.max == 0);
>> +
>> +	return dev_priv->vbt.backlight.min_brightness *
>> +		panel->backlight.max / 255;
>> +}
>
> Is this the user version or the hw version?  If hw, why not just use
> min_brightness directly?

My understanding is that the VBT value is a coefficient
(min_brightness/255) of the hw max to get the hw value. I may be wrong
as the spec sucks. 255 as the biggest absolute hw min value sounds
awfully small to me though.

BR,
Jani.


>
> -- 
> Jesse Barnes, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center