Re: [PATCH] drm/atomic: Take the atomic toys away from X

[Yves-Alexis Perez]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On Fri, 2020-05-08 at 11:54 +0200, Greg KH wrote:
> > Hi Daniel and Greg (especially). It seems that this patch was never
> > applied to
> > stable, maybe it fell through the cracks?
> 
> What patch is "this patch"?

Sorry, the patch was in the mail I was replying to:

commit 26b1d3b527e7bf3e24b814d617866ac5199ce68d
Author: Daniel Vetter 
Date:   Thu Sep 5 20:53:18 2019 +0200

drm/atomic: Take the atomic toys away from X

> 
> > It doesn't apply as-is in 4.19 branch but a small change in the context
> > makes
> > it apply. I'm experiencing issues with lightdm and vt-switch in Debian
> > Buster
> > (which has a 4.19 kernel) so I'd appreciate if the patch was included in
> > at
> > least that release.
> 
> What is the git commit id of the patch in Linus's tree?  If you have a
> working backport, that makes it much easier than hoping I can fix it
> up...

The commit id is in Linus tree is 26b1d3b527e7bf3e24b814d617866ac5199ce68d. To
apply properly 69fdf4206a8ba91a277b3d50a3a05b71247635b2 would need to be
cherry-picked as well but it wasn't marked for stable so I didn't bother and
only fixed the context. Here's the backport to 4.19, compile and runtime
tested. It does fix the issue for me (like it did on mainline).

So I guess
Tested-By: Yves-Alexis Perez 

commit 8a99914f7b539542622dc571c82d6cd203bddf64
Author: Daniel Vetter 
Date:   Thu Sep 5 20:53:18 2019 +0200

drm/atomic: Take the atomic toys away from X

The -modesetting ddx has a totally broken idea of how atomic works:
- doesn't disable old connectors, assuming they get auto-disable like
  with the legacy setcrtc
- assumes ASYNC_FLIP is wired through for the atomic ioctl
- not a single call to TEST_ONLY

Iow the implementation is a 1:1 translation of legacy ioctls to
atomic, which is a) broken b) pointless.

We already have bugs in both i915 and amdgpu-DC where this prevents us
from enabling neat features.

If anyone ever cares about atomic in X we can easily add a new atomic
level (req->value == 2) for X to get back the shiny toys.

Since these broken versions of -modesetting have been shipping,
there's really no other way to get out of this bind.

v2:
- add an informational dmesg output (Rob, Ajax)
- reorder after the DRIVER_ATOMIC check to avoid useless noise (Ilia)
- allow req->value > 2 so that X can do another attempt at atomic in
  the future

v3: Go with paranoid, insist that the X should be first (suggested by
Rob)

