Re: [PATCH v3 3/4] backlight: pwm_bl: compute brightness of LED linearly to human eye.

[Matthias Kaehlcke <mka@chromium.org>]

On Wed, Jun 12, 2019 at 08:47:57PM +0100, Daniel Thompson wrote:
> On Wed, Jun 12, 2019 at 12:26:42PM -0700, Matthias Kaehlcke wrote:
> > Hi Daniel,
> > 
> > On Wed, Jun 12, 2019 at 12:03:25PM +0100, Daniel Thompson wrote:
> > > On Tue, Jun 11, 2019 at 03:30:19PM -0700, Matthias Kaehlcke wrote:
> > > > On Tue, Jun 11, 2019 at 09:55:30AM -0700, Brian Norris wrote:
> > > > > On Tue, Jun 11, 2019 at 3:49 AM Daniel Thompson
> > > > > <daniel.thompson@linaro.org> wrote:
> > > > > > This is a long standing flaw in the backlight interfaces. AFAIK generic
> > > > > > userspaces end up with a (flawed) heuristic.
> > > > > 
> > > > > Bingo! Would be nice if we could start to fix this long-standing flaw.
> > > > 
> > > > Agreed!
> > > > 
> > > > How could a fix look like, a sysfs attribute? Would a boolean value
> > > > like 'logarithmic_scale' or 'linear_scale' be enough or could more
> > > > granularity be needed?
> > > 
> > > Certainly "linear" (this device will work more or less correctly if the
> > > userspace applies perceptual curves). Not sure about logarithmic since
> > > what is actually useful is something that is "perceptually linear"
> > > (logarithmic is merely a way to approximate that).
> > > 
> > > I do wonder about a compatible string like most-detailed to
> > > least-detailed description. This for a PWM with the auto-generated
> > > tables we'd see something like:
> > > 
> > > cie-1991,perceptual,non-linear
> > > 
> > > For something that is non-linear but we are not sure what its tables are
> > > we can offer just "non-linear".
> > 
> > Thanks for the feedback!
> > 
> > It seems clear that we want a string for the added flexibility. I can
> > work on a patch with the compatible string like description you
> > suggested and we can discuss in the review if we want to go with that
> > or prefer something else.
> 
> Great, other important thing if we did decide to go this route is there
> must be some devices with multiple strings on day 1 (such as the cie-1991
> example above).

Ok, I can add this to the PWM backlight driver (obviously with the
actual handling of the new attribute in the core).

> Whatever we say the ABI is, if we end up with established userspace
> components that strcmp("linear", ...) and there are no early counter
> examples then we could get stuck without the option to add more
> precise tokens as we learn more.

Indeed, we need userspace to understand this isn't necessarily a
'simple' string.

> > > > The new attribute could be optional (it only exists if explicitly
> > > > specified by the driver) or be set to a default based on a heuristic
> > > > if not specified and be 'fixed' on a case by case basis. The latter
> > > > might violate "don't break userspace" though, so I'm not sure it's a
> > > > good idea.
> > > 
> > > I think we should avoid any heuristic! There are several drivers and we
> > > may not be able to work through all of them and make the correct
> > > decision.
> > 
> > Agreed
> > 
> > > Instead one valid value for the sysfs should be "unknown" and this be
> > > the default for drivers we have not analysed (this also makes it easy to
> > > introduce change here).
> > 
> > An "unknown" value sounds good, it allows userspace to just do what it
> > did/would hace done before this attribute existed.
> > 
> > > We should only set the property to something else for drivers that have
> > > been reviewed.
> > > 
> > > There could be a special case for pwm_bl.c in that I'm prepared to
> > > assume that the hardware components downstream of the PWM have a
> > > roughly linear response and that if the user provided tables that their
> > > function is to provide a perceptually comfortable response.
> > 
> > Unfortunately this isn't universally true :(
> > 
> > At least several Chrome OS devices use a linear brightness scale and
> > userspace does the transformation in the animated slider. A quick
> > 'git grep -A10 brightness-levels arch' suggests that there are
> > multiple other devices/platforms using a linear scale.
> 
> Good point.
> 
> Any clue whether the tables are "stupid" (e.g. offer a poor user experience
> with notchy feeling backlight response) or whether they work because
> some of the downstream componentry has a non-linear response?

Sorry, I don't know details about any of these devices, except some of
the Chrome OS ones.

> > We could treat devices with a predefined brightness table as
> > "unknown", unless there is a (new optional) DT property that indicates
> > the type of the scale.
> 
> If we have an "unknown" and we don't know then I guess I just claimed
> that's what we have to do for cases we don't understand.
> 
> For pwm_bl it would be easy to study the table and calculate how far from the
> line the centre point is... although that bringing back heuristics into
> the picture, albeit more useful ones.

True, distinguishing between 'linear' and 'non-linear' shouldn't be a
big deal.

> As I said... I'd be OK for the pwm_bl to take a few liberties because it
> is so different from the fully fledged LED driver drivers but we don't
> need to go crazy ;-)