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 <[email protected]>
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
Signed-off-by: Mario Limonciello <[email protected]>
---
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 <acpi/button.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/slab.h>
@@ -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
On Tue, May 28, 2024 at 5:03 PM Mario Limonciello
<[email protected]> 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 <[email protected]>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> Signed-off-by: Mario Limonciello <[email protected]>
Reviewed-by: Alex Deucher <[email protected]>
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 <acpi/button.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/slab.h>
> @@ -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
>
On Wed, May 29, 2024 at 9:51 AM Jani Nikula <[email protected]> wrote:
>
> On Wed, 29 May 2024, Alex Deucher <[email protected]> wrote:
> > On Tue, May 28, 2024 at 5:03 PM Mario Limonciello
> > <[email protected]> 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 <[email protected]>
> >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> >> Signed-off-by: Mario Limonciello <[email protected]>
> >
> > Reviewed-by: Alex Deucher <[email protected]>
> >
> > 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 <acpi/button.h>
> >> #include <linux/module.h>
> >> #include <linux/mutex.h>
> >> #include <linux/slab.h>
> >> @@ -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
On Wed, 29 May 2024, Alex Deucher <[email protected]> wrote:
> On Tue, May 28, 2024 at 5:03 PM Mario Limonciello
> <[email protected]> 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 <[email protected]>
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
>> Signed-off-by: Mario Limonciello <[email protected]>
>
> Reviewed-by: Alex Deucher <[email protected]>
>
> 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 <acpi/button.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> #include <linux/slab.h>
>> @@ -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
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 <[email protected]>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> Signed-off-by: Mario Limonciello <[email protected]>
> ---
> 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 <acpi/button.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/slab.h>
> @@ -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
On 5/29/2024 08:55, Alex Deucher wrote:
> On Wed, May 29, 2024 at 9:51 AM Jani Nikula <[email protected]> wrote:
>>
>> On Wed, 29 May 2024, Alex Deucher <[email protected]> wrote:
>>> On Tue, May 28, 2024 at 5:03 PM Mario Limonciello
>>> <[email protected]> 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 <[email protected]>
>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
>>>> Signed-off-by: Mario Limonciello <[email protected]>
>>>
>>> Reviewed-by: Alex Deucher <[email protected]>
>>>
>>> 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 <acpi/button.h>
>>>> #include <linux/module.h>
>>>> #include <linux/mutex.h>
>>>> #include <linux/slab.h>
>>>> @@ -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
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 <[email protected]>
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
>> Signed-off-by: Mario Limonciello <[email protected]>
>> ---
>> 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 <acpi/button.h>
>> #include <linux/module.h>
>> #include <linux/mutex.h>
>> #include <linux/slab.h>
>> @@ -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
>
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 <[email protected]>
> >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> >> Signed-off-by: Mario Limonciello <[email protected]>
> >> ---
> >> 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 <acpi/button.h>
> >> #include <linux/module.h>
> >> #include <linux/mutex.h>
> >> #include <linux/slab.h>
> >> @@ -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
>>>
>>> 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.
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 <[email protected]>
> > >> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> > >> Signed-off-by: Mario Limonciello <[email protected]>
> > >> ---
> > >> 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 <acpi/button.h>
> > >> #include <linux/module.h>
> > >> #include <linux/mutex.h>
> > >> #include <linux/slab.h>
> > >> @@ -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
>> 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?
On Thu, 30 May 2024 at 07:41, Limonciello, Mario
<[email protected]> 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
On Thu, May 30, 2024 at 11:07:53AM +0300, Dmitry Baryshkov wrote:
> On Thu, 30 May 2024 at 07:41, Limonciello, Mario
> <[email protected]> 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
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 <[email protected]>
| 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
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 <[email protected]>
> | 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
}
}
On Wed, 05 Jun 2024, Chris Bainbridge <[email protected]> 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 <[email protected]>
>> | 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
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
On Thu, Jun 06, 2024 at 10:21:07AM +0300, Jani Nikula wrote:
> On Wed, 05 Jun 2024, Chris Bainbridge <[email protected]> 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 <[email protected]>
> >> | 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