Cc: Ilia Mirkin 
References: https://gitlab.freedesktop.org/xorg/xserver/issues/629
References: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/180
References: abbc0697d5fb ("drm/fb: revert the i915 Actually configure
untiled displays from master")
Cc: Maarten Lankhorst 
Reviewed-by: Maarten Lankhorst  (v1)
Reviewed-by: Nicholas Kazlauskas  (v1)
Cc: Michel Dänzer 
Cc: Alex Deucher 
Cc: Adam Jackson 
Acked-by: Adam Jackson 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Rob Clark 
Acked-by: Rob Clark 
Cc: sta...@vger.kernel.org
Signed-off-by: Daniel Vetter 
Link: 
https://patchwork.freedesktop.org/patch/msgid/20190905185318.31363-1-daniel.vet...@ffwll.ch

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ba129b64b61f..b92682f037b2 100644
- --- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -321,7 +321,12 @@ drm_setclientcap(struct drm_device *dev, void *data,
struct drm_file *file_priv)
case DRM_CLIENT_CAP_ATOMIC:
if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
return -EINVAL;
- - if (req->value > 1)
+   /* The modesetting DDX has a totally broken idea of atomic. */
+   if (current->comm[0] == 'X' && req->value == 1) {
+   pr_info("broken atomic modeset userspace detected,
disabling atomic\n");
+   return -EOPNOTSUPP;
+   }
+   if (req->value > 2)
return -EINVAL;
file_priv->atomic = req->value;
file_priv->universal_planes = req->value;


- -- 
Yves-Alexis
-BEGIN PGP SIGNATURE-

iQEzBAEBCAAdFiEE8vi34Qgfo83x35gF3rYcyPpXRFsFAl61SZUACgkQ3rYcyPpX
RFvPfwgAzMyFqiV592RBKu4tx5Ivqa4EC/1OdR8DojyQlw4AP0bYc+4O67xYbvt5
r4JXuGbSu+jW/29V+2t8ZlLi4S7bAvAo2DEhuBdGVzdmD4gM9EC+69oqVeZWWZTm
VUldLrRO8BoPxv14lX/K/kU/o5Pv8ivRoEKs385JU2p1AxNGJI2UUmIXKGtk7Cu/
Fu/flH627RHjTRk2QYsemqHqSAONaHYuSiYm783hz4wYiPOZQdGvS+ifHwMAhUqh
scaCxv+pBOxaLuZAfUXFjDX+qAcuJCxaP9bT4soweIpYqjzcAdBSmny0+OLOnie/
HcBzKwpgitKR/cVadZgb0US1oHeo2A==
=l8z1
-END PGP SIGNATURE-
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Take the atomic toys away from X

[Yves-Alexis Perez]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

On Thu, 2019-09-05 at 20:53 +0200, Daniel Vetter wrote:
> The -modesetting ddx has a totally broken idea of how atomic works:
> - doesn't disable old connectors, assuming they get auto-disable like
>   with the legacy setcrtc
> - assumes ASYNC_FLIP is wired through for the atomic ioctl
> - not a single call to TEST_ONLY
> 
> Iow the implementation is a 1:1 translation of legacy ioctls to
> atomic, which is a) broken b) pointless.
> 
> We already have bugs in both i915 and amdgpu-DC where this prevents us
> from enabling neat features.
> 
> If anyone ever cares about atomic in X we can easily add a new atomic
> level (req->value == 2) for X to get back the shiny toys.
> 
> Since these broken versions of -modesetting have been shipping,
> there's really no other way to get out of this bind.

Hi Daniel and Greg (especially). It seems that this patch was never applied to
stable, maybe it fell through the cracks?

It doesn't apply as-is in 4.19 branch but a small change in the context makes
it apply. I'm experiencing issues with lightdm and vt-switch in Debian Buster
(which has a 4.19 kernel) so I'd appreciate if the patch was included in at
least that release.

Regards,
- -- 
Yves-Alexis
-BEGIN PGP SIGNATURE-

iQEzBAEBCAAdFiEE8vi34Qgfo83x35gF3rYcyPpXRFsFAl61ITAACgkQ3rYcyPpX
RFvlaAf9HZ0DTX1fAkNeNFoAgn4pFztnFq0fAwGj5iVIL4q6upE1wE3E8cDgUHeT
maQQvL3YHFXjgzgDHYNIuUMipFE1Djymoy+EB4ZoOftqsJ4CPy4pCMUAh57u7BrV
T+eBtj4n0wY0SgvoPism3QdbxY7CLLgCMJKLNrCPlkDCdJyGsZX9RIgfqvbkGM36
ftwBKcyy1iW5cAv10ehiXi/1zszA8bx2gULim3abcSjjz12ckNvBPy/BDvfFx19V
8cGgG3qD9PLmxRl80H1/mX30Ddw8Md5Fu7I/ndh3EGXLu8p8zod0rQVCQjAEW4X4
ew4tajDD2l9vWzN0sZIlyjq9fNgXBw==
=lPBO
-END PGP SIGNATURE-
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 0/4] Fix Win8 backlight issue

[Yves-Alexis Perez]

On sam., 2013-10-12 at 00:10 +0200, Rafael J. Wysocki wrote:
> On Friday, October 11, 2013 12:42:43 PM Josh Boyer wrote:
> > On Fri, Oct 11, 2013 at 9:27 AM, Aaron Lu  wrote:
> > > v5:
> > > 1 Introduce video.use_native_backlight module parameter and set its
> > >   value to false by default as suggested by Rafael. For Win8 systems
> > >   which have broken ACPI video backlight control, the parameter can be
> > >   set to 1 in kernel cmdline to skip registering ACPI video's backlight
> > >   interface. Due to this change, the acpi_video_verify_backlight_support
> > >   is moved from video_detect.c to video.c - patch 3/4;
> > 
> > That's a fairly untenable position for distro kernels to be in.  They
> > now have to ask every user that reports an issue with the backlight to
> > try setting that option on the command line.  While I appreciate the
> > setting breaks things for some people, doesn't the Win8 issue impact
> > far more people?  Shouldn't it be defaulted to true?
> 
> Well, we have a rule in the kernel not to introduce regressions for users even
> if they are minority.

