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

[Daniel Thompson]

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@xxxxxxxxxx> 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).

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.


> > > 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?


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

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 ;-)


Daniel.