diff mbox

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

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

Commit Message

Luca Tettamanti July 30, 2012, 8:24 p.m. UTC 
On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
> On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> > On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
> >> On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> >> > I just found the first problem (probably a BIOS bug):
> >> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
> >> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
> >> > I intended to use the method to set up the notification handler but now
> >> > my BIOS says that it's not there even if it is...
> >> > Can I assume some default values (e.g. notifications are enabled and will
> >> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
> >> > different)?
> >>
> >> The spec says that the bits in the supported functions vector mean
> >> that if bit n is set, function n+1 exists,
> >
> > Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would
> > mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
> >
> > Maybe if the bit n is set then functions 0..n are available? That would
> > (almost) match what I see...
> 
> From the spec:
> 
> Supported     DWORD Bit vector providing supported functions
> information. Each bit marks
> Functions Bit              support for one specific function of the
> ATIF method. Bit n, if set,
> Vector                        indicates that Function n+1 is supported.

Sorry, I still don't understand it... what's "Function n+1" in this
context?
Does this mean that if bit n is set then the function defined as
1 << (n+1) is supported?

> I don't know how wonky bioses in the wild are however.  I can see what
> our internal teams do in these sort of cases.

That would be helpful :)
Note that on this machine (Toshiba L855-10W) brightness control under
windows does not work with the stock catalyst driver, it works only with
the (oldish) driver supplied by Toshiba.

Anyway, I'm attaching v2 of my patches, I've incorporated the
suggestions and some bits of code from joeyli, and now brightness
control is actually implemented.

Still missing is the issue of event 0x81 and the conflict with video.ko;
the handler should probably return NOTIFY_BAD to veto the keypress.

Luca
From f0f8699eabee0d47b93fba14f8126b821cc106a5 Mon Sep 17 00:00:00 2001
From: Luca Tettamanti <kronos.it@gmail.com>
Date: Sun, 29 Jul 2012 17:04:43 +0200
Subject: [PATCH 1/4] drm/radeon: refactor radeon_atif_call

Don't hard-code function number, this will allow to reuse the function.
v2: add support for the 2nd parameter (from Lee, Chun-Yi
<jlee@suse.com>).

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

Comments

Alex Deucher July 30, 2012, 8:30 p.m. UTC | #1 
On Mon, Jul 30, 2012 at 4:24 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
>> On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
>> > On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
>> >> On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
>> >> > I just found the first problem (probably a BIOS bug):
>> >> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
>> >> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
>> >> > I intended to use the method to set up the notification handler but now
>> >> > my BIOS says that it's not there even if it is...
>> >> > Can I assume some default values (e.g. notifications are enabled and will
>> >> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
>> >> > different)?
>> >>
>> >> The spec says that the bits in the supported functions vector mean
>> >> that if bit n is set, function n+1 exists,
>> >
>> > Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would
>> > mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
>> >
>> > Maybe if the bit n is set then functions 0..n are available? That would
>> > (almost) match what I see...
>>
>> From the spec:
>>
>> Supported     DWORD Bit vector providing supported functions
>> information. Each bit marks
>> Functions Bit              support for one specific function of the
>> ATIF method. Bit n, if set,
>> Vector                        indicates that Function n+1 is supported.
>
> Sorry, I still don't understand it... what's "Function n+1" in this
> context?
> Does this mean that if bit n is set then the function defined as
> 1 << (n+1) is supported?

It means if bit n is set in teh supported function vector, function
n+1 is supported.  E.g., if bit 1 is set, function 2
(ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS) is supported.  If bit 3 is
set function 4 (ATIF_FUNCTION_GET_LID_STATE) is supported, etc.

Alex


