On Wed, 19 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff@xxxxxxxxx> wrote: >> -----Original Message----- >> From: Stéphane Marchesin [mailto:stephane.marchesin@xxxxxxxxx] >> Sent: Tuesday, November 18, 2014 3:53 PM >> To: Eoff, Ullysses A >> Cc: Jani Nikula; Jesse Barnes; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> Subject: Re: [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@xxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
