[PATCH 0/2] drm/vkms: Introduce basic support for configfs

[PATCH 0/2] drm/vkms: Introduce basic support for configfs

[Rodrigo Siqueira]

This patchset introduces the support for configfs in vkms by adding a
primary structure for handling the vkms subsystem and exposing
connectors as a use case.  This series allows enabling/disabling virtual
and writeback connectors on the fly. The first patch of this series
reworks the initialization and cleanup code of each type of connector,
with this change, the second patch adds the configfs support for vkms.
It is important to highlight that this patchset depends on
https://patchwork.freedesktop.org/series/61738/.

After applying this series, the user can utilize these features with the
following steps:

1. Load vkms without parameter

  modprobe vkms

2. Mount a configfs filesystem

  mount -t configfs none /mnt/

After that, the vkms subsystem will look like this:

vkms/
 |__connectors
    |__Virtual
        |__ enable

The connectors directories have information related to connectors, and
as can be seen, the virtual connector is enabled by default. Inside a
connector directory (e.g., Virtual) has an attribute named ‘enable’
which is used to enable and disable the target connector. For example,
the Virtual connector has the enable attribute set to 1. If the user
wants to enable the writeback connector it is required to use the mkdir
command, as follows:

  cd /mnt/vkms/connectors
  mkdir Writeback

After the above command, the writeback connector will be enabled, and
the user could see the following tree:

vkms/
 |__connectors
    |__Virtual
    |   |__ enable
    |__Writeback
        |__ enable

If the user wants to remove the writeback connector, it is required to
use the command rmdir, for example

  rmdir Writeback

Another way to enable and disable a connector it is by using the enable
attribute, for example, we can disable the Virtual connector with:

  echo 0 > /mnt/vkms/connectors/Virtual/enable

And enable it again with:

  echo 1 > /mnt/vkms/connectors/Virtual/enable

It is important to highlight that configfs 'obey' the parameters used
during the vkms load and does not allow users to remove a connector
directory if it was load via module parameter. For example:

  modprobe vkms enable_writeback=1

vkms/
 |__connectors
    |__Virtual
    |   |__ enable
    |__Writeback
        |__ enable

If the user tries to remove the Writeback connector with “rmdir
Writeback”, the operation will be not permitted because the Writeback
connector was loaded with the modules. However, the user may disable the
writeback connector with:

  echo 0 > /mnt/vkms/connectors/Writeback/enable


Rodrigo Siqueira (2):
  drm/vkms: Add enable/disable functions per connector
  drm/vkms: Introduce configfs for enabling/disabling connectors

 drivers/gpu/drm/vkms/Makefile         |   3 +-
 drivers/gpu/drm/vkms/vkms_configfs.c  | 229 ++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c       |   6 +
 drivers/gpu/drm/vkms/vkms_drv.h       |  17 ++
 drivers/gpu/drm/vkms/vkms_output.c    |  84 ++++++----
 drivers/gpu/drm/vkms/vkms_writeback.c |  31 +++-
 6 files changed, 332 insertions(+), 38 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c

-- 
2.21.0


-- 
Rodrigo Siqueira
https://siqueira.tech

[PATCH 1/2] drm/vkms: Add enable/disable functions per connector

[Rodrigo Siqueira]

Currently, virtual and writeback connectors have the code responsible
for initialization and cleanup spread around different places in vkms.
This patch creates an enable and disable function per connector which
allows vkms to hotplug connectors easily.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.h       |  5 ++
 drivers/gpu/drm/vkms/vkms_output.c    | 84 +++++++++++++++++----------
 drivers/gpu/drm/vkms/vkms_writeback.c | 31 ++++++++--
 3 files changed, 83 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 9ff2cd4ebf81..a1ca5c658355 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -152,7 +152,12 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 /* Composer Support */
 void vkms_composer_worker(struct work_struct *work);
 
+/* Virtual connector */
+int enable_virtual_connector(struct vkms_device *vkmsdev);
+void disable_virtual_connector(struct vkms_device *vkmsdev);
+
 /* Writeback */
 int enable_writeback_connector(struct vkms_device *vkmsdev);
+void disable_writeback_connector(struct vkms_device *connector);
 
 #endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index aea1d4410864..26ecab52e82e 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -6,6 +6,7 @@
 
 static void vkms_connector_destroy(struct drm_connector *connector)
 {
+	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 }
 
@@ -35,37 +36,19 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.get_modes    = vkms_conn_get_modes,
 };
 
-int vkms_output_init(struct vkms_device *vkmsdev, int index)
+int enable_virtual_connector(struct vkms_device *vkmsdev)
 {
 	struct vkms_output *output = &vkmsdev->output;
-	struct drm_device *dev = &vkmsdev->drm;
 	struct drm_connector *connector = &output->connector;
 	struct drm_encoder *encoder = &output->encoder;
-	struct drm_crtc *crtc = &output->crtc;
-	struct drm_plane *primary, *cursor = NULL;
+	struct drm_device *dev = &vkmsdev->drm;
 	int ret;
 
-	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
-	if (IS_ERR(primary))
-		return PTR_ERR(primary);
-
-	if (enable_cursor) {
-		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
-		if (IS_ERR(cursor)) {
-			ret = PTR_ERR(cursor);
-			goto err_cursor;
-		}
-	}
-
-	ret = vkms_crtc_init(dev, crtc, primary, cursor);
-	if (ret)
-		goto err_crtc;
-
 	ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
 				 DRM_MODE_CONNECTOR_VIRTUAL);
 	if (ret) {
 		DRM_ERROR("Failed to init connector\n");
-		goto err_connector;
+		return ret;
 	}
 
 	drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
@@ -84,17 +67,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 		goto err_attach;
 	}
 
-	if (enable_writeback) {
-		ret = enable_writeback_connector(vkmsdev);
-		if (!ret) {
-			output->composer_enabled = true;
-			DRM_INFO("Writeback connector enabled");
-		} else {
-			DRM_ERROR("Failed to init writeback connector\n");
-		}
-	}
-
-	drm_mode_config_reset(dev);
+	drm_connector_register(connector);
 
 	return 0;
 
@@ -104,6 +77,53 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 err_encoder:
 	drm_connector_cleanup(connector);
 
