Re: [Intel-xe] [PATCH] drm/xe/display: Use acpi_target_system_state only if ACPI_SLEEP is enabled

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 05 Sep 2023, Francois Dugast <francois.dugast@intel.com> wrote:
> On Mon, Sep 04, 2023 at 01:12:02PM +0300, Jani Nikula wrote:
>> On Fri, 01 Sep 2023, Francois Dugast <francois.dugast@intel.com> wrote:
>> > This fixes the build when ACPI_SLEEP is disabled.
>> 
>> What is the build failure exactly? It's usually helpful to copy-paste
>> the error in the commit message.
>
> Sure, I will add it to the commit message in the next revision. Here it is:
>
> drivers/gpu/drm/xe/xe_display.c:334:23: error: implicit declaration of function ‘acpi_target_system_state’; did you mean ‘acpi_get_system_info’? [-Werror=implicit-function-declaration]
>   334 |         bool s2idle = acpi_target_system_state() < ACPI_STATE_S3;
>
>> 
>> acpi_target_system_state() should evaluate to ACPI_STATE_S0 for
>> CONFIG_ACPI_SLEEP=n, and the changes here would be unnecessary.
>> 
>> Most likely the problem is CONFIG_ACPI=n leading to <acpi/acpi_bus.h>
>> not being included at all, and the function not being present. Directly
>> including <acpi/acpi_bus.h> would be the cleaner fix.
>
> It seems that with CONFIG_ACPI_SLEEP=n, acpi_target_sleep_state evaluates to
> ACPI_STATE_S0 but acpi_target_system_state is not defined as it is compiled out
> in acpi/sleep.c, which causes the error above. Including <acpi/acpi_bus.h> does
> not help.

Meh. I was lookig at sleep.h defining it as static inline for
CONFIG_ACPI_SLEEP=n:

#ifdef CONFIG_ACPI_SLEEP
u32 acpi_target_system_state(void);
#else
static inline u32 acpi_target_system_state(void) { return ACPI_STATE_S0; }
#endif

But the whole thing is wrapped inside #ifdef CONFIG_ACPI.

i915_driver.c has a wrapper for this, suspend_to_idle(). I guess have to
mimic that then to not duplicate the #ifdefs.


BR,
Jani.


>
> Regards,
> Francois
>
>> 
>> BR,
>> Jani.
>> 
>> >
>> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
>> > ---
>> >  drivers/gpu/drm/xe/xe_display.c | 14 ++++++++++++--
>> >  1 file changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_display.c b/drivers/gpu/drm/xe/xe_display.c
>> > index b6b547c4877c..54f52b6d702d 100644
>> > --- a/drivers/gpu/drm/xe/xe_display.c
>> > +++ b/drivers/gpu/drm/xe/xe_display.c
>> > @@ -331,7 +331,12 @@ static void intel_suspend_encoders(struct xe_device *xe)
>> >  
>> >  void xe_display_pm_suspend(struct xe_device *xe)
>> >  {
>> > -	bool s2idle = acpi_target_system_state() < ACPI_STATE_S3;
>> > +	bool s2idle;
>> > +#if IS_ENABLED(CONFIG_ACPI_SLEEP)
>> > +	s2idle = acpi_target_system_state() < ACPI_STATE_S3;
>> > +#else
>> > +	s2idle = true;
>> > +#endif
>> >  	if (!xe->info.enable_display)
>> >  		return;
>> >  
>> > @@ -360,7 +365,12 @@ void xe_display_pm_suspend(struct xe_device *xe)
>> >  
>> >  void xe_display_pm_suspend_late(struct xe_device *xe)
>> >  {
>> > -	bool s2idle = acpi_target_system_state() < ACPI_STATE_S3;
>> > +	bool s2idle;
>> > +#if IS_ENABLED(CONFIG_ACPI_SLEEP)
>> > +	s2idle = acpi_target_system_state() < ACPI_STATE_S3;
>> > +#else
>> > +	s2idle = true;
>> > +#endif
>> >  	if (!xe->info.enable_display)
>> >  		return;
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center