[PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Mario Limonciello]

In kernel 6.1 the backlight registration code was overhauled so that
at most one backlight device got registered. As part of this change
there was code added to cover the "nomodeset" case to still allow
making an acpi_video0 device if the BIOS contained backlight control
methods.

This fallback logic however is failing on the BIOS from a number of
motherboard manufacturers supporting Ryzen APUs.  What happens is
the amdgpu driver finishes registration and as expected doesn't
create a backlight control device since no eDP panels are connected
to a desktop.

Then 8 seconds later the ACPI video detection code creates an
acpi_video0 device that is non-operational. GNOME then creates a
backlight slider.

To avoid this situation from happening add support for video drivers
to notify the ACPI video detection code that no panel was detected.

To reduce the risk of regressions on multi-GPU systems:
* only use this logic when the system is reported as a desktop enclosure.
* in the amdgpu code only report into this for APUs.

Mario Limonciello (2):
  ACPI: video: Allow GPU drivers to report no panels
  drm/amd/display: Report to ACPI video if no panels were found

 drivers/acpi/acpi_video.c                         | 12 ++++++++++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 ++++
 include/acpi/video.h                              |  1 +
 3 files changed, 17 insertions(+)

-- 
2.34.1

[PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Mario Limonciello]

In kernel 6.1 the backlight registration code was overhauled so that
at most one backlight device got registered. As part of this change
there was code added to cover the "nomodeset" case to still allow
making an acpi_video0 device if the BIOS contained backlight control
methods.

This fallback logic however is failing on the BIOS from a number of
motherboard manufacturers supporting Ryzen APUs.  What happens is
the amdgpu driver finishes registration and as expected doesn't
create a backlight control device since no eDP panels are connected
to a desktop.

Then 8 seconds later the ACPI video detection code creates an
acpi_video0 device that is non-operational. GNOME then creates a
backlight slider.

To avoid this situation from happening add support for video drivers
to notify the ACPI video detection code that no panel was detected.

To reduce the risk of regressions on multi-GPU systems:
* only use this logic when the system is reported as a desktop enclosure.
* in the amdgpu code only report into this for APUs.

Mario Limonciello (2):
  ACPI: video: Allow GPU drivers to report no panels
  drm/amd/display: Report to ACPI video if no panels were found

 drivers/acpi/acpi_video.c                         | 12 ++++++++++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 ++++
 include/acpi/video.h                              |  1 +
 3 files changed, 17 insertions(+)

-- 
2.34.1

[PATCH 1/2] ACPI: video: Allow GPU drivers to report no panels

[Mario Limonciello]

The current logic for the ACPI backlight detection will create
a backlight device if no native or vendor drivers have created
8 seconds after the system has booted if the ACPI tables
included backlight control methods.

If the GPU drivers have loaded, they may be able to report whether
any LCD panels were found.  Allow using this information to factor
in whether to make an acpi_video0 backlight device.

To avoid risks for regressions on complicated configurations with
muxes and multiple native drivers, only take into account drivers
that have reported this when the system is a desktop.

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/acpi_video.c | 12 ++++++++++++
 include/acpi/video.h      |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 32953646caeb..e297f8877797 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -82,6 +82,7 @@ MODULE_PARM_DESC(register_backlight_delay,
 
 static bool may_report_brightness_keys;
 static int register_count;
+static bool native_reported_nolcd;
 static DEFINE_MUTEX(register_count_mutex);
 static DEFINE_MUTEX(video_list_lock);
 static LIST_HEAD(video_bus_head);
@@ -1811,6 +1812,9 @@ static bool acpi_video_should_register_backlight(struct acpi_video_device *dev)
 		return false;
 	}
 
+	if (native_reported_nolcd)
+		return false;
+
 	if (only_lcd)
 		return dev->flags.lcd;
 	return true;
@@ -2178,6 +2182,14 @@ static bool should_check_lcd_flag(void)
 	return false;
 }
 
+void acpi_video_report_nolcd(void)
+{
+	/* Only take into account native driver reporting on desktops */
+	if (dmi_is_desktop())
+		native_reported_nolcd = true;
+}
+EXPORT_SYMBOL(acpi_video_report_nolcd);
+
 int acpi_video_register(void)
 {
 	int ret = 0;
diff --git a/include/acpi/video.h b/include/acpi/video.h
index a275c35e5249..1fccb111c197 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -53,6 +53,7 @@ enum acpi_backlight_type {
 };
 
 #if IS_ENABLED(CONFIG_ACPI_VIDEO)
+extern void acpi_video_report_nolcd(void);
 extern int acpi_video_register(void);
 extern void acpi_video_unregister(void);
 extern void acpi_video_register_backlight(void);
-- 
2.34.1

[PATCH 1/2] ACPI: video: Allow GPU drivers to report no panels

[Mario Limonciello]

The current logic for the ACPI backlight detection will create
a backlight device if no native or vendor drivers have created
8 seconds after the system has booted if the ACPI tables
included backlight control methods.

If the GPU drivers have loaded, they may be able to report whether
any LCD panels were found.  Allow using this information to factor
in whether to make an acpi_video0 backlight device.

To avoid risks for regressions on complicated configurations with
muxes and multiple native drivers, only take into account drivers
that have reported this when the system is a desktop.

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/acpi_video.c | 12 ++++++++++++
 include/acpi/video.h      |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 32953646caeb..e297f8877797 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -82,6 +82,7 @@ MODULE_PARM_DESC(register_backlight_delay,
 
 static bool may_report_brightness_keys;
 static int register_count;
+static bool native_reported_nolcd;
 static DEFINE_MUTEX(register_count_mutex);
 static DEFINE_MUTEX(video_list_lock);
 static LIST_HEAD(video_bus_head);
@@ -1811,6 +1812,9 @@ static bool acpi_video_should_register_backlight(struct acpi_video_device *dev)
 		return false;
 	}
 
+	if (native_reported_nolcd)
+		return false;
+
 	if (only_lcd)
 		return dev->flags.lcd;
 	return true;
@@ -2178,6 +2182,14 @@ static bool should_check_lcd_flag(void)
 	return false;
 }
 
