Re: [PATCH] drm/radeon: add new AMD ACPI header and update relevant code

[joeyli <jlee@suse.com>]

於 日，2012-07-29 於 15:10 +0200，Luca Tettamanti 提到：
> On Sun, Jul 29, 2012 at 11:51:48AM +0800, joeyli wrote:
> > Hi Luca, 
> > 
> > 於 六，2012-07-28 於 16:56 +0200，Luca Tettamanti 提到：
> > > 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)?
> > > 
> > 
> > Did you check your DSDT for there have some "Notify (VGA, 0x81)"
> > statement in AFN0..AFN15?
> > If YES, I think that means your machine in case the 0x81 is for ATI used
> > by default.
> 
> Yes, my point is that the nofication is there, but since
> GET_SYSTEM_PARAMETERS is not announced I have not way to check it.
> IOW, what is implemented in the DSDT does not match what is announced by
> VERIFY_INTERFACE.
> For reference this is the DSDT: http://pastebin.com/KKS7ZsTt
> 
> Luca
> 

Yes, saw the problem in your DSDT:

        Method (AF00, 0, NotSerialized)
        {
            CreateWordField (ATIB, Zero, SSZE)
		...
            Store (0x80, NMSK)
            Store (0x02, SFUN)		<=== 10b, bit 0 is 0
            Return (ATIB)
        }

But, AF01 still supported in ATIF on this machine, maybe we should still
try GET_SYSTEM_PARAMETERS even the function vector set to 0?
No idea...


On the other hand, 
My patch to avoid 0x81 event conflict with acpi/video driver like below.
This patch woks on my notebook. Your patches do much more things then
mine, so I think my patch just for reference.  

There have a problem is:
If we want use acpi_notifier_call_chain to check does event consume by
any notifier in acpi's blocking notifier chain, then we need return
NOTIFY_BAD in radeon_acpi but not NOTIFY_OK.

So, I suggest radeon_acpi should register to acpi notifier chain by
itself but not append on radeon_acpi_event in radeon_pm. 

And,
suggest also check the device class is ACPI_VIDEO_CLASS like following:

+static int radeon_acpi_video_event(struct notifier_block *nb,
...
+	if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0)
+		return NOTIFY_DONE;
+

Thanks a lot!
Joey Lee


>From 9c0c42ab8f37dafd3b512ca395b64bf39819d26b Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi <jlee@suse.com>
Date: Mon, 30 Jul 2012 16:17:05 +0800
Subject: [PATCH] drm/radeon avoid 0x81 acpi event conflict with acpi video
 driver

drm/radeon avoid 0x81 acpi event conflict with acpi video driver

Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/acpi/video.c                 |    6 +-
 drivers/gpu/drm/radeon/radeon_acpi.c |  150 ++++++++++++++++++++++++++++++----
 2 files changed, 139 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1e0a9e1..e32492d 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1457,7 +1457,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
 		acpi_video_device_enumerate(video);
 		acpi_video_device_rebind(video);
 		acpi_bus_generate_proc_event(device, event, 0);
-		keycode = KEY_SWITCHVIDEOMODE;
+		if (!acpi_notifier_call_chain(device, event, 0))
+			keycode = KEY_SWITCHVIDEOMODE;
 		break;
 
 	case ACPI_VIDEO_NOTIFY_CYCLE:	/* Cycle Display output hotkey pressed. */
@@ -1479,7 +1480,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
 		break;
 	}
 