+	return ret;
+}
+
+void disable_virtual_connector(struct vkms_device *vkmsdev)
+{
+	struct vkms_output *output = &vkmsdev->output;
+
+	drm_connector_unregister(&output->connector);
+	drm_connector_cleanup(&output->connector);
+	drm_encoder_cleanup(&output->encoder);
+}
+
+int vkms_output_init(struct vkms_device *vkmsdev, int index)
+{
+	struct vkms_output *output = &vkmsdev->output;
+	struct drm_device *dev = &vkmsdev->drm;
+	struct drm_crtc *crtc = &output->crtc;
+	struct drm_plane *primary, *cursor = NULL;
+	int ret;
+
+	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
+	if (IS_ERR(primary))
+		return PTR_ERR(primary);
+
+	if (enable_cursor) {
+		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
+		if (IS_ERR(cursor)) {
+			ret = PTR_ERR(cursor);
+			goto err_cursor;
+		}
+	}
+
+	ret = vkms_crtc_init(dev, crtc, primary, cursor);
+	if (ret)
+		goto err_crtc;
+
+	ret = enable_virtual_connector(vkmsdev);
+	if (ret)
+		goto err_connector;
+
+	if (enable_writeback)
+		enable_writeback_connector(vkmsdev);
+
+	drm_mode_config_reset(dev);
+
+	return 0;
+
 err_connector:
 	drm_crtc_cleanup(crtc);
 
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 34dad37a0236..6a3f37d60c1d 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -125,17 +125,38 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
 	.atomic_commit = vkms_wb_atomic_commit,
 };
 
+void disable_writeback_connector(struct vkms_device *vkmsdev)
+{
+	struct vkms_output *output = &vkmsdev->output;
+
+	output->composer_enabled = false;
+	drm_connector_unregister(&output->wb_connector.base);
+	drm_connector_cleanup(&output->wb_connector.base);
+	drm_encoder_cleanup(&output->wb_connector.encoder);
+}
+
 int enable_writeback_connector(struct vkms_device *vkmsdev)
 {
 	struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
+	int ret;
 
 	vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
 	drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
 
-	return drm_writeback_connector_init(&vkmsdev->drm, wb,
-					    &vkms_wb_connector_funcs,
-					    &vkms_wb_encoder_helper_funcs,
-					    vkms_wb_formats,
-					    ARRAY_SIZE(vkms_wb_formats));
+	ret = drm_writeback_connector_init(&vkmsdev->drm, wb,
+					   &vkms_wb_connector_funcs,
+					   &vkms_wb_encoder_helper_funcs,
+					   vkms_wb_formats,
+					   ARRAY_SIZE(vkms_wb_formats));
+	if (!ret) {
+		vkmsdev->output.composer_enabled = true;
+		DRM_INFO("Writeback connector enabled");
+	} else {
+		DRM_ERROR("Failed to init writeback connector\n");
+	}
+
+	drm_connector_register(&wb->base);
+
+	return ret;
 }
 
-- 
2.21.0

[PATCH 2/2] drm/vkms: Introduce configfs for enabling/disabling connectors

[Rodrigo Siqueira]

This patch introduces an implementation of vkms subsystems through
configfs; we want to be able to reconfigure vkms instance without having
to reload the module. This commit adds the primary support for configfs
by allowing vkms to expose the ability to enable/disable its connectors.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/gpu/drm/vkms/Makefile        |   3 +-
 drivers/gpu/drm/vkms/vkms_configfs.c | 229 +++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c      |   6 +
 drivers/gpu/drm/vkms/vkms_drv.h      |  12 ++
 4 files changed, 249 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c

diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 333d3cead0e3..f90c016cd9fe 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -6,6 +6,7 @@ vkms-y := \
 	vkms_crtc.o \
 	vkms_gem.o \
 	vkms_composer.o \
