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

["Limonciello, Mario" <mario.limonciello@amd.com>]

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.