Well, for some users, the regression actually happened when support for
Win8 OSI call was introduced.

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 0/4] Fix Win8 backlight issue

[Yves-Alexis Perez]

On sam., 2013-10-12 at 01:27 +0200, Rafael J. Wysocki wrote:
> If we are to use a Kconfig option, why don't we use one instead of rather than
> in addition to a command line option?  Say, we have
> CONFIG_ACPI_VIDEO_WIN8_WORKAROUND and if that is set, the code will work like
> the previous version of the Aaron's patchset (the one without
> video.use_native_backlight)?

Doesn't this mean user will have to rebuild distro kernel in order to
test behavior?

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

[Yves-Alexis Perez]

On ven., 2013-09-27 at 15:05 -0300, Henrique de Moraes Holschuh wrote:
> On Fri, 27 Sep 2013, Yves-Alexis Perez wrote:
> > On ven., 2013-09-27 at 12:20 -0300, Henrique de Moraes Holschuh wrote:
> > > Some testing on a *60 (T60,X60...) would also be best, I cannot test
> > > this on
> > > my T43.
> > > 
> > > Anyway, the code itself looks fine, so:
> > 
> > I can test on T61, would that help?
> 
> I think so.
> 
Ok, just tested on my T61 with Intel graphics.

I've checked that on Linux 3.12
(6cac446bd37d9381815fe4c2b0e7b1fd1085000c), brightness keys work fine
like they do in 3.2.

Then I've applied the patchset. The brightness keys still work, I still
have 16 levels.

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/4] thinkpad-acpi: fix handle locate for video and query of _BCL

[Yves-Alexis Perez]

On ven., 2013-09-27 at 12:20 -0300, Henrique de Moraes Holschuh wrote:
> Some testing on a *60 (T60,X60...) would also be best, I cannot test
> this on
> my T43.
> 
> Anyway, the code itself looks fine, so:

I can test on T61, would that help?

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/3] ACPI / video: Do not register backlight if win8 and native interface exists

[Yves-Alexis Perez]

On mar., 2013-09-17 at 17:23 +0800, Aaron Lu wrote:
> According to Matthew Garrett, "Windows 8 leaves backlight control up
> to individual graphics drivers rather than making ACPI calls itself.
> There's plenty of evidence to suggest that the Intel driver for
> Windows 8 doesn't use the ACPI interface, including the fact that
> it's broken on a bunch of machines when the OS claims to support
> Windows 8.  The simplest thing to do appears to be to disable the
> ACPI backlight interface on these systems".
> 
> So for Win8 systems, if there is native backlight control interface
> registered by GPU driver, ACPI video will not register its own. For
> users who prefer to keep ACPI video's backlight interface, the
> existing
> kernel cmdline option acpi_backlight=video can be used.
> 
> This patch is an evolution from previous work done by Matthew Garrett,
> Chun-Yi Lee, Seth Forshee and Rafael J. Wysocki.
> 
> Signed-off-by: Aaron Lu 
Tested-by: Yves-Alexis Perez 
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] ACPI / video: seperate backlight control and event interface

[Yves-Alexis Perez]

On mar., 2013-09-17 at 17:23 +0800, Aaron Lu wrote:
> The backlight control and event delivery functionality provided by
> ACPI
> video module is mixed together and registered all during video device
> enumeration time. As a result, the two functionality are also removed
> together on module unload time or by the acpi_video_unregister
> function.
> The two functionalities are actually independent and one may be useful
> while the other one may be broken, so it is desirable to seperate the
> two functionalities such that it is clear and easy to disable one
> functionality without affecting the other one.
> 
> APIs to selectively remove backlight control interface and/or event
> delivery functionality can be easily added once needed.
> 
> Signed-off-by: Aaron Lu 
Tested-by: Yves-Alexis Perez 
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] backlight: introduce backlight_device_registered

[Yves-Alexis Perez]

