Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Thu, Jun 06, 2024 at 10:21:07AM +0300, Jani Nikula wrote: > On Wed, 05 Jun 2024, Chris Bainbridge wrote: > > On Tue, Jun 04, 2024 at 10:02:29AM +0800, kernel test robot wrote: > >> Hi Mario, > >> > >> kernel test robot noticed the following build errors: > >> > >> [auto build test ERROR on drm-misc/drm-misc-next] > >> [also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next > >> drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip > >> linus/master v6.10-rc2 next-20240603] > >> [If your patch is applied to the wrong git tree, kindly drop us a note. > >> And when submitting patch, we suggest to use '--base' as documented in > >> https://git-scm.com/docs/git-format-patch#_base_tree_information] > >> > >> url: > >> https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/drm-client-Detect-when-ACPI-lid-is-closed-during-initialization/20240529-050440 > >> base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next > >> patch link: > >> https://lore.kernel.org/r/20240528210319.1242-1-mario.limonciello%40amd.com > >> patch subject: [PATCH v2] drm/client: Detect when ACPI lid is closed > >> during initialization > >> config: i386-randconfig-053-20240604 > >> (https://download.01.org/0day-ci/archive/20240604/[email protected]/config) > >> compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0 > >> reproduce (this is a W=1 build): > >> (https://download.01.org/0day-ci/archive/20240604/[email protected]/reproduce) > >> > >> If you fix the issue in a separate patch/commit (i.e. not just a new > >> version of > >> the same patch/commit), kindly add following tags > >> | Reported-by: kernel test robot > >> | Closes: > >> https://lore.kernel.org/oe-kbuild-all/[email protected]/ > >> > >> All errors (new ones prefixed by >>): > >> > >>ld: drivers/gpu/drm/drm_client_modeset.o: in function > >> `drm_client_match_edp_lid': > >> >> drivers/gpu/drm/drm_client_modeset.c:281:(.text+0x221b): undefined > >> >> reference to `acpi_lid_open' > >> > >> > >> vim +281 drivers/gpu/drm/drm_client_modeset.c > >> > >>260 > >>261 static void drm_client_match_edp_lid(struct drm_device *dev, > >>262 struct drm_connector > >> **connectors, > >>263 unsigned int > >> connector_count, > >>264 bool *enabled) > >>265 { > >>266 int i; > >>267 > >>268 for (i = 0; i < connector_count; i++) { > >>269 struct drm_connector *connector = connectors[i]; > >>270 > >>271 switch (connector->connector_type) { > >>272 case DRM_MODE_CONNECTOR_LVDS: > >>273 case DRM_MODE_CONNECTOR_eDP: > >>274 if (!enabled[i]) > >>275 continue; > >>276 break; > >>277 default: > >>278 continue; > >>279 } > >>280 > >> > 281 if (!acpi_lid_open()) { > >>282 drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid > >> is closed, disabling\n", > >>283 connector->base.id, > >> connector->name); > >>284 enabled[i] = false; > >>285 } > >>286 } > >>287 } > >>288 > >> > >> -- > >> 0-DAY CI Kernel Test Service > >> https://github.com/intel/lkp-tests/wiki > > > > The failed config has CONFIG_ACPI_BUTTON=m. The build failure can be > > fixed with: > > > > diff --git a/drivers/gpu/drm/drm_client_modeset.c > > b/drivers/gpu/drm/drm_client_modeset.c > > index b76438c31761..0271e66f44f8 100644 > > --- a/drivers/gpu/drm/drm_client_modeset.c > > +++ b/drivers/gpu/drm/drm_client_modeset.c > > @@ -271,11 +271,13 @@ static void drm_client_match_edp_lid(struct > > drm_device *dev, > > if (connector->connector_type != DRM_MODE_CONNECTOR_eDP || > > !enabled[i]) > > continue; > > > > +#if defined(CONFIG_ACPI_BUTTON) > > if (!acpi_lid_open()) { > > drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, > > disabling\n", > > connector->base.id, connector->name); > > enabled[i] = false; > > } > > +#endif > > } > > } > > No. This is because > > CONFIG_DRM=y > CONFIG_ACPI_BUTTON=m > > The pedantically correct fix is probably having DRM > > depends on ACPI_BUTTON || ACPI_BUTTON=n > > but seeing how i915 and xe just > > select ACPI_BUTTON if ACPI Huh. We should nuke that as we haven't used this lid stuff in ages. -- Ville Syrjälä Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Wed, 05 Jun 2024, Chris Bainbridge wrote: > On Tue, Jun 04, 2024 at 10:02:29AM +0800, kernel test robot wrote: >> Hi Mario, >> >> kernel test robot noticed the following build errors: >> >> [auto build test ERROR on drm-misc/drm-misc-next] >> [also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next >> drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip >> linus/master v6.10-rc2 next-20240603] >> [If your patch is applied to the wrong git tree, kindly drop us a note. >> And when submitting patch, we suggest to use '--base' as documented in >> https://git-scm.com/docs/git-format-patch#_base_tree_information] >> >> url: >> https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/drm-client-Detect-when-ACPI-lid-is-closed-during-initialization/20240529-050440 >> base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next >> patch link: >> https://lore.kernel.org/r/20240528210319.1242-1-mario.limonciello%40amd.com >> patch subject: [PATCH v2] drm/client: Detect when ACPI lid is closed during >> initialization >> config: i386-randconfig-053-20240604 >> (https://download.01.org/0day-ci/archive/20240604/[email protected]/config) >> compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0 >> reproduce (this is a W=1 build): >> (https://download.01.org/0day-ci/archive/20240604/[email protected]/reproduce) >> >> If you fix the issue in a separate patch/commit (i.e. not just a new version >> of >> the same patch/commit), kindly add following tags >> | Reported-by: kernel test robot >> | Closes: >> https://lore.kernel.org/oe-kbuild-all/[email protected]/ >> >> All errors (new ones prefixed by >>): >> >>ld: drivers/gpu/drm/drm_client_modeset.o: in function >> `drm_client_match_edp_lid': >> >> drivers/gpu/drm/drm_client_modeset.c:281:(.text+0x221b): undefined >> >> reference to `acpi_lid_open' >> >> >> vim +281 drivers/gpu/drm/drm_client_modeset.c >> >>260 >>261 static void drm_client_match_edp_lid(struct drm_device *dev, >>262struct drm_connector >> **connectors, >>263unsigned int >> connector_count, >>264bool *enabled) >>265 { >>266 int i; >>267 >>268 for (i = 0; i < connector_count; i++) { >>269 struct drm_connector *connector = connectors[i]; >>270 >>271 switch (connector->connector_type) { >>272 case DRM_MODE_CONNECTOR_LVDS: >>273 case DRM_MODE_CONNECTOR_eDP: >>274 if (!enabled[i]) >>275 continue; >>276 break; >>277 default: >>278 continue; >>279 } >>280 >> > 281 if (!acpi_lid_open()) { >>282 drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid >> is closed, disabling\n", >>283 connector->base.id, >> connector->name); >>284 enabled[i] = false; >>285 } >>286 } >>287 } >>288 >> >> -- >> 0-DAY CI Kernel Test Service >> https://github.com/intel/lkp-tests/wiki > > The failed config has CONFIG_ACPI_BUTTON=m. The build failure can be > fixed with: > > diff --git a/drivers/gpu/drm/drm_client_modeset.c > b/drivers/gpu/drm/drm_client_modeset.c > index b76438c31761..0271e66f44f8 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -271,11 +271,13 @@ static void drm_client_match_edp_lid(struct drm_device > *dev, > if (connector->connector_type != DRM_MODE_CONNECTOR_eDP || > !enabled[i]) > continue; > > +#if defined(CONFIG_ACPI_BUTTON) > if (!acpi_lid_open()) { > drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, > disabling\n", > connector->base.id, connector->name); > enabled[i] = false; > } > +#endif > } > } No. This is because CONFIG_DRM=y CONFIG_ACPI_BUTTON=m The pedantically correct fix is probably having DRM depends on ACPI_BUTTON || ACPI_BUTTON=n but seeing how i915 and xe just select ACPI_BUTTON if ACPI and nouveau basically uses acpi_lid_open() it if it's reachable with no regard to kconfig, it's anyone's guess what will work. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Tue, Jun 04, 2024 at 10:02:29AM +0800, kernel test robot wrote: > Hi Mario, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on drm-misc/drm-misc-next] > [also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next > drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip > linus/master v6.10-rc2 next-20240603] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: > https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/drm-client-Detect-when-ACPI-lid-is-closed-during-initialization/20240529-050440 > base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next > patch link: > https://lore.kernel.org/r/20240528210319.1242-1-mario.limonciello%40amd.com > patch subject: [PATCH v2] drm/client: Detect when ACPI lid is closed during > initialization > config: i386-randconfig-053-20240604 > (https://download.01.org/0day-ci/archive/20240604/[email protected]/config) > compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0 > reproduce (this is a W=1 build): > (https://download.01.org/0day-ci/archive/20240604/[email protected]/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version > of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot > | Closes: > https://lore.kernel.org/oe-kbuild-all/[email protected]/ > > All errors (new ones prefixed by >>): > >ld: drivers/gpu/drm/drm_client_modeset.o: in function > `drm_client_match_edp_lid': > >> drivers/gpu/drm/drm_client_modeset.c:281:(.text+0x221b): undefined > >> reference to `acpi_lid_open' > > > vim +281 drivers/gpu/drm/drm_client_modeset.c > >260 >261static void drm_client_match_edp_lid(struct drm_device *dev, >262 struct drm_connector > **connectors, >263 unsigned int > connector_count, >264 bool *enabled) >265{ >266int i; >267 >268for (i = 0; i < connector_count; i++) { >269struct drm_connector *connector = connectors[i]; >270 >271switch (connector->connector_type) { >272case DRM_MODE_CONNECTOR_LVDS: >273case DRM_MODE_CONNECTOR_eDP: >274if (!enabled[i]) >275continue; >276break; >277default: >278continue; >279} >280 > > 281if (!acpi_lid_open()) { >282drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid > is closed, disabling\n", >283connector->base.id, > connector->name); >284enabled[i] = false; >285} >286} >287} >288 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki The failed config has CONFIG_ACPI_BUTTON=m. The build failure can be fixed with: diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index b76438c31761..0271e66f44f8 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -271,11 +271,13 @@ static void drm_client_match_edp_lid(struct drm_device *dev, if (connector->connector_type != DRM_MODE_CONNECTOR_eDP || !enabled[i]) continue; +#if defined(CONFIG_ACPI_BUTTON) if (!acpi_lid_open()) { drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, disabling\n", connector->base.id, connector->name); enabled[i] = false; } +#endif } }
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
Hi Mario, kernel test robot noticed the following build errors: [auto build test ERROR on drm-misc/drm-misc-next] [also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.10-rc2 next-20240603] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Mario-Limonciello/drm-client-Detect-when-ACPI-lid-is-closed-during-initialization/20240529-050440 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20240528210319.1242-1-mario.limonciello%40amd.com patch subject: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization config: i386-randconfig-053-20240604 (https://download.01.org/0day-ci/archive/20240604/[email protected]/config) compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240604/[email protected]/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/ All errors (new ones prefixed by >>): ld: drivers/gpu/drm/drm_client_modeset.o: in function `drm_client_match_edp_lid': >> drivers/gpu/drm/drm_client_modeset.c:281:(.text+0x221b): undefined reference >> to `acpi_lid_open' vim +281 drivers/gpu/drm/drm_client_modeset.c 260 261 static void drm_client_match_edp_lid(struct drm_device *dev, 262 struct drm_connector **connectors, 263 unsigned int connector_count, 264 bool *enabled) 265 { 266 int i; 267 268 for (i = 0; i < connector_count; i++) { 269 struct drm_connector *connector = connectors[i]; 270 271 switch (connector->connector_type) { 272 case DRM_MODE_CONNECTOR_LVDS: 273 case DRM_MODE_CONNECTOR_eDP: 274 if (!enabled[i]) 275 continue; 276 break; 277 default: 278 continue; 279 } 280 > 281 if (!acpi_lid_open()) { 282 drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, disabling\n", 283 connector->base.id, connector->name); 284 enabled[i] = false; 285 } 286 } 287 } 288 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Thu, May 30, 2024 at 11:07:53AM +0300, Dmitry Baryshkov wrote: > On Thu, 30 May 2024 at 07:41, Limonciello, Mario > wrote: > > > > > > >> Also a direct acpi_lid_open() call seems a bit iffy. But I guess if > > >> someone needs this to work on non-ACPI system they get to figure out > > >> how to abstract it better. acpi_lid_open() does seem to return != 0 > > >> when ACPI is not supported, so at least it would err on the side > > >> of enabling everything. > > > > > > Thanks. I was going to comment, but you got it first. I think a proper > > > implementation should check for SW_LID input device instead of simply > > > using acpi_lid_open(). This will handle the issue for other, > > > non-ACPI-based laptops. > > > > > > > Can you suggest how this would actually work? AFAICT the only way to > > discover if input devices support SW_LID would be to iterate all the > > input devices in the kernel and look for whether ->swbit has SW_LID set. > > > > This then turns into a dependency problem of whether any myriad of > > drivers have started to report SW_LID. It's also a state machine > > problem because other drivers can be unloaded at will. > > > > And then what do you if more than one sets SW_LID? > > It might be easier to handle this in the input subsystem. For example > by using a refcount-like variable which handles all the LIDs and > counts if all of them are closed. Or if any of the LIDs is closed. Yes, install an input handler matching on EV_SW/SW_LID so you will get notified when input devices capable of reporting SW_LID appear and disappear and also when SW_LID event is being generated, and handle as you wish. Something like https://chromium.googlesource.com/chromiumos/third_party/kernel/+/40e9f6a991856ee7d504ac1ccd587e435775cfc4%5E%21/#F0 In practice I think it is pretty safe to assume only 1 lid for a laptop/device. Thanks. -- Dmitry
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Thu, 30 May 2024 at 07:41, Limonciello, Mario wrote: > > > >> Also a direct acpi_lid_open() call seems a bit iffy. But I guess if > >> someone needs this to work on non-ACPI system they get to figure out > >> how to abstract it better. acpi_lid_open() does seem to return != 0 > >> when ACPI is not supported, so at least it would err on the side > >> of enabling everything. > > > > Thanks. I was going to comment, but you got it first. I think a proper > > implementation should check for SW_LID input device instead of simply > > using acpi_lid_open(). This will handle the issue for other, > > non-ACPI-based laptops. > > > > Can you suggest how this would actually work? AFAICT the only way to > discover if input devices support SW_LID would be to iterate all the > input devices in the kernel and look for whether ->swbit has SW_LID set. > > This then turns into a dependency problem of whether any myriad of > drivers have started to report SW_LID. It's also a state machine > problem because other drivers can be unloaded at will. > > And then what do you if more than one sets SW_LID? It might be easier to handle this in the input subsystem. For example by using a refcount-like variable which handles all the LIDs and counts if all of them are closed. Or if any of the LIDs is closed. > > IOW - a lot of complexity for a non-ACPI system. Does such a problem > exist in non-ACPI systems? There are non-ACPI laptops. For example Chromebooks. Or Lenovo X13s, Lenovo Yoga C630, Lenovo Flex5G, etc. We are expecting more to come in the next few months. And I don't see why they won't have the same problem. -- With best wishes Dmitry
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
Also a direct acpi_lid_open() call seems a bit iffy. But I guess if someone needs this to work on non-ACPI system they get to figure out how to abstract it better. acpi_lid_open() does seem to return != 0 when ACPI is not supported, so at least it would err on the side of enabling everything. Thanks. I was going to comment, but you got it first. I think a proper implementation should check for SW_LID input device instead of simply using acpi_lid_open(). This will handle the issue for other, non-ACPI-based laptops. Can you suggest how this would actually work? AFAICT the only way to discover if input devices support SW_LID would be to iterate all the input devices in the kernel and look for whether ->swbit has SW_LID set. This then turns into a dependency problem of whether any myriad of drivers have started to report SW_LID. It's also a state machine problem because other drivers can be unloaded at will. And then what do you if more than one sets SW_LID? IOW - a lot of complexity for a non-ACPI system. Does such a problem exist in non-ACPI systems?
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Wed, May 29, 2024 at 06:39:21PM +0300, Ville Syrjälä wrote: > On Wed, May 29, 2024 at 09:45:55AM -0500, Mario Limonciello wrote: > > On 5/29/2024 09:14, Ville Syrjälä wrote: > > > On Tue, May 28, 2024 at 04:03:19PM -0500, Mario Limonciello wrote: > > >> If the lid on a laptop is closed when eDP connectors are populated > > >> then it remains enabled when the initial framebuffer configuration > > >> is built. > > >> > > >> When creating the initial framebuffer configuration detect the ACPI > > >> lid status and if it's closed disable any eDP connectors. > > >> > > >> Reported-by: Chris Bainbridge > > >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 > > >> Signed-off-by: Mario Limonciello > > >> --- > > >> Cc: [email protected] > > >> v1->v2: > > >> * Match LVDS as well > > >> --- > > >> drivers/gpu/drm/drm_client_modeset.c | 30 > > >> 1 file changed, 30 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/drm_client_modeset.c > > >> b/drivers/gpu/drm/drm_client_modeset.c > > >> index 31af5cf37a09..0b0411086e76 100644 > > >> --- a/drivers/gpu/drm/drm_client_modeset.c > > >> +++ b/drivers/gpu/drm/drm_client_modeset.c > > >> @@ -8,6 +8,7 @@ > > >>*/ > > >> > > >> #include "drm/drm_modeset_lock.h" > > >> +#include > > >> #include > > >> #include > > >> #include > > >> @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct > > >> drm_connector **connectors, > > >> enabled[i] = drm_connector_enabled(connectors[i], > > >> false); > > >> } > > >> > > >> +static void drm_client_match_edp_lid(struct drm_device *dev, > > >> + struct drm_connector **connectors, > > >> + unsigned int connector_count, > > >> + bool *enabled) > > >> +{ > > >> +int i; > > >> + > > >> +for (i = 0; i < connector_count; i++) { > > >> +struct drm_connector *connector = connectors[i]; > > >> + > > >> +switch (connector->connector_type) { > > >> +case DRM_MODE_CONNECTOR_LVDS: > > >> +case DRM_MODE_CONNECTOR_eDP: What about DPI or DSI panels? I think they should also be handled in a similar way. Not sure regarding the SPI. > > >> +if (!enabled[i]) > > >> +continue; > > >> +break; > > >> +default: > > >> +continue; > > >> +} > > >> + > > >> +if (!acpi_lid_open()) { > > >> +drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is > > >> closed, disabling\n", > > >> +connector->base.id, > > >> connector->name); > > >> +enabled[i] = false; > > >> +} > > >> +} > > >> +} > > > [trimmed] > > The other potential issue here is whether acpi_lid_open() is actually > trustworthy. Some kms drivers have/had some lid handling in their own > code, and I'm pretty sure those have often needed quirks/modparams > to actually do sensible things on certain machines. > > FWIW I ripped out all the lid crap from i915 long ago since it was > half backed, mostly broken, and ugly, and I'm not looking to add it > back there. But I do think handling that in drm_client does seem > somewhat sane, as that should more or less match what userspace > clients would do. Just a question of how bad the quirk situation > will get... Looking at drivers/acpi/button.c, the button driver handles some of the quirks when posting the data to the input subsystem. > > > Also a direct acpi_lid_open() call seems a bit iffy. But I guess if > someone needs this to work on non-ACPI system they get to figure out > how to abstract it better. acpi_lid_open() does seem to return != 0 > when ACPI is not supported, so at least it would err on the side > of enabling everything. Thanks. I was going to comment, but you got it first. I think a proper implementation should check for SW_LID input device instead of simply using acpi_lid_open(). This will handle the issue for other, non-ACPI-based laptops. -- With best wishes Dmitry
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
If you don't hook into some lid notify event how is one supposed to get the display back to life after opening the lid? I guess in my mind it's a tangential to the "initial modeset". The DRM master can issue a modeset to enable the combination as desired. This code is run whenever there's a hotplug/etc. Not sure why you're only thinking about the initial modeset. Got it; so in that case adding a notification chain for lid events to run it again should do the trick. When I tested I did confirm that with mutter such an event is received and it does the modeset to enable the eDP when lid is opened. This code isn't relevant when you have a userspace drm master calling the shots. Right. Let me ask this - what happens if no DRM master running and you hotplug a DP cable? Does a "new" clone configuration get done? Yes, this code reprobes the displays and comes up with a new config to suit the new situation. Got it; in this case you're right we should have some notification chain. Do you think it should be in the initial patch or a follow up? The other potential issue here is whether acpi_lid_open() is actually trustworthy. Some kms drivers have/had some lid handling in their own code, and I'm pretty sure those have often needed quirks/modparams to actually do sensible things on certain machines. FWIW I ripped out all the lid crap from i915 long ago since it was half backed, mostly broken, and ugly, and I'm not looking to add it back there. But I do think handling that in drm_client does seem somewhat sane, as that should more or less match what userspace clients would do. Just a question of how bad the quirk situation will get... If the lid reporting is wrong it's not just drm_client that would falter. There are other parts of the kernel that rely upon acpi_lid_open() being accurate and IMO it would be best to put any quirks to the effect in drivers/acpi/button.c. If it can't be relied upon then it's best to just report -EINVAL or -ENODEV. Also a direct acpi_lid_open() call seems a bit iffy. But I guess if someone needs this to work on non-ACPI system they get to figure out how to abstract it better. acpi_lid_open() does seem to return != 0 when ACPI is not supported, so at least it would err on the side of enabling everything. Yeah acpi_lid_open() seemed fine to me specifically because non ACPI hardcodes to open.
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Wed, May 29, 2024 at 09:45:55AM -0500, Mario Limonciello wrote: > On 5/29/2024 09:14, Ville Syrjälä wrote: > > On Tue, May 28, 2024 at 04:03:19PM -0500, Mario Limonciello wrote: > >> If the lid on a laptop is closed when eDP connectors are populated > >> then it remains enabled when the initial framebuffer configuration > >> is built. > >> > >> When creating the initial framebuffer configuration detect the ACPI > >> lid status and if it's closed disable any eDP connectors. > >> > >> Reported-by: Chris Bainbridge > >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 > >> Signed-off-by: Mario Limonciello > >> --- > >> Cc: [email protected] > >> v1->v2: > >> * Match LVDS as well > >> --- > >> drivers/gpu/drm/drm_client_modeset.c | 30 > >> 1 file changed, 30 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_client_modeset.c > >> b/drivers/gpu/drm/drm_client_modeset.c > >> index 31af5cf37a09..0b0411086e76 100644 > >> --- a/drivers/gpu/drm/drm_client_modeset.c > >> +++ b/drivers/gpu/drm/drm_client_modeset.c > >> @@ -8,6 +8,7 @@ > >>*/ > >> > >> #include "drm/drm_modeset_lock.h" > >> +#include > >> #include > >> #include > >> #include > >> @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct > >> drm_connector **connectors, > >>enabled[i] = drm_connector_enabled(connectors[i], false); > >> } > >> > >> +static void drm_client_match_edp_lid(struct drm_device *dev, > >> + struct drm_connector **connectors, > >> + unsigned int connector_count, > >> + bool *enabled) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < connector_count; i++) { > >> + struct drm_connector *connector = connectors[i]; > >> + > >> + switch (connector->connector_type) { > >> + case DRM_MODE_CONNECTOR_LVDS: > >> + case DRM_MODE_CONNECTOR_eDP: > >> + if (!enabled[i]) > >> + continue; > >> + break; > >> + default: > >> + continue; > >> + } > >> + > >> + if (!acpi_lid_open()) { > >> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, > >> disabling\n", > >> + connector->base.id, connector->name); > >> + enabled[i] = false; > >> + } > >> + } > >> +} > > > > If you don't hook into some lid notify event how is one supposed to get > > the display back to life after opening the lid? > > I guess in my mind it's a tangential to the "initial modeset". The DRM > master can issue a modeset to enable the combination as desired. This code is run whenever there's a hotplug/etc. Not sure why you're only thinking about the initial modeset. > > When I tested I did confirm that with mutter such an event is received > and it does the modeset to enable the eDP when lid is opened. This code isn't relevant when you have a userspace drm master calling the shots. > > Let me ask this - what happens if no DRM master running and you hotplug > a DP cable? Does a "new" clone configuration get done? Yes, this code reprobes the displays and comes up with a new config to suit the new situation. The other potential issue here is whether acpi_lid_open() is actually trustworthy. Some kms drivers have/had some lid handling in their own code, and I'm pretty sure those have often needed quirks/modparams to actually do sensible things on certain machines. FWIW I ripped out all the lid crap from i915 long ago since it was half backed, mostly broken, and ugly, and I'm not looking to add it back there. But I do think handling that in drm_client does seem somewhat sane, as that should more or less match what userspace clients would do. Just a question of how bad the quirk situation will get... Also a direct acpi_lid_open() call seems a bit iffy. But I guess if someone needs this to work on non-ACPI system they get to figure out how to abstract it better. acpi_lid_open() does seem to return != 0 when ACPI is not supported, so at least it would err on the side of enabling everything. -- Ville Syrjälä Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On 5/29/2024 09:14, Ville Syrjälä wrote: On Tue, May 28, 2024 at 04:03:19PM -0500, Mario Limonciello wrote: If the lid on a laptop is closed when eDP connectors are populated then it remains enabled when the initial framebuffer configuration is built. When creating the initial framebuffer configuration detect the ACPI lid status and if it's closed disable any eDP connectors. Reported-by: Chris Bainbridge Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 Signed-off-by: Mario Limonciello --- Cc: [email protected] v1->v2: * Match LVDS as well --- drivers/gpu/drm/drm_client_modeset.c | 30 1 file changed, 30 insertions(+) diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 31af5cf37a09..0b0411086e76 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -8,6 +8,7 @@ */ #include "drm/drm_modeset_lock.h" +#include #include #include #include @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors, enabled[i] = drm_connector_enabled(connectors[i], false); } +static void drm_client_match_edp_lid(struct drm_device *dev, +struct drm_connector **connectors, +unsigned int connector_count, +bool *enabled) +{ + int i; + + for (i = 0; i < connector_count; i++) { + struct drm_connector *connector = connectors[i]; + + switch (connector->connector_type) { + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + if (!enabled[i]) + continue; + break; + default: + continue; + } + + if (!acpi_lid_open()) { + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, disabling\n", + connector->base.id, connector->name); + enabled[i] = false; + } + } +} If you don't hook into some lid notify event how is one supposed to get the display back to life after opening the lid? I guess in my mind it's a tangential to the "initial modeset". The DRM master can issue a modeset to enable the combination as desired. When I tested I did confirm that with mutter such an event is received and it does the modeset to enable the eDP when lid is opened. Let me ask this - what happens if no DRM master running and you hotplug a DP cable? Does a "new" clone configuration get done? + static bool drm_client_target_cloned(struct drm_device *dev, struct drm_connector **connectors, unsigned int connector_count, @@ -844,6 +873,7 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width, memset(crtcs, 0, connector_count * sizeof(*crtcs)); memset(offsets, 0, connector_count * sizeof(*offsets)); + drm_client_match_edp_lid(dev, connectors, connector_count, enabled); if (!drm_client_target_cloned(dev, connectors, connector_count, modes, offsets, enabled, width, height) && !drm_client_target_preferred(dev, connectors, connector_count, modes, -- 2.43.0
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On 5/29/2024 08:55, Alex Deucher wrote: On Wed, May 29, 2024 at 9:51 AM Jani Nikula wrote: On Wed, 29 May 2024, Alex Deucher wrote: On Tue, May 28, 2024 at 5:03 PM Mario Limonciello wrote: If the lid on a laptop is closed when eDP connectors are populated then it remains enabled when the initial framebuffer configuration is built. When creating the initial framebuffer configuration detect the ACPI lid status and if it's closed disable any eDP connectors. Reported-by: Chris Bainbridge Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 Signed-off-by: Mario Limonciello Reviewed-by: Alex Deucher Do you have drm-misc access or do you need someone to apply this for you? I've bounced this to intel-gfx and intel-xe lists to get CI testing. I'd appreciate holding off on merging until we have results. Sure. Thanks for the review and pushing it to CI testing infra. I don't have any drm-misc access so if everything looks good then one of you guys please merge it for me. Thanks! Alex Thanks, Jani. Alex --- Cc: [email protected] v1->v2: * Match LVDS as well --- drivers/gpu/drm/drm_client_modeset.c | 30 1 file changed, 30 insertions(+) diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c index 31af5cf37a09..0b0411086e76 100644 --- a/drivers/gpu/drm/drm_client_modeset.c +++ b/drivers/gpu/drm/drm_client_modeset.c @@ -8,6 +8,7 @@ */ #include "drm/drm_modeset_lock.h" +#include #include #include #include @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors, enabled[i] = drm_connector_enabled(connectors[i], false); } +static void drm_client_match_edp_lid(struct drm_device *dev, +struct drm_connector **connectors, +unsigned int connector_count, +bool *enabled) +{ + int i; + + for (i = 0; i < connector_count; i++) { + struct drm_connector *connector = connectors[i]; + + switch (connector->connector_type) { + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + if (!enabled[i]) + continue; + break; + default: + continue; + } + + if (!acpi_lid_open()) { + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, disabling\n", + connector->base.id, connector->name); + enabled[i] = false; + } + } +} + static bool drm_client_target_cloned(struct drm_device *dev, struct drm_connector **connectors, unsigned int connector_count, @@ -844,6 +873,7 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width, memset(crtcs, 0, connector_count * sizeof(*crtcs)); memset(offsets, 0, connector_count * sizeof(*offsets)); + drm_client_match_edp_lid(dev, connectors, connector_count, enabled); if (!drm_client_target_cloned(dev, connectors, connector_count, modes, offsets, enabled, width, height) && !drm_client_target_preferred(dev, connectors, connector_count, modes, -- 2.43.0 -- Jani Nikula, Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Tue, May 28, 2024 at 04:03:19PM -0500, Mario Limonciello wrote: > If the lid on a laptop is closed when eDP connectors are populated > then it remains enabled when the initial framebuffer configuration > is built. > > When creating the initial framebuffer configuration detect the ACPI > lid status and if it's closed disable any eDP connectors. > > Reported-by: Chris Bainbridge > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 > Signed-off-by: Mario Limonciello > --- > Cc: [email protected] > v1->v2: > * Match LVDS as well > --- > drivers/gpu/drm/drm_client_modeset.c | 30 > 1 file changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c > b/drivers/gpu/drm/drm_client_modeset.c > index 31af5cf37a09..0b0411086e76 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -8,6 +8,7 @@ > */ > > #include "drm/drm_modeset_lock.h" > +#include > #include > #include > #include > @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct > drm_connector **connectors, > enabled[i] = drm_connector_enabled(connectors[i], false); > } > > +static void drm_client_match_edp_lid(struct drm_device *dev, > + struct drm_connector **connectors, > + unsigned int connector_count, > + bool *enabled) > +{ > + int i; > + > + for (i = 0; i < connector_count; i++) { > + struct drm_connector *connector = connectors[i]; > + > + switch (connector->connector_type) { > + case DRM_MODE_CONNECTOR_LVDS: > + case DRM_MODE_CONNECTOR_eDP: > + if (!enabled[i]) > + continue; > + break; > + default: > + continue; > + } > + > + if (!acpi_lid_open()) { > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, > disabling\n", > + connector->base.id, connector->name); > + enabled[i] = false; > + } > + } > +} If you don't hook into some lid notify event how is one supposed to get the display back to life after opening the lid? > + > static bool drm_client_target_cloned(struct drm_device *dev, >struct drm_connector **connectors, >unsigned int connector_count, > @@ -844,6 +873,7 @@ int drm_client_modeset_probe(struct drm_client_dev > *client, unsigned int width, > memset(crtcs, 0, connector_count * sizeof(*crtcs)); > memset(offsets, 0, connector_count * sizeof(*offsets)); > > + drm_client_match_edp_lid(dev, connectors, connector_count, > enabled); > if (!drm_client_target_cloned(dev, connectors, connector_count, > modes, > offsets, enabled, width, height) > && > !drm_client_target_preferred(dev, connectors, > connector_count, modes, > -- > 2.43.0 -- Ville Syrjälä Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Wed, May 29, 2024 at 9:51 AM Jani Nikula wrote: > > On Wed, 29 May 2024, Alex Deucher wrote: > > On Tue, May 28, 2024 at 5:03 PM Mario Limonciello > > wrote: > >> > >> If the lid on a laptop is closed when eDP connectors are populated > >> then it remains enabled when the initial framebuffer configuration > >> is built. > >> > >> When creating the initial framebuffer configuration detect the ACPI > >> lid status and if it's closed disable any eDP connectors. > >> > >> Reported-by: Chris Bainbridge > >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 > >> Signed-off-by: Mario Limonciello > > > > Reviewed-by: Alex Deucher > > > > Do you have drm-misc access or do you need someone to apply this for you? > > I've bounced this to intel-gfx and intel-xe lists to get CI testing. I'd > appreciate holding off on merging until we have results. Sure. Alex > > Thanks, > Jani. > > > > > Alex > > > >> --- > >> Cc: [email protected] > >> v1->v2: > >> * Match LVDS as well > >> --- > >> drivers/gpu/drm/drm_client_modeset.c | 30 > >> 1 file changed, 30 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_client_modeset.c > >> b/drivers/gpu/drm/drm_client_modeset.c > >> index 31af5cf37a09..0b0411086e76 100644 > >> --- a/drivers/gpu/drm/drm_client_modeset.c > >> +++ b/drivers/gpu/drm/drm_client_modeset.c > >> @@ -8,6 +8,7 @@ > >> */ > >> > >> #include "drm/drm_modeset_lock.h" > >> +#include > >> #include > >> #include > >> #include > >> @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct > >> drm_connector **connectors, > >> enabled[i] = drm_connector_enabled(connectors[i], false); > >> } > >> > >> +static void drm_client_match_edp_lid(struct drm_device *dev, > >> +struct drm_connector **connectors, > >> +unsigned int connector_count, > >> +bool *enabled) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < connector_count; i++) { > >> + struct drm_connector *connector = connectors[i]; > >> + > >> + switch (connector->connector_type) { > >> + case DRM_MODE_CONNECTOR_LVDS: > >> + case DRM_MODE_CONNECTOR_eDP: > >> + if (!enabled[i]) > >> + continue; > >> + break; > >> + default: > >> + continue; > >> + } > >> + > >> + if (!acpi_lid_open()) { > >> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, > >> disabling\n", > >> + connector->base.id, connector->name); > >> + enabled[i] = false; > >> + } > >> + } > >> +} > >> + > >> static bool drm_client_target_cloned(struct drm_device *dev, > >> struct drm_connector **connectors, > >> unsigned int connector_count, > >> @@ -844,6 +873,7 @@ int drm_client_modeset_probe(struct drm_client_dev > >> *client, unsigned int width, > >> memset(crtcs, 0, connector_count * sizeof(*crtcs)); > >> memset(offsets, 0, connector_count * sizeof(*offsets)); > >> > >> + drm_client_match_edp_lid(dev, connectors, connector_count, > >> enabled); > >> if (!drm_client_target_cloned(dev, connectors, > >> connector_count, modes, > >> offsets, enabled, width, > >> height) && > >> !drm_client_target_preferred(dev, connectors, > >> connector_count, modes, > >> -- > >> 2.43.0 > >> > > -- > Jani Nikula, Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Wed, 29 May 2024, Alex Deucher wrote: > On Tue, May 28, 2024 at 5:03 PM Mario Limonciello > wrote: >> >> If the lid on a laptop is closed when eDP connectors are populated >> then it remains enabled when the initial framebuffer configuration >> is built. >> >> When creating the initial framebuffer configuration detect the ACPI >> lid status and if it's closed disable any eDP connectors. >> >> Reported-by: Chris Bainbridge >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 >> Signed-off-by: Mario Limonciello > > Reviewed-by: Alex Deucher > > Do you have drm-misc access or do you need someone to apply this for you? I've bounced this to intel-gfx and intel-xe lists to get CI testing. I'd appreciate holding off on merging until we have results. Thanks, Jani. > > Alex > >> --- >> Cc: [email protected] >> v1->v2: >> * Match LVDS as well >> --- >> drivers/gpu/drm/drm_client_modeset.c | 30 >> 1 file changed, 30 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c >> b/drivers/gpu/drm/drm_client_modeset.c >> index 31af5cf37a09..0b0411086e76 100644 >> --- a/drivers/gpu/drm/drm_client_modeset.c >> +++ b/drivers/gpu/drm/drm_client_modeset.c >> @@ -8,6 +8,7 @@ >> */ >> >> #include "drm/drm_modeset_lock.h" >> +#include >> #include >> #include >> #include >> @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct >> drm_connector **connectors, >> enabled[i] = drm_connector_enabled(connectors[i], false); >> } >> >> +static void drm_client_match_edp_lid(struct drm_device *dev, >> +struct drm_connector **connectors, >> +unsigned int connector_count, >> +bool *enabled) >> +{ >> + int i; >> + >> + for (i = 0; i < connector_count; i++) { >> + struct drm_connector *connector = connectors[i]; >> + >> + switch (connector->connector_type) { >> + case DRM_MODE_CONNECTOR_LVDS: >> + case DRM_MODE_CONNECTOR_eDP: >> + if (!enabled[i]) >> + continue; >> + break; >> + default: >> + continue; >> + } >> + >> + if (!acpi_lid_open()) { >> + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, >> disabling\n", >> + connector->base.id, connector->name); >> + enabled[i] = false; >> + } >> + } >> +} >> + >> static bool drm_client_target_cloned(struct drm_device *dev, >> struct drm_connector **connectors, >> unsigned int connector_count, >> @@ -844,6 +873,7 @@ int drm_client_modeset_probe(struct drm_client_dev >> *client, unsigned int width, >> memset(crtcs, 0, connector_count * sizeof(*crtcs)); >> memset(offsets, 0, connector_count * sizeof(*offsets)); >> >> + drm_client_match_edp_lid(dev, connectors, connector_count, >> enabled); >> if (!drm_client_target_cloned(dev, connectors, >> connector_count, modes, >> offsets, enabled, width, >> height) && >> !drm_client_target_preferred(dev, connectors, >> connector_count, modes, >> -- >> 2.43.0 >> -- Jani Nikula, Intel
Re: [PATCH v2] drm/client: Detect when ACPI lid is closed during initialization
On Tue, May 28, 2024 at 5:03 PM Mario Limonciello wrote: > > If the lid on a laptop is closed when eDP connectors are populated > then it remains enabled when the initial framebuffer configuration > is built. > > When creating the initial framebuffer configuration detect the ACPI > lid status and if it's closed disable any eDP connectors. > > Reported-by: Chris Bainbridge > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349 > Signed-off-by: Mario Limonciello Reviewed-by: Alex Deucher Do you have drm-misc access or do you need someone to apply this for you? Alex > --- > Cc: [email protected] > v1->v2: > * Match LVDS as well > --- > drivers/gpu/drm/drm_client_modeset.c | 30 > 1 file changed, 30 insertions(+) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c > b/drivers/gpu/drm/drm_client_modeset.c > index 31af5cf37a09..0b0411086e76 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -8,6 +8,7 @@ > */ > > #include "drm/drm_modeset_lock.h" > +#include > #include > #include > #include > @@ -257,6 +258,34 @@ static void drm_client_connectors_enabled(struct > drm_connector **connectors, > enabled[i] = drm_connector_enabled(connectors[i], false); > } > > +static void drm_client_match_edp_lid(struct drm_device *dev, > +struct drm_connector **connectors, > +unsigned int connector_count, > +bool *enabled) > +{ > + int i; > + > + for (i = 0; i < connector_count; i++) { > + struct drm_connector *connector = connectors[i]; > + > + switch (connector->connector_type) { > + case DRM_MODE_CONNECTOR_LVDS: > + case DRM_MODE_CONNECTOR_eDP: > + if (!enabled[i]) > + continue; > + break; > + default: > + continue; > + } > + > + if (!acpi_lid_open()) { > + drm_dbg_kms(dev, "[CONNECTOR:%d:%s] lid is closed, > disabling\n", > + connector->base.id, connector->name); > + enabled[i] = false; > + } > + } > +} > + > static bool drm_client_target_cloned(struct drm_device *dev, > struct drm_connector **connectors, > unsigned int connector_count, > @@ -844,6 +873,7 @@ int drm_client_modeset_probe(struct drm_client_dev > *client, unsigned int width, > memset(crtcs, 0, connector_count * sizeof(*crtcs)); > memset(offsets, 0, connector_count * sizeof(*offsets)); > > + drm_client_match_edp_lid(dev, connectors, connector_count, > enabled); > if (!drm_client_target_cloned(dev, connectors, > connector_count, modes, > offsets, enabled, width, > height) && > !drm_client_target_preferred(dev, connectors, > connector_count, modes, > -- > 2.43.0 >