-	vkms_writeback.o
+	vkms_writeback.o \
+	vkms_configfs.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
new file mode 100644
index 000000000000..5d1a30517cca
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include "vkms_drv.h"
+
+static struct config_group *connector_group;
+static struct vkms_device *vkms_device;
+
+static char *available_connectors[] = {
+	"Virtual",
+	"Writeback",
+};
+
+struct connectors_fs {
+	struct config_item item;
+	bool enable;
+};
+
+static int enable_connector(const char *name)
+{
+	int ret = 0;
+
+	if (!strcmp(name, "Writeback"))
+		ret = enable_writeback_connector(vkms_device);
+	else if (!strcmp(name, "Virtual"))
+		ret = enable_virtual_connector(vkms_device);
+
+	if (ret)
+		return ret;
+
+	drm_mode_config_reset(&vkms_device->drm);
+	return 0;
+}
+
+static void disable_connector(const char *name)
+{
+	if (!strcmp(name, "Writeback"))
+		disable_writeback_connector(vkms_device);
+	else if (!strcmp(name, "Virtual"))
+		disable_virtual_connector(vkms_device);
+}
+
+static inline struct connectors_fs *to_conn_item(struct config_item *item)
+{
+	return item ? container_of(item, struct connectors_fs, item) : NULL;
+}
+
+static ssize_t conn_fs_enable_show(struct config_item *item, char *page)
+{
+	struct connectors_fs *conn_item = to_conn_item(item);
+
+	return sprintf(page, "%d\n", conn_item->enable);
+}
+
+static ssize_t conn_fs_enable_store(struct config_item *item,
+				    const char *page, size_t count)
+{
+	struct connectors_fs *conn_item = to_conn_item(item);
+	char *p = (char *)page;
+	unsigned int enable;
+	int ret;
+
+	ret = kstrtouint(p, 10, &enable);
+	if (ret)
+		return -EINVAL;
+
+	if (enable > 1)
+		return -EINVAL;
+
+	if (enable == conn_item->enable)
+		return count;
+
+	if (enable) {
+		ret = enable_connector(item->ci_name);
+		if (ret)
+			return ret;
+	} else {
+		disable_connector(item->ci_name);
+	}
+
+	conn_item->enable = enable ? true : false;
+	return count;
+}
+
+CONFIGFS_ATTR(conn_fs_, enable);
+
+static struct configfs_attribute *connectors_fs_attrs[] = {
+	&conn_fs_attr_enable,
+	NULL,
+};
+
+static void connectors_fs_release(struct config_item *item)
+{
+	kfree(to_conn_item(item));
+}
+
+static struct configfs_item_operations connectors_fs_ops = {
+	.release = connectors_fs_release,
+};
+
+static const struct config_item_type connectors_fs_type = {
+	.ct_item_ops	= &connectors_fs_ops,
+	.ct_attrs	= connectors_fs_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_item *make_connector_item(struct config_group *group,
+					       const char *name)
+{
+	struct connectors_fs *conn_item;
+	int i, ret, total_conn = ARRAY_SIZE(available_connectors);
+
+	for (i = 0; i < total_conn; i++)
+		if (!strcmp(name, available_connectors[i]))
+			break;
+
+	if (i == total_conn)
+		return ERR_PTR(-EINVAL);
+
+	ret = enable_connector(name);
+	if (ret)
+		return ERR_PTR(ret);
+
+	conn_item = kzalloc(sizeof(*conn_item), GFP_KERNEL);
+	if (!conn_item)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&conn_item->item, name,
+				   &connectors_fs_type);
+
+	conn_item->enable = true;
+
+	return &conn_item->item;
+}
+
+static void drop_connector_item(struct config_group *group,
+				struct config_item *item)
+{
+	char *name = item->ci_name;
+
+	disable_connector(name);
+
+	config_item_put(item);
+}
+
+static struct configfs_group_operations connector_group_ops = {
+	.make_item	= make_connector_item,
+	.drop_item	= drop_connector_item,
+};
+
+static const struct config_item_type connector_type = {
+	.ct_group_ops	= &connector_group_ops,
+	.ct_owner	= THIS_MODULE,
+};
+
+static const struct config_item_type vkms_subsystem_type = {
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem vkms_subsystem = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "vkms",
+			.ci_type = &vkms_subsystem_type,
+		},
+	},
+	.su_mutex = __MUTEX_INITIALIZER(vkms_subsystem.su_mutex),
+};
+
+static void init_default_conn_configfs(struct config_group *root)
+{
+	struct vkms_config_state *config_state = &vkms_device->config_state;
+	int i, ret;
+
+	connector_group = configfs_register_default_group(root, "connectors",
+							  &connector_type);
+	if (IS_ERR(connector_group)) {
+		ret = PTR_ERR(connector_group);
+		pr_err("Error %d while registering functions group\n", ret);
+		return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(available_connectors); i++) {
+		struct config_group *group = config_state->connectors[i];
+
+		if (!strcmp(available_connectors[i], "Writeback") &&
+		    !enable_writeback)
+			continue;
+
+		group = configfs_register_default_group(connector_group,
+							available_connectors[i],
+							&connectors_fs_type);
+		if (IS_ERR(connector_group)) {
+			ret = PTR_ERR(config_state->connectors[i]);
+			DRM_ERROR("Error %d while trying to register %s\n",
+				  ret, available_connectors[i]);
+			continue;
+		}
+
+		to_conn_item(&group->cg_item)->enable = true;
+	}
+}
+
+int vkms_configfs_init(struct vkms_device *vkmsdev)
+{
+	struct config_group *root = &vkms_subsystem.su_group;
+	int ret;
+
+	vkms_device = vkmsdev;
+
+	config_group_init(root);
+	ret = configfs_register_subsystem(&vkms_subsystem);
+	if (ret) {
+		pr_err("Error %d while registering subsystem %s\n", ret,
+		       root->cg_item.ci_namebuf);
+		goto err;
+	}
+
+	init_default_conn_configfs(root);
+
+	return 0;
+
+err:
+	return ret;
+}
+
+void vkms_configfs_exit(void)
+{
+	configfs_unregister_subsystem(&vkms_subsystem);
+}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 152d7de24a76..a930a5a52ce4 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -166,6 +166,10 @@ static int __init vkms_init(void)
 	if (ret)
 		goto out_fini;
 
+	ret = vkms_configfs_init(vkms_device);
+	if (ret)
+		DRM_ERROR("Could not initialize configfs");
+
 	ret = drm_dev_register(&vkms_device->drm, 0);
 	if (ret)
 		goto out_fini;
@@ -194,6 +198,8 @@ static void __exit vkms_exit(void)
 	drm_dev_put(&vkms_device->drm);
 
 	kfree(vkms_device);
+
+	vkms_configfs_exit();
 }
 
 module_init(vkms_init);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index a1ca5c658355..c811bc192606 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -9,6 +9,7 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_writeback.h>
 #include <linux/hrtimer.h>
+#include <linux/configfs.h>
 
 #define XRES_MIN    20
 #define YRES_MIN    20
@@ -83,10 +84,17 @@ struct vkms_output {
 	spinlock_t composer_lock;
 };
 