On mar., 2013-09-17 at 17:23 +0800, Aaron Lu wrote:
> Introduce a new API for modules to query if a specific type of
> backlight
> device has been registered. This is useful for some backlight device
> provider module(e.g. ACPI video) to know if a native control
> interface(e.g. the interface created by i915) is available and then do
> things accordingly(e.g. avoid register its own on Win8 systems).
> 
> Signed-off-by: Aaron Lu 
Tested-by: Yves-Alexis Perez 
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] Fix Win8 backlight issue

[Yves-Alexis Perez]

On mar., 2013-09-17 at 17:23 +0800, Aaron Lu wrote:
> v1 has the subject of "Rework ACPI video driver" and is posted here:
> http://lkml.org/lkml/2013/9/9/74
> Since the objective is really to fix Win8 backlight issues, I changed
> the subject in this version, sorry about that.
> 
> This patchset has three patches, the first introduced a new API named
> backlight_device_registered in backlight layer that can be used for
> backlight interface provider module to check if a specific type
> backlight
> interface has been registered, see changelog for patch 1/3 for
> details.
> Then patch 2/3 does the cleanup to sepeate the backlight control and
> event delivery functionality in the ACPI video module and patch 3/3
> solves some Win8 backlight control problems by avoiding register ACPI
> video's backlight interface if:
> 1 Kernel cmdline option acpi_backlight=video is not given;
> 2 This is a Win8 system;
> 3 Native backlight control interface exists.

I've tested this on my x230 (using pure UEFI with CSM disabled). As far
as I can tell, it works as I would expect:

- brightness keys work in initramfs, in console after boot, in lightdm
prompt and in Xfce (wether or not xfce4-power-manager is running).

I don't have brightness notifications (from xfce4-power-manager) but I
don't usually need them so I'm fine with that.

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

[Yves-Alexis Perez]

On mer., 2013-09-11 at 08:45 +, Matthew Garrett wrote:
> On Wed, 2013-09-11 at 11:45 +0300, Jani Nikula wrote:
> 
> > Before plunging forward, have you observed any difference between the
> > boot modes? We have reports [1] that the backlight behaviour is
> > different with UEFI vs. UEFI+CSM or legacy boot. So I'm wondering if the
> > acpi_gbl_osi_data >= ACPI_OSI_WIN_8 check in patch 2/2 is the whole
> > story.
> > 
> > Further, if we tell the BIOS we're Windows 8 to use the tested BIOS code
> > paths, what guarantees do we have of UEFI+CSM or legacy boots working?
> 
> We have no evidence of Windows behaving differently based on the exposed
> firmware type.
> 
I do have an X230 which boots fine using pure UEFI or UEFI+CSM (and I
guess I can try booting it with a grml for pure legacy), so I can do
tests if needed.