>
>> I don't know how wonky bioses in the wild are however.  I can see what
>> our internal teams do in these sort of cases.
>
> That would be helpful :)
> Note that on this machine (Toshiba L855-10W) brightness control under
> windows does not work with the stock catalyst driver, it works only with
> the (oldish) driver supplied by Toshiba.
>
> Anyway, I'm attaching v2 of my patches, I've incorporated the
> suggestions and some bits of code from joeyli, and now brightness
> control is actually implemented.
>
> Still missing is the issue of event 0x81 and the conflict with video.ko;
> the handler should probably return NOTIFY_BAD to veto the keypress.
>
> Luca
Luca Tettamanti July 30, 2012, 8:36 p.m. UTC | #2 
On Mon, Jul 30, 2012 at 10:30 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 4:24 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
>> On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
>>> Supported     DWORD Bit vector providing supported functions
>>> information. Each bit marks
>>> Functions Bit              support for one specific function of the
>>> ATIF method. Bit n, if set,
>>> Vector                        indicates that Function n+1 is supported.
>>
>> Sorry, I still don't understand it... what's "Function n+1" in this
>> context?
>> Does this mean that if bit n is set then the function defined as
>> 1 << (n+1) is supported?
>
> It means if bit n is set in teh supported function vector, function
> n+1 is supported.  E.g., if bit 1 is set, function 2
> (ATIF_FUNCTION_GET_SYSTEM_BIOS_REQUESTS) is supported.  If bit 3 is
> set function 4 (ATIF_FUNCTION_GET_LID_STATE) is supported, etc.

Great, just had an epiphany ;-) "n+1" refers to the value that's
passed down to ATIF, I was still thinking in terms of bitmasks...
Ok, so my code is correct, BIOS is botched... meh.

L
Alex Deucher July 30, 2012, 8:45 p.m. UTC | #3 
On Mon, Jul 30, 2012 at 4:24 PM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 10:20:15AM -0400, Alex Deucher wrote:
>> On Sun, Jul 29, 2012 at 9:06 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
>> > On Sat, Jul 28, 2012 at 05:29:25PM -0400, Alex Deucher wrote:
>> >> On Sat, Jul 28, 2012 at 10:56 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
>> >> > I just found the first problem (probably a BIOS bug):
>> >> > ATIF_FUNCTION_GET_SYSTEM_PARAMETERS is implemented in the DSDT, but the
>> >> > corresponding bit ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED is not set :(
>> >> > I intended to use the method to set up the notification handler but now
>> >> > my BIOS says that it's not there even if it is...
>> >> > Can I assume some default values (e.g. notifications are enabled and will
>> >> > use 0x81 unless ATIF_FUNCTION_GET_SYSTEM_PARAMETERS says something
>> >> > different)?
>> >>
>> >> The spec says that the bits in the supported functions vector mean
>> >> that if bit n is set, function n+1 exists,
>> >
>> > Hum, I don't follow. The vector in my case is 0x2 (1 << 1), that would
>> > mean that ATIF_SELECT_ACTIVE_DISPLAYS_SUPPORTED (1 << 2) is supported?
>> >
>> > Maybe if the bit n is set then functions 0..n are available? That would
>> > (almost) match what I see...
>>
>> From the spec:
>>
>> Supported     DWORD Bit vector providing supported functions
>> information. Each bit marks
>> Functions Bit              support for one specific function of the
>> ATIF method. Bit n, if set,
>> Vector                        indicates that Function n+1 is supported.
>
> Sorry, I still don't understand it... what's "Function n+1" in this
> context?
> Does this mean that if bit n is set then the function defined as
> 1 << (n+1) is supported?
>
>> I don't know how wonky bioses in the wild are however.  I can see what
>> our internal teams do in these sort of cases.
>
> That would be helpful :)
> Note that on this machine (Toshiba L855-10W) brightness control under
> windows does not work with the stock catalyst driver, it works only with
> the (oldish) driver supplied by Toshiba.
>
> Anyway, I'm attaching v2 of my patches, I've incorporated the
> suggestions and some bits of code from joeyli, and now brightness
> control is actually implemented.