+void acpi_video_report_nolcd(void)
+{
+	/* Only take into account native driver reporting on desktops */
+	if (dmi_is_desktop())
+		native_reported_nolcd = true;
+}
+EXPORT_SYMBOL(acpi_video_report_nolcd);
+
 int acpi_video_register(void)
 {
 	int ret = 0;
diff --git a/include/acpi/video.h b/include/acpi/video.h
index a275c35e5249..1fccb111c197 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -53,6 +53,7 @@ enum acpi_backlight_type {
 };
 
 #if IS_ENABLED(CONFIG_ACPI_VIDEO)
+extern void acpi_video_report_nolcd(void);
 extern int acpi_video_register(void);
 extern void acpi_video_unregister(void);
 extern void acpi_video_register_backlight(void);
-- 
2.34.1

[PATCH 2/2] drm/amd/display: Report to ACPI video if no panels were found

[Mario Limonciello]

On desktop APUs amdgpu doesn't create a native backlight device
as no eDP panels are found.  However if the BIOS has reported
backlight control methods in the ACPI tables then an acpi_video0
backlight device will be made 8 seconds after boot.

This has manifested in a power slider on a number of desktop APUs
ranging from Ryzen 5000 through Ryzen 7000 on various motherboard
manufacturers. To avoid this, report to the acpi video detection
that the system does not have any panel connected in the native
driver.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1783786
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 512c32327eb1..b73f61ac5dd5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4371,6 +4371,10 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
 		amdgpu_set_panel_orientation(&aconnector->base);
 	}
 
+	/* If we didn't find a panel, notify the acpi video detection */
+	if (dm->adev->flags & AMD_IS_APU && dm->num_of_edps == 0)
+		acpi_video_report_nolcd();
+
 	/* Software is initialized. Now we can register interrupt handlers. */
 	switch (adev->asic_type) {
 #if defined(CONFIG_DRM_AMD_DC_SI)
-- 
2.34.1

[PATCH 2/2] drm/amd/display: Report to ACPI video if no panels were found

[Mario Limonciello]

On desktop APUs amdgpu doesn't create a native backlight device
as no eDP panels are found.  However if the BIOS has reported
backlight control methods in the ACPI tables then an acpi_video0
backlight device will be made 8 seconds after boot.

This has manifested in a power slider on a number of desktop APUs
ranging from Ryzen 5000 through Ryzen 7000 on various motherboard
manufacturers. To avoid this, report to the acpi video detection
that the system does not have any panel connected in the native
driver.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=1783786
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 512c32327eb1..b73f61ac5dd5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4371,6 +4371,10 @@ static int amdgpu_dm_initialize_drm_device(struct amdgpu_device *adev)
 		amdgpu_set_panel_orientation(&aconnector->base);
 	}
 
+	/* If we didn't find a panel, notify the acpi video detection */
+	if (dm->adev->flags & AMD_IS_APU && dm->num_of_edps == 0)
+		acpi_video_report_nolcd();
+
 	/* Software is initialized. Now we can register interrupt handlers. */
 	switch (adev->asic_type) {
 #if defined(CONFIG_DRM_AMD_DC_SI)
-- 
2.34.1

Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Hans de Goede]

Hi All,

Mario, thank you for working on this.

On 12/7/22 20:31, Mario Limonciello wrote:
> In kernel 6.1 the backlight registration code was overhauled so that
> at most one backlight device got registered. As part of this change
> there was code added to cover the "nomodeset" case to still allow
> making an acpi_video0 device if the BIOS contained backlight control
> methods.
> 
> This fallback logic however is failing on the BIOS from a number of
> motherboard manufacturers supporting Ryzen APUs.  What happens is
> the amdgpu driver finishes registration and as expected doesn't
> create a backlight control device since no eDP panels are connected
> to a desktop.
> 
> Then 8 seconds later the ACPI video detection code creates an
> acpi_video0 device that is non-operational. GNOME then creates a
> backlight slider.

Note that the problem of the creating a non functional acpi_video0
device happened before the overhaul too.

The difference is that now we have the in kernel GPU drivers
all call acpi_video_register_backlight() when they detect an
internal-panel for which they don't provide native-backlight
control themselves (to avoid the acpi_video0 backlight registering
before the native backlight gets a chance to register).

The timeout is only there in case no drivers ever call
acpi_video_register_backlight(). nomodeset is one case, but
loosing backlight control in the nomodeset case would be fine
IMHO. The bigger worry why we have the timeout is because of
the nvidia binary driver, for devices which use that driver +
rely on apci_video# for backlight control.

Back to the issue at hand of the unwanted (non functional)
apci_video# on various AMD APU using desktops.

The native drivers now all calling acpi_video_register_backlight()
gives us a chance to actually do something about it, so in that
sense the 6.1 backlight refactor is relevant.

> To avoid this situation from happening add support for video drivers
> to notify the ACPI video detection code that no panel was detected.
> 
> To reduce the risk of regressions on multi-GPU systems:
> * only use this logic when the system is reported as a desktop enclosure.
> * in the amdgpu code only report into this for APUs.

I'm afraid that there still is a potential issue for dual
GPU machines. The chassistype is not 100% reliable.

Lets say we have a machine with the wrong chassis-type with
an AMD APU + nvidia GPU which relies on acpi_video0 for
backlight control.

Then if the LCD is connected to the nvidia GPU only, the
amdgpu code will call the new acpi_video_report_nolcd()
function.

And then even if the nvidia binary driver is patched
by nvidia to call the new  acpi_video_register_backlight()
when it does see a panel, then acpi_video_should_register_backlight()
will still return false.

Basically the problem is that we only want to not try
and register the acpi_video0 backlight on dual GPU
machines if the output detection on *both* GPUs has not
found any builtin LCD panel.

But this series disables acpi_video0 backlight registration
as soon as *one* of the *two* GPUs has not found an internal
LCD panel.

As discussed above, after the backlight refactor,
GPU(KMS) drivers are expected to call
acpi_video_register_backlight() when necessary for any
internal panels connected to the GPU they are driving.

This mostly fixes the issue of having an acpi_video0 on
desktop APUs, except that the timeout thingie which was
added to avoid regressions still causes the acpi_video0
backlight to get registered.

Note that this timeout is already configurable through
the register_backlight_delay module option; and setting
that option to 0 disables the timeout based fallback
completely.

So another fix for this might be to just change the
default value of register_backlight_delay to 0 for
kernel 6.2 .  This is a change which I want to make
eventually anyways; so we might just as well do this
now to fix the spurious acpi_video0 on desktop APUs
issue.   And if this does cause issues for nvidia
binary driver users, they can easily work around this
by setting the module option.

Or alternatively we could go with this series,
reworked so that calling acpi_video_report_nolcd()
only cancels the timeout.  This way drivers for another
GPU can still get the acpi_video0 if necessary by
explicitly calling acpi_video_register_backlight().

Personally I have a small preference for just changing
the default of register_backlight_delay to 0, disabling
the timeout based fallback starting with 6.2 .

I did not do this for 6.1 because there were already
many other backlight changes in 6.1, so I wanted to
have the fallback behavior there as a safeguard
against things not working as planned.

Regards,

Hans










