Message ID | 20230704074506.2304939-1-kai.heng.feng@canonical.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f45:0:b0:3ea:f831:8777 with SMTP id v5csp1045644vqx; Tue, 4 Jul 2023 00:53:12 -0700 (PDT) X-Google-Smtp-Source: APBJJlHoh4AA8m5JggvHeCMu16ImCSiSEm8Ybgvjr+3cDuoQThqMHbFIXoE8me8ggvWLN0/w4ZYd X-Received: by 2002:a17:902:e989:b0:1b6:b44d:a5b6 with SMTP id f9-20020a170902e98900b001b6b44da5b6mr8897939plb.14.1688457192482; Tue, 04 Jul 2023 00:53:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1688457192; cv=none; d=google.com; s=arc-20160816; b=mjThaWBLSzd5qSML6IvdWBDh5OduG8tsEGnenTM0ubnc1Sa2XzPNWQRu0C5pzKUiIZ 3rNSAeUnhuh6Mhb/O/Ux04mnMB7PjNFtFbqu223z301dX43THRFJax4J1jVIC0t4I5TW 3PSl+CshjVfaBRt4l3g+7X/GCKodxa3qTMVoZsVp4r9plxHLeCfP1tDvjm7kAxTUCkSI jh+2uev+sCQZ2LWhoW32XlbdGLM3QDrm7N1xR8HY5j+6F4rHUWa9zL6F1c77kcm0X9uZ vGr6jQ2J+9Ushmig+cQcYpI3uaJPuMpLeQ+y5m1qQABjd1IjflItPg9wfBtC9W84IjB+ nEjQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=lkUhVHLr2kT3Uz1Nhg29KNs1j5fZGCeKmuPwJkP9IQE=; fh=9tfvK4rssieCsxrsHqXzYnEP+fEijpKpYFUFUmNm+BU=; b=fHyNoeifdgL2JBefirVcnHXz6nfCm7gCoSMjSmZjATyIZ0tbEV4YaWYz0eiFokd0P3 oqWkHu3+xkEgW15DPyIvAvEABd/DEcsMs/O+jF/BPD3/JQfQ8ighf8nU7E8E8tNKMuJU Gd1HC97GsHfdW/HYqnJ/xH3GzBDS1wo2I9yqi0cP9h94p7SJTLUTmeGORamduHOYYEIC Mo2iFgyUEODF3E++TKVowsZv0JG/S6QIgOS3WNNtnX1uM8NRiXNchgFCPmz89vC9cQqd jGaI9+oNFZoV/wxBPixxbvbl5EzgHgCrWhue3oXygeBS1uF5HTJs6lgqYQXe3/zWDJJo 8ppw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@canonical.com header.s=20210705 header.b=bob7LbA0; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=canonical.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j7-20020a17090276c700b001b22f31cce9si19348906plt.179.2023.07.04.00.52.57; Tue, 04 Jul 2023 00:53:12 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@canonical.com header.s=20210705 header.b=bob7LbA0; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=canonical.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231534AbjGDHqe (ORCPT <rfc822;gnulinuxfreebsd@gmail.com> + 99 others); Tue, 4 Jul 2023 03:46:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55352 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231373AbjGDHqD (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 4 Jul 2023 03:46:03 -0400 Received: from smtp-relay-canonical-0.canonical.com (smtp-relay-canonical-0.canonical.com [185.125.188.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F288BCA; Tue, 4 Jul 2023 00:45:59 -0700 (PDT) Received: from localhost.localdomain (unknown [10.101.196.174]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-canonical-0.canonical.com (Postfix) with ESMTPSA id BD81F41491; Tue, 4 Jul 2023 07:45:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1688456758; bh=lkUhVHLr2kT3Uz1Nhg29KNs1j5fZGCeKmuPwJkP9IQE=; h=From:To:Cc:Subject:Date:Message-Id:MIME-Version; b=bob7LbA0qP3MsdGQiWOb4iskOLfQt5PyJirGVsfuJFBis7NQ6LFQ6mLe/zxqpBma2 SmLT4omedm1GBkTWP7bxnGguLvxZ3bRTAkYLK3WzdCawyYLjWalmflqzm/b5Xgns0M DzUPwe04IzeK+P6uVOyksz3FjlmpI2+SOe3gGWAtKD4LPLU6UItv4Pem1u3Aatz4A4 wt/FZUtMOhtSjbAjxrBMm9IZE6JPd7KhTUTpLhv7FDo/0xHADI9eU1pMGlRsTZc4DM 4JhVwLoxg7eOfx3NXntnQC3wtin0CAjnI7mTG0VaDYG95ylZD8vWUqSVXEZ7uF4Jix 0AyNjZCBs+Pag== From: Kai-Heng Feng <kai.heng.feng@canonical.com> To: rafael@kernel.org, lenb@kernel.org Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] ACPI: video: Invoke _PS0 at boot for ACPI video Date: Tue, 4 Jul 2023 15:45:06 +0800 Message-Id: <20230704074506.2304939-1-kai.heng.feng@canonical.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1770475689529938627?= X-GMAIL-MSGID: =?utf-8?q?1770475689529938627?= |
Series |
ACPI: video: Invoke _PS0 at boot for ACPI video
|
|
Commit Message
Kai-Heng Feng
July 4, 2023, 7:45 a.m. UTC
Screen brightness can only be changed once on some HP laptops.
Vendor identified the root cause as Linux doesn't invoke _PS0 at boot
for all ACPI devices:
Scope (\_SB.PC00.GFX0)
{
Scope (DD1F)
{
Method (_PS0, 0, Serialized) // _PS0: Power State 0
{
If (CondRefOf (\_SB.PC00.LPCB.EC0.SSBC))
{
\_SB.PC00.LPCB.EC0.SSBC ()
}
}
...
}
...
}
_PS0 doesn't get invoked for all ACPI devices because of commit
7cd8407d53ef ("ACPI / PM: Do not execute _PS0 for devices without _PSC
during initialization").
For now explicitly call _PS0 for ACPI video to workaround the issue.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/acpi/acpi_video.c | 2 ++
1 file changed, 2 insertions(+)
Comments
On Tue, Jul 4, 2023 at 9:46 AM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Screen brightness can only be changed once on some HP laptops. > > Vendor identified the root cause as Linux doesn't invoke _PS0 at boot > for all ACPI devices: This part of the changelog is confusing, because the evaluation of _PS0 is not a separate operation. _PS0 gets evaluated when devices undergo transitions from low-power states to D0. > Scope (\_SB.PC00.GFX0) > { > Scope (DD1F) > { > Method (_PS0, 0, Serialized) // _PS0: Power State 0 > { > If (CondRefOf (\_SB.PC00.LPCB.EC0.SSBC)) > { > \_SB.PC00.LPCB.EC0.SSBC () > } > } > ... > } > ... > } > > _PS0 doesn't get invoked for all ACPI devices because of commit > 7cd8407d53ef ("ACPI / PM: Do not execute _PS0 for devices without _PSC > during initialization"). And yes, Linux doesn't put all of the ACPI devices into D0 during initialization, but the above commit has a little to do with that. > For now explicitly call _PS0 for ACPI video to workaround the issue. This is not what the patch is doing. > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/acpi/acpi_video.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > index 62f4364e4460..793259bd18c8 100644 > --- a/drivers/acpi/acpi_video.c > +++ b/drivers/acpi/acpi_video.c > @@ -2027,6 +2027,8 @@ static int acpi_video_bus_add(struct acpi_device *device) > if (error) > goto err_put_video; > > + acpi_device_fix_up_power_extended(device); > + I would like to know what Hans thinks about this. > pr_info("%s [%s] (multi-head: %s rom: %s post: %s)\n", > ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device), > video->flags.multihead ? "yes" : "no", > --
Hi, On 7/4/23 18:58, Rafael J. Wysocki wrote: > On Tue, Jul 4, 2023 at 9:46 AM Kai-Heng Feng > <kai.heng.feng@canonical.com> wrote: >> >> Screen brightness can only be changed once on some HP laptops. >> >> Vendor identified the root cause as Linux doesn't invoke _PS0 at boot >> for all ACPI devices: > > This part of the changelog is confusing, because the evaluation of > _PS0 is not a separate operation. _PS0 gets evaluated when devices > undergo transitions from low-power states to D0. > >> Scope (\_SB.PC00.GFX0) >> { >> Scope (DD1F) >> { >> Method (_PS0, 0, Serialized) // _PS0: Power State 0 >> { >> If (CondRefOf (\_SB.PC00.LPCB.EC0.SSBC)) >> { >> \_SB.PC00.LPCB.EC0.SSBC () >> } >> } >> ... >> } >> ... >> } >> >> _PS0 doesn't get invoked for all ACPI devices because of commit >> 7cd8407d53ef ("ACPI / PM: Do not execute _PS0 for devices without _PSC >> during initialization"). So this _PS0 which seems to be the one which needs to run here, is not the _PS0 for the GFX0 ACPI device, but rather for a child ACPI device-node which describes the connector (assumed based on the small part of quoted DSDT, the actual definition of the DD1F device-node is missing). Having a _PS0 method on a connector object is really weird IMHO. But if we need to invoke such a _PS0 method then IMHO that really should be done in the drm/kms driver. E.g. at least the i915 code already contains code to map the ACPI connector objects to the drm_connector objects, so it should be relatively easily to make that try and do a power-transition to D0 when enabling the connector. Also can you provide some more info on the hw on which this is being seen: 1. What GPU(s) is/are being used 2. If there is a mux for hybrid gfx in which mode is the mux set ? 3. Wjich method is used to control the brightness (which backlight-class-devices show up under /sys/class/backlight) ? And can you add this info to the commit msg for the next version of the patch ? Regards, Hans > > And yes, Linux doesn't put all of the ACPI devices into D0 during > initialization, but the above commit has a little to do with that. > >> For now explicitly call _PS0 for ACPI video to workaround the issue. > > This is not what the patch is doing. > >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> drivers/acpi/acpi_video.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c >> index 62f4364e4460..793259bd18c8 100644 >> --- a/drivers/acpi/acpi_video.c >> +++ b/drivers/acpi/acpi_video.c >> @@ -2027,6 +2027,8 @@ static int acpi_video_bus_add(struct acpi_device *device) >> if (error) >> goto err_put_video; >> >> + acpi_device_fix_up_power_extended(device); >> + > > I would like to know what Hans thinks about this. > >> pr_info("%s [%s] (multi-head: %s rom: %s post: %s)\n", >> ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device), >> video->flags.multihead ? "yes" : "no", >> -- >
On Wed, Jul 5, 2023 at 12:59 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Jul 4, 2023 at 9:46 AM Kai-Heng Feng > <kai.heng.feng@canonical.com> wrote: > > > > Screen brightness can only be changed once on some HP laptops. > > > > Vendor identified the root cause as Linux doesn't invoke _PS0 at boot > > for all ACPI devices: > > This part of the changelog is confusing, because the evaluation of > _PS0 is not a separate operation. _PS0 gets evaluated when devices > undergo transitions from low-power states to D0. But not at boot. > > > Scope (\_SB.PC00.GFX0) > > { > > Scope (DD1F) > > { > > Method (_PS0, 0, Serialized) // _PS0: Power State 0 > > { > > If (CondRefOf (\_SB.PC00.LPCB.EC0.SSBC)) > > { > > \_SB.PC00.LPCB.EC0.SSBC () > > } > > } > > ... > > } > > ... > > } > > > > _PS0 doesn't get invoked for all ACPI devices because of commit > > 7cd8407d53ef ("ACPI / PM: Do not execute _PS0 for devices without _PSC > > during initialization"). > > And yes, Linux doesn't put all of the ACPI devices into D0 during > initialization, but the above commit has a little to do with that. Devices without _PSC now doesn't have _PS0 evaluated at boot time. I don't quite understand why it's not related to this commit? > > > For now explicitly call _PS0 for ACPI video to workaround the issue. > > This is not what the patch is doing. To be specific, it's for the child device nodes under ACPI GFX. Kai-Heng > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/acpi/acpi_video.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > > index 62f4364e4460..793259bd18c8 100644 > > --- a/drivers/acpi/acpi_video.c > > +++ b/drivers/acpi/acpi_video.c > > @@ -2027,6 +2027,8 @@ static int acpi_video_bus_add(struct acpi_device *device) > > if (error) > > goto err_put_video; > > > > + acpi_device_fix_up_power_extended(device); > > + > > I would like to know what Hans thinks about this. > > > pr_info("%s [%s] (multi-head: %s rom: %s post: %s)\n", > > ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device), > > video->flags.multihead ? "yes" : "no", > > --
On Wed, Jul 5, 2023 at 6:33 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 7/4/23 18:58, Rafael J. Wysocki wrote: > > On Tue, Jul 4, 2023 at 9:46 AM Kai-Heng Feng > > <kai.heng.feng@canonical.com> wrote: > >> > >> Screen brightness can only be changed once on some HP laptops. > >> > >> Vendor identified the root cause as Linux doesn't invoke _PS0 at boot > >> for all ACPI devices: > > > > This part of the changelog is confusing, because the evaluation of > > _PS0 is not a separate operation. _PS0 gets evaluated when devices > > undergo transitions from low-power states to D0. > > > >> Scope (\_SB.PC00.GFX0) > >> { > >> Scope (DD1F) > >> { > >> Method (_PS0, 0, Serialized) // _PS0: Power State 0 > >> { > >> If (CondRefOf (\_SB.PC00.LPCB.EC0.SSBC)) > >> { > >> \_SB.PC00.LPCB.EC0.SSBC () > >> } > >> } > >> ... > >> } > >> ... > >> } > >> > >> _PS0 doesn't get invoked for all ACPI devices because of commit > >> 7cd8407d53ef ("ACPI / PM: Do not execute _PS0 for devices without _PSC > >> during initialization"). > > So this _PS0 which seems to be the one which needs to run here, > is not the _PS0 for the GFX0 ACPI device, but rather for a child ACPI device-node which describes the connector (assumed based on the small part of quoted DSDT, the actual definition of the DD1F device-node is missing). I'll file a bugzilla and attach a full acpidump there. > > Having a _PS0 method on a connector object is really weird IMHO. But if we need to invoke such a _PS0 method then IMHO that really should be done in the drm/kms driver. E.g. at least the i915 code already contains code to map the ACPI connector objects to the drm_connector objects, so it should be relatively easily to make that try and do a power-transition to D0 when enabling the connector. Or put all ACPI devices to D0 at boot? According to the BIOS folks that's what Windows does. This way we can drop acpi_device_fix_up_power* helpers altogether. > > Also can you provide some more info on the hw on which this is being seen: > > 1. What GPU(s) is/are being used Intel GFX. AFAIK AMD based laptops also require this fixup too. > 2. If there is a mux for hybrid gfx in which mode is the mux set ? No. This happens to mux-less and dGPU-less laptops. > 3. Wjich method is used to control the brightness (which backlight-class-devices show up under /sys/class/backlight) ? intel_backlight. > > And can you add this info to the commit msg for the next version of the patch ? Sure. Can putting all devices to D0 be considered too? It's a better solution for the long wrong. Kai-Heng > > Regards, > > Hans > > > > > > > > And yes, Linux doesn't put all of the ACPI devices into D0 during > > initialization, but the above commit has a little to do with that. > > > >> For now explicitly call _PS0 for ACPI video to workaround the issue. > > > > This is not what the patch is doing. > > > >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >> --- > >> drivers/acpi/acpi_video.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > >> index 62f4364e4460..793259bd18c8 100644 > >> --- a/drivers/acpi/acpi_video.c > >> +++ b/drivers/acpi/acpi_video.c > >> @@ -2027,6 +2027,8 @@ static int acpi_video_bus_add(struct acpi_device *device) > >> if (error) > >> goto err_put_video; > >> > >> + acpi_device_fix_up_power_extended(device); > >> + > > > > I would like to know what Hans thinks about this. > > > >> pr_info("%s [%s] (multi-head: %s rom: %s post: %s)\n", > >> ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device), > >> video->flags.multihead ? "yes" : "no", > >> -- > > >
Hi, On 7/6/23 10:20, Kai-Heng Feng wrote: > On Wed, Jul 5, 2023 at 6:33 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 7/4/23 18:58, Rafael J. Wysocki wrote: >>> On Tue, Jul 4, 2023 at 9:46 AM Kai-Heng Feng >>> <kai.heng.feng@canonical.com> wrote: >>>> >>>> Screen brightness can only be changed once on some HP laptops. >>>> >>>> Vendor identified the root cause as Linux doesn't invoke _PS0 at boot >>>> for all ACPI devices: >>> >>> This part of the changelog is confusing, because the evaluation of >>> _PS0 is not a separate operation. _PS0 gets evaluated when devices >>> undergo transitions from low-power states to D0. >>> >>>> Scope (\_SB.PC00.GFX0) >>>> { >>>> Scope (DD1F) >>>> { >>>> Method (_PS0, 0, Serialized) // _PS0: Power State 0 >>>> { >>>> If (CondRefOf (\_SB.PC00.LPCB.EC0.SSBC)) >>>> { >>>> \_SB.PC00.LPCB.EC0.SSBC () >>>> } >>>> } >>>> ... >>>> } >>>> ... >>>> } >>>> >>>> _PS0 doesn't get invoked for all ACPI devices because of commit >>>> 7cd8407d53ef ("ACPI / PM: Do not execute _PS0 for devices without _PSC >>>> during initialization"). >> >> So this _PS0 which seems to be the one which needs to run here, >> is not the _PS0 for the GFX0 ACPI device, but rather for a child ACPI device-node which describes the connector (assumed based on the small part of quoted DSDT, the actual definition of the DD1F device-node is missing). > > I'll file a bugzilla and attach a full acpidump there. > >> >> Having a _PS0 method on a connector object is really weird IMHO. But if we need to invoke such a _PS0 method then IMHO that really should be done in the drm/kms driver. E.g. at least the i915 code already contains code to map the ACPI connector objects to the drm_connector objects, so it should be relatively easily to make that try and do a power-transition to D0 when enabling the connector. > > Or put all ACPI devices to D0 at boot? > According to the BIOS folks that's what Windows does. > This way we can drop acpi_device_fix_up_power* helpers altogether. Doing that will leave any devices for which we lack a driver at D0 for ever, so that IMHO is not a good idea. I guess calling acpi_device_fix_up_power_extended(device) from the ACPI-video code, so that the connector sub-objects are put in D0 is somewhat ok. Although I would prefer to see you first try to do the same thing from the i915 driver instead. If we do end up doing this from the acpi-video code please add a comment above the call why this is done; and as Rafael mentioned the commit msg needs to better explain things too. Regards, Hans > >> >> Also can you provide some more info on the hw on which this is being seen: >> >> 1. What GPU(s) is/are being used > > Intel GFX. > > AFAIK AMD based laptops also require this fixup too. > >> 2. If there is a mux for hybrid gfx in which mode is the mux set ? > > No. This happens to mux-less and dGPU-less laptops. > >> 3. Wjich method is used to control the brightness (which backlight-class-devices show up under /sys/class/backlight) ? > > intel_backlight. > >> >> And can you add this info to the commit msg for the next version of the patch ? > > Sure. > Can putting all devices to D0 be considered too? It's a better > solution for the long wrong. > > Kai-Heng > >> >> Regards, >> >> Hans >> >> >> >> >>> >>> And yes, Linux doesn't put all of the ACPI devices into D0 during >>> initialization, but the above commit has a little to do with that. >>> >>>> For now explicitly call _PS0 for ACPI video to workaround the issue. >>> >>> This is not what the patch is doing. >>> >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> --- >>>> drivers/acpi/acpi_video.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c >>>> index 62f4364e4460..793259bd18c8 100644 >>>> --- a/drivers/acpi/acpi_video.c >>>> +++ b/drivers/acpi/acpi_video.c >>>> @@ -2027,6 +2027,8 @@ static int acpi_video_bus_add(struct acpi_device *device) >>>> if (error) >>>> goto err_put_video; >>>> >>>> + acpi_device_fix_up_power_extended(device); >>>> + >>> >>> I would like to know what Hans thinks about this. >>> >>>> pr_info("%s [%s] (multi-head: %s rom: %s post: %s)\n", >>>> ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device), >>>> video->flags.multihead ? "yes" : "no", >>>> -- >>> >> >
Hi Hans, On Mon, Jul 10, 2023 at 8:54 PM Hans de Goede <hdegoede@redhat.com> wrote: [snipped] > > Or put all ACPI devices to D0 at boot? > > According to the BIOS folks that's what Windows does. > > This way we can drop acpi_device_fix_up_power* helpers altogether. > > Doing that will leave any devices for which we lack a driver at D0 for ever, > so that IMHO is not a good idea. > > I guess calling acpi_device_fix_up_power_extended(device) from the > ACPI-video code, so that the connector sub-objects are put in D0 is > somewhat ok. Although I would prefer to see you first try to do > the same thing from the i915 driver instead. I was told by vendor that the same design is used on AMD based laptops too. So I think putting this in ACPI-video is a better approach. > > If we do end up doing this from the acpi-video code please add > a comment above the call why this is done; and as Rafael mentioned > the commit msg needs to better explain things too. Sure. Kai-Heng > > Regards, > > Hans > > > > > > >> > >> Also can you provide some more info on the hw on which this is being seen: > >> > >> 1. What GPU(s) is/are being used > > > > Intel GFX. > > > > AFAIK AMD based laptops also require this fixup too. > > > >> 2. If there is a mux for hybrid gfx in which mode is the mux set ? > > > > No. This happens to mux-less and dGPU-less laptops. > > > >> 3. Wjich method is used to control the brightness (which backlight-class-devices show up under /sys/class/backlight) ? > > > > intel_backlight. > > > >> > >> And can you add this info to the commit msg for the next version of the patch ? > > > > Sure. > > Can putting all devices to D0 be considered too? It's a better > > solution for the long wrong. > > > > Kai-Heng > > > >> > >> Regards, > >> > >> Hans > >> > >> > >> > >> > >>> > >>> And yes, Linux doesn't put all of the ACPI devices into D0 during > >>> initialization, but the above commit has a little to do with that. > >>> > >>>> For now explicitly call _PS0 for ACPI video to workaround the issue. > >>> > >>> This is not what the patch is doing. > >>> > >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > >>>> --- > >>>> drivers/acpi/acpi_video.c | 2 ++ > >>>> 1 file changed, 2 insertions(+) > >>>> > >>>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > >>>> index 62f4364e4460..793259bd18c8 100644 > >>>> --- a/drivers/acpi/acpi_video.c > >>>> +++ b/drivers/acpi/acpi_video.c > >>>> @@ -2027,6 +2027,8 @@ static int acpi_video_bus_add(struct acpi_device *device) > >>>> if (error) > >>>> goto err_put_video; > >>>> > >>>> + acpi_device_fix_up_power_extended(device); > >>>> + > >>> > >>> I would like to know what Hans thinks about this. > >>> > >>>> pr_info("%s [%s] (multi-head: %s rom: %s post: %s)\n", > >>>> ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device), > >>>> video->flags.multihead ? "yes" : "no", > >>>> -- > >>> > >> > > >
diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c index 62f4364e4460..793259bd18c8 100644 --- a/drivers/acpi/acpi_video.c +++ b/drivers/acpi/acpi_video.c @@ -2027,6 +2027,8 @@ static int acpi_video_bus_add(struct acpi_device *device) if (error) goto err_put_video; + acpi_device_fix_up_power_extended(device); + pr_info("%s [%s] (multi-head: %s rom: %s post: %s)\n", ACPI_VIDEO_DEVICE_NAME, acpi_device_bid(device), video->flags.multihead ? "yes" : "no",