Message-ID: <CALWYx-ZSe0MO7Lhp=DVG3LNUU5ZnDWA1PjOL1nWm6n-Aj8TYkg@mail.gmail.com>
Date:   Thu, 21 Sep 2023 12:44:10 +0900
From:   Brandon Ross Pollack <brpol@...omium.org>
To:     Helen Koike <helen.koike@...labora.com>
Cc:     marius.vlad@...labora.com, mairacanal@...eup.net,
        jshargo@...omium.org, hamohammed.sa@...il.com,
        rodrigosiqueiramelo@...il.com, linux-doc@...r.kernel.org,
        hirono@...omium.org, corbet@....net, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, melissa.srw@...il.com,
        mduggan@...omium.org, mripard@...nel.org, tzimmermann@...e.de
Subject: Re: [PATCH v6 7/7] drm/vkms Add hotplug support via configfs to VKMS.

Sorry, these tests are actually running in the chromeOS infrastructure
environment!  A similar test can be written in IGT (and I think is in
the other chain that Marius published)

On Thu, Sep 21, 2023 at 3:03 AM Helen Koike <helen.koike@...labora.com> wrote:
>
> Hello!
>
> Thanks for the patch.
>
> On 29/08/2023 02:30, Brandon Pollack wrote:
> > This change adds the ability to read or write a "1" or a "0" to the
> > newly added "connected" attribute of a connector in the vkms entry in
> > configfs.
> >
> > A write will trigger a call to drm_kms_helper_hotplug_event, causing a
> > hotplug uevent.
> >
> > With this we can write virtualized multidisplay tests that involve
> > hotplugging displays (eg recompositing windows when a monitor is turned
> > off).
>
> Are these tests going to be added in igt?
>
> I was just wondering if it requires any special thing for drm ci:
>
> https://lists.freedesktop.org/archives/dri-devel/2023-September/423719.html
>
> (btw, it would be awesome of you could test your changes with drm ci :)
>
> Regards,
> Helen
>
> >
> > Signed-off-by: Brandon Pollack <brpol@...omium.org>
> > ---
> >   Documentation/gpu/vkms.rst           |  2 +-
> >   drivers/gpu/drm/vkms/vkms_configfs.c | 68 ++++++++++++++++++++++++++--
> >   drivers/gpu/drm/vkms/vkms_drv.h      | 11 +++++
> >   drivers/gpu/drm/vkms/vkms_output.c   | 47 ++++++++++++++++++-
> >   4 files changed, 123 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> > index c3875bf66dba..7f715097539c 100644
> > --- a/Documentation/gpu/vkms.rst
> > +++ b/Documentation/gpu/vkms.rst
> > @@ -145,7 +145,7 @@ We want to be able to manipulate vkms instances without having to reload the
> >   module. Such configuration can be added as extensions to vkms's ConfigFS
> >   support. Use-cases:
> >
> > -- Hotplug/hotremove connectors on the fly (to be able to test DP MST handling
> > +- Hotremove connectors on the fly (to be able to test DP MST handling
> >     of compositors).
> >
> >   - Change output configuration: Plug/unplug screens, change EDID, allow changing
> > diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> > index bc35dcc47585..d231e28101ae 100644
> > --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> > +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> > @@ -1,5 +1,6 @@
> >   // SPDX-License-Identifier: GPL-2.0+
> >
> > +#include "drm/drm_probe_helper.h"
> >   #include <linux/configfs.h>
> >   #include <linux/mutex.h>
> >   #include <linux/platform_device.h>
> > @@ -40,6 +41,7 @@
> >    *   `-- vkms
> >    *       `-- test
> >    *           |-- connectors
> > + *                `-- connected
> >    *           |-- crtcs
> >    *           |-- encoders
> >    *           |-- planes
> > @@ -89,6 +91,14 @@
> >    *
> >    *   echo 1 > /config/vkms/test/enabled
> >    *
> > + * By default no display is "connected" so to connect a connector you'll also
> > + * have to write 1 to a connectors "connected" attribute::
> > + *
> > + *   echo 1 > /config/vkms/test/connectors/connector/connected
> > + *
> > + * One can verify that this is worked using the `modetest` utility or the
> > + * equivalent for your platform.
> > + *
> >    * When you're done with the virtual device, you can clean up the device like
> >    * so::
> >    *
> > @@ -236,7 +246,58 @@ static void add_possible_encoders(struct config_group *parent,
> >
> >   /*  Connector item, e.g. /config/vkms/device/connectors/ID */
> >
> > +static ssize_t connector_connected_show(struct config_item *item, char *buf)
> > +{
> > +     struct vkms_config_connector *connector =
> > +             item_to_config_connector(item);
> > +     struct vkms_configfs *configfs = connector_item_to_configfs(item);
> > +     bool connected = false;
> > +
> > +     mutex_lock(&configfs->lock);
> > +     connected = connector->connected;
> > +     mutex_unlock(&configfs->lock);
> > +
> > +     return sprintf(buf, "%d\n", connected);
> > +}
> > +
> > +static ssize_t connector_connected_store(struct config_item *item,
> > +                                      const char *buf, size_t len)
> > +{
> > +     struct vkms_config_connector *connector =
> > +             item_to_config_connector(item);
> > +     struct vkms_configfs *configfs = connector_item_to_configfs(item);
> > +     int val, ret;
> > +
> > +     ret = kstrtouint(buf, 10, &val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (val != 1 && val != 0)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&configfs->lock);
> > +     connector->connected = val;
> > +     if (!connector->connector) {
> > +             pr_info("VKMS Device %s is not yet enabled, connector will be enabled on start",
> > +                     configfs->device_group.cg_item.ci_name);
> > +     }
> > +     mutex_unlock(&configfs->lock);
> > +
> > +     if (connector->connector)
> > +             drm_kms_helper_hotplug_event(connector->connector->dev);
> > +
> > +     return len;
> > +}
> > +
> > +CONFIGFS_ATTR(connector_, connected);
> > +
> > +static struct configfs_attribute *connector_attrs[] = {
> > +     &connector_attr_connected,
> > +     NULL,
> > +};
> > +
> >   static struct config_item_type connector_type = {
> > +     .ct_attrs = connector_attrs,
> >       .ct_owner = THIS_MODULE,
> >   };
> >
> > @@ -264,7 +325,7 @@ static ssize_t plane_type_show(struct config_item *item, char *buf)
> >       plane_type = plane->type;
> >       mutex_unlock(&configfs->lock);
> >
> > -     return sprintf(buf, "%u", plane_type);
> > +     return sprintf(buf, "%u\n", plane_type);
> >   }
> >
> >   static ssize_t plane_type_store(struct config_item *item, const char *buf,
> > @@ -319,6 +380,7 @@ static struct config_group *connectors_group_make(struct config_group *group,
> >                                   &connector_type);
> >       add_possible_encoders(&connector->config_group,
> >                             &connector->possible_encoders.group);
> > +     connector->connected = false;
> >
> >       return &connector->config_group;
> >   }
> > @@ -500,7 +562,7 @@ static ssize_t device_enabled_show(struct config_item *item, char *buf)
> >       is_enabled = configfs->vkms_device != NULL;
> >       mutex_unlock(&configfs->lock);
> >
> > -     return sprintf(buf, "%d", is_enabled);
> > +     return sprintf(buf, "%d\n", is_enabled);
> >   }
> >
> >   static ssize_t device_enabled_store(struct config_item *item, const char *buf,
> > @@ -557,7 +619,7 @@ static ssize_t device_id_show(struct config_item *item, char *buf)
> >
> >       mutex_unlock(&configfs->lock);
> >
> > -     return sprintf(buf, "%d", id);
> > +     return sprintf(buf, "%d\n", id);
> >   }
> >
> >   CONFIGFS_ATTR_RO(device_, id);
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 2b9545ada9c2..5336281f397e 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -3,6 +3,7 @@
> >   #ifndef _VKMS_DRV_H_
> >   #define _VKMS_DRV_H_
> >
> > +#include "drm/drm_connector.h"
> >   #include <linux/configfs.h>
> >   #include <linux/hrtimer.h>
> >
> > @@ -147,7 +148,9 @@ struct vkms_config_links {
> >
> >   struct vkms_config_connector {
> >       struct config_group config_group;
> > +     struct drm_connector *connector;
> >       struct vkms_config_links possible_encoders;
> > +     bool connected;
> >   };
> >
> >   struct vkms_config_crtc {
> > @@ -220,6 +223,10 @@ struct vkms_device {
> >   #define item_to_configfs(item) \
> >       container_of(to_config_group(item), struct vkms_configfs, device_group)
> >
> > +#define connector_item_to_configfs(item)                                     \
> > +     container_of(to_config_group(item->ci_parent), struct vkms_configfs, \
> > +                  connectors_group)
> > +
> >   #define item_to_config_connector(item)                                    \
> >       container_of(to_config_group(item), struct vkms_config_connector, \
> >                    config_group)
> > @@ -279,4 +286,8 @@ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev,
> >   int vkms_init_configfs(void);
> >   void vkms_unregister_configfs(void);
> >
> > +/* Connector hotplugging */
> > +enum drm_connector_status vkms_connector_detect(struct drm_connector *connector,
> > +                                             bool force);
> > +
> >   #endif /* _VKMS_DRV_H_ */
> > diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 0ee1f3f4a305..1a1cd0202c5f 100644
> > --- a/drivers/gpu/drm/vkms/vkms_output.c
> > +++ b/drivers/gpu/drm/vkms/vkms_output.c
> > @@ -1,5 +1,6 @@
> >   // SPDX-License-Identifier: GPL-2.0+
> >
> > +#include <drm/drm_print.h>
> >   #include <drm/drm_atomic_helper.h>
> >   #include <drm/drm_connector.h>
> >   #include <drm/drm_crtc.h>
> > @@ -8,10 +9,12 @@
> >   #include <drm/drm_plane.h>
> >   #include <drm/drm_probe_helper.h>
> >   #include <drm/drm_simple_kms_helper.h>
> > +#include <linux/printk.h>
> >
> >   #include "vkms_drv.h"
> >
> >   static const struct drm_connector_funcs vkms_connector_funcs = {
> > +     .detect = vkms_connector_detect,
> >       .fill_modes = drm_helper_probe_single_connector_modes,
> >       .destroy = drm_connector_cleanup,
> >       .reset = drm_atomic_helper_connector_reset,
> > @@ -19,6 +22,48 @@ static const struct drm_connector_funcs vkms_connector_funcs = {
> >       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> >   };
> >
> > +static const struct vkms_config_connector *
> > +find_config_for_connector(struct drm_connector *connector)
> > +{
> > +     struct vkms_device *vkms = drm_device_to_vkms_device(connector->dev);
> > +     struct vkms_configfs *configfs = vkms->configfs;
> > +     struct config_item *item;
> > +
> > +     if (!configfs) {
> > +             pr_info("Default connector has no configfs entry");
> > +             return NULL;
> > +     }
> > +
> > +     list_for_each_entry(item, &configfs->connectors_group.cg_children,
> > +                         ci_entry) {
> > +             struct vkms_config_connector *config_connector =
> > +                     item_to_config_connector(item);
> > +             if (config_connector->connector == connector)
> > +                     return config_connector;
> > +     }
> > +
> > +     pr_warn("Could not find config to match connector %s, but configfs was initialized",
> > +             connector->name);
> > +
> > +     return NULL;
> > +}
> > +
> > +enum drm_connector_status vkms_connector_detect(struct drm_connector *connector,
> > +                                             bool force)
> > +{
> > +     enum drm_connector_status status = connector_status_connected;
> > +     const struct vkms_config_connector *config_connector =
> > +             find_config_for_connector(connector);
> > +
> > +     if (!config_connector)
> > +             return connector_status_connected;
> > +
> > +     if (!config_connector->connected)
> > +             status = connector_status_disconnected;
> > +
> > +     return status;
> > +}
> > +
> >   static const struct drm_encoder_funcs vkms_encoder_funcs = {
> >       .destroy = drm_encoder_cleanup,
> >   };
> > @@ -280,12 +325,12 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> >               struct vkms_config_connector *config_connector =
> >                       item_to_config_connector(item);
> >               struct drm_connector *connector = vkms_connector_init(vkmsdev);
> > -
> >               if (IS_ERR(connector)) {
> >                       DRM_ERROR("Failed to init connector from config: %s",
> >                                 item->ci_name);
> >                       return PTR_ERR(connector);
> >               }
> > +             config_connector->connector = connector;
> >
> >               for (int j = 0; j < output->num_encoders; j++) {
> >                       struct encoder_map *encoder = &encoder_map[j];

Powered by blists - more mailing lists