> 
> Mario Limonciello (2):
>   ACPI: video: Allow GPU drivers to report no panels
>   drm/amd/display: Report to ACPI video if no panels were found
> 
>  drivers/acpi/acpi_video.c                         | 12 ++++++++++++
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 ++++
>  include/acpi/video.h                              |  1 +
>  3 files changed, 17 insertions(+)
> 

Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Hans de Goede]

Hi All,

Mario, thank you for working on this.

On 12/7/22 20:31, Mario Limonciello wrote:
> In kernel 6.1 the backlight registration code was overhauled so that
> at most one backlight device got registered. As part of this change
> there was code added to cover the "nomodeset" case to still allow
> making an acpi_video0 device if the BIOS contained backlight control
> methods.
> 
> This fallback logic however is failing on the BIOS from a number of
> motherboard manufacturers supporting Ryzen APUs.  What happens is
> the amdgpu driver finishes registration and as expected doesn't
> create a backlight control device since no eDP panels are connected
> to a desktop.
> 
> Then 8 seconds later the ACPI video detection code creates an
> acpi_video0 device that is non-operational. GNOME then creates a
> backlight slider.

Note that the problem of the creating a non functional acpi_video0
device happened before the overhaul too.

The difference is that now we have the in kernel GPU drivers
all call acpi_video_register_backlight() when they detect an
internal-panel for which they don't provide native-backlight
control themselves (to avoid the acpi_video0 backlight registering
before the native backlight gets a chance to register).

The timeout is only there in case no drivers ever call
acpi_video_register_backlight(). nomodeset is one case, but
loosing backlight control in the nomodeset case would be fine
IMHO. The bigger worry why we have the timeout is because of
the nvidia binary driver, for devices which use that driver +
rely on apci_video# for backlight control.

Back to the issue at hand of the unwanted (non functional)
apci_video# on various AMD APU using desktops.

The native drivers now all calling acpi_video_register_backlight()
gives us a chance to actually do something about it, so in that
sense the 6.1 backlight refactor is relevant.

> To avoid this situation from happening add support for video drivers
> to notify the ACPI video detection code that no panel was detected.
> 
> To reduce the risk of regressions on multi-GPU systems:
> * only use this logic when the system is reported as a desktop enclosure.
> * in the amdgpu code only report into this for APUs.

I'm afraid that there still is a potential issue for dual
GPU machines. The chassistype is not 100% reliable.

Lets say we have a machine with the wrong chassis-type with
an AMD APU + nvidia GPU which relies on acpi_video0 for
backlight control.

Then if the LCD is connected to the nvidia GPU only, the
amdgpu code will call the new acpi_video_report_nolcd()
function.

And then even if the nvidia binary driver is patched
by nvidia to call the new  acpi_video_register_backlight()
when it does see a panel, then acpi_video_should_register_backlight()
will still return false.

Basically the problem is that we only want to not try
and register the acpi_video0 backlight on dual GPU
machines if the output detection on *both* GPUs has not
found any builtin LCD panel.

But this series disables acpi_video0 backlight registration
as soon as *one* of the *two* GPUs has not found an internal
LCD panel.

As discussed above, after the backlight refactor,
GPU(KMS) drivers are expected to call
acpi_video_register_backlight() when necessary for any
internal panels connected to the GPU they are driving.

This mostly fixes the issue of having an acpi_video0 on
desktop APUs, except that the timeout thingie which was
added to avoid regressions still causes the acpi_video0
backlight to get registered.

Note that this timeout is already configurable through
the register_backlight_delay module option; and setting
that option to 0 disables the timeout based fallback
completely.

So another fix for this might be to just change the
default value of register_backlight_delay to 0 for
kernel 6.2 .  This is a change which I want to make
eventually anyways; so we might just as well do this
now to fix the spurious acpi_video0 on desktop APUs
issue.   And if this does cause issues for nvidia
binary driver users, they can easily work around this
by setting the module option.

Or alternatively we could go with this series,
reworked so that calling acpi_video_report_nolcd()
only cancels the timeout.  This way drivers for another
GPU can still get the acpi_video0 if necessary by
explicitly calling acpi_video_register_backlight().

Personally I have a small preference for just changing
the default of register_backlight_delay to 0, disabling
the timeout based fallback starting with 6.2 .

I did not do this for 6.1 because there were already
many other backlight changes in 6.1, so I wanted to
have the fallback behavior there as a safeguard
against things not working as planned.

Regards,

Hans










> 
> Mario Limonciello (2):
>   ACPI: video: Allow GPU drivers to report no panels
>   drm/amd/display: Report to ACPI video if no panels were found
> 
>  drivers/acpi/acpi_video.c                         | 12 ++++++++++++
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 ++++
>  include/acpi/video.h                              |  1 +
>  3 files changed, 17 insertions(+)
> 

Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Limonciello, Mario]

On 12/7/2022 15:04, Hans de Goede wrote:
> Hi All,
> 
> Mario, thank you for working on this.

Sure

<snip>
> 
> Note that the problem of the creating a non functional acpi_video0
> device happened before the overhaul too.
> 
> The difference is that now we have the in kernel GPU drivers
> all call acpi_video_register_backlight() when they detect an
> internal-panel for which they don't provide native-backlight
> control themselves (to avoid the acpi_video0 backlight registering
> before the native backlight gets a chance to register).
> 
> The timeout is only there in case no drivers ever call
> acpi_video_register_backlight(). nomodeset is one case, but
> loosing backlight control in the nomodeset case would be fine
> IMHO. The bigger worry why we have the timeout is because of
> the nvidia binary driver, for devices which use that driver +
> rely on apci_video# for backlight control.
> 
> Back to the issue at hand of the unwanted (non functional)
> apci_video# on various AMD APU using desktops.
> 

Thanks for explaining.

> The native drivers now all calling acpi_video_register_backlight()
> gives us a chance to actually do something about it, so in that
> sense the 6.1 backlight refactor is relevant.
> 
>> To avoid this situation from happening add support for video drivers
>> to notify the ACPI video detection code that no panel was detected.
>>
>> To reduce the risk of regressions on multi-GPU systems:
>> * only use this logic when the system is reported as a desktop enclosure.
>> * in the amdgpu code only report into this for APUs.
> 
> I'm afraid that there still is a potential issue for dual
> GPU machines. The chassistype is not 100% reliable.

Have you ever seen an A+N machine with unreliable chassis type?

Given Windows HLK certification and knowing that these are to
be based off reference BIOS of laptops, I would be really surprised
if this was wrong on an A+N laptop.

> 
> Lets say we have a machine with the wrong chassis-type with
> an AMD APU + nvidia GPU which relies on acpi_video0 for
> backlight control.
> 
> Then if the LCD is connected to the nvidia GPU only, the
> amdgpu code will call the new acpi_video_report_nolcd()
> function.

