ch to fix this Re: Dell/acpi_video hw.acpi.video.out0 is probably a bug, and an important one. Re: Dell laptops

[Bruno Ducrot]

On Mon, Aug 07, 2006 at 01:08:42PM -0500, Eric Anderson wrote:
> On 07/15/06 22:21, Bruno Ducrot wrote:
> >On Sat, Jul 15, 2006 at 09:01:23PM -0400, john at utzweb.net wrote:
> >>>Hi John,
> >>Hello Bruno
> >>
> >>>On Fri, Jul 14, 2006 at 02:05:40AM -0400, john at utzweb.net wrote:
> >>>>acpi_video.c expects the lcd to be identified as 0x0110, but my Dell
> >>>>Latitude C400 (and probably others) id's the lcd at 0x0400:
> >>>>
> >>>>Device (LCD)
> >>>>                {
> >>>>                    Method (_ADR, 0, NotSerialized)
> >>>>                    {
> >>>>                        Return (0x0400)
> >>>>                    }
> >>>>
> >>>>
> >>>>so, acpi_video needs to account for this.
> >>>>
> >>>>
> >>>>got this sorted, and now the display turns back on, here's the patch, i
> >>>>already send-pr'd it
> >>>Youre somewhat right, but your patch is wrong.
> >>Thankyou for taking interest and reviewing my analysis and patch.
> >>
> >>I would beg to differ with your assertions concerning the patch's
> >>correctness, because the operation you mention below is handled a few
> >>lines above the patch.
> >>
> >>> Actually you have to check if ((adr & 0x0400) == 0x0400).
> >>the & occurs at the top of the switch, here's the destroy case:
> >
> >But with the *WRONG* mask.  It used to be 0xffff with ACPI v2, but
> >shall now be 0x0f00 with ACPI v3.
> >
> >If for example the _ADR is 0x0401, then your patch won't work.  Same
> >if for example the _ADR is 0x0101, which identify a CRT monitor, etc.
> >
> >The only one value that must be kept for backward compatility is
> >(adr & 0xffff) == 0x0110 which is for an internal Flat Panel, instead
> >of a CRT monitor if we take the new specification into account without
> >this very specific value.
> >
> >BTW I compiled and found some stupid mistakes.  I also changed my DSDT
> >such that I'm pretty sure it will work for you, and for others where
> >the _ADR may be 0x04xx as well.
> >
> >Please consider that one:
> >
> >Index: acpi_video.c
> >===================================================================
> >RCS file: /home/ncvs/src/sys/dev/acpica/acpi_video.c,v
> >retrieving revision 1.12
> >diff -u -p -r1.12 acpi_video.c
> >--- acpi_video.c	20 Dec 2005 22:42:16 -0000	1.12
> >+++ acpi_video.c	16 Jul 2006 03:11:24 -0000
> >@@ -110,9 +110,12 @@ static void	vo_set_device_state(ACPI_HAN
> > 
> > /* _DOD and subdev's _ADR */
> > #define DOD_DEVID_MASK		0xffff
> >+#define DOD_DEVID_TYPE		0x0f00
> > #define DOD_DEVID_MONITOR	0x0100
> >-#define DOD_DEVID_PANEL		0x0110
> > #define DOD_DEVID_TV		0x0200
> >+#define DOD_DEVID_DIGITAL	0x0300
> >+#define DOD_DEVID_PANEL		0x0400
> >+#define DOD_DEVID_PANEL_COMPAT	0x0110
> > #define DOD_BIOS		(1 << 16)
> > #define DOD_NONVGA		(1 << 17)
> > #define DOD_HEAD_ID_SHIFT	18
> >@@ -409,27 +412,37 @@ acpi_video_vo_init(UINT32 adr)
> > 	struct acpi_video_output_queue *voqh;
> > 
> > 	ACPI_SERIAL_ASSERT(video);
> >-	switch (adr & DOD_DEVID_MASK) {
> >-	case DOD_DEVID_MONITOR:
> >-		desc = "CRT monitor";
> >-		type = "crt";
> >-		voqh = &crt_units;
> >-		break;
> >-	case DOD_DEVID_PANEL:
> >+	if ((adr & DOD_DEVID_MASK) == DOD_DEVID_PANEL_COMPAT) {
> > 		desc = "LCD panel";
> > 		type = "lcd";
> > 		voqh = &lcd_units;
> >-		break;
> >-	case DOD_DEVID_TV:
> >-		desc = "TV";
> >-		type = "tv";
> >-		voqh = &tv_units;
> >-		break;
> >-	default:
> >-		desc = "unknown output";
> >-		type = "out";
> >-		voqh = &other_units;
> >-	}
> >+	} else
> >+		switch (adr & DOD_DEVID_TYPE) {
> >+		case DOD_DEVID_MONITOR:
> >+			desc = "CRT monitor";
> >+			type = "crt";
> >+			voqh = &crt_units;
> >+			break;
> >+		case DOD_DEVID_DIGITAL:
> >+			desc = "Digital monitor";
> >+			type = "crt";
> >+			voqh = &crt_units;
> >+			break;
> >+		case DOD_DEVID_PANEL:
> >+			desc = "LCD panel";
> >+			type = "lcd";
> >+			voqh = &lcd_units;
> >+			break;
> >+		case DOD_DEVID_TV:
> >+			desc = "TV";
> >+			type = "tv";
> >+			voqh = &tv_units;
> >+			break;
> >+		default:
> >+			desc = "unknown output";
> >+			type = "out";
> >+			voqh = &other_units;
> >+		}
> > 
> > 	n = 0;
> > 	vn = vp = NULL;
> >@@ -553,19 +566,25 @@ acpi_video_vo_destroy(struct acpi_video_
> > 	if (vo->vo_levels != NULL)
> > 		AcpiOsFree(vo->vo_levels);
> > 
> >-	switch (vo->adr & DOD_DEVID_MASK) {
> >-	case DOD_DEVID_MONITOR:
> >-		voqh = &crt_units;
> >-		break;
> >-	case DOD_DEVID_PANEL:
> >+	if ((vo->adr & 0xffff) == DOD_DEVID_PANEL_COMPAT)
> > 		voqh = &lcd_units;
> >-		break;
> >-	case DOD_DEVID_TV:
> >-		voqh = &tv_units;
> >-		break;
> >-	default:
> >-		voqh = &other_units;
> >-	}
> >+	else
> >+		switch (vo->adr & DOD_DEVID_TYPE) {
> >+		case DOD_DEVID_MONITOR:
> >+			voqh = &crt_units;
> >+			break;
> >+		case DOD_DEVID_DIGITAL:
> >+			voqh = &crt_units;
> >+			break;
> >+		case DOD_DEVID_PANEL:
> >+			voqh = &lcd_units;
> >+			break;
> >+		case DOD_DEVID_TV:
> >+			voqh = &tv_units;
> >+			break;
> >+		default:
> >+			voqh = &other_units;
> >+		}
> > 	STAILQ_REMOVE(voqh, vo, acpi_video_output, vo_unit.next);
> > 	free(vo, M_ACPIVIDEO);
> > }
> >
> >
> >Cheers,
> >
> 
> 
> Did this ever get committed?  If so, MFC'ed?
> 
> 

Not yet.  I thought to add more acpi 3.0 stuff onto acpi_video first,
but it might be better indeed to first commit those bits first.
Actually this will be the patch written by Hiroki Sato (see PR 100271).

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.