-	if (event != ACPI_VIDEO_NOTIFY_SWITCH)
+	if (event != ACPI_VIDEO_NOTIFY_SWITCH ||
+	    event != ACPI_VIDEO_NOTIFY_PROBE)
 		acpi_notifier_call_chain(device, event, 0);
 
 	if (keycode) {
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c
index 5b32e49..37ed5e1 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -3,6 +3,7 @@
 #include <linux/slab.h>
 #include <acpi/acpi_drivers.h>
 #include <acpi/acpi_bus.h>
+#include <acpi/video.h>
 
 #include "drmP.h"
 #include "drm.h"
@@ -13,36 +14,150 @@
 
 #include <linux/vga_switcheroo.h>
 
+struct atif_verify_output {
+	u16 size;		/* structure size in bytes (includes size field) */
+	u16 version;		/* version */
+	u32 notif_mask;		/* supported notifications mask */
+	u32 func_bits;		/* supported functions bit vector */
+} __attribute__((packed));
+
+static struct atif_verify_output *verify_output;
+
+struct atif_get_sysparams_output {
+	u16 size;		/* structure size in bytes (includes size field) */
+	u32 valid_flags_mask;	/* valid flags mask */
+	u32 flags;		/* flags */
+	u8  notify_code;	/* notify command code */
+} __attribute__((packed));
+
+static u8 notify_code;
+
 /* Call the ATIF method
  *
  * Note: currently we discard the output
  */
-static int radeon_atif_call(acpi_handle handle)
+static int radeon_atif_call(acpi_handle handle, int function, struct acpi_buffer *params, struct acpi_buffer *output)
 {
 	acpi_status status;
 	union acpi_object atif_arg_elements[2];
 	struct acpi_object_list atif_arg;
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL};
+	union acpi_object *obj;
 
-	atif_arg.count = 2;
+	atif_arg.count = 1;
 	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;
 
-	status = acpi_evaluate_object(handle, "ATIF", &atif_arg, &buffer);
+	if (params) {
+		atif_arg.count = 2;
+		atif_arg_elements[1].type = ACPI_TYPE_BUFFER;
+		atif_arg_elements[1].buffer.length = params->length;
+		atif_arg_elements[1].buffer.pointer = params->pointer;
+	}
 
-	/* Fail only if calling the method fails and ATIF is supported */
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		DRM_DEBUG_DRIVER("failed to evaluate ATIF got %s\n",
-				 acpi_format_exception(status));
-		kfree(buffer.pointer);
+	status = acpi_evaluate_object(handle, "ATIF", &atif_arg, output);
+	if (ACPI_FAILURE(status))
 		return 1;
+
+	obj = (union acpi_object *) output->pointer;
+	if (!obj)
+		return 1;
+	else if (obj->type != ACPI_TYPE_BUFFER) {
+		kfree(obj);
+		return 1;
+	} else if (obj->buffer.length != 256) {
+		DRM_DEBUG_DRIVER("Unknown ATIF output buffer length %d\n", obj->buffer.length);
+		kfree(obj);
+		return 1;
+	}
+
+	return 0;
+}
+
+static int radeon_atif_verify(acpi_handle handle)
+{
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct atif_verify_output *voutput;
+	union acpi_object *obj;
+	int ret;
+
+	/* evaluate the ATIF verify function */
+	ret = radeon_atif_call(handle, ATIF_FUNCTION_VERIFY_INTERFACE, NULL, &output);
+	if (ret) {
+		kfree(output.pointer);
+		return ret;
+	}
+
+	obj = (union acpi_object *) output.pointer;
+
+	voutput = (struct atif_verify_output *) obj->buffer.pointer;
+	verify_output = kzalloc(sizeof(struct atif_verify_output), GFP_KERNEL);		/* TODO: kfree */
+	verify_output->size = voutput->size;
+	verify_output->version = voutput->version;
+	verify_output->notif_mask = voutput->notif_mask;
+	verify_output->func_bits = voutput->func_bits;
+
+	kfree(output.pointer);
+	return 0;
+}
+
+static int radeon_acpi_video_event(struct notifier_block *nb,
+				   unsigned long val, void *data)
+{
+	/* The only video events relevant to opregion are 0x81 or n. These indicate
+	   either a docking event, lid switch or display switch request. In
+	   Linux, these are handled by the dock, button and video drivers.
+	*/
+
+	struct acpi_bus_event *event = data;
+	int ret = NOTIFY_BAD;		/* Question: for fill acpi's blocking notifier chains */
+
+	if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0)
+		return NOTIFY_DONE;
+
+	if (event->type != notify_code)
+		return NOTIFY_DONE;
+
+	/* TODO: run anything should take care by radeon */
+
+	return ret;
+}
+
+static struct notifier_block radeon_acpi_notifier = {
+	.notifier_call = radeon_acpi_video_event,
+};
+
+static int radeon_atif_get_system_param(acpi_handle handle)
+{
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct atif_get_sysparams_output *params_output;
+	union acpi_object *obj;
+	u32 flags;
+	int ret;
+
+	/* evaluate the ATIF get system parameters function */
+	ret = radeon_atif_call(handle, ATIF_FUNCTION_GET_SYSTEM_PARAMETERS, NULL, &output);
+	if (ret) {
+		kfree(output.pointer);
+		return ret;
 	}
 
-	kfree(buffer.pointer);
+	obj = (union acpi_object *) output.pointer;
+
+	params_output = (struct atif_get_sysparams_output *) obj->buffer.pointer;
+	flags = params_output->flags;
+
+	if (flags) {
+		if (flags == 1)
+			notify_code = 0x81;
+		else if (flags == 2)
+			notify_code = params_output->notify_code;
+
+		register_acpi_notifier(&radeon_acpi_notifier);
+	}
+
+	kfree(output.pointer);
 	return 0;
 }
 
@@ -59,11 +174,16 @@ int radeon_acpi_init(struct radeon_device *rdev)
 	if (!ASIC_IS_AVIVO(rdev) || !rdev->bios || !handle)
 		return 0;
 
-	/* Call the ATIF method */
-	ret = radeon_atif_call(handle);
+	/* Call the ATIF verify function */
+	ret = radeon_atif_verify(handle);
 	if (ret)
 		return ret;
 
+	if (verify_output->func_bits & ATIF_GET_SYSTEM_PARAMETERS_SUPPORTED)
+		ret = radeon_atif_get_system_param(handle);
+		if (ret)
+			return ret;
+
 	return 0;
 }
 
-- 
1.7.7



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