+ Dan Dadap

Dan - the context is this series:
https://patchwork.freedesktop.org/series/111745/

Do you know if this is real or just conceptual?

> 
> And then even if the nvidia binary driver is patched
> by nvidia to call the new  acpi_video_register_backlight()
> when it does see a panel, then acpi_video_should_register_backlight()
> will still return false.
> 
> Basically the problem is that we only want to not try
> and register the acpi_video0 backlight on dual GPU
> machines if the output detection on *both* GPUs has not
> found any builtin LCD panel.
> 
> But this series disables acpi_video0 backlight registration
> as soon as *one* of the *two* GPUs has not found an internal
> LCD panel.
> 
> As discussed above, after the backlight refactor,
> GPU(KMS) drivers are expected to call
> acpi_video_register_backlight() when necessary for any
> internal panels connected to the GPU they are driving.
> 
> This mostly fixes the issue of having an acpi_video0 on
> desktop APUs, except that the timeout thingie which was
> added to avoid regressions still causes the acpi_video0
> backlight to get registered.
> 
> Note that this timeout is already configurable through
> the register_backlight_delay module option; and setting
> that option to 0 disables the timeout based fallback
> completely.
> 
> So another fix for this might be to just change the
> default value of register_backlight_delay to 0 for
> kernel 6.2 .  This is a change which I want to make
> eventually anyways; so we might just as well do this
> now to fix the spurious acpi_video0 on desktop APUs
> issue.   And if this does cause issues for nvidia
> binary driver users, they can easily work around this
> by setting the module option.
> 
> Or alternatively we could go with this series,
> reworked so that calling acpi_video_report_nolcd()
> only cancels the timeout.  This way drivers for another
> GPU can still get the acpi_video0 if necessary by
> explicitly calling acpi_video_register_backlight().
> 
> Personally I have a small preference for just changing
> the default of register_backlight_delay to 0, disabling
> the timeout based fallback starting with 6.2 .

How about we do both?  Then you can always restore 
register_backlight_delay without risk of introducing
regression of acpi_video0 coming back to desktop APU's.

> 
> I did not do this for 6.1 because there were already
> many other backlight changes in 6.1, so I wanted to
> have the fallback behavior there as a safeguard
> against things not working as planned.
> 

If you're open to my idea of both since I'm already
touching all this anyway I am fine to roll that change
into another patch for default of 0 too in a v2.

Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Limonciello, Mario]

On 12/7/2022 15:04, Hans de Goede wrote:
> Hi All,
> 
> Mario, thank you for working on this.

Sure

<snip>
> 
> Note that the problem of the creating a non functional acpi_video0
> device happened before the overhaul too.
> 
> The difference is that now we have the in kernel GPU drivers
> all call acpi_video_register_backlight() when they detect an
> internal-panel for which they don't provide native-backlight
> control themselves (to avoid the acpi_video0 backlight registering
> before the native backlight gets a chance to register).
> 
> The timeout is only there in case no drivers ever call
> acpi_video_register_backlight(). nomodeset is one case, but
> loosing backlight control in the nomodeset case would be fine
> IMHO. The bigger worry why we have the timeout is because of
> the nvidia binary driver, for devices which use that driver +
> rely on apci_video# for backlight control.
> 
> Back to the issue at hand of the unwanted (non functional)
> apci_video# on various AMD APU using desktops.
> 

Thanks for explaining.

> The native drivers now all calling acpi_video_register_backlight()
> gives us a chance to actually do something about it, so in that
> sense the 6.1 backlight refactor is relevant.
> 
>> To avoid this situation from happening add support for video drivers
>> to notify the ACPI video detection code that no panel was detected.
>>
>> To reduce the risk of regressions on multi-GPU systems:
>> * only use this logic when the system is reported as a desktop enclosure.
>> * in the amdgpu code only report into this for APUs.
> 
> I'm afraid that there still is a potential issue for dual
> GPU machines. The chassistype is not 100% reliable.

Have you ever seen an A+N machine with unreliable chassis type?

Given Windows HLK certification and knowing that these are to
be based off reference BIOS of laptops, I would be really surprised
if this was wrong on an A+N laptop.

> 
> Lets say we have a machine with the wrong chassis-type with
> an AMD APU + nvidia GPU which relies on acpi_video0 for
> backlight control.
> 
> Then if the LCD is connected to the nvidia GPU only, the
> amdgpu code will call the new acpi_video_report_nolcd()
> function.

+ Dan Dadap

Dan - the context is this series:
https://patchwork.freedesktop.org/series/111745/

Do you know if this is real or just conceptual?

> 
> And then even if the nvidia binary driver is patched
> by nvidia to call the new  acpi_video_register_backlight()
> when it does see a panel, then acpi_video_should_register_backlight()
> will still return false.
> 
> Basically the problem is that we only want to not try
> and register the acpi_video0 backlight on dual GPU
> machines if the output detection on *both* GPUs has not
> found any builtin LCD panel.
> 
> But this series disables acpi_video0 backlight registration
> as soon as *one* of the *two* GPUs has not found an internal
> LCD panel.
> 
> As discussed above, after the backlight refactor,
> GPU(KMS) drivers are expected to call
> acpi_video_register_backlight() when necessary for any
> internal panels connected to the GPU they are driving.
> 
> This mostly fixes the issue of having an acpi_video0 on
> desktop APUs, except that the timeout thingie which was
> added to avoid regressions still causes the acpi_video0
> backlight to get registered.
> 
> Note that this timeout is already configurable through
> the register_backlight_delay module option; and setting
> that option to 0 disables the timeout based fallback
> completely.
> 
> So another fix for this might be to just change the
> default value of register_backlight_delay to 0 for
> kernel 6.2 .  This is a change which I want to make
> eventually anyways; so we might just as well do this
> now to fix the spurious acpi_video0 on desktop APUs
> issue.   And if this does cause issues for nvidia
> binary driver users, they can easily work around this
> by setting the module option.
> 
> Or alternatively we could go with this series,
> reworked so that calling acpi_video_report_nolcd()
> only cancels the timeout.  This way drivers for another
> GPU can still get the acpi_video0 if necessary by
> explicitly calling acpi_video_register_backlight().
> 
> Personally I have a small preference for just changing
> the default of register_backlight_delay to 0, disabling
> the timeout based fallback starting with 6.2 .

How about we do both?  Then you can always restore 
register_backlight_delay without risk of introducing
regression of acpi_video0 coming back to desktop APU's.

> 
> I did not do this for 6.1 because there were already
> many other backlight changes in 6.1, so I wanted to
> have the fallback behavior there as a safeguard
> against things not working as planned.
> 

