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

[Eoff, Ullysses A]

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> Sent: Thursday, November 20, 2014 12:58 AM
> To: Eoff, Ullysses A; Stéphane Marchesin
> Cc: Jesse Barnes; intel-gfx at lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
> 
> On Wed, 19 Nov 2014, "Eoff, Ullysses A" <ullysses.a.eoff at intel.com> wrote:
> > Jani,
> >
> > First of all, thank you for your explanation.  I was unaware of the
> > motivations behind the current implementation.  You are exactly
> > right that the whole reason for all this is that userspace is expecting
> > actual_brightness to always match the brightness it set.
> >
> > I would like to point out that I never meant to suggest there was a
> > "functional" bug in the driver.  And my motivations were more about
> > testability via sysfs than anything else and the assumption that brightness
> > was supposed to always equal actual_brightness.  I really should have
> > been more explicit in pointing this out much sooner if it wasn't clear.
> 
> Maybe you missed it, but we did have a reported bug [1] which was fixed
> (or worked around) by your
> 
> commit 673e7bbdb3920b62cfc6c710bea626b0a9b0f43a
> Author: U. Artie Eoff <ullysses.a.eoff at intel.com>
> Date:   Mon Sep 29 15:49:32 2014 -0700
> 
>     drm/i915: intel_backlight scale() math WA
> 
> and there was the same discussion about scaling problems as here. It's a
> real issue, no need for regrets on your part.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=85861
> 

I was not aware of this other report.  But yeah, that patch was a
byproduct of the assumption that brightness always  matches
actual_brightness. It was that assumption that led me to discover
the rounding errors... so the assumption isn't all bad ;)

> > Thanks for providing a link to the documentation... I wish I had been
> > more diligent in looking for this in the first place.  I'm always more
> > inclined to defer to the documentation.  And in light of it, I stand
> > corrected.
> >
> > Although your new patch would likely work, I don't think it's necessary
> > anymore to make brightness==actual_brightness always hold true; since
> > testing for that is the incorrect thing to do in the first place (based on
> > the documentation).  Nonetheless, testing brightness from userspace will
> > just have to get a little more creative.
> 
> I don't think there's anything really horribly wrong with the patch, so
> I'm starting to think maybe we should just apply it. I assume it would
> help your scenario too. If there's still userspace out there that makes
> the assumption anyway, and fails because of it, we may not even have a
> choice.
> 
> Thoughts?
> 

Right, there's really no harm in having your patch.  It will definitely help
solve the scenario I'm working on.  I'm fine either way even though
I know the assumption shouldn't be made from userspace now.

> > Apologies for all the noise... now I'll go make some elsewhere. ;)
> 
> Oh, never mind about that.
> 
> 

Thank you Jani.

U. Artie

> BR,
> Jani.
> 
> >
> > U. Artie
> >
> >> -----Original Message-----
> >> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> >> Sent: Wednesday, November 19, 2014 12:57 AM
> >> To: Eoff, Ullysses A; Stéphane Marchesin
> >> Cc: Jesse Barnes; intel-gfx at lists.freedesktop.org
> >> Subject: RE: [Intel-gfx] [PATCH] drm/i915: expose a fixed brightness range to userspace
> >>
> >> 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
> 
> --
> Jani Nikula, Intel Open Source Technology Center