Regarding patches 3 and 4, it might be easier to just store a pointer
to the relevant encoder when you verify ATIF rather than walking the
encoder list every time.

Alex

>
> Still missing is the issue of event 0x81 and the conflict with video.ko;
> the handler should probably return NOTIFY_BAD to veto the keypress.
>
> Luca
Luca Tettamanti July 31, 2012, 9:16 a.m. UTC | #4 
On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> Regarding patches 3 and 4, it might be easier to just store a pointer
> to the relevant encoder when you verify ATIF rather than walking the
> encoder list every time.

Makes sense, I was unsure about the lifetime of the encoders, but
AFAICS they're destroyed only when the module in unloaded.

Luca
Alex Deucher July 31, 2012, 1:58 p.m. UTC | #5 
On Tue, Jul 31, 2012 at 5:16 AM, Luca Tettamanti <kronos.it@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 10:45 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> Regarding patches 3 and 4, it might be easier to just store a pointer
>> to the relevant encoder when you verify ATIF rather than walking the
>> encoder list every time.
>
> Makes sense, I was unsure about the lifetime of the encoders, but
> AFAICS they're destroyed only when the module in unloaded.

They are present for the life of the driver.

Alex
diff mbox

Patch

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c
index 5b32e49..158e514 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -14,23 +14,30 @@ 
 #include <linux/vga_switcheroo.h>
 
 /* Call the ATIF method
- *
- * Note: currently we discard the output
  */
-static int radeon_atif_call(acpi_handle handle)
+static union acpi_object *radeon_atif_call(acpi_handle handle, int function,
+		struct acpi_buffer *params)
 {
 	acpi_status status;
 	union acpi_object atif_arg_elements[2];
 	struct acpi_object_list atif_arg;
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL};
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
 	atif_arg.count = 2;
 	atif_arg.pointer = &atif_arg_elements[0];
 
 	atif_arg_elements[0].type = ACPI_TYPE_INTEGER;
-	atif_arg_elements[0].integer.value = ATIF_FUNCTION_VERIFY_INTERFACE;
-	atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
-	atif_arg_elements[1].integer.value = 0;
+	atif_arg_elements[0].integer.value = function;
+
+	if (params) {
+		atif_arg_elements[1].type = ACPI_TYPE_BUFFER;
+		atif_arg_elements[1].buffer.length = params->length;
+		atif_arg_elements[1].buffer.pointer = params->pointer;
+	} else {
+		/* We need a second fake parameter */
+		atif_arg_elements[1].type = ACPI_TYPE_INTEGER;
+		atif_arg_elements[1].integer.value = 0;
+	}
 
 	status = acpi_evaluate_object(handle, "ATIF", &atif_arg, &buffer);
 
@@ -39,18 +46,18 @@  static int radeon_atif_call(acpi_handle handle)
 		DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
 				 acpi_format_exception(status));
 		kfree(buffer.pointer);
-		return 1;
+		return NULL;
 	}
 
-	kfree(buffer.pointer);
-	return 0;
+	return buffer.pointer;
 }
 
 /* Call all ACPI methods here */
 int radeon_acpi_init(struct radeon_device *rdev)
 {
 	acpi_handle handle;
-	int ret;
+	union acpi_object *info;
+	int ret = 0;
 
 	/* Get the device handle */
 	handle = DEVICE_ACPI_HANDLE(&rdev->pdev->dev);
@@ -60,10 +67,11 @@  int radeon_acpi_init(struct radeon_device *rdev)
 		return 0;
 
 	/* Call the ATIF method */
-	ret = radeon_atif_call(handle);
-	if (ret)
-		return ret;
+	info = radeon_atif_call(handle, ATIF_FUNCTION_VERIFY_INTERFACE, NULL);
+	if (!info)
+		ret = -EIO;
 
-	return 0;
+	kfree(info);
+	return ret;
 }