If you're open to my idea of both since I'm already
touching all this anyway I am fine to roll that change
into another patch for default of 0 too in a v2.

Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Hans de Goede]

Hi,

On 12/7/22 22:21, Limonciello, Mario wrote:
> On 12/7/2022 15:04, Hans de Goede wrote:
>> Hi All,
>>
>> Mario, thank you for working on this.
> 
> Sure
> 
> <snip>
>>
>> Note that the problem of the creating a non functional acpi_video0
>> device happened before the overhaul too.
>>
>> The difference is that now we have the in kernel GPU drivers
>> all call acpi_video_register_backlight() when they detect an
>> internal-panel for which they don't provide native-backlight
>> control themselves (to avoid the acpi_video0 backlight registering
>> before the native backlight gets a chance to register).
>>
>> The timeout is only there in case no drivers ever call
>> acpi_video_register_backlight(). nomodeset is one case, but
>> loosing backlight control in the nomodeset case would be fine
>> IMHO. The bigger worry why we have the timeout is because of
>> the nvidia binary driver, for devices which use that driver +
>> rely on apci_video# for backlight control.
>>
>> Back to the issue at hand of the unwanted (non functional)
>> apci_video# on various AMD APU using desktops.
>>
> 
> Thanks for explaining.
> 
>> The native drivers now all calling acpi_video_register_backlight()
>> gives us a chance to actually do something about it, so in that
>> sense the 6.1 backlight refactor is relevant.
>>
>>> To avoid this situation from happening add support for video drivers
>>> to notify the ACPI video detection code that no panel was detected.
>>>
>>> To reduce the risk of regressions on multi-GPU systems:
>>> * only use this logic when the system is reported as a desktop enclosure.
>>> * in the amdgpu code only report into this for APUs.
>>
>> I'm afraid that there still is a potential issue for dual
>> GPU machines. The chassistype is not 100% reliable.
> 
> Have you ever seen an A+N machine with unreliable chassis type?

Not specifically. I just know from experience to not
rely on chassis type.

E.g. I would not be surprised to have some of the desktop-replacement
class laptops from e.g. clevo which sometimes even come with
a desktop CPU for moar power, have their chassis type wrong.

Granted those are not using AMD APUs (yet), but that might change
with the ryzen 7000 series where every CPU is an APU too...

> Given Windows HLK certification and knowing that these are to
> be based off reference BIOS of laptops, I would be really surprised
> if this was wrong on an A+N laptop.

I agree this is unlikely. But I have seen all sort of wrong
chassis-type settings in devices which are not from the
big OEMs.  And AFAIK these sometimes also play fasr and loose
with the Windows certification.

>> Lets say we have a machine with the wrong chassis-type with
>> an AMD APU + nvidia GPU which relies on acpi_video0 for
>> backlight control.
>>
>> Then if the LCD is connected to the nvidia GPU only, the
>> amdgpu code will call the new acpi_video_report_nolcd()
>> function.
> 
> + Dan Dadap
> 
> Dan - the context is this series:
> https://patchwork.freedesktop.org/series/111745/
> 
> Do you know if this is real or just conceptual?
> 
>>
>> And then even if the nvidia binary driver is patched
>> by nvidia to call the new  acpi_video_register_backlight()
>> when it does see a panel, then acpi_video_should_register_backlight()
>> will still return false.
>>
>> Basically the problem is that we only want to not try
>> and register the acpi_video0 backlight on dual GPU
>> machines if the output detection on *both* GPUs has not
>> found any builtin LCD panel.
>>
>> But this series disables acpi_video0 backlight registration
>> as soon as *one* of the *two* GPUs has not found an internal
>> LCD panel.
>>
>> As discussed above, after the backlight refactor,
>> GPU(KMS) drivers are expected to call
>> acpi_video_register_backlight() when necessary for any
>> internal panels connected to the GPU they are driving.
>>
>> This mostly fixes the issue of having an acpi_video0 on
>> desktop APUs, except that the timeout thingie which was
>> added to avoid regressions still causes the acpi_video0
>> backlight to get registered.
>>
>> Note that this timeout is already configurable through
>> the register_backlight_delay module option; and setting
>> that option to 0 disables the timeout based fallback
>> completely.
>>
>> So another fix for this might be to just change the
>> default value of register_backlight_delay to 0 for
>> kernel 6.2 .  This is a change which I want to make
>> eventually anyways; so we might just as well do this
>> now to fix the spurious acpi_video0 on desktop APUs
>> issue.   And if this does cause issues for nvidia
>> binary driver users, they can easily work around this
>> by setting the module option.
>>
>> Or alternatively we could go with this series,
>> reworked so that calling acpi_video_report_nolcd()
>> only cancels the timeout.  This way drivers for another
>> GPU can still get the acpi_video0 if necessary by
>> explicitly calling acpi_video_register_backlight().
>>
>> Personally I have a small preference for just changing
>> the default of register_backlight_delay to 0, disabling
>> the timeout based fallback starting with 6.2 .
> 
> How about we do both?  Then you can always restore register_backlight_delay without risk of introducing
> regression of acpi_video0 coming back to desktop APU's.

Doing both sounds like a good idea, I like it.

It would be great if you can rework the series to just cancel
the timeout from acpi_video_report_nolcd() + add a patch
to change the default register_backlight_delay to 0.

>> I did not do this for 6.1 because there were already
>> many other backlight changes in 6.1, so I wanted to
>> have the fallback behavior there as a safeguard
>> against things not working as planned.
>>
> 
> If you're open to my idea of both since I'm already
> touching all this anyway I am fine to roll that change
> into another patch for default of 0 too in a v2.

Adding the patch for default of 0 sounds great, thanks.

Regards,

Hans

Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Hans de Goede]

Hi,

