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

[Eoff, Ullysses A]

> -----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)].

--U. Artie

> Stéphane
> 
> 
> > ----
> > U. Artie
> >
> >> -----Original Message-----
> >> From: Jesse Barnes [mailto:jbarnes at virtuousgeek.org]
> >> Sent: Tuesday, November 18, 2014 9:23 AM
> >> To: Eoff, Ullysses A
> >> Cc: intel-gfx at lists.freedesktop.org; Jani Nikula
> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
> >>
> >> On Tue, 11 Nov 2014 12:30:38 -0800
> >> "U. Artie Eoff" <ullysses.a.eoff at intel.com> wrote:
> >>
> >> > A userspace brightness range that is larger than the hardware
> >> > brightness range does not have a 1:1 mapping and can result
> >> > in brightness != actual_brightness for some values.
> >> >
> >> > Expose a fixed brightness range of [0..1000] to userspace so
> >> > that all values can map 1:1 into the hardware brightness
> >> > range.  This would assume that the hardware range is always
> >> > greater than 1000, otherwise we're right back to the original
> >> > issue.
> >> >
> >> > This patch is based on Jani Nikula's proposed change in the
> >> > referenced ML thread, except use the range [0..1000] instead;
> >> > which was recommended by Jesse Barnes for smoother backlight
> >> > transitions.
> >> >
> >> > Reference: http://lists.freedesktop.org/archives/intel-gfx/2014-November/055221.html
> >> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff at intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_panel.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> >> > index 4d63839..f74f5f2 100644
> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> > @@ -1057,7 +1057,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 = 1000;
> >> >     props.brightness = scale_hw_to_user(connector,
> >> >                                         panel->backlight.level,
> >> >                                         props.max_brightness);
> >>
> >> Yeah looks ok to me.  I guess Jani has to ack it too.
> >>
> >> Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> >>
> >> --
> >> Jesse Barnes, Intel Open Source Technology Center