[Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace

[Jani Nikula]

On Wed, 19 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff at intel.com> wrote:
>> -----Original Message-----
>> From: Stéphane Marchesin [mailto:stephane.marchesin at gmail.com]
>> Sent: Tuesday, November 18, 2014 3:53 PM
>> To: Eoff, Ullysses A
>> Cc: Jani Nikula; Jesse Barnes; intel-gfx at lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
>> 
>> On Tue, Nov 18, 2014 at 3:18 PM, Eoff, Ullysses A
>> <ullysses.a.eoff at intel.com> wrote:
>> > Thanks Jesse for the ack.
>> >
>> > Unfortunately I just learned from Stéphane that there
>> > are certain devices which only support 256 levels, so this
>> > patch would do us no good at solving the real issue for
>> > such devices.
>> >
>> > Why can't we just use a dynamic 1:1 mapping as was
>> > suggested before?  I would vote for that instead.
>> >
>> 
>> Right, from my (consumer's) perspective, a 1:1 mapping is simpler. But
>> the confusing part for me is that (as far as I can see) the current
>> mapping should be 1:1 (because the user and hw ranges are the same),
>> even though it goes through the scale logic. Is the scale() function
>> maybe not the identity? If it isn't, maybe we just need to make it
>> so...
>> 
>
> Yes, if the user and hw ranges are the same, then there will be a
> 1:1 mapping, currently, and no issue.  It's the other case where
> the hw range is smaller than the user range we end up with
> brightness != actual_brightness in sysfs.  The scale logic rounds
> into discrete values of the ranges where multiple user values can
> scale to the same hw value in this case.  Right now, user range is
> [0..max hw] and hw range is [min_hw..max_hw].  If min_hw > 0,
> then we encounter the problem.  The proposal is to set the user
> range to [0..(hw_max - hw_min)].

Some things to consider.

Have you heard of any requirements to support changing backlight PWM
frequency run time? We currently don't support it, and it would require
a fixed range. The backlight class interface does not support changing
max brightness on the fly. Sure, we can implement this later if
required, but we now have most of what's needed for this in place.

The luminance of the backlight is not a linear function of the
brightness value set. Currently a single brightness step has a different
luminance change depending on the absolute value. There's been talk
about letting userspace fix this, but I'm not convinced the userspace
has any chance of abstracting the plethora of hardware out there. As it
happens, the ACPI opregion the driver has access to, does have a lookup
table for this. We could fix this in the driver, but not if we commit to
having 1:1 mapping.

Another thing to consider is that the max value we currently expose is
quite meaningless to the userspace. I question the point of exposing a
range of, say, 0..10000 when in reality you'll only get maybe 100
distinct levels of brightness, depending on the backlight frequency.

An interesting and perhaps counter intuitive detail, the higher the PWM
frequency, i.e. the higher the exposed max, the fewer user
distinguishable levels you can actually get from the backlight. This is
due to the rise and fall times in the backlight following the PWM
signal.

Finally, it seems to me the problem with the scaling boils down to
userspace expecting actual_brightness to always match the brightness it
set. That's the value read back from the hardware. The ABI explicitly
says the brightness stored in the driver may not be the actual
brightness [1]. I don't think there are guarantees that all hardware
would or could maintain the precision either. I think that's broken in
userspace, but we're not supposed to say such things.

Soo... here's an attempt to be constructive after all the whining
above. ;) How about this to always return the same value if the actual
brightness duty cycle in the hardware has not changed? Totally untested,
of course.

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 4d63839bd9b4..8678467d5d83 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1024,7 +1024,12 @@ static int intel_backlight_device_get_brightness(struct backlight_device *bd)
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 
 	hw_level = intel_panel_get_backlight(connector);
-	ret = scale_hw_to_user(connector, hw_level, bd->props.max_brightness);
+	if (hw_level == scale_user_to_hw(connector, bd->props.brightness,
+					 bd->props.max_brightness))
+		ret = bd->props.brightness;
+	else
+		ret = scale_hw_to_user(connector, hw_level,
+				       bd->props.max_brightness);
 
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 	intel_runtime_pm_put(dev_priv);


BR,
Jani.



[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/stable/sysfs-class-backlight


-- 
Jani Nikula, Intel Open Source Technology Center