On 12/7/22 22:21, Limonciello, Mario wrote:
> On 12/7/2022 15:04, Hans de Goede wrote:
>> Hi All,
>>
>> Mario, thank you for working on this.
> 
> Sure
> 
> <snip>
>>
>> Note that the problem of the creating a non functional acpi_video0
>> device happened before the overhaul too.
>>
>> The difference is that now we have the in kernel GPU drivers
>> all call acpi_video_register_backlight() when they detect an
>> internal-panel for which they don't provide native-backlight
>> control themselves (to avoid the acpi_video0 backlight registering
>> before the native backlight gets a chance to register).
>>
>> The timeout is only there in case no drivers ever call
>> acpi_video_register_backlight(). nomodeset is one case, but
>> loosing backlight control in the nomodeset case would be fine
>> IMHO. The bigger worry why we have the timeout is because of
>> the nvidia binary driver, for devices which use that driver +
>> rely on apci_video# for backlight control.
>>
>> Back to the issue at hand of the unwanted (non functional)
>> apci_video# on various AMD APU using desktops.
>>
> 
> Thanks for explaining.
> 
>> The native drivers now all calling acpi_video_register_backlight()
>> gives us a chance to actually do something about it, so in that
>> sense the 6.1 backlight refactor is relevant.
>>
>>> To avoid this situation from happening add support for video drivers
>>> to notify the ACPI video detection code that no panel was detected.
>>>
>>> To reduce the risk of regressions on multi-GPU systems:
>>> * only use this logic when the system is reported as a desktop enclosure.
>>> * in the amdgpu code only report into this for APUs.
>>
>> I'm afraid that there still is a potential issue for dual
>> GPU machines. The chassistype is not 100% reliable.
> 
> Have you ever seen an A+N machine with unreliable chassis type?

Not specifically. I just know from experience to not
rely on chassis type.

E.g. I would not be surprised to have some of the desktop-replacement
class laptops from e.g. clevo which sometimes even come with
a desktop CPU for moar power, have their chassis type wrong.

Granted those are not using AMD APUs (yet), but that might change
with the ryzen 7000 series where every CPU is an APU too...

> Given Windows HLK certification and knowing that these are to
> be based off reference BIOS of laptops, I would be really surprised
> if this was wrong on an A+N laptop.

I agree this is unlikely. But I have seen all sort of wrong
chassis-type settings in devices which are not from the
big OEMs.  And AFAIK these sometimes also play fasr and loose
with the Windows certification.

>> Lets say we have a machine with the wrong chassis-type with
>> an AMD APU + nvidia GPU which relies on acpi_video0 for
>> backlight control.
>>
>> Then if the LCD is connected to the nvidia GPU only, the
>> amdgpu code will call the new acpi_video_report_nolcd()
>> function.
> 
> + Dan Dadap
> 
> Dan - the context is this series:
> https://patchwork.freedesktop.org/series/111745/
> 
> Do you know if this is real or just conceptual?
> 
>>
>> And then even if the nvidia binary driver is patched
>> by nvidia to call the new  acpi_video_register_backlight()
>> when it does see a panel, then acpi_video_should_register_backlight()
>> will still return false.
>>
>> Basically the problem is that we only want to not try
>> and register the acpi_video0 backlight on dual GPU
>> machines if the output detection on *both* GPUs has not
>> found any builtin LCD panel.
>>
>> But this series disables acpi_video0 backlight registration
>> as soon as *one* of the *two* GPUs has not found an internal
>> LCD panel.
>>
>> As discussed above, after the backlight refactor,
>> GPU(KMS) drivers are expected to call
>> acpi_video_register_backlight() when necessary for any
>> internal panels connected to the GPU they are driving.
>>
>> This mostly fixes the issue of having an acpi_video0 on
>> desktop APUs, except that the timeout thingie which was
>> added to avoid regressions still causes the acpi_video0
>> backlight to get registered.
>>
>> Note that this timeout is already configurable through
>> the register_backlight_delay module option; and setting
>> that option to 0 disables the timeout based fallback
>> completely.
>>
>> So another fix for this might be to just change the
>> default value of register_backlight_delay to 0 for
>> kernel 6.2 .  This is a change which I want to make
>> eventually anyways; so we might just as well do this
>> now to fix the spurious acpi_video0 on desktop APUs
>> issue.   And if this does cause issues for nvidia
>> binary driver users, they can easily work around this
>> by setting the module option.
>>
>> Or alternatively we could go with this series,
>> reworked so that calling acpi_video_report_nolcd()
>> only cancels the timeout.  This way drivers for another
>> GPU can still get the acpi_video0 if necessary by
>> explicitly calling acpi_video_register_backlight().
>>
>> Personally I have a small preference for just changing
>> the default of register_backlight_delay to 0, disabling
>> the timeout based fallback starting with 6.2 .
> 
> How about we do both?  Then you can always restore register_backlight_delay without risk of introducing
> regression of acpi_video0 coming back to desktop APU's.

Doing both sounds like a good idea, I like it.

It would be great if you can rework the series to just cancel
the timeout from acpi_video_report_nolcd() + add a patch
to change the default register_backlight_delay to 0.

>> I did not do this for 6.1 because there were already
>> many other backlight changes in 6.1, so I wanted to
>> have the fallback behavior there as a safeguard
>> against things not working as planned.
>>
> 
> If you're open to my idea of both since I'm already
> touching all this anyway I am fine to roll that change
> into another patch for default of 0 too in a v2.

Adding the patch for default of 0 sounds great, thanks.

Regards,

Hans

Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Daniel Dadap]

On Wed, Dec 07, 2022 at 10:32:05PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/7/22 22:21, Limonciello, Mario wrote:
> > On 12/7/2022 15:04, Hans de Goede wrote:
> >> Hi All,
> >>
> >> Mario, thank you for working on this.
> > 
> > Sure
> > 
> > <snip>
> >>
> >> Note that the problem of the creating a non functional acpi_video0
> >> device happened before the overhaul too.
> >>
> >> The difference is that now we have the in kernel GPU drivers
> >> all call acpi_video_register_backlight() when they detect an
> >> internal-panel for which they don't provide native-backlight
> >> control themselves (to avoid the acpi_video0 backlight registering
> >> before the native backlight gets a chance to register).
> >>
> >> The timeout is only there in case no drivers ever call
> >> acpi_video_register_backlight(). nomodeset is one case, but
> >> loosing backlight control in the nomodeset case would be fine
> >> IMHO. The bigger worry why we have the timeout is because of
> >> the nvidia binary driver, for devices which use that driver +
> >> rely on apci_video# for backlight control.
> >>
> >> Back to the issue at hand of the unwanted (non functional)
> >> apci_video# on various AMD APU using desktops.
> >>
> > 
> > Thanks for explaining.
> > 
> >> The native drivers now all calling acpi_video_register_backlight()
> >> gives us a chance to actually do something about it, so in that
> >> sense the 6.1 backlight refactor is relevant.
> >>
> >>> To avoid this situation from happening add support for video drivers
> >>> to notify the ACPI video detection code that no panel was detected.
> >>>
> >>> To reduce the risk of regressions on multi-GPU systems:
> >>> * only use this logic when the system is reported as a desktop enclosure.
> >>> * in the amdgpu code only report into this for APUs.
> >>
> >> I'm afraid that there still is a potential issue for dual
> >> GPU machines. The chassistype is not 100% reliable.
> > 
> > Have you ever seen an A+N machine with unreliable chassis type?
> 
> Not specifically. I just know from experience to not
> rely on chassis type.
> 
> E.g. I would not be surprised to have some of the desktop-replacement
> class laptops from e.g. clevo which sometimes even come with
> a desktop CPU for moar power, have their chassis type wrong.
> 
> Granted those are not using AMD APUs (yet), but that might change
> with the ryzen 7000 series where every CPU is an APU too...
> 
> > Given Windows HLK certification and knowing that these are to
> > be based off reference BIOS of laptops, I would be really surprised
> > if this was wrong on an A+N laptop.
> 
> I agree this is unlikely. But I have seen all sort of wrong
> chassis-type settings in devices which are not from the
> big OEMs.  And AFAIK these sometimes also play fasr and loose
> with the Windows certification.
> 
> >> Lets say we have a machine with the wrong chassis-type with
> >> an AMD APU + nvidia GPU which relies on acpi_video0 for
> >> backlight control.
> >>
> >> Then if the LCD is connected to the nvidia GPU only, the
> >> amdgpu code will call the new acpi_video_report_nolcd()
> >> function.
> > 
> > + Dan Dadap
> > 
> > Dan - the context is this series:
> > https://patchwork.freedesktop.org/series/111745/
> > 
> > Do you know if this is real or just conceptual?

