Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Jul 2023 01:07:30 +0000
To:     Hans de Goede <hdegoede@redhat.com>
From:   =?utf-8?Q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>
Cc:     acpi4asus-user@lists.sourceforge.net,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hwmon@vger.kernel.org
Subject: Re: [PATCH v4 1/1] platform/x86: asus-wmi: add support for ASUS screenpad
Message-ID: <MFcnkX194DPlx9DJUNWx43ia7ew825I6K1k8hOoUYImBv390MzOdWngMLTzr5g4iv_9o9x8qE2FqoVcKUcN6s6lHdbIzfj3eDXQLgtU3w9o=@protonmail.com>
In-Reply-To: <350cfb54-f435-4482-5a40-18d4358bf747@redhat.com>
References: <20230630041743.911303-1-luke@ljones.dev> <20230630041743.911303-2-luke@ljones.dev> <974093b4-5dac-dc29-8f86-304eb5c6c19b@redhat.com> <39e6044b960c1099ecc34d76f78aad12496b23e2.camel@ljones.dev> <350cfb54-f435-4482-5a40-18d4358bf747@redhat.com>
Feedback-ID: 20568564:user:proton
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi


2023. j=C3=BAlius 11., kedd 11:42 keltez=C3=A9ssel, Hans de Goede <hdegoede=
@redhat.com> =C3=ADrta:

> [...]
> >>
> >> If settings below 60 are no good, then the correct way to handle
> >> this is to limit the range to 0 - (255-60) and add / substract
> >> 60 when setting / getting the brightness.
> >>
> >> E.g. do something like this:
> >>
> >> #define SCREENPAD_MIN_BRIGHTNESS        60
> >> #define SCREENPAD_MAX_BRIGHTNESS        255
> >>
> >>         props.max_brightness =3D SCREENPAD_MAX_BRIGHTNESS -
> >> SCREENPAD_MIN_BRIGHTNESS;
> >>
> >> And in update_screenpad_bl_status() do:
> >>
> >>         ctrl_param =3D bd->props.brightness + SCREENPAD_MIN_BRIGHTNESS=
;
> >>
> >> And when reading the brightness substract SCREENPAD_MIN_BRIGHTNESS,
> >> clamping to a minimum value of 0.
> >>
> >> This avoids a dead-zone in the brightness range between 0-60 .
> >
> > Hi Hans, I think this is the wrong thing to do.
> >
> > The initial point of that first `brightness =3D 60;` is only to set it =
to
> > an acceptable brightness on boot. We don't want to prevent the user
> > from going below that brightness at all - this is just to ensure the
> > screen is visible on boot if the brightness was under that value, and
> > it is usually only under that value if it was set to off before
> > shutdown/reboot.
> >
> > It's not to try and put the range between 60-255, it's just to make the
> > screen visible on boot if it was off previously. The folks who have
> > tested this have found that this is the desired behaviour they expect.
>=20
> I see.
>=20
> So if I understand things correctly then 60 is a good default,
> but the screen can go darker and still be usable.
>=20
> But 1 leads to an unusable screen, so we still need
> a SCREENPAD_MIN_BRIGHTNESS to avoid the screen being able
> to go so dark that it is no longer usable and then still
> move the range a bit, but just not by 60, but by some
> other number (something in the 10-30 range I guess?)
>=20
> Combined with adding a:
>=20
> #define SCREENPAD_DEFAULT_BRIGHTNESS        60
>=20
> And at boot when the read back brightness <
> SCREENPAD_MIN_BRIGHTNESS set it to SCREENPAD_DEFAULT_BRIGHTNESS.
>=20
> We really want to avoid users to be able to set an unusable
> low brightness level. As mentioned in the new panel brightness
> API proposal:
>=20
> https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@re=
dhat.com/
>=20
> "3. The meaning of 0 is not clearly defined, it can be either off,
>    or minimum brightness at which the display is still readable
>   (in a low light environment)"
>=20
> and the plan going forward is to:
>=20
> "Unlike the /sys/class/backlight/foo/brightness this brightness property
> has a clear definition for the value 0. The kernel must ensure that 0
> means minimum brightness (so 0 should _never_ turn the backlight off).
> If necessary the kernel must enforce a minimum value by adding
> an offset to the value seen in the property to ensure this behavior."
>=20
> So I really want to see this new backlight driver implement the
> new planned behavior for 0 from the start, with 0 meaning minimum
> *usable* brightness.
>=20
> Regards,
>=20
> Hans


Sorry for hijacking this thread, but then how can I turn backlight off?
I quite liked how I was able to turn my laptop display (almost) completely =
off
with the brightness hotkeys / brightness slider in gnome-shell, and I was q=
uite
annoyed when this was changed in gnome-settings-daemon and forced the minim=
um
brightness to be 1% of max_brightness.

(https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/merge_requests/17)

Also, "minimum brightness at which the display is still readable" is not re=
ally
well-defined, so it can (will) happen that the minimum brightness values do=
n't match,
so it is theoretically possible that I cannot set both my laptop panel and =
external
monitor to the same desired brightness level. Or am I missing something?


Regards,
Barnab=C3=A1s P=C5=91cze