Re: [PATCH v3] drm/fb-helper: Detect when lid is closed during initialization

[Mario Limonciello <mario.limonciello@xxxxxxx>]

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@xxxxxxxxx>
> > > > Reported-by: Chris Bainbridge <chris.bainbridge@xxxxxxxxx>
> > > > Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3349
> > > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > > > ---
> > > > 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: