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

[Limonciello, Mario]

On 12/8/2022 11:49, Daniel Dadap wrote:
> On Thu, Dec 08, 2022 at 10:42:05AM -0600, Mario Limonciello wrote:
>> 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 enable the fallback logic for making an acpi_video0
>> backlight device.
>>
>> Suggested-by: Hans de Goede <hdegoede at redhat.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>> v2->v3:
>>   * Add Hans' R-b
>>   * Add missing declaration for non CONFIG_ACPI_VIDEO case
>> v1->v2:
>>   * Cancel registration for backlight device instead (Hans)
>>   * drop desktop check (Dan)
>> ---
>>   drivers/acpi/acpi_video.c | 11 +++++++++++
>>   include/acpi/video.h      |  2 ++
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index 32953646caeb..f64fdb029090 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -2178,6 +2178,17 @@ static bool should_check_lcd_flag(void)
>>   	return false;
>>   }
>>   
>> +/*
>> + * At least one graphics driver has reported that no LCD is connected
>> + * via the native interface. cancel the registration for fallback acpi_video0.
>> + * If another driver still deems this necessary, it can explicitly register it.
>> + */
>> +void acpi_video_report_nolcd(void)
>> +{
>> +	cancel_delayed_work(&video_bus_register_backlight_work);
>> +}
>> +EXPORT_SYMBOL(acpi_video_report_nolcd);
>> +
> 
> Thanks for removing the desktop check.

Sure.

> 
> It's not entirely clear to me what happens if you try to cancel a
> delayed work that was never scheduled. I got as far as determining that
> del_timer() in kernel/time/timer.c will probably return 0, but I didn't
> really feel like walking through the rest of try_to_grab_pending() to
> figure out what happens next. You've probably already tested this with
> the default disabled timer, so as long as nothing bad happened there,
> this seems fine.
> 

Yeah; I did test it and nothing blew up during my test.

> This is probably overly complicated, so if you think it's worth doing, I
> would definitely add it later, but I wonder if it might make sense to
> pass an acpi_handle to a _BC[LM] method or one of its parents, so that
> this could be scoped to a particular device. Looking at the ACPI table
> dump from a random multi-GPU laptop, it looks like there are two
> instances of _BCL, one under _SB.GP<number>.VGA.LCD for the iGPU, and
> the other under _SB.PCI<num>.GPP<num>.PEGP.EDP<num> for the dGPU. The
> caller would pass in handles for methods/devices that it will handle,
> and then the fallback, if it runs at all, would skip any handles that
> were registered with it when it crawls for _BC[LM]. Or the equivalent
> inverse logic, or something else like that. I think it's probably fine
> to keep the current unscoped design and just assert that any other GPU
> drivers that want the ACPI video driver to handle panel backlight should
> register it explicitly; if for some reason that ends up not working out,
> we could revisit scoping it then.

Yeah that is a lot more complex but complete setup.  I think if we end 
up having to revert the 3rd patch and having GPU drivers call the 
registration explicitly isn't a good idea for some reason it's worth 
considering.

> 
>>   int acpi_video_register(void)
>>   {
>>   	int ret = 0;
>> diff --git a/include/acpi/video.h b/include/acpi/video.h
>> index a275c35e5249..a56c8d45e9f8 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);
>> @@ -69,6 +70,7 @@ extern int acpi_video_get_levels(struct acpi_device *device,
>>   				 struct acpi_video_device_brightness **dev_br,
>>   				 int *pmax_level);
>>   #else
>> +static inline void acpi_video_report_nolcd(void) { return; };
>>   static inline int acpi_video_register(void) { return -ENODEV; }
>>   static inline void acpi_video_unregister(void) { return; }
>>   static inline void acpi_video_register_backlight(void) { return; }
>> -- 
>> 2.34.1
>>