+#define MAX_CONN_CONFIGFS 10
+
+struct vkms_config_state {
+	struct config_group *connectors[MAX_CONN_CONFIGFS];
+};
+
 struct vkms_device {
 	struct drm_device drm;
 	struct platform_device *platform;
 	struct vkms_output output;
+	struct vkms_config_state config_state;
 };
 
 struct vkms_gem_object {
@@ -160,4 +168,8 @@ void disable_virtual_connector(struct vkms_device *vkmsdev);
 int enable_writeback_connector(struct vkms_device *vkmsdev);
 void disable_writeback_connector(struct vkms_device *connector);
 
+/* Configfs */
+int vkms_configfs_init(struct vkms_device *vkmsdev);
+void vkms_configfs_exit(void);
+
 #endif /* _VKMS_DRV_H_ */
-- 
2.21.0

Re: [PATCH 0/2] drm/vkms: Introduce basic support for configfs

[Daniel Vetter]

On Mon, Jul 01, 2019 at 12:23:39AM -0300, Rodrigo Siqueira wrote:
> This patchset introduces the support for configfs in vkms by adding a
> primary structure for handling the vkms subsystem and exposing
> connectors as a use case.  This series allows enabling/disabling virtual
> and writeback connectors on the fly. The first patch of this series
> reworks the initialization and cleanup code of each type of connector,
> with this change, the second patch adds the configfs support for vkms.
> It is important to highlight that this patchset depends on
> https://patchwork.freedesktop.org/series/61738/.
> 
> After applying this series, the user can utilize these features with the
> following steps:
> 
> 1. Load vkms without parameter
> 
>   modprobe vkms
> 
> 2. Mount a configfs filesystem
> 
>   mount -t configfs none /mnt/
> 
> After that, the vkms subsystem will look like this:
> 
> vkms/
>  |__connectors
>     |__Virtual
>         |__ enable
> 
> The connectors directories have information related to connectors, and
> as can be seen, the virtual connector is enabled by default. Inside a
> connector directory (e.g., Virtual) has an attribute named ‘enable’
> which is used to enable and disable the target connector. For example,
> the Virtual connector has the enable attribute set to 1. If the user
> wants to enable the writeback connector it is required to use the mkdir
> command, as follows:
> 
>   cd /mnt/vkms/connectors
>   mkdir Writeback
> 
> After the above command, the writeback connector will be enabled, and
> the user could see the following tree:
> 
> vkms/
>  |__connectors
>     |__Virtual
>     |   |__ enable
>     |__Writeback
>         |__ enable
> 
> If the user wants to remove the writeback connector, it is required to
> use the command rmdir, for example
> 
>   rmdir Writeback
> 
> Another way to enable and disable a connector it is by using the enable
> attribute, for example, we can disable the Virtual connector with:
> 
>   echo 0 > /mnt/vkms/connectors/Virtual/enable
> 
> And enable it again with:
> 
>   echo 1 > /mnt/vkms/connectors/Virtual/enable
> 
> It is important to highlight that configfs 'obey' the parameters used
> during the vkms load and does not allow users to remove a connector
> directory if it was load via module parameter. For example:
> 
>   modprobe vkms enable_writeback=1
> 
> vkms/
>  |__connectors
>     |__Virtual
>     |   |__ enable
>     |__Writeback
>         |__ enable
> 
> If the user tries to remove the Writeback connector with “rmdir
> Writeback”, the operation will be not permitted because the Writeback
> connector was loaded with the modules. However, the user may disable the
> writeback connector with:
> 
>   echo 0 > /mnt/vkms/connectors/Writeback/enable

I guess I should have put a warning into that task that step one is
designing the interface. Here's the fundamental thoughts:

- The _only_ thing we can hotplug after drm_dev_register() is a
  drm_connector. That's an interesting use-case, but atm not really
  supported by the vkms codebase. So we can't just enable/disable
  writeback like this. We also can't change _anything_ else in the drm
  driver like this.

- The other bit we want is support multiple vkms instances, to simulate
  multi-gpus and fun stuff like that.

- Therefore vkms configs should be at the drm_device level, so a
  directory under configfs/vkms/ represents an entire device.

- We need a special config item to control
  drm_dev_register/drm_dev_unregister. While a drm_device is registers,
  all other config items need to fail if userspace tries to change them.
  Maybe this should be a top-level "enable" property.

- Every time enable goes from 0 -> 1 we need to create a completely new
  vkms instance. The old one might still be around, waiting for the last
  few references to disappear.

- enable going from 1 -> 0 needs to be treated like a physical hotunplug,
  i.e. not drm_dev_unregister but drm_dev_unplug. We also need to annotate
  all the vkms code with drm_dev_enter/exit() as the kerneldoc of
  drm_dev_unplug explains.

- rmdir should be treated like enable going from 1 -> 0. Or maybe we
  should disable enable every going from 1 -> 0, would propably simplify
  everything.

- The initial instance created at module load also neeeds to be removable
  like this.

Once we have all this, then can we start to convert driver module options
over to configs and add cool features. But lots of infrastructure needed
first.

Also, we probably want some nasty testcases which races an rmdir in
configfs against userspace still doing ioctl calls against vkms. This is
ideal for validation the hotunplug infrastructure we have in drm.

An alternative is adding connector hotplugging. But I think before we do
that we need to have support for more crtc and more connectors as static
module options. So maybe not a good starting point for configfs.

The above text should probably be added to the vkms.rst todo item ...
-Daniel

> 
> 
> Rodrigo Siqueira (2):
>   drm/vkms: Add enable/disable functions per connector
>   drm/vkms: Introduce configfs for enabling/disabling connectors
> 
>  drivers/gpu/drm/vkms/Makefile         |   3 +-
>  drivers/gpu/drm/vkms/vkms_configfs.c  | 229 ++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_drv.c       |   6 +
>  drivers/gpu/drm/vkms/vkms_drv.h       |  17 ++
>  drivers/gpu/drm/vkms/vkms_output.c    |  84 ++++++----
>  drivers/gpu/drm/vkms/vkms_writeback.c |  31 +++-
>  6 files changed, 332 insertions(+), 38 deletions(-)
>  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
> 
> -- 
> 2.21.0
> 
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/2] drm/vkms: Introduce basic support for configfs

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 7735 bytes --]

On 07/10, Daniel Vetter wrote:
> On Mon, Jul 01, 2019 at 12:23:39AM -0300, Rodrigo Siqueira wrote:
> > This patchset introduces the support for configfs in vkms by adding a
> > primary structure for handling the vkms subsystem and exposing
> > connectors as a use case.  This series allows enabling/disabling virtual
> > and writeback connectors on the fly. The first patch of this series
> > reworks the initialization and cleanup code of each type of connector,
> > with this change, the second patch adds the configfs support for vkms.
> > It is important to highlight that this patchset depends on
> > https://patchwork.freedesktop.org/series/61738/.
> > 
> > After applying this series, the user can utilize these features with the
> > following steps:
> > 
> > 1. Load vkms without parameter
> > 
> >   modprobe vkms
> > 
> > 2. Mount a configfs filesystem
> > 
> >   mount -t configfs none /mnt/
> > 
> > After that, the vkms subsystem will look like this:
> > 
> > vkms/
> >  |__connectors
> >     |__Virtual
> >         |__ enable
> > 
> > The connectors directories have information related to connectors, and
> > as can be seen, the virtual connector is enabled by default. Inside a
> > connector directory (e.g., Virtual) has an attribute named ‘enable’
> > which is used to enable and disable the target connector. For example,
> > the Virtual connector has the enable attribute set to 1. If the user
> > wants to enable the writeback connector it is required to use the mkdir
> > command, as follows:
> > 
> >   cd /mnt/vkms/connectors
> >   mkdir Writeback
> > 
> > After the above command, the writeback connector will be enabled, and
> > the user could see the following tree:
> > 
> > vkms/
> >  |__connectors
> >     |__Virtual
> >     |   |__ enable
> >     |__Writeback
> >         |__ enable
> > 
> > If the user wants to remove the writeback connector, it is required to
> > use the command rmdir, for example
> > 
> >   rmdir Writeback
> > 
> > Another way to enable and disable a connector it is by using the enable
> > attribute, for example, we can disable the Virtual connector with:
> > 
> >   echo 0 > /mnt/vkms/connectors/Virtual/enable
> > 
> > And enable it again with:
> > 
> >   echo 1 > /mnt/vkms/connectors/Virtual/enable
> > 
> > It is important to highlight that configfs 'obey' the parameters used
> > during the vkms load and does not allow users to remove a connector
> > directory if it was load via module parameter. For example:
> > 
> >   modprobe vkms enable_writeback=1
> > 
> > vkms/
> >  |__connectors
> >     |__Virtual
> >     |   |__ enable
> >     |__Writeback
> >         |__ enable
> > 
> > If the user tries to remove the Writeback connector with “rmdir
> > Writeback”, the operation will be not permitted because the Writeback
> > connector was loaded with the modules. However, the user may disable the
> > writeback connector with:
> > 
> >   echo 0 > /mnt/vkms/connectors/Writeback/enable

