[PATCH] ACPI / video: update the skip case for acpi_video_device_in_dod

[PATCH] ACPI / video: update the skip case for acpi_video_device_in_dod

[Aaron Lu]

If the firmware has declared more than 8 video output devices, and the
one that control the internal panel's backlight is listed after the
first 8 output devices, the _DOD will not include it due to the current
i915 operation region implementation. As a result, we will not create a
backlight device for it while we should. Solve this problem by special
case the firmware that has 8+ output devices in that if we see such a
firmware, we do not test if the device is in _DOD list. The creation of
the backlight device will also enable the firmware to emit events on
backlight hotkey press when the acpi_osi= cmdline option is specified on
those affected ASUS laptops.

Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=70241
Reported-and-tested-by: Oleksij Rempel <linux@rempel-privat.de>
Reported-and-tested-by: Dmitry Tunin <hanipouspilot@gmail.com>
Reported-and-tested-by: Jimbo <jaime.91@hotmail.es>
Cc: stable@vger.kernel.org
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/video.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 8b844931b936..c72e79d2c5ad 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -155,6 +155,7 @@ struct acpi_video_bus {
 	u8 dos_setting;
 	struct acpi_video_enumerated_device *attached_array;
 	u8 attached_count;
+	u8 child_count;
 	struct acpi_video_bus_cap cap;
 	struct acpi_video_bus_flags flags;
 	struct list_head video_device_list;
@@ -1176,8 +1177,12 @@ static bool acpi_video_device_in_dod(struct acpi_video_device *device)
 	struct acpi_video_bus *video = device->video;
 	int i;
 
-	/* If we have a broken _DOD, no need to test */
-	if (!video->attached_count)
+	/*
+	 * If we have a broken _DOD or we have more than 8 output devices
+	 * under the graphics controller node that we can't proper deal with
+	 * in the operation region code currently, no need to test.
+	 */
+	if (!video->attached_count || video->child_count > 8)
 		return true;
 
 	for (i = 0; i < video->attached_count; i++) {
@@ -1430,6 +1435,7 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
 			dev_err(&dev->dev, "Can't attach device\n");
 			break;
 		}
+		video->child_count++;
 	}
 	return status;
 }
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] ACPI / video: update the skip case for acpi_video_device_in_dod

[Jani Nikula]

On Mon, 15 Dec 2014, Aaron Lu <aaron.lu@intel.com> wrote:
> If the firmware has declared more than 8 video output devices, and the
> one that control the internal panel's backlight is listed after the
> first 8 output devices, the _DOD will not include it due to the current
> i915 operation region implementation. As a result, we will not create a
> backlight device for it while we should. Solve this problem by special
> case the firmware that has 8+ output devices in that if we see such a
> firmware, we do not test if the device is in _DOD list. The creation of
> the backlight device will also enable the firmware to emit events on
> backlight hotkey press when the acpi_osi= cmdline option is specified on
> those affected ASUS laptops.
>
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=70241
> Reported-and-tested-by: Oleksij Rempel <linux@rempel-privat.de>
> Reported-and-tested-by: Dmitry Tunin <hanipouspilot@gmail.com>
> Reported-and-tested-by: Jimbo <jaime.91@hotmail.es>
> Cc: stable@vger.kernel.org
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

Obviously I'd like to have the proper information in the opregion
DIDL/CADL fields, but it's not going to happen soon, nor is it going to
fit under stable size limits.

I like the simplicity here. FWIW,

Acked-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/acpi/video.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 8b844931b936..c72e79d2c5ad 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -155,6 +155,7 @@ struct acpi_video_bus {
>  	u8 dos_setting;
>  	struct acpi_video_enumerated_device *attached_array;
>  	u8 attached_count;
> +	u8 child_count;
>  	struct acpi_video_bus_cap cap;
>  	struct acpi_video_bus_flags flags;
>  	struct list_head video_device_list;
> @@ -1176,8 +1177,12 @@ static bool acpi_video_device_in_dod(struct acpi_video_device *device)
>  	struct acpi_video_bus *video = device->video;
>  	int i;
>  
> -	/* If we have a broken _DOD, no need to test */
> -	if (!video->attached_count)
> +	/*
> +	 * If we have a broken _DOD or we have more than 8 output devices
> +	 * under the graphics controller node that we can't proper deal with
> +	 * in the operation region code currently, no need to test.
> +	 */
> +	if (!video->attached_count || video->child_count > 8)
>  		return true;
>  
>  	for (i = 0; i < video->attached_count; i++) {
> @@ -1430,6 +1435,7 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
>  			dev_err(&dev->dev, "Can't attach device\n");
>  			break;
>  		}
> +		video->child_count++;
>  	}
>  	return status;
>  }
> -- 
> 2.1.0
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] ACPI / video: update the skip case for acpi_video_device_in_dod

[Aaron Lu]