I'm not aware of any specific examples of an A+N system with the
incorrect chassis type, but I agree that relying on it to be accurate
seems a bit fragile. Besides the "notebook that says it's a desktop"
possibility that Hans speculated on, I could imagine e.g. an All-in-One
form factor system, whose design is more similar to a notebook,
reporting its chassis type as desktop.
 
> >>
> >> And then even if the nvidia binary driver is patched
> >> by nvidia to call the new  acpi_video_register_backlight()
> >> when it does see a panel, then acpi_video_should_register_backlight()
> >> will still return false.
> >>
> >> Basically the problem is that we only want to not try
> >> and register the acpi_video0 backlight on dual GPU
> >> machines if the output detection on *both* GPUs has not
> >> found any builtin LCD panel.
> >>
> >> But this series disables acpi_video0 backlight registration
> >> as soon as *one* of the *two* GPUs has not found an internal
> >> LCD panel.

Yeah, it does seem a little backwards to have the drivers report that
they do not see any panels, when we don't know whether there might be a
panel on another GPU whose driver hasn't registered its native backlight
handler yet. I trust the DRM drivers reporting whether a panel is
internal more than I'd trust the DMI chassis type - "positive" reporting
when a GPU driver finds an internal panel seems like it would be more
reliable than "negative" reporting when a GPU driver does not find any
internal panels. IIUC, that's the intent with having the DRM-KMS drivers
explicitly call acpi_video_register_backlight() if needed, as described
below.

> >> As discussed above, after the backlight refactor,
> >> GPU(KMS) drivers are expected to call
> >> acpi_video_register_backlight() when necessary for any
> >> internal panels connected to the GPU they are driving.
> >>
> >> This mostly fixes the issue of having an acpi_video0 on
> >> desktop APUs, except that the timeout thingie which was
> >> added to avoid regressions still causes the acpi_video0
> >> backlight to get registered.
> >>
> >> Note that this timeout is already configurable through
> >> the register_backlight_delay module option; and setting
> >> that option to 0 disables the timeout based fallback
> >> completely.
> >>
> >> So another fix for this might be to just change the
> >> default value of register_backlight_delay to 0 for
> >> kernel 6.2 .  This is a change which I want to make
> >> eventually anyways; so we might just as well do this
> >> now to fix the spurious acpi_video0 on desktop APUs
> >> issue.   And if this does cause issues for nvidia
> >> binary driver users, they can easily work around this
> >> by setting the module option.
> >>
> >> Or alternatively we could go with this series,
> >> reworked so that calling acpi_video_report_nolcd()
> >> only cancels the timeout.  This way drivers for another
> >> GPU can still get the acpi_video0 if necessary by
> >> explicitly calling acpi_video_register_backlight().
> >>
> >> Personally I have a small preference for just changing
> >> the default of register_backlight_delay to 0, disabling
> >> the timeout based fallback starting with 6.2 .
> > 
> > How about we do both?  Then you can always restore register_backlight_delay without risk of introducing
> > regression of acpi_video0 coming back to desktop APU's.
> 
> Doing both sounds like a good idea, I like it.

I suppose if there is a reason this needs to be structured with the
"negative" reporting, even on systems with multiple GPUs, that at least
one of the GPU drivers that *does* have an internal panel can decide
whether to register a native handler, or explicitly register the ACPI
video backlight handler. And even on an A+N system with a mux (and
without a GPU-agnostic backlight interface like nvidia-wmi-ec-backlight),
I'd expect that the system would POST to the (AMD) iGPU, and the panel
should be connected to the iGPU when the DRM drivers load. I guess what
I'm saying is I don't really see any value in the dmi_is_desktop() check
since I'd expect amdgpu to be the driver which owns the panel at boot
time, and even if that ends up not being the case, the dGPU driver can
fix things up later.
 
> It would be great if you can rework the series to just cancel
> the timeout from acpi_video_report_nolcd() + add a patch
> to change the default register_backlight_delay to 0.
> 
> >> I did not do this for 6.1 because there were already
> >> many other backlight changes in 6.1, so I wanted to
> >> have the fallback behavior there as a safeguard
> >> against things not working as planned.
> >>
> > 
> > If you're open to my idea of both since I'm already
> > touching all this anyway I am fine to roll that change
> > into another patch for default of 0 too in a v2.
> 
> Adding the patch for default of 0 sounds great, thanks.
> 
> Regards,
> 
> Hans
> 
> 
> 

Re: [PATCH 0/2] Avoid creating acpi_video0 on desktop APUs

[Daniel Dadap]