Thanks for detail this issue, I just have some few questions inline.
 
> I guess I should have put a warning into that task that step one is
> designing the interface. Here's the fundamental thoughts:
> 
> - The _only_ thing we can hotplug after drm_dev_register() is a
>   drm_connector. That's an interesting use-case, but atm not really
>   supported by the vkms codebase. So we can't just enable/disable
>   writeback like this. We also can't change _anything_ else in the drm
>   driver like this.

In the first patch of this series, I tried to decouple enable/disable
for virtual and writeback connectors; I tried to take advantage of
drm_connector_[register/unregister] in each connector. Can we use the
first patch or it doesn't make sense?

I did not understand why writeback connectors should not be registered
and unregister by calling drm_connector_[register/unregister], is it a
writeback or vkms limitation? Could you detail why we cannot change
connectors as I did?

Additionally, below you said "enable going from 1 -> 0, needs to be
treated like a physical hotunplug", do you mean that we first have to
add support for drm_dev_plug and drm_dev_unplug in vkms?
 
> - The other bit we want is support multiple vkms instances, to simulate
>   multi-gpus and fun stuff like that.

Do you mean something like this:

configfs/vkms/instance1
|_enable_device 
|_more_stuff
configfs/vkms/instance2
|_enable_device
|_more_stuff
configfs/vkms/instanceN
|_enable_device
|_more_stuff

Will each instance work like a totally independent device? What is the
main benefit of this? I can think about some use case for testing
prime, but I cannot see other things.
 
> - Therefore vkms configs should be at the drm_device level, so a
>   directory under configfs/vkms/ represents an entire device.
> 
> - We need a special config item to control
>   drm_dev_register/drm_dev_unregister. While a drm_device is registers,
>   all other config items need to fail if userspace tries to change them.
>   Maybe this should be a top-level "enable" property.
> 
> - Every time enable goes from 0 -> 1 we need to create a completely new
>   vkms instance. The old one might still be around, waiting for the last
>   few references to disappear.
> 
> - enable going from 1 -> 0 needs to be treated like a physical hotunplug,
>   i.e. not drm_dev_unregister but drm_dev_unplug. We also need to annotate
>   all the vkms code with drm_dev_enter/exit() as the kerneldoc of
>   drm_dev_unplug explains.
> 
> - rmdir should be treated like enable going from 1 -> 0. Or maybe we
>   should disable enable every going from 1 -> 0, would propably simplify
>   everything.
> 
> - The initial instance created at module load also neeeds to be removable
>   like this.
> 
> Once we have all this, then can we start to convert driver module options
> over to configs and add cool features. But lots of infrastructure needed
> first.
> 
> Also, we probably want some nasty testcases which races an rmdir in
> configfs against userspace still doing ioctl calls against vkms. This is
> ideal for validation the hotunplug infrastructure we have in drm.
> 
> An alternative is adding connector hotplugging. But I think before we do
> that we need to have support for more crtc and more connectors as static
> module options. So maybe not a good starting point for configfs.

So, probably the first set of tasks should be:

1. Enable multiple CRTC via module parameters. For example:
  modprobe vkms crtcs=13
2. Enable multiple connectors via module parameters. For example:
  modprobe vkms crtcs=3 connector=3 // 3 virtual connectors per crtc

Thanks again,
 
> The above text should probably be added to the vkms.rst todo item ...
> -Daniel
> 
> > 
> > 
> > Rodrigo Siqueira (2):
> >   drm/vkms: Add enable/disable functions per connector
> >   drm/vkms: Introduce configfs for enabling/disabling connectors
> > 
> >  drivers/gpu/drm/vkms/Makefile         |   3 +-
> >  drivers/gpu/drm/vkms/vkms_configfs.c  | 229 ++++++++++++++++++++++++++
> >  drivers/gpu/drm/vkms/vkms_drv.c       |   6 +
> >  drivers/gpu/drm/vkms/vkms_drv.h       |  17 ++
> >  drivers/gpu/drm/vkms/vkms_output.c    |  84 ++++++----
> >  drivers/gpu/drm/vkms/vkms_writeback.c |  31 +++-
> >  6 files changed, 332 insertions(+), 38 deletions(-)
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
> > 
> > -- 
> > 2.21.0
> > 
> > 
> > -- 
> > Rodrigo Siqueira
> > https://siqueira.tech
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Rodrigo Siqueira
https://siqueira.tech

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] drm/vkms: Introduce basic support for configfs

[Vasilev, Oleg]

[-- Attachment #1: Type: text/plain, Size: 9465 bytes --]

On Fri, 2019-07-12 at 00:07 -0300, Rodrigo Siqueira wrote:
> On 07/10, Daniel Vetter wrote:
> > On Mon, Jul 01, 2019 at 12:23:39AM -0300, Rodrigo Siqueira wrote:
> > > This patchset introduces the support for configfs in vkms by
> > > adding a
> > > primary structure for handling the vkms subsystem and exposing
> > > connectors as a use case.  This series allows enabling/disabling
> > > virtual
> > > and writeback connectors on the fly. The first patch of this
> > > series
> > > reworks the initialization and cleanup code of each type of
> > > connector,
> > > with this change, the second patch adds the configfs support for
> > > vkms.
> > > It is important to highlight that this patchset depends on
> > > https://patchwork.freedesktop.org/series/61738/.
> > > 
> > > After applying this series, the user can utilize these features
> > > with the
> > > following steps:
> > > 
> > > 1. Load vkms without parameter
> > > 
> > >   modprobe vkms
> > > 
> > > 2. Mount a configfs filesystem
> > > 
> > >   mount -t configfs none /mnt/
> > > 
> > > After that, the vkms subsystem will look like this:
> > > 
> > > vkms/
> > >  |__connectors
> > >     |__Virtual
> > >         |__ enable
> > > 
> > > The connectors directories have information related to
> > > connectors, and
> > > as can be seen, the virtual connector is enabled by default.
> > > Inside a
> > > connector directory (e.g., Virtual) has an attribute named
> > > ‘enable’
> > > which is used to enable and disable the target connector. For
> > > example,
> > > the Virtual connector has the enable attribute set to 1. If the
> > > user
> > > wants to enable the writeback connector it is required to use the
> > > mkdir
> > > command, as follows:
> > > 
> > >   cd /mnt/vkms/connectors
> > >   mkdir Writeback
> > > 
> > > After the above command, the writeback connector will be enabled,
> > > and
> > > the user could see the following tree:
> > > 
> > > vkms/
> > >  |__connectors
> > >     |__Virtual
> > >     |   |__ enable
> > >     |__Writeback
> > >         |__ enable
> > > 
> > > If the user wants to remove the writeback connector, it is
> > > required to
> > > use the command rmdir, for example
> > > 
> > >   rmdir Writeback
> > > 
> > > Another way to enable and disable a connector it is by using the
> > > enable
> > > attribute, for example, we can disable the Virtual connector
> > > with:
> > > 
> > >   echo 0 > /mnt/vkms/connectors/Virtual/enable
> > > 
> > > And enable it again with:
> > > 
> > >   echo 1 > /mnt/vkms/connectors/Virtual/enable
> > > 
> > > It is important to highlight that configfs 'obey' the parameters
> > > used
> > > during the vkms load and does not allow users to remove a
> > > connector
> > > directory if it was load via module parameter. For example:
> > > 
> > >   modprobe vkms enable_writeback=1
> > > 
> > > vkms/
> > >  |__connectors
> > >     |__Virtual
> > >     |   |__ enable
> > >     |__Writeback
> > >         |__ enable
> > > 
> > > If the user tries to remove the Writeback connector with “rmdir
> > > Writeback”, the operation will be not permitted because the
> > > Writeback
> > > connector was loaded with the modules. However, the user may
> > > disable the
> > > writeback connector with:
> > > 
> > >   echo 0 > /mnt/vkms/connectors/Writeback/enable
> 
> Thanks for detail this issue, I just have some few questions inline.
>  
> > I guess I should have put a warning into that task that step one is
> > designing the interface. Here's the fundamental thoughts:
> > 
> > - The _only_ thing we can hotplug after drm_dev_register() is a
> >   drm_connector. That's an interesting use-case, but atm not really
> >   supported by the vkms codebase. So we can't just enable/disable
> >   writeback like this. We also can't change _anything_ else in the
> > drm
> >   driver like this.
> 
> In the first patch of this series, I tried to decouple enable/disable
> for virtual and writeback connectors; I tried to take advantage of
> drm_connector_[register/unregister] in each connector. Can we use the
> first patch or it doesn't make sense?
> 
> I did not understand why writeback connectors should not be
> registered
> and unregister by calling drm_connector_[register/unregister], is it
> a
> writeback or vkms limitation? Could you detail why we cannot change
> connectors as I did?

Hi,

I guess, some more stuff should happen during the hotplug, like
drm_kms_helper_hotplug_event()?

> 
> Additionally, below you said "enable going from 1 -> 0, needs to be
> treated like a physical hotunplug", do you mean that we first have to
> add support for drm_dev_plug and drm_dev_unplug in vkms?
>  
> > - The other bit we want is support multiple vkms instances, to
> > simulate
> >   multi-gpus and fun stuff like that.
> 
> Do you mean something like this:
> 
> configfs/vkms/instance1
> > _enable_device 
> > _more_stuff
> configfs/vkms/instance2
> > _enable_device
> > _more_stuff
> configfs/vkms/instanceN
> > _enable_device
> > _more_stuff

I think it would be a good idea. This way the creation of new device
could look like this:

mkdir -p instanceN/connector/virtual0
echo "virtual" > instanceN/connector/virtual0/type
echo 1 > instanceN/connector/virtual0/enable
mkdir -p instanceN/crtc/crtc0
...
echo 1 > instanceN/enable

Once the last command is executed, the whole instanceN/ becomes
immutable, except for
 - instanceN/enable, so we can later disable it
 - instanceN/connector, where we can create new connectors, it would be
treated as a hotplug.

> 
> Will each instance work like a totally independent device? What is
> the
> main benefit of this? I can think about some use case for testing
> prime, but I cannot see other things.

We can test that userspace always select the right device to display. 

>  
> > - Therefore vkms configs should be at the drm_device level, so a
> >   directory under configfs/vkms/ represents an entire device.
> > 
> > - We need a special config item to control
> >   drm_dev_register/drm_dev_unregister. While a drm_device is
> > registers,
> >   all other config items need to fail if userspace tries to change
> > them.
> >   Maybe this should be a top-level "enable" property.
> > 
> > - Every time enable goes from 0 -> 1 we need to create a completely
> > new
> >   vkms instance. The old one might still be around, waiting for the
> > last
> >   few references to disappear.
> > 
> > - enable going from 1 -> 0 needs to be treated like a physical
> > hotunplug,
> >   i.e. not drm_dev_unregister but drm_dev_unplug. We also need to
> > annotate
> >   all the vkms code with drm_dev_enter/exit() as the kerneldoc of
> >   drm_dev_unplug explains.
> > 
> > - rmdir should be treated like enable going from 1 -> 0. Or maybe
> > we
> >   should disable enable every going from 1 -> 0, would propably
> > simplify
> >   everything.
> > 
> > - The initial instance created at module load also neeeds to be
> > removable
> >   like this.
> > 
> > Once we have all this, then can we start to convert driver module
> > options
> > over to configs and add cool features. But lots of infrastructure
> > needed
> > first.
> > 
> > Also, we probably want some nasty testcases which races an rmdir in
> > configfs against userspace still doing ioctl calls against vkms.
> > This is
> > ideal for validation the hotunplug infrastructure we have in drm.
> > 
> > An alternative is adding connector hotplugging. But I think before
> > we do
> > that we need to have support for more crtc and more connectors as
> > static
> > module options. So maybe not a good starting point for configfs.
> 
> So, probably the first set of tasks should be:
> 
> 1. Enable multiple CRTC via module parameters. For example:
>   modprobe vkms crtcs=13
> 2. Enable multiple connectors via module parameters. For example:
>   modprobe vkms crtcs=3 connector=3 // 3 virtual connectors per crtc

But do we want to have those parameters as module options, if we then
plan to replace those with configfs?  

> 
> Thanks again,
>  
> > The above text should probably be added to the vkms.rst todo item
> > ...
> > -Daniel
> > 
> > > 
> > > Rodrigo Siqueira (2):
> > >   drm/vkms: Add enable/disable functions per connector
> > >   drm/vkms: Introduce configfs for enabling/disabling connectors
> > > 
> > >  drivers/gpu/drm/vkms/Makefile         |   3 +-
> > >  drivers/gpu/drm/vkms/vkms_configfs.c  | 229
> > > ++++++++++++++++++++++++++
> > >  drivers/gpu/drm/vkms/vkms_drv.c       |   6 +
> > >  drivers/gpu/drm/vkms/vkms_drv.h       |  17 ++
> > >  drivers/gpu/drm/vkms/vkms_output.c    |  84 ++++++----
> > >  drivers/gpu/drm/vkms/vkms_writeback.c |  31 +++-
> > >  6 files changed, 332 insertions(+), 38 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
> > > 
> > > -- 
> > > 2.21.0
> > > 
> > > 
> > > -- 
> > > Rodrigo Siqueira
> > > https://siqueira.tech
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Oleg Vasilev <oleg.vasilev@intel.com>
Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3261 bytes --]