Regards,
-- 
Yves-Alexis

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mer., 2013-07-17 at 10:51 -0500, Felipe Contreras wrote:
> On Sat, Jun 22, 2013 at 4:46 PM, Yves-Alexis Perez  wrote:
> > Before Linux support for acpi_osi("Windows 2012") (and when booting with
> > acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> > just fine, whether in console, in the display manager or in my desktop
> > environment (Xfce). xfce4-power-manager just needs to be told that the
> > brightness keys are already handled and it doesn't need to do anything.
> 
> How do you tell xfce4-power-manager that?

xfconf-query -c xfce4-power-manager
-p /xfce4-power-manager/change-brightness-on-key-events -s false

You might have to create the key before. See
https://bugzilla.xfce.org/show_bug.cgi?id=7541 for more information.

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mer., 2013-07-17 at 10:51 -0500, Felipe Contreras wrote:
> On Sat, Jun 22, 2013 at 4:46 PM, Yves-Alexis Perez  
> wrote:
> > Before Linux support for acpi_osi("Windows 2012") (and when booting with
> > acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> > just fine, whether in console, in the display manager or in my desktop
> > environment (Xfce). xfce4-power-manager just needs to be told that the
> > brightness keys are already handled and it doesn't need to do anything.
> 
> How do you tell xfce4-power-manager that?

xfconf-query -c xfce4-power-manager
-p /xfce4-power-manager/change-brightness-on-key-events -s false

You might have to create the key before. See
https://bugzilla.xfce.org/show_bug.cgi?id=7541 for more information.

Regards,
-- 
Yves-Alexis
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130717/10d0c0cc/attachment-0001.pgp>

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mar., 2013-06-25 at 22:33 +0100, Matthew Garrett wrote:
> > I was referring to “standardize the behaviour by leaving up to
> > userspace”. A lot of thinkpads (for example) (all the pre-windows 8
> > ones) have a perfectly working ACPI backlight interface.
> 
> And this patchset won't alter their behaviour.

Sorry if I was unclear and if my mail implied that. It was about your
remark later in the thread (and the mail from Daniel Vetter)
> 
> > Also, if the kernel has no way of knowing which levels work, I fail to
> > see how userspace can do better.
> 
> It can't. That's why this patchset disables the ACPI interface on 
> Windows 8 systems.
> 
> > I understand that switching to intel_backlight instead of acpi_video0
> > follows what Windows 8 recommends but for me it looks orthogonal to the
> > fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
> > mean, it's not the first time firmware people break some kernel
> > behavior. I know it's usually not easy to contact them, but shouldn't
> > those methods be fixed, instead of somehow blindly switching to graphic
> > drivers?
> 
> No. The correct answer to all firmware issues is "Are we making the same 
> firmware calls as the version of Windows that this hardware thinks it's 
> running". If Windows 8 doesn't make these calls, we shouldn't make these 
> calls.

But if that introduce regressions, shouldn't workarounds be found then?
Sorry if I keep repeating that but brightness keys handling in-kernel is
quite a useful feature and losing it (because of the “behave exactly
like Windows 8 kernel” policy) is indeed a regression.
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mar., 2013-06-25 at 22:14 +0100, Matthew Garrett wrote:
> On Tue, Jun 25, 2013 at 11:10:11PM +0200, Yves-Alexis Perez wrote:
> > On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> > > I agree, we should standardise the behaviour. And the only way we can 
> > > standardise the behaviour is to leave it up to userspace.
> > > 
> > It's pretty clear we disagree on this and that my opinion won't really
> > matter here. But letting userspace handle that just means broken
> > functionality for those who have the chance (apparently) to have an ACPI
> > backlight interface.
> 
> Which, as we've already established, you don't - Lenovo broke it. Your 
> Thinkpad claims to have 100 available levels, and most of them don't 
> work. The kernel has no way of knowing which levels work and which 
> don't, so leaving this up to the kernel won't actually fix your system 
> either.

I was referring to “standardize the behaviour by leaving up to
userspace”. A lot of thinkpads (for example) (all the pre-windows 8
ones) have a perfectly working ACPI backlight interface.

Also, if the kernel has no way of knowing which levels work, I fail to
see how userspace can do better.

I understand that switching to intel_backlight instead of acpi_video0
follows what Windows 8 recommends but for me it looks orthogonal to the
fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
mean, it's not the first time firmware people break some kernel
behavior. I know it's usually not easy to contact them, but shouldn't
those methods be fixed, instead of somehow blindly switching to graphic
drivers?
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> On Tue, Jun 25, 2013 at 10:43:57PM +0200, Yves-Alexis Perez wrote:
> > On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> > > Right, the kernel has special-casing to hook the backlight keys up to 
> > > the ACPI backlight control. This is an awful thing, because there's no 
> > > way to detect this case other than parsing a single driver-specific 
> > > module parameter.
> > 
> > I'm not sure what that means. To detect what case exactly? That the
> > brightness is handled by video.ko?
> 
> That the kernel will automatically handle backlight key presses.

Ok, so for detection by userspace? hal managed to do that just fine, it
seems that upower doesn't, for some reason.

> The behaviour is already inconsistent. If you have an ACPI backlight 
> interface, hitting the keys makes your brightness change without any 
> userspace help. If you don't, it doesn't.

At least on the same (class of) hardware it always behaves the same.
> 
> > And in the end, the user just want the brightness keys to correctly
> > handle the brightness, full stop. Having multiple brightness daemons
> > using different policies on different hardware is a nightmare for the
> > end user, imho. From a user point of view, having it handled all in the
> > kernel works really pretty fine and is completely transparent (I have to
> > admit that from that point of view, it was even better when it was
> > handled by the EC but those times seem long gone).
> 
> I agree, we should standardise the behaviour. And the only way we can 
> standardise the behaviour is to leave it up to userspace.
> 
It's pretty clear we disagree on this and that my opinion won't really
matter here. But letting userspace handle that just means broken
functionality for those who have the chance (apparently) to have an ACPI
backlight interface.

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:
> 
> > Before Linux support for acpi_osi("Windows 2012") (and when booting with
> > acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> > just fine, whether in console, in the display manager or in my desktop
> > environment (Xfce). xfce4-power-manager just needs to be told that the
> > brightness keys are already handled and it doesn't need to do anything.
> 
> Right, the kernel has special-casing to hook the backlight keys up to 
> the ACPI backlight control. This is an awful thing, because there's no 
> way to detect this case other than parsing a single driver-specific 
> module parameter.

I'm not sure what that means. To detect what case exactly? That the
brightness is handled by video.ko?
> 
> Could this functionality be duplicated across other backlight drivers? 
> Not easily. The ACPI driver receives keypresses and performs backlight 
> control. The i915 driver doesn't receive keypresses. We could easily tie 
> certain keycodes into backlight events, but which backlight should they 
> control? You're really starting to get into the kind of complex policy 
> decision that's best left to userspace, which is where it should have 
> been to begin with.
> 
Well, I get the reasoning, but I'm not sure I agree. That means
userspace behavior is inconsistent depending on who does it
(gnome-power-manager, gnome-setting-daemon, whatever), and it usually
means there's nothing handling the brightness before those are running,
not to mention people not running them because they don't run a full
blown desktop environment (until someone starts thinking it's a good
idea to handle brightness in systemd).

And in the end, the user just want the brightness keys to correctly
handle the brightness, full stop. Having multiple brightness daemons
using different policies on different hardware is a nightmare for the
end user, imho. From a user point of view, having it handled all in the
kernel works really pretty fine and is completely transparent (I have to
admit that from that point of view, it was even better when it was
handled by the EC but those times seem long gone).

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mar., 2013-06-25 at 22:33 +0100, Matthew Garrett wrote:
> > I was referring to ?standardize the behaviour by leaving up to
> > userspace?. A lot of thinkpads (for example) (all the pre-windows 8
> > ones) have a perfectly working ACPI backlight interface.
> 
> And this patchset won't alter their behaviour.

Sorry if I was unclear and if my mail implied that. It was about your
remark later in the thread (and the mail from Daniel Vetter)
> 
> > Also, if the kernel has no way of knowing which levels work, I fail to
> > see how userspace can do better.
> 
> It can't. That's why this patchset disables the ACPI interface on 
> Windows 8 systems.
> 
> > I understand that switching to intel_backlight instead of acpi_video0
> > follows what Windows 8 recommends but for me it looks orthogonal to the
> > fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
> > mean, it's not the first time firmware people break some kernel
> > behavior. I know it's usually not easy to contact them, but shouldn't
> > those methods be fixed, instead of somehow blindly switching to graphic
> > drivers?
> 
> No. The correct answer to all firmware issues is "Are we making the same 
> firmware calls as the version of Windows that this hardware thinks it's 
> running". If Windows 8 doesn't make these calls, we shouldn't make these 
> calls.

But if that introduce regressions, shouldn't workarounds be found then?
Sorry if I keep repeating that but brightness keys handling in-kernel is
quite a useful feature and losing it (because of the ?behave exactly
like Windows 8 kernel? policy) is indeed a regression.
-- 
Yves-Alexis
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: 

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mar., 2013-06-25 at 22:14 +0100, Matthew Garrett wrote:
> On Tue, Jun 25, 2013 at 11:10:11PM +0200, Yves-Alexis Perez wrote:
> > On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> > > I agree, we should standardise the behaviour. And the only way we can 
> > > standardise the behaviour is to leave it up to userspace.
> > > 
> > It's pretty clear we disagree on this and that my opinion won't really
> > matter here. But letting userspace handle that just means broken
> > functionality for those who have the chance (apparently) to have an ACPI
> > backlight interface.
> 
> Which, as we've already established, you don't - Lenovo broke it. Your 
> Thinkpad claims to have 100 available levels, and most of them don't 
> work. The kernel has no way of knowing which levels work and which 
> don't, so leaving this up to the kernel won't actually fix your system 
> either.

I was referring to ?standardize the behaviour by leaving up to
userspace?. A lot of thinkpads (for example) (all the pre-windows 8
ones) have a perfectly working ACPI backlight interface.

Also, if the kernel has no way of knowing which levels work, I fail to
see how userspace can do better.

I understand that switching to intel_backlight instead of acpi_video0
follows what Windows 8 recommends but for me it looks orthogonal to the
fact ACPI methods now have some awkward (Lenovo) or broken (Dell). I
mean, it's not the first time firmware people break some kernel
behavior. I know it's usually not easy to contact them, but shouldn't
those methods be fixed, instead of somehow blindly switching to graphic
drivers?
-- 
Yves-Alexis
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130625/f4ccbc33/attachment.pgp>

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mar., 2013-06-25 at 21:54 +0100, Matthew Garrett wrote:
> On Tue, Jun 25, 2013 at 10:43:57PM +0200, Yves-Alexis Perez wrote:
> > On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> > > Right, the kernel has special-casing to hook the backlight keys up to 
> > > the ACPI backlight control. This is an awful thing, because there's no 
> > > way to detect this case other than parsing a single driver-specific 
> > > module parameter.
> > 
> > I'm not sure what that means. To detect what case exactly? That the
> > brightness is handled by video.ko?
> 
> That the kernel will automatically handle backlight key presses.

Ok, so for detection by userspace? hal managed to do that just fine, it
seems that upower doesn't, for some reason.

> The behaviour is already inconsistent. If you have an ACPI backlight 
> interface, hitting the keys makes your brightness change without any 
> userspace help. If you don't, it doesn't.

At least on the same (class of) hardware it always behaves the same.
> 
> > And in the end, the user just want the brightness keys to correctly
> > handle the brightness, full stop. Having multiple brightness daemons
> > using different policies on different hardware is a nightmare for the
> > end user, imho. From a user point of view, having it handled all in the
> > kernel works really pretty fine and is completely transparent (I have to
> > admit that from that point of view, it was even better when it was
> > handled by the EC but those times seem long gone).
> 
> I agree, we should standardise the behaviour. And the only way we can 
> standardise the behaviour is to leave it up to userspace.
> 
It's pretty clear we disagree on this and that my opinion won't really
matter here. But letting userspace handle that just means broken
functionality for those who have the chance (apparently) to have an ACPI
backlight interface.

Regards,
-- 
Yves-Alexis
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130625/345510ae/attachment.pgp>

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On mar., 2013-06-25 at 17:08 +0100, Matthew Garrett wrote:
> On Sat, Jun 22, 2013 at 11:46:39PM +0200, Yves-Alexis Perez wrote:
> 
> > Before Linux support for acpi_osi("Windows 2012") (and when booting with
> > acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
> > just fine, whether in console, in the display manager or in my desktop
> > environment (Xfce). xfce4-power-manager just needs to be told that the
> > brightness keys are already handled and it doesn't need to do anything.
> 
> Right, the kernel has special-casing to hook the backlight keys up to 
> the ACPI backlight control. This is an awful thing, because there's no 
> way to detect this case other than parsing a single driver-specific 
> module parameter.

I'm not sure what that means. To detect what case exactly? That the
brightness is handled by video.ko?
> 
> Could this functionality be duplicated across other backlight drivers? 
> Not easily. The ACPI driver receives keypresses and performs backlight 
> control. The i915 driver doesn't receive keypresses. We could easily tie 
> certain keycodes into backlight events, but which backlight should they 
> control? You're really starting to get into the kind of complex policy 
> decision that's best left to userspace, which is where it should have 
> been to begin with.
> 
Well, I get the reasoning, but I'm not sure I agree. That means
userspace behavior is inconsistent depending on who does it
(gnome-power-manager, gnome-setting-daemon, whatever), and it usually
means there's nothing handling the brightness before those are running,
not to mention people not running them because they don't run a full
blown desktop environment (until someone starts thinking it's a good
idea to handle brightness in systemd).

And in the end, the user just want the brightness keys to correctly
handle the brightness, full stop. Having multiple brightness daemons
using different policies on different hardware is a nightmare for the
end user, imho. From a user point of view, having it handled all in the
kernel works really pretty fine and is completely transparent (I have to
admit that from that point of view, it was even better when it was
handled by the EC but those times seem long gone).

Regards,
-- 
Yves-Alexis
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130625/d78f51ec/attachment-0001.pgp>

Re: [PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On dim., 2013-06-09 at 19:01 -0400, Matthew Garrett wrote:
> The first two patches in this series are picked from other patchesets aimed at
> solving similar problems. The last simply unregisters ACPI backlight control
> on Windows 8 systems when using an Intel GPU. Similar code could be added to
> other drivers, but I'm reluctant to do so without further investigation as
> to the behaviour of the vendor drivers under Windows.

Hi,

I've read this thread coming from
https://bugzilla.kernel.org/show_bug.cgi?id=51231 and tried the patches
on a Lenovo ThinkPad X230 with intel graphics.

The problem with thoses fixes is that they still introduce a regression
in how the brightness is handled, at least for me.

Before Linux support for acpi_osi("Windows 2012") (and when booting with
acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
just fine, whether in console, in the display manager or in my desktop
environment (Xfce). xfce4-power-manager just needs to be told that the
brightness keys are already handled and it doesn't need to do anything.

After Linux started responding to Win8 ACPI checks, it somehow broke,
but not completely. Brightness keys are not handled by the kernel
anymore (so no brightness adjustment in console or without a hotkey
daemon running). If I run xfce4-power-manager and tell it to handle the
brightness keys, it does work (although the number of brightness levels
is not completely right). That means both acpi_video0 and
intel_backlight backlight interfaces work, they just don't have the same
precision and max_brightness (more details on the bug).

When adding those three patches, well, acpi_video0 is removed (as
intended), but I still don't have brightness handling in the kernel and
need to have something handling it in userspace.

I really think this is a bad idea. In some cases it might be the only
solution, but having a place where this is set consistently would be
really best, imho. Every userspace daemon might do things differently,
and not everyone even has it. And I'm really not sure brightness keys
should be handled by a desktop environment anyway.

So can the previous behavior be actually restored? Maybe it was easier
to pass the brightness keys information from thinkpad_acpi.ko to
video.ko than it is to i915.ko but since it goes to the input layer
anyway I guess it could be fed to module handling that in a way or
another?

Please keep me on CC: on replies, I'm not subscribed to the various
lists.

Regards,
-- 
Yves-Alexis


signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/3] Fix backlight issues on some Windows 8 systems

[Yves-Alexis Perez]

On dim., 2013-06-09 at 19:01 -0400, Matthew Garrett wrote:
> The first two patches in this series are picked from other patchesets aimed at
> solving similar problems. The last simply unregisters ACPI backlight control
> on Windows 8 systems when using an Intel GPU. Similar code could be added to
> other drivers, but I'm reluctant to do so without further investigation as
> to the behaviour of the vendor drivers under Windows.

Hi,

I've read this thread coming from
https://bugzilla.kernel.org/show_bug.cgi?id=51231 and tried the patches
on a Lenovo ThinkPad X230 with intel graphics.

The problem with thoses fixes is that they still introduce a regression
in how the brightness is handled, at least for me.

Before Linux support for acpi_osi("Windows 2012") (and when booting with
acpi_osi="!Windows 2012"), brightness keys were handled by the kernel
just fine, whether in console, in the display manager or in my desktop
environment (Xfce). xfce4-power-manager just needs to be told that the
brightness keys are already handled and it doesn't need to do anything.

After Linux started responding to Win8 ACPI checks, it somehow broke,
but not completely. Brightness keys are not handled by the kernel
anymore (so no brightness adjustment in console or without a hotkey
daemon running). If I run xfce4-power-manager and tell it to handle the
brightness keys, it does work (although the number of brightness levels
is not completely right). That means both acpi_video0 and
intel_backlight backlight interfaces work, they just don't have the same
precision and max_brightness (more details on the bug).

When adding those three patches, well, acpi_video0 is removed (as
intended), but I still don't have brightness handling in the kernel and
need to have something handling it in userspace.

I really think this is a bad idea. In some cases it might be the only
solution, but having a place where this is set consistently would be
really best, imho. Every userspace daemon might do things differently,
and not everyone even has it. And I'm really not sure brightness keys
should be handled by a desktop environment anyway.

So can the previous behavior be actually restored? Maybe it was easier
to pass the brightness keys information from thinkpad_acpi.ko to
video.ko than it is to i915.ko but since it goes to the input layer
anyway I guess it could be fed to module handling that in a way or
another?

Please keep me on CC: on replies, I'm not subscribed to the various
lists.

Regards,
-- 
Yves-Alexis
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: