[PATCH v3] drm/fb-helper: Detect when lid is closed during initialization
Mario Limonciello
mario.limonciello at amd.com
Fri Jun 14 14:20:53 UTC 2024
On 6/14/2024 09:17, Thomas Zimmermann wrote:
> Hi
>
> Am 14.06.24 um 15:47 schrieb Mario Limonciello:
>> On 6/14/2024 03:15, Thomas Zimmermann wrote:
>>> Hi Mario
>>>
>>> Am 13.06.24 um 07:17 schrieb Mario Limonciello:
>>>> 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
>>>> lid status and if it's closed disable any eDP connectors.
>>>>
>>>> Also set up a workqueue to monitor for any future lid events.
>>>
>>> After reading through this patchset, I think fbdev emulation is not
>>> the right place for this code, as lid state is global.
>>>
>>> You could put this into drm_client_modeset.c and track lid state per
>>> client. drm_fb_helper_lid_work() would call the client's hotplug
>>> callback. But preferable, lid state should be tracked per DRM device
>>> in struct drm_mode_config and call drm_client_dev_hotplug() on each
>>> lid-state event. Thoughts? Best regards Thomas
>>
>> This is pretty similar to what I first did when moving from ACPI over
>> to generic input switch.
>>
>> It works for the initial configuration. But I don't believe it makes
>> sense for the lid switch events because not all DRM clients will
>> "want" to respond to the lid switch events. By leaving it up to the
>> client for everything except fbdev emulation they can also track the
>> lid switch and decide the policy.
>
>
> All our current clients do fbdev emulation, possibly others would be the
> panic screen and a boot-up logo. A panic screen doesn't do actual mode
> setting, but any other client would most likely want enable and disable
> the display depending on the lid state. Having this code in the DRM
> client helpers make perfect sense. But as it's global state, it makes no
> sense to set this up per client. Hence the suggestion to manage this in
> per DRM device.
>
> It would also make sense to try to integrate this into the probe
> helpers. When the lid state changes, the probe helpers would invoke the
> driver's regular hotplugging code.
Got it; I'll do some experimentation with this change, thanks for the
feedback.
>
>
>>
>> I also worry about what happens if the kernel does a hotplug callback
>> on lid events as well at the client choosing to do it. Don't we end up
>> with two modesets? So then I would think you need a handshake of some
>> sort to decide whether to do it for a given client where fbdev
>> emulation would opt in and then all other clients can choose to opt in
>> or not.
>
>
> What do you mean by the kernel does a hotplug event and the client does
> one? There should really only be one place to handle all of this. If we
> end up with two modesets, we'd get an additional flicker when the lid
> gets opened.
>
I'll see if my worry is founded after I move it all over.
>
> Best regards
> Thomas
>
>>
>>>>
>>>> Suggested-by: Dmitry Torokhov <dmitry.torokhov at gmail.com>
>>>> Reported-by: Chris Bainbridge <chris.bainbridge at gmail.com>
>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
>>>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>>>> ---
>>>> v2->v3:
>>>> * Use input device instead of ACPI device
>>>> * Detect lid open/close events
>>>> ---
>>>> drivers/gpu/drm/drm_client_modeset.c | 29 ++++++
>>>> drivers/gpu/drm/drm_fb_helper.c | 132
>>>> +++++++++++++++++++++++++++
>>>> include/drm/drm_device.h | 6 ++
>>>> include/drm/drm_fb_helper.h | 2 +
>>>> 4 files changed, 169 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_client_modeset.c
>>>> b/drivers/gpu/drm/drm_client_modeset.c
>>>> index 31af5cf37a09..b8adfe87334b 100644
>>>> --- a/drivers/gpu/drm/drm_client_modeset.c
>>>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>>>> @@ -257,6 +257,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 (dev->lid_closed) {
>>>> + 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 +872,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,
>>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>>>> b/drivers/gpu/drm/drm_fb_helper.c
>>>> index d612133e2cf7..41dd5887599a 100644
>>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>>> @@ -30,6 +30,8 @@
>>>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> #include <linux/console.h>
>>>> +#include <linux/input.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> #include <linux/pci.h>
>>>> #include <linux/sysrq.h>
>>>> #include <linux/vga_switcheroo.h>
>>>> @@ -413,6 +415,128 @@ static void drm_fb_helper_damage_work(struct
>>>> work_struct *work)
>>>> drm_fb_helper_fb_dirty(helper);
>>>> }
>>>> +static void drm_fb_helper_lid_event(struct input_handle *handle,
>>>> unsigned int type,
>>>> + unsigned int code, int value)
>>>> +{
>>>> + if (type == EV_SW && code == SW_LID) {
>>>> + struct drm_fb_helper *fb_helper = handle->handler->private;
>>>> +
>>>> + if (value != fb_helper->dev->lid_closed) {
>>>> + fb_helper->dev->lid_closed = value;
>>>> + queue_work(fb_helper->input_wq, &fb_helper->lid_work);
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> +struct drm_fb_lid {
>>>> + struct input_handle handle;
>>>> +};
>>>> +
>>>> +static int drm_fb_helper_lid_connect(struct input_handler *handler,
>>>> + struct input_dev *dev,
>>>> + const struct input_device_id *id)
>>>> +{
>>>> + struct drm_fb_helper *fb_helper = handler->private;
>>>> + struct drm_fb_lid *lid;
>>>> + char *name;
>>>> + int error;
>>>> +
>>>> + lid = kzalloc(sizeof(*lid), GFP_KERNEL);
>>>> + if (!lid)
>>>> + return -ENOMEM;
>>>> +
>>>> + name = kasprintf(GFP_KERNEL, "drm-fb-helper-lid-%s",
>>>> dev_name(&dev->dev));
>>>> + if (!name) {
>>>> + error = -ENOMEM;
>>>> + goto err_free_lid;
>>>> + }
>>>> +
>>>> + lid->handle.dev = dev;
>>>> + lid->handle.handler = handler;
>>>> + lid->handle.name = name;
>>>> + lid->handle.private = lid;
>>>> +
>>>> + error = input_register_handle(&lid->handle);
>>>> + if (error)
>>>> + goto err_free_name;
>>>> +
>>>> + error = input_open_device(&lid->handle);
>>>> + if (error)
>>>> + goto err_unregister_handle;
>>>> +
>>>> + fb_helper->dev->lid_closed = dev->sw[SW_LID];
>>>> + drm_dbg_kms(fb_helper->dev, "initial lid state is set to %d\n",
>>>> fb_helper->dev->lid_closed);
>>>> +
>>>> + return 0;
>>>> +
>>>> +err_unregister_handle:
>>>> + input_unregister_handle(&lid->handle);
>>>> +err_free_name:
>>>> + kfree(name);
>>>> +err_free_lid:
>>>> + kfree(lid);
>>>> + return error;
>>>> +}
>>>> +
>>>> +static void drm_fb_helper_lid_disconnect(struct input_handle *handle)
>>>> +{
>>>> + struct drm_fb_lid *lid = handle->private;
>>>> +
>>>> + input_close_device(handle);
>>>> + input_unregister_handle(handle);
>>>> +
>>>> + kfree(handle->name);
>>>> + kfree(lid);
>>>> +}
>>>> +
>>>> +static const struct input_device_id drm_fb_helper_lid_ids[] = {
>>>> + {
>>>> + .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
>>>> INPUT_DEVICE_ID_MATCH_SWBIT,
>>>> + .evbit = { BIT_MASK(EV_SW) },
>>>> + .swbit = { [BIT_WORD(SW_LID)] = BIT_MASK(SW_LID) },
>>>> + },
>>>> + { },
>>>> +};
>>>> +
>>>> +static struct input_handler drm_fb_helper_lid_handler = {
>>>> + .event = drm_fb_helper_lid_event,
>>>> + .connect = drm_fb_helper_lid_connect,
>>>> + .disconnect = drm_fb_helper_lid_disconnect,
>>>> + .name = "drm-fb-helper-lid",
>>>> + .id_table = drm_fb_helper_lid_ids,
>>>> +};
>>>> +
>>>> +static void drm_fb_helper_lid_work(struct work_struct *work)
>>>> +{
>>>> + struct drm_fb_helper *fb_helper = container_of(work, struct
>>>> drm_fb_helper,
>>>> + lid_work);
>>>> + drm_fb_helper_hotplug_event(fb_helper);
>>>> +}
>>>> +
>>>> +static int drm_fb_helper_create_lid_handler(struct drm_fb_helper
>>>> *fb_helper)
>>>> +{
>>>> + int ret = 0;
>>>> +
>>>> + if (fb_helper->deferred_setup)
>>>> + return 0;
>>>> +
>>>> + fb_helper->input_wq = create_singlethread_workqueue("drm-fb-lid");
>>>> + if (fb_helper->input_wq == NULL)
>>>> + return -ENOMEM;
>>>> +
>>>> + drm_fb_helper_lid_handler.private = fb_helper;
>>>> + ret = input_register_handler(&drm_fb_helper_lid_handler);
>>>> + if (ret)
>>>> + goto remove_wq;
>>>> +
>>>> + return 0;
>>>> +
>>>> +remove_wq:
>>>> + destroy_workqueue(fb_helper->input_wq);
>>>> + fb_helper->input_wq = NULL;
>>>> + return ret;
>>>> +}
>>>> +
>>>> /**
>>>> * drm_fb_helper_prepare - setup a drm_fb_helper structure
>>>> * @dev: DRM device
>>>> @@ -445,6 +569,7 @@ void drm_fb_helper_prepare(struct drm_device
>>>> *dev, struct drm_fb_helper *helper,
>>>> spin_lock_init(&helper->damage_lock);
>>>> INIT_WORK(&helper->resume_work, drm_fb_helper_resume_worker);
>>>> INIT_WORK(&helper->damage_work, drm_fb_helper_damage_work);
>>>> + INIT_WORK(&helper->lid_work, drm_fb_helper_lid_work);
>>>> helper->damage_clip.x1 = helper->damage_clip.y1 = ~0;
>>>> mutex_init(&helper->lock);
>>>> helper->funcs = funcs;
>>>> @@ -593,6 +718,9 @@ void drm_fb_helper_fini(struct drm_fb_helper
>>>> *fb_helper)
>>>> if (!drm_fbdev_emulation)
>>>> return;
>>>> + input_unregister_handler(&drm_fb_helper_lid_handler);
>>>> + destroy_workqueue(fb_helper->input_wq);
>>>> +
>>>> cancel_work_sync(&fb_helper->resume_work);
>>>> cancel_work_sync(&fb_helper->damage_work);
>>>> @@ -1842,6 +1970,10 @@
>>>> __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper
>>>> *fb_helper)
>>>> width = dev->mode_config.max_width;
>>>> height = dev->mode_config.max_height;
>>>> + ret = drm_fb_helper_create_lid_handler(fb_helper);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> drm_client_modeset_probe(&fb_helper->client, width, height);
>>>> ret = drm_fb_helper_single_fb_probe(fb_helper);
>>>> if (ret < 0) {
>>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
>>>> index 63767cf24371..619af597784c 100644
>>>> --- a/include/drm/drm_device.h
>>>> +++ b/include/drm/drm_device.h
>>>> @@ -316,6 +316,12 @@ struct drm_device {
>>>> * Root directory for debugfs files.
>>>> */
>>>> struct dentry *debugfs_root;
>>>> +
>>>> + /**
>>>> + * @lid_closed: Flag to tell the lid switch state
>>>> + */
>>>> + bool lid_closed;
>>>> +
>>>> };
>>>> #endif
>>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>>>> index 375737fd6c36..7fb36c10299d 100644
>>>> --- a/include/drm/drm_fb_helper.h
>>>> +++ b/include/drm/drm_fb_helper.h
>>>> @@ -143,6 +143,8 @@ struct drm_fb_helper {
>>>> spinlock_t damage_lock;
>>>> struct work_struct damage_work;
>>>> struct work_struct resume_work;
>>>> + struct work_struct lid_work;
>>>> + struct workqueue_struct *input_wq;
>>>> /**
>>>> * @lock:
>>>
>>
>
More information about the amd-gfx
mailing list