On Wed, Dec 07, 2022 at 10:32:05PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/7/22 22:21, Limonciello, Mario wrote:
> > On 12/7/2022 15:04, Hans de Goede wrote:
> >> Hi All,
> >>
> >> Mario, thank you for working on this.
> > 
> > Sure
> > 
> > <snip>
> >>
> >> Note that the problem of the creating a non functional acpi_video0
> >> device happened before the overhaul too.
> >>
> >> The difference is that now we have the in kernel GPU drivers
> >> all call acpi_video_register_backlight() when they detect an
> >> internal-panel for which they don't provide native-backlight
> >> control themselves (to avoid the acpi_video0 backlight registering
> >> before the native backlight gets a chance to register).
> >>
> >> The timeout is only there in case no drivers ever call
> >> acpi_video_register_backlight(). nomodeset is one case, but
> >> loosing backlight control in the nomodeset case would be fine
> >> IMHO. The bigger worry why we have the timeout is because of
> >> the nvidia binary driver, for devices which use that driver +
> >> rely on apci_video# for backlight control.
> >>
> >> Back to the issue at hand of the unwanted (non functional)
> >> apci_video# on various AMD APU using desktops.
> >>
> > 
> > Thanks for explaining.
> > 
> >> The native drivers now all calling acpi_video_register_backlight()
> >> gives us a chance to actually do something about it, so in that
> >> sense the 6.1 backlight refactor is relevant.
> >>
> >>> To avoid this situation from happening add support for video drivers
> >>> to notify the ACPI video detection code that no panel was detected.
> >>>
> >>> To reduce the risk of regressions on multi-GPU systems:
> >>> * only use this logic when the system is reported as a desktop enclosure.
> >>> * in the amdgpu code only report into this for APUs.
> >>
> >> I'm afraid that there still is a potential issue for dual
> >> GPU machines. The chassistype is not 100% reliable.
> > 
> > Have you ever seen an A+N machine with unreliable chassis type?
> 
> Not specifically. I just know from experience to not
> rely on chassis type.
> 
> E.g. I would not be surprised to have some of the desktop-replacement
> class laptops from e.g. clevo which sometimes even come with
> a desktop CPU for moar power, have their chassis type wrong.
> 
> Granted those are not using AMD APUs (yet), but that might change
> with the ryzen 7000 series where every CPU is an APU too...
> 
> > Given Windows HLK certification and knowing that these are to
> > be based off reference BIOS of laptops, I would be really surprised
> > if this was wrong on an A+N laptop.
> 
> I agree this is unlikely. But I have seen all sort of wrong
> chassis-type settings in devices which are not from the
> big OEMs.  And AFAIK these sometimes also play fasr and loose
> with the Windows certification.
> 
> >> Lets say we have a machine with the wrong chassis-type with
> >> an AMD APU + nvidia GPU which relies on acpi_video0 for
> >> backlight control.
> >>
> >> Then if the LCD is connected to the nvidia GPU only, the
> >> amdgpu code will call the new acpi_video_report_nolcd()
> >> function.
> > 
> > + Dan Dadap
> > 
> > Dan - the context is this series:
> > https://patchwork.freedesktop.org/series/111745/
> > 
> > Do you know if this is real or just conceptual?

I'm not aware of any specific examples of an A+N system with the
incorrect chassis type, but I agree that relying on it to be accurate
seems a bit fragile. Besides the "notebook that says it's a desktop"
possibility that Hans speculated on, I could imagine e.g. an All-in-One
form factor system, whose design is more similar to a notebook,
reporting its chassis type as desktop.
 
> >>
> >> And then even if the nvidia binary driver is patched
> >> by nvidia to call the new  acpi_video_register_backlight()
> >> when it does see a panel, then acpi_video_should_register_backlight()
> >> will still return false.
> >>
> >> Basically the problem is that we only want to not try
> >> and register the acpi_video0 backlight on dual GPU
> >> machines if the output detection on *both* GPUs has not
> >> found any builtin LCD panel.
> >>
> >> But this series disables acpi_video0 backlight registration
> >> as soon as *one* of the *two* GPUs has not found an internal
> >> LCD panel.

Yeah, it does seem a little backwards to have the drivers report that
they do not see any panels, when we don't know whether there might be a
panel on another GPU whose driver hasn't registered its native backlight
handler yet. I trust the DRM drivers reporting whether a panel is
internal more than I'd trust the DMI chassis type - "positive" reporting
when a GPU driver finds an internal panel seems like it would be more
reliable than "negative" reporting when a GPU driver does not find any
internal panels. IIUC, that's the intent with having the DRM-KMS drivers
explicitly call acpi_video_register_backlight() if needed, as described
below.

> >> As discussed above, after the backlight refactor,
> >> GPU(KMS) drivers are expected to call
> >> acpi_video_register_backlight() when necessary for any
> >> internal panels connected to the GPU they are driving.
> >>
> >> This mostly fixes the issue of having an acpi_video0 on
> >> desktop APUs, except that the timeout thingie which was
> >> added to avoid regressions still causes the acpi_video0
> >> backlight to get registered.
> >>
> >> Note that this timeout is already configurable through
> >> the register_backlight_delay module option; and setting
> >> that option to 0 disables the timeout based fallback
> >> completely.
> >>
> >> So another fix for this might be to just change the
> >> default value of register_backlight_delay to 0 for
> >> kernel 6.2 .  This is a change which I want to make
> >> eventually anyways; so we might just as well do this
> >> now to fix the spurious acpi_video0 on desktop APUs
> >> issue.   And if this does cause issues for nvidia
> >> binary driver users, they can easily work around this
> >> by setting the module option.
> >>
> >> Or alternatively we could go with this series,
> >> reworked so that calling acpi_video_report_nolcd()
> >> only cancels the timeout.  This way drivers for another
> >> GPU can still get the acpi_video0 if necessary by
> >> explicitly calling acpi_video_register_backlight().
> >>
> >> Personally I have a small preference for just changing
> >> the default of register_backlight_delay to 0, disabling
> >> the timeout based fallback starting with 6.2 .
> > 
> > How about we do both?  Then you can always restore register_backlight_delay without risk of introducing
> > regression of acpi_video0 coming back to desktop APU's.
> 
> Doing both sounds like a good idea, I like it.

I suppose if there is a reason this needs to be structured with the
"negative" reporting, even on systems with multiple GPUs, that at least
one of the GPU drivers that *does* have an internal panel can decide
whether to register a native handler, or explicitly register the ACPI
video backlight handler. And even on an A+N system with a mux (and
without a GPU-agnostic backlight interface like nvidia-wmi-ec-backlight),
I'd expect that the system would POST to the (AMD) iGPU, and the panel
should be connected to the iGPU when the DRM drivers load. I guess what
I'm saying is I don't really see any value in the dmi_is_desktop() check
since I'd expect amdgpu to be the driver which owns the panel at boot
time, and even if that ends up not being the case, the dGPU driver can
fix things up later.
 
> It would be great if you can rework the series to just cancel
> the timeout from acpi_video_report_nolcd() + add a patch
> to change the default register_backlight_delay to 0.
> 
> >> I did not do this for 6.1 because there were already
> >> many other backlight changes in 6.1, so I wanted to
> >> have the fallback behavior there as a safeguard
> >> against things not working as planned.
> >>
> > 
> > If you're open to my idea of both since I'm already
> > touching all this anyway I am fine to roll that change
> > into another patch for default of 0 too in a v2.
> 
> Adding the patch for default of 0 sounds great, thanks.
> 
> Regards,
> 
> Hans
> 
> 
>