Re: [PATCH 0/2] drm/vkms: Introduce basic support for configfs

[Daniel Vetter]

On Mon, Jul 15, 2019 at 11:09:24AM +0000, Vasilev, Oleg wrote:
> On Fri, 2019-07-12 at 00:07 -0300, Rodrigo Siqueira wrote:
> > On 07/10, Daniel Vetter wrote:
> > > On Mon, Jul 01, 2019 at 12:23:39AM -0300, Rodrigo Siqueira wrote:
> > > > This patchset introduces the support for configfs in vkms by
> > > > adding a
> > > > primary structure for handling the vkms subsystem and exposing
> > > > connectors as a use case.  This series allows enabling/disabling
> > > > virtual
> > > > and writeback connectors on the fly. The first patch of this
> > > > series
> > > > reworks the initialization and cleanup code of each type of
> > > > connector,
> > > > with this change, the second patch adds the configfs support for
> > > > vkms.
> > > > It is important to highlight that this patchset depends on
> > > > https://patchwork.freedesktop.org/series/61738/.
> > > > 
> > > > After applying this series, the user can utilize these features
> > > > with the
> > > > following steps:
> > > > 
> > > > 1. Load vkms without parameter
> > > > 
> > > >   modprobe vkms
> > > > 
> > > > 2. Mount a configfs filesystem
> > > > 
> > > >   mount -t configfs none /mnt/
> > > > 
> > > > After that, the vkms subsystem will look like this:
> > > > 
> > > > vkms/
> > > >  |__connectors
> > > >     |__Virtual
> > > >         |__ enable
> > > > 
> > > > The connectors directories have information related to
> > > > connectors, and
> > > > as can be seen, the virtual connector is enabled by default.
> > > > Inside a
> > > > connector directory (e.g., Virtual) has an attribute named
> > > > ‘enable’
> > > > which is used to enable and disable the target connector. For
> > > > example,
> > > > the Virtual connector has the enable attribute set to 1. If the
> > > > user
> > > > wants to enable the writeback connector it is required to use the
> > > > mkdir
> > > > command, as follows:
> > > > 
> > > >   cd /mnt/vkms/connectors
> > > >   mkdir Writeback
> > > > 
> > > > After the above command, the writeback connector will be enabled,
> > > > and
> > > > the user could see the following tree:
> > > > 
> > > > vkms/
> > > >  |__connectors
> > > >     |__Virtual
> > > >     |   |__ enable
> > > >     |__Writeback
> > > >         |__ enable
> > > > 
> > > > If the user wants to remove the writeback connector, it is
> > > > required to
> > > > use the command rmdir, for example
> > > > 
> > > >   rmdir Writeback
> > > > 
> > > > Another way to enable and disable a connector it is by using the
> > > > enable
> > > > attribute, for example, we can disable the Virtual connector
> > > > with:
> > > > 
> > > >   echo 0 > /mnt/vkms/connectors/Virtual/enable
> > > > 
> > > > And enable it again with:
> > > > 
> > > >   echo 1 > /mnt/vkms/connectors/Virtual/enable
> > > > 
> > > > It is important to highlight that configfs 'obey' the parameters
> > > > used
> > > > during the vkms load and does not allow users to remove a
> > > > connector
> > > > directory if it was load via module parameter. For example:
> > > > 
> > > >   modprobe vkms enable_writeback=1
> > > > 
> > > > vkms/
> > > >  |__connectors
> > > >     |__Virtual
> > > >     |   |__ enable
> > > >     |__Writeback
> > > >         |__ enable
> > > > 
> > > > If the user tries to remove the Writeback connector with “rmdir
> > > > Writeback”, the operation will be not permitted because the
> > > > Writeback
> > > > connector was loaded with the modules. However, the user may
> > > > disable the
> > > > writeback connector with:
> > > > 
> > > >   echo 0 > /mnt/vkms/connectors/Writeback/enable
> > 
> > Thanks for detail this issue, I just have some few questions inline.
> >  
> > > I guess I should have put a warning into that task that step one is
> > > designing the interface. Here's the fundamental thoughts:
> > > 
> > > - The _only_ thing we can hotplug after drm_dev_register() is a
> > >   drm_connector. That's an interesting use-case, but atm not really
> > >   supported by the vkms codebase. So we can't just enable/disable
> > >   writeback like this. We also can't change _anything_ else in the
> > > drm
> > >   driver like this.
> > 
> > In the first patch of this series, I tried to decouple enable/disable
> > for virtual and writeback connectors; I tried to take advantage of
> > drm_connector_[register/unregister] in each connector. Can we use the
> > first patch or it doesn't make sense?