On 12/15/2014 05:02 PM, Jani Nikula wrote:
> On Mon, 15 Dec 2014, Aaron Lu <aaron.lu@intel.com> wrote:
>> If the firmware has declared more than 8 video output devices, and the
>> one that control the internal panel's backlight is listed after the
>> first 8 output devices, the _DOD will not include it due to the current
>> i915 operation region implementation. As a result, we will not create a
>> backlight device for it while we should. Solve this problem by special
>> case the firmware that has 8+ output devices in that if we see such a
>> firmware, we do not test if the device is in _DOD list. The creation of
>> the backlight device will also enable the firmware to emit events on
>> backlight hotkey press when the acpi_osi= cmdline option is specified on
>> those affected ASUS laptops.
>>
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=70241
>> Reported-and-tested-by: Oleksij Rempel <linux@rempel-privat.de>
>> Reported-and-tested-by: Dmitry Tunin <hanipouspilot@gmail.com>
>> Reported-and-tested-by: Jimbo <jaime.91@hotmail.es>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> 
> Obviously I'd like to have the proper information in the opregion
> DIDL/CADL fields, but it's not going to happen soon, nor is it going to
> fit under stable size limits.
> 
> I like the simplicity here. FWIW,
> 
> Acked-by: Jani Nikula <jani.nikula@intel.com>

Thanks :-)

BTW, this patch is more of a fix for my previous commit 0b8db271f159
("ACPI / video: check _DOD list when creating backlight devices") that
made the acpi_video interface disappear on these affected laptops so I
would like to fix it quickly. And I agree that a proper implementation
of the DIDL/CADL list is needed so that people can get rid of the
acpi_osi= cmdline option they currently need to get hotkey events.

Regards,
Aaron

> 
>> ---
>>  drivers/acpi/video.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index 8b844931b936..c72e79d2c5ad 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -155,6 +155,7 @@ struct acpi_video_bus {
>>  	u8 dos_setting;
>>  	struct acpi_video_enumerated_device *attached_array;
>>  	u8 attached_count;
>> +	u8 child_count;
>>  	struct acpi_video_bus_cap cap;
>>  	struct acpi_video_bus_flags flags;
>>  	struct list_head video_device_list;
>> @@ -1176,8 +1177,12 @@ static bool acpi_video_device_in_dod(struct acpi_video_device *device)
>>  	struct acpi_video_bus *video = device->video;
>>  	int i;
>>  
>> -	/* If we have a broken _DOD, no need to test */
>> -	if (!video->attached_count)
>> +	/*
>> +	 * If we have a broken _DOD or we have more than 8 output devices
>> +	 * under the graphics controller node that we can't proper deal with
>> +	 * in the operation region code currently, no need to test.
>> +	 */
>> +	if (!video->attached_count || video->child_count > 8)
>>  		return true;
>>  
>>  	for (i = 0; i < video->attached_count; i++) {
>> @@ -1430,6 +1435,7 @@ acpi_video_bus_get_devices(struct acpi_video_bus *video,
>>  			dev_err(&dev->dev, "Can't attach device\n");
>>  			break;
>>  		}
>> +		video->child_count++;
>>  	}
>>  	return status;
>>  }
>> -- 
>> 2.1.0
>>
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] ACPI / video: update the skip case for acpi_video_device_in_dod

[Rafael J. Wysocki]

On Tuesday, December 16, 2014 11:21:15 AM Aaron Lu wrote:
> On 12/15/2014 05:02 PM, Jani Nikula wrote:
> > On Mon, 15 Dec 2014, Aaron Lu <aaron.lu@intel.com> wrote:
> >> If the firmware has declared more than 8 video output devices, and the
> >> one that control the internal panel's backlight is listed after the
> >> first 8 output devices, the _DOD will not include it due to the current
> >> i915 operation region implementation. As a result, we will not create a
> >> backlight device for it while we should. Solve this problem by special
> >> case the firmware that has 8+ output devices in that if we see such a
> >> firmware, we do not test if the device is in _DOD list. The creation of
> >> the backlight device will also enable the firmware to emit events on
> >> backlight hotkey press when the acpi_osi= cmdline option is specified on
> >> those affected ASUS laptops.
> >>
> >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=70241
> >> Reported-and-tested-by: Oleksij Rempel <linux@rempel-privat.de>
> >> Reported-and-tested-by: Dmitry Tunin <hanipouspilot@gmail.com>
> >> Reported-and-tested-by: Jimbo <jaime.91@hotmail.es>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> > 
> > Obviously I'd like to have the proper information in the opregion
> > DIDL/CADL fields, but it's not going to happen soon, nor is it going to
> > fit under stable size limits.
> > 
> > I like the simplicity here. FWIW,
> > 
> > Acked-by: Jani Nikula <jani.nikula@intel.com>
> 
> Thanks :-)
> 
> BTW, this patch is more of a fix for my previous commit 0b8db271f159
> ("ACPI / video: check _DOD list when creating backlight devices") that
> made the acpi_video interface disappear on these affected laptops so I
> would like to fix it quickly. And I agree that a proper implementation
> of the DIDL/CADL list is needed so that people can get rid of the
> acpi_osi= cmdline option they currently need to get hotkey events.

Patch applied, thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.