diff mbox

drm/radeon: add new AMD ACPI header and update relevant code

Message ID 20120801085739.GA7793@growl (mailing list archive)
State New, archived
Headers show

Commit Message

Luca Tettamanti Aug. 1, 2012, 8:57 a.m. UTC 
On Tue, Jul 31, 2012 at 05:33:16PM -0400, Alex Deucher wrote:
> Patches look good.  I picked them up and combined them with may
> patches plus a few other small fixes.  They are available here:
> http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
> Let me know what you think.

Looks ok, I lost one fix along the road though, I'm attaching the patch.

Luca
From 0f71d5b56b9e5eee3194b5b926767511281ea0a6 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti <kronos.it@gmail.com>
Date: Wed, 1 Aug 2012 10:53:19 +0200
Subject: [PATCH] drm/radeon: fix, enable notifications with device specific
 command code

Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
---
 drivers/gpu/drm/radeon/radeon_acpi.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Alex Deucher Aug. 1, 2012, 1:56 p.m. UTC | #1 
On Wed, Aug 1, 2012 at 4:57 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> On Tue, Jul 31, 2012 at 05:33:16PM -0400, Alex Deucher wrote:
>> Patches look good.  I picked them up and combined them with may
>> patches plus a few other small fixes.  They are available here:
>> http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
>> Let me know what you think.
>
> Looks ok, I lost one fix along the road though, I'm attaching the patch.

Thanks, I rolled it into your original patch.  New tree:
http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches

Alex

>
> Luca
Alex Deucher Aug. 2, 2012, 3:03 p.m. UTC | #2 
I admit I'm not really an ACPI expert, but thinking about this more,
I'm wondering if maybe we should just send the appropriate brightness
change, switch display, etc. event to userspace rather than handling
it directly in the radeon driver, then let userspace callback down via
the bl interface, etc.  With backlight for example, does handling it
in the kernel driver as per your patch prevent userspace from seeing
the brightness up/down event?  Wouldn't that break things like OSD
brightness displays and such?

Alex

On Wed, Aug 1, 2012 at 9:56 AM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Wed, Aug 1, 2012 at 4:57 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
>> On Tue, Jul 31, 2012 at 05:33:16PM -0400, Alex Deucher wrote:
>>> Patches look good.  I picked them up and combined them with may
>>> patches plus a few other small fixes.  They are available here:
>>> http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
>>> Let me know what you think.
>>
>> Looks ok, I lost one fix along the road though, I'm attaching the patch.
>
> Thanks, I rolled it into your original patch.  New tree:
> http://cgit.freedesktop.org/~agd5f/linux/log/?h=acpi_patches
>
> Alex
>
>>
>> Luca
Luca Tettamanti Aug. 2, 2012, 4:31 p.m. UTC | #3 
On Thu, Aug 2, 2012 at 5:03 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> I admit I'm not really an ACPI expert, but thinking about this more,
> I'm wondering if maybe we should just send the appropriate brightness
> change, switch display, etc. event to userspace rather than handling
> it directly in the radeon driver, then let userspace callback down via
> the bl interface, etc.  With backlight for example, does handling it
> in the kernel driver as per your patch prevent userspace from seeing
> the brightness up/down event?  Wouldn't that break things like OSD
> brightness displays and such?

No, the event is sent to userspace by the standard ACPI video driver,
it works as before.
Changing brightness usually goes like this:
1) user presses a hotkey
2) a notification is generated (0x86 or 0x87)
3) video.ko handles the notification and calls into ACPI to change the
level (_BCM) and firmware does its magic
4) a key press (brightness up/down) is sent to userspace

With ATIF step 3 does not actually change the brightness, it just send
out another event (VIDEO_PROBE, or one of the device specific ones) so
we need to take care of that too. The rest of the process, including
the delivery of the key presses, stays the same.

Luca
Alex Deucher Aug. 2, 2012, 4:33 p.m. UTC | #4 
On Thu, Aug 2, 2012 at 12:31 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> On Thu, Aug 2, 2012 at 5:03 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> I admit I'm not really an ACPI expert, but thinking about this more,
>> I'm wondering if maybe we should just send the appropriate brightness
>> change, switch display, etc. event to userspace rather than handling
>> it directly in the radeon driver, then let userspace callback down via
>> the bl interface, etc.  With backlight for example, does handling it
>> in the kernel driver as per your patch prevent userspace from seeing
>> the brightness up/down event?  Wouldn't that break things like OSD
>> brightness displays and such?
>
> No, the event is sent to userspace by the standard ACPI video driver,
> it works as before.
> Changing brightness usually goes like this:
> 1) user presses a hotkey
> 2) a notification is generated (0x86 or 0x87)
> 3) video.ko handles the notification and calls into ACPI to change the
> level (_BCM) and firmware does its magic
> 4) a key press (brightness up/down) is sent to userspace
>
> With ATIF step 3 does not actually change the brightness, it just send
> out another event (VIDEO_PROBE, or one of the device specific ones) so
> we need to take care of that too. The rest of the process, including
> the delivery of the key presses, stays the same.

Excellent!  thanks for clarifying.

Alex
Alex Deucher Aug. 2, 2012, 8:54 p.m. UTC | #5 
On Thu, Aug 2, 2012 at 12:33 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Thu, Aug 2, 2012 at 12:31 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
>> On Thu, Aug 2, 2012 at 5:03 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>> I admit I'm not really an ACPI expert, but thinking about this more,
>>> I'm wondering if maybe we should just send the appropriate brightness
>>> change, switch display, etc. event to userspace rather than handling
>>> it directly in the radeon driver, then let userspace callback down via
>>> the bl interface, etc.  With backlight for example, does handling it
>>> in the kernel driver as per your patch prevent userspace from seeing
>>> the brightness up/down event?  Wouldn't that break things like OSD
>>> brightness displays and such?
>>
>> No, the event is sent to userspace by the standard ACPI video driver,
>> it works as before.
>> Changing brightness usually goes like this:
>> 1) user presses a hotkey
>> 2) a notification is generated (0x86 or 0x87)
>> 3) video.ko handles the notification and calls into ACPI to change the
>> level (_BCM) and firmware does its magic
>> 4) a key press (brightness up/down) is sent to userspace
>>
>> With ATIF step 3 does not actually change the brightness, it just send
>> out another event (VIDEO_PROBE, or one of the device specific ones) so
>> we need to take care of that too. The rest of the process, including
>> the delivery of the key presses, stays the same.

Updated tree with fixes to a few existing patches and improved support
for ATPX and initial support for ATCS:
http://cgit.freedesktop.org/~agd5f/linux/?h=acpi_patches

Alex
diff mbox

Patch

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c
index 14ae8aa..a812b9a 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -214,6 +214,7 @@  static int radeon_atif_get_notification_params(acpi_handle handle,
 			err = -EINVAL;
 			goto out;
 		}
+		n->enabled = true;
 		n->command_code = params.command_code;
 	}