No, because you also add the drm_encoder. That cannot be hotadded/removed
right now. So you'd need to add a drm_encoder preemptively for all
writeback connectors you might need, which doesn't make much sense.

> > I did not understand why writeback connectors should not be
> > registered
> > and unregister by calling drm_connector_[register/unregister], is it
> > a
> > writeback or vkms limitation? Could you detail why we cannot change
> > connectors as I did?
> 
> Hi,
> 
> I guess, some more stuff should happen during the hotplug, like
> drm_kms_helper_hotplug_event()?

writeback is generally a SoC feature. I don't think anyone expects that
you can hotplug a writeback connector.
> 
> > 
> > Additionally, below you said "enable going from 1 -> 0, needs to be
> > treated like a physical hotunplug", do you mean that we first have to
> > add support for drm_dev_plug and drm_dev_unplug in vkms?
> >  
> > > - The other bit we want is support multiple vkms instances, to
> > > simulate
> > >   multi-gpus and fun stuff like that.
> > 
> > Do you mean something like this:
> > 
> > configfs/vkms/instance1
> > > _enable_device 
> > > _more_stuff
> > configfs/vkms/instance2
> > > _enable_device
> > > _more_stuff
> > configfs/vkms/instanceN
> > > _enable_device
> > > _more_stuff
> 
> I think it would be a good idea. This way the creation of new device
> could look like this:
> 
> mkdir -p instanceN/connector/virtual0
> echo "virtual" > instanceN/connector/virtual0/type

virtual should be a top-level property. I'm not aware of any real driver
where vblank works on some connector/crtc, but not on another one.

> echo 1 > instanceN/connector/virtual0/enable

For the initial connectors those don't need separate enable properties,
they get enabled with the global enable knob.

> mkdir -p instanceN/crtc/crtc0
> ...
> echo 1 > instanceN/enable
> 
> Once the last command is executed, the whole instanceN/ becomes
> immutable, except for
>  - instanceN/enable, so we can later disable it
>  - instanceN/connector, where we can create new connectors, it would be
> treated as a hotplug.

For hotplugged connectors we need a separate enable knob. But we're still
_very_ far away from making that possible I think.

> > Will each instance work like a totally independent device? What is
> > the
> > main benefit of this? I can think about some use case for testing
> > prime, but I cannot see other things.
> 
> We can test that userspace always select the right device to display. 

Also hotunplug behaviour. We can take out an entire drm_device, and make
sure userspace and the kernel copes correctly. This is _really_ hard to
get right on both sides.

And it's a real thing with stuff like usb display link.

> > > - Therefore vkms configs should be at the drm_device level, so a
> > >   directory under configfs/vkms/ represents an entire device.
> > > 
> > > - We need a special config item to control
> > >   drm_dev_register/drm_dev_unregister. While a drm_device is
> > > registers,
> > >   all other config items need to fail if userspace tries to change
> > > them.
> > >   Maybe this should be a top-level "enable" property.
> > > 
> > > - Every time enable goes from 0 -> 1 we need to create a completely
> > > new
> > >   vkms instance. The old one might still be around, waiting for the
> > > last
> > >   few references to disappear.
> > > 
> > > - enable going from 1 -> 0 needs to be treated like a physical
> > > hotunplug,
> > >   i.e. not drm_dev_unregister but drm_dev_unplug. We also need to
> > > annotate
> > >   all the vkms code with drm_dev_enter/exit() as the kerneldoc of
> > >   drm_dev_unplug explains.
> > > 
> > > - rmdir should be treated like enable going from 1 -> 0. Or maybe
> > > we
> > >   should disable enable every going from 1 -> 0, would propably
> > > simplify
> > >   everything.
> > > 
> > > - The initial instance created at module load also neeeds to be
> > > removable
> > >   like this.
> > > 
> > > Once we have all this, then can we start to convert driver module
> > > options
> > > over to configs and add cool features. But lots of infrastructure
> > > needed
> > > first.
> > > 
> > > Also, we probably want some nasty testcases which races an rmdir in
> > > configfs against userspace still doing ioctl calls against vkms.
> > > This is
> > > ideal for validation the hotunplug infrastructure we have in drm.
> > > 
> > > An alternative is adding connector hotplugging. But I think before
> > > we do
> > > that we need to have support for more crtc and more connectors as
> > > static
> > > module options. So maybe not a good starting point for configfs.
> > 
> > So, probably the first set of tasks should be:
> > 
> > 1. Enable multiple CRTC via module parameters. For example:
> >   modprobe vkms crtcs=13
> > 2. Enable multiple connectors via module parameters. For example:
> >   modprobe vkms crtcs=3 connector=3 // 3 virtual connectors per crtc
> 
> But do we want to have those parameters as module options, if we then
> plan to replace those with configfs?  

Imo we should phase out module options, or at least not add more. Then you
can just have a small script which sets up the vkms device you want, and
then run the tests. I'd go further even, and say we should have
default_device=0 knob to start out with nothing when you load the driver.
-Daniel

> 
> > 
> > Thanks again,
> >  
> > > The above text should probably be added to the vkms.rst todo item
> > > ...
> > > -Daniel
> > > 
> > > > 
> > > > Rodrigo Siqueira (2):
> > > >   drm/vkms: Add enable/disable functions per connector
> > > >   drm/vkms: Introduce configfs for enabling/disabling connectors
> > > > 
> > > >  drivers/gpu/drm/vkms/Makefile         |   3 +-
> > > >  drivers/gpu/drm/vkms/vkms_configfs.c  | 229
> > > > ++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/vkms/vkms_drv.c       |   6 +
> > > >  drivers/gpu/drm/vkms/vkms_drv.h       |  17 ++
> > > >  drivers/gpu/drm/vkms/vkms_output.c    |  84 ++++++----
> > > >  drivers/gpu/drm/vkms/vkms_writeback.c |  31 +++-
> > > >  6 files changed, 332 insertions(+), 38 deletions(-)
> > > >  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
> > > > 
> > > > -- 
> > > > 2.21.0
> > > > 
> > > > 
> > > > -- 
> > > > Rodrigo Siqueira
> > > > https://siqueira.tech
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> -- 
> Oleg Vasilev <oleg.vasilev@intel.com>
> Intel Corporation



> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch