[Intel-gfx] [PATCH] drm/i915: intel_backlight scale() math WA v2

[Jesse Barnes]

On Mon, 10 Nov 2014 16:15:57 +0200
Jani Nikula <jani.nikula at linux.intel.com> wrote:

> On Mon, 10 Nov 2014, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> > On Sat, 08 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff at intel.com>
> > wrote:
> >> On 09/24/2014 10:42 AM, Eoff, Ullysses A wrote:
> >>>> -----Original Message-----
> >>>> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org]
> >>>> On Behalf Of Jani Nikula Sent: Wednesday, September 24, 2014
> >>>> 10:08 AM To: Hans de Goede; Joe Konno;
> >>>> intel-gfx at lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH]
> >>>> drm/i915: intel_backlight scale() math WA v2
> >>>>
> >>>> On Wed, 24 Sep 2014, Hans de Goede <hdegoede at redhat.com> wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 09/24/2014 05:54 PM, Joe Konno wrote:
> >>>>>> From: Joe Konno <joe.konno at intel.com>
> >>>>>>
> >>>>>> Improper truncated integer division in the scale() function
> >>>>>> causes actual_brightness != brightness. This (partial)
> >>>>>> work-around should be sufficient for a majority of use-cases,
> >>>>>> but it is by no means a complete solution.
> >>>>>>
> >>>>>> TODO: Determine how best to scale "user" values to "hw"
> >>>>>> values, and vice-versa, when the ranges are of different
> >>>>>> sizes. That would be a buggy scenario even with this
> >>>>>> work-around.
> >>>>>>
> >>>>>> The issue was introduced in the following (v3.17-rc1) commit:
> >>>>>>
> >>>>>>     6dda730 drm/i915: respect the VBT minimum backlight
> >>>>>> brightness
> >>>>>>
> >>>>>> v2: (thanks to Chris Wilson) clarify commit message, use
> >>>>>> rounded division macro
> >>>>> I wonder why do scaling at all, why not simply shift hw_min -
> >>>>> hw_max range to 0 - (hw_max - hw_min) range and set
> >>>>> max_brightness as seen by userspace to (hw_max - hw_min) ?
> >>>> Mostly in preparation for a future where we expose an arbitrary
> >>>> range, say 0..100 or 0..255 to the userspace.
> >>>>
> >>> The problem with this scaling method is that scaling from user
> >>> level to hw level and back to user level is ambiguous since there
> >>> isn't a 1:1 mapping between the user range and the hw range.
> >>>
> >>> On the other hand, this patch does fix a bug in the currently
> >>> used method (scaling). That, at least, is an improvement
> >>> nonetheless.
> >>>
> >>> U. Artie
> >> Apologies for resurrecting an old thread.  But I think we still
> >> need to address
> >> this issue about not having a 1:1 mapping between user and hw
> >> levels.
> >>
> >> Right now, the problem is that the user range is larger than the hw
> >> range which
> >> results in one or more user levels mapping to the same hw level.
> >> And when userspace requests one of those levels, the result that
> >> is reported back to userspace might not be the same as what was
> >> requested.  Take for example, on my system the hw range is [398,
> >> 7812] and the user range is [0, 7812]. Suppose
> >> userspace requests level 7017.  This maps to hw level 7058.  And
> >> when userspace requests the current level, 7018 is reported back
> >> (+1 from what was originally requested).  In fact, with these
> >> particular ranges, there are exactly
> >> 398 values that this occurs.
> >>
> >> This problem will be compounded the larger the difference in
> >> length of the discrete ranges; so long as user range > hw range.
> >>
> >> Hans' solution would fix this problem, giving 1:1 mapping from hw
> >> to user levels.
> >>
> >> Jani's [future] solution would work too, since exposing a smaller
> >> range to userspace than the hw range would isolate the non 1:1
> >> mapping inside the driver.
> >
> > I think we should just pick an arbitrary range, say 0..100, and be
> > done with it. It's not like you'd be able to get much more than 100
> > distinct brightness levels out of the backlight anyway, no matter
> > what the PWM settings.
> >
> > BR,
> > Jani.
> 
> PS. This (totally untested) patch should do it:
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_panel.c index b001c90312e7..a6680081415b
> 100644 --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1035,7 +1035,7 @@ static int
> intel_backlight_device_register(struct intel_connector *connector)
>  	 * Note: Everything should work even if the backlight device
> max
>  	 * presented to the userspace is arbitrarily chosen.
>  	 */
> -	props.max_brightness = panel->backlight.max;
> +	props.max_brightness = 100;
>  	props.brightness = scale_hw_to_user(connector,
>  					    panel->backlight.level,
>  					    props.max_brightness);

100% agreed on exposing a fixed range.  But iirc Keith did some playing
around with fading in and out of backlights and found that we needed
about 1000 levels to make it smooth (definitely possible on some
platforms, though not all).  So my only nitpick would be that we have a
range that allows a bit more precision.

Thanks,
Jesse